Re: [PATCH RFT] scsi: pm8001: Fix FW crash for maxcpus=1

[John Garry]

On 05/01/2022 04:03, Damien Le Moal wrote:
> On 1/5/22 03:26, John Garry wrote:
> > According to the comment in check_fw_ready() we should not check the
> > IOP1_READY field in register SCRATCH_PAD_1 for 8008 or 8009 controllers.
> >
> > However we check this very field in process_oq() for processing the highest
> > index interrupt vector. Change that function to not check IOP1_READY for
> > those mentioned controllers, but do check ILA_READY in both cases.
> >
> > The reason I assume that this was not hit earlier was because we always
> > allocated 64 MSI(X), and just did not pass the vector index check in
> > process_oq(), i.e.  the handler never ran for vector index 63.
> >
> > Signed-off-by: John Garry<john.garry@xxxxxxxxxx>
> >
> > diff --git a/drivers/scsi/pm8001/pm80xx_hwi.c b/drivers/scsi/pm8001/pm80xx_hwi.c
> > index 2101fc5761c3..77b8bb30615b 100644
> > --- a/drivers/scsi/pm8001/pm80xx_hwi.c
> > +++ b/drivers/scsi/pm8001/pm80xx_hwi.c
> > @@ -4162,9 +4162,16 @@ static int process_oq(struct pm8001_hba_info *pm8001_ha, u8 vec)
> >   	u32 regval;
> >   
> >   	if (vec == (pm8001_ha->max_q_num - 1)) {
> > +		u32 mipsall_ready;
> > +
> > +		if ((pm8001_ha->chip_id == chip_8008) ||
> > +		    (pm8001_ha->chip_id == chip_8009))
> nit: no need for the inner brackets here.

ok, I can fix that.

But I would also like opinion from microchip guys/maintainer on why this 
code is here at all. Seems strange in the way we check in this register 
in the interrupt handler for only a specific vector and, also, why we 
check at all in an interrupt handler.
> > +			mipsall_ready = SCRATCH_PAD_MIPSALL_READY_8PORT;
> > +		else
> > +			mipsall_ready = SCRATCH_PAD_MIPSALL_READY_16PORT;
> > +
> >   		regval = pm8001_cr32(pm8001_ha, 0, MSGU_SCRATCH_PAD_1);
> > -		if ((regval & SCRATCH_PAD_MIPSALL_READY) !=
> > -					SCRATCH_PAD_MIPSALL_READY) {
> > +		if ((regval & mipsall_ready) != mipsall_ready) {
> >   			pm8001_ha->controller_fatal_error = true;
> >   			pm8001_dbg(pm8001_ha, FAIL,
> >   				   "Firmware Fatal error! Regval:0x%x\n",
> > diff --git a/drivers/scsi/pm8001/pm80xx_hwi.h b/drivers/scsi/pm8001/pm80xx_hwi.h
> > index c7e5d93bea92..c41ed039c92a 100644
> > --- a/drivers/scsi/pm8001/pm80xx_hwi.h
> > +++ b/drivers/scsi/pm8001/pm80xx_hwi.h
> > @@ -1405,8 +1405,12 @@ typedef struct SASProtocolTimerConfig SASProtocolTimerConfig_t;
> >   #define SCRATCH_PAD_BOOT_LOAD_SUCCESS	0x0
> >   #define SCRATCH_PAD_IOP0_READY		0xC00
> >   #define SCRATCH_PAD_IOP1_READY		0x3000
> > -#define SCRATCH_PAD_MIPSALL_READY	(SCRATCH_PAD_IOP1_READY | \
> > +#define SCRATCH_PAD_MIPSALL_READY_16PORT	(SCRATCH_PAD_IOP1_READY | \
> >   					SCRATCH_PAD_IOP0_READY | \
> > +					SCRATCH_PAD_ILA_READY | \
> > +					SCRATCH_PAD_RAAE_READY)
> > +#define SCRATCH_PAD_MIPSALL_READY_8PORT	(SCRATCH_PAD_IOP0_READY | \
> > +					SCRATCH_PAD_ILA_READY | \
> >   					SCRATCH_PAD_RAAE_READY)
> >   
> >   /* boot loader state */
> Otherwise, looks OK to me.
> I tested with and without max_cpus=1 with a ATTO Technology, Inc.
> ExpressSAS 12Gb/s SAS/SATA HBA (rev 06) adapter and everything is OK.
> That adapter uses chip_8072 though, not 8008 or 8009.
>
> Feel free to add:
>
> Reviewed-by: Damien Le Moal<damien.lemoal@xxxxxxxxxxxxxxxxxx>
> Tested-by: Damien Le Moal<damien.lemoal@xxxxxxxxxxxxxxxxxx>

Thanks!

john