Message-ID: <d2d3c903-fb91-e218-9e0a-aeb2ff9e401a@opensource.wdc.com>
Date:   Wed, 5 Jan 2022 13:03:52 +0900
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.4.0
Subject: Re: [PATCH RFT] scsi: pm8001: Fix FW crash for maxcpus=1
Content-Language: en-US
To:     John Garry <john.garry@huawei.com>, jejb@linux.ibm.com,
        martin.petersen@oracle.com, jinpu.wang@cloud.ionos.com,
        Ajish.Koshy@microchip.com, Viswas.G@microchip.com
Cc:     linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org,
        vishakhavc@google.com, ipylypiv@google.com,
        Ruksar.devadi@microchip.com,
        Vasanthalakshmi.Tharmarajan@microchip.com
References: <1641320780-81620-1-git-send-email-john.garry@huawei.com>
From:   Damien Le Moal <damien.lemoal@opensource.wdc.com>
Organization: Western Digital Research
In-Reply-To: <1641320780-81620-1-git-send-email-john.garry@huawei.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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@huawei.com>
> 
> 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.

> +			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@opensource.wdc.com>
Tested-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>

-- 
Damien Le Moal
Western Digital Research