scsi: pm80xx: Use module param to set pcs event log severity

The pm8006 driver sets pcs event log threshold very high which causes
most of the FW logs to not be captured. This adds a module parameter to
configure pcs event log severity with 3 (medium severity) as the
default.

Signed-off-by: Bhavesh Jashnani <bjashnani@google.com>
Signed-off-by: Salomon Dushimirimana <salomondush@google.com>
---
 drivers/scsi/pm8001/pm8001_init.c | 4 ++++
 drivers/scsi/pm8001/pm8001_sas.h  | 2 ++
 drivers/scsi/pm8001/pm80xx_hwi.c  | 3 ++-
 3 files changed, 8 insertions(+), 1 deletion(-)

Hi,

On Thu, Oct 17, 2024 at 12:10 AM Salomon Dushimirimana
<salomondush@google.com> wrote:
>
> The pm8006 driver sets pcs event log threshold very high which causes
> most of the FW logs to not be captured. This adds a module parameter to
> configure pcs event log severity with 3 (medium severity) as the
> default.
upstream does not like more module parameters, can we just change the
default to 3, any harm?

Thx!
>
> Signed-off-by: Bhavesh Jashnani <bjashnani@google.com>
> Signed-off-by: Salomon Dushimirimana <salomondush@google.com>
> ---
>  drivers/scsi/pm8001/pm8001_init.c | 4 ++++
>  drivers/scsi/pm8001/pm8001_sas.h  | 2 ++
>  drivers/scsi/pm8001/pm80xx_hwi.c  | 3 ++-
>  3 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/pm8001/pm8001_init.c b/drivers/scsi/pm8001/pm8001_init.c
> index 1e63cb6cd8e3..355aab0c982a 100644
> --- a/drivers/scsi/pm8001/pm8001_init.c
> +++ b/drivers/scsi/pm8001/pm8001_init.c
> @@ -68,6 +68,10 @@ static bool pm8001_read_wwn = true;
>  module_param_named(read_wwn, pm8001_read_wwn, bool, 0444);
>  MODULE_PARM_DESC(zoned, "Get WWN from the controller. Default: true");
>
> +uint pcs_event_log_severity = 0x03;
> +module_param(pcs_event_log_severity, int, 0644);
> +MODULE_PARM_DESC(pcs_event_log_severity, "PCS event log severity level");
> +
>  static struct scsi_transport_template *pm8001_stt;
>  static int pm8001_init_ccb_tag(struct pm8001_hba_info *);
>
> diff --git a/drivers/scsi/pm8001/pm8001_sas.h b/drivers/scsi/pm8001/pm8001_sas.h
> index ced6721380a8..42c7b3f7afbf 100644
> --- a/drivers/scsi/pm8001/pm8001_sas.h
> +++ b/drivers/scsi/pm8001/pm8001_sas.h
> @@ -96,6 +96,8 @@ extern struct list_head hba_list;
>  extern const struct pm8001_dispatch pm8001_8001_dispatch;
>  extern const struct pm8001_dispatch pm8001_80xx_dispatch;
>
> +extern uint pcs_event_log_severity;
> +
>  struct pm8001_hba_info;
>  struct pm8001_ccb_info;
>  struct pm8001_device;
> diff --git a/drivers/scsi/pm8001/pm80xx_hwi.c b/drivers/scsi/pm8001/pm80xx_hwi.c
> index 8fe886dc5e47..9b237a764d0b 100644
> --- a/drivers/scsi/pm8001/pm80xx_hwi.c
> +++ b/drivers/scsi/pm8001/pm80xx_hwi.c
> @@ -763,7 +763,8 @@ static void init_default_table_values(struct pm8001_hba_info *pm8001_ha)
>                 pm8001_ha->memoryMap.region[IOP].phys_addr_lo;
>         pm8001_ha->main_cfg_tbl.pm80xx_tbl.pcs_event_log_size           =
>                                                         PM8001_EVENT_LOG_SIZE;
> -       pm8001_ha->main_cfg_tbl.pm80xx_tbl.pcs_event_log_severity       = 0x01;
> +       pm8001_ha->main_cfg_tbl.pm80xx_tbl.pcs_event_log_severity       =
> +               pcs_event_log_severity;
>         pm8001_ha->main_cfg_tbl.pm80xx_tbl.fatal_err_interrupt          = 0x01;
>
>         /* Enable higher IQs and OQs, 32 to 63, bit 16 */
> --
> 2.47.0.rc1.288.g06298d1525-goog
>

Hi,

3 works well for Google, but a different value might be better for
others. Having a module parameter would allow users to customize the
level of logging based on their specific needs. If that is not a
concern, I can change the default to just 3.

Thank you!

Salomon Dushimirimana


On Thu, Oct 17, 2024 at 2:45 AM Jinpu Wang <jinpu.wang@ionos.com> wrote:
>
> Hi,
>
> On Thu, Oct 17, 2024 at 12:10 AM Salomon Dushimirimana
> <salomondush@google.com> wrote:
> >
> > The pm8006 driver sets pcs event log threshold very high which causes
> > most of the FW logs to not be captured. This adds a module parameter to
> > configure pcs event log severity with 3 (medium severity) as the
> > default.
> upstream does not like more module parameters, can we just change the
> default to 3, any harm?
>
> Thx!
> >
> > Signed-off-by: Bhavesh Jashnani <bjashnani@google.com>
> > Signed-off-by: Salomon Dushimirimana <salomondush@google.com>
> > ---
> >  drivers/scsi/pm8001/pm8001_init.c | 4 ++++
> >  drivers/scsi/pm8001/pm8001_sas.h  | 2 ++
> >  drivers/scsi/pm8001/pm80xx_hwi.c  | 3 ++-
> >  3 files changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/scsi/pm8001/pm8001_init.c b/drivers/scsi/pm8001/pm8001_init.c
> > index 1e63cb6cd8e3..355aab0c982a 100644
> > --- a/drivers/scsi/pm8001/pm8001_init.c
> > +++ b/drivers/scsi/pm8001/pm8001_init.c
> > @@ -68,6 +68,10 @@ static bool pm8001_read_wwn = true;
> >  module_param_named(read_wwn, pm8001_read_wwn, bool, 0444);
> >  MODULE_PARM_DESC(zoned, "Get WWN from the controller. Default: true");
> >
> > +uint pcs_event_log_severity = 0x03;
> > +module_param(pcs_event_log_severity, int, 0644);
> > +MODULE_PARM_DESC(pcs_event_log_severity, "PCS event log severity level");
> > +
> >  static struct scsi_transport_template *pm8001_stt;
> >  static int pm8001_init_ccb_tag(struct pm8001_hba_info *);
> >
> > diff --git a/drivers/scsi/pm8001/pm8001_sas.h b/drivers/scsi/pm8001/pm8001_sas.h
> > index ced6721380a8..42c7b3f7afbf 100644
> > --- a/drivers/scsi/pm8001/pm8001_sas.h
> > +++ b/drivers/scsi/pm8001/pm8001_sas.h
> > @@ -96,6 +96,8 @@ extern struct list_head hba_list;
> >  extern const struct pm8001_dispatch pm8001_8001_dispatch;
> >  extern const struct pm8001_dispatch pm8001_80xx_dispatch;
> >
> > +extern uint pcs_event_log_severity;
> > +
> >  struct pm8001_hba_info;
> >  struct pm8001_ccb_info;
> >  struct pm8001_device;
> > diff --git a/drivers/scsi/pm8001/pm80xx_hwi.c b/drivers/scsi/pm8001/pm80xx_hwi.c
> > index 8fe886dc5e47..9b237a764d0b 100644
> > --- a/drivers/scsi/pm8001/pm80xx_hwi.c
> > +++ b/drivers/scsi/pm8001/pm80xx_hwi.c
> > @@ -763,7 +763,8 @@ static void init_default_table_values(struct pm8001_hba_info *pm8001_ha)
> >                 pm8001_ha->memoryMap.region[IOP].phys_addr_lo;
> >         pm8001_ha->main_cfg_tbl.pm80xx_tbl.pcs_event_log_size           =
> >                                                         PM8001_EVENT_LOG_SIZE;
> > -       pm8001_ha->main_cfg_tbl.pm80xx_tbl.pcs_event_log_severity       = 0x01;
> > +       pm8001_ha->main_cfg_tbl.pm80xx_tbl.pcs_event_log_severity       =
> > +               pcs_event_log_severity;
> >         pm8001_ha->main_cfg_tbl.pm80xx_tbl.fatal_err_interrupt          = 0x01;
> >
> >         /* Enable higher IQs and OQs, 32 to 63, bit 16 */
> > --
> > 2.47.0.rc1.288.g06298d1525-goog
> >

Salomon,

> 3 works well for Google, but a different value might be better for
> others. Having a module parameter would allow users to customize the
> level of logging based on their specific needs. If that is not a
> concern, I can change the default to just 3.

How verbose will a value of 3 be during normal operation? I don't object
to capturing more information during failure scenarios as long as we're
not flooding the logs with noise when things are nominal.

Hi,

These are the log levels and their descriptions for the pm80xx controllers -
0 - Disable logging
1 - Critical error
2 - Warning
3 - Notice
4 - Information
5 - Debugging
6 - Reserved

Loglevel 3 helps capture messages of severity level 3 and above. It is
not expected to be verbose and helps avoid the log overflow problem.

For debugging, there may be a need to change the loglevel, hence
making it configurable.

Thanks,
Salomon Dushimirimana

Salomon Dushimirimana


On Fri, Oct 25, 2024 at 11:45 AM Martin K. Petersen
<martin.petersen@oracle.com> wrote:
>
>
> Salomon,
>
> > 3 works well for Google, but a different value might be better for
> > others. Having a module parameter would allow users to customize the
> > level of logging based on their specific needs. If that is not a
> > concern, I can change the default to just 3.
>
> How verbose will a value of 3 be during normal operation? I don't object
> to capturing more information during failure scenarios as long as we're
> not flooding the logs with noise when things are nominal.
>
> --
> Martin K. Petersen      Oracle Linux Engineering

Salomon,

> The pm8006 driver sets pcs event log threshold very high which causes
> most of the FW logs to not be captured. This adds a module parameter
> to configure pcs event log severity with 3 (medium severity) as the
> default.

Applied to 6.13/scsi-staging, thanks!

On Wed, 16 Oct 2024 22:09:44 +0000, Salomon Dushimirimana wrote:

> The pm8006 driver sets pcs event log threshold very high which causes
> most of the FW logs to not be captured. This adds a module parameter to
> configure pcs event log severity with 3 (medium severity) as the
> default.
> 
> 

Applied to 6.13/scsi-queue, thanks!

[1/1] scsi: pm80xx: Use module param to set pcs event log severity
      https://git.kernel.org/mkp/scsi/c/c8d81a438544