On Wed, 2020-03-25 at 17:28 +0100, Martin Wilck wrote:
> On Tue, 2020-03-24 at 16:51 -0700, Arun Easi wrote:
> > On Wed, 5 Feb 2020, 1:44pm, mwilck@xxxxxxxx wrote:
> >
> > > From: Martin Wilck <mwilck@xxxxxxxx>
> > >
> > > Since commit 45235022da99 ("scsi: qla2xxx: Fix driver unload by
> > > shutting down chip"),
> > > it is possible that FC commands are scheduled after the adapter
> > > firmware
> > > has been shut down. IO sent to the firmware in this situation
> > > hangs
> > > indefinitely. Avoid this for the LOGO code path that is typically
> > > taken
> > > when adapters are shut down.
> > >
> > > Fixes: 45235022da99 ("scsi: qla2xxx: Fix driver unload by
> > > shutting
> > > down chip")
> > > Signed-off-by: Martin Wilck <mwilck@xxxxxxxx>
> > > Reviewed-by: Roman Bolshakov <r.bolshakov@xxxxxxxxx>
> > > ---
> > > drivers/scsi/qla2xxx/qla_mbx.c | 3 +++
> > > drivers/scsi/qla2xxx/qla_os.c | 3 +++
> > > 2 files changed, 6 insertions(+)
> > >
> > > [...]
> > > --- a/drivers/scsi/qla2xxx/qla_os.c
> > > +++ b/drivers/scsi/qla2xxx/qla_os.c
> > > @@ -4878,6 +4878,9 @@ qla2x00_post_work(struct scsi_qla_host
> > > *vha,
> > > struct qla_work_evt *e)
> > > unsigned long flags;
> > > bool q = false;
> > >
> > > + if (!vha->hw->flags.fw_started)
> > > + return QLA_FUNCTION_FAILED;
> > > +
> >
> > I'd probably not do it here; rather in qla2x00_get_sp()
> > /qla2x00_start_sp()/ qla2xxx_get_qpair_sp() time. Not all works are
> > for
> > posting to firmware (QLA_EVT_IDC_ACK, QLA_EVT_UNMAP etc.).
>
> qla81xx_idc_ack() calls qla2x00_mailbox_command(), which should be
> avoided IMO. But I'll review the various cases and re-post the patch.
Thinking about it once more, the approach is racy.
qla2x00_try_stop_firmware() can be called any time, and it sets
fw_started = 0 *after* actually stopping the firmware. Even if we check
fw_started, the firwmare might be stopped between our check and our
actual mailbox / FW register access, and we'd end up hanging.
At least the check should be as close as possible to the actual FW
access, e.g. in qla2x00_mailbox_command(), or before writing to the
request queue registers in qla2x00_start_sp() etc.
Perhaps the (!fw_started) condition should be treated like
ABORT_ISP_ACTIVE in qla2x00_mailbox_command, i.e. execute only if
is_rom_cmd() returns true?
I can re-post, but I feel this should really be done by someone who
knows exactly how the firmware operates, IOW Marvell staff.
Regards
Martin
