2021-07-06 18:17:40

by Don Brace

[permalink] [raw]
Subject: [smartpqi updates PATCH 0/9] smartpqi updates

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(-)

--
2.28.0.rc1.9.ge7ae437ac1


2021-07-06 18:17:45

by Don Brace

[permalink] [raw]
Subject: [smartpqi updates PATCH 1/9] smartpqi: add pci id for H3C controller

From: Mahesh Rajashekhara <[email protected]>

Add support for H3C P4408-Ma-8i-2GB device ID
VID_9005, DID_028F, SVID_193D and SDID_1108

Reviewed-by: Kevin Barnett <[email protected]>
Reviewed-by: Mike McGowen <[email protected]>
Reviewed-by: Murthy Bhat <[email protected]>
Reviewed-by: Scott Benesh <[email protected]>
Reviewed-by: Scott Teel <[email protected]>
Signed-off-by: Mahesh Rajashekhara <[email protected]>
Signed-off-by: Don Brace <[email protected]>
---
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 dcc0b9618a64..d977c7b30d5c 100644
--- a/drivers/scsi/smartpqi/smartpqi_init.c
+++ b/drivers/scsi/smartpqi/smartpqi_init.c
@@ -8711,6 +8711,10 @@ static const struct pci_device_id pqi_pci_id_table[] = {
PCI_DEVICE_SUB(PCI_VENDOR_ID_ADAPTEC2, 0x028f,
0x193d, 0x1107)
},
+ {
+ PCI_DEVICE_SUB(PCI_VENDOR_ID_ADAPTEC2, 0x028f,
+ 0x193d, 0x1108)
+ },
{
PCI_DEVICE_SUB(PCI_VENDOR_ID_ADAPTEC2, 0x028f,
0x193d, 0x8460)
--
2.28.0.rc1.9.ge7ae437ac1

2021-07-06 18:17:50

by Don Brace

[permalink] [raw]
Subject: [smartpqi updates PATCH 3/9] smartpqi: update copyright notices

From: Kevin Barnett <[email protected]>

Updated copyright notices and company name strings to reflect
Microchip ownership.

Reviewed-by: Mike McGowen <[email protected]>
Reviewed-by: Scott Benesh <[email protected]>
Reviewed-by: Scott Teel <[email protected]>
Signed-off-by: Kevin Barnett <[email protected]>
Signed-off-by: Don Brace <[email protected]>
---
drivers/scsi/smartpqi/smartpqi.h | 6 +++---
drivers/scsi/smartpqi/smartpqi_init.c | 12 ++++++------
drivers/scsi/smartpqi/smartpqi_sas_transport.c | 4 ++--
drivers/scsi/smartpqi/smartpqi_sis.c | 4 ++--
drivers/scsi/smartpqi/smartpqi_sis.h | 4 ++--
5 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/scsi/smartpqi/smartpqi.h b/drivers/scsi/smartpqi/smartpqi.h
index d7dac5572274..f340afc011b5 100644
--- a/drivers/scsi/smartpqi/smartpqi.h
+++ b/drivers/scsi/smartpqi/smartpqi.h
@@ -1,7 +1,7 @@
/* SPDX-License-Identifier: GPL-2.0 */
/*
- * driver for Microsemi PQI-based storage controllers
- * Copyright (c) 2019-2020 Microchip Technology Inc. and its subsidiaries
+ * driver for Microchip PQI-based storage controllers
+ * Copyright (c) 2019-2021 Microchip Technology Inc. and its subsidiaries
* Copyright (c) 2016-2018 Microsemi Corporation
* Copyright (c) 2016 PMC-Sierra, Inc.
*
@@ -59,7 +59,7 @@ struct pqi_device_registers {
/*
* controller registers
*
- * These are defined by the Microsemi implementation.
+ * These are defined by the Microchip implementation.
*
* Some registers (those named sis_*) are only used when in
* legacy SIS mode before we transition the controller into
diff --git a/drivers/scsi/smartpqi/smartpqi_init.c b/drivers/scsi/smartpqi/smartpqi_init.c
index 7958316841a4..5ce1c41a758d 100644
--- a/drivers/scsi/smartpqi/smartpqi_init.c
+++ b/drivers/scsi/smartpqi/smartpqi_init.c
@@ -1,7 +1,7 @@
// SPDX-License-Identifier: GPL-2.0
/*
- * driver for Microsemi PQI-based storage controllers
- * Copyright (c) 2019-2020 Microchip Technology Inc. and its subsidiaries
+ * driver for Microchip PQI-based storage controllers
+ * Copyright (c) 2019-2021 Microchip Technology Inc. and its subsidiaries
* Copyright (c) 2016-2018 Microsemi Corporation
* Copyright (c) 2016 PMC-Sierra, Inc.
*
@@ -39,7 +39,7 @@
#define DRIVER_RELEASE 8
#define DRIVER_REVISION 45

-#define DRIVER_NAME "Microsemi PQI Driver (v" \
+#define DRIVER_NAME "Microchip SmartPQI Driver (v" \
DRIVER_VERSION BUILD_TIMESTAMP ")"
#define DRIVER_NAME_SHORT "smartpqi"

@@ -48,8 +48,8 @@
#define PQI_POST_RESET_DELAY_SECS 5
#define PQI_POST_OFA_RESET_DELAY_UPON_TIMEOUT_SECS 10

-MODULE_AUTHOR("Microsemi");
-MODULE_DESCRIPTION("Driver for Microsemi Smart Family Controller version "
+MODULE_AUTHOR("Microchip");
+MODULE_DESCRIPTION("Driver for Microchip Smart Family Controller version "
DRIVER_VERSION);
MODULE_VERSION(DRIVER_VERSION);
MODULE_LICENSE("GPL");
@@ -8448,7 +8448,7 @@ static void pqi_print_ctrl_info(struct pci_dev *pci_dev,
if (id->driver_data)
ctrl_description = (char *)id->driver_data;
else
- ctrl_description = "Microsemi Smart Family Controller";
+ ctrl_description = "Microchip Smart Family Controller";

dev_info(&pci_dev->dev, "%s found\n", ctrl_description);
}
diff --git a/drivers/scsi/smartpqi/smartpqi_sas_transport.c b/drivers/scsi/smartpqi/smartpqi_sas_transport.c
index dd628cc87f78..afd9bafebd1d 100644
--- a/drivers/scsi/smartpqi/smartpqi_sas_transport.c
+++ b/drivers/scsi/smartpqi/smartpqi_sas_transport.c
@@ -1,7 +1,7 @@
// SPDX-License-Identifier: GPL-2.0
/*
- * driver for Microsemi PQI-based storage controllers
- * Copyright (c) 2019-2020 Microchip Technology Inc. and its subsidiaries
+ * driver for Microchip PQI-based storage controllers
+ * Copyright (c) 2019-2021 Microchip Technology Inc. and its subsidiaries
* Copyright (c) 2016-2018 Microsemi Corporation
* Copyright (c) 2016 PMC-Sierra, Inc.
*
diff --git a/drivers/scsi/smartpqi/smartpqi_sis.c b/drivers/scsi/smartpqi/smartpqi_sis.c
index c954620628e0..d63c46a8e38b 100644
--- a/drivers/scsi/smartpqi/smartpqi_sis.c
+++ b/drivers/scsi/smartpqi/smartpqi_sis.c
@@ -1,7 +1,7 @@
// SPDX-License-Identifier: GPL-2.0
/*
- * driver for Microsemi PQI-based storage controllers
- * Copyright (c) 2019-2020 Microchip Technology Inc. and its subsidiaries
+ * driver for Microchip PQI-based storage controllers
+ * Copyright (c) 2019-2021 Microchip Technology Inc. and its subsidiaries
* Copyright (c) 2016-2018 Microsemi Corporation
* Copyright (c) 2016 PMC-Sierra, Inc.
*
diff --git a/drivers/scsi/smartpqi/smartpqi_sis.h b/drivers/scsi/smartpqi/smartpqi_sis.h
index 12cd2ab1aead..d29c1352a826 100644
--- a/drivers/scsi/smartpqi/smartpqi_sis.h
+++ b/drivers/scsi/smartpqi/smartpqi_sis.h
@@ -1,7 +1,7 @@
/* SPDX-License-Identifier: GPL-2.0 */
/*
- * driver for Microsemi PQI-based storage controllers
- * Copyright (c) 2019-2020 Microchip Technology Inc. and its subsidiaries
+ * driver for Microchip PQI-based storage controllers
+ * Copyright (c) 2019-2021 Microchip Technology Inc. and its subsidiaries
* Copyright (c) 2016-2018 Microsemi Corporation
* Copyright (c) 2016 PMC-Sierra, Inc.
*
--
2.28.0.rc1.9.ge7ae437ac1

2021-07-06 18:17:57

by Don Brace

[permalink] [raw]
Subject: [smartpqi updates PATCH 9/9] smartpqi: update version to 2.1.10-020

Reviewed-by: Kevin Barnett <[email protected]>
Reviewed-by: Mike McGowen <[email protected]>
Reviewed-by: Scott Benesh <[email protected]>
Reviewed-by: Scott Teel <[email protected]>
Reviewed-by: Gerry Morong <[email protected]>
Signed-off-by: Don Brace <[email protected]>
---
drivers/scsi/smartpqi/smartpqi_init.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/smartpqi/smartpqi_init.c b/drivers/scsi/smartpqi/smartpqi_init.c
index eeaf0568b5e3..a41acf8729a3 100644
--- a/drivers/scsi/smartpqi/smartpqi_init.c
+++ b/drivers/scsi/smartpqi/smartpqi_init.c
@@ -33,11 +33,11 @@
#define BUILD_TIMESTAMP
#endif

-#define DRIVER_VERSION "2.1.8-045"
+#define DRIVER_VERSION "2.1.10-020"
#define DRIVER_MAJOR 2
#define DRIVER_MINOR 1
-#define DRIVER_RELEASE 8
-#define DRIVER_REVISION 45
+#define DRIVER_RELEASE 10
+#define DRIVER_REVISION 20

#define DRIVER_NAME "Microchip SmartPQI Driver (v" \
DRIVER_VERSION BUILD_TIMESTAMP ")"
--
2.28.0.rc1.9.ge7ae437ac1

2021-07-06 18:18:00

by Don Brace

[permalink] [raw]
Subject: [smartpqi updates PATCH 2/9] smartpqi: rm unsupported controller features msgs

From: Kevin Barnett <[email protected]>

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.

Reviewed-by: Mike McGowen <[email protected]>
Reviewed-by: Scott Benesh <[email protected]>
Reviewed-by: Scott Teel <[email protected]>
Signed-off-by: Kevin Barnett <[email protected]>
Signed-off-by: Don Brace <[email protected]>
---
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,
--
2.28.0.rc1.9.ge7ae437ac1

2021-07-06 18:18:04

by Don Brace

[permalink] [raw]
Subject: [smartpqi updates PATCH 8/9] smartpqi: fix isr accessing null structure member

From: Mike McGowen <[email protected]>

Correct driver's ISR accessing a data structure member
that has not been fully initialized during driver init.
- The pqi queue groups can be null when an interrupt fires.

Reviewed-by: Kevin Barnett <[email protected]>
Reviewed-by: Scott Benesh <[email protected]>
Reviewed-by: Scott Teel <[email protected]>
Signed-off-by: Mike McGowen <[email protected]>
Signed-off-by: Don Brace <[email protected]>
---
drivers/scsi/smartpqi/smartpqi_init.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/smartpqi/smartpqi_init.c b/drivers/scsi/smartpqi/smartpqi_init.c
index 6f2263abaa8c..eeaf0568b5e3 100644
--- a/drivers/scsi/smartpqi/smartpqi_init.c
+++ b/drivers/scsi/smartpqi/smartpqi_init.c
@@ -7757,11 +7757,11 @@ static int pqi_ctrl_init(struct pqi_ctrl_info *ctrl_info)

pqi_init_operational_queues(ctrl_info);

- rc = pqi_request_irqs(ctrl_info);
+ rc = pqi_create_queues(ctrl_info);
if (rc)
return rc;

- rc = pqi_create_queues(ctrl_info);
+ rc = pqi_request_irqs(ctrl_info);
if (rc)
return rc;

--
2.28.0.rc1.9.ge7ae437ac1

2021-07-06 18:18:06

by Don Brace

[permalink] [raw]
Subject: [smartpqi updates PATCH 6/9] smartpqi: add PCI-ID for new Norsi controller

From: Mike McGowen <[email protected]>

Reviewed-by: Kevin Barnett <[email protected]>
Reviewed-by: Scott Benesh <[email protected]>
Reviewed-by: Scott Teel <[email protected]>
Signed-off-by: Mike McGowen <[email protected]>
Signed-off-by: Don Brace <[email protected]>
---
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)
--
2.28.0.rc1.9.ge7ae437ac1

2021-07-06 18:18:07

by Don Brace

[permalink] [raw]
Subject: [smartpqi updates PATCH 5/9] smartpqi: add PCI id for H3C P4408 controller

From: Balsundar P <[email protected]>

Added support for H3C P4408-Mr-8i-2GB device ID.
VID_9005, DID_028F, SVID_193D and SDID_1109

Reviewed-by: Kevin Barnett <[email protected]>
Reviewed-by: Mike McGowen <[email protected]>
Reviewed-by: Scott Benesh <[email protected]>
Reviewed-by: Scott Teel <[email protected]>
Signed-off-by: Balsundar P <[email protected]>
Signed-off-by: Don Brace <[email protected]>
---
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 c2ddb66b5c2d..1195e70b6ec3 100644
--- a/drivers/scsi/smartpqi/smartpqi_init.c
+++ b/drivers/scsi/smartpqi/smartpqi_init.c
@@ -8714,6 +8714,10 @@ static const struct pci_device_id pqi_pci_id_table[] = {
PCI_DEVICE_SUB(PCI_VENDOR_ID_ADAPTEC2, 0x028f,
0x193d, 0x1108)
},
+ {
+ PCI_DEVICE_SUB(PCI_VENDOR_ID_ADAPTEC2, 0x028f,
+ 0x193d, 0x1109)
+ },
{
PCI_DEVICE_SUB(PCI_VENDOR_ID_ADAPTEC2, 0x028f,
0x193d, 0x8460)
--
2.28.0.rc1.9.ge7ae437ac1

2021-07-06 18:20:54

by Don Brace

[permalink] [raw]
Subject: [smartpqi updates PATCH 7/9] smartpqi: add PCI IDs for new ZTE controllers

From: Balsundar P <[email protected]>

Added support for ZTE RM241-18i 2G device ID.
VID_9005, DID_028F, SVID_1CF2 and SDID_5445

Added support for ZTE RM242-18i 4G device ID.
VID_9005, DID_028F, SVID_1CF2 and SDID_5446

Added support for ZTE RM243-18i device ID.
VID_9005, DID_028F, SVID_1CF2 and SDID_5447

Added support for ZTE SDPSA/B-18i 4G device ID.
VID_9005, DID_028F, SVID_1CF2 and SDID_0B27

Added support for ZTE SDPSA/B_I-18i device ID.
VID_9005, DID_028F, SVID_1CF2 and SDID_0B29

Added support for ZTE SDPSA/B_L-18i 2G device ID.
VID_9005, DID_028F, SVID_1CF2 and SDID_0B45

Reviewed-by: Kevin Barnett <[email protected]>
Reviewed-by: Mike McGowen <[email protected]>
Reviewed-by: Scott Benesh <[email protected]>
Reviewed-by: Scott Teel <[email protected]>
Signed-off-by: Balsundar P <[email protected]>
Signed-off-by: Don Brace <[email protected]>
---
drivers/scsi/smartpqi/smartpqi_init.c | 24 ++++++++++++++++++++++++
1 file changed, 24 insertions(+)

diff --git a/drivers/scsi/smartpqi/smartpqi_init.c b/drivers/scsi/smartpqi/smartpqi_init.c
index a8dfb6101830..6f2263abaa8c 100644
--- a/drivers/scsi/smartpqi/smartpqi_init.c
+++ b/drivers/scsi/smartpqi/smartpqi_init.c
@@ -9182,6 +9182,30 @@ static const struct pci_device_id pqi_pci_id_table[] = {
PCI_DEVICE_SUB(PCI_VENDOR_ID_ADAPTEC2, 0x028f,
0x1dfc, 0x3161)
},
+ {
+ PCI_DEVICE_SUB(PCI_VENDOR_ID_ADAPTEC2, 0x028f,
+ 0x1cf2, 0x5445)
+ },
+ {
+ PCI_DEVICE_SUB(PCI_VENDOR_ID_ADAPTEC2, 0x028f,
+ 0x1cf2, 0x5446)
+ },
+ {
+ PCI_DEVICE_SUB(PCI_VENDOR_ID_ADAPTEC2, 0x028f,
+ 0x1cf2, 0x5447)
+ },
+ {
+ PCI_DEVICE_SUB(PCI_VENDOR_ID_ADAPTEC2, 0x028f,
+ 0x1cf2, 0x0b27)
+ },
+ {
+ PCI_DEVICE_SUB(PCI_VENDOR_ID_ADAPTEC2, 0x028f,
+ 0x1cf2, 0x0b29)
+ },
+ {
+ PCI_DEVICE_SUB(PCI_VENDOR_ID_ADAPTEC2, 0x028f,
+ 0x1cf2, 0x0b45)
+ },
{
PCI_DEVICE_SUB(PCI_VENDOR_ID_ADAPTEC2, 0x028f,
PCI_ANY_ID, PCI_ANY_ID)
--
2.28.0.rc1.9.ge7ae437ac1

2021-07-07 07:30:21

by Paul Menzel

[permalink] [raw]
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 <[email protected]>
>
> 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 <[email protected]>
> Reviewed-by: Scott Benesh <[email protected]>
> Reviewed-by: Scott Teel <[email protected]>
> Signed-off-by: Kevin Barnett <[email protected]>
> Signed-off-by: Don Brace <[email protected]>
> ---
> 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,
>

2021-07-07 07:36:11

by Paul Menzel

[permalink] [raw]
Subject: Re: [smartpqi updates PATCH 3/9] smartpqi: update copyright notices

Dear Kevin, dear Don,


Am 06.07.21 um 20:16 schrieb Don Brace:
> From: Kevin Barnett <[email protected]>
>
> Updated copyright notices and company name strings to reflect

… from Microsemi to Microchip …

> Microchip ownership.

You also change the driver name. Maybe do that in a separate commit with
a dedicated commit messages summary.

Name changes affecting strings showing up in log messages are always
confusing for people’s muscle memory. No idea, if it’d be better to
include both names in the driver name until the next Linux LTS series is
released.


Kind regards,

Paul


> Reviewed-by: Mike McGowen <[email protected]>
> Reviewed-by: Scott Benesh <[email protected]>
> Reviewed-by: Scott Teel <[email protected]>
> Signed-off-by: Kevin Barnett <[email protected]>
> Signed-off-by: Don Brace <[email protected]>
> ---
> drivers/scsi/smartpqi/smartpqi.h | 6 +++---
> drivers/scsi/smartpqi/smartpqi_init.c | 12 ++++++------
> drivers/scsi/smartpqi/smartpqi_sas_transport.c | 4 ++--
> drivers/scsi/smartpqi/smartpqi_sis.c | 4 ++--
> drivers/scsi/smartpqi/smartpqi_sis.h | 4 ++--
> 5 files changed, 15 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/scsi/smartpqi/smartpqi.h b/drivers/scsi/smartpqi/smartpqi.h
> index d7dac5572274..f340afc011b5 100644
> --- a/drivers/scsi/smartpqi/smartpqi.h
> +++ b/drivers/scsi/smartpqi/smartpqi.h
> @@ -1,7 +1,7 @@
> /* SPDX-License-Identifier: GPL-2.0 */
> /*
> - * driver for Microsemi PQI-based storage controllers
> - * Copyright (c) 2019-2020 Microchip Technology Inc. and its subsidiaries
> + * driver for Microchip PQI-based storage controllers
> + * Copyright (c) 2019-2021 Microchip Technology Inc. and its subsidiaries
> * Copyright (c) 2016-2018 Microsemi Corporation
> * Copyright (c) 2016 PMC-Sierra, Inc.
> *
> @@ -59,7 +59,7 @@ struct pqi_device_registers {
> /*
> * controller registers
> *
> - * These are defined by the Microsemi implementation.
> + * These are defined by the Microchip implementation.
> *
> * Some registers (those named sis_*) are only used when in
> * legacy SIS mode before we transition the controller into
> diff --git a/drivers/scsi/smartpqi/smartpqi_init.c b/drivers/scsi/smartpqi/smartpqi_init.c
> index 7958316841a4..5ce1c41a758d 100644
> --- a/drivers/scsi/smartpqi/smartpqi_init.c
> +++ b/drivers/scsi/smartpqi/smartpqi_init.c
> @@ -1,7 +1,7 @@
> // SPDX-License-Identifier: GPL-2.0
> /*
> - * driver for Microsemi PQI-based storage controllers
> - * Copyright (c) 2019-2020 Microchip Technology Inc. and its subsidiaries
> + * driver for Microchip PQI-based storage controllers
> + * Copyright (c) 2019-2021 Microchip Technology Inc. and its subsidiaries
> * Copyright (c) 2016-2018 Microsemi Corporation
> * Copyright (c) 2016 PMC-Sierra, Inc.
> *
> @@ -39,7 +39,7 @@
> #define DRIVER_RELEASE 8
> #define DRIVER_REVISION 45
>
> -#define DRIVER_NAME "Microsemi PQI Driver (v" \
> +#define DRIVER_NAME "Microchip SmartPQI Driver (v" \
> DRIVER_VERSION BUILD_TIMESTAMP ")"
> #define DRIVER_NAME_SHORT "smartpqi"
>
> @@ -48,8 +48,8 @@
> #define PQI_POST_RESET_DELAY_SECS 5
> #define PQI_POST_OFA_RESET_DELAY_UPON_TIMEOUT_SECS 10
>
> -MODULE_AUTHOR("Microsemi");
> -MODULE_DESCRIPTION("Driver for Microsemi Smart Family Controller version "
> +MODULE_AUTHOR("Microchip");
> +MODULE_DESCRIPTION("Driver for Microchip Smart Family Controller version "
> DRIVER_VERSION);
> MODULE_VERSION(DRIVER_VERSION);
> MODULE_LICENSE("GPL");
> @@ -8448,7 +8448,7 @@ static void pqi_print_ctrl_info(struct pci_dev *pci_dev,
> if (id->driver_data)
> ctrl_description = (char *)id->driver_data;
> else
> - ctrl_description = "Microsemi Smart Family Controller";
> + ctrl_description = "Microchip Smart Family Controller";
>
> dev_info(&pci_dev->dev, "%s found\n", ctrl_description);
> }
> diff --git a/drivers/scsi/smartpqi/smartpqi_sas_transport.c b/drivers/scsi/smartpqi/smartpqi_sas_transport.c
> index dd628cc87f78..afd9bafebd1d 100644
> --- a/drivers/scsi/smartpqi/smartpqi_sas_transport.c
> +++ b/drivers/scsi/smartpqi/smartpqi_sas_transport.c
> @@ -1,7 +1,7 @@
> // SPDX-License-Identifier: GPL-2.0
> /*
> - * driver for Microsemi PQI-based storage controllers
> - * Copyright (c) 2019-2020 Microchip Technology Inc. and its subsidiaries
> + * driver for Microchip PQI-based storage controllers
> + * Copyright (c) 2019-2021 Microchip Technology Inc. and its subsidiaries
> * Copyright (c) 2016-2018 Microsemi Corporation
> * Copyright (c) 2016 PMC-Sierra, Inc.
> *
> diff --git a/drivers/scsi/smartpqi/smartpqi_sis.c b/drivers/scsi/smartpqi/smartpqi_sis.c
> index c954620628e0..d63c46a8e38b 100644
> --- a/drivers/scsi/smartpqi/smartpqi_sis.c
> +++ b/drivers/scsi/smartpqi/smartpqi_sis.c
> @@ -1,7 +1,7 @@
> // SPDX-License-Identifier: GPL-2.0
> /*
> - * driver for Microsemi PQI-based storage controllers
> - * Copyright (c) 2019-2020 Microchip Technology Inc. and its subsidiaries
> + * driver for Microchip PQI-based storage controllers
> + * Copyright (c) 2019-2021 Microchip Technology Inc. and its subsidiaries
> * Copyright (c) 2016-2018 Microsemi Corporation
> * Copyright (c) 2016 PMC-Sierra, Inc.
> *
> diff --git a/drivers/scsi/smartpqi/smartpqi_sis.h b/drivers/scsi/smartpqi/smartpqi_sis.h
> index 12cd2ab1aead..d29c1352a826 100644
> --- a/drivers/scsi/smartpqi/smartpqi_sis.h
> +++ b/drivers/scsi/smartpqi/smartpqi_sis.h
> @@ -1,7 +1,7 @@
> /* SPDX-License-Identifier: GPL-2.0 */
> /*
> - * driver for Microsemi PQI-based storage controllers
> - * Copyright (c) 2019-2020 Microchip Technology Inc. and its subsidiaries
> + * driver for Microchip PQI-based storage controllers
> + * Copyright (c) 2019-2021 Microchip Technology Inc. and its subsidiaries
> * Copyright (c) 2016-2018 Microsemi Corporation
> * Copyright (c) 2016 PMC-Sierra, Inc.
> *
>

2021-07-07 07:38:51

by Paul Menzel

[permalink] [raw]
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 <[email protected]>

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 <[email protected]>
> Reviewed-by: Scott Benesh <[email protected]>
> Reviewed-by: Scott Teel <[email protected]>
> Signed-off-by: Mike McGowen <[email protected]>
> Signed-off-by: Don Brace <[email protected]>
> ---
> 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)
>

2021-07-07 07:51:32

by Paul Menzel

[permalink] [raw]
Subject: Re: [smartpqi updates PATCH 8/9] smartpqi: fix isr accessing null structure member

Dear Don, dear Mike,


Am 06.07.21 um 20:16 schrieb Don Brace:
> From: Mike McGowen <[email protected]>
>
> Correct driver's ISR accessing a data structure member
> that has not been fully initialized during driver init.

Does that crash the Linux kernel?

> - The pqi queue groups can be null when an interrupt fires.

If it fixes a crash(?), please add a Fixes: tag so it can be backported
to the stable series.

> Reviewed-by: Kevin Barnett <[email protected]>
> Reviewed-by: Scott Benesh <[email protected]>
> Reviewed-by: Scott Teel <[email protected]>
> Signed-off-by: Mike McGowen <[email protected]>
> Signed-off-by: Don Brace <[email protected]>
> ---
> drivers/scsi/smartpqi/smartpqi_init.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/scsi/smartpqi/smartpqi_init.c b/drivers/scsi/smartpqi/smartpqi_init.c
> index 6f2263abaa8c..eeaf0568b5e3 100644
> --- a/drivers/scsi/smartpqi/smartpqi_init.c
> +++ b/drivers/scsi/smartpqi/smartpqi_init.c
> @@ -7757,11 +7757,11 @@ static int pqi_ctrl_init(struct pqi_ctrl_info *ctrl_info)
>
> pqi_init_operational_queues(ctrl_info);
>
> - rc = pqi_request_irqs(ctrl_info);
> + rc = pqi_create_queues(ctrl_info);

From a quick look, these two functions are quite different. It’d be
great if you elaborated a bit in the commit message, what else the new
function does.

Also, with this change, `pqi_request_irqs()` does not seem to have any
users anymore. Without your patch:

$ git grep pqi_request_irqs
drivers/scsi/smartpqi/smartpqi_init.c:static int
pqi_request_irqs(struct pqi_ctrl_info *ctrl_info)
drivers/scsi/smartpqi/smartpqi_init.c: rc =
pqi_request_irqs(ctrl_info);

> if (rc)
> return rc;
>
> - rc = pqi_create_queues(ctrl_info);
> + rc = pqi_request_irqs(ctrl_info);
> if (rc)
> return rc;
>


Kind regards,

Paul

2021-07-08 19:05:49

by Don Brace

[permalink] [raw]
Subject: RE: [smartpqi updates PATCH 2/9] smartpqi: rm unsupported controller features msgs

-----Original Message-----
From: Paul Menzel [mailto:[email protected]]
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 <[email protected]>
>
> 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 <[email protected]>
> Reviewed-by: Scott Benesh <[email protected]>
> Reviewed-by: Scott Teel <[email protected]>
> Signed-off-by: Kevin Barnett <[email protected]>
> Signed-off-by: Don Brace <[email protected]>
> ---
> 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,
>

2021-07-08 19:11:57

by Don Brace

[permalink] [raw]
Subject: RE: [smartpqi updates PATCH 3/9] smartpqi: update copyright notices

-----Original Message-----
From: Paul Menzel [mailto:[email protected]]
Subject: Re: [smartpqi updates PATCH 3/9] smartpqi: update copyright notices

Dear Kevin, dear Don,


Am 06.07.21 um 20:16 schrieb Don Brace:
> From: Kevin Barnett <[email protected]>
>
> Updated copyright notices and company name strings to reflect

… from Microsemi to Microchip …

> Microchip ownership.

You also change the driver name. Maybe do that in a separate commit with a dedicated commit messages summary.

Don: I'll separate the driver name from the copyright patches and send up a new patch in V2.

Thanks for your review.


Name changes affecting strings showing up in log messages are always confusing for people’s muscle memory. No idea, if it’d be better to include both names in the driver name until the next Linux LTS series is released.

Don: We would rather just have one name.


Kind regards,

Paul


> Reviewed-by: Mike McGowen <[email protected]>
> Reviewed-by: Scott Benesh <[email protected]>
> Reviewed-by: Scott Teel <[email protected]>
> Signed-off-by: Kevin Barnett <[email protected]>
> Signed-off-by: Don Brace <[email protected]>
> ---
> drivers/scsi/smartpqi/smartpqi.h | 6 +++---
> drivers/scsi/smartpqi/smartpqi_init.c | 12 ++++++------
> drivers/scsi/smartpqi/smartpqi_sas_transport.c | 4 ++--
> drivers/scsi/smartpqi/smartpqi_sis.c | 4 ++--
> drivers/scsi/smartpqi/smartpqi_sis.h | 4 ++--
> 5 files changed, 15 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/scsi/smartpqi/smartpqi.h
> b/drivers/scsi/smartpqi/smartpqi.h
> index d7dac5572274..f340afc011b5 100644
> --- a/drivers/scsi/smartpqi/smartpqi.h
> +++ b/drivers/scsi/smartpqi/smartpqi.h
> @@ -1,7 +1,7 @@
> /* SPDX-License-Identifier: GPL-2.0 */
> /*
> - * driver for Microsemi PQI-based storage controllers
> - * Copyright (c) 2019-2020 Microchip Technology Inc. and its subsidiaries
> + * driver for Microchip PQI-based storage controllers
> + * Copyright (c) 2019-2021 Microchip Technology Inc. and its subsidiaries
> * Copyright (c) 2016-2018 Microsemi Corporation
> * Copyright (c) 2016 PMC-Sierra, Inc.
> *
> @@ -59,7 +59,7 @@ struct pqi_device_registers {
> /*
> * controller registers
> *
> - * These are defined by the Microsemi implementation.
> + * These are defined by the Microchip implementation.
> *
> * Some registers (those named sis_*) are only used when in
> * legacy SIS mode before we transition the controller into diff
> --git a/drivers/scsi/smartpqi/smartpqi_init.c
> b/drivers/scsi/smartpqi/smartpqi_init.c
> index 7958316841a4..5ce1c41a758d 100644
> --- a/drivers/scsi/smartpqi/smartpqi_init.c
> +++ b/drivers/scsi/smartpqi/smartpqi_init.c
> @@ -1,7 +1,7 @@
> // SPDX-License-Identifier: GPL-2.0
> /*
> - * driver for Microsemi PQI-based storage controllers
> - * Copyright (c) 2019-2020 Microchip Technology Inc. and its subsidiaries
> + * driver for Microchip PQI-based storage controllers
> + * Copyright (c) 2019-2021 Microchip Technology Inc. and its subsidiaries
> * Copyright (c) 2016-2018 Microsemi Corporation
> * Copyright (c) 2016 PMC-Sierra, Inc.
> *
> @@ -39,7 +39,7 @@
> #define DRIVER_RELEASE 8
> #define DRIVER_REVISION 45
>
> -#define DRIVER_NAME "Microsemi PQI Driver (v" \
> +#define DRIVER_NAME "Microchip SmartPQI Driver (v" \
> DRIVER_VERSION BUILD_TIMESTAMP ")"
> #define DRIVER_NAME_SHORT "smartpqi"
>
> @@ -48,8 +48,8 @@
> #define PQI_POST_RESET_DELAY_SECS 5
> #define PQI_POST_OFA_RESET_DELAY_UPON_TIMEOUT_SECS 10
>
> -MODULE_AUTHOR("Microsemi");
> -MODULE_DESCRIPTION("Driver for Microsemi Smart Family Controller version "
> +MODULE_AUTHOR("Microchip");
> +MODULE_DESCRIPTION("Driver for Microchip Smart Family Controller version "
> DRIVER_VERSION);
> MODULE_VERSION(DRIVER_VERSION);
> MODULE_LICENSE("GPL");
> @@ -8448,7 +8448,7 @@ static void pqi_print_ctrl_info(struct pci_dev *pci_dev,
> if (id->driver_data)
> ctrl_description = (char *)id->driver_data;
> else
> - ctrl_description = "Microsemi Smart Family Controller";
> + ctrl_description = "Microchip Smart Family Controller";
>
> dev_info(&pci_dev->dev, "%s found\n", ctrl_description);
> }
> diff --git a/drivers/scsi/smartpqi/smartpqi_sas_transport.c
> b/drivers/scsi/smartpqi/smartpqi_sas_transport.c
> index dd628cc87f78..afd9bafebd1d 100644
> --- a/drivers/scsi/smartpqi/smartpqi_sas_transport.c
> +++ b/drivers/scsi/smartpqi/smartpqi_sas_transport.c
> @@ -1,7 +1,7 @@
> // SPDX-License-Identifier: GPL-2.0
> /*
> - * driver for Microsemi PQI-based storage controllers
> - * Copyright (c) 2019-2020 Microchip Technology Inc. and its subsidiaries
> + * driver for Microchip PQI-based storage controllers
> + * Copyright (c) 2019-2021 Microchip Technology Inc. and its subsidiaries
> * Copyright (c) 2016-2018 Microsemi Corporation
> * Copyright (c) 2016 PMC-Sierra, Inc.
> *
> diff --git a/drivers/scsi/smartpqi/smartpqi_sis.c
> b/drivers/scsi/smartpqi/smartpqi_sis.c
> index c954620628e0..d63c46a8e38b 100644
> --- a/drivers/scsi/smartpqi/smartpqi_sis.c
> +++ b/drivers/scsi/smartpqi/smartpqi_sis.c
> @@ -1,7 +1,7 @@
> // SPDX-License-Identifier: GPL-2.0
> /*
> - * driver for Microsemi PQI-based storage controllers
> - * Copyright (c) 2019-2020 Microchip Technology Inc. and its subsidiaries
> + * driver for Microchip PQI-based storage controllers
> + * Copyright (c) 2019-2021 Microchip Technology Inc. and its subsidiaries
> * Copyright (c) 2016-2018 Microsemi Corporation
> * Copyright (c) 2016 PMC-Sierra, Inc.
> *
> diff --git a/drivers/scsi/smartpqi/smartpqi_sis.h
> b/drivers/scsi/smartpqi/smartpqi_sis.h
> index 12cd2ab1aead..d29c1352a826 100644
> --- a/drivers/scsi/smartpqi/smartpqi_sis.h
> +++ b/drivers/scsi/smartpqi/smartpqi_sis.h
> @@ -1,7 +1,7 @@
> /* SPDX-License-Identifier: GPL-2.0 */
> /*
> - * driver for Microsemi PQI-based storage controllers
> - * Copyright (c) 2019-2020 Microchip Technology Inc. and its subsidiaries
> + * driver for Microchip PQI-based storage controllers
> + * Copyright (c) 2019-2021 Microchip Technology Inc. and its subsidiaries
> * Copyright (c) 2016-2018 Microsemi Corporation
> * Copyright (c) 2016 PMC-Sierra, Inc.
> *
>

2021-07-08 21:20:05

by Don Brace

[permalink] [raw]
Subject: RE: [smartpqi updates PATCH 8/9] smartpqi: fix isr accessing null structure member

From: Paul Menzel [mailto:[email protected]]
Subject: Re: [smartpqi updates PATCH 8/9] smartpqi: fix isr accessing null structure member

Dear Don, dear Mike,


Am 06.07.21 um 20:16 schrieb Don Brace:
> From: Mike McGowen <[email protected]>
>
> Correct driver's ISR accessing a data structure member that has not
> been fully initialized during driver init.

Does that crash the Linux kernel?
Don:
No. I updated the title and description to reflect this. It resulted in some brief access to uninitialized members.
This was found during some internal testing, no bugs were ever filed for this change.

Thanks for your review.

> - The pqi queue groups can be null when an interrupt fires.

If it fixes a crash(?), please add a Fixes: tag so it can be backported to the stable series.



Kind regards,

Paul

2021-07-08 21:39:05

by Don Brace

[permalink] [raw]
Subject: RE: [smartpqi updates PATCH 6/9] smartpqi: add PCI-ID for new Norsi controller

From: Paul Menzel [mailto:[email protected]]
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 <[email protected]>

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 <[email protected]>
> Reviewed-by: Scott Benesh <[email protected]>
> Reviewed-by: Scott Teel <[email protected]>
> Signed-off-by: Mike McGowen <[email protected]>
> Signed-off-by: Don Brace <[email protected]>
> ---
> 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)
>

2021-07-09 07:26:17

by Paul Menzel

[permalink] [raw]
Subject: Re: [smartpqi updates PATCH 2/9] smartpqi: rm unsupported controller features msgs

[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 [email protected]:
> -----Original Message-----
> From: Paul Menzel [mailto:[email protected]]
> 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 <[email protected]>
>>
>> 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 <[email protected]>
>> Reviewed-by: Scott Benesh <[email protected]>
>> Reviewed-by: Scott Teel <[email protected]>
>> Signed-off-by: Kevin Barnett <[email protected]>
>> Signed-off-by: Don Brace <[email protected]>
>> ---
>> 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,
>>

2021-07-09 15:07:25

by Don Brace

[permalink] [raw]
Subject: RE: [smartpqi updates PATCH 2/9] smartpqi: rm unsupported controller features msgs

-----Original Message-----
From: Paul Menzel [mailto:[email protected]]

Dear Don,


Thank you for your reply.


Am 08.07.21 um 21:04 schrieb [email protected]:
> -----Original Message-----
> From: Paul Menzel [mailto:[email protected]]
> 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 <[email protected]>
>>
>> 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 <[email protected]>
>> Reviewed-by: Scott Benesh <[email protected]>
>> Reviewed-by: Scott Teel <[email protected]>
>> Signed-off-by: Kevin Barnett <[email protected]>
>> Signed-off-by: Don Brace <[email protected]>
>> ---
>> 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,
>>