Skip to contentSkip to conversation

hpsa driver updates

PATCH In review Stephen M. Cameron 3 participants 25 messages Nov 11, 2009 – Nov 16, 2009

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

cover hpsa driver updates

The following series implements hpsa scsi driver for HP Smart Arrays, and some updates since the last time.
The first 5 patches in the series are already in Andrew Morton's tree.

---

Andrew Morton (1):
avoid helpful cleanup patches.

Stephen M. Cameron (16):
hpsa: fix typo that causes scsi status to be lost
hpsa: Make fill_cmd() return void
hpsa: Remove sendcmd, in no case are we required to poll for completions.
hpsa: Flush cache with interrupts still enabled.
hpsa: Retry driver initiated commands on unit attention
hpsa: decode unit attention condition and retry commands.
hpsa: Make hpsa_sdev_attrs static
hpsa: Allow device rescan to be triggered via sysfs.
Add thread to allow controllers to register for rescan for new devices
hpsa: Factor out some pci_unmap code
hpsa: Factor out command submission sequence
hpsa: Use shost_priv instead of accessing host->hostdata[0] directly.
hpsa: Allocate the correct amount of extra space for the scsi host Fix use of unallocated memory for MSA2xxx enclosure device data.
hpsa: Fix vendor id check
Add hpsa driver for HP Smart Array controllers.

 drivers/scsi/Kconfig    |   10 
 drivers/scsi/Makefile   |    1 
 drivers/scsi/hpsa.c     | 3531 +++++++++++++++++++++++++++++++++++++++++++++++
 drivers/scsi/hpsa.h     |  273 ++++
 drivers/scsi/hpsa_cmd.h |  326 ++++
 5 files changed, 4141 insertions(+), 0 deletions(-)
 create mode 100644 drivers/scsi/hpsa.c
 create mode 100644 drivers/scsi/hpsa.h
 create mode 100644 drivers/scsi/hpsa_cmd.h
Signature · 1 line
-- steve
#1 lore raw

PATCH 1/17 Add hpsa driver for HP Smart Array controllers.

From: \\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\"Stephen M. Cameron\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\" <[email protected]>

This driver supports a subset of HP Smart Array Controllers. It is a SCSI alternative to the cciss driver.

---

 drivers/scsi/Kconfig    |   10 
 drivers/scsi/Makefile   |    1 
 drivers/scsi/hpsa.c     | 3444 +++++++++++++++++++++++++++++++++++++++++++++++
 drivers/scsi/hpsa.h     |  268 ++++
 drivers/scsi/hpsa_cmd.h |  303 ++++
 5 files changed, 4026 insertions(+), 0 deletions(-)
 create mode 100644 drivers/scsi/hpsa.c
 create mode 100644 drivers/scsi/hpsa.h
 create mode 100644 drivers/scsi/hpsa_cmd.h
drivers/scsi/Kconfig +10 -0
index e11cca4..6dd3800 100644
@@ -388,6 +388,16 @@ config BLK_DEV_3W_XXXX_RAID
 	  Please read the comments at the top of
 	  <file:drivers/scsi/3w-xxxx.c>.
 
+config SCSI_HPSA
+	tristate "HP Smart Array SCSI driver"
+	depends on PCI && SCSI
+	help
+	  This driver supports HP Smart Array Controllers (circa 2009).
+	  It is a SCSI alternative to the cciss driver, which is a block
+	  driver.  Anyone wishing to use HP Smart Array controllers who
+	  would prefer the devices be presented to linux as SCSI devices,
+	  rather than as generic block devices should say Y here.
+
 config SCSI_3W_9XXX
 	tristate "3ware 9xxx SATA-RAID support"
 	depends on PCI && SCSI
drivers/scsi/Makefile +1 -0
index 3ad61db..ae8c153 100644
@@ -90,6 +90,7 @@ obj-$(CONFIG_SCSI_BFA_FC)	+= bfa/
 obj-$(CONFIG_SCSI_PAS16)	+= pas16.o
 obj-$(CONFIG_SCSI_T128)		+= t128.o
 obj-$(CONFIG_SCSI_DMX3191D)	+= dmx3191d.o
+obj-$(CONFIG_SCSI_HPSA)		+= hpsa.o
 obj-$(CONFIG_SCSI_DTC3280)	+= dtc.o
 obj-$(CONFIG_SCSI_SYM53C8XX_2)	+= sym53c8xx_2/
 obj-$(CONFIG_SCSI_ZALON)	+= zalon7xx.o
drivers/scsi/hpsa.c new file +3444 -0

2,977 lines not on this page · raw message

drivers/scsi/hpsa.h new file +268 -0

268 lines not on this page · raw message

drivers/scsi/hpsa_cmd.h new file +303 -0

303 lines not on this page · raw message

The rest of this diff is not shown. View the raw message.

Part of the diff is not shown here. View the raw message.

#2 lore raw

PATCH 2/17 avoid helpful cleanup patches.

In reply to Stephen M. Cameron

From: Andrew Morton <[email protected]>

Cc "Stephen M. Cameron" <[email protected]>
Cc James Bottomley <[email protected]>
Cc Jens Axboe <[email protected]>
Cc Mike Miller <[email protected]>
Cc Stephen M. Cameron <[email protected]>

---

 drivers/scsi/hpsa.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/scsi/hpsa.c +1 -1
index ea175ff..186f9a2 100644
@@ -214,7 +214,7 @@ static inline int is_logical_dev_addr_mode(unsigned char scsi3addr[])
 static const char *raid_label[] = { "0", "4", "1(1+0)", "5", "5+1", "ADG",
 	"UNKNOWN"
 };
-#define RAID_UNKNOWN (sizeof(raid_label) / sizeof(raid_label[0]) - 1)
+#define RAID_UNKNOWN (ARRAY_SIZE(raid_label) - 1)
 
 static ssize_t raid_level_show(struct device *dev,
 	     struct device_attribute *attr, char *buf)
#3 lore raw

PATCH 3/17 hpsa: Fix vendor id check

In reply to Stephen M. Cameron

hpsa: Fix vendor id check

---

 drivers/scsi/hpsa.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/scsi/hpsa.c +1 -1
index 186f9a2..6c4d9fd 100644
@@ -3080,7 +3080,7 @@ static int hpsa_pci_init(struct ctlr_info *h, struct pci_dev *pdev)
 
 	if (prod_index == ARRAY_SIZE(products)) {
 		prod_index--;
-		if (subsystem_vendor_id == !PCI_VENDOR_ID_HP ||
+		if (subsystem_vendor_id != PCI_VENDOR_ID_HP ||
 				!hpsa_allow_any) {
 			dev_warn(&pdev->dev, "unrecognized board ID:"
 				" 0x%08lx, ignoring.\n",
#4 lore raw

PATCH 4/17 Fix use of unallocated memory for MSA2xxx enclosure device data.

In reply to Stephen M. Cameron

Fix use of unallocated memory for MSA2xxx enclosure device data. If you happened to have fewer physical devices reported by CCISS_REPORT_LUNS than the total number of MSA2012 enclosures (unlikely), the data for some enclosure(s) would get stored into, or cause other device data to be stored into unallocated territory.

---

 drivers/scsi/hpsa.c |   26 ++++++++++++++++++++++----
 1 files changed, 22 insertions(+), 4 deletions(-)
drivers/scsi/hpsa.c +22 -4
index 6c4d9fd..e402155 100644
@@ -1314,7 +1314,8 @@ static void figure_bus_target_lun(struct ctlr_info *h,
 static int add_msa2xxx_enclosure_device(struct ctlr_info *h,
 	struct hpsa_scsi_dev_t *tmpdevice,
 	struct hpsa_scsi_dev_t *this_device, __u8 *lunaddrbytes,
-	int bus, int target, int lun, unsigned long lunzerobits[])
+	int bus, int target, int lun, unsigned long lunzerobits[],
+	int *nmsa2xxx_enclosures)
 {
 	unsigned char scsi3addr[8];
 
@@ -1333,10 +1334,19 @@ static int add_msa2xxx_enclosure_device(struct ctlr_info *h,
 	if (is_hba_lunid(scsi3addr))
 		return 0; /* Don't add the RAID controller here. */
 
+#define MAX_MSA2XXX_ENCLOSURES 32
+	if (*nmsa2xxx_enclosures >= MAX_MSA2XXX_ENCLOSURES) {
+		dev_warn(&h->pdev->dev, "Maximum number of MSA2XXX "
+			"enclosures exceeded.  Check your hardware "
+			"configuration.");
+		return 0;
+	}
+
 	memset(scsi3addr, 0, 8);
 	scsi3addr[3] = target;
 	if (hpsa_update_device_info(h, scsi3addr, this_device))
 		return 0;
+	(*nmsa2xxx_enclosures)++;
 	hpsa_set_bus_target_lun(this_device, bus, target, 0);
 	set_bit(target, lunzerobits);
 	return 1;
@@ -1416,7 +1426,7 @@ static void hpsa_update_scsi_devices(struct ctlr_info *h, int hostno)
 	struct hpsa_scsi_dev_t **currentsd, *this_device, *tmpdevice;
 	int ncurrent = 0;
 	int reportlunsize = sizeof(*physdev_list) + HPSA_MAX_PHYS_LUN * 8;
-	int i;
+	int i, nmsa2xxx_enclosures, ndevs_to_allocate;
 	int bus, target, lun;
 	DECLARE_BITMAP(lunzerobits, HPSA_MAX_TARGETS_PER_CTLR);
 
@@ -1438,8 +1448,14 @@ static void hpsa_update_scsi_devices(struct ctlr_info *h, int hostno)
 			logdev_list, &nlogicals))
 		goto out;
 
+	/* We might see up to 32 MSA2xxx enclosures, actually 8 of them
+	 * but each of them 4 times through different paths.  The plus 1
+	 * is for the RAID controller.
+	 */
+	ndevs_to_allocate = nphysicals + nlogicals + MAX_MSA2XXX_ENCLOSURES + 1;
+
 	/* Allocate the per device structures */
-	for (i = 0; i < nphysicals + nlogicals + 1; i++) {
+	for (i = 0; i < ndevs_to_allocate; i++) {
 		currentsd[i] = kzalloc(sizeof(*currentsd[i]), GFP_KERNEL);
 		if (!currentsd[i]) {
 			dev_warn(&h->pdev->dev, "out of memory at %s:%d\n",
@@ -1450,6 +1466,7 @@ static void hpsa_update_scsi_devices(struct ctlr_info *h, int hostno)
 	}
 
 	/* adjust our table of devices */
+	nmsa2xxx_enclosures = 0;
 	for (i = 0; i < nphysicals + nlogicals + 1; i++) {
 		__u8 *lunaddrbytes;
 
@@ -1482,7 +1499,8 @@ static void hpsa_update_scsi_devices(struct ctlr_info *h, int hostno)
 		 * there is no lun 0.
 		 */
 		if (add_msa2xxx_enclosure_device(h, tmpdevice, this_device,
-			lunaddrbytes, bus, target, lun, lunzerobits)) {
+				lunaddrbytes, bus, target, lun, lunzerobits,
+				&nmsa2xxx_enclosures)) {
 			ncurrent++;
 			this_device = currentsd[ncurrent];
 		}
#5 lore raw

Re: [PATCH 04/17] Fix use of unallocated memory for MSA2xxx enclosure device data.

drivers/scsi/hpsa.c static int add_msa2xxx_enclosure_device(struct ctlr_info *h, · line 1343

Quoted 7 lines · On Wed, 11 Nov 2009 10:50:43 -0600 "Stephen M. Cameron"…
drivers/scsi/hpsa.c +7 -0
+#define MAX_MSA2XXX_ENCLOSURES 32
+	if (*nmsa2xxx_enclosures >= MAX_MSA2XXX_ENCLOSURES) {
+		dev_warn(&h->pdev->dev, "Maximum number of MSA2XXX "
+			"enclosures exceeded.  Check your hardware "
+			"configuration.");
+		return 0;
+	}

The message would be a bit more helpful if it told the operator what the limit actually is.

#6 lore raw

PATCH 5/17 hpsa: Allocate the correct amount of extra space for the scsi host

In reply to Stephen M. Cameron

hpsa: Allocate the correct amount of extra space for the scsi host

---

 drivers/scsi/hpsa.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/scsi/hpsa.c +1 -1
index e402155..85be09c 100644
@@ -890,7 +890,7 @@ static int hpsa_scsi_detect(struct ctlr_info *h)
 	struct Scsi_Host *sh;
 	int error;
 
-	sh = scsi_host_alloc(&hpsa_driver_template, sizeof(*h));
+	sh = scsi_host_alloc(&hpsa_driver_template, sizeof(h));
 	if (sh == NULL)
 		goto fail;
 
#7 lore raw

PATCH 6/17 hpsa: Use shost_priv instead of accessing host->hostdata[0] directly.

In reply to Stephen M. Cameron

Use shost_priv instead of accessing host->hostdata[0] directly.

---

 drivers/scsi/hpsa.c |   20 +++++++++++++-------
 1 files changed, 13 insertions(+), 7 deletions(-)
drivers/scsi/hpsa.c +13 -7
index 85be09c..d5a795e 100644
@@ -177,6 +177,12 @@ static struct scsi_host_template hpsa_driver_template = {
 	.sdev_attrs = hpsa_sdev_attrs,
 };
 
+static inline struct ctlr_info *sdev_to_hba(struct scsi_device *sdev)
+{
+	unsigned long *priv = shost_priv(sdev->host);
+	return (struct ctlr_info *) *priv;
+}
+
 /* Enqueuing and dequeuing functions for cmdlists. */
 static inline void addQ(struct hlist_head *list, struct CommandList *c)
 {
@@ -227,7 +233,7 @@ static ssize_t raid_level_show(struct device *dev,
 	unsigned long flags;
 
 	sdev = to_scsi_device(dev);
-	h = (struct ctlr_info *) sdev->host->hostdata[0];
+	h = sdev_to_hba(sdev);
 	spin_lock_irqsave(&h->lock, flags);
 	hdev = sdev->hostdata;
 	if (!hdev) {
@@ -260,7 +266,7 @@ static ssize_t lunid_show(struct device *dev,
 	unsigned char lunid[8];
 
 	sdev = to_scsi_device(dev);
-	h = (struct ctlr_info *) sdev->host->hostdata[0];
+	h = sdev_to_hba(sdev);
 	spin_lock_irqsave(&h->lock, flags);
 	hdev = sdev->hostdata;
 	if (!hdev) {
@@ -284,7 +290,7 @@ static ssize_t unique_id_show(struct device *dev,
 	unsigned char sn[16];
 
 	sdev = to_scsi_device(dev);
-	h = (struct ctlr_info *) sdev->host->hostdata[0];
+	h = sdev_to_hba(sdev);
 	spin_lock_irqsave(&h->lock, flags);
 	hdev = sdev->hostdata;
 	if (!hdev) {
@@ -677,7 +683,7 @@ static int hpsa_slave_alloc(struct scsi_device *sdev)
 	unsigned long flags;
 	struct ctlr_info *h;
 
-	h = (struct ctlr_info *) sdev->host->hostdata[0];
+	h = sdev_to_hba(sdev);
 	spin_lock_irqsave(&h->devlock, flags);
 	sd = lookup_hpsa_scsi_dev(h, sdev_channel(sdev),
 		sdev_id(sdev), sdev->lun);
@@ -1615,7 +1621,7 @@ static int hpsa_scsi_queue_command(struct scsi_cmnd *cmd,
 	unsigned long flags;
 
 	/* Get the ptr to our adapter structure out of cmd->host. */
-	h = (struct ctlr_info *) cmd->device->host->hostdata[0];
+	h = sdev_to_hba(cmd->device);
 	dev = cmd->device->hostdata;
 	if (!dev) {
 		cmd->result = DID_NO_CONNECT << 16;
@@ -1795,7 +1801,7 @@ static int hpsa_eh_device_reset_handler(struct scsi_cmnd *scsicmd)
 	struct hpsa_scsi_dev_t *dev;
 
 	/* find the controller to which the command to be aborted was sent */
-	h = (struct ctlr_info *) scsicmd->device->host->hostdata[0];
+	h = sdev_to_hba(scsicmd->device);
 	if (h == NULL) /* paranoia */
 		return FAILED;
 	dev_warn(&h->pdev->dev, "resetting drive\n");
@@ -2325,7 +2331,7 @@ static int hpsa_ioctl(struct scsi_device *dev, int cmd, void *arg)
 	struct ctlr_info *h;
 	void __user *argp = (void __user *)arg;
 
-	h = (struct ctlr_info *) dev->host->hostdata[0];
+	h = sdev_to_hba(dev);
 
 	switch (cmd) {
 	case CCISS_DEREGDISK:
#8 lore raw

PATCH 7/17 hpsa: Factor out command submission sequence

In reply to Stephen M. Cameron

hpsa: Factor out command submission sequence. There are a few places where the same sequence of statements occurs to submit and wait for the completion of a command. Factor these out into a separate function.

---

 drivers/scsi/hpsa.c |   10 ++--------
 1 files changed, 2 insertions(+), 8 deletions(-)
drivers/scsi/hpsa.c +2 -8
index d5a795e..81fbbff 100644
@@ -2086,7 +2086,6 @@ static int hpsa_passthru_ioctl(struct ctlr_info *h, void __user *argp)
 	struct CommandList *c;
 	char *buff = NULL;
 	union u64bit temp64;
-	DECLARE_COMPLETION_ONSTACK(wait);
 
 	if (!argp)
 		return -EINVAL;
@@ -2144,9 +2143,7 @@ static int hpsa_passthru_ioctl(struct ctlr_info *h, void __user *argp)
 		c->SG[0].Len = iocommand.buf_size;
 		c->SG[0].Ext = 0; /* we are not chaining*/
 	}
-	c->waiting = &wait;
-	enqueue_cmd_and_start_io(h, c);
-	wait_for_completion(&wait);
+	hpsa_scsi_do_simple_cmd_core(h, c);
 
 	/* unlock the buffers from DMA */
 	temp64.val32.lower = c->SG[0].Addr.lower;
@@ -2186,7 +2183,6 @@ static int hpsa_big_passthru_ioctl(struct ctlr_info *h, void __user *argp)
 	BYTE sg_used = 0;
 	int status = 0;
 	int i;
-	DECLARE_COMPLETION_ONSTACK(wait);
 	__u32 left;
 	__u32 sz;
 	BYTE __user *data_ptr;
@@ -2280,9 +2276,7 @@ static int hpsa_big_passthru_ioctl(struct ctlr_info *h, void __user *argp)
 			c->SG[i].Ext = 0;
 		}
 	}
-	c->waiting = &wait;
-	enqueue_cmd_and_start_io(h, c);
-	wait_for_completion(&wait);
+	hpsa_scsi_do_simple_cmd_core(h, c);
 	/* unlock the buffers from DMA */
 	for (i = 0; i < sg_used; i++) {
 		temp64.val32.lower = c->SG[i].Addr.lower;
#9 lore raw

PATCH 8/17 hpsa: Factor out some pci_unmap code

In reply to Stephen M. Cameron

hpsa: Factor out some pci_unmap code. There are a few places where the same, or nearly the same code is used to pci_unmap data buffers of driver initiated commands. Factor these out into a separate function.

---

 drivers/scsi/hpsa.c |   45 ++++++++++++++-------------------------------
 1 files changed, 14 insertions(+), 31 deletions(-)
drivers/scsi/hpsa.c +14 -31
index 81fbbff..74c054a 100644
@@ -928,17 +928,18 @@ static int hpsa_scsi_detect(struct ctlr_info *h)
 	return -1;
 }
 
-static void hpsa_unmap_one(struct pci_dev *pdev,
-		struct CommandList *cp,
-		size_t buflen,
-		int data_direction)
+static void hpsa_pci_unmap(struct pci_dev *pdev,
+	struct CommandList *c, int sg_used, int data_direction)
 {
+	int i;
 	union u64bit addr64;
 
-	addr64.val32.lower = cp->SG[0].Addr.lower;
-	addr64.val32.upper = cp->SG[0].Addr.upper;
-	pci_unmap_single(pdev, (dma_addr_t) addr64.val,
-		buflen, data_direction);
+	for (i = 0; i < sg_used; i++) {
+		addr64.val32.lower = c->SG[i].Addr.lower;
+		addr64.val32.upper = c->SG[i].Addr.upper;
+		pci_unmap_single(pdev, (dma_addr_t) addr64.val, c->SG[i].Len,
+			data_direction);
+	}
 }
 
 static void hpsa_map_one(struct pci_dev *pdev,
@@ -1055,7 +1056,7 @@ static int hpsa_scsi_do_inquiry(struct ctlr_info *h, unsigned char *scsi3addr,
 		TYPE_CMD);
 	if (rc == 0) {
 		hpsa_scsi_do_simple_cmd_core(h, c);
-		hpsa_unmap_one(h->pdev, c, bufsize, PCI_DMA_FROMDEVICE);
+		hpsa_pci_unmap(h->pdev, c, 1, PCI_DMA_FROMDEVICE);
 
 		ei = c->err_info;
 		if (ei->CommandStatus != 0 &&
@@ -1162,7 +1163,7 @@ static int hpsa_scsi_do_report_luns(struct ctlr_info *h, int logical,
 	if (extended_response)
 		c->Request.CDB[1] = extended_response;
 	hpsa_scsi_do_simple_cmd_core(h, c);
-	hpsa_unmap_one(h->pdev, c, bufsize, PCI_DMA_FROMDEVICE);
+	hpsa_pci_unmap(h->pdev, c, 1, PCI_DMA_FROMDEVICE);
 	ei = c->err_info;
 	if (ei->CommandStatus != 0 &&
 	    ei->CommandStatus != CMD_DATA_UNDERRUN) {
@@ -2144,12 +2145,7 @@ static int hpsa_passthru_ioctl(struct ctlr_info *h, void __user *argp)
 		c->SG[0].Ext = 0; /* we are not chaining*/
 	}
 	hpsa_scsi_do_simple_cmd_core(h, c);
-
-	/* unlock the buffers from DMA */
-	temp64.val32.lower = c->SG[0].Addr.lower;
-	temp64.val32.upper = c->SG[0].Addr.upper;
-	pci_unmap_single(h->pdev, (dma_addr_t) temp64.val, iocommand.buf_size,
-			 PCI_DMA_BIDIRECTIONAL);
+	hpsa_pci_unmap(h->pdev, c, 1, PCI_DMA_BIDIRECTIONAL);
 
 	/* Copy the error information out */
 	memcpy(&iocommand.error_info, c->err_info,
@@ -2277,14 +2273,7 @@ static int hpsa_big_passthru_ioctl(struct ctlr_info *h, void __user *argp)
 		}
 	}
 	hpsa_scsi_do_simple_cmd_core(h, c);
-	/* unlock the buffers from DMA */
-	for (i = 0; i < sg_used; i++) {
-		temp64.val32.lower = c->SG[i].Addr.lower;
-		temp64.val32.upper = c->SG[i].Addr.upper;
-		pci_unmap_single(h->pdev,
-			(dma_addr_t) temp64.val, buff_size[i],
-			PCI_DMA_BIDIRECTIONAL);
-	}
+	hpsa_pci_unmap(h->pdev, c, sg_used, PCI_DMA_BIDIRECTIONAL);
 	/* Copy the error information out */
 	memcpy(&ioc->error_info, c->err_info, sizeof(ioc->error_info));
 	if (copy_to_user(argp, ioc, sizeof(*ioc))) {
@@ -2501,7 +2490,6 @@ static int sendcmd_core(struct ctlr_info *h, struct CommandList *c)
 	int i;
 	unsigned long complete;
 	int status = IO_ERROR;
-	union u64bit buff_dma_handle;
 
 resend_cmd1:
 	/*
@@ -2596,12 +2584,7 @@ resend_cmd1:
 	} while (1);
 
 cleanup1:
-
-	/* unlock the data buffer from DMA */
-	buff_dma_handle.val32.lower = c->SG[0].Addr.lower;
-	buff_dma_handle.val32.upper = c->SG[0].Addr.upper;
-	pci_unmap_single(h->pdev, (dma_addr_t) buff_dma_handle.val,
-			 c->SG[0].Len, PCI_DMA_BIDIRECTIONAL);
+	hpsa_pci_unmap(h->pdev, c, 1, PCI_DMA_BIDIRECTIONAL);
 	return status;
 }
 
#10 lore raw

PATCH 9/17 Add thread to allow controllers to register for rescan for new devices

In reply to Stephen M. Cameron

Add thread to allow controllers to register for rescan for new devices (borrowed code from cciss.)

---

 drivers/scsi/hpsa.c |  147 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 drivers/scsi/hpsa.h |    5 ++
 2 files changed, 150 insertions(+), 2 deletions(-)
drivers/scsi/hpsa.c +145 -2
index 74c054a..35267cb 100644
@@ -47,6 +47,7 @@
 #include <linux/string.h>
 #include <linux/bitmap.h>
 #include <asm/atomic.h>
+#include <linux/kthread.h>
 #include "hpsa_cmd.h"
 #include "hpsa.h"
 
@@ -145,6 +146,7 @@ static ssize_t lunid_show(struct device *dev,
 	struct device_attribute *attr, char *buf);
 static ssize_t unique_id_show(struct device *dev,
 	struct device_attribute *attr, char *buf);
+static void hpsa_update_scsi_devices(struct ctlr_info *h, int hostno);
 
 DEVICE_ATTR(raid_level, S_IRUGO, raid_level_show, NULL);
 DEVICE_ATTR(lunid, S_IRUGO, lunid_show, NULL);
@@ -183,6 +185,129 @@ static inline struct ctlr_info *sdev_to_hba(struct scsi_device *sdev)
 	return (struct ctlr_info *) *priv;
 }
 
+static struct task_struct *hpsa_scan_thread;
+static DEFINE_MUTEX(scan_mutex);
+static LIST_HEAD(scan_q);
+static int scan_thread(void *data);
+
+/**
+ * add_to_scan_list() - add controller to rescan queue
+ * @h:		      Pointer to the controller.
+ *
+ * Adds the controller to the rescan queue if not already on the queue.
+ *
+ * returns 1 if added to the queue, 0 if skipped (could be on the
+ * queue already, or the controller could be initializing or shutting
+ * down).
+ **/
+static int add_to_scan_list(struct ctlr_info *h)
+{
+	struct ctlr_info *test_h;
+	int found = 0;
+	int ret = 0;
+
+	if (h->busy_initializing)
+		return 0;
+
+	if (!mutex_trylock(&h->busy_shutting_down))
+		return 0;
+
+	mutex_lock(&scan_mutex);
+	list_for_each_entry(test_h, &scan_q, scan_list) {
+		if (test_h == h) {
+			found = 1;
+			break;
+		}
+	}
+	if (!found && !h->busy_scanning) {
+		INIT_COMPLETION(h->scan_wait);
+		list_add_tail(&h->scan_list, &scan_q);
+		ret = 1;
+	}
+	mutex_unlock(&scan_mutex);
+	mutex_unlock(&h->busy_shutting_down);
+
+	return ret;
+}
+
+/**
+ * remove_from_scan_list() - remove controller from rescan queue
+ * @h:			   Pointer to the controller.
+ *
+ * Removes the controller from the rescan queue if present. Blocks if
+ * the controller is currently conducting a rescan.  The controller
+ * can be in one of three states:
+ * 1. Doesn't need a scan
+ * 2. On the scan list, but not scanning yet (we remove it)
+ * 3. Busy scanning (and not on the list). In this case we want to wait for
+ *    the scan to complete to make sure the scanning thread for this
+ *    controller is completely idle.
+ **/
+static void remove_from_scan_list(struct ctlr_info *h)
+{
+	struct ctlr_info *test_h, *tmp_h;
+
+	mutex_lock(&scan_mutex);
+	list_for_each_entry_safe(test_h, tmp_h, &scan_q, scan_list) {
+		if (test_h == h) { /* state 2. */
+			list_del(&h->scan_list);
+			complete_all(&h->scan_wait);
+			mutex_unlock(&scan_mutex);
+			return;
+		}
+	}
+	if (h->busy_scanning) { /* state 3. */
+		mutex_unlock(&scan_mutex);
+		wait_for_completion(&h->scan_wait);
+	} else { /* state 1, nothing to do. */
+		mutex_unlock(&scan_mutex);
+	}
+}
+
+/* scan_thread() - kernel thread used to rescan controllers
+ * @data:	 Ignored.
+ *
+ * A kernel thread used scan for drive topology changes on
+ * controllers. The thread processes only one controller at a time
+ * using a queue.  Controllers are added to the queue using
+ * add_to_scan_list() and removed from the queue either after done
+ * processing or using remove_from_scan_list().
+ *
+ * returns 0.
+ **/
+static int scan_thread(__attribute__((unused)) void *data)
+{
+	struct ctlr_info *h;
+	int host_no;
+
+	while (1) {
+		set_current_state(TASK_INTERRUPTIBLE);
+		schedule();
+		if (kthread_should_stop())
+			break;
+
+		while (1) {
+			mutex_lock(&scan_mutex);
+			if (list_empty(&scan_q)) {
+				mutex_unlock(&scan_mutex);
+				break;
+			}
+			h = list_entry(scan_q.next, struct ctlr_info,
+					scan_list);
+			list_del(&h->scan_list);
+			h->busy_scanning = 1;
+			mutex_unlock(&scan_mutex);
+			host_no = h->scsi_host ?  h->scsi_host->host_no : -1;
+			hpsa_update_scsi_devices(h, host_no);
+			complete_all(&h->scan_wait);
+			mutex_lock(&scan_mutex);
+			h->busy_scanning = 0;
+			mutex_unlock(&scan_mutex);
+		}
+	}
+	return 0;
+}
+
 /* Enqueuing and dequeuing functions for cmdlists. */
 static inline void addQ(struct hlist_head *list, struct CommandList *c)
 {
@@ -3269,8 +3394,11 @@ static int __devinit hpsa_init_one(struct pci_dev *pdev,
 	if (!h)
 		return -1;
 
+	h->busy_initializing = 1;
 	INIT_HLIST_HEAD(&h->cmpQ);
 	INIT_HLIST_HEAD(&h->reqQ);
+	mutex_init(&h->busy_shutting_down);
+	init_completion(&h->scan_wait);
 	if (hpsa_pci_init(h, pdev) != 0)
 		goto clean1;
 
@@ -3330,7 +3458,7 @@ static int __devinit hpsa_init_one(struct pci_dev *pdev,
 	h->access.set_intr_mask(h, HPSA_INTR_ON);
 
 	hpsa_register_scsi(h);	/* hook ourselves into SCSI subsystem */
-
+	h->busy_initializing = 0;
 	return 1;
 
 clean4:
@@ -3347,6 +3475,7 @@ clean4:
 	free_irq(h->intr[SIMPLE_MODE_INT], h);
 clean2:
 clean1:
+	h->busy_initializing = 0;
 	kfree(h);
 	return -1;
 }
@@ -3387,6 +3516,8 @@ static void __devexit hpsa_remove_one(struct pci_dev *pdev)
 		return;
 	}
 	h = pci_get_drvdata(pdev);
+	mutex_lock(&h->busy_shutting_down);
+	remove_from_scan_list(h);
 	hpsa_unregister_scsi(h);	/* unhook from SCSI subsystem */
 	hpsa_shutdown(pdev);
 	iounmap(h->vaddr);
@@ -3403,6 +3534,7 @@ static void __devexit hpsa_remove_one(struct pci_dev *pdev)
 	 */
 	pci_release_regions(pdev);
 	pci_set_drvdata(pdev, NULL);
+	mutex_unlock(&h->busy_shutting_down);
 	kfree(h);
 }
 
@@ -3433,12 +3565,23 @@ static struct pci_driver hpsa_pci_driver = {
  */
 static int __init hpsa_init(void)
 {
-	return pci_register_driver(&hpsa_pci_driver);
+	int err;
+	/* Start the scan thread */
+	hpsa_scan_thread = kthread_run(scan_thread, NULL, "hpsa_scan");
+	if (IS_ERR(hpsa_scan_thread)) {
+		err = PTR_ERR(hpsa_scan_thread);
+		return -ENODEV;
+	}
+	err = pci_register_driver(&hpsa_pci_driver);
+	if (err)
+		kthread_stop(hpsa_scan_thread);
+	return err;
 }
 
 static void __exit hpsa_cleanup(void)
 {
 	pci_unregister_driver(&hpsa_pci_driver);
+	kthread_stop(hpsa_scan_thread);
 }
 
 module_init(hpsa_init);
drivers/scsi/hpsa.h +5 -0
index 6491d4e..ffa8c50 100644
@@ -91,6 +91,11 @@ struct ctlr_info {
 	unsigned long  		*cmd_pool_bits;
 	int			nr_allocs;
 	int			nr_frees;
+	int			busy_initializing;
+	int			busy_scanning;
+	struct mutex		busy_shutting_down;
+	struct list_head	scan_list;
+	struct completion	scan_wait;
 
 	struct Scsi_Host *scsi_host;
 	spinlock_t devlock; /* to protect hba[ctlr]->dev[];  */
#11 lore raw

Re: [PATCH 09/17] Add thread to allow controllers to register for rescan for new devices

Quoted 13 lines · On Wed, 11 Nov 2009 10:51:09 -0600 "Stephen M. Cameron"…

Add thread to allow controllers to register for rescan for new devices (borrowed code from cciss.)

+ * add_to_scan_list() - add controller to rescan queue
+ * @h: Pointer to the controller.
+ *
+ * Adds the controller to the rescan queue if not already on the queue.
+ *
+ * returns 1 if added to the queue, 0 if skipped (could be on the
+ * queue already, or the controller could be initializing or shutting
+ * down).
+ **/

drivers/scsi/hpsa.c +1 -0
+static int add_to_scan_list(struct ctlr_info *h)

drivers/scsi/hpsa.c static inline struct ctlr_info *sdev_to_hba(struct scsi_device *sdev) · line 213

Quoted 10 lines
drivers/scsi/hpsa.c +10 -0
+{
+	struct ctlr_info *test_h;
+	int found = 0;
+	int ret = 0;
+
+	if (h->busy_initializing)
+		return 0;
+
+	if (!mutex_trylock(&h->busy_shutting_down))
+		return 0;

Generally a trylock needs a comment explaining wtf it's there for.

This one certainly does.

Quoted 22 lines
drivers/scsi/hpsa.c +19 -0
+	mutex_lock(&scan_mutex);
+	list_for_each_entry(test_h, &scan_q, scan_list) {
+		if (test_h == h) {
+			found = 1;
+			break;
+		}
+	}
+	if (!found && !h->busy_scanning) {
+		INIT_COMPLETION(h->scan_wait);
+		list_add_tail(&h->scan_list, &scan_q);
+		ret = 1;
+	}
+	mutex_unlock(&scan_mutex);
+	mutex_unlock(&h->busy_shutting_down);
+
+	return ret;
+}
 
...
 
+/* scan_thread() - kernel thread used to rescan controllers
+ * @data:	 Ignored.

drivers/scsi/hpsa.c static inline struct ctlr_info *sdev_to_hba(struct scsi_device *sdev) · line 278

Quoted 10 lines
drivers/scsi/hpsa.c +10 -0
+ *
+ * A kernel thread used scan for drive topology changes on
+ * controllers. The thread processes only one controller at a time
+ * using a queue.  Controllers are added to the queue using
+ * add_to_scan_list() and removed from the queue either after done
+ * processing or using remove_from_scan_list().
+ *
+ * returns 0.
+ **/
+static int scan_thread(__attribute__((unused)) void *data)

Is the attribute actually needed?

We have various helper macros, such as __always_unused.

drivers/scsi/hpsa.c static inline struct ctlr_info *sdev_to_hba(struct scsi_device *sdev) · line 287

Quoted 9 lines
drivers/scsi/hpsa.c +9 -0
+{
+	struct ctlr_info *h;
+	int host_no;
+
+	while (1) {
+		set_current_state(TASK_INTERRUPTIBLE);
+		schedule();
+		if (kthread_should_stop())
+			break;

Looks wrong. If a kthread_stop() comes in just before the set_current_state(), I think the thread will sleep forever?

Quoted 25 lines
drivers/scsi/hpsa.c +22 -0
+		while (1) {
+			mutex_lock(&scan_mutex);
+			if (list_empty(&scan_q)) {
+				mutex_unlock(&scan_mutex);
+				break;
+			}
+			h = list_entry(scan_q.next, struct ctlr_info,
+					scan_list);
+			list_del(&h->scan_list);
+			h->busy_scanning = 1;
+			mutex_unlock(&scan_mutex);
+			host_no = h->scsi_host ?  h->scsi_host->host_no : -1;
+			hpsa_update_scsi_devices(h, host_no);
+			complete_all(&h->scan_wait);
+			mutex_lock(&scan_mutex);
+			h->busy_scanning = 0;
+			mutex_unlock(&scan_mutex);
+		}
+	}
+	return 0;
+}
+
 
...
#12 lore raw

PATCH 10/17 hpsa: Allow device rescan to be triggered via sysfs.

In reply to Stephen M. Cameron

Allow device rescan to be triggered via sysfs.

---

 drivers/scsi/hpsa.c |   24 ++++++++++++++++++++++++
 1 files changed, 24 insertions(+), 0 deletions(-)
drivers/scsi/hpsa.c +24 -0
index 35267cb..dfef62d 100644
@@ -147,10 +147,13 @@ static ssize_t lunid_show(struct device *dev,
 static ssize_t unique_id_show(struct device *dev,
 	struct device_attribute *attr, char *buf);
 static void hpsa_update_scsi_devices(struct ctlr_info *h, int hostno);
+static ssize_t host_store_rescan(struct device *dev,
+	 struct device_attribute *attr, const char *buf, size_t count);
 
 DEVICE_ATTR(raid_level, S_IRUGO, raid_level_show, NULL);
 DEVICE_ATTR(lunid, S_IRUGO, lunid_show, NULL);
 DEVICE_ATTR(unique_id, S_IRUGO, unique_id_show, NULL);
+DEVICE_ATTR(rescan, S_IWUSR, NULL, host_store_rescan);
 
 struct device_attribute *hpsa_sdev_attrs[] = {
 	&dev_attr_raid_level,
@@ -159,6 +162,11 @@ struct device_attribute *hpsa_sdev_attrs[] = {
 	NULL,
 };
 
+static struct device_attribute *hpsa_shost_attrs[] = {
+	&dev_attr_rescan,
+	NULL,
+};
+
 static struct scsi_host_template hpsa_driver_template = {
 	.module			= THIS_MODULE,
 	.name			= "hpsa",
@@ -177,6 +185,7 @@ static struct scsi_host_template hpsa_driver_template = {
 	.compat_ioctl		= hpsa_compat_ioctl,
 #endif
 	.sdev_attrs = hpsa_sdev_attrs,
+	.shost_attrs = hpsa_shost_attrs,
 };
 
 static inline struct ctlr_info *sdev_to_hba(struct scsi_device *sdev)
@@ -308,6 +317,21 @@ static int scan_thread(__attribute__((unused)) void *data)
 	return 0;
 }
 
+static ssize_t host_store_rescan(struct device *dev,
+				 struct device_attribute *attr,
+				 const char *buf, size_t count)
+{
+	struct ctlr_info *h;
+	struct Scsi_Host *shost = class_to_shost(dev);
+	unsigned long *priv = shost_priv(shost);
+	h = (struct ctlr_info *) *priv;
+	if (add_to_scan_list(h)) {
+		wake_up_process(hpsa_scan_thread);
+		wait_for_completion_interruptible(&h->scan_wait);
+	}
+	return count;
+}
+
 /* Enqueuing and dequeuing functions for cmdlists. */
 static inline void addQ(struct hlist_head *list, struct CommandList *c)
 {
#13 lore raw

Re: [PATCH 10/17] hpsa: Allow device rescan to be triggered via sysfs.

Quoted 1 line · On Wed, 11 Nov 2009 10:51:14 -0600 "Stephen M. Cameron"…

Allow device rescan to be triggered via sysfs.

When adding a new userspace interface to the kernel, please fully describe that interface in the changelog.

Is there somewhere where this should have been documented?

#14 lore raw

PATCH 11/17 hpsa: Make hpsa_sdev_attrs static

In reply to Stephen M. Cameron

hpsa: Make hpsa_sdev_attrs static

---

 drivers/scsi/hpsa.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/scsi/hpsa.c +1 -1
index dfef62d..f893a53 100644
@@ -155,7 +155,7 @@ DEVICE_ATTR(lunid, S_IRUGO, lunid_show, NULL);
 DEVICE_ATTR(unique_id, S_IRUGO, unique_id_show, NULL);
 DEVICE_ATTR(rescan, S_IWUSR, NULL, host_store_rescan);
 
-struct device_attribute *hpsa_sdev_attrs[] = {
+static struct device_attribute *hpsa_sdev_attrs[] = {
 	&dev_attr_raid_level,
 	&dev_attr_lunid,
 	&dev_attr_unique_id,
#15 lore raw

PATCH 12/17 hpsa: decode unit attention condition and retry commands.

In reply to Stephen M. Cameron

hpsa: decode unit attention condition and mark commands with DID_SOFT_ERROR to make the mid-layer retry them.

---

 drivers/scsi/hpsa.c     |   69 ++++++++++++++++++++++++++++++++++++++++++++++-
 drivers/scsi/hpsa_cmd.h |   23 ++++++++++++++++
 2 files changed, 91 insertions(+), 1 deletions(-)
drivers/scsi/hpsa.c +68 -1
index f893a53..6f63d45 100644
@@ -149,6 +149,10 @@ static ssize_t unique_id_show(struct device *dev,
 static void hpsa_update_scsi_devices(struct ctlr_info *h, int hostno);
 static ssize_t host_store_rescan(struct device *dev,
 	 struct device_attribute *attr, const char *buf, size_t count);
+static int check_for_unit_attention(struct ctlr_info *h,
+	struct CommandList *c);
+static void check_ioctl_unit_attention(struct ctlr_info *h,
+	struct CommandList *c);
 
 DEVICE_ATTR(raid_level, S_IRUGO, raid_level_show, NULL);
 DEVICE_ATTR(lunid, S_IRUGO, lunid_show, NULL);
@@ -317,6 +321,57 @@ static int scan_thread(__attribute__((unused)) void *data)
 	return 0;
 }
 
+static int check_for_unit_attention(struct ctlr_info *h,
+	struct CommandList *c)
+{
+	if (c->err_info->SenseInfo[2] != UNIT_ATTENTION)
+		return 0;
+
+	switch (c->err_info->SenseInfo[12]) {
+	case STATE_CHANGED:
+		dev_warn(&h->pdev->dev, "hpsa%d: a state change "
+			"detected, command retried\n", h->ctlr);
+		break;
+	case LUN_FAILED:
+		dev_warn(&h->pdev->dev, "hpsa%d: LUN failure "
+			"detected, action required\n", h->ctlr);
+		break;
+	case REPORT_LUNS_CHANGED:
+		dev_warn(&h->pdev->dev, "hpsa%d: report LUN data "
+			"changed\n", h->ctlr);
+	/*
+	 * Here, we could call add_to_scan_list and wake up the scan thread,
+	 * except that it's quite likely that we will get more than one
+	 * REPORT_LUNS_CHANGED condition in quick succession, which means
+	 * that those which occur after the first one will likely happen
+	 * *during* the scan_thread's rescan.  And the rescan code is not
+	 * robust enough to restart in the middle, undoing what it has already
+	 * done, and it's not clear that it's even possible to do this, since
+	 * part of what it does is notify the SCSI mid layer, which starts
+	 * doing it's own i/o to read partition tables and so on, and the
+	 * driver doesn't have visibility to know what might need undoing.
+	 * In any event, if possible, it is horribly complicated to get right
+	 * so we just don't do it for now.
+	 *
+	 * Note: this REPORT_LUNS_CHANGED condition only occurs on the MSA2012.
+	 */
+		break;
+	case POWER_OR_RESET:
+		dev_warn(&h->pdev->dev, "hpsa%d: a power on "
+			"or device reset detected\n", h->ctlr);
+		break;
+	case UNIT_ATTENTION_CLEARED:
+		dev_warn(&h->pdev->dev, "hpsa%d: unit attention "
+		    "cleared by another initiator\n", h->ctlr);
+		break;
+	default:
+		dev_warn(&h->pdev->dev, "hpsa%d: unknown "
+			"unit attention detected\n", h->ctlr);
+		break;
+	}
+	return 1;
+}
+
 static ssize_t host_store_rescan(struct device *dev,
 				 struct device_attribute *attr,
 				 const char *buf, size_t count)
@@ -903,7 +958,10 @@ static void complete_scsi_command(struct CommandList *cp,
 		}
 
 		if (ei->ScsiStatus == SAM_STAT_CHECK_CONDITION) {
-
+			if (check_for_unit_attention(h, cp)) {
+				cmd->result = DID_SOFT_ERROR << 16;
+				break;
+			}
 			if (sense_key == ILLEGAL_REQUEST) {
 				/* If ASC/ASCQ indicate Logical Unit
 				 * Not Supported condition,
@@ -2295,6 +2353,7 @@ static int hpsa_passthru_ioctl(struct ctlr_info *h, void __user *argp)
 	}
 	hpsa_scsi_do_simple_cmd_core(h, c);
 	hpsa_pci_unmap(h->pdev, c, 1, PCI_DMA_BIDIRECTIONAL);
+	check_ioctl_unit_attention(h, c);
 
 	/* Copy the error information out */
 	memcpy(&iocommand.error_info, c->err_info,
@@ -2423,6 +2482,7 @@ static int hpsa_big_passthru_ioctl(struct ctlr_info *h, void __user *argp)
 	}
 	hpsa_scsi_do_simple_cmd_core(h, c);
 	hpsa_pci_unmap(h->pdev, c, sg_used, PCI_DMA_BIDIRECTIONAL);
+	check_ioctl_unit_attention(h, c);
 	/* Copy the error information out */
 	memcpy(&ioc->error_info, c->err_info, sizeof(ioc->error_info));
 	if (copy_to_user(argp, ioc, sizeof(*ioc))) {
@@ -2455,6 +2515,13 @@ cleanup1:
 	return status;
 }
 
+static void check_ioctl_unit_attention(struct ctlr_info *h,
+	struct CommandList *c)
+{
+	if (c->err_info->CommandStatus == CMD_TARGET_STATUS &&
+			c->err_info->ScsiStatus != SAM_STAT_CHECK_CONDITION)
+		(void) check_for_unit_attention(h, c);
+}
 /*
  * ioctl
  */
drivers/scsi/hpsa_cmd.h +23 -0
index 0564df2..12d7138 100644
@@ -41,6 +41,29 @@
 #define CMD_TIMEOUT             0x000B
 #define CMD_UNABORTABLE		0x000C
 
+/* Unit Attentions ASC's as defined for the MSA2012sa */
+#define POWER_OR_RESET			0x29
+#define STATE_CHANGED			0x2a
+#define UNIT_ATTENTION_CLEARED		0x2f
+#define LUN_FAILED			0x3e
+#define REPORT_LUNS_CHANGED		0x3f
+
+/* Unit Attentions ASCQ's as defined for the MSA2012sa */
+
+	/* These ASCQ's defined for ASC = POWER_OR_RESET */
+#define POWER_ON_RESET			0x00
+#define POWER_ON_REBOOT			0x01
+#define SCSI_BUS_RESET			0x02
+#define MSA_TARGET_RESET		0x03
+#define CONTROLLER_FAILOVER		0x04
+#define TRANSCEIVER_SE			0x05
+#define TRANSCEIVER_LVD			0x06
+
+	/* These ASCQ's defined for ASC = STATE_CHANGED */
+#define RESERVATION_PREEMPTED		0x03
+#define ASYM_ACCESS_CHANGED		0x06
+#define LUN_CAPACITY_CHANGED		0x09
+
 /* transfer direction */
 #define XFER_NONE               0x00
 #define XFER_WRITE              0x01
#16 lore raw

PATCH 13/17 hpsa: Retry driver initiated commands on unit attention

In reply to Stephen M. Cameron

hpsa: Retry driver initiated commands on unit attention. There are a some commands initiated by the driver which are neither ioctls nor normal i/o. If any of these commands completes with UNIT ATTENTION, they should be retried.

---

 drivers/scsi/hpsa.c |   22 ++++++++++++++++------
 1 files changed, 16 insertions(+), 6 deletions(-)
drivers/scsi/hpsa.c +16 -6
index 6f63d45..e3b80fa 100644
@@ -1173,7 +1173,7 @@ static void hpsa_map_one(struct pci_dev *pdev,
 	cp->Header.SGTotal = (__u16) 1; /* total sgs in this cmd list */
 }
 
-static void hpsa_scsi_do_simple_cmd_core(struct ctlr_info *h,
+static inline void hpsa_scsi_do_simple_cmd_core(struct ctlr_info *h,
 	struct CommandList *c)
 {
 	DECLARE_COMPLETION_ONSTACK(wait);
@@ -1183,6 +1183,19 @@ static void hpsa_scsi_do_simple_cmd_core(struct ctlr_info *h,
 	wait_for_completion(&wait);
 }
 
+static void hpsa_scsi_do_simple_cmd_with_retry(struct ctlr_info *h,
+	struct CommandList *c, int data_direction)
+{
+	int retry_count = 0;
+
+	do {
+		memset(c->err_info, 0, sizeof(c->err_info));
+		hpsa_scsi_do_simple_cmd_core(h, c);
+		retry_count++;
+	} while (check_for_unit_attention(h, c) && retry_count <= 3);
+	hpsa_pci_unmap(h->pdev, c, 1, data_direction);
+}
+
 static void hpsa_scsi_interpret_error(struct CommandList *cp)
 {
 	struct ErrorInfo *ei;
@@ -1262,9 +1275,7 @@ static int hpsa_scsi_do_inquiry(struct ctlr_info *h, unsigned char *scsi3addr,
 	rc = fill_cmd(c, HPSA_INQUIRY, h, buf, bufsize, page, scsi3addr,
 		TYPE_CMD);
 	if (rc == 0) {
-		hpsa_scsi_do_simple_cmd_core(h, c);
-		hpsa_pci_unmap(h->pdev, c, 1, PCI_DMA_FROMDEVICE);
-
+		hpsa_scsi_do_simple_cmd_with_retry(h, c, PCI_DMA_FROMDEVICE);
 		ei = c->err_info;
 		if (ei->CommandStatus != 0 &&
 		    ei->CommandStatus != CMD_DATA_UNDERRUN) {
@@ -1369,8 +1380,7 @@ static int hpsa_scsi_do_report_luns(struct ctlr_info *h, int logical,
 
 	if (extended_response)
 		c->Request.CDB[1] = extended_response;
-	hpsa_scsi_do_simple_cmd_core(h, c);
-	hpsa_pci_unmap(h->pdev, c, 1, PCI_DMA_FROMDEVICE);
+	hpsa_scsi_do_simple_cmd_with_retry(h, c, PCI_DMA_FROMDEVICE);
 	ei = c->err_info;
 	if (ei->CommandStatus != 0 &&
 	    ei->CommandStatus != CMD_DATA_UNDERRUN) {
#17 lore raw

Re: [PATCH 13/17] hpsa: Retry driver initiated commands on unit attention

drivers/scsi/hpsa.c static void hpsa_scsi_do_simple_cmd_core(struct ctlr_info *h, · line 1197

Quoted 12 lines · On Wed, 11 Nov 2009 10:51:29 -0600 "Stephen M. Cameron"…
drivers/scsi/hpsa.c +12 -0
+static void hpsa_scsi_do_simple_cmd_with_retry(struct ctlr_info *h,
+	struct CommandList *c, int data_direction)
+{
+	int retry_count = 0;
+
+	do {
+		memset(c->err_info, 0, sizeof(c->err_info));
+		hpsa_scsi_do_simple_cmd_core(h, c);
+		retry_count++;
+	} while (check_for_unit_attention(h, c) && retry_count <= 3);
+	hpsa_pci_unmap(h->pdev, c, 1, data_direction);
+}

If this times out, no indication of the error is returned to the caller and no message is displayed to the operator.

Doesn't seem very robust?

#18 lore raw

PATCH 14/17 hpsa: Flush cache with interrupts still enabled.

In reply to Stephen M. Cameron

hpsa: Flush cache with interrupts still enabled. It's not necessary to disable interrupts prior to flushing the cache on driver unload. Also, don't dma from the stack.

---

 drivers/scsi/hpsa.c |   39 ++++++++++++++++++++++++++++++---------
 1 files changed, 30 insertions(+), 9 deletions(-)
drivers/scsi/hpsa.c +30 -9
index e3b80fa..d171a55 100644
@@ -3581,24 +3581,45 @@ clean1:
 	return -1;
 }
 
+static void hpsa_flush_cache(struct ctlr_info *h)
+{
+	char *flush_buf;
+	struct CommandList *c;
+	int rc;
+
+	flush_buf = kzalloc(4, GFP_KERNEL);
+	if (!flush_buf)
+		return;
+
+	c = cmd_special_alloc(h);
+	if (!c) {
+		dev_warn(&h->pdev->dev, "cmd_special_alloc returned NULL!\n");
+		goto out_of_memory;
+	}
+	rc = fill_cmd(c, HPSA_CACHE_FLUSH, h, flush_buf, 4, 0,
+		RAID_CTLR_LUNID, TYPE_CMD);
+	if (rc == 0) {
+		hpsa_scsi_do_simple_cmd_with_retry(h, c, PCI_DMA_TODEVICE);
+		if (c->err_info->CommandStatus != 0)
+			dev_warn(&h->pdev->dev,
+				"error flushing cache on controller\n");
+	}
+	cmd_special_free(h, c);
+out_of_memory:
+	kfree(flush_buf);
+}
+
 static void hpsa_shutdown(struct pci_dev *pdev)
 {
 	struct ctlr_info *h;
-	char flush_buf[4];
-	int return_code;
 
 	h = pci_get_drvdata(pdev);
 	/* Turn board interrupts off  and send the flush cache command
 	 * sendcmd will turn off interrupt, and send the flush...
 	 * To write all data in the battery backed cache to disks
 	 */
-	memset(flush_buf, 0, 4);
-	return_code = sendcmd(HPSA_CACHE_FLUSH, h, flush_buf, 4, 0,
-				RAID_CTLR_LUNID, TYPE_CMD);
-	if (return_code != IO_OK) {
-		dev_warn(&pdev->dev, "error flushing cache on controller %d\n",
-		       h->ctlr);
-	}
+	hpsa_flush_cache(h);
+	h->access.set_intr_mask(h, HPSA_INTR_OFF);
 	free_irq(h->intr[2], h);
 #ifdef CONFIG_PCI_MSI
 	if (h->msix_vector)
#19 lore raw

PATCH 15/17 hpsa: Remove sendcmd, in no case are we required to poll for completions.

In reply to Stephen M. Cameron

hpsa: Remove sendcmd, there are no instances in which we must poll for command completion. This means sendcmd_core() and pollcomplete() can be removed as well.

---

 drivers/scsi/hpsa.c |  156 ---------------------------------------------------
 1 files changed, 0 insertions(+), 156 deletions(-)
drivers/scsi/hpsa.c +0 -156
index d171a55..6dc7ce6 100644
@@ -117,8 +117,6 @@ static int number_of_controllers;
 static irqreturn_t do_hpsa_intr(int irq, void *dev_id);
 static int hpsa_ioctl(struct scsi_device *dev, int cmd, void *arg);
 static void start_io(struct ctlr_info *h);
-static int sendcmd(__u8 cmd, struct ctlr_info *h, void *buff, size_t size,
-		   __u8 page_code, unsigned char *scsi3addr, int cmd_type);
 
 #ifdef CONFIG_COMPAT
 static int hpsa_compat_ioctl(struct scsi_device *dev, int cmd, void *arg);
@@ -131,7 +129,6 @@ static struct CommandList *cmd_special_alloc(struct ctlr_info *h);
 static int fill_cmd(struct CommandList *c, __u8 cmd, struct ctlr_info *h,
 	void *buff, size_t size, __u8 page_code, unsigned char *scsi3addr,
 	int cmd_type);
-static int sendcmd_core(struct ctlr_info *h, struct CommandList *c);
 
 static int hpsa_scsi_queue_command(struct scsi_cmnd *cmd,
 		void (*done)(struct scsi_cmnd *));
@@ -2685,159 +2682,6 @@ static int fill_cmd(struct CommandList *c, __u8 cmd, struct ctlr_info *h,
 }
 
 /*
- *   Wait polling for a command to complete.
- *   The memory mapped FIFO is polled for the completion.
- *   Used only at init time, interrupts from the HBA are disabled.
- */
-static unsigned long pollcomplete(struct ctlr_info *h)
-{
-	unsigned long done;
-	int i;
-
-	/* Wait (up to HPSA_MAX_POLL_TIME_SECS) for a command to complete */
-	for (i = HPSA_MAX_POLL_TIME_SECS * HZ; i > 0; i--) {
-		done = h->access.command_completed(h);
-		if (done == FIFO_EMPTY)
-			schedule_timeout_uninterruptible(1);
-		else
-			return done;
-	}
-	/* Invalid address to tell caller we ran out of time */
-	dev_warn(&h->pdev->dev, "pollcomplete(): returning 1\n");
-	return 1;
-}
-
-/* Send command c to controller h and poll for it to complete.
- * Turns interrupts off on the board.  Used at driver init time
- * and during SCSI error recovery.
- */
-static int sendcmd_core(struct ctlr_info *h, struct CommandList *c)
-{
-	int i;
-	unsigned long complete;
-	int status = IO_ERROR;
-
-resend_cmd1:
-	/*
-	 * Disable interrupt
-	 */
-	h->access.set_intr_mask(h, HPSA_INTR_OFF);
-
-	/* Make sure there is room in the command FIFO
-	 * Actually it should be completely empty at this time
-	 * unless we are in here doing error handling for the scsi
-	 * side of the driver.
-	 */
-	for (i = 200000; i > 0; i--) {
-		/* if fifo isn't full go */
-		if (!(h->access.fifo_full(h)))
-			break;
-		udelay(10);
-		dev_warn(&h->pdev->dev, "sendcmd FIFO full, waiting!\n");
-	}
-	h->access.submit_command(h, c); /* Send the cmd */
-	do {
-		complete = pollcomplete(h);
-
-		if (complete == 1) {
-			dev_warn(&h->pdev->dev,
-			       "sendcmd timeout, no command list address "
-				"returned!\n");
-			status = IO_ERROR;
-			break;
-		}
-
-		/* If it's not the cmd we're looking for, save it for later */
-		if ((complete & ~HPSA_ERROR_BIT) != c->busaddr) {
-			dev_warn(&h->pdev->dev, "unexpected command "
-				"completion.\n");
-			continue;
-		}
-
-		/* It is our command.  If no error, we're done. */
-		if (!(complete & HPSA_ERROR_BIT)) {
-			status = IO_OK;
-			break;
-		}
-
-		/* There is an error... */
-
-		/* if data overrun or underun on Report command ignore it */
-		if (((c->Request.CDB[0] == HPSA_REPORT_LOG) ||
-			(c->Request.CDB[0] == HPSA_REPORT_PHYS) ||
-			(c->Request.CDB[0] == HPSA_INQUIRY)) &&
-			((c->err_info->CommandStatus == CMD_DATA_OVERRUN) ||
-			(c->err_info->CommandStatus == CMD_DATA_UNDERRUN))) {
-			complete = c->busaddr;
-			status = IO_OK;
-			break;
-		}
-		if (c->err_info->CommandStatus == CMD_UNSOLICITED_ABORT) {
-			dev_warn(&h->pdev->dev, "unsolicited abort %p\n", c);
-			if (c->retry_count < MAX_CMD_RETRIES) {
-				dev_warn(&h->pdev->dev, "retrying %p\n", c);
-				c->retry_count++;
-				/* erase the old error information */
-				memset(c->err_info, 0, sizeof(c->err_info));
-				goto resend_cmd1;
-			}
-			dev_warn(&h->pdev->dev,
-				"retried %p too many times\n", c);
-			status = IO_ERROR;
-			goto cleanup1;
-		}
-		if (c->err_info->CommandStatus == CMD_UNABORTABLE) {
-			dev_warn(&h->pdev->dev,
-				"command could not be aborted.\n");
-			status = IO_ERROR;
-			goto cleanup1;
-		}
-		dev_warn(&h->pdev->dev, "sendcmd error\n");
-		dev_warn(&h->pdev->dev,
-			"cmd = 0x%02x, CommandStatus = 0x%02x\n",
-			c->Request.CDB[0], c->err_info->CommandStatus);
-		if (c->err_info->CommandStatus == CMD_TARGET_STATUS) {
-			dev_warn(&h->pdev->dev, "target status = 0x%02x\n",
-				c->err_info->ScsiStatus);
-			if (c->err_info->ScsiStatus == 2) /* chk cond */
-				dev_warn(&h->pdev->dev, "sense key = 0x%02x\n",
-					0xf & c->err_info->SenseInfo[2]);
-		}
-
-		status = IO_ERROR;
-		goto cleanup1;
-
-	} while (1);
-
-cleanup1:
-	hpsa_pci_unmap(h->pdev, c, 1, PCI_DMA_BIDIRECTIONAL);
-	return status;
-}
-
-/*
- * Send a command to the controller, and wait for it to complete.
- * Used at init time, and during SCSI error recovery.
- */
-static int sendcmd(__u8 cmd, struct ctlr_info *h, void *buff, size_t size,
-		   __u8 page_code, unsigned char *scsi3addr, int cmd_type)
-{
-	struct CommandList *c;
-	int status;
-
-	c = cmd_alloc(h);
-	if (c == NULL) {
-		dev_warn(&h->pdev->dev, "unable to get memory");
-		return IO_ERROR;
-	}
-	status = fill_cmd(c, cmd, h, buff, size, page_code,
-		scsi3addr, cmd_type);
-	if (status == IO_OK)
-		status = sendcmd_core(h, c);
-	cmd_free(h, c);
-	return status;
-}
-
-/*
  * Map (physical) PCI mem into (virtual) kernel space
  */
 static void __iomem *remap_pci_mem(ulong base, ulong size)
#20 lore raw

PATCH 16/17 hpsa: Make fill_cmd() return void

In reply to Stephen M. Cameron

hpsa: Make fill_cmd() return void, and BUG() if it is asked to fill out an unknown command or command type, as that should simply not happen as all the commands and command types are fixed, and known ahead of time.

---

 drivers/scsi/hpsa.c |   73 +++++++++++++++++----------------------------------
 1 files changed, 25 insertions(+), 48 deletions(-)
drivers/scsi/hpsa.c +25 -48
index 6dc7ce6..5097995 100644
@@ -126,7 +126,7 @@ static void cmd_free(struct ctlr_info *h, struct CommandList *c);
 static void cmd_special_free(struct ctlr_info *h, struct CommandList *c);
 static struct CommandList *cmd_alloc(struct ctlr_info *h);
 static struct CommandList *cmd_special_alloc(struct ctlr_info *h);
-static int fill_cmd(struct CommandList *c, __u8 cmd, struct ctlr_info *h,
+static void fill_cmd(struct CommandList *c, __u8 cmd, struct ctlr_info *h,
 	void *buff, size_t size, __u8 page_code, unsigned char *scsi3addr,
 	int cmd_type);
 
@@ -1258,7 +1258,7 @@ static int hpsa_scsi_do_inquiry(struct ctlr_info *h, unsigned char *scsi3addr,
 			unsigned char page, unsigned char *buf,
 			unsigned char bufsize)
 {
-	int rc;
+	int rc = IO_OK;
 	struct CommandList *c;
 	struct ErrorInfo *ei;
 
@@ -1269,16 +1269,12 @@ static int hpsa_scsi_do_inquiry(struct ctlr_info *h, unsigned char *scsi3addr,
 		return -1;
 	}
 
-	rc = fill_cmd(c, HPSA_INQUIRY, h, buf, bufsize, page, scsi3addr,
-		TYPE_CMD);
-	if (rc == 0) {
-		hpsa_scsi_do_simple_cmd_with_retry(h, c, PCI_DMA_FROMDEVICE);
-		ei = c->err_info;
-		if (ei->CommandStatus != 0 &&
-		    ei->CommandStatus != CMD_DATA_UNDERRUN) {
-			hpsa_scsi_interpret_error(c);
-			rc = -1;
-		}
+	fill_cmd(c, HPSA_INQUIRY, h, buf, bufsize, page, scsi3addr, TYPE_CMD);
+	hpsa_scsi_do_simple_cmd_with_retry(h, c, PCI_DMA_FROMDEVICE);
+	ei = c->err_info;
+	if (ei->CommandStatus != 0 && ei->CommandStatus != CMD_DATA_UNDERRUN) {
+		hpsa_scsi_interpret_error(c);
+		rc = -1;
 	}
 	cmd_special_free(h, c);
 	return rc;
@@ -1286,7 +1282,7 @@ static int hpsa_scsi_do_inquiry(struct ctlr_info *h, unsigned char *scsi3addr,
 
 static int hpsa_send_reset(struct ctlr_info *h, unsigned char *scsi3addr)
 {
-	int rc;
+	int rc = IO_OK;
 	struct CommandList *c;
 	struct ErrorInfo *ei;
 
@@ -1297,11 +1293,7 @@ static int hpsa_send_reset(struct ctlr_info *h, unsigned char *scsi3addr)
 		return -1;
 	}
 
-	rc = fill_cmd(c, HPSA_DEVICE_RESET_MSG, h, NULL, 0, 0, scsi3addr,
-		TYPE_MSG);
-	if (rc != 0)
-		goto out;
-
+	fill_cmd(c, HPSA_DEVICE_RESET_MSG, h, NULL, 0, 0, scsi3addr, TYPE_MSG);
 	hpsa_scsi_do_simple_cmd_core(h, c);
 	/* no unmap needed here because no data xfer. */
 
@@ -1310,7 +1302,6 @@ static int hpsa_send_reset(struct ctlr_info *h, unsigned char *scsi3addr)
 		hpsa_scsi_interpret_error(c);
 		rc = -1;
 	}
-out:
 	cmd_special_free(h, c);
 	return rc;
 }
@@ -1357,7 +1348,7 @@ static int hpsa_scsi_do_report_luns(struct ctlr_info *h, int logical,
 		struct ReportLUNdata *buf, int bufsize,
 		int extended_response)
 {
-	int rc;
+	int rc = IO_OK;
 	struct CommandList *c;
 	unsigned char scsi3addr[8];
 	struct ErrorInfo *ei;
@@ -1370,11 +1361,8 @@ static int hpsa_scsi_do_report_luns(struct ctlr_info *h, int logical,
 
 	memset(&scsi3addr[0], 0, 8); /* address the controller */
 
-	rc = fill_cmd(c, logical ? HPSA_REPORT_LOG : HPSA_REPORT_PHYS, h,
+	fill_cmd(c, logical ? HPSA_REPORT_LOG : HPSA_REPORT_PHYS, h,
 		buf, bufsize, 0, scsi3addr, TYPE_CMD);
-	if (rc != 0)
-		goto out;
-
 	if (extended_response)
 		c->Request.CDB[1] = extended_response;
 	hpsa_scsi_do_simple_cmd_with_retry(h, c, PCI_DMA_FROMDEVICE);
@@ -1384,7 +1372,6 @@ static int hpsa_scsi_do_report_luns(struct ctlr_info *h, int logical,
 		hpsa_scsi_interpret_error(c);
 		rc = -1;
 	}
-out:
 	cmd_special_free(h, c);
 	return rc;
 }
@@ -1944,7 +1931,7 @@ static int hpsa_register_scsi(struct ctlr_info *h)
 static int wait_for_device_to_become_ready(struct ctlr_info *h,
 	unsigned char lunaddr[])
 {
-	int rc;
+	int rc = 0;
 	int count = 0;
 	int waittime = HZ;
 	struct CommandList *c;
@@ -1971,15 +1958,7 @@ static int wait_for_device_to_become_ready(struct ctlr_info *h,
 			waittime = waittime * 2;
 
 		/* Send the Test Unit Ready */
-		rc = fill_cmd(c, TEST_UNIT_READY, h, NULL, 0, 0,
-			lunaddr, TYPE_CMD);
-		if (rc != 0) {
-			/* We don't expect to get in here */
-			dev_warn(&h->pdev->dev, "fill_cmd failed at %s:%d\n",
-				__FILE__, __LINE__);
-			break;
-		}
-
+		fill_cmd(c, TEST_UNIT_READY, h, NULL, 0, 0, lunaddr, TYPE_CMD);
 		hpsa_scsi_do_simple_cmd_core(h, c);
 		/* no unmap needed here because no data xfer. */
 
@@ -2558,7 +2537,7 @@ static int hpsa_ioctl(struct scsi_device *dev, int cmd, void *arg)
 	}
 }
 
-static int fill_cmd(struct CommandList *c, __u8 cmd, struct ctlr_info *h,
+static void fill_cmd(struct CommandList *c, __u8 cmd, struct ctlr_info *h,
 	void *buff, size_t size, __u8 page_code, unsigned char *scsi3addr,
 	int cmd_type)
 {
@@ -2631,7 +2610,8 @@ static int fill_cmd(struct CommandList *c, __u8 cmd, struct ctlr_info *h,
 			break;
 		default:
 			dev_warn(&h->pdev->dev, "unknown command 0x%c\n", cmd);
-			return IO_ERROR;
+			BUG();
+			return;
 		}
 	} else if (cmd_type == TYPE_MSG) {
 		switch (cmd) {
@@ -2655,11 +2635,11 @@ static int fill_cmd(struct CommandList *c, __u8 cmd, struct ctlr_info *h,
 		default:
 			dev_warn(&h->pdev->dev, "unknown message type %d\n",
 				cmd);
-			return IO_ERROR;
+			BUG();
 		}
 	} else {
 		dev_warn(&h->pdev->dev, "unknown command type %d\n", cmd_type);
-		return IO_ERROR;
+		BUG();
 	}
 
 	switch (c->Request.Type.Direction) {
@@ -2678,7 +2658,7 @@ static int fill_cmd(struct CommandList *c, __u8 cmd, struct ctlr_info *h,
 
 	hpsa_map_one(h->pdev, c, buff, size, pci_dir);
 
-	return IO_OK;
+	return;
 }
 
 /*
@@ -3429,7 +3409,6 @@ static void hpsa_flush_cache(struct ctlr_info *h)
 {
 	char *flush_buf;
 	struct CommandList *c;
-	int rc;
 
 	flush_buf = kzalloc(4, GFP_KERNEL);
 	if (!flush_buf)
@@ -3440,14 +3419,12 @@ static void hpsa_flush_cache(struct ctlr_info *h)
 		dev_warn(&h->pdev->dev, "cmd_special_alloc returned NULL!\n");
 		goto out_of_memory;
 	}
-	rc = fill_cmd(c, HPSA_CACHE_FLUSH, h, flush_buf, 4, 0,
+	fill_cmd(c, HPSA_CACHE_FLUSH, h, flush_buf, 4, 0,
 		RAID_CTLR_LUNID, TYPE_CMD);
-	if (rc == 0) {
-		hpsa_scsi_do_simple_cmd_with_retry(h, c, PCI_DMA_TODEVICE);
-		if (c->err_info->CommandStatus != 0)
-			dev_warn(&h->pdev->dev,
-				"error flushing cache on controller\n");
-	}
+	hpsa_scsi_do_simple_cmd_with_retry(h, c, PCI_DMA_TODEVICE);
+	if (c->err_info->CommandStatus != 0)
+		dev_warn(&h->pdev->dev,
+			"error flushing cache on controller\n");
 	cmd_special_free(h, c);
 out_of_memory:
 	kfree(flush_buf);
#21 lore raw

PATCH 17/17 hpsa: fix typo that causes scsi status to be lost

In reply to Stephen M. Cameron

hpsa: fix typo that causes scsi status to be lost

---

 drivers/scsi/hpsa.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
drivers/scsi/hpsa.c +2 -2
index 5097995..06eeefd 100644
@@ -987,7 +987,7 @@ static void complete_scsi_command(struct CommandList *cp,
 
 
 			/* Must be some other type of check condition */
-			cmd->result |= (ei->ScsiStatus < 1);
+			cmd->result |= (ei->ScsiStatus << 1);
 			dev_warn(&h->pdev->dev, "cp %p has check condition: "
 					"unknown type: "
 					"Sense: 0x%x, ASC: 0x%x, ASCQ: 0x%x, "
@@ -1010,7 +1010,7 @@ static void complete_scsi_command(struct CommandList *cp,
 		 */
 		if (ei->ScsiStatus) {
 
-			cmd->result |= (ei->ScsiStatus < 1);
+			cmd->result |= (ei->ScsiStatus << 1);
 			dev_warn(&h->pdev->dev, "cp %p has status 0x%x "
 				"Sense: 0x%x, ASC: 0x%x, ASCQ: 0x%x, "
 				"Returning result: 0x%x\n",
#22 lore raw
In reply to Stephen M. Cameron
Quoted 18 lines · Stephen M. Cameron

The following series implements hpsa scsi driver for HP Smart Arrays, and some updates since the last time.
The first 5 patches in the series are already in Andrew Morton's tree.

---

Andrew Morton (1):
avoid helpful cleanup patches.

Stephen M. Cameron (16):
hpsa: fix typo that causes scsi status to be lost
hpsa: Make fill_cmd() return void
hpsa: Remove sendcmd, in no case are we required to poll for completions.
hpsa: Flush cache with interrupts still enabled.
hpsa: Retry driver initiated commands on unit attention
hpsa: decode unit attention condition and retry commands.
hpsa: Make hpsa_sdev_attrs static
hpsa: Allow device rescan to be triggered via sysfs.

Quoted 8 lines

Add thread to allow controllers to register for rescan for new devices
hpsa: Factor out some pci_unmap code
hpsa: Factor out command submission sequence
hpsa: Use shost_priv instead of accessing host->hostdata[0] directly.
hpsa: Allocate the correct amount of extra space for the scsi host Fix use of unallocated memory for MSA2xxx enclosure device data.
hpsa: Fix vendor id check
Add hpsa driver for HP Smart Array controllers.

Actually, it's pretty difficult to review a 17 patch series like this because the human mind (or at least mine) doesn't retain sufficient context from patch to patch. I ended up just pulling all 17 into a tree and reviewing the finished driver.

That said:

in hpsa.c:

Quoted 4 lines

static struct device_attribute *hpsa_shost_attrs[] = {
&dev_attr_rescan,
NULL,
};

We already have a host scan attribute which (admittedly using the transport class logic) you can plug into ... can't you just use it? It supplies user context, so you could dispense with all that scan thread stuff as well, I think.

Quoted 3 lines

static DEFINE_MUTEX(scan_mutex);
static LIST_HEAD(scan_q);
static int scan_thread(void *data);

These names are too generic. We already have a scan_mutex at least defined at the top level. I know they're protected by static, but that doesn't necessarily help if they show up in a debug stack trace.

All of this report luns stuff looks fairly identical to the report luns we do in scsi_scan.c ... barring the initial command, which could be translated. Wouldn't it be easier to have the generic code parse and do all of this?

Quoted 3 lines

static int hpsa_scsi_queue_command(struct scsi_cmnd *cmd,
void (*done)(struct scsi_cmnd *))
{

[...]

Quoted 8 lines
	c = cmd_alloc(h);
	spin_unlock_irqrestore(&h->lock, flags);
	if (c == NULL) {			/* trouble... */
		dev_err(&h->pdev->dev, "cmd_alloc returned NULL!\n");
		cmd->result = DID_NO_CONNECT << 16;
		done(cmd);
		return 0;
	}

I think you want to return SCSI_MLQUEUE_HOST_BUSY here, which will trigger a throttle and retry after either something frees or I/O pressure builds more.

Quoted 2 lines

static int hpsa_eh_device_reset_handler(struct scsi_cmnd *scsicmd) {

[...]

Quoted 3 lines
	rc = hpsa_send_reset(h, dev->scsi3addr);
	if (rc == 0 && wait_for_device_to_become_ready(h, dev->scsi3addr) == 0)
		return SUCCESS;

So the first thing we do after a device reset successful return is send a test unit ready to the failing device ... there's no real need for you to duplicate that, is there?

The ioctl stuff looks like you could do it all with SG_IO now rather than rolling your own versions ... or is there some backward compatibility problem here?

Quoted 2 lines

static __devinit int hpsa_hard_reset_controller(struct pci_dev *pdev) {

[...]

Quoted 2 lines
	set_current_state(TASK_UNINTERRUPTIBLE);
	schedule_timeout(HZ >> 1);

msleep(500) please .. This isn't the only place this occurs, could you replace all of them?

in hpsa_cmd.h:

Quoted 18 lines

/* Unit Attentions ASC's as defined for the MSA2012sa */
#define POWER_OR_RESET 0x29
#define STATE_CHANGED 0x2a
#define UNIT_ATTENTION_CLEARED 0x2f
#define LUN_FAILED 0x3e
#define REPORT_LUNS_CHANGED 0x3f

/* Unit Attentions ASCQ's as defined for the MSA2012sa */

	/* These ASCQ's defined for ASC = POWER_OR_RESET */

#define POWER_ON_RESET 0x00
#define POWER_ON_REBOOT 0x01
#define SCSI_BUS_RESET 0x02
#define MSA_TARGET_RESET 0x03
#define CONTROLLER_FAILOVER 0x04
#define TRANSCEIVER_SE 0x05
#define TRANSCEIVER_LVD 0x06

Quoted 4 lines
	/* These ASCQ's defined for ASC = STATE_CHANGED */

#define RESERVATION_PREEMPTED 0x03
#define ASYM_ACCESS_CHANGED 0x06
#define LUN_CAPACITY_CHANGED 0x09

Traditionally we've shied away from putting ASC/ASCQ values into defines ... but these all look to be global not hpsa local, so they should be in a common central file.

Otherwise looks OK to a cursory glance.

James

#23 lore raw
Quoted 4 lines · On Mon, 16 Nov 2009 15:06:49 -0600 James Bottomley <[email protected]>…

Actually, it's pretty difficult to review a 17 patch series like this because the human mind (or at least mine) doesn't retain sufficient context from patch to patch. I ended up just pulling all 17 into a tree and reviewing the finished driver.

I rolled all the patches together:

From: "Stephen M. Cameron" <[email protected]>

Add hpsa driver for HP Smart Array controllers.

This driver supports a subset of HP Smart Array Controllers. It is a SCSI alternative to the cciss driver.

[[email protected]: avoid helpful cleanup patches]
Cc Jens Axboe <[email protected]>
Cc James Bottomley <[email protected]>

---

 drivers/scsi/Kconfig    |   10 
 drivers/scsi/Makefile   |    1 
 drivers/scsi/hpsa.c     | 3531 ++++++++++++++++++++++++++++++++++++++
 drivers/scsi/hpsa.h     |  273 ++
 drivers/scsi/hpsa_cmd.h |  326 +++
 5 files changed, 4141 insertions(+)
drivers/scsi/Kconfig renamed from drivers/scsi/Kconfig~scsi-add-hpsa-driver-for-hp-smart-array-controllers +10 -0
diff -puN drivers/scsi/Kconfig~scsi-add-hpsa-driver-for-hp-smart-array-controllers drivers/scsi/Kconfig
@@ -388,6 +388,16 @@ config BLK_DEV_3W_XXXX_RAID
 	  Please read the comments at the top of
 	  <file:drivers/scsi/3w-xxxx.c>.
 
+config SCSI_HPSA
+	tristate "HP Smart Array SCSI driver"
+	depends on PCI && SCSI
+	help
+	  This driver supports HP Smart Array Controllers (circa 2009).
+	  It is a SCSI alternative to the cciss driver, which is a block
+	  driver.  Anyone wishing to use HP Smart Array controllers who
+	  would prefer the devices be presented to linux as SCSI devices,
+	  rather than as generic block devices should say Y here.
+
 config SCSI_3W_9XXX
 	tristate "3ware 9xxx SATA-RAID support"
 	depends on PCI && SCSI
drivers/scsi/Makefile renamed from drivers/scsi/Makefile~scsi-add-hpsa-driver-for-hp-smart-array-controllers +1 -0
diff -puN drivers/scsi/Makefile~scsi-add-hpsa-driver-for-hp-smart-array-controllers drivers/scsi/Makefile
@@ -91,6 +91,7 @@ obj-$(CONFIG_SCSI_BFA_FC)	+= bfa/
 obj-$(CONFIG_SCSI_PAS16)	+= pas16.o
 obj-$(CONFIG_SCSI_T128)		+= t128.o
 obj-$(CONFIG_SCSI_DMX3191D)	+= dmx3191d.o
+obj-$(CONFIG_SCSI_HPSA)		+= hpsa.o
 obj-$(CONFIG_SCSI_DTC3280)	+= dtc.o
 obj-$(CONFIG_SCSI_SYM53C8XX_2)	+= sym53c8xx_2/
 obj-$(CONFIG_SCSI_ZALON)	+= zalon7xx.o
drivers/scsi/hpsa.c new file +3531 -0

2,977 lines not on this page · raw message

drivers/scsi/hpsa.h new file +273 -0

273 lines not on this page · raw message

drivers/scsi/hpsa_cmd.h new file +326 -0

326 lines not on this page · raw message

The rest of this diff is not shown. View the raw message.

_

Part of the diff is not shown here. View the raw message.

#24 lore raw
In reply to James Bottomley
Quoted 28 lines · James Bottomley
Quoted 18 lines · Stephen M. Cameron

The following series implements hpsa scsi driver for HP Smart Arrays, and some updates since the last time.
The first 5 patches in the series are already in Andrew Morton's tree.

---

Andrew Morton (1):
avoid helpful cleanup patches.

Stephen M. Cameron (16):
hpsa: fix typo that causes scsi status to be lost
hpsa: Make fill_cmd() return void
hpsa: Remove sendcmd, in no case are we required to poll for completions.
hpsa: Flush cache with interrupts still enabled.
hpsa: Retry driver initiated commands on unit attention
hpsa: decode unit attention condition and retry commands.
hpsa: Make hpsa_sdev_attrs static
hpsa: Allow device rescan to be triggered via sysfs.

Quoted 8 lines

Add thread to allow controllers to register for rescan for new devices
hpsa: Factor out some pci_unmap code
hpsa: Factor out command submission sequence
hpsa: Use shost_priv instead of accessing host->hostdata[0] directly.
hpsa: Allocate the correct amount of extra space for the scsi host Fix use of unallocated memory for MSA2xxx enclosure device data.
hpsa: Fix vendor id check
Add hpsa driver for HP Smart Array controllers.

Quoted 5 lines

Actually, it's pretty difficult to review a 17 patch series like this because the human mind (or at least mine) doesn't retain sufficient context from patch to patch. I ended up just pulling all 17 into a tree and reviewing the finished driver.

I was trying to err on the safe side, in case anyone wanted the separate patches, on the grounds that it's easier to bake a cake than to unbake one. If it's preferred, I can pre-bake them into one big patch before sending next time. Also, not all the patches are authored by me, so I figured I ought to preserve that.

Quoted 11 lines

That said:

in hpsa.c:

Quoted 4 lines

static struct device_attribute *hpsa_shost_attrs[] = {
&dev_attr_rescan,
NULL,
};

We already have a host scan attribute which (admittedly using the transport class logic) you can plug into ... can't you just use it?

Maybe. I'll take a look at it.

Quoted 11 lines

It
supplies user context, so you could dispense with all that scan thread stuff as well, I think.

Quoted 3 lines

static DEFINE_MUTEX(scan_mutex);
static LIST_HEAD(scan_q);
static int scan_thread(void *data);

These names are too generic. We already have a scan_mutex at least defined at the top level. I know they're protected by static, but that doesn't necessarily help if they show up in a debug stack trace.

Ok.

Quoted 6 lines

All of this report luns stuff looks fairly identical to the report luns we do in scsi_scan.c ... barring the initial command, which could be translated. Wouldn't it be easier to have the generic code parse and do all of this?

Maybe. There are complications. We present some physical devices (tape drives, changers, the RAID controller) some logical (logical drives), the RAID controller is scsi revision 0, not 5, if we present it first, the REPORT_LUNS is never sent by the upper layers because of this, and only 7 devices get discovered, etc. If we present the first logical drive first, it sends the REPORT LUNS to the the drive, etc. It gets a little weird, as our hardware is a little weird. If it is possible, I doubt I would be able to have such a change ready by 2.6.33 timeframe.

Quoted 13 lines
Quoted 3 lines

static int hpsa_scsi_queue_command(struct scsi_cmnd *cmd,
void (*done)(struct scsi_cmnd *))
{

[...]

Quoted 8 lines
	c = cmd_alloc(h);
	spin_unlock_irqrestore(&h->lock, flags);
	if (c == NULL) {			/* trouble... */
		dev_err(&h->pdev->dev, "cmd_alloc returned NULL!\n");
		cmd->result = DID_NO_CONNECT << 16;
		done(cmd);
		return 0;
	}
Quoted 3 lines

I think you want to return SCSI_MLQUEUE_HOST_BUSY here, which will trigger a throttle and retry after either something frees or I/O pressure builds more.

Ok.

Quoted 11 lines
Quoted 3 lines

static int hpsa_eh_device_reset_handler(struct scsi_cmnd *scsicmd) {

[...]

Quoted 3 lines
	rc = hpsa_send_reset(h, dev->scsi3addr);
	if (rc == 0 && wait_for_device_to_become_ready(h, dev->scsi3addr) == 0)
		return SUCCESS;

So the first thing we do after a device reset successful return is send a test unit ready to the failing device ... there's no real need for you to duplicate that, is there?

It didn't seem to work until I did that. this was awhile ago that I wrote this so my memory of it isn't too clear, but as I recall, I tested this (in cciss, which is what this code evolved from) with a tape drive
with the long timeout set rather short, and "mt -f /dev/st0 erase" The
artificially shortened timeout would mean that the erase would time out, and this reset code would get triggered. I found that without my change, the device didn't become ready in time, even though the upper layers sent TURs, and the device would get kicked off. So I put in that code to make it work. Maybe it was the mid layer that needed tweaking? maybe the slow to recover tape drives deserved to get kicked off?

Quoted 5 lines

The ioctl stuff looks like you could do it all with SG_IO now rather than rolling your own versions ... or is there some backward compatibility problem here?

Yeah, mainly backward compatibility.

HP's array config utility (ACU), snmp storage agents,
firmware flashing utilities, cciss_vol_status,
arrayprobe, to name a few things, depend on the passthrough ioctl and there is more complete error information returned by the passthrough. I also have a vague recollection of some buffer size limitation with SG_IO I'm not sure, might be wrong about that, but I know flashing firmware takes some large buffers.

Quoted 9 lines
Quoted 3 lines

static __devinit int hpsa_hard_reset_controller(struct pci_dev *pdev) {

[...]

Quoted 2 lines
	set_current_state(TASK_UNINTERRUPTIBLE);
	schedule_timeout(HZ >> 1);

msleep(500) please .. This isn't the only place this occurs, could you replace all of them?

Ok.

Quoted 21 lines

in hpsa_cmd.h:

Quoted 18 lines

/* Unit Attentions ASC's as defined for the MSA2012sa */
#define POWER_OR_RESET 0x29
#define STATE_CHANGED 0x2a
#define UNIT_ATTENTION_CLEARED 0x2f
#define LUN_FAILED 0x3e
#define REPORT_LUNS_CHANGED 0x3f

/* Unit Attentions ASCQ's as defined for the MSA2012sa */

	/* These ASCQ's defined for ASC = POWER_OR_RESET */

#define POWER_ON_RESET 0x00
#define POWER_ON_REBOOT 0x01
#define SCSI_BUS_RESET 0x02
#define MSA_TARGET_RESET 0x03
#define CONTROLLER_FAILOVER 0x04
#define TRANSCEIVER_SE 0x05
#define TRANSCEIVER_LVD 0x06

Quoted 9 lines
Quoted 4 lines
	/* These ASCQ's defined for ASC = STATE_CHANGED */

#define RESERVATION_PREEMPTED 0x03
#define ASYM_ACCESS_CHANGED 0x06
#define LUN_CAPACITY_CHANGED 0x09

Traditionally we've shied away from putting ASC/ASCQ values into defines ... but these all look to be global not hpsa local, so they should be in a common central file.

Ok. I think there are some in there we didn't even use. We should probably omit them in those cases.

Quoted 4 lines

Otherwise looks OK to a cursory glance.

James