[PATCH 0/2] pm80xx updates

[PATCH 0/2] pm80xx updates

[Ajish Koshy]

This patchset includes bugfixes for pm80xx driver

Ajish Koshy (2):
  scsi: pm80xx: mask and unmask upper interrupt vectors 32-63
  scsi: pm80xx: enable upper inbound, outbound queues

 drivers/scsi/pm8001/pm80xx_hwi.c | 42 ++++++++++++++++++++++++++++----
 1 file changed, 37 insertions(+), 5 deletions(-)

-- 
2.31.1

[PATCH 1/2] scsi: pm80xx: mask and unmask upper interrupt vectors 32-63

[Ajish Koshy]

When upper inbound and outbound queues 32-63 are enabled, we see upper
vectors 32-63 in interrupt service routine. We need corresponding
registers to handle masking and unmasking of these upper interrupts.

To achieve this, we use registers MSGU_ODMR_U(0x34) to mask and
MSGU_ODMR_CLR_U(0x3C) to unmask the interrupts. In these registers bit
0-31 represents interrupt vectors 32-63.

Signed-off-by: Ajish Koshy <Ajish.Koshy@microchip.com>
Signed-off-by: Viswas G <Viswas.G@microchip.com>
---
 drivers/scsi/pm8001/pm80xx_hwi.c | 35 +++++++++++++++++++++++++++-----
 1 file changed, 30 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/pm8001/pm80xx_hwi.c b/drivers/scsi/pm8001/pm80xx_hwi.c
index 9bb31f66db85..b92e82a576e3 100644
--- a/drivers/scsi/pm8001/pm80xx_hwi.c
+++ b/drivers/scsi/pm8001/pm80xx_hwi.c
@@ -1728,9 +1728,20 @@ pm80xx_chip_interrupt_enable(struct pm8001_hba_info *pm8001_ha, u8 vec)
 {
 #ifdef PM8001_USE_MSIX
 	u32 mask;
-	mask = (u32)(1 << vec);
+	u32 vec_u;
 
-	pm8001_cw32(pm8001_ha, 0, MSGU_ODMR_CLR, (u32)(mask & 0xFFFFFFFF));
+	if (vec < 32) {
+		mask = (u32)(1 << vec);
+		/*vectors 0 - 31*/
+		pm8001_cw32(pm8001_ha, 0, MSGU_ODMR_CLR,
+			    (u32)(mask & 0xFFFFFFFF));
+	} else {
+		vec_u = vec - 32;
+		mask = (u32)(1 << vec_u);
+		/*vectors 32 - 63*/
+		pm8001_cw32(pm8001_ha, 0, MSGU_ODMR_CLR_U,
+			    (u32)(mask & 0xFFFFFFFF));
+	}
 	return;
 #endif
 	pm80xx_chip_intx_interrupt_enable(pm8001_ha);
@@ -1747,11 +1758,25 @@ pm80xx_chip_interrupt_disable(struct pm8001_hba_info *pm8001_ha, u8 vec)
 {
 #ifdef PM8001_USE_MSIX
 	u32 mask;
-	if (vec == 0xFF)
+	u32 vec_u;
+
+	if (vec == 0xFF) {
 		mask = 0xFFFFFFFF;
-	else
+		/* disable all vectors 0-31, 32-63*/
+		pm8001_cw32(pm8001_ha, 0, MSGU_ODMR, mask);
+		pm8001_cw32(pm8001_ha, 0, MSGU_ODMR_U, mask);
+	} else if (vec < 32) {
 		mask = (u32)(1 << vec);
-	pm8001_cw32(pm8001_ha, 0, MSGU_ODMR, (u32)(mask & 0xFFFFFFFF));
+		/*vectors 0 - 31*/
+		pm8001_cw32(pm8001_ha, 0, MSGU_ODMR,
+			    (u32)(mask & 0xFFFFFFFF));
+	} else {
+		vec_u = vec - 32;
+		mask = (u32)(1 << vec_u);
+		/*vectors 32 - 63*/
+		pm8001_cw32(pm8001_ha, 0, MSGU_ODMR_U,
+			    (u32)(mask & 0xFFFFFFFF));
+	}
 	return;
 #endif
 	pm80xx_chip_intx_interrupt_disable(pm8001_ha);
-- 
2.31.1

Re: [PATCH 1/2] scsi: pm80xx: mask and unmask upper interrupt vectors 32-63

[Damien Le Moal]

On 3/28/22 17:42, Ajish Koshy wrote:
> When upper inbound and outbound queues 32-63 are enabled, we see upper
> vectors 32-63 in interrupt service routine. We need corresponding
> registers to handle masking and unmasking of these upper interrupts.
> 
> To achieve this, we use registers MSGU_ODMR_U(0x34) to mask and
> MSGU_ODMR_CLR_U(0x3C) to unmask the interrupts. In these registers bit
> 0-31 represents interrupt vectors 32-63.
> 
> Signed-off-by: Ajish Koshy <Ajish.Koshy@microchip.com>
> Signed-off-by: Viswas G <Viswas.G@microchip.com>
> ---
>  drivers/scsi/pm8001/pm80xx_hwi.c | 35 +++++++++++++++++++++++++++-----
>  1 file changed, 30 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/scsi/pm8001/pm80xx_hwi.c b/drivers/scsi/pm8001/pm80xx_hwi.c
> index 9bb31f66db85..b92e82a576e3 100644
> --- a/drivers/scsi/pm8001/pm80xx_hwi.c
> +++ b/drivers/scsi/pm8001/pm80xx_hwi.c
> @@ -1728,9 +1728,20 @@ pm80xx_chip_interrupt_enable(struct pm8001_hba_info *pm8001_ha, u8 vec)
>  {
>  #ifdef PM8001_USE_MSIX
>  	u32 mask;
> -	mask = (u32)(1 << vec);
> +	u32 vec_u;
>  
> -	pm8001_cw32(pm8001_ha, 0, MSGU_ODMR_CLR, (u32)(mask & 0xFFFFFFFF));
> +	if (vec < 32) {
> +		mask = (u32)(1 << vec);
> +		/*vectors 0 - 31*/
> +		pm8001_cw32(pm8001_ha, 0, MSGU_ODMR_CLR,
> +			    (u32)(mask & 0xFFFFFFFF));
> +	} else {
> +		vec_u = vec - 32;
> +		mask = (u32)(1 << vec_u);
> +		/*vectors 32 - 63*/
> +		pm8001_cw32(pm8001_ha, 0, MSGU_ODMR_CLR_U,
> +			    (u32)(mask & 0xFFFFFFFF));
> +	}
>  	return;
>  #endif
>  	pm80xx_chip_intx_interrupt_enable(pm8001_ha);
> @@ -1747,11 +1758,25 @@ pm80xx_chip_interrupt_disable(struct pm8001_hba_info *pm8001_ha, u8 vec)
>  {
>  #ifdef PM8001_USE_MSIX
>  	u32 mask;
> -	if (vec == 0xFF)
> +	u32 vec_u;
> +
> +	if (vec == 0xFF) {
>  		mask = 0xFFFFFFFF;
> -	else
> +		/* disable all vectors 0-31, 32-63*/
> +		pm8001_cw32(pm8001_ha, 0, MSGU_ODMR, mask);
> +		pm8001_cw32(pm8001_ha, 0, MSGU_ODMR_U, mask);
> +	} else if (vec < 32) {
>  		mask = (u32)(1 << vec);
> -	pm8001_cw32(pm8001_ha, 0, MSGU_ODMR, (u32)(mask & 0xFFFFFFFF));
> +		/*vectors 0 - 31*/
> +		pm8001_cw32(pm8001_ha, 0, MSGU_ODMR,
> +			    (u32)(mask & 0xFFFFFFFF));

mask is a u32, so the "& 0xFFFFFFFF" seems to be completely pointless...
And pm8001_cw32() takes a u32 value as last argument so the cast is also
pointless.

> +	} else {
> +		vec_u = vec - 32;
> +		mask = (u32)(1 << vec_u);

Cast not needed and this should probably be "1U << vec_u".
Also, the vec_u variable seems completely unnecessary.

> +		/*vectors 32 - 63*/
> +		pm8001_cw32(pm8001_ha, 0, MSGU_ODMR_U,
> +			    (u32)(mask & 0xFFFFFFFF));

Same here: The cast and masking are not needed.

> +	}
>  	return;
>  #endif
>  	pm80xx_chip_intx_interrupt_disable(pm8001_ha);


-- 
Damien Le Moal
Western Digital Research

RE: [PATCH 1/2] scsi: pm80xx: mask and unmask upper interrupt vectors 32-63

[Ajish.Koshy]

Thanks Damien for your comments here. Will make these changes in
PATCH V2.

> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> On 3/28/22 17:42, Ajish Koshy wrote:
> > When upper inbound and outbound queues 32-63 are enabled, we see
> upper
> > vectors 32-63 in interrupt service routine. We need corresponding
> > registers to handle masking and unmasking of these upper interrupts.
> >
> > To achieve this, we use registers MSGU_ODMR_U(0x34) to mask and
> > MSGU_ODMR_CLR_U(0x3C) to unmask the interrupts. In these registers bit
> > 0-31 represents interrupt vectors 32-63.
> >
> > Signed-off-by: Ajish Koshy <Ajish.Koshy@microchip.com>
> > Signed-off-by: Viswas G <Viswas.G@microchip.com>
> > ---
> >  drivers/scsi/pm8001/pm80xx_hwi.c | 35
> > +++++++++++++++++++++++++++-----
> >  1 file changed, 30 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/scsi/pm8001/pm80xx_hwi.c
> > b/drivers/scsi/pm8001/pm80xx_hwi.c
> > index 9bb31f66db85..b92e82a576e3 100644
> > --- a/drivers/scsi/pm8001/pm80xx_hwi.c
> > +++ b/drivers/scsi/pm8001/pm80xx_hwi.c
> > @@ -1728,9 +1728,20 @@ pm80xx_chip_interrupt_enable(struct
> > pm8001_hba_info *pm8001_ha, u8 vec)  {  #ifdef PM8001_USE_MSIX
> >       u32 mask;
> > -     mask = (u32)(1 << vec);
> > +     u32 vec_u;
> >
> > -     pm8001_cw32(pm8001_ha, 0, MSGU_ODMR_CLR, (u32)(mask &
> 0xFFFFFFFF));
> > +     if (vec < 32) {
> > +             mask = (u32)(1 << vec);
> > +             /*vectors 0 - 31*/
> > +             pm8001_cw32(pm8001_ha, 0, MSGU_ODMR_CLR,
> > +                         (u32)(mask & 0xFFFFFFFF));
> > +     } else {
> > +             vec_u = vec - 32;
> > +             mask = (u32)(1 << vec_u);
> > +             /*vectors 32 - 63*/
> > +             pm8001_cw32(pm8001_ha, 0, MSGU_ODMR_CLR_U,
> > +                         (u32)(mask & 0xFFFFFFFF));
> > +     }
> >       return;
> >  #endif
> >       pm80xx_chip_intx_interrupt_enable(pm8001_ha);
> > @@ -1747,11 +1758,25 @@ pm80xx_chip_interrupt_disable(struct
> > pm8001_hba_info *pm8001_ha, u8 vec)  {  #ifdef PM8001_USE_MSIX
> >       u32 mask;
> > -     if (vec == 0xFF)
> > +     u32 vec_u;
> > +
> > +     if (vec == 0xFF) {
> >               mask = 0xFFFFFFFF;
> > -     else
> > +             /* disable all vectors 0-31, 32-63*/
> > +             pm8001_cw32(pm8001_ha, 0, MSGU_ODMR, mask);
> > +             pm8001_cw32(pm8001_ha, 0, MSGU_ODMR_U, mask);
> > +     } else if (vec < 32) {
> >               mask = (u32)(1 << vec);
> > -     pm8001_cw32(pm8001_ha, 0, MSGU_ODMR, (u32)(mask &
> 0xFFFFFFFF));
> > +             /*vectors 0 - 31*/
> > +             pm8001_cw32(pm8001_ha, 0, MSGU_ODMR,
> > +                         (u32)(mask & 0xFFFFFFFF));
> 
> mask is a u32, so the "& 0xFFFFFFFF" seems to be completely pointless...
> And pm8001_cw32() takes a u32 value as last argument so the cast is also
> pointless.

OK
> 
> > +     } else {
> > +             vec_u = vec - 32;
> > +             mask = (u32)(1 << vec_u);
> 
> Cast not needed and this should probably be "1U << vec_u".
> Also, the vec_u variable seems completely unnecessary.

OK
> 
> > +             /*vectors 32 - 63*/
> > +             pm8001_cw32(pm8001_ha, 0, MSGU_ODMR_U,
> > +                         (u32)(mask & 0xFFFFFFFF));
> 
> Same here: The cast and masking are not needed.

OK
> 
> > +     }
> >       return;
> >  #endif
> >       pm80xx_chip_intx_interrupt_disable(pm8001_ha);
> 
> 
> --
> Damien Le Moal
> Western Digital Research

Re: [PATCH 1/2] scsi: pm80xx: mask and unmask upper interrupt vectors 32-63

[Jinpu Wang]

On Mon, Mar 28, 2022 at 10:42 AM Ajish Koshy <Ajish.Koshy@microchip.com> wrote:
>
> When upper inbound and outbound queues 32-63 are enabled, we see upper
> vectors 32-63 in interrupt service routine. We need corresponding
> registers to handle masking and unmasking of these upper interrupts.
>
> To achieve this, we use registers MSGU_ODMR_U(0x34) to mask and
> MSGU_ODMR_CLR_U(0x3C) to unmask the interrupts. In these registers bit
> 0-31 represents interrupt vectors 32-63.
>
> Signed-off-by: Ajish Koshy <Ajish.Koshy@microchip.com>
> Signed-off-by: Viswas G <Viswas.G@microchip.com>
same as patch2, the fixes commit should be add here.
> ---
>  drivers/scsi/pm8001/pm80xx_hwi.c | 35 +++++++++++++++++++++++++++-----
>  1 file changed, 30 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/scsi/pm8001/pm80xx_hwi.c b/drivers/scsi/pm8001/pm80xx_hwi.c
> index 9bb31f66db85..b92e82a576e3 100644
> --- a/drivers/scsi/pm8001/pm80xx_hwi.c
> +++ b/drivers/scsi/pm8001/pm80xx_hwi.c
> @@ -1728,9 +1728,20 @@ pm80xx_chip_interrupt_enable(struct pm8001_hba_info *pm8001_ha, u8 vec)
>  {
>  #ifdef PM8001_USE_MSIX
>         u32 mask;
> -       mask = (u32)(1 << vec);
> +       u32 vec_u;
>
> -       pm8001_cw32(pm8001_ha, 0, MSGU_ODMR_CLR, (u32)(mask & 0xFFFFFFFF));
> +       if (vec < 32) {
> +               mask = (u32)(1 << vec);
> +               /*vectors 0 - 31*/
> +               pm8001_cw32(pm8001_ha, 0, MSGU_ODMR_CLR,
> +                           (u32)(mask & 0xFFFFFFFF));
> +       } else {
> +               vec_u = vec - 32;
> +               mask = (u32)(1 << vec_u);
> +               /*vectors 32 - 63*/
> +               pm8001_cw32(pm8001_ha, 0, MSGU_ODMR_CLR_U,
> +                           (u32)(mask & 0xFFFFFFFF));
> +       }
>         return;
>  #endif
>         pm80xx_chip_intx_interrupt_enable(pm8001_ha);
> @@ -1747,11 +1758,25 @@ pm80xx_chip_interrupt_disable(struct pm8001_hba_info *pm8001_ha, u8 vec)
>  {
>  #ifdef PM8001_USE_MSIX
>         u32 mask;
> -       if (vec == 0xFF)
> +       u32 vec_u;
> +
> +       if (vec == 0xFF) {
>                 mask = 0xFFFFFFFF;
> -       else
> +               /* disable all vectors 0-31, 32-63*/
> +               pm8001_cw32(pm8001_ha, 0, MSGU_ODMR, mask);
> +               pm8001_cw32(pm8001_ha, 0, MSGU_ODMR_U, mask);
> +       } else if (vec < 32) {
>                 mask = (u32)(1 << vec);
> -       pm8001_cw32(pm8001_ha, 0, MSGU_ODMR, (u32)(mask & 0xFFFFFFFF));
> +               /*vectors 0 - 31*/
> +               pm8001_cw32(pm8001_ha, 0, MSGU_ODMR,
> +                           (u32)(mask & 0xFFFFFFFF));
> +       } else {
> +               vec_u = vec - 32;
> +               mask = (u32)(1 << vec_u);
> +               /*vectors 32 - 63*/
> +               pm8001_cw32(pm8001_ha, 0, MSGU_ODMR_U,
> +                           (u32)(mask & 0xFFFFFFFF));
> +       }
>         return;
>  #endif
>         pm80xx_chip_intx_interrupt_disable(pm8001_ha);
> --
> 2.31.1
>

RE: [PATCH 1/2] scsi: pm80xx: mask and unmask upper interrupt vectors 32-63

[Ajish.Koshy]

Thanks Jinpu.

> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> On Mon, Mar 28, 2022 at 10:42 AM Ajish Koshy
> <Ajish.Koshy@microchip.com> wrote:
> >
> > When upper inbound and outbound queues 32-63 are enabled, we see
> upper
> > vectors 32-63 in interrupt service routine. We need corresponding
> > registers to handle masking and unmasking of these upper interrupts.
> >
> > To achieve this, we use registers MSGU_ODMR_U(0x34) to mask and
> > MSGU_ODMR_CLR_U(0x3C) to unmask the interrupts. In these registers bit
> > 0-31 represents interrupt vectors 32-63.
> >
> > Signed-off-by: Ajish Koshy <Ajish.Koshy@microchip.com>
> > Signed-off-by: Viswas G <Viswas.G@microchip.com>
> same as patch2, the fixes commit should be add here.

Will make the changes in V2.

> > ---
> >  drivers/scsi/pm8001/pm80xx_hwi.c | 35
> > +++++++++++++++++++++++++++-----
> >  1 file changed, 30 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/scsi/pm8001/pm80xx_hwi.c
> > b/drivers/scsi/pm8001/pm80xx_hwi.c
> > index 9bb31f66db85..b92e82a576e3 100644
> > --- a/drivers/scsi/pm8001/pm80xx_hwi.c
> > +++ b/drivers/scsi/pm8001/pm80xx_hwi.c
> > @@ -1728,9 +1728,20 @@ pm80xx_chip_interrupt_enable(struct
> > pm8001_hba_info *pm8001_ha, u8 vec)  {  #ifdef PM8001_USE_MSIX
> >         u32 mask;
> > -       mask = (u32)(1 << vec);
> > +       u32 vec_u;
> >
> > -       pm8001_cw32(pm8001_ha, 0, MSGU_ODMR_CLR, (u32)(mask &
> 0xFFFFFFFF));
> > +       if (vec < 32) {
> > +               mask = (u32)(1 << vec);
> > +               /*vectors 0 - 31*/
> > +               pm8001_cw32(pm8001_ha, 0, MSGU_ODMR_CLR,
> > +                           (u32)(mask & 0xFFFFFFFF));
> > +       } else {
> > +               vec_u = vec - 32;
> > +               mask = (u32)(1 << vec_u);
> > +               /*vectors 32 - 63*/
> > +               pm8001_cw32(pm8001_ha, 0, MSGU_ODMR_CLR_U,
> > +                           (u32)(mask & 0xFFFFFFFF));
> > +       }
> >         return;
> >  #endif
> >         pm80xx_chip_intx_interrupt_enable(pm8001_ha);
> > @@ -1747,11 +1758,25 @@ pm80xx_chip_interrupt_disable(struct
> > pm8001_hba_info *pm8001_ha, u8 vec)  {  #ifdef PM8001_USE_MSIX
> >         u32 mask;
> > -       if (vec == 0xFF)
> > +       u32 vec_u;
> > +
> > +       if (vec == 0xFF) {
> >                 mask = 0xFFFFFFFF;
> > -       else
> > +               /* disable all vectors 0-31, 32-63*/
> > +               pm8001_cw32(pm8001_ha, 0, MSGU_ODMR, mask);
> > +               pm8001_cw32(pm8001_ha, 0, MSGU_ODMR_U, mask);
> > +       } else if (vec < 32) {
> >                 mask = (u32)(1 << vec);
> > -       pm8001_cw32(pm8001_ha, 0, MSGU_ODMR, (u32)(mask &
> 0xFFFFFFFF));
> > +               /*vectors 0 - 31*/
> > +               pm8001_cw32(pm8001_ha, 0, MSGU_ODMR,
> > +                           (u32)(mask & 0xFFFFFFFF));
> > +       } else {
> > +               vec_u = vec - 32;
> > +               mask = (u32)(1 << vec_u);
> > +               /*vectors 32 - 63*/
> > +               pm8001_cw32(pm8001_ha, 0, MSGU_ODMR_U,
> > +                           (u32)(mask & 0xFFFFFFFF));
> > +       }
> >         return;
> >  #endif
> >         pm80xx_chip_intx_interrupt_disable(pm8001_ha);
> > --
> > 2.31.1
> >

Thanks,
Ajish

[PATCH 2/2] scsi: pm80xx: enable upper inbound, outbound queues

[Ajish Koshy]

Executing driver on servers with more than 32 CPUs were faced with command
timeouts. This is because we were not geting completions for commands
submitted on IQ32 - IQ63.

Set E64Q bit to enable upper inbound and outbound queues 32 to 63 in the
MPI main configuration table.

Added 500ms delay after successful MPI initialization as mentioned in
controller datasheet.

Signed-off-by: Ajish Koshy <Ajish.Koshy@microchip.com>
Signed-off-by: Viswas G <Viswas.G@microchip.com>
---
 drivers/scsi/pm8001/pm80xx_hwi.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/scsi/pm8001/pm80xx_hwi.c b/drivers/scsi/pm8001/pm80xx_hwi.c
index b92e82a576e3..f04c6c589615 100644
--- a/drivers/scsi/pm8001/pm80xx_hwi.c
+++ b/drivers/scsi/pm8001/pm80xx_hwi.c
@@ -766,6 +766,10 @@ static void init_default_table_values(struct pm8001_hba_info *pm8001_ha)
 	pm8001_ha->main_cfg_tbl.pm80xx_tbl.pcs_event_log_severity	= 0x01;
 	pm8001_ha->main_cfg_tbl.pm80xx_tbl.fatal_err_interrupt		= 0x01;
 
+	/* Enable higher IQs and OQs, 32 to 63, bit 16*/
+	if (pm8001_ha->max_q_num > 32)
+		pm8001_ha->main_cfg_tbl.pm80xx_tbl.fatal_err_interrupt |=
+							(1 << 16);
 	/* Disable end to end CRC checking */
 	pm8001_ha->main_cfg_tbl.pm80xx_tbl.crc_core_dump = (0x1 << 16);
 
@@ -1027,6 +1031,9 @@ static int mpi_init_check(struct pm8001_hba_info *pm8001_ha)
 	if (0x0000 != gst_len_mpistate)
 		return -EBUSY;
 
+	/* Wait for 500ms after successful MPI initialization*/
+	msleep(500);
+
 	return 0;
 }
 
-- 
2.31.1

Re: [PATCH 2/2] scsi: pm80xx: enable upper inbound, outbound queues

[Damien Le Moal]

On 3/28/22 17:42, Ajish Koshy wrote:
> Executing driver on servers with more than 32 CPUs were faced with command
> timeouts. This is because we were not geting completions for commands
> submitted on IQ32 - IQ63.
> 
> Set E64Q bit to enable upper inbound and outbound queues 32 to 63 in the
> MPI main configuration table.
> 
> Added 500ms delay after successful MPI initialization as mentioned in
> controller datasheet.
> 
> Signed-off-by: Ajish Koshy <Ajish.Koshy@microchip.com>
> Signed-off-by: Viswas G <Viswas.G@microchip.com>
> ---
>  drivers/scsi/pm8001/pm80xx_hwi.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/scsi/pm8001/pm80xx_hwi.c b/drivers/scsi/pm8001/pm80xx_hwi.c
> index b92e82a576e3..f04c6c589615 100644
> --- a/drivers/scsi/pm8001/pm80xx_hwi.c
> +++ b/drivers/scsi/pm8001/pm80xx_hwi.c
> @@ -766,6 +766,10 @@ static void init_default_table_values(struct pm8001_hba_info *pm8001_ha)
>  	pm8001_ha->main_cfg_tbl.pm80xx_tbl.pcs_event_log_severity	= 0x01;
>  	pm8001_ha->main_cfg_tbl.pm80xx_tbl.fatal_err_interrupt		= 0x01;
>  
> +	/* Enable higher IQs and OQs, 32 to 63, bit 16*/
> +	if (pm8001_ha->max_q_num > 32)
> +		pm8001_ha->main_cfg_tbl.pm80xx_tbl.fatal_err_interrupt |=
> +							(1 << 16);

No need for the brackets.

>  	/* Disable end to end CRC checking */
>  	pm8001_ha->main_cfg_tbl.pm80xx_tbl.crc_core_dump = (0x1 << 16);
>  
> @@ -1027,6 +1031,9 @@ static int mpi_init_check(struct pm8001_hba_info *pm8001_ha)
>  	if (0x0000 != gst_len_mpistate)
>  		return -EBUSY;
>  
> +	/* Wait for 500ms after successful MPI initialization*/

Is this comment really necessary ? Anybody sees that this will wait. It
may be better to explain *why* the wait is needed.

> +	msleep(500);
> +
>  	return 0;
>  }
>  


-- 
Damien Le Moal
Western Digital Research

RE: [PATCH 2/2] scsi: pm80xx: enable upper inbound, outbound queues

[Ajish.Koshy]

Thanks Damien for your comments here. Will make these changes in
PATCH V2.

> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> On 3/28/22 17:42, Ajish Koshy wrote:
> > Executing driver on servers with more than 32 CPUs were faced with
> > command timeouts. This is because we were not geting completions for
> > commands submitted on IQ32 - IQ63.
> >
> > Set E64Q bit to enable upper inbound and outbound queues 32 to 63 in
> > the MPI main configuration table.
> >
> > Added 500ms delay after successful MPI initialization as mentioned in
> > controller datasheet.
> >
> > Signed-off-by: Ajish Koshy <Ajish.Koshy@microchip.com>
> > Signed-off-by: Viswas G <Viswas.G@microchip.com>
> > ---
> >  drivers/scsi/pm8001/pm80xx_hwi.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/drivers/scsi/pm8001/pm80xx_hwi.c
> > b/drivers/scsi/pm8001/pm80xx_hwi.c
> > index b92e82a576e3..f04c6c589615 100644
> > --- a/drivers/scsi/pm8001/pm80xx_hwi.c
> > +++ b/drivers/scsi/pm8001/pm80xx_hwi.c
> > @@ -766,6 +766,10 @@ static void init_default_table_values(struct
> pm8001_hba_info *pm8001_ha)
> >       pm8001_ha->main_cfg_tbl.pm80xx_tbl.pcs_event_log_severity       =
> 0x01;
> >       pm8001_ha->main_cfg_tbl.pm80xx_tbl.fatal_err_interrupt          = 0x01;
> >
> > +     /* Enable higher IQs and OQs, 32 to 63, bit 16*/
> > +     if (pm8001_ha->max_q_num > 32)
> > +             pm8001_ha->main_cfg_tbl.pm80xx_tbl.fatal_err_interrupt |=
> > +                                                     (1 << 16);
> 
> No need for the brackets.

OK
> 
> >       /* Disable end to end CRC checking */
> >       pm8001_ha->main_cfg_tbl.pm80xx_tbl.crc_core_dump = (0x1 << 16);
> >
> > @@ -1027,6 +1031,9 @@ static int mpi_init_check(struct
> pm8001_hba_info *pm8001_ha)
> >       if (0x0000 != gst_len_mpistate)
> >               return -EBUSY;
> >
> > +     /* Wait for 500ms after successful MPI initialization*/
> 
> Is this comment really necessary ? Anybody sees that this will wait. It may be
> better to explain *why* the wait is needed.

Yes you may be right here. The code itself speaks about this. 

The reason for delay is not mentioned in the datasheet.

When going through this E64Q bit details they mentioned about the MPI
initialization steps to be followed. 
Since MPI Initialization is mentioned separately irrespective of E64Q bit, I believe
this may be needed to be followed in all the cases.

Well here controller datasheet is not talking much on this wait. It simply says
"Note: It is recommended to wait 500ms after the MPI-S field indicates
the host has successfully initialized the MPI before sending commands."

> 
> > +     msleep(500);
> > +
> >       return 0;
> >  }
> >
> 
> 
> --
> Damien Le Moal
> Western Digital Research

Thanks,
Ajish

Re: [PATCH 2/2] scsi: pm80xx: enable upper inbound, outbound queues

[Jinpu Wang]

On Mon, Mar 28, 2022 at 10:42 AM Ajish Koshy <Ajish.Koshy@microchip.com> wrote:
>
> Executing driver on servers with more than 32 CPUs were faced with command
> timeouts. This is because we were not geting completions for commands
> submitted on IQ32 - IQ63.
>
> Set E64Q bit to enable upper inbound and outbound queues 32 to 63 in the
> MPI main configuration table.
>
> Added 500ms delay after successful MPI initialization as mentioned in
> controller datasheet.
>
> Signed-off-by: Ajish Koshy <Ajish.Koshy@microchip.com>
> Signed-off-by: Viswas G <Viswas.G@microchip.com>
Please add
  Fixes: 05c6c029a44d ("scsi: pm80xx: Increase number of supported
queues")  so it will pickup
automatically by stable.

> ---
>  drivers/scsi/pm8001/pm80xx_hwi.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/drivers/scsi/pm8001/pm80xx_hwi.c b/drivers/scsi/pm8001/pm80xx_hwi.c
> index b92e82a576e3..f04c6c589615 100644
> --- a/drivers/scsi/pm8001/pm80xx_hwi.c
> +++ b/drivers/scsi/pm8001/pm80xx_hwi.c
> @@ -766,6 +766,10 @@ static void init_default_table_values(struct pm8001_hba_info *pm8001_ha)
>         pm8001_ha->main_cfg_tbl.pm80xx_tbl.pcs_event_log_severity       = 0x01;
>         pm8001_ha->main_cfg_tbl.pm80xx_tbl.fatal_err_interrupt          = 0x01;
>
> +       /* Enable higher IQs and OQs, 32 to 63, bit 16*/
> +       if (pm8001_ha->max_q_num > 32)
> +               pm8001_ha->main_cfg_tbl.pm80xx_tbl.fatal_err_interrupt |=
> +                                                       (1 << 16);
>         /* Disable end to end CRC checking */
>         pm8001_ha->main_cfg_tbl.pm80xx_tbl.crc_core_dump = (0x1 << 16);
>
> @@ -1027,6 +1031,9 @@ static int mpi_init_check(struct pm8001_hba_info *pm8001_ha)
>         if (0x0000 != gst_len_mpistate)
>                 return -EBUSY;
>
> +       /* Wait for 500ms after successful MPI initialization*/
> +       msleep(500);
> +
>         return 0;
>  }
>
> --
> 2.31.1
>

RE: [PATCH 2/2] scsi: pm80xx: enable upper inbound, outbound queues

[Ajish.Koshy]

Thanks Jinpu for your comments here. Will make the changes in
PATCH V2.

> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> On Mon, Mar 28, 2022 at 10:42 AM Ajish Koshy
> <Ajish.Koshy@microchip.com> wrote:
> >
> > Executing driver on servers with more than 32 CPUs were faced with
> > command timeouts. This is because we were not geting completions for
> > commands submitted on IQ32 - IQ63.
> >
> > Set E64Q bit to enable upper inbound and outbound queues 32 to 63 in
> > the MPI main configuration table.
> >
> > Added 500ms delay after successful MPI initialization as mentioned in
> > controller datasheet.
> >
> > Signed-off-by: Ajish Koshy <Ajish.Koshy@microchip.com>
> > Signed-off-by: Viswas G <Viswas.G@microchip.com>
> Please add
>   Fixes: 05c6c029a44d ("scsi: pm80xx: Increase number of supported
> queues")  so it will pickup
> automatically by stable.

OK
> 
> > ---
> >  drivers/scsi/pm8001/pm80xx_hwi.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/drivers/scsi/pm8001/pm80xx_hwi.c
> > b/drivers/scsi/pm8001/pm80xx_hwi.c
> > index b92e82a576e3..f04c6c589615 100644
> > --- a/drivers/scsi/pm8001/pm80xx_hwi.c
> > +++ b/drivers/scsi/pm8001/pm80xx_hwi.c
> > @@ -766,6 +766,10 @@ static void init_default_table_values(struct
> pm8001_hba_info *pm8001_ha)
> >         pm8001_ha->main_cfg_tbl.pm80xx_tbl.pcs_event_log_severity       =
> 0x01;
> >         pm8001_ha->main_cfg_tbl.pm80xx_tbl.fatal_err_interrupt          =
> 0x01;
> >
> > +       /* Enable higher IQs and OQs, 32 to 63, bit 16*/
> > +       if (pm8001_ha->max_q_num > 32)
> > +               pm8001_ha->main_cfg_tbl.pm80xx_tbl.fatal_err_interrupt |=
> > +                                                       (1 << 16);
> >         /* Disable end to end CRC checking */
> >         pm8001_ha->main_cfg_tbl.pm80xx_tbl.crc_core_dump = (0x1 <<
> > 16);
> >
> > @@ -1027,6 +1031,9 @@ static int mpi_init_check(struct
> pm8001_hba_info *pm8001_ha)
> >         if (0x0000 != gst_len_mpistate)
> >                 return -EBUSY;
> >
> > +       /* Wait for 500ms after successful MPI initialization*/
> > +       msleep(500);
> > +
> >         return 0;
> >  }
> >
> > --
> > 2.31.1
> >

Thanks,
Ajish

Re: [PATCH 2/2] scsi: pm80xx: enable upper inbound, outbound queues

[John Garry]

On 28/03/2022 09:42, Ajish Koshy wrote:
> Executing driver on servers with more than 32 CPUs were faced with command
> timeouts. This is because we were not geting completions for commands
> submitted on IQ32 - IQ63.
> 
> Set E64Q bit to enable upper inbound and outbound queues 32 to 63 in the
> MPI main configuration table.
> 
> Added 500ms delay after successful MPI initialization as mentioned in
> controller datasheet.
> 
> Signed-off-by: Ajish Koshy <Ajish.Koshy@microchip.com>
> Signed-off-by: Viswas G <Viswas.G@microchip.com>
> ---


I'm not sure if this change was meant to help, but I still see timeouts 
on my 96-core arm64 machine with this change - see log at bottom.

I still get the feeling that this issue I mention is timing related, as 
it goes away when I enable lots of heavy debug (like kasan, kmemleak, 
prove_locking, etc.


>   drivers/scsi/pm8001/pm80xx_hwi.c | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/scsi/pm8001/pm80xx_hwi.c b/drivers/scsi/pm8001/pm80xx_hwi.c
> index b92e82a576e3..f04c6c589615 100644
> --- a/drivers/scsi/pm8001/pm80xx_hwi.c
> +++ b/drivers/scsi/pm8001/pm80xx_hwi.c
> @@ -766,6 +766,10 @@ static void init_default_table_values(struct pm8001_hba_info *pm8001_ha)
>   	pm8001_ha->main_cfg_tbl.pm80xx_tbl.pcs_event_log_severity	= 0x01;
>   	pm8001_ha->main_cfg_tbl.pm80xx_tbl.fatal_err_interrupt		= 0x01;
>   
> +	/* Enable higher IQs and OQs, 32 to 63, bit 16*/
> +	if (pm8001_ha->max_q_num > 32)
> +		pm8001_ha->main_cfg_tbl.pm80xx_tbl.fatal_err_interrupt |=
> +							(1 << 16);
>   	/* Disable end to end CRC checking */
>   	pm8001_ha->main_cfg_tbl.pm80xx_tbl.crc_core_dump = (0x1 << 16);
>   
> @@ -1027,6 +1031,9 @@ static int mpi_init_check(struct pm8001_hba_info *pm8001_ha)
>   	if (0x0000 != gst_len_mpistate)
>   		return -EBUSY;
>   
> +	/* Wait for 500ms after successful MPI initialization*/
> +	msleep(500);
> +
>   	return 0;
>   }
>   



  126.037932] EXT4-fs (sda1): recovery complete
[  126.042297] EXT4-fs (sda1): mounted filesystem with ordered data 
mode. Quota mode: none.
[  159.939179] sas: Enter sas_scsi_recover_host busy: 256 failed: 256
[  159.945390] sas: sas_scsi_find_task: aborting task 0x(____ptrval____)
[  181.862870] sas: TMF task timeout for 5000c500a7b95a49 and not done
[  193.436187] pm80xx0:: pm8001_abort_task 1126: rc= 5
[  193.436188] pm80xx0:: mpi_ssp_completion 1937: sas IO status 0x1
[  193.441064] sas: sas_scsi_find_task: querying task 0x(____ptrval____)
[  193.447048] pm80xx0:: mpi_ssp_completion 1948: SAS Address of IO 
Failure Drive:5000c500a7b95a49
[  193.453528] pm80xx0:: mpi_ssp_completion 1937: sas IO status 0x3b
[  193.462158] pm80xx0:: mpi_ssp_completion 2185: task 
0x(____ptrval____) done with io_status 0x1 resp 0x0 stat 0x8c but 
aborted by upper layer!
[  193.468237] pm80xx0:: mpi_ssp_completion 1948: SAS Address of IO 
Failure Drive:5000c500a7b95a49
[  193.489658] sas: TMF task open reject failed  5000c500a7b95a49
[  193.495538] pm80xx0:: mpi_ssp_completion 1937: sas IO status 0x3b
[  193.501619] pm80xx0:: mpi_ssp_completion 1948: SAS Address of IO 
Failure Drive:5000c500a7b95a49
[  193.510371] sas: TMF task open reject failed  5000c500a7b95a49
[  193.516252] pm80xx0:: mpi_ssp_completion 1937: sas IO status 0x3b
[  193.522333] pm80xx0:: mpi_ssp_completion 1948: SAS Address of IO 
Failure Drive:5000c500a7b95a49
[  193.531075] sas: TMF task open reject failed  5000c500a7b95a49
[  193.536899] sas: executing TMF for 5000c500a7b95a49 failed after 3 
attempts!
[  193.543937] pm80xx: rc= -5
[  193.546631] sas: sas_scsi_find_task: task 0x(____ptrval____) result 
code -5 not handled
[  193.554622] sas: sas_scsi_find_task: aborting task 0x(____ptrval____)
[  193.561052] sas: sas_eh_handle_sas_errors: task 0x(____ptrval____) is 
done
[  193.567917] sas: sas_scsi_find_task: aborting task 0x(____ptrval____)
ls'ing 0
[  214.630859] sas: TMF task timeout for 5000c500a7b95a49 and not done
[  226.241090] pm80xx0:: mpi_ssp_completion 1937: sas IO status 0x1
[  226.241093] pm80xx0:: pm8001_abort_task 1126: rc= 5
[  226.247084] pm80xx0:: mpi_ssp_completion 1948: SAS Address of IO 
Failure Drive:5000c500a7b95a49
[  226.247087] pm80xx0:: mpi_ssp_completion 2185: task 
0x(____ptrval____) done with io_status 0x1 resp 0x0 stat 0x8c but 
aborted by upper layer!
[  226.273324] sas: sas_eh_handle_sas_errors: task 0x(____ptrval____) is 
done
[  226.280188] sas: sas_scsi_find_task: aborting task 0x(____ptrval____)
[  247.398856] sas: TMF task timeout for 5000c500a7b95a49 and not done

RE: [PATCH 2/2] scsi: pm80xx: enable upper inbound, outbound queues

[Ajish.Koshy]

Hi John,

> I'm not sure if this change was meant to help, but I still see timeouts on my
> 96-core arm64 machine with this change - see log at bottom.
> 
> I still get the feeling that this issue I mention is timing related, as it goes away
> when I enable lots of heavy debug (like kasan, kmemleak, prove_locking, etc.
> 

Thank you for sharing your observation on arm server with respect to these
patches.

It will be difficult to comment right now on arm server and as you said issue is not
observed when debugs are enabled.

Current patch was tested on amd 128 core server x86_64, drives connected to
enclosure. 

Loading driver(without patch) on this 128 core server had issues.

To get better understanding, booted the server in two scenarios, boot arguments
1. nr_cpus=32
2. nr_cpus=34

For #1 nr_cpus=32, was able to load the driver, run fio, unload the driver
without any issue.

But for #2 nr_cpus=34, things were not working properly. Loading had issues.
Error handler was getting triggered. After giving some debug prints, it was visible
that request submitted on IQ31 or less were getting completion whereas for requests
on IQ32 and IQ33 were not receiving completion/interrupt.

Post enabling this E64Q bit in the controller, made a difference. Now driver can be loaded,
run FIO, unload it without observing any issue, including on 128 core default setting.

Thanks,
Ajish