Re: CVS commit: src/sys/dev/scsipi

[Taylor R Campbell <riastradh%NetBSD.org@localhost>]

> Module Name:    src
> Committed By:   nat
> Date:           Mon Oct 28 14:36:43 UTC 2024
> 
> Modified Files:
>         src/sys/dev/ic: ncr5380sbc.c
>         src/sys/dev/scsipi: scsipi_base.c scsipiconf.h
> 
> Log Message:
> Introduce scsipi_done_once.
> 
> This allows for transfers to be sucessfully aborted on the ncr5380sbc(4).
> 
> This may be usefull in future for other scsi controllers.

I'm confused, didn't we conclude this change has to be unnecessary
months ago?

It is very weird that such a change to the fundamental control flow of
scsi transfers is needed for one particular driver, which makes me
strongly suspect something is wrong with that driver in particular --
or there's something wrong with the scsi subsystem's control flow
itself, which is _really important_ to understand before we flail
around with hacks in the driver-independent scsi subsystem, because
whatever the issue is might affect all drivers.

>  	/* Tell common SCSI code it is done. */
> -	scsipi_done(xs);
> +	scsipi_done_once(xs);
>  
>  	sc->sc_state = NCR_IDLE;
>  	/* Now ncr5380_sched() may be called again. */
> +
> +	/* Check the queue. */
> +	scsipi_channel_thaw(&sc->sc_channel, 0);

Why isn't it enough to do this, with or without the aborting
conditional?

+	const bool aborting = sc->sc_state & NCR_ABORTING;
+
 	/* Tell common SCSI code it is done. */
+	if (aborting)
+		scsipi_channel_freeze(&sc->sc_channel, 1);
 	scsipi_done(xs);
 
 	sc->sc_state = NCR_IDLE;
 	/* Now ncr5380_sched() may be called again. */
+
+	/* Check the queue if we were aborting. */
+	if (aborting)
+		scsipi_channel_thaw(&sc->sc_channel, 1);

The logic in scsipi_done now is essentially:

	scsipi_done_once(...);
	for (;;)	/* scspi_run_queue */
		mutex_enter(chan_mtx(chan));
		if (chan->chan_qfreeze != 0) {
			mutex_exit(chan_mtx(chan));
			break;
		}
		...
	}

And when scsipi_channel_thaw brings the freezecnt down to zero, it
does scsipi_run_queue.

So freezing the channel should have the same effect as
scsipi_done_once, and then thawing the channel should have the same
effect as scsipi_run_queue -- unless there is something else
interesting going on like a recursion of scsipi_done into itself in a
surprising place.