Re: SCSI changes - PR58452 for review

[Taylor R Campbell <campbell+netbsd-tech-kern%mumble.net@localhost>]

> Date: Fri, 9 Aug 2024 22:15:44 +1000
> From: Nat Sloss <nathanialsloss%yahoo.com.au@localhost>
> 
> On Wed, 7 Aug 2024 10:21:28 Taylor R Campbell wrote:
> 
> > Instead of introducing scsipi_done_once, why not just freeze and thaw
> > the channel around scsipi_done?
> 
> I tried the following and it resulted in a slew of error messages from the 
> kernel resetting the scsi bus/ aborting.
> 
> +	scsipi_channel_freeze(&sc->sc_channel, 1);
>  
>   	/* Tell common SCSI code it is done. */
>   	scsipi_done(xs);
>    	sc->sc_state = NCR_IDLE;
> 
> 	/* Now ncr5380_sched() may be called again. */
> 
> +	scsipi_channel_thaw(&sc->sc_channel, 1);

Can you maybe do the freeze/thaw conditionally, only if the xfer is
being aborted?

+	if (xs is aborting)
+		scsipi_channel_freeze(&sc->sc_channel, 1);
...	scsipi_done(xs); ...
+	if (xs is aborting)
+		scsipi_channel_thaw(&sc->sc_channel, 1);

> I guess in both cases another request is sent to the controller in 
> scsipi_run_queue, called from scsipi_done which results in an inconsistient 
> state.

What inconsistent state is that?

> scspi_done_once, and thaw with an argument of 0 to kick the queue as posted 
> earlier works great (with a noticable improvent - most shell commands 
> responding/starting twice as fast as usual).

I'm just puzzled how this could make a difference, because the logic
is essentially:

	/* scsipi_channel_freeze(&sc->sc_channel, 1); */
	chan->chan_qfreeze++;

	/* scsipi_done(xs); */
	handle scsi xfer completion actions;
	if (chan->chan_qfreeze == 0) {	/* scsipi_run_queue */
		/* look for more work to do and do it */
	}
	/* sc->sc_state = NCR_IDLE; */

	/* scsipi_channel_freeze(&sc->sc_channel, 1); */
	if (--chan->chan_qfreeze == 0) {
		if (chan->chan_qfreeze == 0) {	/* scsipi_run_queue */
			/* look for more work to do and do it */
		}
	}

So unless there's other logic inside scsipi_done that depends on
chan->chan_qfreeze, it seems like the scsipi_done_once approach and
the freeze/scsipi_done/thaw approach should be equivalent.

But there's obviously something else going on here that I don't
understand -- and I find that concerning, because it means there's a
_much more interesting_ semantic difference between scsipi_done and
scsipi_done_once than I understand, and that makes me concerned
there's something incoherent about the scsi API that might affect lots
of drivers.