hpsa driver updates
Subsystems (2)
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
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
@@ -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
@@ -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.
PATCH 2/17 avoid helpful cleanup patches.
In reply to Stephen M. CameronFrom: Andrew Morton <[email protected]>
---
drivers/scsi/hpsa.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/scsi/hpsa.c +1 -1
@@ -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)
PATCH 3/17 hpsa: Fix vendor id check
In reply to Stephen M. Cameronhpsa: Fix vendor id check
---
drivers/scsi/hpsa.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/scsi/hpsa.c +1 -1
@@ -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",
PATCH 4/17 Fix use of unallocated memory for MSA2xxx enclosure device data.
In reply to Stephen M. CameronFix 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
@@ -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]; }
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.
PATCH 5/17 hpsa: Allocate the correct amount of extra space for the scsi host
In reply to Stephen M. Cameronhpsa: 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
@@ -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;
PATCH 6/17 hpsa: Use shost_priv instead of accessing host->hostdata[0] directly.
In reply to Stephen M. CameronUse 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
@@ -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 :
PATCH 7/17 hpsa: Factor out command submission sequence
In reply to Stephen M. Cameronhpsa: 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
@@ -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;
PATCH 8/17 hpsa: Factor out some pci_unmap code
In reply to Stephen M. Cameronhpsa: 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
@@ -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; }
PATCH 9/17 Add thread to allow controllers to register for rescan for new devices
In reply to Stephen M. CameronAdd 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
@@ -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
@@ -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[]; */
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 ; +} +
PATCH 10/17 hpsa: Allow device rescan to be triggered via sysfs.
In reply to Stephen M. CameronAllow 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
@@ -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) {
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?
PATCH 11/17 hpsa: Make hpsa_sdev_attrs static
In reply to Stephen M. Cameronhpsa: Make hpsa_sdev_attrs static
---
drivers/scsi/hpsa.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/scsi/hpsa.c +1 -1
@@ -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,
PATCH 12/17 hpsa: decode unit attention condition and retry commands.
In reply to Stephen M. Cameronhpsa: 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
@@ -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
@@ -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
PATCH 13/17 hpsa: Retry driver initiated commands on unit attention
In reply to Stephen M. Cameronhpsa: 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
@@ -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 ) {
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?
PATCH 14/17 hpsa: Flush cache with interrupts still enabled.
In reply to Stephen M. Cameronhpsa: 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
@@ -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)
PATCH 15/17 hpsa: Remove sendcmd, in no case are we required to poll for completions.
In reply to Stephen M. Cameronhpsa: 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
@@ -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)
PATCH 16/17 hpsa: Make fill_cmd() return void
In reply to Stephen M. Cameronhpsa: 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
@@ -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);
PATCH 17/17 hpsa: fix typo that causes scsi status to be lost
In reply to Stephen M. Cameronhpsa: 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
@@ -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",
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: Makefill_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: Makehpsa_sdev_attrsstatic
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 somepci_unmapcode
hpsa: Factor out command submission sequence
hpsa: Useshost_privinstead 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);
staticLIST_HEAD(scan_q);
static intscan_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)(structscsi_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
__devinitinthpsa_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 */
#definePOWER_OR_RESET0x29
#defineSTATE_CHANGED0x2a
#defineUNIT_ATTENTION_CLEARED0x2f
#defineLUN_FAILED0x3e
#defineREPORT_LUNS_CHANGED0x3f/* Unit Attentions ASCQ's as defined for the MSA2012sa */
/* These ASCQ's defined for ASC = POWER_OR_RESET */#define
POWER_ON_RESET0x00
#definePOWER_ON_REBOOT0x01
#defineSCSI_BUS_RESET0x02
#defineMSA_TARGET_RESET0x03
#defineCONTROLLER_FAILOVER0x04
#defineTRANSCEIVER_SE0x05
#defineTRANSCEIVER_LVD0x06
Quoted 4 lines
/* These ASCQ's defined for ASC = STATE_CHANGED */#define
RESERVATION_PREEMPTED0x03
#defineASYM_ACCESS_CHANGED0x06
#defineLUN_CAPACITY_CHANGED0x09
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
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.
---
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
@@ -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
@@ -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.
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: Makefill_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: Makehpsa_sdev_attrsstatic
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 somepci_unmapcode
hpsa: Factor out command submission sequence
hpsa: Useshost_privinstead 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);
staticLIST_HEAD(scan_q);
static intscan_thread(void *data);These names are too generic. We already have a
scan_mutexat 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)(structscsi_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_BUSYhere, 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_IOnow 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
__devinitinthpsa_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 */
#definePOWER_OR_RESET0x29
#defineSTATE_CHANGED0x2a
#defineUNIT_ATTENTION_CLEARED0x2f
#defineLUN_FAILED0x3e
#defineREPORT_LUNS_CHANGED0x3f/* Unit Attentions ASCQ's as defined for the MSA2012sa */
/* These ASCQ's defined for ASC = POWER_OR_RESET */#define
POWER_ON_RESET0x00
#definePOWER_ON_REBOOT0x01
#defineSCSI_BUS_RESET0x02
#defineMSA_TARGET_RESET0x03
#defineCONTROLLER_FAILOVER0x04
#defineTRANSCEIVER_SE0x05
#defineTRANSCEIVER_LVD0x06
Quoted 9 lines
Quoted 4 lines
/* These ASCQ's defined for ASC = STATE_CHANGED */#define
RESERVATION_PREEMPTED0x03
#defineASYM_ACCESS_CHANGED0x06
#defineLUN_CAPACITY_CHANGED0x09Traditionally 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