[PATCH RESEND 0/6] hpsa: support legacy boards

[PATCH RESEND 0/6] hpsa: support legacy boards

[Hannes Reinecke]

(Resend to include linux-scsi)

Hi all,

this patch adds support for legacy boards, ie for boards previously
supported by cciss only.
With this patchset the hpsa driver should work with all Smart Array
boards if the 'hpsa_allow_any' module option is set, rendering the
cciss driver obsolete.

Hannes Reinecke (5):
  hpsa: consolidate status variables
  hpsa: add support for legacy boards
  hpsa: disable volume status check for older controller
  hpsa: Ignore errors for unsupported LV_DEVICE_ID VPD page
  hpsa: do not print errors for unsupported report luns format

Jeff Mahoney (1):
  hpsa: handle unsupported devices more gracefully

 drivers/scsi/hpsa.c | 131 ++++++++++++++++++++++++++++++++++++++++++++--------
 drivers/scsi/hpsa.h |  57 ++++++++++++++++++++---
 2 files changed, 162 insertions(+), 26 deletions(-)

-- 
1.8.5.6

[PATCH 1/6] hpsa: consolidate status variables

[Hannes Reinecke]

Consolidate several boolean variable into one field.

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/hpsa.h | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/hpsa.h b/drivers/scsi/hpsa.h
index 1c49741..bd16f38 100644
--- a/drivers/scsi/hpsa.h
+++ b/drivers/scsi/hpsa.h
@@ -246,7 +246,6 @@ struct ctlr_info {
 	struct delayed_work monitor_ctlr_work;
 	struct delayed_work rescan_ctlr_work;
 	struct delayed_work event_monitor_work;
-	int remove_in_progress;
 	/* Address of h->q[x] is passed to intr handler to know which queue */
 	u8 q[MAX_REPLY_QUEUES];
 	char intrname[MAX_REPLY_QUEUES][16];	/* "hpsa0-msix00" names */
@@ -289,18 +288,19 @@ struct ctlr_info {
 		CTLR_STATE_CHANGE_EVENT_AIO_CONFIG_CHANGE)
 	spinlock_t offline_device_lock;
 	struct list_head offline_device_list;
-	int	acciopath_status;
-	int	drv_req_rescan;
+	unsigned int acciopath_status:1;
+	unsigned int drv_req_rescan:1;
+	unsigned int discovery_polling:1;
+	unsigned int reset_in_progress:1;
+	unsigned int needs_abort_tags_swizzled:1;
+	unsigned int remove_in_progress:1;
 	int	raid_offload_debug;
-	int     discovery_polling;
 	struct  ReportLUNdata *lastlogicals;
-	int	needs_abort_tags_swizzled;
 	struct workqueue_struct *resubmit_wq;
 	struct workqueue_struct *rescan_ctlr_wq;
 	atomic_t abort_cmds_available;
 	wait_queue_head_t event_sync_wait_queue;
 	struct mutex reset_mutex;
-	u8 reset_in_progress;
 	struct hpsa_sas_node *sas_host;
 	spinlock_t reset_lock;
 };
-- 
1.8.5.6

[PATCH 2/6] hpsa: add support for legacy boards

[Hannes Reinecke]

Add support for legacy boards, ensuring to enable the driver for
those boards only when 'hpsa_allow_any' is set.

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/hpsa.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++-------
 drivers/scsi/hpsa.h | 45 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 92 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
index 8914eab..7ca6078 100644
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -148,6 +148,8 @@
 	{PCI_VENDOR_ID_HP, 0x333f, 0x103c, 0x333f},
 	{PCI_VENDOR_ID_HP,     PCI_ANY_ID,	PCI_ANY_ID, PCI_ANY_ID,
 		PCI_CLASS_STORAGE_RAID << 8, 0xffff << 8, 0},
+	{PCI_VENDOR_ID_COMPAQ,     PCI_ANY_ID,	PCI_ANY_ID, PCI_ANY_ID,
+		PCI_CLASS_STORAGE_RAID << 8, 0xffff << 8, 0},
 	{0,}
 };
 
@@ -158,6 +160,26 @@
  *  access = Address of the struct of function pointers
  */
 static struct board_type products[] = {
+	{0x40700E11, "Smart Array 5300", &SA5A_access},
+	{0x40800E11, "Smart Array 5i", &SA5B_access},
+	{0x40820E11, "Smart Array 532", &SA5B_access},
+	{0x40830E11, "Smart Array 5312", &SA5B_access},
+	{0x409A0E11, "Smart Array 641", &SA5A_access},
+	{0x409B0E11, "Smart Array 642", &SA5A_access},
+	{0x409C0E11, "Smart Array 6400", &SA5A_access},
+	{0x409D0E11, "Smart Array 6400 EM", &SA5A_access},
+	{0x40910E11, "Smart Array 6i", &SA5A_access},
+	{0x3225103C, "Smart Array P600", &SA5A_access},
+	{0x3223103C, "Smart Array P800", &SA5A_access},
+	{0x3234103C, "Smart Array P400", &SA5A_access},
+	{0x3235103C, "Smart Array P400i", &SA5A_access},
+	{0x3211103C, "Smart Array E200i", &SA5A_access},
+	{0x3212103C, "Smart Array E200", &SA5A_access},
+	{0x3213103C, "Smart Array E200i", &SA5A_access},
+	{0x3214103C, "Smart Array E200i", &SA5A_access},
+	{0x3215103C, "Smart Array E200i", &SA5A_access},
+	{0x3237103C, "Smart Array E500", &SA5A_access},
+	{0x323D103C, "Smart Array P700m", &SA5A_access},
 	{0x3241103C, "Smart Array P212", &SA5_access},
 	{0x3243103C, "Smart Array P410", &SA5_access},
 	{0x3245103C, "Smart Array P410i", &SA5_access},
@@ -278,7 +300,8 @@ static int hpsa_find_cfg_addrs(struct pci_dev *pdev, void __iomem *vaddr,
 			       u64 *cfg_offset);
 static int hpsa_pci_find_memory_BAR(struct pci_dev *pdev,
 				    unsigned long *memory_bar);
-static int hpsa_lookup_board_id(struct pci_dev *pdev, u32 *board_id);
+static int hpsa_lookup_board_id(struct pci_dev *pdev, u32 *board_id,
+				bool *supported);
 static int wait_for_device_to_become_ready(struct ctlr_info *h,
 					   unsigned char lunaddr[],
 					   int reply_queue);
@@ -7232,7 +7255,8 @@ static int hpsa_interrupt_mode(struct ctlr_info *h)
 	return 0;
 }
 
-static int hpsa_lookup_board_id(struct pci_dev *pdev, u32 *board_id)
+static int hpsa_lookup_board_id(struct pci_dev *pdev, u32 *board_id,
+				bool *supported)
 {
 	int i;
 	u32 subsystem_vendor_id, subsystem_device_id;
@@ -7242,9 +7266,22 @@ static int hpsa_lookup_board_id(struct pci_dev *pdev, u32 *board_id)
 	*board_id = ((subsystem_device_id << 16) & 0xffff0000) |
 		    subsystem_vendor_id;
 
+	if (supported)
+		*supported = true;
 	for (i = 0; i < ARRAY_SIZE(products); i++)
-		if (*board_id == products[i].board_id)
-			return i;
+		if (*board_id == products[i].board_id) {
+			if (products[i].access != &SA5A_access &&
+			    products[i].access != &SA5B_access)
+				return i;
+			if (hpsa_allow_any) {
+				dev_warn(&pdev->dev,
+					 "unsupported board ID: 0x%08x\n",
+					 *board_id);
+				if (supported)
+					*supported = false;
+				return i;
+			}
+		}
 
 	if ((subsystem_vendor_id != PCI_VENDOR_ID_HP &&
 		subsystem_vendor_id != PCI_VENDOR_ID_COMPAQ) ||
@@ -7253,6 +7290,8 @@ static int hpsa_lookup_board_id(struct pci_dev *pdev, u32 *board_id)
 			"0x%08x, ignoring.\n", *board_id);
 			return -ENODEV;
 	}
+	if (supported)
+		*supported = false;
 	return ARRAY_SIZE(products) - 1; /* generic unknown smart array */
 }
 
@@ -7555,13 +7594,14 @@ static void hpsa_free_pci_init(struct ctlr_info *h)
 static int hpsa_pci_init(struct ctlr_info *h)
 {
 	int prod_index, err;
+	bool supported;
 
-	prod_index = hpsa_lookup_board_id(h->pdev, &h->board_id);
+	prod_index = hpsa_lookup_board_id(h->pdev, &h->board_id, &supported);
 	if (prod_index < 0)
 		return prod_index;
 	h->product_name = products[prod_index].product_name;
 	h->access = *(products[prod_index].access);
-
+	h->supported = supported;
 	pci_disable_link_state(h->pdev, PCIE_LINK_STATE_L0S |
 			       PCIE_LINK_STATE_L1 | PCIE_LINK_STATE_CLKPM);
 
@@ -8241,7 +8281,7 @@ static int hpsa_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	if (number_of_controllers == 0)
 		printk(KERN_INFO DRIVER_NAME "\n");
 
-	rc = hpsa_lookup_board_id(pdev, &board_id);
+	rc = hpsa_lookup_board_id(pdev, &board_id, NULL);
 	if (rc < 0) {
 		dev_warn(&pdev->dev, "Board ID not found\n");
 		return rc;
diff --git a/drivers/scsi/hpsa.h b/drivers/scsi/hpsa.h
index bd16f38..ccec1e6 100644
--- a/drivers/scsi/hpsa.h
+++ b/drivers/scsi/hpsa.h
@@ -294,6 +294,7 @@ struct ctlr_info {
 	unsigned int reset_in_progress:1;
 	unsigned int needs_abort_tags_swizzled:1;
 	unsigned int remove_in_progress:1;
+	unsigned int supported:1;
 	int	raid_offload_debug;
 	struct  ReportLUNdata *lastlogicals;
 	struct workqueue_struct *resubmit_wq;
@@ -447,6 +448,25 @@ static void SA5_intr_mask(struct ctlr_info *h, unsigned long val)
 	}
 }
 
+/*
+ *  This card is the opposite of the other cards.
+ *   0 turns interrupts on...
+ *   0x04 turns them off...
+ */
+static void SA5B_intr_mask(struct ctlr_info *h, unsigned long val)
+{
+	if (val) { /* Turn interrupts on */
+		h->interrupts_enabled = 1;
+		writel(0, h->vaddr + SA5_REPLY_INTR_MASK_OFFSET);
+		(void) readl(h->vaddr + SA5_REPLY_INTR_MASK_OFFSET);
+	} else { /* Turn them off */
+		h->interrupts_enabled = 0;
+		writel(SA5B_INTR_OFF,
+		       h->vaddr + SA5_REPLY_INTR_MASK_OFFSET);
+		(void) readl(h->vaddr + SA5_REPLY_INTR_MASK_OFFSET);
+	}
+}
+
 static void SA5_performant_intr_mask(struct ctlr_info *h, unsigned long val)
 {
 	if (val) { /* turn on interrupts */
@@ -549,6 +569,16 @@ static bool SA5_ioaccel_mode1_intr_pending(struct ctlr_info *h)
 		true : false;
 }
 
+/*
+ *      Returns true if an interrupt is pending..
+ */
+static bool SA5B_intr_pending(struct ctlr_info *h)
+{
+	unsigned long register_value  =
+		readl(h->vaddr + SA5_INTR_STATUS);
+	return (register_value & SA5B_INTR_PENDING);
+}
+
 #define IOACCEL_MODE1_REPLY_QUEUE_INDEX  0x1A0
 #define IOACCEL_MODE1_PRODUCER_INDEX     0x1B8
 #define IOACCEL_MODE1_CONSUMER_INDEX     0x1BC
@@ -587,6 +617,21 @@ static unsigned long SA5_ioaccel_mode1_completed(struct ctlr_info *h, u8 q)
 	.command_completed = SA5_completed,
 };
 
+/* Duplicate entry of the above to mark unsupported boards */
+static struct access_method SA5A_access = {
+	.submit_command = SA5_submit_command,
+	.set_intr_mask = SA5_intr_mask,
+	.intr_pending = SA5_intr_pending,
+	.command_completed = SA5_completed,
+};
+
+static struct access_method SA5B_access = {
+	.submit_command = SA5_submit_command,
+	.set_intr_mask = SA5B_intr_mask,
+	.intr_pending = SA5B_intr_pending,
+	.command_completed = SA5_completed,
+};
+
 static struct access_method SA5_ioaccel_mode1_access = {
 	.submit_command = SA5_submit_command,
 	.set_intr_mask = SA5_performant_intr_mask,
-- 
1.8.5.6

[PATCH 3/6] hpsa: disable volume status check for older controller

[Hannes Reinecke]

Older Controller might not support volume status, so assume
the volume is online here.

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/hpsa.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
index 7ca6078..4ebf5d4 100644
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -3832,6 +3832,17 @@ static int hpsa_update_device_info(struct ctlr_info *h,
 		if (h->fw_support & MISC_FW_RAID_OFFLOAD_BASIC)
 			hpsa_get_ioaccel_status(h, scsi3addr, this_device);
 		volume_offline = hpsa_volume_offline(h, scsi3addr);
+		if (volume_offline == HPSA_VPD_LV_STATUS_UNSUPPORTED &&
+		    !h->supported) {
+			/*
+			 * Older / unsupported controllers might not support
+			 * volume status
+			 */
+			dev_info(&h->pdev->dev,
+				 "C0:T%d:L%d Volume status not available, assuming online.\n",
+				 this_device->target, this_device->lun);
+			volume_offline = 0;
+		}
 		this_device->volume_offline = volume_offline;
 		if (volume_offline == HPSA_LV_FAILED) {
 			rc = HPSA_LV_FAILED;
-- 
1.8.5.6

[PATCH 4/6] hpsa: Ignore errors for unsupported LV_DEVICE_ID VPD page

[Hannes Reinecke]

Older controller do not support the LV_DEVICE_ID VPD page, so
we shouldn't print out an error message here.

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/hpsa.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
index 4ebf5d4..dbd5fbc 100644
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -3814,7 +3814,7 @@ static int hpsa_update_device_info(struct ctlr_info *h,
 	memset(this_device->device_id, 0,
 		sizeof(this_device->device_id));
 	if (hpsa_get_device_id(h, scsi3addr, this_device->device_id, 8,
-		sizeof(this_device->device_id)))
+		sizeof(this_device->device_id) < 0))
 		dev_err(&h->pdev->dev,
 			"hpsa%d: %s: can't get device id for host %d:C0:T%d:L%d\t%s\t%.16s\n",
 			h->ctlr, __func__,
-- 
1.8.5.6

[PATCH 5/6] hpsa: do not print errors for unsupported report luns format

[Hannes Reinecke]

Older/Unsupported controllers might not support the 'extended'
report luns format, but as they are unsupported we don't need
to print out an error here.

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/hpsa.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
index dbd5fbc..790958f 100644
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -3588,7 +3588,7 @@ static int hpsa_scsi_do_report_luns(struct ctlr_info *h, int logical,
 	memset(scsi3addr, 0, sizeof(scsi3addr));
 	if (fill_cmd(c, logical ? HPSA_REPORT_LOG : HPSA_REPORT_PHYS, h,
 		buf, bufsize, 0, scsi3addr, TYPE_CMD)) {
-		rc = -1;
+		rc = -EAGAIN;
 		goto out;
 	}
 	if (extended_response)
@@ -3601,16 +3601,17 @@ static int hpsa_scsi_do_report_luns(struct ctlr_info *h, int logical,
 	if (ei->CommandStatus != 0 &&
 	    ei->CommandStatus != CMD_DATA_UNDERRUN) {
 		hpsa_scsi_interpret_error(h, c);
-		rc = -1;
+		rc = -EIO;
 	} else {
 		struct ReportLUNdata *rld = buf;
 
 		if (rld->extended_response_flag != extended_response) {
-			dev_err(&h->pdev->dev,
-				"report luns requested format %u, got %u\n",
-				extended_response,
-				rld->extended_response_flag);
-			rc = -1;
+			if (h->supported)
+				dev_err(&h->pdev->dev,
+					"report luns requested format %u, got %u\n",
+					extended_response,
+					rld->extended_response_flag);
+			rc = -EOPNOTSUPP;
 		}
 	}
 out:
@@ -3626,7 +3627,10 @@ static inline int hpsa_scsi_do_report_phys_luns(struct ctlr_info *h,
 
 	rc = hpsa_scsi_do_report_luns(h, 0, buf, bufsize,
 				      HPSA_REPORT_PHYS_EXTENDED);
-	if (!rc || !hpsa_allow_any)
+	if (rc == -EOPNOTSUPP) {
+		if (h->supported)
+			return rc;
+	} else if (rc < 0)
 		return rc;
 
 	/* REPORT PHYS EXTENDED is not supported */
@@ -6605,7 +6609,6 @@ static int fill_cmd(struct CommandList *c, u8 cmd, struct ctlr_info *h,
 		default:
 			dev_warn(&h->pdev->dev, "unknown command 0x%c\n", cmd);
 			BUG();
-			return -1;
 		}
 	} else if (cmd_type == TYPE_MSG) {
 		switch (cmd) {
-- 
1.8.5.6

[PATCH 6/6] hpsa: handle unsupported devices more gracefully

[Hannes Reinecke]

From: Jeff Mahoney <jeffm@suse.com>

Add a warning message if an unsupported device is found and the
hpsa_allow_any parameter is not set.
Also make the hpsa_allow_any parameter writeable once the hpsa
driver is loaded.

Signed-off-by: Jeff Mahoney <jeffm@suse.com>
---
 drivers/scsi/hpsa.c | 43 ++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 40 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
index 790958f..d523ec0 100644
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -82,10 +82,19 @@
 MODULE_VERSION(HPSA_DRIVER_VERSION);
 MODULE_LICENSE("GPL");
 
-static int hpsa_allow_any;
-module_param(hpsa_allow_any, int, S_IRUGO|S_IWUSR);
+static bool hpsa_allow_any = false;
+static bool hpsa_claimed_unsupported = false;
+static int hpsa_set_hpsa_allow_any(const char *buffer,
+				   const struct kernel_param *kp);
+static struct kernel_param_ops hpsa_allow_any_ops = {
+	.set = hpsa_set_hpsa_allow_any,
+	.get = param_get_bool,
+};
+
+module_param_cb(hpsa_allow_any, &hpsa_allow_any_ops, &hpsa_allow_any,
+		S_IRUGO|S_IWUSR);
 MODULE_PARM_DESC(hpsa_allow_any,
-		"Allow hpsa driver to access unknown HP Smart Array hardware");
+		"Allow hpsa driver to access unsupported HP Smart Array hardware");
 static int hpsa_simple_mode;
 module_param(hpsa_simple_mode, int, S_IRUGO|S_IWUSR);
 MODULE_PARM_DESC(hpsa_simple_mode,
@@ -7302,10 +7311,12 @@ static int hpsa_lookup_board_id(struct pci_dev *pdev, u32 *board_id,
 		!hpsa_allow_any) {
 		dev_warn(&pdev->dev, "unrecognized board ID: "
 			"0x%08x, ignoring.\n", *board_id);
+		dev_warn(&pdev->dev, "This device may be enabled by loading the hpsa module with the hpsa_allow_any=1 option or by writing \"%s\" to /sys/bus/pci/drivers/hpsa/bind while the module is loaded. Please note that the driver is untested with this device and will result in an unsupported environment.\n", dev_name(&pdev->dev));
 			return -ENODEV;
 	}
 	if (supported)
 		*supported = false;
+	hpsa_claimed_unsupported = true;
 	return ARRAY_SIZE(products) - 1; /* generic unknown smart array */
 }
 
@@ -9622,5 +9633,31 @@ static void __attribute__((unused)) verify_offsets(void)
 #undef VERIFY_OFFSET
 }
 
+static int hpsa_set_hpsa_allow_any(const char *buffer,
+				   const struct kernel_param *kp)
+{
+	int ret;
+	struct kernel_param dummy_kp = *kp;
+	bool newval;
+
+	dummy_kp.arg = &newval;
+
+	ret = param_set_bool(buffer, &dummy_kp);
+	if (ret)
+		return ret;
+
+	if (hpsa_allow_any && !newval) {
+		if (hpsa_claimed_unsupported) {
+			pr_info("hpsa: can't disable hpsa_allow_any parameter. Devices already in use.\n");
+			return -EPERM;
+		} else
+			hpsa_allow_any = false;
+	} else if (!hpsa_allow_any && newval) {
+		pr_info("hpsa: allowing unsupported devices. If devices are claimed, this will result in an unsupported environment.\n");
+		hpsa_allow_any = true;
+	}
+	return 0;
+}
+
 module_init(hpsa_init);
 module_exit(hpsa_cleanup);
-- 
1.8.5.6

Re: [PATCH 1/6] hpsa: consolidate status variables

[Christoph Hellwig]

These seem to be modified from sysfs files, so you'll run into
read-modify-write atomicy issues with bitfields.

That being said while plain ints are somewhat safe the right thing
would be to use proper atomic bitops.

Re: [PATCH 2/6] hpsa: add support for legacy boards

[Christoph Hellwig]

On Tue, Aug 08, 2017 at 10:35:11AM +0200, Hannes Reinecke wrote:
> Add support for legacy boards, ensuring to enable the driver for
> those boards only when 'hpsa_allow_any' is set.

Why the wildcard instead of the specific IDs from the cciss driver?

Re: [PATCH 3/6] hpsa: disable volume status check for older controller

[Christoph Hellwig]

Why that weird supported flag check?

(which also means I need to got back to the previous patch and understand
that part..)

Re: [PATCH 2/6] hpsa: add support for legacy boards

[Christoph Hellwig]

> -static int hpsa_lookup_board_id(struct pci_dev *pdev, u32 *board_id)
> +static int hpsa_lookup_board_id(struct pci_dev *pdev, u32 *board_id,
> +				bool *supported)
>  {
>  	int i;
>  	u32 subsystem_vendor_id, subsystem_device_id;
> @@ -7242,9 +7266,22 @@ static int hpsa_lookup_board_id(struct pci_dev *pdev, u32 *board_id)
>  	*board_id = ((subsystem_device_id << 16) & 0xffff0000) |
>  		    subsystem_vendor_id;
>  
> +	if (supported)
> +		*supported = true;
>  	for (i = 0; i < ARRAY_SIZE(products); i++)
> -		if (*board_id == products[i].board_id)
> -			return i;
> +		if (*board_id == products[i].board_id) {
> +			if (products[i].access != &SA5A_access &&
> +			    products[i].access != &SA5B_access)
> +				return i;
> +			if (hpsa_allow_any) {
> +				dev_warn(&pdev->dev,
> +					 "unsupported board ID: 0x%08x\n",
> +					 *board_id);
> +				if (supported)
> +					*supported = false;
> +				return i;
> +			}
> +		}

Can you explain the point of the supported flag?

> +	unsigned long register_value  =
> +		readl(h->vaddr + SA5_INTR_STATUS);
> +	return (register_value & SA5B_INTR_PENDING);

This should be condensed into:

	return readl(h->vaddr + SA5_INTR_STATUS) & SA5B_INTR_PENDING;

>  	.command_completed = SA5_completed,
>  };
>  
> +/* Duplicate entry of the above to mark unsupported boards */
> +static struct access_method SA5A_access = {
> +	.submit_command = SA5_submit_command,
> +	.set_intr_mask = SA5_intr_mask,
> +	.intr_pending = SA5_intr_pending,
> +	.command_completed = SA5_completed,
> +};
> +
> +static struct access_method SA5B_access = {
> +	.submit_command = SA5_submit_command,
> +	.set_intr_mask = SA5B_intr_mask,
> +	.intr_pending = SA5B_intr_pending,
> +	.command_completed = SA5_completed,
> +};

Please align the fields nicely, e.g.:

static struct access_method SA5A_access = {
	.submit_command 	= SA5_submit_command,
	.set_intr_mask		= SA5_intr_mask,
	...

Re: [PATCH 4/6] hpsa: Ignore errors for unsupported LV_DEVICE_ID VPD page

[Christoph Hellwig]

Looks fine,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 5/6] hpsa: do not print errors for unsupported report luns format

[Christoph Hellwig]

I don't like the misnamed supported flag.  Either we should always ignore
the errors, or key it off a specific flag for newer firmware.

Re: [PATCH 6/6] hpsa: handle unsupported devices more gracefully

[Christoph Hellwig]

On Tue, Aug 08, 2017 at 10:35:15AM +0200, Hannes Reinecke wrote:
> From: Jeff Mahoney <jeffm@suse.com>
> 
> Add a warning message if an unsupported device is found and the
> hpsa_allow_any parameter is not set.
> Also make the hpsa_allow_any parameter writeable once the hpsa
> driver is loaded.

This looks ok, but maybe we should also plan for just setting the
flag by defauly sooner or later?

> +static struct kernel_param_ops hpsa_allow_any_ops = {

const?

> +
> +	if (hpsa_allow_any && !newval) {
> +		if (hpsa_claimed_unsupported) {
> +			pr_info("hpsa: can't disable hpsa_allow_any parameter. Devices already in use.\n");
> +			return -EPERM;
> +		} else
> +			hpsa_allow_any = false;
> +	} else if (!hpsa_allow_any && newval) {
> +		pr_info("hpsa: allowing unsupported devices. If devices are claimed, this will result in an unsupported environment.\n");
> +		hpsa_allow_any = true;
> +	}
> +	return 0;

Do we really need this to start with?  It's normal that module
parameters only affect newly probed devices when changed at runtime,
so I think we could just stick to the simple module parameter with
permissions that allow runtime changes.

Re: [PATCH 2/6] hpsa: add support for legacy boards

[Hannes Reinecke]

On 08/09/2017 03:45 PM, Christoph Hellwig wrote:
>> -static int hpsa_lookup_board_id(struct pci_dev *pdev, u32 *board_id)
>> +static int hpsa_lookup_board_id(struct pci_dev *pdev, u32 *board_id,
>> +				bool *supported)
>>  {
>>  	int i;
>>  	u32 subsystem_vendor_id, subsystem_device_id;
>> @@ -7242,9 +7266,22 @@ static int hpsa_lookup_board_id(struct pci_dev *pdev, u32 *board_id)
>>  	*board_id = ((subsystem_device_id << 16) & 0xffff0000) |
>>  		    subsystem_vendor_id;
>>  
>> +	if (supported)
>> +		*supported = true;
>>  	for (i = 0; i < ARRAY_SIZE(products); i++)
>> -		if (*board_id == products[i].board_id)
>> -			return i;
>> +		if (*board_id == products[i].board_id) {
>> +			if (products[i].access != &SA5A_access &&
>> +			    products[i].access != &SA5B_access)
>> +				return i;
>> +			if (hpsa_allow_any) {
>> +				dev_warn(&pdev->dev,
>> +					 "unsupported board ID: 0x%08x\n",
>> +					 *board_id);
>> +				if (supported)
>> +					*supported = false;
>> +				return i;
>> +			}
>> +		}
> 
> Can you explain the point of the supported flag?
> 
'hpsa_allow_any' just enables the _driver_ to bind to older /
unsupported boards, it doesn't tell us if this particular instance is an
old board.
For older boards we should suppress messages from not-implemented
features, whereas on 'real' boards those features should be implemented,
and we should be throwing an error or a warning.
Hence the =supported" flag.

>> +	unsigned long register_value  =
>> +		readl(h->vaddr + SA5_INTR_STATUS);
>> +	return (register_value & SA5B_INTR_PENDING);
> 
> This should be condensed into:
> 
> 	return readl(h->vaddr + SA5_INTR_STATUS) & SA5B_INTR_PENDING;
> 
Yeah; but this is _actually_ just copied over, so other lines will be
affected here, too.
Will be adding a patch for that.

>>  	.command_completed = SA5_completed,
>>  };
>>  
>> +/* Duplicate entry of the above to mark unsupported boards */
>> +static struct access_method SA5A_access = {
>> +	.submit_command = SA5_submit_command,
>> +	.set_intr_mask = SA5_intr_mask,
>> +	.intr_pending = SA5_intr_pending,
>> +	.command_completed = SA5_completed,
>> +};
>> +
>> +static struct access_method SA5B_access = {
>> +	.submit_command = SA5_submit_command,
>> +	.set_intr_mask = SA5B_intr_mask,
>> +	.intr_pending = SA5B_intr_pending,
>> +	.command_completed = SA5_completed,
>> +};
> 
> Please align the fields nicely, e.g.:
> 
> static struct access_method SA5A_access = {
> 	.submit_command 	= SA5_submit_command,
> 	.set_intr_mask		= SA5_intr_mask,
> 	...
> 
Okay.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH 5/6] hpsa: do not print errors for unsupported report luns format

[Hannes Reinecke]

On 08/09/2017 03:46 PM, Christoph Hellwig wrote:
> I don't like the misnamed supported flag.  Either we should always ignore
> the errors, or key it off a specific flag for newer firmware.
> 
Well, there is no such hardware flag; there are several features
implemented across the board. Hence we need to store it in the driver
structure, which actually is the reason for the 'supported' flag.

I can rename it to 'legacy_board' or the like if 'supported' is not an
acceptable naming ...

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH 2/6] hpsa: add support for legacy boards

[Hannes Reinecke]

On 08/09/2017 03:41 PM, Christoph Hellwig wrote:
> On Tue, Aug 08, 2017 at 10:35:11AM +0200, Hannes Reinecke wrote:
>> Add support for legacy boards, ensuring to enable the driver for
>> those boards only when 'hpsa_allow_any' is set.
> 
> Why the wildcard instead of the specific IDs from the cciss driver?
> 
Because I'm lazy? And we're catching those boards with the wildcard
entry anyway? And the distinction between wildcard entry (designed to
catch unsupported devices) and pci IDs from the cciss drivers (which
will be unsupported, too) is meaningless?

But if you insist ...

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH 2/6] hpsa: add support for legacy boards

[Christoph Hellwig]

On Wed, Aug 09, 2017 at 05:08:05PM +0200, Hannes Reinecke wrote:
> On 08/09/2017 03:41 PM, Christoph Hellwig wrote:
> > On Tue, Aug 08, 2017 at 10:35:11AM +0200, Hannes Reinecke wrote:
> >> Add support for legacy boards, ensuring to enable the driver for
> >> those boards only when 'hpsa_allow_any' is set.
> > 
> > Why the wildcard instead of the specific IDs from the cciss driver?
> > 
> Because I'm lazy? And we're catching those boards with the wildcard
> entry anyway? And the distinction between wildcard entry (designed to
> catch unsupported devices) and pci IDs from the cciss drivers (which
> will be unsupported, too) is meaningless?

So what again is unsupported?

How do we know there never was any other compaq device claiming to be
raid class?

RE: [PATCH RESEND 0/6] hpsa: support legacy boards

[Don Brace]

> -----Original Message-----
> From: Hannes Reinecke [mailto:hare@suse.de]
> Sent: Tuesday, August 08, 2017 3:35 AM
> To: Martin K. Petersen <martin.petersen@oracle.com>
> Cc: Don Brace <don.brace@microsemi.com>; Christoph Hellwig
> <hch@lst.de>; James Bottomley
> <james.bottomley@hansenpartnership.com>; Meelis Roos
> <mroos@linux.ee>; linux-scsi@vger.kernel.org; Hannes Reinecke
> <hare@suse.de>
> Subject: [PATCH RESEND 0/6] hpsa: support legacy boards
> 
> EXTERNAL EMAIL
> 
> 
> (Resend to include linux-scsi)
> 
> Hi all,
> 
> this patch adds support for legacy boards, ie for boards previously
> supported by cciss only.
> With this patchset the hpsa driver should work with all Smart Array
> boards if the 'hpsa_allow_any' module option is set, rendering the
> cciss driver obsolete.
> 
> Hannes Reinecke (5):
>   hpsa: consolidate status variables
>   hpsa: add support for legacy boards
>   hpsa: disable volume status check for older controller
>   hpsa: Ignore errors for unsupported LV_DEVICE_ID VPD page
>   hpsa: do not print errors for unsupported report luns format
> 
> Jeff Mahoney (1):
>   hpsa: handle unsupported devices more gracefully
> 
>  drivers/scsi/hpsa.c | 131
> ++++++++++++++++++++++++++++++++++++++++++++--------
>  drivers/scsi/hpsa.h |  57 ++++++++++++++++++++---
>  2 files changed, 162 insertions(+), 26 deletions(-)
> 
> --
> 1.8.5.6

NACK this series

We do not want to support these older controllers in the hpsa driver.

While the driver may load and manage requests under healthy conditions
many features will not be supported. Customers will not have support
for any issues involving these controllers.

We would need to track these changes in our OOB driver adding
more confusion.

For more information please see:
http://cciss.sourceforge.net


Thanks,
Don Brace
ESC - Smart Storage
Microsemi Corporation

Re: [PATCH RESEND 0/6] hpsa: support legacy boards

[Hannes Reinecke]

On 08/10/2017 12:08 AM, Don Brace wrote:
>> -----Original Message-----
>> From: Hannes Reinecke [mailto:hare@suse.de]
>> Sent: Tuesday, August 08, 2017 3:35 AM
>> To: Martin K. Petersen <martin.petersen@oracle.com>
>> Cc: Don Brace <don.brace@microsemi.com>; Christoph Hellwig
>> <hch@lst.de>; James Bottomley
>> <james.bottomley@hansenpartnership.com>; Meelis Roos
>> <mroos@linux.ee>; linux-scsi@vger.kernel.org; Hannes Reinecke
>> <hare@suse.de>
>> Subject: [PATCH RESEND 0/6] hpsa: support legacy boards
>>
>> EXTERNAL EMAIL
>>
>>
>> (Resend to include linux-scsi)
>>
>> Hi all,
>>
>> this patch adds support for legacy boards, ie for boards previously
>> supported by cciss only.
>> With this patchset the hpsa driver should work with all Smart Array
>> boards if the 'hpsa_allow_any' module option is set, rendering the
>> cciss driver obsolete.
>>
>> Hannes Reinecke (5):
>>   hpsa: consolidate status variables
>>   hpsa: add support for legacy boards
>>   hpsa: disable volume status check for older controller
>>   hpsa: Ignore errors for unsupported LV_DEVICE_ID VPD page
>>   hpsa: do not print errors for unsupported report luns format
>>
>> Jeff Mahoney (1):
>>   hpsa: handle unsupported devices more gracefully
>>
>>  drivers/scsi/hpsa.c | 131
>> ++++++++++++++++++++++++++++++++++++++++++++--------
>>  drivers/scsi/hpsa.h |  57 ++++++++++++++++++++---
>>  2 files changed, 162 insertions(+), 26 deletions(-)
>>
>> --
>> 1.8.5.6
> 
> NACK this series
> 
> We do not want to support these older controllers in the hpsa driver.
> 
> While the driver may load and manage requests under healthy conditions
> many features will not be supported. Customers will not have support
> for any issues involving these controllers.
> 
> We would need to track these changes in our OOB driver adding
> more confusion.
> 
Weelll ... that is _precisely_ why I introduced the 'unsupported' flag
to the hba structure; once this is set we have encountered an
unsupported board, and things might not work as expected.

More importantly, you don't have to support that board.

We can even add a stronger wording if you like, eg printing out a big
fat warning in dmesg
'This board is unsupported, some features might not work'.

So where's the bother here?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH RESEND 0/6] hpsa: support legacy boards

[Christoph Hellwig]

No device support in Linux is unsupported, sorry.  I think we're
getting into the corporate bullshit game a little too much here.

Re: [PATCH RESEND 0/6] hpsa: support legacy boards

[Hannes Reinecke]

On 08/10/2017 09:09 AM, Christoph Hellwig wrote:
> No device support in Linux is unsupported, sorry.  I think we're
> getting into the corporate bullshit game a little too much here.
> 
Well, yes and no.

My intention with the 'unsupported' flag was to differentiate legit
errors / warnings from known / expected failures.

Modern (or 'supported' as Don puts it) boards have quite some features
built in, and any failure to activate these features should be debugged.
Older (or 'unsupported') boards might not necessarily have these
features, so a failure here is actually to be expected, seeing that a
firmware update for these boards is not likely to happen.

And this was precisely the meaning of the 'unsupported' flag; if that is
set we'll blank out some known warnings or short-circuit feature
detections which are known not to be present. Plus we have a marker
(and, with the latest patch, even a dmesg warning) letting us
differentiate between legit errors and unsupported/unimplemented features.

Personally I'm happy to support the legacy side of things (ie blanking
out errors from unsupported controllers), if that is of any help ...

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH RESEND 0/6] hpsa: support legacy boards

[James Bottomley]

On Thu, 2017-08-10 at 09:09 +0200, Christoph Hellwig wrote:
> No device support in Linux is unsupported, sorry.  I think we're
> getting into the corporate bullshit game a little too much here.

I think there are two different definitions of supported here.  To us,
any device to which the driver attaches is "supported".  However, if
it's never been tested before it may not work very well.  In the Linux
way, we'll try to fix the bugs when they're reported and in that sense
we support the device until nothing in the kernel attaches to its ids
anymore.

In the corporate world "supported" means we'll sell you a contract
giving you certain rights to report bugs and have us fix them.  There
are definite reasons why corporations only support a small range of new
devices, even though devices not on this list may still be attached to
by the driver and thus we (Linux Community) would try to fix the bug
reports for.

I think what you're basically asking for is a different name for the
flag, which is fine?  how about 'legacy' instead?

James

Re: [PATCH RESEND 0/6] hpsa: support legacy boards

[Hannes Reinecke]

On 08/10/2017 04:06 PM, James Bottomley wrote:
> On Thu, 2017-08-10 at 09:09 +0200, Christoph Hellwig wrote:
>> No device support in Linux is unsupported, sorry.  I think we're
>> getting into the corporate bullshit game a little too much here.
> 
> I think there are two different definitions of supported here.  To us,
> any device to which the driver attaches is "supported".  However, if
> it's never been tested before it may not work very well.  In the Linux
> way, we'll try to fix the bugs when they're reported and in that sense
> we support the device until nothing in the kernel attaches to its ids
> anymore.
> 
> In the corporate world "supported" means we'll sell you a contract
> giving you certain rights to report bugs and have us fix them.  There
> are definite reasons why corporations only support a small range of new
> devices, even though devices not on this list may still be attached to
> by the driver and thus we (Linux Community) would try to fix the bug
> reports for.
> 
> I think what you're basically asking for is a different name for the
> flag, which is fine?  how about 'legacy' instead?
> 
Sure, no problem with that.

Don?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

RE: [PATCH RESEND 0/6] hpsa: support legacy boards

[Don Brace]

> -----Original Message-----
> From: Hannes Reinecke [mailto:hare@suse.de]
> Sent: Thursday, August 10, 2017 9:11 AM
> To: James Bottomley <James.Bottomley@HansenPartnership.com>;
> Christoph Hellwig <hch@lst.de>
> Cc: Don Brace <don.brace@microsemi.com>; Martin K. Petersen
> <martin.petersen@oracle.com>; Meelis Roos <mroos@linux.ee>; linux-
> scsi@vger.kernel.org
> Subject: Re: [PATCH RESEND 0/6] hpsa: support legacy boards
> 
> EXTERNAL EMAIL
> 
> 
> On 08/10/2017 04:06 PM, James Bottomley wrote:
> > On Thu, 2017-08-10 at 09:09 +0200, Christoph Hellwig wrote:
> >> No device support in Linux is unsupported, sorry.  I think we're
> >> getting into the corporate bullshit game a little too much here.
> >
> > I think there are two different definitions of supported here.  To us,
> > any device to which the driver attaches is "supported".  However, if
> > it's never been tested before it may not work very well.  In the Linux
> > way, we'll try to fix the bugs when they're reported and in that sense
> > we support the device until nothing in the kernel attaches to its ids
> > anymore.
> >
> > In the corporate world "supported" means we'll sell you a contract
> > giving you certain rights to report bugs and have us fix them.  There
> > are definite reasons why corporations only support a small range of new
> > devices, even though devices not on this list may still be attached to
> > by the driver and thus we (Linux Community) would try to fix the bug
> > reports for.
> >
> > I think what you're basically asking for is a different name for the
> > flag, which is fine?  how about 'legacy' instead?
> >
> Sure, no problem with that.
> 
> Don?
> 
> Cheers,
> 
> Hannes
> --
> Dr. Hannes Reinecke                Teamlead Storage & Networking
> hare@suse.de                                   +49 911 74053 688
> SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
> GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
> HRB 21284 (AG Nürnberg)

Ok, but to clarify...
 * Will the cciss driver be removed when these patches are applied? Otherwise
    what will prevent the cciss driver from loading over these devices?
    i.e. how often will users need to be reminded to add cciss.cciss_allow_hpsa flag?

Re: [PATCH RESEND 0/6] hpsa: support legacy boards

[Hannes Reinecke]

On 08/10/2017 09:15 PM, Don Brace wrote:
>> -----Original Message-----
>> From: Hannes Reinecke [mailto:hare@suse.de]
>> Sent: Thursday, August 10, 2017 9:11 AM
>> To: James Bottomley <James.Bottomley@HansenPartnership.com>;
>> Christoph Hellwig <hch@lst.de>
>> Cc: Don Brace <don.brace@microsemi.com>; Martin K. Petersen
>> <martin.petersen@oracle.com>; Meelis Roos <mroos@linux.ee>; linux-
>> scsi@vger.kernel.org
>> Subject: Re: [PATCH RESEND 0/6] hpsa: support legacy boards
>>
>> EXTERNAL EMAIL
>>
>>
>> On 08/10/2017 04:06 PM, James Bottomley wrote:
>>> On Thu, 2017-08-10 at 09:09 +0200, Christoph Hellwig wrote:
>>>> No device support in Linux is unsupported, sorry.  I think we're
>>>> getting into the corporate bullshit game a little too much here.
>>>
>>> I think there are two different definitions of supported here.  To us,
>>> any device to which the driver attaches is "supported".  However, if
>>> it's never been tested before it may not work very well.  In the Linux
>>> way, we'll try to fix the bugs when they're reported and in that sense
>>> we support the device until nothing in the kernel attaches to its ids
>>> anymore.
>>>
>>> In the corporate world "supported" means we'll sell you a contract
>>> giving you certain rights to report bugs and have us fix them.  There
>>> are definite reasons why corporations only support a small range of new
>>> devices, even though devices not on this list may still be attached to
>>> by the driver and thus we (Linux Community) would try to fix the bug
>>> reports for.
>>>
>>> I think what you're basically asking for is a different name for the
>>> flag, which is fine?  how about 'legacy' instead?
>>>
>> Sure, no problem with that.
>>
>> Don?
>>
>> Cheers,
>>
>> Hannes
>> --
>> Dr. Hannes Reinecke                Teamlead Storage & Networking
>> hare@suse.de                                   +49 911 74053 688
>> SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
>> GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
>> HRB 21284 (AG Nürnberg)
> 
> Ok, but to clarify...
>  * Will the cciss driver be removed when these patches are applied? Otherwise
>     what will prevent the cciss driver from loading over these devices?
>     i.e. how often will users need to be reminded to add cciss.cciss_allow_hpsa flag?
> 
> 
The idea is to remove the cciss driver completely once these changes are
in. Preferably with the same patchset to avoid any timing issues.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH 3/6] hpsa: disable volume status check for older controller

[Tomas Henzl]

On 8.8.2017 10:35, Hannes Reinecke wrote:
> Older Controller might not support volume status, so assume
> the volume is online here.
>
> Signed-off-by: Hannes Reinecke <hare@suse.com>
> ---
>  drivers/scsi/hpsa.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
> index 7ca6078..4ebf5d4 100644
> --- a/drivers/scsi/hpsa.c
> +++ b/drivers/scsi/hpsa.c
> @@ -3832,6 +3832,17 @@ static int hpsa_update_device_info(struct ctlr_info *h,
>  		if (h->fw_support & MISC_FW_RAID_OFFLOAD_BASIC)
>  			hpsa_get_ioaccel_status(h, scsi3addr, this_device);
>  		volume_offline = hpsa_volume_offline(h, scsi3addr);
> +		if (volume_offline == HPSA_VPD_LV_STATUS_UNSUPPORTED &&
> +		    !h->supported) {
> +			/*
> +			 * Older / unsupported controllers might not support
> +			 * volume status
> +			 */
> +			dev_info(&h->pdev->dev,
> +				 "C0:T%d:L%d Volume status not available, assuming online.\n",
> +				 this_device->target, this_device->lun);
> +			volume_offline = 0;

Hi,
could we have here
volume_offline = HPSA_LV_OK;
instead ?
tomash

> +		}
>  		this_device->volume_offline = volume_offline;
>  		if (volume_offline == HPSA_LV_FAILED) {
>  			rc = HPSA_LV_FAILED;

RE: [PATCH RESEND 0/6] hpsa: support legacy boards

[Don Brace]

> -----Original Message-----
> From: Hannes Reinecke [mailto:hare@suse.de]
> Sent: Friday, August 11, 2017 1:43 AM
> To: Don Brace <don.brace@microsemi.com>; James Bottomley
> <James.Bottomley@HansenPartnership.com>; Christoph Hellwig
> <hch@lst.de>
> Cc: Martin K. Petersen <martin.petersen@oracle.com>; Meelis Roos
> <mroos@linux.ee>; linux-scsi@vger.kernel.org
> Subject: Re: [PATCH RESEND 0/6] hpsa: support legacy boards
> 
> EXTERNAL EMAIL

...
> >
> > Ok, but to clarify...
> >  * Will the cciss driver be removed when these patches are applied?
> Otherwise
> >     what will prevent the cciss driver from loading over these devices?
> >     i.e. how often will users need to be reminded to add
> cciss.cciss_allow_hpsa flag?
> >
> >
> The idea is to remove the cciss driver completely once these changes are
> in. Preferably with the same patchset to avoid any timing issues.
> 
> Cheers,
> 
> Hannes
> --
> Dr. Hannes Reinecke                Teamlead Storage & Networking
> hare@suse.de                                   +49 911 74053 688
> SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
> GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
> HRB 21284 (AG Nürnberg)

OK, then I ACK the series once the reviews by others have been resolved.
Thank-you and  everyone else for your patches and attention to this issue.
And thanks to everyone for your input.

Acked-by: Don Brace <don.brace@microsemi.com>

Re: [PATCH 1/6] hpsa: consolidate status variables

[Hannes Reinecke]

On 08/09/2017 03:40 PM, Christoph Hellwig wrote:
> These seem to be modified from sysfs files, so you'll run into
> read-modify-write atomicy issues with bitfields.
> 
> That being said while plain ints are somewhat safe the right thing
> would be to use proper atomic bitops.
> 
True, but rewriting that into atomics would change the flow of the
driver by quite a bit (as quite some spinlocks can be removed),
so I'd rather _not_ attempt this now.

So I'll be dropping this patch.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH 3/6] hpsa: disable volume status check for older controller

[Hannes Reinecke]

On 08/11/2017 03:23 PM, Tomas Henzl wrote:
> On 8.8.2017 10:35, Hannes Reinecke wrote:
>> Older Controller might not support volume status, so assume
>> the volume is online here.
>>
>> Signed-off-by: Hannes Reinecke <hare@suse.com>
>> ---
>>  drivers/scsi/hpsa.c | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
>> index 7ca6078..4ebf5d4 100644
>> --- a/drivers/scsi/hpsa.c
>> +++ b/drivers/scsi/hpsa.c
>> @@ -3832,6 +3832,17 @@ static int hpsa_update_device_info(struct ctlr_info *h,
>>  		if (h->fw_support & MISC_FW_RAID_OFFLOAD_BASIC)
>>  			hpsa_get_ioaccel_status(h, scsi3addr, this_device);
>>  		volume_offline = hpsa_volume_offline(h, scsi3addr);
>> +		if (volume_offline == HPSA_VPD_LV_STATUS_UNSUPPORTED &&
>> +		    !h->supported) {
>> +			/*
>> +			 * Older / unsupported controllers might not support
>> +			 * volume status
>> +			 */
>> +			dev_info(&h->pdev->dev,
>> +				 "C0:T%d:L%d Volume status not available, assuming online.\n",
>> +				 this_device->target, this_device->lun);
>> +			volume_offline = 0;
> 
> Hi,
> could we have here
> volume_offline = HPSA_LV_OK;
> instead ?

Hmm; rather not (for now).
At several places we're just checking for (!h->volume_offline); before
changing that toe HPSA_LV_OK we'd need to change all of them to
(h->volume_offline == HPSA_LV_OK) to avoid any issues here.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)