Re: SCSI changes - PR58452 for review

On Sun, 4 Aug 2024 03:47:23 Taylor R Campbell wrote:

> > Biggest change is for ncr5380 which is to stop scsipi_done from being
> > reentrant in 5380sbc_done,  This helps with aborting transfers something
> > which is common with dse(4). which previously if nested to deep in
> > recursive calls to scsipi_done would cause a panic.
> 
> Why do we end up in recursion here?  Why does the driver need to
> decide when to break this recursion?  If drivers sometimes _skip_
> scsipi_run_queue, won't they sometimes wind up with queued xfers that
> never get issued to the hardware?
> 

My concern too...

> It seems to me that either
> 
> (a) scsipi_run_queue -> scsipi_adapter_request -> driver adapt_request
>     ought to arrange for scsipi_done to be invoked asynchronously, and
>     it's a bug for it to call scsipi_done synchronously; or
> 
> (b) if it's really important for driver adapt_request to synchronously
>     call scsipi_done, then that should _never_ synchronously re-enter
>     scsipi_run_queue but instead should _always_ defer to an
>     asynchronous trigger like a softint or something; or
> 
> (c) maybe, if it's really important for _both_ synchronous calls to
>     happen, then when scsipi_done/scsipi_run_queue detects it is being
>     recursively called, the inner call should detect this and do
>     nothing so it can unwind to the outer call to continue processing
>     the queue.

Where it panics is:
    625 			if (sc->sc_state != NCR_IDLE) {
    626 				panic("%s: polled request, abort failed",
    627 				    __func__);
    628 

in dev/ic/ncr5380.sbc

I believe this is because it gets nested in scspi_done from grabbing transfers 
from the queue that the state machine is not updated (IDLE is set from 
ncr5380_done which is not compete as it is processing xfers from the queue)

I dunno if it is possible to comment out the panic as the abort was issued 
(and hopefully handled).


Best regards,

Nat