[PATCH 00/17] hpsa updates for April, 2012

[PATCH 00/17] hpsa updates for April, 2012

[Stephen M. Cameron]

The two big changes here are adding an abort handler, and using multiple
reply queues for command completions.  

Matt Bondurant (1):
      hpsa: retry driver initiated commands on busy status

Matt Gates (2):
      hpsa: use multiple reply queues
      hpsa: refine interrupt handler locking for greater concurrency

Mike Miller (1):
      hpsa: add new RAID level "1(ADM)"

Stephen M. Cameron (13):
      hpsa: call pci_disable_device on driver unload
      hpsa: do not skip disabled devices
      hpsa: enable bus master bit after pci_enable_device
      hpsa: suppress excessively chatty error messages
      hpsa: do not read from controller unnecessarily in completion code
      hpsa: remove unused parameter from finish_cmd
      hpsa: add abort error handler function
      hpsa: do aborts two ways
      hpsa: factor out tail calls to next_command() in process_(non)indexed_cmd()
      hpsa: factor out hpsa_free_irqs_and_disable_msix
      hpsa: use new IS_ENABLED macro
      hpsa: removed unused member maxQsinceinit
      hpsa: dial down lockup detection during firmware flash


 drivers/block/cciss.c   |    2 
 drivers/scsi/hpsa.c     |  689 +++++++++++++++++++++++++++++++++++++----------
 drivers/scsi/hpsa.h     |   85 ++++--
 drivers/scsi/hpsa_cmd.h |   37 ++-
 4 files changed, 643 insertions(+), 170 deletions(-)

-- 
-- steve

[PATCH 01/17] hpsa: call pci_disable_device on driver unload

[Stephen M. Cameron]

From: Stephen M. Cameron <scameron@beardog.cce.hp.com>

As Jenx Axboe explained to me: "In earlier times (2.6.18 and pre, iirc), Linux
disabled IO and mem bars on pci_disable_device(). Now in newer kernel it does
not. And in the newer kernels you run into problems if you DON'T disable the
device on exit, since when it later loads the device is already in the enabled
state - and pci_enable_device() then does nothing. This typically screws
MSI/MSI-X." This is what the big scary comment that says pci_disable_device
does "something nasty" to smart arrays was evidently referring to.

If pci_disable_device is not called on driver rmmod, subsequently insmod'ing
the driver may in result in some cases fail to be able to receive interrupts,
esp.  if other drivers are loaded between unloading and loading hpsa.

Signed-off-by: Stephen M. Cameron <scameron@beardog.cce.hp.com>
---
 drivers/scsi/hpsa.c |   10 ++--------
 1 files changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
index 500e20d..1a6c319 100644
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -3987,10 +3987,7 @@ err_out_free_res:
 		iounmap(h->cfgtable);
 	if (h->vaddr)
 		iounmap(h->vaddr);
-	/*
-	 * Deliberately omit pci_disable_device(): it does something nasty to
-	 * Smart Array controllers that pci_enable_device does not undo
-	 */
+	pci_disable_device(h->pdev);
 	pci_release_regions(h->pdev);
 	return err;
 }
@@ -4529,10 +4526,7 @@ static void __devexit hpsa_remove_one(struct pci_dev *pdev)
 	kfree(h->cmd_pool_bits);
 	kfree(h->blockFetchTable);
 	kfree(h->hba_inquiry_data);
-	/*
-	 * Deliberately omit pci_disable_device(): it does something nasty to
-	 * Smart Array controllers that pci_enable_device does not undo
-	 */
+	pci_disable_device(pdev);
 	pci_release_regions(pdev);
 	pci_set_drvdata(pdev, NULL);
 	kfree(h);

[PATCH 02/17] hpsa: do not skip disabled devices

[Stephen M. Cameron]

From: Stephen M. Cameron <scameron@beardog.cce.hp.com>

There was code to skip "disabled" devices which was intended to
skip devices disabled in the BIOS, but it really just checks to
see if the device can write to host memory, which this is disabled
by pci_disable_device on driver unload, so this check has the effect
of preventing subsequent load of the driver.  And devices disabled in
the BIOS don't show up at all anyway, so this check never made any
sense to begin with, and should be removed.

Signed-off-by: Stephen M. Cameron <scameron@beardog.cce.hp.com>
---
 drivers/scsi/hpsa.c |   13 -------------
 1 files changed, 0 insertions(+), 13 deletions(-)

diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
index 1a6c319..5119ce6 100644
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -3705,14 +3705,6 @@ static int __devinit hpsa_lookup_board_id(struct pci_dev *pdev, u32 *board_id)
 	return ARRAY_SIZE(products) - 1; /* generic unknown smart array */
 }
 
-static inline bool hpsa_board_disabled(struct pci_dev *pdev)
-{
-	u16 command;
-
-	(void) pci_read_config_word(pdev, PCI_COMMAND, &command);
-	return ((command & PCI_COMMAND_MEMORY) == 0);
-}
-
 static int __devinit hpsa_pci_find_memory_BAR(struct pci_dev *pdev,
 	unsigned long *memory_bar)
 {
@@ -3932,11 +3924,6 @@ static int __devinit hpsa_pci_init(struct ctlr_info *h)
 	h->product_name = products[prod_index].product_name;
 	h->access = *(products[prod_index].access);
 
-	if (hpsa_board_disabled(h->pdev)) {
-		dev_warn(&h->pdev->dev, "controller appears to be disabled\n");
-		return -ENODEV;
-	}
-
 	pci_disable_link_state(h->pdev, PCIE_LINK_STATE_L0S |
 			       PCIE_LINK_STATE_L1 | PCIE_LINK_STATE_CLKPM);
 

[PATCH 03/17] hpsa: enable bus master bit after pci_enable_device

[Stephen M. Cameron]

From: Stephen M. Cameron <scameron@beardog.cce.hp.com>

pci_disable_device() disables the bus master bit and pci_enable_device does
not re-enable it.  It needs to be enabled.

Signed-off-by: Stephen M. Cameron <scameron@beardog.cce.hp.com>
---
 drivers/scsi/hpsa.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
index 5119ce6..363619d 100644
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -3917,6 +3917,7 @@ static int __devinit hpsa_enter_simple_mode(struct ctlr_info *h)
 static int __devinit hpsa_pci_init(struct ctlr_info *h)
 {
 	int prod_index, err;
+	u16 command_register;
 
 	prod_index = hpsa_lookup_board_id(h->pdev, &h->board_id);
 	if (prod_index < 0)
@@ -3933,6 +3934,11 @@ static int __devinit hpsa_pci_init(struct ctlr_info *h)
 		return err;
 	}
 
+	/* Enable bus mastering (pci_disable_device may disable this) */
+	pci_read_config_word(h->pdev, PCI_COMMAND, &command_register);
+	command_register |= PCI_COMMAND_MASTER;
+	pci_write_config_word(h->pdev, PCI_COMMAND, command_register);
+
 	err = pci_request_regions(h->pdev, HPSA);
 	if (err) {
 		dev_err(&h->pdev->dev,

[PATCH 04/17] hpsa: suppress excessively chatty error messages

[Stephen M. Cameron]

From: Stephen M. Cameron <scameron@beardog.cce.hp.com>

Default behavior for any CHECK CONDITION excepting a few special cases is to
print out certain parts of the sense buffer and the CDB.  Default behavior
should be to print nothing and let the upper layers or applications decide what
to do about these.  The same information is already available by setting the
appropriate bits of the scsi_logging_level kernel parameter or via
/proc/sys/dev/scsi/logging_level.

Signed-off-by: Stephen M. Cameron <scameron@beardog.cce.hp.com>
---
 drivers/scsi/hpsa.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
index 906672b..fcb9fb2 100644
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -1193,7 +1193,7 @@ static void complete_scsi_command(struct CommandList *cp)
 				break;
 			}
 			/* Must be some other type of check condition */
-			dev_warn(&h->pdev->dev, "cp %p has check condition: "
+			dev_dbg(&h->pdev->dev, "cp %p has check condition: "
 					"unknown type: "
 					"Sense: 0x%x, ASC: 0x%x, ASCQ: 0x%x, "
 					"Returning result: 0x%x, "

[PATCH 05/17] hpsa: do not read from controller unnecessarily in completion code

[Stephen M. Cameron]

From: Stephen M. Cameron <scameron@beardog.cce.hp.com>

MSI/MSI-X interrupts can't race the DMA completion they are communicating
so no need to read from controller to flush the DMA to the host if
MSI or MSI-X interrupts are being used.

Signed-off-by: Stephen M. Cameron <scameron@beardog.cce.hp.com>
---
 drivers/scsi/hpsa.h |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/hpsa.h b/drivers/scsi/hpsa.h
index 7b28d54..48f7812 100644
--- a/drivers/scsi/hpsa.h
+++ b/drivers/scsi/hpsa.h
@@ -258,12 +258,12 @@ static unsigned long SA5_performant_completed(struct ctlr_info *h)
 {
 	unsigned long register_value = FIFO_EMPTY;
 
-	/* flush the controller write of the reply queue by reading
-	 * outbound doorbell status register.
-	 */
-	register_value = readl(h->vaddr + SA5_OUTDB_STATUS);
 	/* msi auto clears the interrupt pending bit. */
 	if (!(h->msi_vector || h->msix_vector)) {
+		/* flush the controller write of the reply queue by reading
+		 * outbound doorbell status register.
+		 */
+		register_value = readl(h->vaddr + SA5_OUTDB_STATUS);
 		writel(SA5_OUTDB_CLEAR_PERF_BIT, h->vaddr + SA5_OUTDB_CLEAR);
 		/* Do a read in order to flush the write to the controller
 		 * (as per spec.)

[PATCH 06/17] hpsa: retry driver initiated commands on busy status

[Stephen M. Cameron]

From: Matt Bondurant <Matthew.dav.bondurant@hp.com>

In shared SAS configurations we might get a busy status
during driver initiated commands (e.g. during rescan for
devices).  We should retry the command in such cases rather
than giving up.

Signed-off-by: Matt Bondurant <Matthew.dav.bondurant@hp.com>
Signed-off-by: Stephen M. Cameron <scameron@beardog.cce.hp.com>
---
 drivers/scsi/hpsa.c |   12 +++++++++++-
 1 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
index fcb9fb2..7355891 100644
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -234,6 +234,15 @@ static int check_for_unit_attention(struct ctlr_info *h,
 	return 1;
 }
 
+static int check_for_busy(struct ctlr_info *h, struct CommandList *c)
+{
+	if (c->err_info->CommandStatus != CMD_TARGET_STATUS ||
+		c->err_info->ScsiStatus != SAM_STAT_BUSY)
+		return 0;
+	dev_warn(&h->pdev->dev, HPSA "device busy");
+	return 1;
+}
+
 static ssize_t host_store_rescan(struct device *dev,
 				 struct device_attribute *attr,
 				 const char *buf, size_t count)
@@ -1379,7 +1388,8 @@ static void hpsa_scsi_do_simple_cmd_with_retry(struct ctlr_info *h,
 		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);
+	} while ((check_for_unit_attention(h, c) ||
+			check_for_busy(h, c)) && retry_count <= 3);
 	hpsa_pci_unmap(h->pdev, c, 1, data_direction);
 }
 

[PATCH 07/17] hpsa: remove unused parameter from finish_cmd

[Stephen M. Cameron]

From: Stephen M. Cameron <scameron@beardog.cce.hp.com>

Signed-off-by: Stephen M. Cameron <scameron@beardog.cce.hp.com>
---
 drivers/scsi/hpsa.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
index 7355891..e45bfda 100644
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -3055,7 +3055,7 @@ static inline int bad_tag(struct ctlr_info *h, u32 tag_index,
 	return 0;
 }
 
-static inline void finish_cmd(struct CommandList *c, u32 raw_tag)
+static inline void finish_cmd(struct CommandList *c)
 {
 	removeQ(c);
 	if (likely(c->cmd_type == CMD_SCSI))
@@ -3095,7 +3095,7 @@ static inline u32 process_indexed_cmd(struct ctlr_info *h,
 	if (bad_tag(h, tag_index, raw_tag))
 		return next_command(h);
 	c = h->cmd_pool + tag_index;
-	finish_cmd(c, raw_tag);
+	finish_cmd(c);
 	return next_command(h);
 }
 
@@ -3109,7 +3109,7 @@ static inline u32 process_nonindexed_cmd(struct ctlr_info *h,
 	tag = hpsa_tag_discard_error_bits(h, raw_tag);
 	list_for_each_entry(c, &h->cmpQ, list) {
 		if ((c->busaddr & 0xFFFFFFE0) == (tag & 0xFFFFFFE0)) {
-			finish_cmd(c, raw_tag);
+			finish_cmd(c);
 			return next_command(h);
 		}
 	}
@@ -4165,7 +4165,7 @@ static void fail_all_cmds_on_list(struct ctlr_info *h, struct list_head *list)
 	while (!list_empty(list)) {
 		c = list_entry(list->next, struct CommandList, list);
 		c->err_info->CommandStatus = CMD_HARDWARE_ERR;
-		finish_cmd(c, c->Header.Tag.lower);
+		finish_cmd(c);
 	}
 }
 

[PATCH 08/17] hpsa: add abort error handler function

[Stephen M. Cameron]

From: Stephen M. Cameron <scameron@beardog.cce.hp.com>

Signed-off-by: Stephen M. Cameron <scameron@beardog.cce.hp.com>
---
 drivers/scsi/hpsa.c     |  221 +++++++++++++++++++++++++++++++++++++++++++++++
 drivers/scsi/hpsa.h     |   21 ++++
 drivers/scsi/hpsa_cmd.h |   31 ++++++-
 3 files changed, 271 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
index e45bfda..9882ef4 100644
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -159,6 +159,7 @@ static int hpsa_change_queue_depth(struct scsi_device *sdev,
 	int qdepth, int reason);
 
 static int hpsa_eh_device_reset_handler(struct scsi_cmnd *scsicmd);
+static int hpsa_eh_abort_handler(struct scsi_cmnd *scsicmd);
 static int hpsa_slave_alloc(struct scsi_device *sdev);
 static void hpsa_slave_destroy(struct scsi_device *sdev);
 
@@ -180,6 +181,7 @@ static int __devinit hpsa_pci_find_memory_BAR(struct pci_dev *pdev,
 static int __devinit hpsa_lookup_board_id(struct pci_dev *pdev, u32 *board_id);
 static int __devinit hpsa_wait_for_board_state(struct pci_dev *pdev,
 	void __iomem *vaddr, int wait_for_ready);
+static inline void finish_cmd(struct CommandList *c);
 #define BOARD_NOT_READY 0
 #define BOARD_READY 1
 
@@ -506,6 +508,7 @@ static struct scsi_host_template hpsa_driver_template = {
 	.change_queue_depth	= hpsa_change_queue_depth,
 	.this_id		= -1,
 	.use_clustering		= ENABLE_CLUSTERING,
+	.eh_abort_handler	= hpsa_eh_abort_handler,
 	.eh_device_reset_handler = hpsa_eh_device_reset_handler,
 	.ioctl			= hpsa_ioctl,
 	.slave_alloc		= hpsa_slave_alloc,
@@ -2344,6 +2347,191 @@ static int hpsa_eh_device_reset_handler(struct scsi_cmnd *scsicmd)
 	return FAILED;
 }
 
+static int hpsa_send_abort(struct ctlr_info *h, unsigned char *scsi3addr,
+	struct CommandList *abort)
+{
+	int rc = IO_OK;
+	struct CommandList *c;
+	struct ErrorInfo *ei;
+
+	c = cmd_special_alloc(h);
+	if (c == NULL) {	/* trouble... */
+		dev_warn(&h->pdev->dev, "cmd_special_alloc returned NULL!\n");
+		return -ENOMEM;
+	}
+
+	fill_cmd(c, HPSA_ABORT_MSG, h, abort, 0, 0, scsi3addr, TYPE_MSG);
+	hpsa_scsi_do_simple_cmd_core(h, c);
+	dev_dbg(&h->pdev->dev, "%s: Tag:0x%08x:%08x: do_simple_cmd_core completed.\n",
+		__func__, abort->Header.Tag.upper, abort->Header.Tag.lower);
+	/* no unmap needed here because no data xfer. */
+
+	ei = c->err_info;
+	switch (ei->CommandStatus) {
+	case CMD_SUCCESS:
+		break;
+	case CMD_UNABORTABLE: /* Very common, don't make noise. */
+		rc = -1;
+		break;
+	default:
+		dev_dbg(&h->pdev->dev, "%s: Tag:0x%08x:%08x: interpreting error.\n",
+			__func__, abort->Header.Tag.upper,
+			abort->Header.Tag.lower);
+		hpsa_scsi_interpret_error(c);
+		rc = -1;
+		break;
+	}
+	cmd_special_free(h, c);
+	dev_dbg(&h->pdev->dev, "%s: Tag:0x%08x:%08x: Finished.\n", __func__,
+		abort->Header.Tag.upper, abort->Header.Tag.lower);
+	return rc;
+}
+
+/*
+ * hpsa_find_cmd_in_queue
+ *
+ * Used to determine whether a command (find) is still present
+ * in queue_head.   Optionally excludes the last element of queue_head.
+ *
+ * This is used to avoid unnecessary aborts.  Commands in h->reqQ have
+ * not yet been submitted, and so can be aborted by the driver without
+ * sending an abort to the hardware.
+ *
+ * Returns pointer to command if found in queue, NULL otherwise.
+ */
+static struct CommandList *hpsa_find_cmd_in_queue(struct ctlr_info *h,
+			struct scsi_cmnd *find, struct list_head *queue_head)
+{
+	unsigned long flags;
+	struct CommandList *c = NULL;	/* ptr into cmpQ */
+
+	if (!find)
+		return 0;
+	spin_lock_irqsave(&h->lock, flags);
+	list_for_each_entry(c, queue_head, list) {
+		if (c->scsi_cmd == NULL) /* e.g.: passthru ioctl */
+			continue;
+		if (c->scsi_cmd == find) {
+			spin_unlock_irqrestore(&h->lock, flags);
+			return c;
+		}
+	}
+	spin_unlock_irqrestore(&h->lock, flags);
+	return NULL;
+}
+
+/* Send an abort for the specified command.
+ *	If the device and controller support it,
+ *		send a task abort request.
+ */
+static int hpsa_eh_abort_handler(struct scsi_cmnd *sc)
+{
+
+	int i, rc;
+	struct ctlr_info *h;
+	struct hpsa_scsi_dev_t *dev;
+	struct CommandList *abort; /* pointer to command to be aborted */
+	struct CommandList *found;
+	struct scsi_cmnd *as;	/* ptr to scsi cmd inside aborted command. */
+	char msg[256];		/* For debug messaging. */
+	int ml = 0;
+
+	/* Find the controller of the command to be aborted */
+	h = sdev_to_hba(sc->device);
+	if (WARN(h == NULL,
+			"ABORT REQUEST FAILED, Controller lookup failed.\n"))
+		return FAILED;
+
+	/* Check that controller supports some kind of task abort */
+	if (!(HPSATMF_PHYS_TASK_ABORT & h->TMFSupportFlags) &&
+		!(HPSATMF_LOG_TASK_ABORT & h->TMFSupportFlags))
+		return FAILED;
+
+	memset(msg, 0, sizeof(msg));
+	ml += sprintf(msg+ml, "ABORT REQUEST on C%d:B%d:T%d:L%d ",
+		h->scsi_host->host_no, sc->device->channel,
+		sc->device->id, sc->device->lun);
+
+	/* Find the device of the command to be aborted */
+	dev = sc->device->hostdata;
+	if (!dev) {
+		dev_err(&h->pdev->dev, "%s FAILED, Device lookup failed.\n",
+				msg);
+		return FAILED;
+	}
+
+	/* Get SCSI command to be aborted */
+	abort = (struct CommandList *) sc->host_scribble;
+	if (abort == NULL) {
+		dev_err(&h->pdev->dev, "%s FAILED, Command to abort is NULL.\n",
+				msg);
+		return FAILED;
+	}
+
+	ml += sprintf(msg+ml, "Tag:0x%08x:%08x ",
+		abort->Header.Tag.upper, abort->Header.Tag.lower);
+	as  = (struct scsi_cmnd *) abort->scsi_cmd;
+	if (as != NULL)
+		ml += sprintf(msg+ml, "Command:0x%x SN:0x%lx ",
+			as->cmnd[0], as->serial_number);
+	dev_dbg(&h->pdev->dev, "%s\n", msg);
+	dev_warn(&h->pdev->dev, "Abort request on C%d:B%d:T%d:L%d\n",
+		h->scsi_host->host_no, dev->bus, dev->target, dev->lun);
+
+	/* Search reqQ to See if command is queued but not submitted,
+	 * if so, complete the command with aborted status and remove
+	 * it from the reqQ.
+	 */
+	found = hpsa_find_cmd_in_queue(h, sc, &h->reqQ);
+	if (found) {
+		found->err_info->CommandStatus = CMD_ABORTED;
+		finish_cmd(found);
+		dev_info(&h->pdev->dev, "%s Request SUCCEEDED (driver queue).\n",
+				msg);
+		return SUCCESS;
+	}
+
+	/* not in reqQ, if also not in cmpQ, must have already completed */
+	found = hpsa_find_cmd_in_queue(h, sc, &h->cmpQ);
+	if (!found)  {
+		dev_dbg(&h->pdev->dev, "%s Request FAILED (not known to driver).\n",
+				msg);
+		return SUCCESS;
+	}
+
+	/*
+	 * Command is in flight, or possibly already completed
+	 * by the firmware (but not to the scsi mid layer) but we can't
+	 * distinguish which.  Send the abort down.
+	 */
+	rc = hpsa_send_abort(h, dev->scsi3addr, abort);
+	if (rc != 0) {
+		dev_dbg(&h->pdev->dev, "%s Request FAILED.\n", msg);
+		dev_warn(&h->pdev->dev, "FAILED abort on device C%d:B%d:T%d:L%d\n",
+			h->scsi_host->host_no,
+			dev->bus, dev->target, dev->lun);
+		return FAILED;
+	}
+	dev_info(&h->pdev->dev, "%s REQUEST SUCCEEDED.\n", msg);
+
+	/* If the abort(s) above completed and actually aborted the
+	 * command, then the command to be aborted should already be
+	 * completed.  If not, wait around a bit more to see if they
+	 * manage to complete normally.
+	 */
+#define ABORT_COMPLETE_WAIT_SECS 30
+	for (i = 0; i < ABORT_COMPLETE_WAIT_SECS * 10; i++) {
+		found = hpsa_find_cmd_in_queue(h, sc, &h->cmpQ);
+		if (!found)
+			return SUCCESS;
+		msleep(100);
+	}
+	dev_warn(&h->pdev->dev, "%s FAILED. Aborted command has not completed after %d seconds.\n",
+		msg, ABORT_COMPLETE_WAIT_SECS);
+	return FAILED;
+}
+
+
 /*
  * For operations that cannot sleep, a command block is allocated at init,
  * and managed by cmd_alloc() and cmd_free() using a simple bitmap to track
@@ -2876,6 +3064,7 @@ static void fill_cmd(struct CommandList *c, u8 cmd, struct ctlr_info *h,
 	int cmd_type)
 {
 	int pci_dir = XFER_NONE;
+	struct CommandList *a; /* for commands to be aborted */
 
 	c->cmd_type = CMD_IOCTL_PEND;
 	c->Header.ReplyQueue = 0;
@@ -2959,8 +3148,35 @@ static void fill_cmd(struct CommandList *c, u8 cmd, struct ctlr_info *h,
 			c->Request.CDB[5] = 0x00;
 			c->Request.CDB[6] = 0x00;
 			c->Request.CDB[7] = 0x00;
+			break;
+		case  HPSA_ABORT_MSG:
+			a = buff;       /* point to command to be aborted */
+			dev_dbg(&h->pdev->dev, "Abort Tag:0x%08x:%08x using request Tag:0x%08x:%08x\n",
+				a->Header.Tag.upper, a->Header.Tag.lower,
+				c->Header.Tag.upper, c->Header.Tag.lower);
+			c->Request.CDBLen = 16;
+			c->Request.Type.Type = TYPE_MSG;
+			c->Request.Type.Attribute = ATTR_SIMPLE;
+			c->Request.Type.Direction = XFER_WRITE;
+			c->Request.Timeout = 0; /* Don't time out */
+			c->Request.CDB[0] = HPSA_TASK_MANAGEMENT;
+			c->Request.CDB[1] = HPSA_TMF_ABORT_TASK;
+			c->Request.CDB[2] = 0x00; /* reserved */
+			c->Request.CDB[3] = 0x00; /* reserved */
+			/* Tag to abort goes in CDB[4]-CDB[11] */
+			c->Request.CDB[4] = a->Header.Tag.lower & 0xFF;
+			c->Request.CDB[5] = (a->Header.Tag.lower >> 8) & 0xFF;
+			c->Request.CDB[6] = (a->Header.Tag.lower >> 16) & 0xFF;
+			c->Request.CDB[7] = (a->Header.Tag.lower >> 24) & 0xFF;
+			c->Request.CDB[8] = a->Header.Tag.upper & 0xFF;
+			c->Request.CDB[9] = (a->Header.Tag.upper >> 8) & 0xFF;
+			c->Request.CDB[10] = (a->Header.Tag.upper >> 16) & 0xFF;
+			c->Request.CDB[11] = (a->Header.Tag.upper >> 24) & 0xFF;
+			c->Request.CDB[12] = 0x00; /* reserved */
+			c->Request.CDB[13] = 0x00; /* reserved */
+			c->Request.CDB[14] = 0x00; /* reserved */
+			c->Request.CDB[15] = 0x00; /* reserved */
 		break;
-
 		default:
 			dev_warn(&h->pdev->dev, "unknown message type %d\n",
 				cmd);
@@ -3840,6 +4056,9 @@ static void __devinit hpsa_find_board_params(struct ctlr_info *h)
 		h->maxsgentries = 31; /* default to traditional values */
 		h->chainsize = 0;
 	}
+
+	/* Find out what task management functions are supported and cache */
+	h->TMFSupportFlags = readl(&(h->cfgtable->TMFSupportFlags));
 }
 
 static inline bool hpsa_CISS_signature_present(struct ctlr_info *h)
diff --git a/drivers/scsi/hpsa.h b/drivers/scsi/hpsa.h
index 48f7812..d8aa95c 100644
--- a/drivers/scsi/hpsa.h
+++ b/drivers/scsi/hpsa.h
@@ -125,6 +125,27 @@ struct ctlr_info {
 	u64 last_heartbeat_timestamp;
 	u32 lockup_detected;
 	struct list_head lockup_list;
+	u32 TMFSupportFlags; /* cache what task mgmt funcs are supported. */
+#define HPSATMF_BITS_SUPPORTED  (1 << 0)
+#define HPSATMF_PHYS_LUN_RESET  (1 << 1)
+#define HPSATMF_PHYS_NEX_RESET  (1 << 2)
+#define HPSATMF_PHYS_TASK_ABORT (1 << 3)
+#define HPSATMF_PHYS_TSET_ABORT (1 << 4)
+#define HPSATMF_PHYS_CLEAR_ACA  (1 << 5)
+#define HPSATMF_PHYS_CLEAR_TSET (1 << 6)
+#define HPSATMF_PHYS_QRY_TASK   (1 << 7)
+#define HPSATMF_PHYS_QRY_TSET   (1 << 8)
+#define HPSATMF_PHYS_QRY_ASYNC  (1 << 9)
+#define HPSATMF_MASK_SUPPORTED  (1 << 16)
+#define HPSATMF_LOG_LUN_RESET   (1 << 17)
+#define HPSATMF_LOG_NEX_RESET   (1 << 18)
+#define HPSATMF_LOG_TASK_ABORT  (1 << 19)
+#define HPSATMF_LOG_TSET_ABORT  (1 << 20)
+#define HPSATMF_LOG_CLEAR_ACA   (1 << 21)
+#define HPSATMF_LOG_CLEAR_TSET  (1 << 22)
+#define HPSATMF_LOG_QRY_TASK    (1 << 23)
+#define HPSATMF_LOG_QRY_TSET    (1 << 24)
+#define HPSATMF_LOG_QRY_ASYNC   (1 << 25)
 };
 #define HPSA_ABORT_MSG 0
 #define HPSA_DEVICE_RESET_MSG 1
diff --git a/drivers/scsi/hpsa_cmd.h b/drivers/scsi/hpsa_cmd.h
index 8049815..14b56c9 100644
--- a/drivers/scsi/hpsa_cmd.h
+++ b/drivers/scsi/hpsa_cmd.h
@@ -82,6 +82,29 @@
 #define TYPE_CMD				0x00
 #define TYPE_MSG				0x01
 
+/* Message Types  */
+#define HPSA_TASK_MANAGEMENT    0x00
+#define HPSA_RESET              0x01
+#define HPSA_SCAN               0x02
+#define HPSA_NOOP               0x03
+
+#define HPSA_CTLR_RESET_TYPE    0x00
+#define HPSA_BUS_RESET_TYPE     0x01
+#define HPSA_TARGET_RESET_TYPE  0x03
+#define HPSA_LUN_RESET_TYPE     0x04
+#define HPSA_NEXUS_RESET_TYPE   0x05
+
+/* Task Management Functions */
+#define HPSA_TMF_ABORT_TASK     0x00
+#define HPSA_TMF_ABORT_TASK_SET 0x01
+#define HPSA_TMF_CLEAR_ACA      0x02
+#define HPSA_TMF_CLEAR_TASK_SET 0x03
+#define HPSA_TMF_QUERY_TASK     0x04
+#define HPSA_TMF_QUERY_TASK_SET 0x05
+#define HPSA_TMF_QUERY_ASYNCEVENT 0x06
+
+
+
 /* config space register offsets */
 #define CFG_VENDORID            0x00
 #define CFG_DEVICEID            0x02
@@ -337,11 +360,17 @@ struct CfgTable {
 	u32		MaxPhysicalDevices;
 	u32		MaxPhysicalDrivesPerLogicalUnit;
 	u32		MaxPerformantModeCommands;
-	u8		reserved[0x78 - 0x58];
+	u32		MaxBlockFetch;
+	u32		PowerConservationSupport;
+	u32		PowerConservationEnable;
+	u32		TMFSupportFlags;
+	u8		TMFTagMask[8];
+	u8		reserved[0x78 - 0x70];
 	u32		misc_fw_support; /* offset 0x78 */
 #define			MISC_FW_DOORBELL_RESET (0x02)
 #define			MISC_FW_DOORBELL_RESET2 (0x010)
 	u8		driver_version[32];
+
 };
 
 #define NUM_BLOCKFETCH_ENTRIES 8

[PATCH 09/17] hpsa: do aborts two ways

[Stephen M. Cameron]

From: Stephen M. Cameron <scameron@beardog.cce.hp.com>

When aborting a command, the tag is supposed to be
specified as 64-bit little endian.  However, some smart
arrays expect the tag of the command to be aborted to be
specified in a strange byte order.  How to tell which sort
of Smart Array firmware we're dealing with is not obvious.
However, because of the way we construct our tags, the values
of any outstanding tag when specified with the "strange" byte
order will not collide with the value specified in the correct
order.  That means we can safely attempt the abort both ways.

Signed-off-by: Stephen M. Cameron <stephenmcameron@gmail.com>
---
 drivers/scsi/hpsa.c |   74 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 72 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
index 9882ef4..c657642 100644
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -2347,8 +2347,23 @@ static int hpsa_eh_device_reset_handler(struct scsi_cmnd *scsicmd)
 	return FAILED;
 }
 
+static void swizzle_abort_tag(u8 *tag)
+{
+	u8 original_tag[8];
+
+	memcpy(original_tag, tag, 8);
+	tag[0] = original_tag[3];
+	tag[1] = original_tag[2];
+	tag[2] = original_tag[1];
+	tag[3] = original_tag[0];
+	tag[4] = original_tag[7];
+	tag[5] = original_tag[6];
+	tag[6] = original_tag[5];
+	tag[7] = original_tag[4];
+}
+
 static int hpsa_send_abort(struct ctlr_info *h, unsigned char *scsi3addr,
-	struct CommandList *abort)
+	struct CommandList *abort, int swizzle)
 {
 	int rc = IO_OK;
 	struct CommandList *c;
@@ -2361,6 +2376,8 @@ static int hpsa_send_abort(struct ctlr_info *h, unsigned char *scsi3addr,
 	}
 
 	fill_cmd(c, HPSA_ABORT_MSG, h, abort, 0, 0, scsi3addr, TYPE_MSG);
+	if (swizzle)
+		swizzle_abort_tag(&c->Request.CDB[4]);
 	hpsa_scsi_do_simple_cmd_core(h, c);
 	dev_dbg(&h->pdev->dev, "%s: Tag:0x%08x:%08x: do_simple_cmd_core completed.\n",
 		__func__, abort->Header.Tag.upper, abort->Header.Tag.lower);
@@ -2420,6 +2437,59 @@ static struct CommandList *hpsa_find_cmd_in_queue(struct ctlr_info *h,
 	return NULL;
 }
 
+static struct CommandList *hpsa_find_cmd_in_queue_by_tag(struct ctlr_info *h,
+					u8 *tag, struct list_head *queue_head)
+{
+	unsigned long flags;
+	struct CommandList *c;
+
+	spin_lock_irqsave(&h->lock, flags);
+	list_for_each_entry(c, queue_head, list) {
+		if (memcmp(&c->Header.Tag, tag, 8) != 0)
+			continue;
+		spin_unlock_irqrestore(&h->lock, flags);
+		return c;
+	}
+	spin_unlock_irqrestore(&h->lock, flags);
+	return NULL;
+}
+
+/* Some Smart Arrays need the abort tag swizzled, and some don't.  It's hard to
+ * tell which kind we're dealing with, so we send the abort both ways.  There
+ * shouldn't be any collisions between swizzled and unswizzled tags due to the
+ * way we construct our tags but we check anyway in case the assumptions which
+ * make this true someday become false.
+ */
+static int hpsa_send_abort_both_ways(struct ctlr_info *h,
+	unsigned char *scsi3addr, struct CommandList *abort)
+{
+	u8 swizzled_tag[8];
+	struct CommandList *c;
+	int rc = 0, rc2 = 0;
+
+	/* we do not expect to find the swizzled tag in our queue, but
+	 * check anyway just to be sure the assumptions which make this
+	 * the case haven't become wrong.
+	 */
+	memcpy(swizzled_tag, &abort->Request.CDB[4], 8);
+	swizzle_abort_tag(swizzled_tag);
+	c = hpsa_find_cmd_in_queue_by_tag(h, swizzled_tag, &h->cmpQ);
+	if (c != NULL) {
+		dev_warn(&h->pdev->dev, "Unexpectedly found byte-swapped tag in completion queue.\n");
+		return hpsa_send_abort(h, scsi3addr, abort, 0);
+	}
+	rc = hpsa_send_abort(h, scsi3addr, abort, 0);
+
+	/* if the command is still in our queue, we can't conclude that it was
+	 * aborted (it might have just completed normally) but in any case
+	 * we don't need to try to abort it another way.
+	 */
+	c = hpsa_find_cmd_in_queue(h, abort->scsi_cmd, &h->cmpQ);
+	if (c)
+		rc2 = hpsa_send_abort(h, scsi3addr, abort, 1);
+	return rc && rc2;
+}
+
 /* Send an abort for the specified command.
  *	If the device and controller support it,
  *		send a task abort request.
@@ -2504,7 +2574,7 @@ static int hpsa_eh_abort_handler(struct scsi_cmnd *sc)
 	 * by the firmware (but not to the scsi mid layer) but we can't
 	 * distinguish which.  Send the abort down.
 	 */
-	rc = hpsa_send_abort(h, dev->scsi3addr, abort);
+	rc = hpsa_send_abort_both_ways(h, dev->scsi3addr, abort);
 	if (rc != 0) {
 		dev_dbg(&h->pdev->dev, "%s Request FAILED.\n", msg);
 		dev_warn(&h->pdev->dev, "FAILED abort on device C%d:B%d:T%d:L%d\n",

[PATCH 10/17] hpsa: factor out tail calls to next_command() in process_(non)indexed_cmd()

[Stephen M. Cameron]

From: Stephen M. Cameron <scameron@beardog.cce.hp.com>

This is in order to smooth the way for upcoming changes to allow use of
multiple reply queues for command completions.

Signed-off-by: Stephen M. Cameron <scameron@beardog.cce.hp.com>
Signed-off-by: Matt Gates <matthew.gates@hp.com>
---
 drivers/scsi/hpsa.c |   30 +++++++++++++++---------------
 1 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
index c657642..96d07d4 100644
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -3371,22 +3371,21 @@ static inline u32 hpsa_tag_discard_error_bits(struct ctlr_info *h, u32 tag)
 }
 
 /* process completion of an indexed ("direct lookup") command */
-static inline u32 process_indexed_cmd(struct ctlr_info *h,
+static inline void process_indexed_cmd(struct ctlr_info *h,
 	u32 raw_tag)
 {
 	u32 tag_index;
 	struct CommandList *c;
 
 	tag_index = hpsa_tag_to_index(raw_tag);
-	if (bad_tag(h, tag_index, raw_tag))
-		return next_command(h);
-	c = h->cmd_pool + tag_index;
-	finish_cmd(c);
-	return next_command(h);
+	if (!bad_tag(h, tag_index, raw_tag)) {
+		c = h->cmd_pool + tag_index;
+		finish_cmd(c);
+	}
 }
 
 /* process completion of a non-indexed command */
-static inline u32 process_nonindexed_cmd(struct ctlr_info *h,
+static inline void process_nonindexed_cmd(struct ctlr_info *h,
 	u32 raw_tag)
 {
 	u32 tag;
@@ -3396,11 +3395,10 @@ static inline u32 process_nonindexed_cmd(struct ctlr_info *h,
 	list_for_each_entry(c, &h->cmpQ, list) {
 		if ((c->busaddr & 0xFFFFFFE0) == (tag & 0xFFFFFFE0)) {
 			finish_cmd(c);
-			return next_command(h);
+			return;
 		}
 	}
 	bad_tag(h, h->nr_cmds + 1, raw_tag);
-	return next_command(h);
 }
 
 /* Some controllers, like p400, will give us one interrupt
@@ -3475,10 +3473,11 @@ static irqreturn_t do_hpsa_intr_intx(int irq, void *dev_id)
 	while (interrupt_pending(h)) {
 		raw_tag = get_next_completion(h);
 		while (raw_tag != FIFO_EMPTY) {
-			if (hpsa_tag_contains_index(raw_tag))
-				raw_tag = process_indexed_cmd(h, raw_tag);
+			if (likely(hpsa_tag_contains_index(raw_tag)))
+				process_indexed_cmd(h, raw_tag);
 			else
-				raw_tag = process_nonindexed_cmd(h, raw_tag);
+				process_nonindexed_cmd(h, raw_tag);
+			raw_tag = next_command(h);
 		}
 	}
 	spin_unlock_irqrestore(&h->lock, flags);
@@ -3495,10 +3494,11 @@ static irqreturn_t do_hpsa_intr_msi(int irq, void *dev_id)
 	h->last_intr_timestamp = get_jiffies_64();
 	raw_tag = get_next_completion(h);
 	while (raw_tag != FIFO_EMPTY) {
-		if (hpsa_tag_contains_index(raw_tag))
-			raw_tag = process_indexed_cmd(h, raw_tag);
+		if (likely(hpsa_tag_contains_index(raw_tag)))
+			process_indexed_cmd(h, raw_tag);
 		else
-			raw_tag = process_nonindexed_cmd(h, raw_tag);
+			process_nonindexed_cmd(h, raw_tag);
+		raw_tag = next_command(h);
 	}
 	spin_unlock_irqrestore(&h->lock, flags);
 	return IRQ_HANDLED;

[PATCH 11/17] hpsa: use multiple reply queues

[Stephen M. Cameron]

From: Matt Gates <matthew.gates@hp.com>

Smart Arrays can support multiple reply queues onto which command
completions may be deposited.  It can help performance quite a bit
to arrange for command completions to be processed on the same CPU
from which they were submitted to increase the likelihood of cache
hits.

Signed-off-by: Matt Gates <matthew.gates@hp.com>
Signed-off-by: Stephen M. Cameron <scameron@beardog.cce.hp.com>
---
 drivers/scsi/hpsa.c     |  181 ++++++++++++++++++++++++++++++++---------------
 drivers/scsi/hpsa.h     |   40 ++++++----
 drivers/scsi/hpsa_cmd.h |    5 +
 3 files changed, 153 insertions(+), 73 deletions(-)

diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
index 96d07d4..29bbf35 100644
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -172,7 +172,7 @@ static void check_ioctl_unit_attention(struct ctlr_info *h,
 static void calc_bucket_map(int *bucket, int num_buckets,
 	int nsgs, int *bucket_map);
 static __devinit void hpsa_put_ctlr_into_performant_mode(struct ctlr_info *h);
-static inline u32 next_command(struct ctlr_info *h);
+static inline u32 next_command(struct ctlr_info *h, u8 q);
 static int __devinit hpsa_find_cfg_addrs(struct pci_dev *pdev,
 	void __iomem *vaddr, u32 *cfg_base_addr, u64 *cfg_base_addr_index,
 	u64 *cfg_offset);
@@ -528,24 +528,25 @@ static inline void addQ(struct list_head *list, struct CommandList *c)
 	list_add_tail(&c->list, list);
 }
 
-static inline u32 next_command(struct ctlr_info *h)
+static inline u32 next_command(struct ctlr_info *h, u8 q)
 {
 	u32 a;
+	struct reply_pool *rq = &h->reply_queue[q];
 
 	if (unlikely(!(h->transMethod & CFGTBL_Trans_Performant)))
-		return h->access.command_completed(h);
+		return h->access.command_completed(h, q);
 
-	if ((*(h->reply_pool_head) & 1) == (h->reply_pool_wraparound)) {
-		a = *(h->reply_pool_head); /* Next cmd in ring buffer */
-		(h->reply_pool_head)++;
+	if ((rq->head[rq->current_entry] & 1) == rq->wraparound) {
+		a = rq->head[rq->current_entry];
+		rq->current_entry++;
 		h->commands_outstanding--;
 	} else {
 		a = FIFO_EMPTY;
 	}
 	/* Check for wraparound */
-	if (h->reply_pool_head == (h->reply_pool + h->max_commands)) {
-		h->reply_pool_head = h->reply_pool;
-		h->reply_pool_wraparound ^= 1;
+	if (rq->current_entry == h->max_commands) {
+		rq->current_entry = 0;
+		rq->wraparound ^= 1;
 	}
 	return a;
 }
@@ -556,8 +557,12 @@ static inline u32 next_command(struct ctlr_info *h)
  */
 static void set_performant_mode(struct ctlr_info *h, struct CommandList *c)
 {
-	if (likely(h->transMethod & CFGTBL_Trans_Performant))
+	if (likely(h->transMethod & CFGTBL_Trans_Performant)) {
 		c->busaddr |= 1 | (h->blockFetchTable[c->Header.SGList] << 1);
+		if (likely(h->msix_vector))
+			c->Header.ReplyQueue =
+				smp_processor_id() % h->nreply_queues;
+	}
 }
 
 static void enqueue_cmd_and_start_io(struct ctlr_info *h,
@@ -3315,9 +3320,9 @@ static void start_io(struct ctlr_info *h)
 	}
 }
 
-static inline unsigned long get_next_completion(struct ctlr_info *h)
+static inline unsigned long get_next_completion(struct ctlr_info *h, u8 q)
 {
-	return h->access.command_completed(h);
+	return h->access.command_completed(h, q);
 }
 
 static inline bool interrupt_pending(struct ctlr_info *h)
@@ -3420,9 +3425,20 @@ static int ignore_bogus_interrupt(struct ctlr_info *h)
 	return 1;
 }
 
-static irqreturn_t hpsa_intx_discard_completions(int irq, void *dev_id)
+/*
+ * Convert &h->q[x] (passed to interrupt handlers) back to h.
+ * Relies on (h-q[x] == x) being true for x such that
+ * 0 <= x < MAX_REPLY_QUEUES.
+ */
+static struct ctlr_info *queue_to_hba(u8 *queue)
 {
-	struct ctlr_info *h = dev_id;
+	return container_of((queue - *queue), struct ctlr_info, q[0]);
+}
+
+static irqreturn_t hpsa_intx_discard_completions(int irq, void *queue)
+{
+	struct ctlr_info *h = queue_to_hba(queue);
+	u8 q = *(u8 *) queue;
 	unsigned long flags;
 	u32 raw_tag;
 
@@ -3434,71 +3450,75 @@ static irqreturn_t hpsa_intx_discard_completions(int irq, void *dev_id)
 	spin_lock_irqsave(&h->lock, flags);
 	h->last_intr_timestamp = get_jiffies_64();
 	while (interrupt_pending(h)) {
-		raw_tag = get_next_completion(h);
+		raw_tag = get_next_completion(h, q);
 		while (raw_tag != FIFO_EMPTY)
-			raw_tag = next_command(h);
+			raw_tag = next_command(h, q);
 	}
 	spin_unlock_irqrestore(&h->lock, flags);
 	return IRQ_HANDLED;
 }
 
-static irqreturn_t hpsa_msix_discard_completions(int irq, void *dev_id)
+static irqreturn_t hpsa_msix_discard_completions(int irq, void *queue)
 {
-	struct ctlr_info *h = dev_id;
+	struct ctlr_info *h = queue_to_hba(queue);
 	unsigned long flags;
 	u32 raw_tag;
+	u8 q = *(u8 *) queue;
 
 	if (ignore_bogus_interrupt(h))
 		return IRQ_NONE;
 
 	spin_lock_irqsave(&h->lock, flags);
+
 	h->last_intr_timestamp = get_jiffies_64();
-	raw_tag = get_next_completion(h);
+	raw_tag = get_next_completion(h, q);
 	while (raw_tag != FIFO_EMPTY)
-		raw_tag = next_command(h);
+		raw_tag = next_command(h, q);
 	spin_unlock_irqrestore(&h->lock, flags);
 	return IRQ_HANDLED;
 }
 
-static irqreturn_t do_hpsa_intr_intx(int irq, void *dev_id)
+static irqreturn_t do_hpsa_intr_intx(int irq, void *queue)
 {
-	struct ctlr_info *h = dev_id;
+	struct ctlr_info *h = queue_to_hba((u8 *) queue);
 	unsigned long flags;
 	u32 raw_tag;
+	u8 q = *(u8 *) queue;
 
 	if (interrupt_not_for_us(h))
 		return IRQ_NONE;
 	spin_lock_irqsave(&h->lock, flags);
 	h->last_intr_timestamp = get_jiffies_64();
 	while (interrupt_pending(h)) {
-		raw_tag = get_next_completion(h);
+		raw_tag = get_next_completion(h, q);
 		while (raw_tag != FIFO_EMPTY) {
 			if (likely(hpsa_tag_contains_index(raw_tag)))
 				process_indexed_cmd(h, raw_tag);
 			else
 				process_nonindexed_cmd(h, raw_tag);
-			raw_tag = next_command(h);
+			raw_tag = next_command(h, q);
 		}
 	}
 	spin_unlock_irqrestore(&h->lock, flags);
 	return IRQ_HANDLED;
 }
 
-static irqreturn_t do_hpsa_intr_msi(int irq, void *dev_id)
+static irqreturn_t do_hpsa_intr_msi(int irq, void *queue)
 {
-	struct ctlr_info *h = dev_id;
+	struct ctlr_info *h = queue_to_hba(queue);
 	unsigned long flags;
 	u32 raw_tag;
+	u8 q = *(u8 *) queue;
 
 	spin_lock_irqsave(&h->lock, flags);
 	h->last_intr_timestamp = get_jiffies_64();
-	raw_tag = get_next_completion(h);
+	raw_tag = get_next_completion(h, q);
 	while (raw_tag != FIFO_EMPTY) {
 		if (likely(hpsa_tag_contains_index(raw_tag)))
 			process_indexed_cmd(h, raw_tag);
 		else
 			process_nonindexed_cmd(h, raw_tag);
-		raw_tag = next_command(h);
+		raw_tag = next_command(h, q);
 	}
 	spin_unlock_irqrestore(&h->lock, flags);
 	return IRQ_HANDLED;
@@ -3934,10 +3954,13 @@ static int find_PCI_BAR_index(struct pci_dev *pdev, unsigned long pci_bar_addr)
 static void __devinit hpsa_interrupt_mode(struct ctlr_info *h)
 {
 #ifdef CONFIG_PCI_MSI
-	int err;
-	struct msix_entry hpsa_msix_entries[4] = { {0, 0}, {0, 1},
-	{0, 2}, {0, 3}
-	};
+	int err, i;
+	struct msix_entry hpsa_msix_entries[MAX_REPLY_QUEUES];
+
+	for (i = 0; i < MAX_REPLY_QUEUES; i++) {
+		hpsa_msix_entries[i].vector = 0;
+		hpsa_msix_entries[i].entry = i;
+	}
 
 	/* Some boards advertise MSI but don't really support it */
 	if ((h->board_id == 0x40700E11) || (h->board_id == 0x40800E11) ||
@@ -3945,12 +3968,11 @@ static void __devinit hpsa_interrupt_mode(struct ctlr_info *h)
 		goto default_int_mode;
 	if (pci_find_capability(h->pdev, PCI_CAP_ID_MSIX)) {
 		dev_info(&h->pdev->dev, "MSIX\n");
-		err = pci_enable_msix(h->pdev, hpsa_msix_entries, 4);
+		err = pci_enable_msix(h->pdev, hpsa_msix_entries,
+						MAX_REPLY_QUEUES);
 		if (!err) {
-			h->intr[0] = hpsa_msix_entries[0].vector;
-			h->intr[1] = hpsa_msix_entries[1].vector;
-			h->intr[2] = hpsa_msix_entries[2].vector;
-			h->intr[3] = hpsa_msix_entries[3].vector;
+			for (i = 0; i < MAX_REPLY_QUEUES; i++)
+				h->intr[i] = hpsa_msix_entries[i].vector;
 			h->msix_vector = 1;
 			return;
 		}
@@ -4370,14 +4392,33 @@ static int hpsa_request_irq(struct ctlr_info *h,
 	irqreturn_t (*msixhandler)(int, void *),
 	irqreturn_t (*intxhandler)(int, void *))
 {
-	int rc;
+	int rc, i;
 
-	if (h->msix_vector || h->msi_vector)
-		rc = request_irq(h->intr[h->intr_mode], msixhandler,
-				0, h->devname, h);
-	else
-		rc = request_irq(h->intr[h->intr_mode], intxhandler,
-				IRQF_SHARED, h->devname, h);
+	/*
+	 * initialize h->q[x] = x so that interrupt handlers know which
+	 * queue to process.
+	 */
+	for (i = 0; i < MAX_REPLY_QUEUES; i++)
+		h->q[i] = (u8) i;
+
+	if (h->intr_mode == PERF_MODE_INT && h->msix_vector) {
+		/* If performant mode and MSI-X, use multiple reply queues */
+		for (i = 0; i < MAX_REPLY_QUEUES; i++)
+			rc = request_irq(h->intr[i], msixhandler,
+					0, h->devname,
+					&h->q[i]);
+	} else {
+		/* Use single reply pool */
+		if (h->msix_vector || h->msi_vector) {
+			rc = request_irq(h->intr[h->intr_mode],
+				msixhandler, 0, h->devname,
+				&h->q[h->intr_mode]);
+		} else {
+			rc = request_irq(h->intr[h->intr_mode],
+				intxhandler, IRQF_SHARED, h->devname,
+				&h->q[h->intr_mode]);
+		}
+	}
 	if (rc) {
 		dev_err(&h->pdev->dev, "unable to get irq %d for %s\n",
 		       h->intr[h->intr_mode], h->devname);
@@ -4410,9 +4451,24 @@ static int __devinit hpsa_kdump_soft_reset(struct ctlr_info *h)
 	return 0;
 }
 
+static void free_irqs(struct ctlr_info *h)
+{
+	int i;
+
+	if (!h->msix_vector || h->intr_mode != PERF_MODE_INT) {
+		/* Single reply queue, only one irq to free */
+		i = h->intr_mode;
+		free_irq(h->intr[i], &h->q[i]);
+		return;
+	}
+
+	for (i = 0; i < MAX_REPLY_QUEUES; i++)
+		free_irq(h->intr[i], &h->q[i]);
+}
+
 static void hpsa_undo_allocations_after_kdump_soft_reset(struct ctlr_info *h)
 {
-	free_irq(h->intr[h->intr_mode], h);
+	free_irqs(h);
 #ifdef CONFIG_PCI_MSI
 	if (h->msix_vector)
 		pci_disable_msix(h->pdev);
@@ -4680,7 +4736,7 @@ reinit_after_soft_reset:
 		spin_lock_irqsave(&h->lock, flags);
 		h->access.set_intr_mask(h, HPSA_INTR_OFF);
 		spin_unlock_irqrestore(&h->lock, flags);
-		free_irq(h->intr[h->intr_mode], h);
+		free_irqs(h);
 		rc = hpsa_request_irq(h, hpsa_msix_discard_completions,
 					hpsa_intx_discard_completions);
 		if (rc) {
@@ -4730,7 +4786,7 @@ reinit_after_soft_reset:
 clean4:
 	hpsa_free_sg_chain_blocks(h);
 	hpsa_free_cmd_pool(h);
-	free_irq(h->intr[h->intr_mode], h);
+	free_irqs(h);
 clean2:
 clean1:
 	kfree(h);
@@ -4773,7 +4829,7 @@ static void hpsa_shutdown(struct pci_dev *pdev)
 	 */
 	hpsa_flush_cache(h);
 	h->access.set_intr_mask(h, HPSA_INTR_OFF);
-	free_irq(h->intr[h->intr_mode], h);
+	free_irqs(h);
 #ifdef CONFIG_PCI_MSI
 	if (h->msix_vector)
 		pci_disable_msix(h->pdev);
@@ -4913,11 +4969,8 @@ static __devinit void hpsa_enter_performant_mode(struct ctlr_info *h,
 	 * 10 = 6 s/g entry or 24k
 	 */
 
-	h->reply_pool_wraparound = 1; /* spec: init to 1 */
-
 	/* Controller spec: zero out this buffer. */
 	memset(h->reply_pool, 0, h->reply_pool_size);
-	h->reply_pool_head = h->reply_pool;
 
 	bft[7] = SG_ENTRIES_IN_CMD + 4;
 	calc_bucket_map(bft, ARRAY_SIZE(bft),
@@ -4927,12 +4980,19 @@ static __devinit void hpsa_enter_performant_mode(struct ctlr_info *h,
 
 	/* size of controller ring buffer */
 	writel(h->max_commands, &h->transtable->RepQSize);
-	writel(1, &h->transtable->RepQCount);
+	writel(h->nreply_queues, &h->transtable->RepQCount);
 	writel(0, &h->transtable->RepQCtrAddrLow32);
 	writel(0, &h->transtable->RepQCtrAddrHigh32);
-	writel(h->reply_pool_dhandle, &h->transtable->RepQAddr0Low32);
-	writel(0, &h->transtable->RepQAddr0High32);
-	writel(CFGTBL_Trans_Performant | use_short_tags,
+
+	for (i = 0; i < h->nreply_queues; i++) {
+		writel(0, &h->transtable->RepQAddr[i].upper);
+		writel(h->reply_pool_dhandle +
+			(h->max_commands * sizeof(u64) * i),
+			&h->transtable->RepQAddr[i].lower);
+	}
+
+	writel(CFGTBL_Trans_Performant | use_short_tags |
+		CFGTBL_Trans_enable_directed_msix,
 		&(h->cfgtable->HostWrite.TransportRequest));
 	writel(CFGTBL_ChangeReq, h->vaddr + SA5_DOORBELL);
 	hpsa_wait_for_mode_change_ack(h);
@@ -4950,6 +5010,7 @@ static __devinit void hpsa_enter_performant_mode(struct ctlr_info *h,
 static __devinit void hpsa_put_ctlr_into_performant_mode(struct ctlr_info *h)
 {
 	u32 trans_support;
+	int i;
 
 	if (hpsa_simple_mode)
 		return;
@@ -4958,12 +5019,20 @@ static __devinit void hpsa_put_ctlr_into_performant_mode(struct ctlr_info *h)
 	if (!(trans_support & PERFORMANT_MODE))
 		return;
 
+	h->nreply_queues = h->msix_vector ? MAX_REPLY_QUEUES : 1;
 	hpsa_get_max_perf_mode_cmds(h);
 	/* Performant mode ring buffer and supporting data structures */
-	h->reply_pool_size = h->max_commands * sizeof(u64);
+	h->reply_pool_size = h->max_commands * sizeof(u64) * h->nreply_queues;
 	h->reply_pool = pci_alloc_consistent(h->pdev, h->reply_pool_size,
 				&(h->reply_pool_dhandle));
 
+	for (i = 0; i < h->nreply_queues; i++) {
+		h->reply_queue[i].head = &h->reply_pool[h->max_commands * i];
+		h->reply_queue[i].size = h->max_commands;
+		h->reply_queue[i].wraparound = 1;  /* spec: init to 1 */
+		h->reply_queue[i].current_entry = 0;
+	}
+
 	/* Need a block fetch table for performant mode */
 	h->blockFetchTable = kmalloc(((SG_ENTRIES_IN_CMD + 1) *
 				sizeof(u32)), GFP_KERNEL);
diff --git a/drivers/scsi/hpsa.h b/drivers/scsi/hpsa.h
index d8aa95c..486a7c0 100644
--- a/drivers/scsi/hpsa.h
+++ b/drivers/scsi/hpsa.h
@@ -34,7 +34,7 @@ struct access_method {
 	void (*set_intr_mask)(struct ctlr_info *h, unsigned long val);
 	unsigned long (*fifo_full)(struct ctlr_info *h);
 	bool (*intr_pending)(struct ctlr_info *h);
-	unsigned long (*command_completed)(struct ctlr_info *h);
+	unsigned long (*command_completed)(struct ctlr_info *h, u8 q);
 };
 
 struct hpsa_scsi_dev_t {
@@ -48,6 +48,13 @@ struct hpsa_scsi_dev_t {
 	unsigned char raid_level;	/* from inquiry page 0xC1 */
 };
 
+struct reply_pool {
+	u64 *head;
+	size_t size;
+	u8 wraparound;
+	u32 current_entry;
+};
+
 struct ctlr_info {
 	int	ctlr;
 	char	devname[8];
@@ -68,7 +75,7 @@ struct ctlr_info {
 #	define DOORBELL_INT	1
 #	define SIMPLE_MODE_INT	2
 #	define MEMQ_MODE_INT	3
-	unsigned int intr[4];
+	unsigned int intr[MAX_REPLY_QUEUES];
 	unsigned int msix_vector;
 	unsigned int msi_vector;
 	int intr_mode; /* either PERF_MODE_INT or SIMPLE_MODE_INT */
@@ -111,13 +118,13 @@ struct ctlr_info {
 	unsigned long transMethod;
 
 	/*
-	 * Performant mode completion buffer
+	 * Performant mode completion buffers
 	 */
 	u64 *reply_pool;
-	dma_addr_t reply_pool_dhandle;
-	u64 *reply_pool_head;
 	size_t reply_pool_size;
-	unsigned char reply_pool_wraparound;
+	struct reply_pool reply_queue[MAX_REPLY_QUEUES];
+	u8 nreply_queues;
+	dma_addr_t reply_pool_dhandle;
 	u32 *blockFetchTable;
 	unsigned char *hba_inquiry_data;
 	u64 last_intr_timestamp;
@@ -125,6 +132,8 @@ struct ctlr_info {
 	u64 last_heartbeat_timestamp;
 	u32 lockup_detected;
 	struct list_head lockup_list;
+	/* Address of h->q[x] is passed to intr handler to know which queue */
+	u8 q[MAX_REPLY_QUEUES];
 	u32 TMFSupportFlags; /* cache what task mgmt funcs are supported. */
 #define HPSATMF_BITS_SUPPORTED  (1 << 0)
 #define HPSATMF_PHYS_LUN_RESET  (1 << 1)
@@ -275,8 +284,9 @@ static void SA5_performant_intr_mask(struct ctlr_info *h, unsigned long val)
 	}
 }
 
-static unsigned long SA5_performant_completed(struct ctlr_info *h)
+static unsigned long SA5_performant_completed(struct ctlr_info *h, u8 q)
 {
+	struct reply_pool *rq = &h->reply_queue[q];
 	unsigned long register_value = FIFO_EMPTY;
 
 	/* msi auto clears the interrupt pending bit. */
@@ -292,19 +302,18 @@ static unsigned long SA5_performant_completed(struct ctlr_info *h)
 		register_value = readl(h->vaddr + SA5_OUTDB_STATUS);
 	}
 
-	if ((*(h->reply_pool_head) & 1) == (h->reply_pool_wraparound)) {
-		register_value = *(h->reply_pool_head);
-		(h->reply_pool_head)++;
+	if ((rq->head[rq->current_entry] & 1) == rq->wraparound) {
+		register_value = rq->head[rq->current_entry];
+		rq->current_entry++;
 		h->commands_outstanding--;
 	} else {
 		register_value = FIFO_EMPTY;
 	}
 	/* Check for wraparound */
-	if (h->reply_pool_head == (h->reply_pool + h->max_commands)) {
-		h->reply_pool_head = h->reply_pool;
-		h->reply_pool_wraparound ^= 1;
+	if (rq->current_entry == h->max_commands) {
+		rq->current_entry = 0;
+		rq->wraparound ^= 1;
 	}
-
 	return register_value;
 }
 
@@ -324,7 +333,8 @@ static unsigned long SA5_fifo_full(struct ctlr_info *h)
  *   returns value read from hardware.
  *     returns FIFO_EMPTY if there is nothing to read
  */
-static unsigned long SA5_completed(struct ctlr_info *h)
+static unsigned long SA5_completed(struct ctlr_info *h,
+	__attribute__((unused)) u8 q)
 {
 	unsigned long register_value
 		= readl(h->vaddr + SA5_REPLY_PORT_OFFSET);
diff --git a/drivers/scsi/hpsa_cmd.h b/drivers/scsi/hpsa_cmd.h
index 14b56c9..43f1631 100644
--- a/drivers/scsi/hpsa_cmd.h
+++ b/drivers/scsi/hpsa_cmd.h
@@ -129,6 +129,7 @@
 #define CFGTBL_Trans_Simple     0x00000002l
 #define CFGTBL_Trans_Performant 0x00000004l
 #define CFGTBL_Trans_use_short_tags 0x20000000l
+#define CFGTBL_Trans_enable_directed_msix (1 << 30)
 
 #define CFGTBL_BusType_Ultra2   0x00000001l
 #define CFGTBL_BusType_Ultra3   0x00000002l
@@ -380,8 +381,8 @@ struct TransTable_struct {
 	u32            RepQCount;
 	u32            RepQCtrAddrLow32;
 	u32            RepQCtrAddrHigh32;
-	u32            RepQAddr0Low32;
-	u32            RepQAddr0High32;
+#define MAX_REPLY_QUEUES 8
+	struct vals32  RepQAddr[MAX_REPLY_QUEUES];
 };
 
 struct hpsa_pci_info {

[PATCH 12/17] hpsa: refine interrupt handler locking for greater concurrency

[Stephen M. Cameron]

From: Matt Gates <matthew.gates@hp.com>

Use spinlocks with finer granularity in the submission and
completion paths to allow concurrent execution for multiple
reply queues.  In particular, do not hold a spin lock while
submitting a request to the device, nor during most of the
interrupt handler.

Signed-off-by: Matt Gates <matthew.gates@hp.com>
Signed-off-by: Stephen M. Cameron <scameron@beardog.cce.hp.com>
---
 drivers/scsi/hpsa.c |   61 +++++++++++++++++++++++++++++++++------------------
 drivers/scsi/hpsa.h |   13 +++++++----
 2 files changed, 48 insertions(+), 26 deletions(-)

diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
index 29bbf35..1ec912b 100644
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -532,6 +532,7 @@ static inline u32 next_command(struct ctlr_info *h, u8 q)
 {
 	u32 a;
 	struct reply_pool *rq = &h->reply_queue[q];
+	unsigned long flags;
 
 	if (unlikely(!(h->transMethod & CFGTBL_Trans_Performant)))
 		return h->access.command_completed(h, q);
@@ -539,7 +540,9 @@ static inline u32 next_command(struct ctlr_info *h, u8 q)
 	if ((rq->head[rq->current_entry] & 1) == rq->wraparound) {
 		a = rq->head[rq->current_entry];
 		rq->current_entry++;
+		spin_lock_irqsave(&h->lock, flags);
 		h->commands_outstanding--;
+		spin_unlock_irqrestore(&h->lock, flags);
 	} else {
 		a = FIFO_EMPTY;
 	}
@@ -574,8 +577,8 @@ static void enqueue_cmd_and_start_io(struct ctlr_info *h,
 	spin_lock_irqsave(&h->lock, flags);
 	addQ(&h->reqQ, c);
 	h->Qdepth++;
-	start_io(h);
 	spin_unlock_irqrestore(&h->lock, flags);
+	start_io(h);
 }
 
 static inline void removeQ(struct CommandList *c)
@@ -2083,9 +2086,8 @@ static int hpsa_scsi_queue_command_lck(struct scsi_cmnd *cmd,
 		done(cmd);
 		return 0;
 	}
-	/* Need a lock as this is being allocated from the pool */
-	c = cmd_alloc(h);
 	spin_unlock_irqrestore(&h->lock, flags);
+	c = cmd_alloc(h);
 	if (c == NULL) {			/* trouble... */
 		dev_err(&h->pdev->dev, "cmd_alloc returned NULL!\n");
 		return SCSI_MLQUEUE_HOST_BUSY;
@@ -2619,14 +2621,21 @@ static struct CommandList *cmd_alloc(struct ctlr_info *h)
 	int i;
 	union u64bit temp64;
 	dma_addr_t cmd_dma_handle, err_dma_handle;
+	unsigned long flags;
 
+	spin_lock_irqsave(&h->lock, flags);
 	do {
 		i = find_first_zero_bit(h->cmd_pool_bits, h->nr_cmds);
-		if (i == h->nr_cmds)
+		if (i == h->nr_cmds) {
+			spin_unlock_irqrestore(&h->lock, flags);
 			return NULL;
+		}
 	} while (test_and_set_bit
 		 (i & (BITS_PER_LONG - 1),
 		  h->cmd_pool_bits + (i / BITS_PER_LONG)) != 0);
+	h->nr_allocs++;
+	spin_unlock_irqrestore(&h->lock, flags);
+
 	c = h->cmd_pool + i;
 	memset(c, 0, sizeof(*c));
 	cmd_dma_handle = h->cmd_pool_dhandle
@@ -2635,7 +2644,6 @@ static struct CommandList *cmd_alloc(struct ctlr_info *h)
 	memset(c->err_info, 0, sizeof(*c->err_info));
 	err_dma_handle = h->errinfo_pool_dhandle
 	    + i * sizeof(*c->err_info);
-	h->nr_allocs++;
 
 	c->cmdindex = i;
 
@@ -2691,11 +2699,14 @@ static struct CommandList *cmd_special_alloc(struct ctlr_info *h)
 static void cmd_free(struct ctlr_info *h, struct CommandList *c)
 {
 	int i;
+	unsigned long flags;
 
 	i = c - h->cmd_pool;
+	spin_lock_irqsave(&h->lock, flags);
 	clear_bit(i & (BITS_PER_LONG - 1),
 		  h->cmd_pool_bits + (i / BITS_PER_LONG));
 	h->nr_frees++;
+	spin_unlock_irqrestore(&h->lock, flags);
 }
 
 static void cmd_special_free(struct ctlr_info *h, struct CommandList *c)
@@ -3299,7 +3310,9 @@ static void __iomem *remap_pci_mem(ulong base, ulong size)
 static void start_io(struct ctlr_info *h)
 {
 	struct CommandList *c;
+	unsigned long flags;
 
+	spin_lock_irqsave(&h->lock, flags);
 	while (!list_empty(&h->reqQ)) {
 		c = list_entry(h->reqQ.next, struct CommandList, list);
 		/* can't do anything if fifo is full */
@@ -3312,12 +3325,23 @@ static void start_io(struct ctlr_info *h)
 		removeQ(c);
 		h->Qdepth--;
 
-		/* Tell the controller execute command */
-		h->access.submit_command(h, c);
-
 		/* Put job onto the completed Q */
 		addQ(&h->cmpQ, c);
+
+		/* Must increment commands_outstanding before unlocking
+		 * and submitting to avoid race checking for fifo full
+		 * condition.
+		 */
+		h->commands_outstanding++;
+		if (h->commands_outstanding > h->max_outstanding)
+			h->max_outstanding = h->commands_outstanding;
+
+		/* Tell the controller execute command */
+		spin_unlock_irqrestore(&h->lock, flags);
+		h->access.submit_command(h, c);
+		spin_lock_irqsave(&h->lock, flags);
 	}
+	spin_unlock_irqrestore(&h->lock, flags);
 }
 
 static inline unsigned long get_next_completion(struct ctlr_info *h, u8 q)
@@ -3348,7 +3372,11 @@ static inline int bad_tag(struct ctlr_info *h, u32 tag_index,
 
 static inline void finish_cmd(struct CommandList *c)
 {
+	unsigned long flags;
+
+	spin_lock_irqsave(&c->h->lock, flags);
 	removeQ(c);
+	spin_unlock_irqrestore(&c->h->lock, flags);
 	if (likely(c->cmd_type == CMD_SCSI))
 		complete_scsi_command(c);
 	else if (c->cmd_type == CMD_IOCTL_PEND)
@@ -3395,14 +3423,18 @@ static inline void process_nonindexed_cmd(struct ctlr_info *h,
 {
 	u32 tag;
 	struct CommandList *c = NULL;
+	unsigned long flags;
 
 	tag = hpsa_tag_discard_error_bits(h, raw_tag);
+	spin_lock_irqsave(&h->lock, flags);
 	list_for_each_entry(c, &h->cmpQ, list) {
 		if ((c->busaddr & 0xFFFFFFE0) == (tag & 0xFFFFFFE0)) {
+			spin_unlock_irqrestore(&h->lock, flags);
 			finish_cmd(c);
 			return;
 		}
 	}
+	spin_unlock_irqrestore(&h->lock, flags);
 	bad_tag(h, h->nr_cmds + 1, raw_tag);
 }
 
@@ -3439,7 +3471,6 @@ static irqreturn_t hpsa_intx_discard_completions(int irq, void *queue)
 {
 	struct ctlr_info *h = queue_to_hba(queue);
 	u8 q = *(u8 *) queue;
-	unsigned long flags;
 	u32 raw_tag;
 
 	if (ignore_bogus_interrupt(h))
@@ -3447,47 +3478,39 @@ static irqreturn_t hpsa_intx_discard_completions(int irq, void *queue)
 
 	if (interrupt_not_for_us(h))
 		return IRQ_NONE;
-	spin_lock_irqsave(&h->lock, flags);
 	h->last_intr_timestamp = get_jiffies_64();
 	while (interrupt_pending(h)) {
 		raw_tag = get_next_completion(h, q);
 		while (raw_tag != FIFO_EMPTY)
 			raw_tag = next_command(h, q);
 	}
-	spin_unlock_irqrestore(&h->lock, flags);
 	return IRQ_HANDLED;
 }
 
 static irqreturn_t hpsa_msix_discard_completions(int irq, void *queue)
 {
 	struct ctlr_info *h = queue_to_hba(queue);
-	unsigned long flags;
 	u32 raw_tag;
 	u8 q = *(u8 *) queue;
 
 	if (ignore_bogus_interrupt(h))
 		return IRQ_NONE;
 
-	spin_lock_irqsave(&h->lock, flags);
-
 	h->last_intr_timestamp = get_jiffies_64();
 	raw_tag = get_next_completion(h, q);
 	while (raw_tag != FIFO_EMPTY)
 		raw_tag = next_command(h, q);
-	spin_unlock_irqrestore(&h->lock, flags);
 	return IRQ_HANDLED;
 }
 
 static irqreturn_t do_hpsa_intr_intx(int irq, void *queue)
 {
 	struct ctlr_info *h = queue_to_hba((u8 *) queue);
-	unsigned long flags;
 	u32 raw_tag;
 	u8 q = *(u8 *) queue;
 
 	if (interrupt_not_for_us(h))
 		return IRQ_NONE;
-	spin_lock_irqsave(&h->lock, flags);
 	h->last_intr_timestamp = get_jiffies_64();
 	while (interrupt_pending(h)) {
 		raw_tag = get_next_completion(h, q);
@@ -3499,18 +3522,15 @@ static irqreturn_t do_hpsa_intr_intx(int irq, void *queue)
 			raw_tag = next_command(h, q);
 		}
 	}
-	spin_unlock_irqrestore(&h->lock, flags);
 	return IRQ_HANDLED;
 }
 
 static irqreturn_t do_hpsa_intr_msi(int irq, void *queue)
 {
 	struct ctlr_info *h = queue_to_hba(queue);
-	unsigned long flags;
 	u32 raw_tag;
 	u8 q = *(u8 *) queue;
 
-	spin_lock_irqsave(&h->lock, flags);
 	h->last_intr_timestamp = get_jiffies_64();
 	raw_tag = get_next_completion(h, q);
 	while (raw_tag != FIFO_EMPTY) {
@@ -3520,7 +3540,6 @@ static irqreturn_t do_hpsa_intr_msi(int irq, void *queue)
 			process_nonindexed_cmd(h, raw_tag);
 		raw_tag = next_command(h, q);
 	}
-	spin_unlock_irqrestore(&h->lock, flags);
 	return IRQ_HANDLED;
 }
 
diff --git a/drivers/scsi/hpsa.h b/drivers/scsi/hpsa.h
index 486a7c0..79c36aa 100644
--- a/drivers/scsi/hpsa.h
+++ b/drivers/scsi/hpsa.h
@@ -246,9 +246,6 @@ static void SA5_submit_command(struct ctlr_info *h,
 		c->Header.Tag.lower);
 	writel(c->busaddr, h->vaddr + SA5_REQUEST_PORT_OFFSET);
 	(void) readl(h->vaddr + SA5_SCRATCHPAD_OFFSET);
-	h->commands_outstanding++;
-	if (h->commands_outstanding > h->max_outstanding)
-		h->max_outstanding = h->commands_outstanding;
 }
 
 /*
@@ -287,7 +284,7 @@ static void SA5_performant_intr_mask(struct ctlr_info *h, unsigned long val)
 static unsigned long SA5_performant_completed(struct ctlr_info *h, u8 q)
 {
 	struct reply_pool *rq = &h->reply_queue[q];
-	unsigned long register_value = FIFO_EMPTY;
+	unsigned long flags, register_value = FIFO_EMPTY;
 
 	/* msi auto clears the interrupt pending bit. */
 	if (!(h->msi_vector || h->msix_vector)) {
@@ -305,7 +302,9 @@ static unsigned long SA5_performant_completed(struct ctlr_info *h, u8 q)
 	if ((rq->head[rq->current_entry] & 1) == rq->wraparound) {
 		register_value = rq->head[rq->current_entry];
 		rq->current_entry++;
+		spin_lock_irqsave(&h->lock, flags);
 		h->commands_outstanding--;
+		spin_unlock_irqrestore(&h->lock, flags);
 	} else {
 		register_value = FIFO_EMPTY;
 	}
@@ -338,9 +337,13 @@ static unsigned long SA5_completed(struct ctlr_info *h,
 {
 	unsigned long register_value
 		= readl(h->vaddr + SA5_REPLY_PORT_OFFSET);
+	unsigned long flags;
 
-	if (register_value != FIFO_EMPTY)
+	if (register_value != FIFO_EMPTY) {
+		spin_lock_irqsave(&h->lock, flags);
 		h->commands_outstanding--;
+		spin_unlock_irqrestore(&h->lock, flags);
+	}
 
 #ifdef HPSA_DEBUG
 	if (register_value != FIFO_EMPTY)

[PATCH 13/17] hpsa: factor out hpsa_free_irqs_and_disable_msix

[Stephen M. Cameron]

From: Stephen M. Cameron <scameron@beardog.cce.hp.com>

Signed-off-by: Stephen M. Cameron <scameron@beardog.cce.hp.com>
---
 drivers/scsi/hpsa.c |   30 ++++++++++++++++--------------
 1 files changed, 16 insertions(+), 14 deletions(-)

diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
index 1ec912b..2e01583 100644
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -4485,15 +4485,23 @@ static void free_irqs(struct ctlr_info *h)
 		free_irq(h->intr[i], &h->q[i]);
 }
 
-static void hpsa_undo_allocations_after_kdump_soft_reset(struct ctlr_info *h)
+static void hpsa_free_irqs_and_disable_msix(struct ctlr_info *h)
 {
 	free_irqs(h);
-#ifdef CONFIG_PCI_MSI
-	if (h->msix_vector)
-		pci_disable_msix(h->pdev);
-	else if (h->msi_vector)
-		pci_disable_msi(h->pdev);
-#endif /* CONFIG_PCI_MSI */
+	if (!IS_ENABLED(PCI_MSI))
+		return;
+	if (h->msix_vector) {
+		if (h->pdev->msix_enabled)
+			pci_disable_msix(h->pdev);
+	} else if (h->msi_vector) {
+		if (h->pdev->msi_enabled)
+			pci_disable_msi(h->pdev);
+	}
+}
+
+static void hpsa_undo_allocations_after_kdump_soft_reset(struct ctlr_info *h)
+{
+	hpsa_free_irqs_and_disable_msix(h);
 	hpsa_free_sg_chain_blocks(h);
 	hpsa_free_cmd_pool(h);
 	kfree(h->blockFetchTable);
@@ -4848,13 +4856,7 @@ static void hpsa_shutdown(struct pci_dev *pdev)
 	 */
 	hpsa_flush_cache(h);
 	h->access.set_intr_mask(h, HPSA_INTR_OFF);
-	free_irqs(h);
-#ifdef CONFIG_PCI_MSI
-	if (h->msix_vector)
-		pci_disable_msix(h->pdev);
-	else if (h->msi_vector)
-		pci_disable_msi(h->pdev);
-#endif				/* CONFIG_PCI_MSI */
+	hpsa_free_irqs_and_disable_msix(h);
 }
 
 static void __devexit hpsa_free_device_info(struct ctlr_info *h)

[PATCH 14/17] hpsa: use new IS_ENABLED macro

[Stephen M. Cameron]

From: Stephen M. Cameron <scameron@beardog.cce.hp.com>

Signed-off-by: Stephen M. Cameron <scameron@beardog.cce.hp.com>
---
 drivers/scsi/hpsa.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
index 2e01583..67a0ad0 100644
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -139,7 +139,7 @@ static irqreturn_t do_hpsa_intr_msi(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);
 
-#ifdef CONFIG_COMPAT
+#if IS_ENABLED(COMPAT)
 static int hpsa_compat_ioctl(struct scsi_device *dev, int cmd, void *arg);
 #endif
 
@@ -513,7 +513,7 @@ static struct scsi_host_template hpsa_driver_template = {
 	.ioctl			= hpsa_ioctl,
 	.slave_alloc		= hpsa_slave_alloc,
 	.slave_destroy		= hpsa_slave_destroy,
-#ifdef CONFIG_COMPAT
+#if IS_ENABLED(COMPAT)
 	.compat_ioctl		= hpsa_compat_ioctl,
 #endif
 	.sdev_attrs = hpsa_sdev_attrs,
@@ -2721,7 +2721,7 @@ static void cmd_special_free(struct ctlr_info *h, struct CommandList *c)
 			    c, (dma_addr_t) (c->busaddr & DIRECT_LOOKUP_MASK));
 }
 
-#ifdef CONFIG_COMPAT
+#if IS_ENABLED(COMPAT)
 
 static int hpsa_ioctl32_passthru(struct scsi_device *dev, int cmd, void *arg)
 {
@@ -3972,7 +3972,7 @@ static int find_PCI_BAR_index(struct pci_dev *pdev, unsigned long pci_bar_addr)
 
 static void __devinit hpsa_interrupt_mode(struct ctlr_info *h)
 {
-#ifdef CONFIG_PCI_MSI
+#if IS_ENABLED(PCI_MSI)
 	int err, i;
 	struct msix_entry hpsa_msix_entries[MAX_REPLY_QUEUES];
 
@@ -4013,7 +4013,7 @@ static void __devinit hpsa_interrupt_mode(struct ctlr_info *h)
 			dev_warn(&h->pdev->dev, "MSI init failed\n");
 	}
 default_int_mode:
-#endif				/* CONFIG_PCI_MSI */
+#endif /* PCI_MSI enabled */
 	/* if we get here we're going to use the default interrupt mode */
 	h->intr[h->intr_mode] = h->pdev->irq;
 }
@@ -4187,7 +4187,7 @@ static inline bool hpsa_CISS_signature_present(struct ctlr_info *h)
 /* Need to enable prefetch in the SCSI core for 6400 in x86 */
 static inline void hpsa_enable_scsi_prefetch(struct ctlr_info *h)
 {
-#ifdef CONFIG_X86
+#if IS_ENABLED(X86)
 	u32 prefetch;
 
 	prefetch = readl(&(h->cfgtable->SCSI_Prefetch));

[PATCH 15/17] hpsa: add new RAID level "1(ADM)"

[Stephen M. Cameron]

From: Mike Miller <mikem@beardog.cce.hp.com>

Signed-off-by: Mike Miller <mikem@beardog.cce.hp.com>
Signed-off-by: Stephen M. Cameron <scameron@beardog.cce.hp.com>
---
 drivers/scsi/hpsa.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
index 67a0ad0..8213875 100644
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -379,7 +379,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"
+	"1(ADM)", "UNKNOWN"
 };
 #define RAID_UNKNOWN (ARRAY_SIZE(raid_label) - 1)
 

[PATCH 16/17] hpsa: removed unused member maxQsinceinit

[Stephen M. Cameron]

From: Stephen M. Cameron <scameron@beardog.cce.hp.com>

Signed-off-by: Stephen M. Cameron <scameron@beardog.cce.hp.com>
---
 drivers/scsi/hpsa.h |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/drivers/scsi/hpsa.h b/drivers/scsi/hpsa.h
index 79c36aa..fb51ef7 100644
--- a/drivers/scsi/hpsa.h
+++ b/drivers/scsi/hpsa.h
@@ -85,7 +85,6 @@ struct ctlr_info {
 	struct list_head reqQ;
 	struct list_head cmpQ;
 	unsigned int Qdepth;
-	unsigned int maxQsinceinit;
 	unsigned int maxSG;
 	spinlock_t lock;
 	int maxsgentries;

[PATCH 17/17] hpsa: dial down lockup detection during firmware flash

[Stephen M. Cameron]

From: Stephen M. Cameron <scameron@beardog.cce.hp.com>

Dial back the aggressiveness of the controller lockup detection thread.
Currently it will declare the controller to be locked up if it goes
for 10 seconds with no interrupts and no change in the heartbeat
register.  Dial back this to 30 seconds with no heartbeat change, and
also snoop the ioctl path and if a firmware flash command is detected,
dial it back further to 4 minutes until the firmware flash command
completes.  The reason for this is that during the firmware flash
operation, the controller apparently doesn't update the heartbeat
register as frequently as it is supposed to, and we can get a false
positive.

Signed-off-by: Stephen M. Cameron <scameron@beardog.cce.hp.com>
---
 drivers/scsi/hpsa.c     |   39 ++++++++++++++++++++++++++++++++++-----
 drivers/scsi/hpsa.h     |    2 ++
 drivers/scsi/hpsa_cmd.h |    1 +
 3 files changed, 37 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
index 8213875..6245c52 100644
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -568,12 +568,42 @@ static void set_performant_mode(struct ctlr_info *h, struct CommandList *c)
 	}
 }
 
+static int is_firmware_flash_cmd(u8 *cdb)
+{
+	return cdb[0] == BMIC_WRITE && cdb[6] == BMIC_FLASH_FIRMWARE;
+}
+
+/*
+ * During firmware flash, the heartbeat register may not update as frequently
+ * as it should.  So we dial down lockup detection during firmware flash. and
+ * dial it back up when firmware flash completes.
+ */
+#define HEARTBEAT_SAMPLE_INTERVAL_DURING_FLASH (240 * HZ)
+#define HEARTBEAT_SAMPLE_INTERVAL (30 * HZ)
+static void dial_down_lockup_detection_during_fw_flash(struct ctlr_info *h,
+		struct CommandList *c)
+{
+	if (!is_firmware_flash_cmd(c->Request.CDB))
+		return;
+	atomic_inc(&h->firmware_flash_in_progress);
+	h->heartbeat_sample_interval = HEARTBEAT_SAMPLE_INTERVAL_DURING_FLASH;
+}
+
+static void dial_up_lockup_detection_on_fw_flash_complete(struct ctlr_info *h,
+		struct CommandList *c)
+{
+	if (is_firmware_flash_cmd(c->Request.CDB) &&
+		atomic_dec_and_test(&h->firmware_flash_in_progress))
+		h->heartbeat_sample_interval = HEARTBEAT_SAMPLE_INTERVAL;
+}
+
 static void enqueue_cmd_and_start_io(struct ctlr_info *h,
 	struct CommandList *c)
 {
 	unsigned long flags;
 
 	set_performant_mode(h, c);
+	dial_down_lockup_detection_during_fw_flash(h, c);
 	spin_lock_irqsave(&h->lock, flags);
 	addQ(&h->reqQ, c);
 	h->Qdepth++;
@@ -3377,6 +3407,7 @@ static inline void finish_cmd(struct CommandList *c)
 	spin_lock_irqsave(&c->h->lock, flags);
 	removeQ(c);
 	spin_unlock_irqrestore(&c->h->lock, flags);
+	dial_up_lockup_detection_on_fw_flash_complete(c->h, c);
 	if (likely(c->cmd_type == CMD_SCSI))
 		complete_scsi_command(c);
 	else if (c->cmd_type == CMD_IOCTL_PEND)
@@ -4560,9 +4591,6 @@ static void controller_lockup_detected(struct ctlr_info *h)
 	spin_unlock_irqrestore(&h->lock, flags);
 }
 
-#define HEARTBEAT_SAMPLE_INTERVAL (10 * HZ)
-#define HEARTBEAT_CHECK_MINIMUM_INTERVAL (HEARTBEAT_SAMPLE_INTERVAL / 2)
-
 static void detect_controller_lockup(struct ctlr_info *h)
 {
 	u64 now;
@@ -4573,7 +4601,7 @@ static void detect_controller_lockup(struct ctlr_info *h)
 	now = get_jiffies_64();
 	/* If we've received an interrupt recently, we're ok. */
 	if (time_after64(h->last_intr_timestamp +
-				(HEARTBEAT_CHECK_MINIMUM_INTERVAL), now))
+				(h->heartbeat_sample_interval), now))
 		return;
 
 	/*
@@ -4582,7 +4610,7 @@ static void detect_controller_lockup(struct ctlr_info *h)
 	 * otherwise don't care about signals in this thread.
 	 */
 	if (time_after64(h->last_heartbeat_timestamp +
-				(HEARTBEAT_CHECK_MINIMUM_INTERVAL), now))
+				(h->heartbeat_sample_interval), now))
 		return;
 
 	/* If heartbeat has not changed since we last looked, we're not ok. */
@@ -4624,6 +4652,7 @@ static void add_ctlr_to_lockup_detector_list(struct ctlr_info *h)
 {
 	unsigned long flags;
 
+	h->heartbeat_sample_interval = HEARTBEAT_SAMPLE_INTERVAL;
 	spin_lock_irqsave(&lockup_detector_lock, flags);
 	list_add_tail(&h->lockup_list, &hpsa_ctlr_list);
 	spin_unlock_irqrestore(&lockup_detector_lock, flags);
diff --git a/drivers/scsi/hpsa.h b/drivers/scsi/hpsa.h
index fb51ef7..9816479 100644
--- a/drivers/scsi/hpsa.h
+++ b/drivers/scsi/hpsa.h
@@ -129,6 +129,8 @@ struct ctlr_info {
 	u64 last_intr_timestamp;
 	u32 last_heartbeat;
 	u64 last_heartbeat_timestamp;
+	u32 heartbeat_sample_interval;
+	atomic_t firmware_flash_in_progress;
 	u32 lockup_detected;
 	struct list_head lockup_list;
 	/* Address of h->q[x] is passed to intr handler to know which queue */
diff --git a/drivers/scsi/hpsa_cmd.h b/drivers/scsi/hpsa_cmd.h
index 43f1631..a894f2e 100644
--- a/drivers/scsi/hpsa_cmd.h
+++ b/drivers/scsi/hpsa_cmd.h
@@ -186,6 +186,7 @@ struct SenseSubsystem_info {
 #define BMIC_WRITE 0x27
 #define BMIC_CACHE_FLUSH 0xc2
 #define HPSA_CACHE_FLUSH 0x01	/* C2 was already being used by HPSA */
+#define BMIC_FLASH_FIRMWARE 0xF7
 
 /* Command List Structure */
 union SCSI3Addr {

Re: [PATCH 03/17] hpsa: enable bus master bit after pci_enable_device

[Khalid Aziz]

On Fri, 2012-04-20 at 10:06 -0500, Stephen M. Cameron wrote:
> From: Stephen M. Cameron <scameron@beardog.cce.hp.com>
> 
> pci_disable_device() disables the bus master bit and pci_enable_device does
> not re-enable it.  It needs to be enabled.
> 
> Signed-off-by: Stephen M. Cameron <scameron@beardog.cce.hp.com>
> ---
>  drivers/scsi/hpsa.c |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
> index 5119ce6..363619d 100644
> --- a/drivers/scsi/hpsa.c
> +++ b/drivers/scsi/hpsa.c
> @@ -3917,6 +3917,7 @@ static int __devinit hpsa_enter_simple_mode(struct ctlr_info *h)
>  static int __devinit hpsa_pci_init(struct ctlr_info *h)
>  {
>  	int prod_index, err;
> +	u16 command_register;
>  
>  	prod_index = hpsa_lookup_board_id(h->pdev, &h->board_id);
>  	if (prod_index < 0)
> @@ -3933,6 +3934,11 @@ static int __devinit hpsa_pci_init(struct ctlr_info *h)
>  		return err;
>  	}
>  
> +	/* Enable bus mastering (pci_disable_device may disable this) */
> +	pci_read_config_word(h->pdev, PCI_COMMAND, &command_register);
> +	command_register |= PCI_COMMAND_MASTER;
> +	pci_write_config_word(h->pdev, PCI_COMMAND, command_register);
> +
>  	err = pci_request_regions(h->pdev, HPSA);
>  	if (err) {
>  		dev_err(&h->pdev->dev,

Wouldn't it be better to use pci_set_master(h->pdev) to enable bus
mastering?

-- 
====================================================================
Khalid Aziz                                         Unix Systems Lab
(970)898-9214                                        Hewlett-Packard
khalid.aziz@hp.com                                  Fort Collins, CO

Re: [PATCH 03/17] hpsa: enable bus master bit after pci_enable_device

[scameron]

On Fri, Apr 20, 2012 at 12:41:50PM -0600, Khalid Aziz wrote:
> On Fri, 2012-04-20 at 10:06 -0500, Stephen M. Cameron wrote:
> > From: Stephen M. Cameron <scameron@beardog.cce.hp.com>
> > 
> > pci_disable_device() disables the bus master bit and pci_enable_device does
> > not re-enable it.  It needs to be enabled.
> > 
> > Signed-off-by: Stephen M. Cameron <scameron@beardog.cce.hp.com>
> > ---
> >  drivers/scsi/hpsa.c |    6 ++++++
> >  1 files changed, 6 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
> > index 5119ce6..363619d 100644
> > --- a/drivers/scsi/hpsa.c
> > +++ b/drivers/scsi/hpsa.c
> > @@ -3917,6 +3917,7 @@ static int __devinit hpsa_enter_simple_mode(struct ctlr_info *h)
> >  static int __devinit hpsa_pci_init(struct ctlr_info *h)
> >  {
> >  	int prod_index, err;
> > +	u16 command_register;
> >  
> >  	prod_index = hpsa_lookup_board_id(h->pdev, &h->board_id);
> >  	if (prod_index < 0)
> > @@ -3933,6 +3934,11 @@ static int __devinit hpsa_pci_init(struct ctlr_info *h)
> >  		return err;
> >  	}
> >  
> > +	/* Enable bus mastering (pci_disable_device may disable this) */
> > +	pci_read_config_word(h->pdev, PCI_COMMAND, &command_register);
> > +	command_register |= PCI_COMMAND_MASTER;
> > +	pci_write_config_word(h->pdev, PCI_COMMAND, command_register);
> > +
> >  	err = pci_request_regions(h->pdev, HPSA);
> >  	if (err) {
> >  		dev_err(&h->pdev->dev,
> 
> Wouldn't it be better to use pci_set_master(h->pdev) to enable bus
> mastering?

Yes.  Thanks.

-- steve

RE: [PATCH 04/17] hpsa: suppress excessively chatty error messages

[Shergill, Gurinder]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2359 bytes --]

| -----Original Message-----
| From: linux-scsi-owner@vger.kernel.org [mailto:linux-scsi-
| owner@vger.kernel.org] On Behalf Of Stephen M. Cameron
| Sent: Friday, April 20, 2012 8:07 AM
| To: james.bottomley@hansenpartnership.com
| Cc: linux-scsi@vger.kernel.org; linux-kernel@vger.kernel.org; Gates, Matt;
| stephenmcameron@gmail.com; thenzl@redhat.com; akpm@linux-foundation.org;
| mikem@beardog.cce.hp.com
| Subject: [PATCH 04/17] hpsa: suppress excessively chatty error messages
| 
| From: Stephen M. Cameron <scameron@beardog.cce.hp.com>
| 
| Default behavior for any CHECK CONDITION excepting a few special cases is
| to print out certain parts of the sense buffer and the CDB.  Default
| behavior should be to print nothing and let the upper layers or
| applications decide what to do about these.  The same information is
| already available by setting the appropriate bits of the
| scsi_logging_level kernel parameter or via
| /proc/sys/dev/scsi/logging_level.

Makes sense.

| 
| Signed-off-by: Stephen M. Cameron <scameron@beardog.cce.hp.com>
| ---
|  drivers/scsi/hpsa.c |    2 +-
|  1 files changed, 1 insertions(+), 1 deletions(-)
| 
| diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c index
| 906672b..fcb9fb2 100644
| --- a/drivers/scsi/hpsa.c
| +++ b/drivers/scsi/hpsa.c
| @@ -1193,7 +1193,7 @@ static void complete_scsi_command(struct CommandList
| *cp)
|  				break;
|  			}
|  			/* Must be some other type of check condition */
| -			dev_warn(&h->pdev->dev, "cp %p has check condition: "
| +			dev_dbg(&h->pdev->dev, "cp %p has check condition: "
|  					"unknown type: "
|  					"Sense: 0x%x, ASC: 0x%x, ASCQ: 0x%x, "
|  					"Returning result: 0x%x, "
| 

Few lines further down in the same function, there is another dev_warn() for SCSI errors.

       /* Problem was not a check condition
         * Pass it up to the upper layers...
         */
        if (ei->ScsiStatus) {
            dev_warn(&h->pdev->dev, "cp %p has status 0x%x "
                "Sense: 0x%x, ASC: 0x%x, ASCQ: 0x%x, "


I am wondering whether it make sense to make the same change here as well as. This will suppress logging for other SCSI status, for example BUSY, TASK SET FULL, etc.

Sunny 

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

RE: [PATCH 04/17] hpsa: suppress excessively chatty error messages

[Shergill, Gurinder]

| -----Original Message-----
| From: linux-scsi-owner@vger.kernel.org [mailto:linux-scsi-
| owner@vger.kernel.org] On Behalf Of Stephen M. Cameron
| Sent: Friday, April 20, 2012 8:07 AM
| To: james.bottomley@hansenpartnership.com
| Cc: linux-scsi@vger.kernel.org; linux-kernel@vger.kernel.org; Gates, Matt;
| stephenmcameron@gmail.com; thenzl@redhat.com; akpm@linux-foundation.org;
| mikem@beardog.cce.hp.com
| Subject: [PATCH 04/17] hpsa: suppress excessively chatty error messages
| 
| From: Stephen M. Cameron <scameron@beardog.cce.hp.com>
| 
| Default behavior for any CHECK CONDITION excepting a few special cases is
| to print out certain parts of the sense buffer and the CDB.  Default
| behavior should be to print nothing and let the upper layers or
| applications decide what to do about these.  The same information is
| already available by setting the appropriate bits of the
| scsi_logging_level kernel parameter or via
| /proc/sys/dev/scsi/logging_level.

Makes sense.

| 
| Signed-off-by: Stephen M. Cameron <scameron@beardog.cce.hp.com>
| ---
|  drivers/scsi/hpsa.c |    2 +-
|  1 files changed, 1 insertions(+), 1 deletions(-)
| 
| diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c index
| 906672b..fcb9fb2 100644
| --- a/drivers/scsi/hpsa.c
| +++ b/drivers/scsi/hpsa.c
| @@ -1193,7 +1193,7 @@ static void complete_scsi_command(struct CommandList
| *cp)
|  				break;
|  			}
|  			/* Must be some other type of check condition */
| -			dev_warn(&h->pdev->dev, "cp %p has check condition: "
| +			dev_dbg(&h->pdev->dev, "cp %p has check condition: "
|  					"unknown type: "
|  					"Sense: 0x%x, ASC: 0x%x, ASCQ: 0x%x, "
|  					"Returning result: 0x%x, "
| 

Few lines further down in the same function, there is another dev_warn() for SCSI errors.

       /* Problem was not a check condition
         * Pass it up to the upper layers...
         */
        if (ei->ScsiStatus) {
            dev_warn(&h->pdev->dev, "cp %p has status 0x%x "
                "Sense: 0x%x, ASC: 0x%x, ASCQ: 0x%x, "


I am wondering whether it make sense to make the same change here as well as. This will suppress logging for other SCSI status, for example BUSY, TASK SET FULL, etc.

Sunny 

RE: [PATCH 06/17] hpsa: retry driver initiated commands on busy status

[Shergill, Gurinder]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2674 bytes --]


| -----Original Message-----
| From: linux-scsi-owner@vger.kernel.org [mailto:linux-scsi-
| owner@vger.kernel.org] On Behalf Of Stephen M. Cameron
| Sent: Friday, April 20, 2012 8:07 AM
| To: james.bottomley@hansenpartnership.com
| Cc: linux-scsi@vger.kernel.org; linux-kernel@vger.kernel.org; Gates, Matt;
| stephenmcameron@gmail.com; thenzl@redhat.com; akpm@linux-foundation.org;
| mikem@beardog.cce.hp.com
| Subject: [PATCH 06/17] hpsa: retry driver initiated commands on busy
| status
| 
| From: Matt Bondurant <Matthew.dav.bondurant@hp.com>
| 
| In shared SAS configurations we might get a busy status during driver
| initiated commands (e.g. during rescan for devices).  We should retry the
| command in such cases rather than giving up.

This may or may not apply to your situation. Some targets/disks return TASK SET FULL when they can't accept any new SCSI commands. If that applies to your situation, it may make sense to also check for SCSI status of SAM_STAT_TASK_SET_FULL.

Sunny

| 
| Signed-off-by: Matt Bondurant <Matthew.dav.bondurant@hp.com>
| Signed-off-by: Stephen M. Cameron <scameron@beardog.cce.hp.com>
| ---
|  drivers/scsi/hpsa.c |   12 +++++++++++-
|  1 files changed, 11 insertions(+), 1 deletions(-)
| 
| diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c index
| fcb9fb2..7355891 100644
| --- a/drivers/scsi/hpsa.c
| +++ b/drivers/scsi/hpsa.c
| @@ -234,6 +234,15 @@ static int check_for_unit_attention(struct ctlr_info
| *h,
|  	return 1;
|  }
| 
| +static int check_for_busy(struct ctlr_info *h, struct CommandList *c) {
| +	if (c->err_info->CommandStatus != CMD_TARGET_STATUS ||
| +		c->err_info->ScsiStatus != SAM_STAT_BUSY)
| +		return 0;
| +	dev_warn(&h->pdev->dev, HPSA "device busy");
| +	return 1;
| +}
| +
|  static ssize_t host_store_rescan(struct device *dev,
|  				 struct device_attribute *attr,
|  				 const char *buf, size_t count)
| @@ -1379,7 +1388,8 @@ static void
| hpsa_scsi_do_simple_cmd_with_retry(struct ctlr_info *h,
|  		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);
| +	} while ((check_for_unit_attention(h, c) ||
| +			check_for_busy(h, c)) && retry_count <= 3);
|  	hpsa_pci_unmap(h->pdev, c, 1, data_direction);  }
| 
| 
| --
| To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
| the body of a message to majordomo@vger.kernel.org More majordomo info at
| http://vger.kernel.org/majordomo-info.html
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

RE: [PATCH 06/17] hpsa: retry driver initiated commands on busy status

[Shergill, Gurinder]

| -----Original Message-----
| From: linux-scsi-owner@vger.kernel.org [mailto:linux-scsi-
| owner@vger.kernel.org] On Behalf Of Stephen M. Cameron
| Sent: Friday, April 20, 2012 8:07 AM
| To: james.bottomley@hansenpartnership.com
| Cc: linux-scsi@vger.kernel.org; linux-kernel@vger.kernel.org; Gates, Matt;
| stephenmcameron@gmail.com; thenzl@redhat.com; akpm@linux-foundation.org;
| mikem@beardog.cce.hp.com
| Subject: [PATCH 06/17] hpsa: retry driver initiated commands on busy
| status
| 
| From: Matt Bondurant <Matthew.dav.bondurant@hp.com>
| 
| In shared SAS configurations we might get a busy status during driver
| initiated commands (e.g. during rescan for devices).  We should retry the
| command in such cases rather than giving up.

This may or may not apply to your situation. Some targets/disks return TASK SET FULL when they can't accept any new SCSI commands. If that applies to your situation, it may make sense to also check for SCSI status of SAM_STAT_TASK_SET_FULL.

Sunny

| 
| Signed-off-by: Matt Bondurant <Matthew.dav.bondurant@hp.com>
| Signed-off-by: Stephen M. Cameron <scameron@beardog.cce.hp.com>
| ---
|  drivers/scsi/hpsa.c |   12 +++++++++++-
|  1 files changed, 11 insertions(+), 1 deletions(-)
| 
| diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c index
| fcb9fb2..7355891 100644
| --- a/drivers/scsi/hpsa.c
| +++ b/drivers/scsi/hpsa.c
| @@ -234,6 +234,15 @@ static int check_for_unit_attention(struct ctlr_info
| *h,
|  	return 1;
|  }
| 
| +static int check_for_busy(struct ctlr_info *h, struct CommandList *c) {
| +	if (c->err_info->CommandStatus != CMD_TARGET_STATUS ||
| +		c->err_info->ScsiStatus != SAM_STAT_BUSY)
| +		return 0;
| +	dev_warn(&h->pdev->dev, HPSA "device busy");
| +	return 1;
| +}
| +
|  static ssize_t host_store_rescan(struct device *dev,
|  				 struct device_attribute *attr,
|  				 const char *buf, size_t count)
| @@ -1379,7 +1388,8 @@ static void
| hpsa_scsi_do_simple_cmd_with_retry(struct ctlr_info *h,
|  		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);
| +	} while ((check_for_unit_attention(h, c) ||
| +			check_for_busy(h, c)) && retry_count <= 3);
|  	hpsa_pci_unmap(h->pdev, c, 1, data_direction);  }
| 
| 
| --
| To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
| the body of a message to majordomo@vger.kernel.org More majordomo info at
| http://vger.kernel.org/majordomo-info.html

Re: [PATCH 14/17] hpsa: use new IS_ENABLED macro

[Paul Gortmaker]

On Fri, Apr 20, 2012 at 11:07 AM, Stephen M. Cameron
<scameron@beardog.cce.hp.com> wrote:
> From: Stephen M. Cameron <scameron@beardog.cce.hp.com>
>
> Signed-off-by: Stephen M. Cameron <scameron@beardog.cce.hp.com>

You've not written a commit log, so I'm left guessing what the
intended rationale is here.  COMPAT, X86 and PCI_MSI are
I believe all bool, so why make this change?  To me it gives
a misleading message that some level of modular awareness
is needed here, when there really isn't any such need.

Thanks,
Paul.
---

> ---
>  drivers/scsi/hpsa.c |   12 ++++++------
>  1 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
> index 2e01583..67a0ad0 100644
> --- a/drivers/scsi/hpsa.c
> +++ b/drivers/scsi/hpsa.c
> @@ -139,7 +139,7 @@ static irqreturn_t do_hpsa_intr_msi(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);
>
> -#ifdef CONFIG_COMPAT
> +#if IS_ENABLED(COMPAT)
>  static int hpsa_compat_ioctl(struct scsi_device *dev, int cmd, void *arg);
>  #endif
>
> @@ -513,7 +513,7 @@ static struct scsi_host_template hpsa_driver_template = {
>        .ioctl                  = hpsa_ioctl,
>        .slave_alloc            = hpsa_slave_alloc,
>        .slave_destroy          = hpsa_slave_destroy,
> -#ifdef CONFIG_COMPAT
> +#if IS_ENABLED(COMPAT)
>        .compat_ioctl           = hpsa_compat_ioctl,
>  #endif
>        .sdev_attrs = hpsa_sdev_attrs,
> @@ -2721,7 +2721,7 @@ static void cmd_special_free(struct ctlr_info *h, struct CommandList *c)
>                            c, (dma_addr_t) (c->busaddr & DIRECT_LOOKUP_MASK));
>  }
>
> -#ifdef CONFIG_COMPAT
> +#if IS_ENABLED(COMPAT)
>
>  static int hpsa_ioctl32_passthru(struct scsi_device *dev, int cmd, void *arg)
>  {
> @@ -3972,7 +3972,7 @@ static int find_PCI_BAR_index(struct pci_dev *pdev, unsigned long pci_bar_addr)
>
>  static void __devinit hpsa_interrupt_mode(struct ctlr_info *h)
>  {
> -#ifdef CONFIG_PCI_MSI
> +#if IS_ENABLED(PCI_MSI)
>        int err, i;
>        struct msix_entry hpsa_msix_entries[MAX_REPLY_QUEUES];
>
> @@ -4013,7 +4013,7 @@ static void __devinit hpsa_interrupt_mode(struct ctlr_info *h)
>                        dev_warn(&h->pdev->dev, "MSI init failed\n");
>        }
>  default_int_mode:
> -#endif                         /* CONFIG_PCI_MSI */
> +#endif /* PCI_MSI enabled */
>        /* if we get here we're going to use the default interrupt mode */
>        h->intr[h->intr_mode] = h->pdev->irq;
>  }
> @@ -4187,7 +4187,7 @@ static inline bool hpsa_CISS_signature_present(struct ctlr_info *h)
>  /* Need to enable prefetch in the SCSI core for 6400 in x86 */
>  static inline void hpsa_enable_scsi_prefetch(struct ctlr_info *h)
>  {
> -#ifdef CONFIG_X86
> +#if IS_ENABLED(X86)
>        u32 prefetch;
>
>        prefetch = readl(&(h->cfgtable->SCSI_Prefetch));
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 14/17] hpsa: use new IS_ENABLED macro

[Paul Gortmaker]

On Fri, Apr 20, 2012 at 11:07 AM, Stephen M. Cameron
<scameron@beardog.cce.hp.com> wrote:
> From: Stephen M. Cameron <scameron@beardog.cce.hp.com>
>
> Signed-off-by: Stephen M. Cameron <scameron@beardog.cce.hp.com>

You've not written a commit log, so I'm left guessing what the
intended rationale is here.  COMPAT, X86 and PCI_MSI are
I believe all bool, so why make this change?  To me it gives
a misleading message that some level of modular awareness
is needed here, when there really isn't any such need.

Thanks,
Paul.
---

> ---
>  drivers/scsi/hpsa.c |   12 ++++++------
>  1 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
> index 2e01583..67a0ad0 100644
> --- a/drivers/scsi/hpsa.c
> +++ b/drivers/scsi/hpsa.c
> @@ -139,7 +139,7 @@ static irqreturn_t do_hpsa_intr_msi(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);
>
> -#ifdef CONFIG_COMPAT
> +#if IS_ENABLED(COMPAT)
>  static int hpsa_compat_ioctl(struct scsi_device *dev, int cmd, void *arg);
>  #endif
>
> @@ -513,7 +513,7 @@ static struct scsi_host_template hpsa_driver_template = {
>        .ioctl                  = hpsa_ioctl,
>        .slave_alloc            = hpsa_slave_alloc,
>        .slave_destroy          = hpsa_slave_destroy,
> -#ifdef CONFIG_COMPAT
> +#if IS_ENABLED(COMPAT)
>        .compat_ioctl           = hpsa_compat_ioctl,
>  #endif
>        .sdev_attrs = hpsa_sdev_attrs,
> @@ -2721,7 +2721,7 @@ static void cmd_special_free(struct ctlr_info *h, struct CommandList *c)
>                            c, (dma_addr_t) (c->busaddr & DIRECT_LOOKUP_MASK));
>  }
>
> -#ifdef CONFIG_COMPAT
> +#if IS_ENABLED(COMPAT)
>
>  static int hpsa_ioctl32_passthru(struct scsi_device *dev, int cmd, void *arg)
>  {
> @@ -3972,7 +3972,7 @@ static int find_PCI_BAR_index(struct pci_dev *pdev, unsigned long pci_bar_addr)
>
>  static void __devinit hpsa_interrupt_mode(struct ctlr_info *h)
>  {
> -#ifdef CONFIG_PCI_MSI
> +#if IS_ENABLED(PCI_MSI)
>        int err, i;
>        struct msix_entry hpsa_msix_entries[MAX_REPLY_QUEUES];
>
> @@ -4013,7 +4013,7 @@ static void __devinit hpsa_interrupt_mode(struct ctlr_info *h)
>                        dev_warn(&h->pdev->dev, "MSI init failed\n");
>        }
>  default_int_mode:
> -#endif                         /* CONFIG_PCI_MSI */
> +#endif /* PCI_MSI enabled */
>        /* if we get here we're going to use the default interrupt mode */
>        h->intr[h->intr_mode] = h->pdev->irq;
>  }
> @@ -4187,7 +4187,7 @@ static inline bool hpsa_CISS_signature_present(struct ctlr_info *h)
>  /* Need to enable prefetch in the SCSI core for 6400 in x86 */
>  static inline void hpsa_enable_scsi_prefetch(struct ctlr_info *h)
>  {
> -#ifdef CONFIG_X86
> +#if IS_ENABLED(X86)
>        u32 prefetch;
>
>        prefetch = readl(&(h->cfgtable->SCSI_Prefetch));
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 14/17] hpsa: use new IS_ENABLED macro

[Stephen Cameron]

On Sun, Apr 22, 2012 at 1:12 PM, Paul Gortmaker
<paul.gortmaker@windriver.com> wrote:
> On Fri, Apr 20, 2012 at 11:07 AM, Stephen M. Cameron
> <scameron@beardog.cce.hp.com> wrote:
>> From: Stephen M. Cameron <scameron@beardog.cce.hp.com>
>>
>> Signed-off-by: Stephen M. Cameron <scameron@beardog.cce.hp.com>
>
> You've not written a commit log, so I'm left guessing what the
> intended rationale is here.  COMPAT, X86 and PCI_MSI are
> I believe all bool, so why make this change?  To me it gives
> a misleading message that some level of modular awareness
> is needed here, when there really isn't any such need.

Well, I saw this commit go by: 69349c2dc01c489eccaa4c472542c08e370c6d7e

   Using IS_ENABLED() within C (vs.  within CPP #if statements) in its
   current form requires us to actually define every possible bool/tristate
   Kconfig option twice (__enabled_* and __enabled_*_MODULE variants).
   [...]

and so I kind of thought IS_ENABLED is the new way to do that sort of thing.

Perhaps I'm wrong about that.  Obviously the patch is not _needed_ for
things to work.

-- steve

Re: [PATCH 14/17] hpsa: use new IS_ENABLED macro

[Stephen Cameron]

On Sun, Apr 22, 2012 at 1:12 PM, Paul Gortmaker
<paul.gortmaker@windriver.com> wrote:
> On Fri, Apr 20, 2012 at 11:07 AM, Stephen M. Cameron
> <scameron@beardog.cce.hp.com> wrote:
>> From: Stephen M. Cameron <scameron@beardog.cce.hp.com>
>>
>> Signed-off-by: Stephen M. Cameron <scameron@beardog.cce.hp.com>
>
> You've not written a commit log, so I'm left guessing what the
> intended rationale is here.  COMPAT, X86 and PCI_MSI are
> I believe all bool, so why make this change?  To me it gives
> a misleading message that some level of modular awareness
> is needed here, when there really isn't any such need.

Well, I saw this commit go by: 69349c2dc01c489eccaa4c472542c08e370c6d7e

   Using IS_ENABLED() within C (vs.  within CPP #if statements) in its
   current form requires us to actually define every possible bool/tristate
   Kconfig option twice (__enabled_* and __enabled_*_MODULE variants).
   [...]

and so I kind of thought IS_ENABLED is the new way to do that sort of thing.

Perhaps I'm wrong about that.  Obviously the patch is not _needed_ for
things to work.

-- steve
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 14/17] hpsa: use new IS_ENABLED macro

[Paul Gortmaker]

On 12-04-22 10:20 PM, Stephen Cameron wrote:
> On Sun, Apr 22, 2012 at 1:12 PM, Paul Gortmaker
> <paul.gortmaker@windriver.com> wrote:
>> On Fri, Apr 20, 2012 at 11:07 AM, Stephen M. Cameron
>> <scameron@beardog.cce.hp.com> wrote:
>>> From: Stephen M. Cameron <scameron@beardog.cce.hp.com>
>>>
>>> Signed-off-by: Stephen M. Cameron <scameron@beardog.cce.hp.com>
>>
>> You've not written a commit log, so I'm left guessing what the
>> intended rationale is here.  COMPAT, X86 and PCI_MSI are
>> I believe all bool, so why make this change?  To me it gives
>> a misleading message that some level of modular awareness
>> is needed here, when there really isn't any such need.
> 
> Well, I saw this commit go by: 69349c2dc01c489eccaa4c472542c08e370c6d7e
> 
>    Using IS_ENABLED() within C (vs.  within CPP #if statements) in its
>    current form requires us to actually define every possible bool/tristate
>    Kconfig option twice (__enabled_* and __enabled_*_MODULE variants).
>    [...]
> 
> and so I kind of thought IS_ENABLED is the new way to do that sort of thing.

In my opinion, IS_ENABLED can/should be used when you have:

#if defined(CONFIG_FOO) || defined(CONFIG_FOO_MODULE)

i.e. "is this driver built in, or can it be loaded as a module?"
The CONFIG_FOO_MODULE doesn't appear in your .config -- it is auto
generated by Kbuild infrastructure.

It will do the Right Thing even for cases where CONFIG_FOO_MODULE
is impossible, but it does as I've said, give the wrong impression
that there is some possibility of modular presence.  At least to
those who are familiar with the implementation and why it exists.
[I'll grant you that the name doesn't convey the use case too well.]

> 
> Perhaps I'm wrong about that.  Obviously the patch is not _needed_ for
> things to work.

I don't think we want to go and mass replace all existing cases of

   #ifdef CONFIG_SOME_BOOL

with:

   #if IS_ENABLED(CONFIG_SOME_BOOL)

There is no value add.  However, replacing instances of:

   #if defined(CONFIG_FOO) || defined(CONFIG_FOO_MODULE)

with:

   #if IS_ENABLED(CONFIG_FOO)

is in my opinion, a reasonable thing to do.  It is shorter, and
it does hide the internally generated _MODULE suffixed "shadow"
variables from appearing directly in the main source files.  And
it tells you that the author was thinking about both the built
in and the modular cases when they wrote it.

Paul.
--

> 
> -- steve

Re: [PATCH 14/17] hpsa: use new IS_ENABLED macro

[Paul Gortmaker]

On 12-04-22 10:20 PM, Stephen Cameron wrote:
> On Sun, Apr 22, 2012 at 1:12 PM, Paul Gortmaker
> <paul.gortmaker@windriver.com> wrote:
>> On Fri, Apr 20, 2012 at 11:07 AM, Stephen M. Cameron
>> <scameron@beardog.cce.hp.com> wrote:
>>> From: Stephen M. Cameron <scameron@beardog.cce.hp.com>
>>>
>>> Signed-off-by: Stephen M. Cameron <scameron@beardog.cce.hp.com>
>>
>> You've not written a commit log, so I'm left guessing what the
>> intended rationale is here.  COMPAT, X86 and PCI_MSI are
>> I believe all bool, so why make this change?  To me it gives
>> a misleading message that some level of modular awareness
>> is needed here, when there really isn't any such need.
> 
> Well, I saw this commit go by: 69349c2dc01c489eccaa4c472542c08e370c6d7e
> 
>    Using IS_ENABLED() within C (vs.  within CPP #if statements) in its
>    current form requires us to actually define every possible bool/tristate
>    Kconfig option twice (__enabled_* and __enabled_*_MODULE variants).
>    [...]
> 
> and so I kind of thought IS_ENABLED is the new way to do that sort of thing.

In my opinion, IS_ENABLED can/should be used when you have:

#if defined(CONFIG_FOO) || defined(CONFIG_FOO_MODULE)

i.e. "is this driver built in, or can it be loaded as a module?"
The CONFIG_FOO_MODULE doesn't appear in your .config -- it is auto
generated by Kbuild infrastructure.

It will do the Right Thing even for cases where CONFIG_FOO_MODULE
is impossible, but it does as I've said, give the wrong impression
that there is some possibility of modular presence.  At least to
those who are familiar with the implementation and why it exists.
[I'll grant you that the name doesn't convey the use case too well.]

> 
> Perhaps I'm wrong about that.  Obviously the patch is not _needed_ for
> things to work.

I don't think we want to go and mass replace all existing cases of

   #ifdef CONFIG_SOME_BOOL

with:

   #if IS_ENABLED(CONFIG_SOME_BOOL)

There is no value add.  However, replacing instances of:

   #if defined(CONFIG_FOO) || defined(CONFIG_FOO_MODULE)

with:

   #if IS_ENABLED(CONFIG_FOO)

is in my opinion, a reasonable thing to do.  It is shorter, and
it does hide the internally generated _MODULE suffixed "shadow"
variables from appearing directly in the main source files.  And
it tells you that the author was thinking about both the built
in and the modular cases when they wrote it.

Paul.
--

> 
> -- steve

Re: [PATCH 14/17] hpsa: use new IS_ENABLED macro

[James Bottomley]

On Mon, 2012-04-23 at 09:56 -0400, Paul Gortmaker wrote:
> On 12-04-22 10:20 PM, Stephen Cameron wrote:
> > On Sun, Apr 22, 2012 at 1:12 PM, Paul Gortmaker
> > <paul.gortmaker@windriver.com> wrote:
> >> On Fri, Apr 20, 2012 at 11:07 AM, Stephen M. Cameron
> >> <scameron@beardog.cce.hp.com> wrote:
> >>> From: Stephen M. Cameron <scameron@beardog.cce.hp.com>
> >>>
> >>> Signed-off-by: Stephen M. Cameron <scameron@beardog.cce.hp.com>
> >>
> >> You've not written a commit log, so I'm left guessing what the
> >> intended rationale is here.  COMPAT, X86 and PCI_MSI are
> >> I believe all bool, so why make this change?  To me it gives
> >> a misleading message that some level of modular awareness
> >> is needed here, when there really isn't any such need.
> > 
> > Well, I saw this commit go by: 69349c2dc01c489eccaa4c472542c08e370c6d7e
> > 
> >    Using IS_ENABLED() within C (vs.  within CPP #if statements) in its
> >    current form requires us to actually define every possible bool/tristate
> >    Kconfig option twice (__enabled_* and __enabled_*_MODULE variants).
> >    [...]
> > 
> > and so I kind of thought IS_ENABLED is the new way to do that sort of thing.

I don't think you need to change the driver unless there's a good
reason.  In kernel terms, it's usually better to wait a bit to see if
the wheels fall off any particular bandwagon before leaping on it ...

> In my opinion, IS_ENABLED can/should be used when you have:
> 
> #if defined(CONFIG_FOO) || defined(CONFIG_FOO_MODULE)
> 
> i.e. "is this driver built in, or can it be loaded as a module?"
> The CONFIG_FOO_MODULE doesn't appear in your .config -- it is auto
> generated by Kbuild infrastructure.
> 
> It will do the Right Thing even for cases where CONFIG_FOO_MODULE
> is impossible, but it does as I've said, give the wrong impression
> that there is some possibility of modular presence.  At least to
> those who are familiar with the implementation and why it exists.
> [I'll grant you that the name doesn't convey the use case too well.]
> 
> > 
> > Perhaps I'm wrong about that.  Obviously the patch is not _needed_ for
> > things to work.
> 
> I don't think we want to go and mass replace all existing cases of
> 
>    #ifdef CONFIG_SOME_BOOL
> 
> with:
> 
>    #if IS_ENABLED(CONFIG_SOME_BOOL)
> 
> There is no value add.  However, replacing instances of:
> 
>    #if defined(CONFIG_FOO) || defined(CONFIG_FOO_MODULE)
> 
> with:
> 
>    #if IS_ENABLED(CONFIG_FOO)
> 
> is in my opinion, a reasonable thing to do.  It is shorter, and
> it does hide the internally generated _MODULE suffixed "shadow"
> variables from appearing directly in the main source files.  And
> it tells you that the author was thinking about both the built
> in and the modular cases when they wrote it.

To be honest, I think we want to use #if IS_ENABLED() for all cases
going forwards, including the boolean case.

The reason is that it's an easier design pattern.  If we have the #ifdef
CONFIG_X vs #if IS_ENABLED(CONFIG_X) depending on whether CONFIG_X can
be modular or not it's just creating pointless rules that someone will
annoy us all by enforcing in a checkpatch or some other script.  Whereas
#if IS_ENABLED(CONFIG_X) is obvious and simple.

James

Re: [PATCH 14/17] hpsa: use new IS_ENABLED macro

[Paul Gortmaker]

On Mon, Apr 23, 2012 at 10:56 AM, James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
> On Mon, 2012-04-23 at 09:56 -0400, Paul Gortmaker wrote:
>> On 12-04-22 10:20 PM, Stephen Cameron wrote:
>> > On Sun, Apr 22, 2012 at 1:12 PM, Paul Gortmaker
>> > <paul.gortmaker@windriver.com> wrote:
>> >> On Fri, Apr 20, 2012 at 11:07 AM, Stephen M. Cameron
>> >> <scameron@beardog.cce.hp.com> wrote:
>> >>> From: Stephen M. Cameron <scameron@beardog.cce.hp.com>
>> >>>
>> >>> Signed-off-by: Stephen M. Cameron <scameron@beardog.cce.hp.com>
>> >>
>> >> You've not written a commit log, so I'm left guessing what the
>> >> intended rationale is here.  COMPAT, X86 and PCI_MSI are
>> >> I believe all bool, so why make this change?  To me it gives
>> >> a misleading message that some level of modular awareness
>> >> is needed here, when there really isn't any such need.
>> >
>> > Well, I saw this commit go by: 69349c2dc01c489eccaa4c472542c08e370c6d7e
>> >
>> >    Using IS_ENABLED() within C (vs.  within CPP #if statements) in its
>> >    current form requires us to actually define every possible bool/tristate
>> >    Kconfig option twice (__enabled_* and __enabled_*_MODULE variants).
>> >    [...]
>> >
>> > and so I kind of thought IS_ENABLED is the new way to do that sort of thing.
>
> I don't think you need to change the driver unless there's a good
> reason.  In kernel terms, it's usually better to wait a bit to see if
> the wheels fall off any particular bandwagon before leaping on it ...
>
>> In my opinion, IS_ENABLED can/should be used when you have:
>>
>> #if defined(CONFIG_FOO) || defined(CONFIG_FOO_MODULE)
>>
>> i.e. "is this driver built in, or can it be loaded as a module?"
>> The CONFIG_FOO_MODULE doesn't appear in your .config -- it is auto
>> generated by Kbuild infrastructure.
>>
>> It will do the Right Thing even for cases where CONFIG_FOO_MODULE
>> is impossible, but it does as I've said, give the wrong impression
>> that there is some possibility of modular presence.  At least to
>> those who are familiar with the implementation and why it exists.
>> [I'll grant you that the name doesn't convey the use case too well.]
>>
>> >
>> > Perhaps I'm wrong about that.  Obviously the patch is not _needed_ for
>> > things to work.
>>
>> I don't think we want to go and mass replace all existing cases of
>>
>>    #ifdef CONFIG_SOME_BOOL
>>
>> with:
>>
>>    #if IS_ENABLED(CONFIG_SOME_BOOL)
>>
>> There is no value add.  However, replacing instances of:
>>
>>    #if defined(CONFIG_FOO) || defined(CONFIG_FOO_MODULE)
>>
>> with:
>>
>>    #if IS_ENABLED(CONFIG_FOO)
>>
>> is in my opinion, a reasonable thing to do.  It is shorter, and
>> it does hide the internally generated _MODULE suffixed "shadow"
>> variables from appearing directly in the main source files.  And
>> it tells you that the author was thinking about both the built
>> in and the modular cases when they wrote it.
>
> To be honest, I think we want to use #if IS_ENABLED() for all cases
> going forwards, including the boolean case.

I guess we will have to agree to disagree then.

>
> The reason is that it's an easier design pattern.  If we have the #ifdef
> CONFIG_X vs #if IS_ENABLED(CONFIG_X) depending on whether CONFIG_X can
> be modular or not it's just creating pointless rules that someone will
> annoy us all by enforcing in a checkpatch or some other script.  Whereas
> #if IS_ENABLED(CONFIG_X) is obvious and simple.

What exactly is an "easier design pattern", and what are the gains?

To me, it has nothing to do with rules, and what the checkpatch folks do
or do not do.  The line "#ifdef CONFIG_FOO" conveys one specific piece
of information.  The line "#if IS_ENABLED(CONFIG_FOO) conveys
a different piece of information, which is a superset of the former.

If you blindly convert all of them, you throw away information that would
otherwise be available to the code reader.  I would not support that.

Thanks,
Paul.
--

>
> James
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 14/17] hpsa: use new IS_ENABLED macro

[James Bottomley]

On Mon, 2012-04-23 at 21:43 -0400, Paul Gortmaker wrote:
> On Mon, Apr 23, 2012 at 10:56 AM, James Bottomley
> <James.Bottomley@hansenpartnership.com> wrote:
> > On Mon, 2012-04-23 at 09:56 -0400, Paul Gortmaker wrote:
> >> On 12-04-22 10:20 PM, Stephen Cameron wrote:
> >> > On Sun, Apr 22, 2012 at 1:12 PM, Paul Gortmaker
> >> > <paul.gortmaker@windriver.com> wrote:
> >> >> On Fri, Apr 20, 2012 at 11:07 AM, Stephen M. Cameron
> >> >> <scameron@beardog.cce.hp.com> wrote:
> >> >>> From: Stephen M. Cameron <scameron@beardog.cce.hp.com>
> >> >>>
> >> >>> Signed-off-by: Stephen M. Cameron <scameron@beardog.cce.hp.com>
> >> >>
> >> >> You've not written a commit log, so I'm left guessing what the
> >> >> intended rationale is here.  COMPAT, X86 and PCI_MSI are
> >> >> I believe all bool, so why make this change?  To me it gives
> >> >> a misleading message that some level of modular awareness
> >> >> is needed here, when there really isn't any such need.
> >> >
> >> > Well, I saw this commit go by: 69349c2dc01c489eccaa4c472542c08e370c6d7e
> >> >
> >> >    Using IS_ENABLED() within C (vs.  within CPP #if statements) in its
> >> >    current form requires us to actually define every possible bool/tristate
> >> >    Kconfig option twice (__enabled_* and __enabled_*_MODULE variants).
> >> >    [...]
> >> >
> >> > and so I kind of thought IS_ENABLED is the new way to do that sort of thing.
> >
> > I don't think you need to change the driver unless there's a good
> > reason.  In kernel terms, it's usually better to wait a bit to see if
> > the wheels fall off any particular bandwagon before leaping on it ...
> >
> >> In my opinion, IS_ENABLED can/should be used when you have:
> >>
> >> #if defined(CONFIG_FOO) || defined(CONFIG_FOO_MODULE)
> >>
> >> i.e. "is this driver built in, or can it be loaded as a module?"
> >> The CONFIG_FOO_MODULE doesn't appear in your .config -- it is auto
> >> generated by Kbuild infrastructure.
> >>
> >> It will do the Right Thing even for cases where CONFIG_FOO_MODULE
> >> is impossible, but it does as I've said, give the wrong impression
> >> that there is some possibility of modular presence.  At least to
> >> those who are familiar with the implementation and why it exists.
> >> [I'll grant you that the name doesn't convey the use case too well.]
> >>
> >> >
> >> > Perhaps I'm wrong about that.  Obviously the patch is not _needed_ for
> >> > things to work.
> >>
> >> I don't think we want to go and mass replace all existing cases of
> >>
> >>    #ifdef CONFIG_SOME_BOOL
> >>
> >> with:
> >>
> >>    #if IS_ENABLED(CONFIG_SOME_BOOL)
> >>
> >> There is no value add.  However, replacing instances of:
> >>
> >>    #if defined(CONFIG_FOO) || defined(CONFIG_FOO_MODULE)
> >>
> >> with:
> >>
> >>    #if IS_ENABLED(CONFIG_FOO)
> >>
> >> is in my opinion, a reasonable thing to do.  It is shorter, and
> >> it does hide the internally generated _MODULE suffixed "shadow"
> >> variables from appearing directly in the main source files.  And
> >> it tells you that the author was thinking about both the built
> >> in and the modular cases when they wrote it.
> >
> > To be honest, I think we want to use #if IS_ENABLED() for all cases
> > going forwards, including the boolean case.
> 
> I guess we will have to agree to disagree then.
> 
> >
> > The reason is that it's an easier design pattern.  If we have the #ifdef
> > CONFIG_X vs #if IS_ENABLED(CONFIG_X) depending on whether CONFIG_X can
> > be modular or not it's just creating pointless rules that someone will
> > annoy us all by enforcing in a checkpatch or some other script.  Whereas
> > #if IS_ENABLED(CONFIG_X) is obvious and simple.
> 
> What exactly is an "easier design pattern", and what are the gains?

It means you only have one rule:

everything is #if IS_ENABLED

vs two arbitrary ones

If it's a boolean symbol, you use #ifdef else if it's tristate, use #if
IS_ENABLED

It's obvious to me the former is simpler.

> To me, it has nothing to do with rules, and what the checkpatch folks do
> or do not do.  The line "#ifdef CONFIG_FOO" conveys one specific piece
> of information.  The line "#if IS_ENABLED(CONFIG_FOO) conveys
> a different piece of information, which is a superset of the former.

That's the point ... why bother with the former?  If the latter is a
superset and you always do the latter, the rule is simpler.

> If you blindly convert all of them, you throw away information that would
> otherwise be available to the code reader.  I would not support that.

Well, a) I'm not advocating converting everything.  I'm advocating using
the superset for everything going forwards.

I don't understand what information would be lost:  All use if #ifdef vs
#if IS_ENABLED tells you is whether the author thought the symbol was
boolean or tristate.  I don't see how that's useful at all and it will
cause problems if a symbol changes (some non-modular code may become
modular for instance) ... then we have to go and chase it through the
code base.

James

Re: [PATCH 14/17] hpsa: use new IS_ENABLED macro

[Paul Gortmaker]

On 12-04-24 04:25 AM, James Bottomley wrote:
> On Mon, 2012-04-23 at 21:43 -0400, Paul Gortmaker wrote:
>> On Mon, Apr 23, 2012 at 10:56 AM, James Bottomley
>> <James.Bottomley@hansenpartnership.com> wrote:
>>> On Mon, 2012-04-23 at 09:56 -0400, Paul Gortmaker wrote:
>>>> On 12-04-22 10:20 PM, Stephen Cameron wrote:
>>>>> On Sun, Apr 22, 2012 at 1:12 PM, Paul Gortmaker
>>>>> <paul.gortmaker@windriver.com> wrote:
>>>>>> On Fri, Apr 20, 2012 at 11:07 AM, Stephen M. Cameron
>>>>>> <scameron@beardog.cce.hp.com> wrote:
>>>>>>> From: Stephen M. Cameron <scameron@beardog.cce.hp.com>
>>>>>>>
>>>>>>> Signed-off-by: Stephen M. Cameron <scameron@beardog.cce.hp.com>
>>>>>>
>>>>>> You've not written a commit log, so I'm left guessing what the
>>>>>> intended rationale is here.  COMPAT, X86 and PCI_MSI are
>>>>>> I believe all bool, so why make this change?  To me it gives
>>>>>> a misleading message that some level of modular awareness
>>>>>> is needed here, when there really isn't any such need.
>>>>>
>>>>> Well, I saw this commit go by: 69349c2dc01c489eccaa4c472542c08e370c6d7e
>>>>>
>>>>>    Using IS_ENABLED() within C (vs.  within CPP #if statements) in its
>>>>>    current form requires us to actually define every possible bool/tristate
>>>>>    Kconfig option twice (__enabled_* and __enabled_*_MODULE variants).
>>>>>    [...]
>>>>>
>>>>> and so I kind of thought IS_ENABLED is the new way to do that sort of thing.
>>>
>>> I don't think you need to change the driver unless there's a good
>>> reason.  In kernel terms, it's usually better to wait a bit to see if
>>> the wheels fall off any particular bandwagon before leaping on it ...
>>>
>>>> In my opinion, IS_ENABLED can/should be used when you have:
>>>>
>>>> #if defined(CONFIG_FOO) || defined(CONFIG_FOO_MODULE)
>>>>
>>>> i.e. "is this driver built in, or can it be loaded as a module?"
>>>> The CONFIG_FOO_MODULE doesn't appear in your .config -- it is auto
>>>> generated by Kbuild infrastructure.
>>>>
>>>> It will do the Right Thing even for cases where CONFIG_FOO_MODULE
>>>> is impossible, but it does as I've said, give the wrong impression
>>>> that there is some possibility of modular presence.  At least to
>>>> those who are familiar with the implementation and why it exists.
>>>> [I'll grant you that the name doesn't convey the use case too well.]
>>>>
>>>>>
>>>>> Perhaps I'm wrong about that.  Obviously the patch is not _needed_ for
>>>>> things to work.
>>>>
>>>> I don't think we want to go and mass replace all existing cases of
>>>>
>>>>    #ifdef CONFIG_SOME_BOOL
>>>>
>>>> with:
>>>>
>>>>    #if IS_ENABLED(CONFIG_SOME_BOOL)
>>>>
>>>> There is no value add.  However, replacing instances of:
>>>>
>>>>    #if defined(CONFIG_FOO) || defined(CONFIG_FOO_MODULE)
>>>>
>>>> with:
>>>>
>>>>    #if IS_ENABLED(CONFIG_FOO)
>>>>
>>>> is in my opinion, a reasonable thing to do.  It is shorter, and
>>>> it does hide the internally generated _MODULE suffixed "shadow"
>>>> variables from appearing directly in the main source files.  And
>>>> it tells you that the author was thinking about both the built
>>>> in and the modular cases when they wrote it.
>>>
>>> To be honest, I think we want to use #if IS_ENABLED() for all cases
>>> going forwards, including the boolean case.
>>
>> I guess we will have to agree to disagree then.
>>
>>>
>>> The reason is that it's an easier design pattern.  If we have the #ifdef
>>> CONFIG_X vs #if IS_ENABLED(CONFIG_X) depending on whether CONFIG_X can
>>> be modular or not it's just creating pointless rules that someone will
>>> annoy us all by enforcing in a checkpatch or some other script.  Whereas
>>> #if IS_ENABLED(CONFIG_X) is obvious and simple.
>>
>> What exactly is an "easier design pattern", and what are the gains?
> 
> It means you only have one rule:
> 
> everything is #if IS_ENABLED
> 
> vs two arbitrary ones

Again I'll have to disagree with you categorizing them as "arbitrary".

> 
> If it's a boolean symbol, you use #ifdef else if it's tristate, use #if
> IS_ENABLED
> 
> It's obvious to me the former is simpler.

So, something in you doesn't go "eeech" when you see:

#if IS_ENABLED(CONFIG_ARM)

...that treats ARM as if it is some kind of driver, or feature
that can simply be enabled at will, alongside of any other
features?  Or worse, when people take it to the next level
and do:

-#ifdef CONFIG_SPARC
-	boo();
-	bar();
-	baz();
-#endif
+	if (IS_ENABLED(CONFIG_SPARC)) {
+		boo();
+		bar();
+		baz();
+	}

These are kinds of instances that I hope we don't entertain
as being improvements, or use cases we'd adopt as defaults.

> 
>> To me, it has nothing to do with rules, and what the checkpatch folks do
>> or do not do.  The line "#ifdef CONFIG_FOO" conveys one specific piece
>> of information.  The line "#if IS_ENABLED(CONFIG_FOO) conveys
>> a different piece of information, which is a superset of the former.
> 
> That's the point ... why bother with the former?  If the latter is a
> superset and you always do the latter, the rule is simpler.
> 
>> If you blindly convert all of them, you throw away information that would
>> otherwise be available to the code reader.  I would not support that.
> 
> Well, a) I'm not advocating converting everything.  I'm advocating using
> the superset for everything going forwards.
> 
> I don't understand what information would be lost:  All use if #ifdef vs
> #if IS_ENABLED tells you is whether the author thought the symbol was
> boolean or tristate.  I don't see how that's useful at all and it will
> cause problems if a symbol changes (some non-modular code may become
> modular for instance) ... then we have to go and chase it through the
> code base.

Hopefully the above makes my concerns more clear -- IS_ENABLED should
not be used blindly without thought.  Some things like core arch configs
will never _ever_ be modular.  Seeing IS_ENABLED used on those kinds of
options just seems ugly to me.  That is what was in this original patch.

But using it on some driver that isn't currently modular, but might be
made so eventually is reasonable.  Maybe you will still call that
distinction arbitrary, and the arch use case not ugly enough to matter.

Nor IMHO should IS_ENABLED be used as a tool to engage a holy war on
eradicating all #ifdef/#endif block.  Just like "goto", sensible uses
of such blocks can make things more readable, not less.

Your point earlier about recommending people show restraint before
rushing out to use the shiny new tool just because they can, was
a good one -- I agree wholeheartedly with that.

Thanks,
Paul.

> 
> James
> 
> 

Re: [PATCH 14/17] hpsa: use new IS_ENABLED macro

[Paul Gortmaker]

On 12-04-24 04:25 AM, James Bottomley wrote:
> On Mon, 2012-04-23 at 21:43 -0400, Paul Gortmaker wrote:
>> On Mon, Apr 23, 2012 at 10:56 AM, James Bottomley
>> <James.Bottomley@hansenpartnership.com> wrote:
>>> On Mon, 2012-04-23 at 09:56 -0400, Paul Gortmaker wrote:
>>>> On 12-04-22 10:20 PM, Stephen Cameron wrote:
>>>>> On Sun, Apr 22, 2012 at 1:12 PM, Paul Gortmaker
>>>>> <paul.gortmaker@windriver.com> wrote:
>>>>>> On Fri, Apr 20, 2012 at 11:07 AM, Stephen M. Cameron
>>>>>> <scameron@beardog.cce.hp.com> wrote:
>>>>>>> From: Stephen M. Cameron <scameron@beardog.cce.hp.com>
>>>>>>>
>>>>>>> Signed-off-by: Stephen M. Cameron <scameron@beardog.cce.hp.com>
>>>>>>
>>>>>> You've not written a commit log, so I'm left guessing what the
>>>>>> intended rationale is here.  COMPAT, X86 and PCI_MSI are
>>>>>> I believe all bool, so why make this change?  To me it gives
>>>>>> a misleading message that some level of modular awareness
>>>>>> is needed here, when there really isn't any such need.
>>>>>
>>>>> Well, I saw this commit go by: 69349c2dc01c489eccaa4c472542c08e370c6d7e
>>>>>
>>>>>    Using IS_ENABLED() within C (vs.  within CPP #if statements) in its
>>>>>    current form requires us to actually define every possible bool/tristate
>>>>>    Kconfig option twice (__enabled_* and __enabled_*_MODULE variants).
>>>>>    [...]
>>>>>
>>>>> and so I kind of thought IS_ENABLED is the new way to do that sort of thing.
>>>
>>> I don't think you need to change the driver unless there's a good
>>> reason.  In kernel terms, it's usually better to wait a bit to see if
>>> the wheels fall off any particular bandwagon before leaping on it ...
>>>
>>>> In my opinion, IS_ENABLED can/should be used when you have:
>>>>
>>>> #if defined(CONFIG_FOO) || defined(CONFIG_FOO_MODULE)
>>>>
>>>> i.e. "is this driver built in, or can it be loaded as a module?"
>>>> The CONFIG_FOO_MODULE doesn't appear in your .config -- it is auto
>>>> generated by Kbuild infrastructure.
>>>>
>>>> It will do the Right Thing even for cases where CONFIG_FOO_MODULE
>>>> is impossible, but it does as I've said, give the wrong impression
>>>> that there is some possibility of modular presence.  At least to
>>>> those who are familiar with the implementation and why it exists.
>>>> [I'll grant you that the name doesn't convey the use case too well.]
>>>>
>>>>>
>>>>> Perhaps I'm wrong about that.  Obviously the patch is not _needed_ for
>>>>> things to work.
>>>>
>>>> I don't think we want to go and mass replace all existing cases of
>>>>
>>>>    #ifdef CONFIG_SOME_BOOL
>>>>
>>>> with:
>>>>
>>>>    #if IS_ENABLED(CONFIG_SOME_BOOL)
>>>>
>>>> There is no value add.  However, replacing instances of:
>>>>
>>>>    #if defined(CONFIG_FOO) || defined(CONFIG_FOO_MODULE)
>>>>
>>>> with:
>>>>
>>>>    #if IS_ENABLED(CONFIG_FOO)
>>>>
>>>> is in my opinion, a reasonable thing to do.  It is shorter, and
>>>> it does hide the internally generated _MODULE suffixed "shadow"
>>>> variables from appearing directly in the main source files.  And
>>>> it tells you that the author was thinking about both the built
>>>> in and the modular cases when they wrote it.
>>>
>>> To be honest, I think we want to use #if IS_ENABLED() for all cases
>>> going forwards, including the boolean case.
>>
>> I guess we will have to agree to disagree then.
>>
>>>
>>> The reason is that it's an easier design pattern.  If we have the #ifdef
>>> CONFIG_X vs #if IS_ENABLED(CONFIG_X) depending on whether CONFIG_X can
>>> be modular or not it's just creating pointless rules that someone will
>>> annoy us all by enforcing in a checkpatch or some other script.  Whereas
>>> #if IS_ENABLED(CONFIG_X) is obvious and simple.
>>
>> What exactly is an "easier design pattern", and what are the gains?
> 
> It means you only have one rule:
> 
> everything is #if IS_ENABLED
> 
> vs two arbitrary ones

Again I'll have to disagree with you categorizing them as "arbitrary".

> 
> If it's a boolean symbol, you use #ifdef else if it's tristate, use #if
> IS_ENABLED
> 
> It's obvious to me the former is simpler.

So, something in you doesn't go "eeech" when you see:

#if IS_ENABLED(CONFIG_ARM)

...that treats ARM as if it is some kind of driver, or feature
that can simply be enabled at will, alongside of any other
features?  Or worse, when people take it to the next level
and do:

-#ifdef CONFIG_SPARC
-	boo();
-	bar();
-	baz();
-#endif
+	if (IS_ENABLED(CONFIG_SPARC)) {
+		boo();
+		bar();
+		baz();
+	}

These are kinds of instances that I hope we don't entertain
as being improvements, or use cases we'd adopt as defaults.

> 
>> To me, it has nothing to do with rules, and what the checkpatch folks do
>> or do not do.  The line "#ifdef CONFIG_FOO" conveys one specific piece
>> of information.  The line "#if IS_ENABLED(CONFIG_FOO) conveys
>> a different piece of information, which is a superset of the former.
> 
> That's the point ... why bother with the former?  If the latter is a
> superset and you always do the latter, the rule is simpler.
> 
>> If you blindly convert all of them, you throw away information that would
>> otherwise be available to the code reader.  I would not support that.
> 
> Well, a) I'm not advocating converting everything.  I'm advocating using
> the superset for everything going forwards.
> 
> I don't understand what information would be lost:  All use if #ifdef vs
> #if IS_ENABLED tells you is whether the author thought the symbol was
> boolean or tristate.  I don't see how that's useful at all and it will
> cause problems if a symbol changes (some non-modular code may become
> modular for instance) ... then we have to go and chase it through the
> code base.

Hopefully the above makes my concerns more clear -- IS_ENABLED should
not be used blindly without thought.  Some things like core arch configs
will never _ever_ be modular.  Seeing IS_ENABLED used on those kinds of
options just seems ugly to me.  That is what was in this original patch.

But using it on some driver that isn't currently modular, but might be
made so eventually is reasonable.  Maybe you will still call that
distinction arbitrary, and the arch use case not ugly enough to matter.

Nor IMHO should IS_ENABLED be used as a tool to engage a holy war on
eradicating all #ifdef/#endif block.  Just like "goto", sensible uses
of such blocks can make things more readable, not less.

Your point earlier about recommending people show restraint before
rushing out to use the shiny new tool just because they can, was
a good one -- I agree wholeheartedly with that.

Thanks,
Paul.

> 
> James
> 
> 

Re: [PATCH 14/17] hpsa: use new IS_ENABLED macro

[scameron]

On Tue, Apr 24, 2012 at 11:15:26AM -0400, Paul Gortmaker wrote:
> On 12-04-24 04:25 AM, James Bottomley wrote:
> > On Mon, 2012-04-23 at 21:43 -0400, Paul Gortmaker wrote:
> >> On Mon, Apr 23, 2012 at 10:56 AM, James Bottomley
> >> <James.Bottomley@hansenpartnership.com> wrote:
> >>> On Mon, 2012-04-23 at 09:56 -0400, Paul Gortmaker wrote:
> >>>> On 12-04-22 10:20 PM, Stephen Cameron wrote:
> >>>>> On Sun, Apr 22, 2012 at 1:12 PM, Paul Gortmaker
> >>>>> <paul.gortmaker@windriver.com> wrote:
> >>>>>> On Fri, Apr 20, 2012 at 11:07 AM, Stephen M. Cameron
> >>>>>> <scameron@beardog.cce.hp.com> wrote:
> >>>>>>> From: Stephen M. Cameron <scameron@beardog.cce.hp.com>
> >>>>>>>
> >>>>>>> Signed-off-by: Stephen M. Cameron <scameron@beardog.cce.hp.com>
> >>>>>>
> >>>>>> You've not written a commit log, so I'm left guessing what the
> >>>>>> intended rationale is here.  COMPAT, X86 and PCI_MSI are
> >>>>>> I believe all bool, so why make this change?  To me it gives
> >>>>>> a misleading message that some level of modular awareness
> >>>>>> is needed here, when there really isn't any such need.
> >>>>>
> >>>>> Well, I saw this commit go by: 69349c2dc01c489eccaa4c472542c08e370c6d7e
> >>>>>
> >>>>>    Using IS_ENABLED() within C (vs.  within CPP #if statements) in its
> >>>>>    current form requires us to actually define every possible bool/tristate
> >>>>>    Kconfig option twice (__enabled_* and __enabled_*_MODULE variants).
> >>>>>    [...]
> >>>>>
> >>>>> and so I kind of thought IS_ENABLED is the new way to do that sort of thing.
> >>>
> >>> I don't think you need to change the driver unless there's a good
> >>> reason.  In kernel terms, it's usually better to wait a bit to see if
> >>> the wheels fall off any particular bandwagon before leaping on it ...
> >>>
> >>>> In my opinion, IS_ENABLED can/should be used when you have:
> >>>>
> >>>> #if defined(CONFIG_FOO) || defined(CONFIG_FOO_MODULE)
> >>>>
> >>>> i.e. "is this driver built in, or can it be loaded as a module?"
> >>>> The CONFIG_FOO_MODULE doesn't appear in your .config -- it is auto
> >>>> generated by Kbuild infrastructure.
> >>>>
> >>>> It will do the Right Thing even for cases where CONFIG_FOO_MODULE
> >>>> is impossible, but it does as I've said, give the wrong impression
> >>>> that there is some possibility of modular presence.  At least to
> >>>> those who are familiar with the implementation and why it exists.
> >>>> [I'll grant you that the name doesn't convey the use case too well.]
> >>>>
> >>>>>
> >>>>> Perhaps I'm wrong about that.  Obviously the patch is not _needed_ for
> >>>>> things to work.
> >>>>
> >>>> I don't think we want to go and mass replace all existing cases of
> >>>>
> >>>>    #ifdef CONFIG_SOME_BOOL
> >>>>
> >>>> with:
> >>>>
> >>>>    #if IS_ENABLED(CONFIG_SOME_BOOL)
> >>>>
> >>>> There is no value add.  However, replacing instances of:
> >>>>
> >>>>    #if defined(CONFIG_FOO) || defined(CONFIG_FOO_MODULE)
> >>>>
> >>>> with:
> >>>>
> >>>>    #if IS_ENABLED(CONFIG_FOO)
> >>>>
> >>>> is in my opinion, a reasonable thing to do.  It is shorter, and
> >>>> it does hide the internally generated _MODULE suffixed "shadow"
> >>>> variables from appearing directly in the main source files.  And
> >>>> it tells you that the author was thinking about both the built
> >>>> in and the modular cases when they wrote it.
> >>>
> >>> To be honest, I think we want to use #if IS_ENABLED() for all cases
> >>> going forwards, including the boolean case.
> >>
> >> I guess we will have to agree to disagree then.
> >>
> >>>
> >>> The reason is that it's an easier design pattern.  If we have the #ifdef
> >>> CONFIG_X vs #if IS_ENABLED(CONFIG_X) depending on whether CONFIG_X can
> >>> be modular or not it's just creating pointless rules that someone will
> >>> annoy us all by enforcing in a checkpatch or some other script.  Whereas
> >>> #if IS_ENABLED(CONFIG_X) is obvious and simple.
> >>
> >> What exactly is an "easier design pattern", and what are the gains?
> > 
> > It means you only have one rule:
> > 
> > everything is #if IS_ENABLED
> > 
> > vs two arbitrary ones
> 
> Again I'll have to disagree with you categorizing them as "arbitrary".
> 
> > 
> > If it's a boolean symbol, you use #ifdef else if it's tristate, use #if
> > IS_ENABLED
> > 
> > It's obvious to me the former is simpler.
> 
> So, something in you doesn't go "eeech" when you see:
> 
> #if IS_ENABLED(CONFIG_ARM)
> 
> ...that treats ARM as if it is some kind of driver, or feature
> that can simply be enabled at will, alongside of any other
> features?  Or worse, when people take it to the next level
> and do:
> 
> -#ifdef CONFIG_SPARC
> -	boo();
> -	bar();
> -	baz();
> -#endif
> +	if (IS_ENABLED(CONFIG_SPARC)) {
> +		boo();
> +		bar();
> +		baz();
> +	}
> 
> These are kinds of instances that I hope we don't entertain
> as being improvements, or use cases we'd adopt as defaults.
> 
> > 
> >> To me, it has nothing to do with rules, and what the checkpatch folks do
> >> or do not do.  The line "#ifdef CONFIG_FOO" conveys one specific piece
> >> of information.  The line "#if IS_ENABLED(CONFIG_FOO) conveys
> >> a different piece of information, which is a superset of the former.
> > 
> > That's the point ... why bother with the former?  If the latter is a
> > superset and you always do the latter, the rule is simpler.
> > 
> >> If you blindly convert all of them, you throw away information that would
> >> otherwise be available to the code reader.  I would not support that.
> > 
> > Well, a) I'm not advocating converting everything.  I'm advocating using
> > the superset for everything going forwards.
> > 
> > I don't understand what information would be lost:  All use if #ifdef vs
> > #if IS_ENABLED tells you is whether the author thought the symbol was
> > boolean or tristate.  I don't see how that's useful at all and it will
> > cause problems if a symbol changes (some non-modular code may become
> > modular for instance) ... then we have to go and chase it through the
> > code base.
> 
> Hopefully the above makes my concerns more clear -- IS_ENABLED should
> not be used blindly without thought.  Some things like core arch configs
> will never _ever_ be modular.  Seeing IS_ENABLED used on those kinds of
> options just seems ugly to me.  That is what was in this original patch.
> 
> But using it on some driver that isn't currently modular, but might be
> made so eventually is reasonable.  Maybe you will still call that
> distinction arbitrary, and the arch use case not ugly enough to matter.
> 
> Nor IMHO should IS_ENABLED be used as a tool to engage a holy war on
> eradicating all #ifdef/#endif block.  Just like "goto", sensible uses
> of such blocks can make things more readable, not less.
> 
> Your point earlier about recommending people show restraint before
> rushing out to use the shiny new tool just because they can, was
> a good one -- I agree wholeheartedly with that.
> 
> Thanks,
> Paul.
> 
> > 
> > James
> > 
> > 


Also, in "[PATCH 13/17] hpsa: factor out hpsa_free_irqs_and_disable_msix", I did this:

[...]
-#ifdef CONFIG_PCI_MSI
-	if (h->msix_vector)
-		pci_disable_msix(h->pdev);
-	else if (h->msi_vector)
-		pci_disable_msi(h->pdev);
-#endif /* CONFIG_PCI_MSI */
+	if (!IS_ENABLED(PCI_MSI))
+		return;
+	if (h->msix_vector) {
+		if (h->pdev->msix_enabled)
+			pci_disable_msix(h->pdev);
+	} else if (h->msi_vector) {
+		if (h->pdev->msi_enabled)
+			pci_disable_msi(h->pdev);
+	}
+}
[...]

Presumably I should not use IS_ENABLED in this instance either.

-- steve