> Date: Fri, 9 Aug 2024 22:15:44 +1000
> From: Nat Sloss <[email protected]>
>
> 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.
