| Message ID | 20210706181618.27960-1-don.brace@microchip.com |
|---|---|
| Headers | show |
| Series | smartpqi updates | expand |
Dear Kevin, dear Don, Am 06.07.21 um 20:16 schrieb Don Brace: > From: Kevin Barnett <kevin.barnett@microchip.com> > > Remove "Feature XYZ not supported by controller" messages. > > During driver initialization, the driver examines the PQI Table Feature bits. > These bits are used by the controller to advertise features supported by the > controller. For any features not supported by the controller, the driver would > display a message in the form: > "Feature XYZ not supported by controller" > Some of these "negative" messages were causing customer confusion. As it’s info log level and not warning or notice, these message are useful in my opinion. You could downgrade them to debug, but I do not see why. If customers do not want to see these info messages, they should filter them out. For completeness, is there an alternative to list the unsupported features from the firmware for example from sysfs? Kind regards, Paul > Reviewed-by: Mike McGowen <mike.mcgowen@microchip.com> > Reviewed-by: Scott Benesh <scott.benesh@microchip.com> > Reviewed-by: Scott Teel <scott.teel@microchip.com> > Signed-off-by: Kevin Barnett <kevin.barnett@microchip.com> > Signed-off-by: Don Brace <don.brace@microchip.com> > --- > drivers/scsi/smartpqi/smartpqi_init.c | 5 +---- > 1 file changed, 1 insertion(+), 4 deletions(-) > > diff --git a/drivers/scsi/smartpqi/smartpqi_init.c b/drivers/scsi/smartpqi/smartpqi_init.c > index d977c7b30d5c..7958316841a4 100644 > --- a/drivers/scsi/smartpqi/smartpqi_init.c > +++ b/drivers/scsi/smartpqi/smartpqi_init.c > @@ -7255,11 +7255,8 @@ struct pqi_firmware_feature { > static void pqi_firmware_feature_status(struct pqi_ctrl_info *ctrl_info, > struct pqi_firmware_feature *firmware_feature) > { > - if (!firmware_feature->supported) { > - dev_info(&ctrl_info->pci_dev->dev, "%s not supported by controller\n", > - firmware_feature->feature_name); > + if (!firmware_feature->supported) > return; > - } > > if (firmware_feature->enabled) { > dev_info(&ctrl_info->pci_dev->dev, >
Dear Murthy, dear Don, Am 06.07.21 um 20:16 schrieb Don Brace: > From: Murthy Bhat <Murthy.Bhat@microchip.com> > > Report on SCSI command that has triggered the reset. > - Also add check for 0 length SCSI commands. Can you please add an example log message line to the git commit message summary? Kind regards, Paul > Reviewed-by: Kevin Barnett <kevin.barnett@microchip.com> > Reviewed-by: Mike McGowen <mike.mcgowen@microchip.com> > Reviewed-by: Scott Benesh <scott.benesh@microchip.com> > Reviewed-by: Scott Teel <scott.teel@microchip.com> > Signed-off-by: Murthy Bhat <Murthy.Bhat@microchip.com> > Signed-off-by: Don Brace <don.brace@microchip.com> > --- > drivers/scsi/smartpqi/smartpqi_init.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/scsi/smartpqi/smartpqi_init.c b/drivers/scsi/smartpqi/smartpqi_init.c > index 5ce1c41a758d..c2ddb66b5c2d 100644 > --- a/drivers/scsi/smartpqi/smartpqi_init.c > +++ b/drivers/scsi/smartpqi/smartpqi_init.c > @@ -6033,8 +6033,10 @@ static int pqi_eh_device_reset_handler(struct scsi_cmnd *scmd) > mutex_lock(&ctrl_info->lun_reset_mutex); > > dev_err(&ctrl_info->pci_dev->dev, > - "resetting scsi %d:%d:%d:%d\n", > - shost->host_no, device->bus, device->target, device->lun); > + "resetting scsi %d:%d:%d:%d due to cmd 0x%02x\n", > + shost->host_no, > + device->bus, device->target, device->lun, > + scmd->cmd_len > 0 ? scmd->cmnd[0] : 0xff); > > pqi_check_ctrl_health(ctrl_info); > if (pqi_ctrl_offline(ctrl_info)) >
Dear Don, dear Mike, Am 06.07.21 um 20:16 schrieb Don Brace: > From: Mike McGowen <mike.mcgowen@microchip.com> Can you please mention the device ID and name in the git commit message, so it can be compared to the code change? See *[smartpqi updates PATCH 1/9] smartpqi: add pci id for H3C controller* as an example. Kind regards, Paul > Reviewed-by: Kevin Barnett <kevin.barnett@microchip.com> > Reviewed-by: Scott Benesh <scott.benesh@microchip.com> > Reviewed-by: Scott Teel <scott.teel@microchip.com> > Signed-off-by: Mike McGowen <mike.mcgowen@microchip.com> > Signed-off-by: Don Brace <don.brace@microchip.com> > --- > drivers/scsi/smartpqi/smartpqi_init.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/scsi/smartpqi/smartpqi_init.c b/drivers/scsi/smartpqi/smartpqi_init.c > index 1195e70b6ec3..a8dfb6101830 100644 > --- a/drivers/scsi/smartpqi/smartpqi_init.c > +++ b/drivers/scsi/smartpqi/smartpqi_init.c > @@ -9178,6 +9178,10 @@ static const struct pci_device_id pqi_pci_id_table[] = { > PCI_DEVICE_SUB(PCI_VENDOR_ID_ADAPTEC2, 0x028f, > PCI_VENDOR_ID_GIGABYTE, 0x1000) > }, > + { > + PCI_DEVICE_SUB(PCI_VENDOR_ID_ADAPTEC2, 0x028f, > + 0x1dfc, 0x3161) > + }, > { > PCI_DEVICE_SUB(PCI_VENDOR_ID_ADAPTEC2, 0x028f, > PCI_ANY_ID, PCI_ANY_ID) >
-----Original Message----- From: Paul Menzel [mailto:pmenzel@molgen.mpg.de] Sent: Wednesday, July 7, 2021 2:29 AM Subject: Re: [smartpqi updates PATCH 2/9] smartpqi: rm unsupported controller features msgs Dear Kevin, dear Don, Am 06.07.21 um 20:16 schrieb Don Brace: > From: Kevin Barnett <kevin.barnett@microchip.com> > > Remove "Feature XYZ not supported by controller" messages. > > During driver initialization, the driver examines the PQI Table Feature bits. > These bits are used by the controller to advertise features supported > by the controller. For any features not supported by the controller, > the driver would display a message in the form: > "Feature XYZ not supported by controller" > Some of these "negative" messages were causing customer confusion. As it’s info log level and not warning or notice, these message are useful in my opinion. You could downgrade them to debug, but I do not see why. If customers do not want to see these info messages, they should filter them out. For completeness, is there an alternative to list the unsupported features from the firmware for example from sysfs? Kind regards, Paul Don> Thanks for your Review. At this time we would prefer to not provide messages about unsupported features. We may add them back at some point but we have taken them out of our out-of-box driver also so we hope to keep the driver code in sync. > Reviewed-by: Mike McGowen <mike.mcgowen@microchip.com> > Reviewed-by: Scott Benesh <scott.benesh@microchip.com> > Reviewed-by: Scott Teel <scott.teel@microchip.com> > Signed-off-by: Kevin Barnett <kevin.barnett@microchip.com> > Signed-off-by: Don Brace <don.brace@microchip.com> > --- > drivers/scsi/smartpqi/smartpqi_init.c | 5 +---- > 1 file changed, 1 insertion(+), 4 deletions(-) > > diff --git a/drivers/scsi/smartpqi/smartpqi_init.c > b/drivers/scsi/smartpqi/smartpqi_init.c > index d977c7b30d5c..7958316841a4 100644 > --- a/drivers/scsi/smartpqi/smartpqi_init.c > +++ b/drivers/scsi/smartpqi/smartpqi_init.c > @@ -7255,11 +7255,8 @@ struct pqi_firmware_feature { > static void pqi_firmware_feature_status(struct pqi_ctrl_info *ctrl_info, > struct pqi_firmware_feature *firmware_feature) > { > - if (!firmware_feature->supported) { > - dev_info(&ctrl_info->pci_dev->dev, "%s not supported by controller\n", > - firmware_feature->feature_name); > + if (!firmware_feature->supported) > return; > - } > > if (firmware_feature->enabled) { > dev_info(&ctrl_info->pci_dev->dev, >
From: Paul Menzel [mailto:pmenzel@molgen.mpg.de] Subject: Re: [smartpqi updates PATCH 4/9] smartpqi: add SCSI cmd info for resets Dear Murthy, dear Don, Am 06.07.21 um 20:16 schrieb Don Brace: > From: Murthy Bhat <Murthy.Bhat@microchip.com> > > Report on SCSI command that has triggered the reset. > - Also add check for 0 length SCSI commands. Can you please add an example log message line to the git commit message summary? Don: Thanks for your review. I added an example. Kind regards, Paul > Reviewed-by: Kevin Barnett <kevin.barnett@microchip.com> > Reviewed-by: Mike McGowen <mike.mcgowen@microchip.com> > Reviewed-by: Scott Benesh <scott.benesh@microchip.com> > Reviewed-by: Scott Teel <scott.teel@microchip.com> > Signed-off-by: Murthy Bhat <Murthy.Bhat@microchip.com> > Signed-off-by: Don Brace <don.brace@microchip.com> > --- > drivers/scsi/smartpqi/smartpqi_init.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/scsi/smartpqi/smartpqi_init.c > b/drivers/scsi/smartpqi/smartpqi_init.c > index 5ce1c41a758d..c2ddb66b5c2d 100644 > --- a/drivers/scsi/smartpqi/smartpqi_init.c > +++ b/drivers/scsi/smartpqi/smartpqi_init.c > @@ -6033,8 +6033,10 @@ static int pqi_eh_device_reset_handler(struct scsi_cmnd *scmd) > mutex_lock(&ctrl_info->lun_reset_mutex); > > dev_err(&ctrl_info->pci_dev->dev, > - "resetting scsi %d:%d:%d:%d\n", > - shost->host_no, device->bus, device->target, device->lun); > + "resetting scsi %d:%d:%d:%d due to cmd 0x%02x\n", > + shost->host_no, > + device->bus, device->target, device->lun, > + scmd->cmd_len > 0 ? scmd->cmnd[0] : 0xff); > > pqi_check_ctrl_health(ctrl_info); > if (pqi_ctrl_offline(ctrl_info)) >
From: Paul Menzel [mailto:pmenzel@molgen.mpg.de] Subject: Re: [smartpqi updates PATCH 6/9] smartpqi: add PCI-ID for new Norsi controller Dear Don, dear Mike, Am 06.07.21 um 20:16 schrieb Don Brace: > From: Mike McGowen <mike.mcgowen@microchip.com> Can you please mention the device ID and name in the git commit message, so it can be compared to the code change? See *[smartpqi updates PATCH 1/9] smartpqi: add pci id for H3C controller* as an example. Kind regards, Paul Don: Updated patch title to reflect the controller. Added more to the patch description. Thanks for your review. > Reviewed-by: Kevin Barnett <kevin.barnett@microchip.com> > Reviewed-by: Scott Benesh <scott.benesh@microchip.com> > Reviewed-by: Scott Teel <scott.teel@microchip.com> > Signed-off-by: Mike McGowen <mike.mcgowen@microchip.com> > Signed-off-by: Don Brace <don.brace@microchip.com> > --- > drivers/scsi/smartpqi/smartpqi_init.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/scsi/smartpqi/smartpqi_init.c > b/drivers/scsi/smartpqi/smartpqi_init.c > index 1195e70b6ec3..a8dfb6101830 100644 > --- a/drivers/scsi/smartpqi/smartpqi_init.c > +++ b/drivers/scsi/smartpqi/smartpqi_init.c > @@ -9178,6 +9178,10 @@ static const struct pci_device_id pqi_pci_id_table[] = { > PCI_DEVICE_SUB(PCI_VENDOR_ID_ADAPTEC2, 0x028f, > PCI_VENDOR_ID_GIGABYTE, 0x1000) > }, > + { > + PCI_DEVICE_SUB(PCI_VENDOR_ID_ADAPTEC2, 0x028f, > + 0x1dfc, 0x3161) > + }, > { > PCI_DEVICE_SUB(PCI_VENDOR_ID_ADAPTEC2, 0x028f, > PCI_ANY_ID, PCI_ANY_ID) >
[I corrected Martin’s email from peterson to peters*e*n. Don, you should have also receive a bounce message from the MTA. I guess Martin has these as a list subscriber nevertheless, but I suggest to resend the series as soon as possible.] Dear Don, Thank you for your reply. Am 08.07.21 um 21:04 schrieb Don.Brace@microchip.com: > -----Original Message----- > From: Paul Menzel [mailto:pmenzel@molgen.mpg.de] > Sent: Wednesday, July 7, 2021 2:29 AM > Subject: Re: [smartpqi updates PATCH 2/9] smartpqi: rm unsupported controller features msgs > Am 06.07.21 um 20:16 schrieb Don Brace: >> From: Kevin Barnett <kevin.barnett@microchip.com> >> >> Remove "Feature XYZ not supported by controller" messages. >> >> During driver initialization, the driver examines the PQI Table Feature bits. >> These bits are used by the controller to advertise features supported >> by the controller. For any features not supported by the controller, >> the driver would display a message in the form: >> "Feature XYZ not supported by controller" >> Some of these "negative" messages were causing customer confusion. > > As it’s info log level and not warning or notice, these message are > useful in my opinion. You could downgrade them to debug, but I do not > see why. If customers do not want to see these info messages, they > should filter them out. > > For completeness, is there an alternative to list the unsupported > features from the firmware for example from sysfs? > Don> Thanks for your Review. At this time we would prefer to not > provide messages about unsupported features. Only because a customer complained? That is not a good enough reason in my opinion. Log messages, often grepped for, are an interface which should only be changed with caution. If these absent feature message were present for a long time, and you suddenly remove them, people looking a newer Linux kernel messages, users conclude the feature is supported now. That is quite a downside in my opinion. > We may add them back at some point but we have taken them out of our > out-of-box driver also so we hope to keep the driver code in sync. That’s why you should develop for Linux master branch and upstream *first* to get external reviews. That argument should not count for Linux upstream reviews in my opinion. Kind regards, Paul >> Reviewed-by: Mike McGowen <mike.mcgowen@microchip.com> >> Reviewed-by: Scott Benesh <scott.benesh@microchip.com> >> Reviewed-by: Scott Teel <scott.teel@microchip.com> >> Signed-off-by: Kevin Barnett <kevin.barnett@microchip.com> >> Signed-off-by: Don Brace <don.brace@microchip.com> >> --- >> drivers/scsi/smartpqi/smartpqi_init.c | 5 +---- >> 1 file changed, 1 insertion(+), 4 deletions(-) >> >> diff --git a/drivers/scsi/smartpqi/smartpqi_init.c >> b/drivers/scsi/smartpqi/smartpqi_init.c >> index d977c7b30d5c..7958316841a4 100644 >> --- a/drivers/scsi/smartpqi/smartpqi_init.c >> +++ b/drivers/scsi/smartpqi/smartpqi_init.c >> @@ -7255,11 +7255,8 @@ struct pqi_firmware_feature { >> static void pqi_firmware_feature_status(struct pqi_ctrl_info *ctrl_info, >> struct pqi_firmware_feature *firmware_feature) >> { >> - if (!firmware_feature->supported) { >> - dev_info(&ctrl_info->pci_dev->dev, "%s not supported by controller\n", >> - firmware_feature->feature_name); >> + if (!firmware_feature->supported) >> return; >> - } >> >> if (firmware_feature->enabled) { >> dev_info(&ctrl_info->pci_dev->dev, >>
-----Original Message----- From: Paul Menzel [mailto:pmenzel@molgen.mpg.de] Dear Don, Thank you for your reply. Am 08.07.21 um 21:04 schrieb Don.Brace@microchip.com: > -----Original Message----- > From: Paul Menzel [mailto:pmenzel@molgen.mpg.de] > Sent: Wednesday, July 7, 2021 2:29 AM > Subject: Re: [smartpqi updates PATCH 2/9] smartpqi: rm unsupported > controller features msgs > Am 06.07.21 um 20:16 schrieb Don Brace: >> From: Kevin Barnett <kevin.barnett@microchip.com> >> >> Remove "Feature XYZ not supported by controller" messages. >> >> During driver initialization, the driver examines the PQI Table Feature bits. >> These bits are used by the controller to advertise features supported >> by the controller. For any features not supported by the controller, >> the driver would display a message in the form: >> "Feature XYZ not supported by controller" >> Some of these "negative" messages were causing customer confusion. > > As it’s info log level and not warning or notice, these message are > useful in my opinion. You could downgrade them to debug, but I do not > see why. If customers do not want to see these info messages, they > should filter them out. > > For completeness, is there an alternative to list the unsupported > features from the firmware for example from sysfs? > Don> Thanks for your Review. At this time we would prefer to not > provide messages about unsupported features. Only because a customer complained? That is not a good enough reason in my opinion. Log messages, often grepped for, are an interface which should only be changed with caution. Don: It was many customers. Enough to have our customer support ask for the messages to be redacted. Also, some of the messages were erroneous causing yet more confusion. I'm sorry, but they have to be removed. If these absent feature message were present for a long time, and you suddenly remove them, people looking a newer Linux kernel messages, users conclude the feature is supported now. That is quite a downside in my opinion. > We may add them back at some point but we have taken them out of our > out-of-box driver also so we hope to keep the driver code in sync. That’s why you should develop for Linux master branch and upstream *first* to get external reviews. That argument should not count for Linux upstream reviews in my opinion. Don: Thanks for your suggestion. Our model has a lot of internal reviews, then several weeks or more of testing before we add patches to the kernel. More automated tests are added daily. Any changes from the kernel community are included in our model. Kind regards, Paul >> Reviewed-by: Mike McGowen <mike.mcgowen@microchip.com> >> Reviewed-by: Scott Benesh <scott.benesh@microchip.com> >> Reviewed-by: Scott Teel <scott.teel@microchip.com> >> Signed-off-by: Kevin Barnett <kevin.barnett@microchip.com> >> Signed-off-by: Don Brace <don.brace@microchip.com> >> --- >> drivers/scsi/smartpqi/smartpqi_init.c | 5 +---- >> 1 file changed, 1 insertion(+), 4 deletions(-) >> >> diff --git a/drivers/scsi/smartpqi/smartpqi_init.c >> b/drivers/scsi/smartpqi/smartpqi_init.c >> index d977c7b30d5c..7958316841a4 100644 >> --- a/drivers/scsi/smartpqi/smartpqi_init.c >> +++ b/drivers/scsi/smartpqi/smartpqi_init.c >> @@ -7255,11 +7255,8 @@ struct pqi_firmware_feature { >> static void pqi_firmware_feature_status(struct pqi_ctrl_info *ctrl_info, >> struct pqi_firmware_feature *firmware_feature) >> { >> - if (!firmware_feature->supported) { >> - dev_info(&ctrl_info->pci_dev->dev, "%s not supported by controller\n", >> - firmware_feature->feature_name); >> + if (!firmware_feature->supported) >> return; >> - } >> >> if (firmware_feature->enabled) { >> dev_info(&ctrl_info->pci_dev->dev, >>
These patches are based on Martin Peterson's 5.14/scsi-queue tree https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git 5.14/scsi-queue Most of these patches consist of adding new PCI devices. The remainder are simple updates to correct some rare issues and clean up some driver messages. This set of changes consist of: * Add in new PCI-IDs. 5 of these patches are adding in new PCI-IDs. * Removing unnecessary unsupported feature messages. * Update copyright information. * Enhance reset messages. - Add SCSI command CDB[0] value to message. - Also check for a 0 length SCSI command that can occur if sg_reset is issued without any outstanding SCSI commands. * Clean up a rare initialization issue where interrupts are enabled before the supporting controller context has been fully initialized causing NULL pointer access. * Update the driver version to 2.1.10-020 Balsundar P (2): smartpqi: add PCI id for H3C P4408 controller smartpqi: add PCI IDs for new ZTE controllers Don Brace (1): smartpqi: update version to 2.1.10-020 Kevin Barnett (2): smartpqi: rm unsupported controller features msgs smartpqi: update copyright notices Mahesh Rajashekhara (1): smartpqi: add pci id for H3C controller Mike McGowen (2): smartpqi: add PCI-ID for new Norsi controller smartpqi: fix isr accessing null structure member Murthy Bhat (1): smartpqi: add SCSI cmd info for resets drivers/scsi/smartpqi/smartpqi.h | 6 +- drivers/scsi/smartpqi/smartpqi_init.c | 69 ++++++++++++++----- .../scsi/smartpqi/smartpqi_sas_transport.c | 4 +- drivers/scsi/smartpqi/smartpqi_sis.c | 4 +- drivers/scsi/smartpqi/smartpqi_sis.h | 4 +- 5 files changed, 61 insertions(+), 26 deletions(-)