Re: [PATCH 2/4] qla2xxx: Simplify the code for aborting SCSI commands

[Martin Wilck <mwilck@suse.de>]

Hi Bart,

On Tue, 2019-12-10 at 15:45 -0500, Bart Van Assche wrote:
> On 12/10/19 3:29 PM, Martin Wilck wrote:
> > On Tue, 2019-12-10 at 12:57 -0500, Bart Van Assche wrote:
> > > On 12/10/19 6:47 AM, Martin Wilck wrote:
> > > > blk_mq_request_started() returns true for requests in
> > > > MQ_RQ_COMPLETE
> > > > state. Is this really an equivalent condition?
> > > > 
> > > > That said, the condition in the current code is sort of
> > > > strange, as
> > > > it's equivalent to !(sp->completed && sp->aborted). I'm
> > > > wondering
> > > > what
> > > > it means if a command is both completed and aborted. Naïvely
> > > > thinking
> > > > (and inferring from the current code) this condition could
> > > > never be
> > > > met, and thus its negation would hold for every command.
> > > > Perhaps
> > > > Quinn
> > > > meant "!(sp->completed || sp->aborted)" ?
> > > 
> > > Hi Martin,
> > > 
> > > The only caller of qla2x00_abort_srb() is
> > > qla2x00_abort_all_cmds().
> > > That
> > > function should only be called after completion interrupts have
> > > been
> > > disabled. In other words, I don't think that we have to worry
> > > about
> > > blk_mq_request_started() encountering the MQ_RQ_COMPLETE state.
> > > No
> > > request should have that state when qla2x00_abort_all_cmds() is
> > > called.
> > 
> > I thought avoiding a race between completion and abort was the
> > whole
> > point of f45bca8c5052 ("scsi: qla2xxx: Fix double scsi_done for
> > abort
> > path"), which introduced the code that you're now changing. But I
> > must
> > be overlooking something then, as Himanshu has acked this.
> 
> Hi Martin,
> 
> My understanding of commit f45bca8c5052 ("scsi: qla2xxx: Fix double
> scsi_done for abort path") is as follows:
> * A long time ago a scsi_done() call was introduced in
> qla2xxx_eh_abort(). Maybe this commit introduced that call:
> 083a469db4ec
> ("[SCSI] qla2xxx: Correct use-after-free oops seen during EH-
> abort.").
> * Calling scsi_done() from qla2xxx_eh_abort() is only fine if the
> firmware does not report a completion after having processed an abort
> request. My conclusion from commit f45bca8c5052 is that the firmware
> does report a completion after having processed an abort request.
> Hence
> the removal of that scsi_done call from qla2xxx_eh_abort().

I think it depends on the scenario in which commands are aborted.
For "regular" aborts, you're certainly right. But in certain scenarios,
e.g. controller removal or module unloading, the current driver shuts
down the firmware early in the shutdown process, and once the firmware
is stopped, I suppose it will not report completions any more.

Whether it was a good idea in the first place to shut down the FW so
early is a different issue, which I tried to adress with the 2 patches
I submitted on Nov. 29th.

Regards,
Martin