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 <[email protected]>
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))
+ 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 */
--
2.26.2
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 <[email protected]>
>
> 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 <[email protected]>
Tested-by: Damien Le Moal <[email protected]>
--
Damien Le Moal
Western Digital Research
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<[email protected]>
>>
>> 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<[email protected]>
> Tested-by: Damien Le Moal<[email protected]>
Thanks!
john
> 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<[email protected]>
> >>
> >> 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.
Here is my initial understanding so far based on the code
and data sheet
1. Controller has the capability to communicate
to the host about fatal error condition via configured
interrupt vector MSI/MSI-X.
2. This capability is achieved by setting two fields
a. Enable Controller Fatal error notification
Dowrd 0x1C, Bit[0].
1 - Enable; 0 - Disable
Code: pm8001_ha->main_cfg_tbl.pm80xx_tbl.
fatal_err_interrupt = 0x01;
b. Fatal Error Interrupt Vector Dword 0x1C, bit[15:8]
This parameter configures which interrupt vector
is used to notify the host of the fatal error.
Code: /* Update Fatal error interrupt vector */
pm8001_ha->main_cfg_tbl.pm80xx_tbl.
fatal_err_interrupt |=
((pm8001_ha->max_q_num - 1) << 8);
Probably this will be the reason why we check
the vector in process_oq() for processing
controller fatal error
if (vec == (pm8001_ha->max_q_num - 1)) {
Please do let me know if it helped in clarification.
> >
> >> + 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<[email protected]>
> > Tested-by: Damien Le Moal<[email protected]>
>
> Thanks!
>
> john
Thanks,
Ajish
On 06/01/2022 13:03, [email protected] wrote:
>> only a specific vector and, also, why we check at all in
>> an interrupt handler.
> Here is my initial understanding so far based on the code
> and data sheet
>
> 1. Controller has the capability to communicate
> to the host about fatal error condition via configured
> interrupt vector MSI/MSI-X.
> 2. This capability is achieved by setting two fields
> a. Enable Controller Fatal error notification
> Dowrd 0x1C, Bit[0].
> 1 - Enable; 0 - Disable
> Code: pm8001_ha->main_cfg_tbl.pm80xx_tbl.
> fatal_err_interrupt = 0x01;
> b. Fatal Error Interrupt Vector Dword 0x1C, bit[15:8]
> This parameter configures which interrupt vector
> is used to notify the host of the fatal error.
> Code: /* Update Fatal error interrupt vector */
> pm8001_ha->main_cfg_tbl.pm80xx_tbl.
> fatal_err_interrupt |=
> ((pm8001_ha->max_q_num - 1) << 8);
>
> Probably this will be the reason why we check
> the vector in process_oq() for processing
> controller fatal error
>
> if (vec == (pm8001_ha->max_q_num - 1)) {
>
> Please do let me know if it helped in clarification.
>
Sounds reasonable. And we only discover the issue for 8008/8009 now as
we have that (pm8001_ha->max_q_num - 1) vector being used for standard IO.
So let me know of any other issue, otherwise I'll send a v2 with the
coding style fixup.
Thanks,
John
On 2022/01/07 0:32, John Garry wrote:
> On 06/01/2022 13:03, [email protected] wrote:
>>> only a specific vector and, also, why we check at all in
>>> an interrupt handler.
>> Here is my initial understanding so far based on the code
>> and data sheet
>>
>> 1. Controller has the capability to communicate
>> to the host about fatal error condition via configured
>> interrupt vector MSI/MSI-X.
>> 2. This capability is achieved by setting two fields
>> a. Enable Controller Fatal error notification
>> Dowrd 0x1C, Bit[0].
>> 1 - Enable; 0 - Disable
>> Code: pm8001_ha->main_cfg_tbl.pm80xx_tbl.
>> fatal_err_interrupt = 0x01;
>> b. Fatal Error Interrupt Vector Dword 0x1C, bit[15:8]
>> This parameter configures which interrupt vector
>> is used to notify the host of the fatal error.
>> Code: /* Update Fatal error interrupt vector */
>> pm8001_ha->main_cfg_tbl.pm80xx_tbl.
>> fatal_err_interrupt |=
>> ((pm8001_ha->max_q_num - 1) << 8);
>>
>> Probably this will be the reason why we check
>> the vector in process_oq() for processing
>> controller fatal error
>>
>> if (vec == (pm8001_ha->max_q_num - 1)) {
>>
>> Please do let me know if it helped in clarification.
>>
>
> Sounds reasonable. And we only discover the issue for 8008/8009 now as
> we have that (pm8001_ha->max_q_num - 1) vector being used for standard IO.
>
> So let me know of any other issue, otherwise I'll send a v2 with the
> coding style fixup.
And maybe add comments about the above so that the information does not get lost ?
>
> Thanks,
> John
--
Damien Le Moal
Western Digital Research
Hi John,
> On 06/01/2022 13:03, [email protected] wrote:
> >> only a specific vector and, also, why we check at all in an
> >> interrupt handler.
> > Here is my initial understanding so far based on the code and data
> > sheet
> >
> > 1. Controller has the capability to communicate to the host about
> > fatal error condition via configured interrupt vector MSI/MSI-X.
> > 2. This capability is achieved by setting two fields
> > a. Enable Controller Fatal error notification
> > Dowrd 0x1C, Bit[0].
> > 1 - Enable; 0 - Disable
> > Code: pm8001_ha->main_cfg_tbl.pm80xx_tbl.
> > fatal_err_interrupt = 0x01;
> > b. Fatal Error Interrupt Vector Dword 0x1C, bit[15:8]
> > This parameter configures which interrupt vector
> > is used to notify the host of the fatal error.
> > Code: /* Update Fatal error interrupt vector */
> > pm8001_ha->main_cfg_tbl.pm80xx_tbl.
> > fatal_err_interrupt |=
> > ((pm8001_ha->max_q_num - 1) << 8);
> >
> > Probably this will be the reason why we check the vector in
> > process_oq() for processing controller fatal error
> >
> > if (vec == (pm8001_ha->max_q_num - 1)) {
> >
> > Please do let me know if it helped in clarification.
> >
>
> Sounds reasonable. And we only discover the issue for 8008/8009 now as we
> have that (pm8001_ha->max_q_num - 1) vector being used for standard IO.
>
> So let me know of any other issue, otherwise I'll send a v2 with the coding
> style fixup.
After going through check_fw_ready(), the change here looks fine.
>
> Thanks,
> John