ALUA device handler update, part 1

As scsi_dh.c is now always compiled in we should be moving the 'dh_state' attribute to the generic code.

Reviewed-by Christoph Hellwig <[email protected]>

Signed-off-by Hannes Reinecke <[email protected]>

---
 drivers/scsi/scsi_dh.c    | 68 +----------------------------------
------------
 drivers/scsi/scsi_sysfs.c | 58
++++++++++++++++++++++++++++++++++++++++

 2 files changed, 59 insertions(+), 67 deletions(-)

drivers/scsi/scsi_dh.c +0 -0

index 0a2168e..57ad0f3 100644
@@ -153,76 +153,11 @@ static void scsi_dh_handler_detach(struct
 scsi_device *sdev)
  	module_put(sdev->handler->module);
  }

drivers/scsi/scsi_dh.c +0 -38

-/*
- * Functions for sysfs attribute 'dh_state'
- */
-static ssize_t
-store_dh_state(struct device *dev, struct device_attribute *attr,
-	       const char *buf, size_t count)
-{
-	struct scsi_device *sdev = to_scsi_device(dev);
-	struct scsi_device_handler *scsi_dh;
-	int err = -EINVAL;
-
-	if (sdev->sdev_state == SDEV_CANCEL ||
-	    sdev->sdev_state == SDEV_DEL)
-		return -ENODEV;
-
-	if (!sdev->handler) {
-		/*
-		 * Attach to a device handler
-		 */
-		scsi_dh = scsi_dh_lookup(buf);
-		if (!scsi_dh)
-			return err;
-		err = scsi_dh_handler_attach(sdev, scsi_dh);
-	} else {
-		if (!strncmp(buf, "detach", 6)) {
-			/*
-			 * Detach from a device handler
-			 */
-			sdev_printk(KERN_WARNING, sdev,
-				    "can't detach handler %s.\n",
-				    sdev->handler->name);
-			err = -EINVAL;
-		} else if (!strncmp(buf, "activate", 8)) {
-			/*
-			 * Activate a device handler
-			 */
-			if (sdev->handler->activate)
-				err = sdev->handler->activate(sdev,

NULL, NULL);

drivers/scsi/scsi_dh.c +0 -10

-			else
-				err = 0;
-		}
-	}
-
-	return err<0?err:count;
-}
-
-static ssize_t
-show_dh_state(struct device *dev, struct device_attribute *attr,

char *buf)

drivers/scsi/scsi_dh.c +0 -13

-{
-	struct scsi_device *sdev = to_scsi_device(dev);
-
-	if (!sdev->handler)
-		return snprintf(buf, 20, "detached\n");
-
-	return snprintf(buf, 20, "%s\n", sdev->handler->name);
-}
-
-static struct device_attribute scsi_dh_state_attr =
-	__ATTR(dh_state, S_IRUGO | S_IWUSR, show_dh_state,
-	       store_dh_state);
-

 int scsi_dh_add_device(struct scsi_device *sdev) {
  struct scsi_device_handler *devinfo = NULL; const char *drv;

drivers/scsi/scsi_dh.c +0 -3

-	int err;
-
-	err = device_create_file(&sdev->sdev_gendev,

&scsi_dh_state_attr);

drivers/scsi/scsi_dh.c +1 -2

-	if (err)
-		return err;
+	int err = 0;

  drv = scsi_dh_find_driver(sdev);
  if (drv)

drivers/scsi/scsi_dh.c +0 -1

@@ -236,7 +171,6 @@ void scsi_dh_remove_device(struct scsi_device
 *sdev)
  {
  	if (sdev->handler)
  		scsi_dh_handler_detach(sdev);
-	device_remove_file(&sdev->sdev_gendev, &scsi_dh_state_attr);
  }

 /*

drivers/scsi/scsi_sysfs.c +1 -0

index f021423..13a5ede 100644
@@ -17,6 +17,7 @@
  #include <scsi/scsi_device.h>
  #include <scsi/scsi_host.h>
  #include <scsi/scsi_tcq.h>
+#include <scsi/scsi_dh.h>
  #include <scsi/scsi_transport.h>
  #include <scsi/scsi_driver.h>

drivers/scsi/scsi_sysfs.c +0 -0

@@ -902,6 +903,60 @@ sdev_show_function(queue_depth, "%d\n");
  static DEVICE_ATTR(queue_depth, S_IRUGO | S_IWUSR,
 sdev_show_queue_depth,
  		   sdev_store_queue_depth);

drivers/scsi/scsi_sysfs.c +3 -0

+#ifdef CONFIG_SCSI_DH
+static ssize_t
+sdev_show_dh_state(struct device *dev, struct device_attribute

*attr,

drivers/scsi/scsi_sysfs.c +12 -0

+		   char *buf)
+{
+	struct scsi_device *sdev = to_scsi_device(dev);
+
+	if (!sdev->handler)
+		return snprintf(buf, 20, "detached\n");
+
+	return snprintf(buf, 20, "%s\n", sdev->handler->name);
+}
+
+static ssize_t
+sdev_store_dh_state(struct device *dev, struct device_attribute

*attr,

drivers/scsi/scsi_sysfs.c +20 -0

+		    const char *buf, size_t count)
+{
+	struct scsi_device *sdev = to_scsi_device(dev);
+	int err = -EINVAL;
+
+	if (sdev->sdev_state == SDEV_CANCEL ||
+	    sdev->sdev_state == SDEV_DEL)
+		return -ENODEV;
+
+	if (!sdev->handler) {
+		/*
+		 * Attach to a device handler
+		 */
+		err = scsi_dh_attach(sdev->request_queue, buf);
+	} else if (!strncmp(buf, "activate", 8)) {
+		/*
+		 * Activate a device handler
+		 */
+		if (sdev->handler->activate)
+			err = sdev->handler->activate(sdev, NULL,

NULL);

drivers/scsi/scsi_sysfs.c +19 -0

+		else
+			err = 0;
+	} else if (!strncmp(buf, "detach", 6)) {
+		/*
+		 * Detach from a device handler
+		 */
+		sdev_printk(KERN_WARNING, sdev,
+			    "can't detach handler %s.\n",
+			    sdev->handler->name);
+		err = -EINVAL;
+	}
+
+	return err < 0 ? err : count;
+}
+
+static DEVICE_ATTR(dh_state, S_IRUGO | S_IWUSR, sdev_show_dh_state,
+		   sdev_store_dh_state);
+#endif
+

 static ssize_t sdev_show_queue_ramp_up_period(struct device *dev, struct device_attribute *attr,

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

"Hannes" == Hannes Reinecke <[email protected]> writes:

device handler initialisation might fail due to a number of reasons. But as device_handlers are optional this shouldn't cause us to disable the device entirely.
So just ignore errors from scsi_dh_add_device().

Signed-off-by Hannes Reinecke <[email protected]>

---
 drivers/scsi/scsi_sysfs.c | 7 ++++---

 1 file changed, 4 insertions(+), 3 deletions(-)

drivers/scsi/scsi_sysfs.c +0 -0

index 13a5ede..7b41b2c 100644
@@ -1118,11 +1118,12 @@ int scsi_sysfs_add_sdev(struct scsi_device
 *sdev)
  	}

  error = scsi_dh_add_device(sdev);

drivers/scsi/scsi_sysfs.c +3 -1

-	if (error) {
+	if (error)
+		/*
+		 * device_handler is optional, so any error can be

ignored

drivers/scsi/scsi_sysfs.c +1 -0

+		 */

  sdev_printk(KERN_INFO, sdev,
  "failed to add device handler:
%d\n", error);

drivers/scsi/scsi_sysfs.c +0 -2

-		return error;
-	}

device_enable_async_suspend(&sdev->sdev_dev); error = device_add(&sdev->sdev_dev);

Non-disk devices might support ALUA, but the firmware implementation is untested and frequently broken. As we're don't actually need it disable ALUA support for non-disk device for now.

Signed-off-by Hannes Reinecke <[email protected]>

Reviewed-by Bart Van Assche <[email protected]>

Reviewed-by Christoph Hellwig <[email protected]>

Reviewed-by Martin K. Petersen <[email protected]>

---
 drivers/scsi/device_handler/scsi_dh_alua.c | 12 ++++++++++++

 1 file changed, 12 insertions(+)

drivers/scsi/device_handler/scsi_dh_alua.c +0 -0

index cc2773b..7d01ef0 100644
@@ -320,6 +320,18 @@ static int alua_check_tpgs(struct scsi_device
 *sdev, struct alua_dh_data *h)
  {
  	int err = SCSI_DH_OK;

drivers/scsi/device_handler/scsi_dh_alua.c +12 -0

+	/*
+	 * ALUA support for non-disk devices is fraught with
+	 * difficulties, so disable it for now.
+	 */
+	if (sdev->type != TYPE_DISK) {
+		h->tpgs = TPGS_MODE_NONE;
+		sdev_printk(KERN_INFO, sdev,
+			    "%s: disable for non-disk devices\n",
+			    ALUA_DH_NAME);
+		return SCSI_DH_DEV_UNSUPP;
+	}
+

  h->tpgs = scsi_device_tpgs(sdev); switch (h->tpgs) {
  case TPGS_MODE_EXPLICIT|TPGS_MODE_IMPLICIT:

The SCSI device now has the VPD page 0x83 information attached, so there is no need to query it again.

Reviewed-by Martin K. Petersen <[email protected]>

Reviewed-by Christoph Hellwig <[email protected]>

Signed-off-by Hannes Reinecke <[email protected]>

---
 drivers/scsi/device_handler/scsi_dh_alua.c | 87 +++++++-------------
----------

 1 file changed, 18 insertions(+), 69 deletions(-)

drivers/scsi/device_handler/scsi_dh_alua.c +0 -0

index 7d01ef0..9b3b2f7 100644
@@ -131,43 +131,6 @@ static struct request *get_alua_req(struct
 scsi_device *sdev,
  }

 /*
- * submit_vpd_inquiry - Issue an INQUIRY VPD page 0x83 command
- * @sdev: sdev the command should be sent to
- */
-static int submit_vpd_inquiry(struct scsi_device *sdev, struct alua_dh_data *h)

drivers/scsi/device_handler/scsi_dh_alua.c +0 -33

-{
-	struct request *rq;
-	int err = SCSI_DH_RES_TEMP_UNAVAIL;
-
-	rq = get_alua_req(sdev, h->buff, h->bufflen, READ);
-	if (!rq)
-		goto done;
-
-	/* Prepare the command. */
-	rq->cmd[0] = INQUIRY;
-	rq->cmd[1] = 1;
-	rq->cmd[2] = 0x83;
-	rq->cmd[4] = h->bufflen;
-	rq->cmd_len = COMMAND_SIZE(INQUIRY);
-
-	rq->sense = h->sense;
-	memset(rq->sense, 0, SCSI_SENSE_BUFFERSIZE);
-	rq->sense_len = h->senselen = 0;
-
-	err = blk_execute_rq(rq->q, NULL, rq, 1);
-	if (err == -EIO) {
-		sdev_printk(KERN_INFO, sdev,
-			    "%s: evpd inquiry failed with %x\n",
-			    ALUA_DH_NAME, rq->errors);
-		h->senselen = rq->sense_len;
-		err = SCSI_DH_IO;
-	}
-	blk_put_request(rq);
-done:
-	return err;
-}
-
-/*

  * submit_rtpg - Issue a REPORT TARGET GROUP STATES command
  * @sdev: sdev the command should be sent to
  */

drivers/scsi/device_handler/scsi_dh_alua.c +0 -0

@@ -359,43 +322,29 @@ static int alua_check_tpgs(struct scsi_device
 *sdev, struct alua_dh_data *h)
  }

 /*
- * alua_vpd_inquiry - Evaluate INQUIRY vpd page 0x83
+ * alua_check_vpd - Evaluate INQUIRY vpd page 0x83
  * @sdev: device to be checked
  *
  * Extract the relative target port and the target port group
  * descriptor from the list of identificators.
  */
-static int alua_vpd_inquiry(struct scsi_device *sdev, struct alua_dh_data *h)
+static int alua_check_vpd(struct scsi_device *sdev, struct alua_dh_data *h)
 {

drivers/scsi/device_handler/scsi_dh_alua.c +0 -2

-	int len;
-	unsigned err;

  unsigned char *d;

drivers/scsi/device_handler/scsi_dh_alua.c +1 -0

+	unsigned char __rcu *vpd_pg83;

- retry:

drivers/scsi/device_handler/scsi_dh_alua.c +4 -17

-	err = submit_vpd_inquiry(sdev, h);
-
-	if (err != SCSI_DH_OK)
-		return err;
-
-	/* Check if vpd page exceeds initial buffer */
-	len = (h->buff[2] << 8) + h->buff[3] + 4;
-	if (len > h->bufflen) {
-		/* Resubmit with the correct length */
-		if (realloc_buffer(h, len)) {
-			sdev_printk(KERN_WARNING, sdev,
-				    "%s: kmalloc buffer failed\n",
-				    ALUA_DH_NAME);
-			/* Temporary failure, bypass */
-			return SCSI_DH_DEV_TEMP_BUSY;
-		}
-		goto retry;
+	rcu_read_lock();
+	if (!rcu_dereference(sdev->vpd_pg83)){
+		rcu_read_unlock();
+		return SCSI_DH_DEV_UNSUPP;

  }

  /*

drivers/scsi/device_handler/scsi_dh_alua.c +1 -1

-	 * Now look for the correct descriptor.
+	 * Look for the correct descriptor.

   */

drivers/scsi/device_handler/scsi_dh_alua.c +3 -2

-	d = h->buff + 4;
-	while (d < h->buff + len) {
+	vpd_pg83 = rcu_dereference(sdev->vpd_pg83);
+	d = vpd_pg83 + 4;
+	while (d < vpd_pg83 + sdev->vpd_pg83_len) {

  switch (d[1] & 0xf) {
  case 0x4:
  /* Relative target port */

drivers/scsi/device_handler/scsi_dh_alua.c +1 -0

@@ -410,6 +359,7 @@ static int alua_vpd_inquiry(struct scsi_device
 *sdev, struct alua_dh_data *h)
  		}
  		d += d[3] + 4;
  	}
+	rcu_read_unlock();

  if (h->group_id == -1) {
  /*

drivers/scsi/device_handler/scsi_dh_alua.c +4 -5

@@ -422,14 +372,13 @@ static int alua_vpd_inquiry(struct scsi_device
 *sdev, struct alua_dh_data *h)
  			    ALUA_DH_NAME);
  		h->state = TPGS_STATE_OPTIMIZED;
  		h->tpgs = TPGS_MODE_NONE;
-		err = SCSI_DH_DEV_UNSUPP;
-	} else {
-		sdev_printk(KERN_INFO, sdev,
-			    "%s: port group %02x rel port %02x\n",
-			    ALUA_DH_NAME, h->group_id, h->rel_port);
+		return SCSI_DH_DEV_UNSUPP;
  	}
+	sdev_printk(KERN_INFO, sdev,
+		    "%s: port group %02x rel port %02x\n",
+		    ALUA_DH_NAME, h->group_id, h->rel_port);

drivers/scsi/device_handler/scsi_dh_alua.c +1 -1

-	return err;
+	return 0;

 }

 static char print_alua_state(int state)

drivers/scsi/device_handler/scsi_dh_alua.c +0 -0

@@ -692,7 +641,7 @@ static int alua_initialize(struct scsi_device
 *sdev, struct alua_dh_data *h)
  	if (err != SCSI_DH_OK)
  		goto out;

drivers/scsi/device_handler/scsi_dh_alua.c +1 -1

-	err = alua_vpd_inquiry(sdev, h);
+	err = alua_check_vpd(sdev, h);

  if (err != SCSI_DH_OK)
  goto out;

Issue different logging messages if ALUA is not supported or the TPGS setting is invalid.

Reviewed-by Martin K. Petersen <[email protected]>

Reviewed-by Christoph Hellwig <[email protected]>

Signed-off-by Hannes Reinecke <[email protected]>

---
 drivers/scsi/device_handler/scsi_dh_alua.c | 10 ++++++++--

 1 file changed, 8 insertions(+), 2 deletions(-)

drivers/scsi/device_handler/scsi_dh_alua.c +0 -0

index 9b3b2f7..c63f304 100644
@@ -310,12 +310,18 @@ static int alua_check_tpgs(struct scsi_device
 *sdev, struct alua_dh_data *h)

  sdev_printk(KERN_INFO, sdev, "%s: supports implicit TPGS\n",
      ALUA_DH_NAME);
  break;

drivers/scsi/device_handler/scsi_dh_alua.c +1 -2

-	default:
-		h->tpgs = TPGS_MODE_NONE;
+	case TPGS_MODE_NONE:

  sdev_printk(KERN_INFO, sdev, "%s: not supported\n", ALUA_DH_NAME);
  err = SCSI_DH_DEV_UNSUPP;
  break;

drivers/scsi/device_handler/scsi_dh_alua.c +7 -0

+	default:
+		sdev_printk(KERN_INFO, sdev,
+			    "%s: unsupported TPGS setting %d\n",
+			    ALUA_DH_NAME, h->tpgs);
+		h->tpgs = TPGS_MODE_NONE;
+		err = SCSI_DH_DEV_UNSUPP;
+		break;

  }

  return err;

The only check for a valid sense code is calling scsi_normalize_sense()
and check the return value. So drop the pointless checks and rely on scsi_normalize_sense() to figure out if the sense code is valid. With that we can also remove the 'senselen' field.

Reviewed-by Bart van Assche <[email protected]>

Signed-off-by Hannes Reinecke <[email protected]>

---
 drivers/scsi/device_handler/scsi_dh_alua.c | 18 +++++++-----------

 1 file changed, 7 insertions(+), 11 deletions(-)

drivers/scsi/device_handler/scsi_dh_alua.c +0 -1

index c63f304..240a5dc 100644
@@ -73,7 +73,6 @@ struct alua_dh_data {
  	int			bufflen;
  	unsigned char		transition_tmo;
  	unsigned char		sense[SCSI_SENSE_BUFFERSIZE];
-	int			senselen;
  	struct scsi_device	*sdev;
  	activate_complete	callback_fn;
  	void			*callback_data;

@@ -158,14 +157,13 @@ static unsigned submit_rtpg(struct scsi_device
 *sdev, struct alua_dh_data *h,

  rq->sense = h->sense; memset(rq->sense, 0, SCSI_SENSE_BUFFERSIZE);

drivers/scsi/device_handler/scsi_dh_alua.c +1 -1

-	rq->sense_len = h->senselen = 0;
+	rq->sense_len = 0;

  err = blk_execute_rq(rq->q, NULL, rq, 1); if (err == -EIO) { sdev_printk(KERN_INFO, sdev, "%s: rtpg failed with %x\n", ALUA_DH_NAME, rq->errors);

drivers/scsi/device_handler/scsi_dh_alua.c +0 -1

-		h->senselen = rq->sense_len;

  err = SCSI_DH_IO;
  }
  blk_put_request(rq);

drivers/scsi/device_handler/scsi_dh_alua.c +0 -0

@@ -194,9 +192,8 @@ static void stpg_endio(struct request *req, int
 error)
  		goto done;
  	}

drivers/scsi/device_handler/scsi_dh_alua.c +0 -2

-	if (req->sense_len > 0) {
-		err = scsi_normalize_sense(h->sense,

SCSI_SENSE_BUFFERSIZE,

drivers/scsi/device_handler/scsi_dh_alua.c +2 -1

-					   &sense_hdr);
+	if (scsi_normalize_sense(h->sense, SCSI_SENSE_BUFFERSIZE,
+				 &sense_hdr)) {

  if (!err) {
  err = SCSI_DH_IO;
  goto done;

drivers/scsi/device_handler/scsi_dh_alua.c +0 -0

@@ -265,7 +262,7 @@ static unsigned submit_stpg(struct alua_dh_data
 *h)

  rq->sense = h->sense; memset(rq->sense, 0, SCSI_SENSE_BUFFERSIZE);

drivers/scsi/device_handler/scsi_dh_alua.c +1 -1

-	rq->sense_len = h->senselen = 0;
+	rq->sense_len = 0;

  rq->end_io_data = h;

  blk_execute_rq_nowait(rq->q, NULL, rq, 1, stpg_endio);

drivers/scsi/device_handler/scsi_dh_alua.c +0 -0

@@ -514,10 +511,9 @@ static int alua_rtpg(struct scsi_device *sdev,

struct alua_dh_data *h, int wait_
  retry:
  err = submit_rtpg(sdev, h, rtpg_ext_hdr_req);

drivers/scsi/device_handler/scsi_dh_alua.c +0 -2

-	if (err == SCSI_DH_IO && h->senselen > 0) {
-		err = scsi_normalize_sense(h->sense,

SCSI_SENSE_BUFFERSIZE,

drivers/scsi/device_handler/scsi_dh_alua.c +2 -2

-					   &sense_hdr);
-		if (!err)
+	if (err == SCSI_DH_IO) {
+		if (!scsi_normalize_sense(h->sense,

SCSI_SENSE_BUFFERSIZE,

drivers/scsi/device_handler/scsi_dh_alua.c +1 -0

+					  &sense_hdr))

  return SCSI_DH_IO;

  /*

Use standard logging functions instead of hand-crafted ones.

Reviewed-by Bart Van Assche <[email protected]>

Signed-off-by Hannes Reinecke <[email protected]>

---
 drivers/scsi/device_handler/scsi_dh_alua.c | 27 +++++++++++++-------
-------

 1 file changed, 13 insertions(+), 14 deletions(-)

drivers/scsi/device_handler/scsi_dh_alua.c +1 -0

index 240a5dc..b817963 100644
@@ -23,6 +23,7 @@
  #include <linux/delay.h>
  #include <linux/module.h>
  #include <scsi/scsi.h>
+#include <scsi/scsi_dbg.h>
  #include <scsi/scsi_eh.h>
  #include <scsi/scsi_dh.h>

drivers/scsi/device_handler/scsi_dh_alua.c +0 -0

@@ -194,19 +195,14 @@ static void stpg_endio(struct request *req, int
 error)

  if (scsi_normalize_sense(h->sense, SCSI_SENSE_BUFFERSIZE, &sense_hdr)) {

drivers/scsi/device_handler/scsi_dh_alua.c +0 -4

-		if (!err) {
-			err = SCSI_DH_IO;
-			goto done;
-		}

  err = alua_check_sense(h->sdev, &sense_hdr); if (err == ADD_TO_MLQUEUE) {
  err = SCSI_DH_RETRY;
  goto done;
  }

drivers/scsi/device_handler/scsi_dh_alua.c +3 -4

-		sdev_printk(KERN_INFO, h->sdev,
-			    "%s: stpg sense code: %02x/%02x/%02x\n",
-			    ALUA_DH_NAME, sense_hdr.sense_key,
-			    sense_hdr.asc, sense_hdr.ascq);
+		sdev_printk(KERN_INFO, h->sdev, "%s: stpg failed\n",
+			    ALUA_DH_NAME);
+		scsi_print_sense_hdr(h->sdev, ALUA_DH_NAME,

&sense_hdr);
  err = SCSI_DH_IO;
  } else if (error)
  err = SCSI_DH_IO;

drivers/scsi/device_handler/scsi_dh_alua.c +0 -0

@@ -532,13 +528,16 @@ static int alua_rtpg(struct scsi_device *sdev,

struct alua_dh_data *h, int wait_
  }

  err = alua_check_sense(sdev, &sense_hdr);

drivers/scsi/device_handler/scsi_dh_alua.c +0 -1

-		if (err == ADD_TO_MLQUEUE && time_before(jiffies,

expiry))

drivers/scsi/device_handler/scsi_dh_alua.c +1 -0

+		if (err == ADD_TO_MLQUEUE && time_before(jiffies,

expiry)) {

drivers/scsi/device_handler/scsi_dh_alua.c +1 -0

+			sdev_printk(KERN_ERR, sdev, "%s: rtpg

retry\n",

drivers/scsi/device_handler/scsi_dh_alua.c +2 -0

+				    ALUA_DH_NAME);
+			scsi_print_sense_hdr(sdev, ALUA_DH_NAME,

&sense_hdr);
  goto retry;

drivers/scsi/device_handler/scsi_dh_alua.c +4 -5

-		sdev_printk(KERN_INFO, sdev,
-			    "%s: rtpg sense code %02x/%02x/%02x\n",
-			    ALUA_DH_NAME, sense_hdr.sense_key,
-			    sense_hdr.asc, sense_hdr.ascq);
-		err = SCSI_DH_IO;
+		}
+		sdev_printk(KERN_ERR, sdev, "%s: rtpg failed\n",
+			    ALUA_DH_NAME);
+		scsi_print_sense_hdr(sdev, ALUA_DH_NAME,

&sense_hdr);

drivers/scsi/device_handler/scsi_dh_alua.c +1 -0

+		return SCSI_DH_IO;

  }
  if (err != SCSI_DH_OK)
  return err;

Fixup submit_rtpg() to always return a standard SCSI return code.

Signed-off-by Hannes Reinecke <[email protected]>

---
 drivers/scsi/device_handler/scsi_dh_alua.c | 33 +++++++++++++++-----
----------

 1 file changed, 17 insertions(+), 16 deletions(-)

drivers/scsi/device_handler/scsi_dh_alua.c +1 -1

index b817963..50fe87c 100644
@@ -138,11 +138,13 @@ static unsigned submit_rtpg(struct scsi_device
 *sdev, struct alua_dh_data *h,
  			    bool rtpg_ext_hdr_req)
  {
  	struct request *rq;
-	int err = SCSI_DH_RES_TEMP_UNAVAIL;
+	int err = 0;

  rq = get_alua_req(sdev, h->buff, h->bufflen, READ);

drivers/scsi/device_handler/scsi_dh_alua.c +2 -1

-	if (!rq)
+	if (!rq) {
+		err = DRIVER_BUSY << 24;

  goto done;

drivers/scsi/device_handler/scsi_dh_alua.c +1 -0

+	}

  /* Prepare the command. */
  rq->cmd[0] = MAINTENANCE_IN;

drivers/scsi/device_handler/scsi_dh_alua.c +0 -0

@@ -160,13 +162,9 @@ static unsigned submit_rtpg(struct scsi_device
 *sdev, struct alua_dh_data *h,
  	memset(rq->sense, 0, SCSI_SENSE_BUFFERSIZE);
  	rq->sense_len = 0;

drivers/scsi/device_handler/scsi_dh_alua.c +3 -7

-	err = blk_execute_rq(rq->q, NULL, rq, 1);
-	if (err == -EIO) {
-		sdev_printk(KERN_INFO, sdev,
-			    "%s: rtpg failed with %x\n",
-			    ALUA_DH_NAME, rq->errors);
-		err = SCSI_DH_IO;
-	}
+	blk_execute_rq(rq->q, NULL, rq, 1);
+	if (rq->errors)
+		err = rq->errors;

  blk_put_request(rq);
 done:
  return err;

drivers/scsi/device_handler/scsi_dh_alua.c +0 -0

@@ -493,7 +491,7 @@ static int alua_rtpg(struct scsi_device *sdev,

struct alua_dh_data *h, int wait_
  struct scsi_sense_hdr sense_hdr;
  int len, k, off, valid_states = 0;
  unsigned char *ucp;

drivers/scsi/device_handler/scsi_dh_alua.c +1 -1

-	unsigned err;
+	unsigned err, retval;

  bool rtpg_ext_hdr_req = 1;
  unsigned long expiry, interval = 0;
  unsigned int tpg_desc_tbl_off;

drivers/scsi/device_handler/scsi_dh_alua.c +0 -0

@@ -505,12 +503,17 @@ static int alua_rtpg(struct scsi_device *sdev,

struct alua_dh_data *h, int wait_
  expiry = round_jiffies_up(jiffies + h-

Quoted 1 line

transition_tmo * HZ);

  retry:

drivers/scsi/device_handler/scsi_dh_alua.c +2 -3

-	err = submit_rtpg(sdev, h, rtpg_ext_hdr_req);
-
-	if (err == SCSI_DH_IO) {
+	retval = submit_rtpg(sdev, h, rtpg_ext_hdr_req);
+	if (retval) {

  if (!scsi_normalize_sense(h->sense, SCSI_SENSE_BUFFERSIZE,

drivers/scsi/device_handler/scsi_dh_alua.c +6 -1

-					  &sense_hdr))
+					  &sense_hdr)) {
+			sdev_printk(KERN_INFO, sdev,
+				    "%s: rtpg failed, result %d\n",
+				    ALUA_DH_NAME, retval);
+			if (driver_byte(retval) == DRIVER_BUSY)
+				return SCSI_DH_DEV_TEMP_BUSY;

  return SCSI_DH_IO;

drivers/scsi/device_handler/scsi_dh_alua.c +1 -0

+		}

  /*
   * submit_rtpg() has failed on existing arrays

drivers/scsi/device_handler/scsi_dh_alua.c +0 -0

@@ -539,8 +542,6 @@ static int alua_rtpg(struct scsi_device *sdev,

struct alua_dh_data *h, int wait_ scsi_print_sense_hdr(sdev, ALUA_DH_NAME, &sense_hdr);
  return SCSI_DH_IO;
  }

drivers/scsi/device_handler/scsi_dh_alua.c +0 -2

-	if (err != SCSI_DH_OK)
-		return err;

  len = (h->buff[0] << 24) + (h->buff[1] << 16) + (h->buff[2] << 8) + h->buff[3] + 4;

Fixup copy-and-paste error in the description of stpg_endio().

Signed-off-by Hannes Reinecke <[email protected]>

Reviewed-by Christoph Hellwig <[email protected]>

Reviewed-by Martin K. Petersen <[email protected]>

---
 drivers/scsi/device_handler/scsi_dh_alua.c | 6 ++----

 1 file changed, 2 insertions(+), 4 deletions(-)

drivers/scsi/device_handler/scsi_dh_alua.c +0 -0

index 50fe87c..ea626732 100644
@@ -171,13 +171,11 @@ done:
  }

 /*
- * alua_stpg - Evaluate SET TARGET GROUP STATES
+ * stpg_endio - Evaluate SET TARGET GROUP STATES
  * @sdev: the device to be evaluated
  * @state: the new target group state

  *
- * Send a SET TARGET GROUP STATES command to the device.
- * We only have to test here if we should resubmit the command;
- * any other error is assumed as a failure.
+ * Evaluate a SET TARGET GROUP STATES command response. */
 static void stpg_endio(struct request *req, int error) {

Last caller is gone, so remove it.

Reviewed-by Bart Van Assche <[email protected]>

Reviewed-by Christoph Hellwig <[email protected]>

Reviewed-by Martin K. Petersen <[email protected]>

Signed-off-by Hannes Reinecke <[email protected]>

---
 include/scsi/scsi_dbg.h | 2 --

 1 file changed, 2 deletions(-)

include/scsi/scsi_dbg.h +0 -0

index f8170e9..56710e0 100644
@@ -12,8 +12,6 @@ extern size_t __scsi_format_command(char *, size_t,
  				   const unsigned char *, size_t);

 extern void scsi_show_extd_sense(const struct scsi_device *, const char *,
   unsigned char, unsigned char); -extern void scsi_show_sense_hdr(const struct scsi_device *, const char *,

include/scsi/scsi_dbg.h +0 -1

-				const struct scsi_sense_hdr *);

 extern void scsi_print_sense_hdr(const struct scsi_device *, const char *,
   const struct scsi_sense_hdr *); extern void scsi_print_sense(const struct scsi_cmnd *);

We should be using a flag when RTPG extended header is not supported, that saves us sending RTPG twice for older arrays.

Reviewed-by Bart Van Assche <[email protected]>

Reviewed-by Christoph Hellwig <[email protected]>

Reviewed-by Martin K. Petersen <[email protected]>

Signed-off-by Hannes Reinecke <[email protected]>

---
 drivers/scsi/device_handler/scsi_dh_alua.c | 15 +++++++--------

 1 file changed, 7 insertions(+), 8 deletions(-)

drivers/scsi/device_handler/scsi_dh_alua.c +0 -0

index ea626732..03f8190 100644
@@ -59,8 +59,9 @@
  #define ALUA_FAILOVER_TIMEOUT		60
  #define ALUA_FAILOVER_RETRIES		5

drivers/scsi/device_handler/scsi_dh_alua.c +1 -1

-/* flags passed from user level */
+/* device handler flags */

 #define ALUA_OPTIMIZE_STPG 1
+#define ALUA_RTPG_EXT_HDR_UNSUPP 2

 struct alua_dh_data {
  int group_id;

drivers/scsi/device_handler/scsi_dh_alua.c +1 -2

@@ -134,8 +135,7 @@ static struct request *get_alua_req(struct
 scsi_device *sdev,
   * submit_rtpg - Issue a REPORT TARGET GROUP STATES command
   * @sdev: sdev the command should be sent to
   */
-static unsigned submit_rtpg(struct scsi_device *sdev, struct
 alua_dh_data *h,
-			    bool rtpg_ext_hdr_req)
+static unsigned submit_rtpg(struct scsi_device *sdev, struct
 alua_dh_data *h)

 {
  struct request *rq;
  int err = 0;

drivers/scsi/device_handler/scsi_dh_alua.c +0 -0

@@ -148,7 +148,7 @@ static unsigned submit_rtpg(struct scsi_device
 *sdev, struct alua_dh_data *h,

  /* Prepare the command. */
  rq->cmd[0] = MAINTENANCE_IN;

drivers/scsi/device_handler/scsi_dh_alua.c +1 -1

-	if (rtpg_ext_hdr_req)
+	if (!(h->flags & ALUA_RTPG_EXT_HDR_UNSUPP))

  rq->cmd[1] = MI_REPORT_TARGET_PGS | MI_EXT_HDR_PARAM_FMT;
  else
  rq->cmd[1] = MI_REPORT_TARGET_PGS;

drivers/scsi/device_handler/scsi_dh_alua.c +0 -0

@@ -490,7 +490,6 @@ static int alua_rtpg(struct scsi_device *sdev,

struct alua_dh_data *h, int wait_
  int len, k, off, valid_states = 0;
  unsigned char *ucp;
  unsigned err, retval;

drivers/scsi/device_handler/scsi_dh_alua.c +0 -1

-	bool rtpg_ext_hdr_req = 1;

  unsigned long expiry, interval = 0;
  unsigned int tpg_desc_tbl_off;
  unsigned char orig_transition_tmo;

drivers/scsi/device_handler/scsi_dh_alua.c +0 -0

@@ -501,7 +500,7 @@ static int alua_rtpg(struct scsi_device *sdev,

struct alua_dh_data *h, int wait_
  expiry = round_jiffies_up(jiffies + h-

Quoted 1 line

transition_tmo * HZ);

  retry:

drivers/scsi/device_handler/scsi_dh_alua.c +1 -1

-	retval = submit_rtpg(sdev, h, rtpg_ext_hdr_req);
+	retval = submit_rtpg(sdev, h);

  if (retval) {
  if (!scsi_normalize_sense(h->sense, SCSI_SENSE_BUFFERSIZE,
    &sense_hdr)) {

drivers/scsi/device_handler/scsi_dh_alua.c +0 -0

@@ -521,10 +520,10 @@ static int alua_rtpg(struct scsi_device *sdev,

struct alua_dh_data *h, int wait_
   * The retry without rtpg_ext_hdr_req set
   * handles this.
   */

drivers/scsi/device_handler/scsi_dh_alua.c +1 -1

-		if (rtpg_ext_hdr_req == 1 &&
+		if (!(h->flags & ALUA_RTPG_EXT_HDR_UNSUPP) &&

      sense_hdr.sense_key == ILLEGAL_REQUEST && sense_hdr.asc == 0x24 && sense_hdr.ascq == 0) {

drivers/scsi/device_handler/scsi_dh_alua.c +1 -1

-			rtpg_ext_hdr_req = 0;
+			h->flags |= ALUA_RTPG_EXT_HDR_UNSUPP;

  goto retry;
  }

Use 'get_unaligned_XX' and 'put_unaligned_XX' instead of open-coding it.

Reviewed-by Martin K. Petersen <[email protected]>

Reviewed-by Christoph Hellwig <[email protected]>

Signed-off-by Hannes Reinecke <[email protected]>

---
 drivers/scsi/device_handler/scsi_dh_alua.c | 23 ++++++++----------
-----

 1 file changed, 8 insertions(+), 15 deletions(-)

drivers/scsi/device_handler/scsi_dh_alua.c +1 -0

index 03f8190..d54f42e 100644
@@ -22,6 +22,7 @@
  #include <linux/slab.h>
  #include <linux/delay.h>
  #include <linux/module.h>
+#include <asm/unaligned.h>
  #include <scsi/scsi.h>
  #include <scsi/scsi_dbg.h>
  #include <scsi/scsi_eh.h>

@@ -152,10 +153,7 @@ static unsigned submit_rtpg(struct scsi_device
 *sdev, struct alua_dh_data *h)

  rq->cmd[1] = MI_REPORT_TARGET_PGS | MI_EXT_HDR_PARAM_FMT;
  else
  rq->cmd[1] = MI_REPORT_TARGET_PGS;

drivers/scsi/device_handler/scsi_dh_alua.c +1 -4

-	rq->cmd[6] = (h->bufflen >> 24) & 0xff;
-	rq->cmd[7] = (h->bufflen >> 16) & 0xff;
-	rq->cmd[8] = (h->bufflen >>  8) & 0xff;
-	rq->cmd[9] = h->bufflen & 0xff;
+	put_unaligned_be32(h->bufflen, &rq->cmd[6]);

  rq->cmd_len = COMMAND_SIZE(MAINTENANCE_IN);
  rq->sense = h->sense;

drivers/scsi/device_handler/scsi_dh_alua.c +1 -2

@@ -236,8 +234,7 @@ static unsigned submit_stpg(struct alua_dh_data
 *h)
  	/* Prepare the data buffer */
  	memset(h->buff, 0, stpg_len);
  	h->buff[4] = TPGS_STATE_OPTIMIZED & 0x0f;
-	h->buff[6] = (h->group_id >> 8) & 0xff;
-	h->buff[7] = h->group_id & 0xff;
+	put_unaligned_be16(h->group_id, &h->buff[6]);

  rq = get_alua_req(sdev, h->buff, stpg_len, WRITE); if (!rq)

drivers/scsi/device_handler/scsi_dh_alua.c +1 -4

@@ -246,10 +243,7 @@ static unsigned submit_stpg(struct alua_dh_data
 *h)
  	/* Prepare the command. */
  	rq->cmd[0] = MAINTENANCE_OUT;
  	rq->cmd[1] = MO_SET_TARGET_PGS;
-	rq->cmd[6] = (stpg_len >> 24) & 0xff;
-	rq->cmd[7] = (stpg_len >> 16) & 0xff;
-	rq->cmd[8] = (stpg_len >>  8) & 0xff;
-	rq->cmd[9] = stpg_len & 0xff;
+	put_unaligned_be32(stpg_len, &rq->cmd[6]);
  	rq->cmd_len = COMMAND_SIZE(MAINTENANCE_OUT);

  rq->sense = h->sense;

drivers/scsi/device_handler/scsi_dh_alua.c +0 -0

@@ -343,11 +337,11 @@ static int alua_check_vpd(struct scsi_device
 *sdev, struct alua_dh_data *h)
  		switch (d[1] & 0xf) {

  case 0x4:
  /* Relative target port */

drivers/scsi/device_handler/scsi_dh_alua.c +1 -1

-			h->rel_port = (d[6] << 8) + d[7];
+			h->rel_port = get_unaligned_be16(&d[6]);

  break;
  case 0x5:
  /* Target port group */

drivers/scsi/device_handler/scsi_dh_alua.c +1 -1

-			h->group_id = (d[6] << 8) + d[7];
+			h->group_id = get_unaligned_be16(&d[6]);

  break;
  default:
  break;

drivers/scsi/device_handler/scsi_dh_alua.c +0 -0

@@ -540,8 +534,7 @@ static int alua_rtpg(struct scsi_device *sdev,

struct alua_dh_data *h, int wait_
  return SCSI_DH_IO;
  }

drivers/scsi/device_handler/scsi_dh_alua.c +1 -2

-	len = (h->buff[0] << 24) + (h->buff[1] << 16) +
-		(h->buff[2] << 8) + h->buff[3] + 4;
+	len = get_unaligned_be32(&h->buff[0]) + 4;

  if (len > h->bufflen) {
  /* Resubmit with the correct length */

drivers/scsi/device_handler/scsi_dh_alua.c +0 -0

@@ -576,7 +569,7 @@ static int alua_rtpg(struct scsi_device *sdev,

struct alua_dh_data *h, int wait_
       k < len;
       k += off, ucp += off) {

drivers/scsi/device_handler/scsi_dh_alua.c +1 -1

-		if (h->group_id == (ucp[2] << 8) + ucp[3]) {
+		if (h->group_id == get_unaligned_be16(&ucp[2])) {

  h->state = ucp[0] & 0x0f;
  h->pref = ucp[0] >> 7;
  valid_states = ucp[1];

Instead of returning an error code in alua_check_tpgs() we should rather return the tpgs mode directly and have a cleaner syntax.

Signed-off-by Hannes Reinecke <[email protected]>

---
 drivers/scsi/device_handler/scsi_dh_alua.c | 25 +++++++++++---------
-----

 1 file changed, 11 insertions(+), 14 deletions(-)

drivers/scsi/device_handler/scsi_dh_alua.c +2 -2

index d54f42e..c9751c9 100644
@@ -262,24 +262,23 @@ static unsigned submit_stpg(struct alua_dh_data
 *h)
   * Examine the TPGS setting of the sdev to find out if ALUA
   * is supported.
   */
-static int alua_check_tpgs(struct scsi_device *sdev, struct
 alua_dh_data *h)
+static int alua_check_tpgs(struct scsi_device *sdev)
  {
-	int err = SCSI_DH_OK;
+	int tpgs = TPGS_MODE_NONE;

  /*
   * ALUA support for non-disk devices is fraught with
   * difficulties, so disable it for now.
   */
  if (sdev->type != TYPE_DISK) {

drivers/scsi/device_handler/scsi_dh_alua.c +0 -1

-		h->tpgs = TPGS_MODE_NONE;

  sdev_printk(KERN_INFO, sdev,
      "%s: disable for non-disk devices\n", ALUA_DH_NAME);

drivers/scsi/device_handler/scsi_dh_alua.c +1 -1

-		return SCSI_DH_DEV_UNSUPP;
+		return tpgs;

  }

drivers/scsi/device_handler/scsi_dh_alua.c +2 -2

-	h->tpgs = scsi_device_tpgs(sdev);
-	switch (h->tpgs) {
+	tpgs = scsi_device_tpgs(sdev);
+	switch (tpgs) {

  case TPGS_MODE_EXPLICIT|TPGS_MODE_IMPLICIT:
  sdev_printk(KERN_INFO, sdev,
      "%s: supports implicit and explicit TPGS\n",

drivers/scsi/device_handler/scsi_dh_alua.c +0 -0

@@ -296,18 +295,16 @@ static int alua_check_tpgs(struct scsi_device
 *sdev, struct alua_dh_data *h)

  case TPGS_MODE_NONE:
  sdev_printk(KERN_INFO, sdev, "%s: not supported\n", ALUA_DH_NAME);

drivers/scsi/device_handler/scsi_dh_alua.c +0 -1

-		err = SCSI_DH_DEV_UNSUPP;

  break;
  default:
  sdev_printk(KERN_INFO, sdev,
      "%s: unsupported TPGS setting %d\n",

drivers/scsi/device_handler/scsi_dh_alua.c +2 -3

-			    ALUA_DH_NAME, h->tpgs);
-		h->tpgs = TPGS_MODE_NONE;
-		err = SCSI_DH_DEV_UNSUPP;
+			    ALUA_DH_NAME, tpgs);
+		tpgs = TPGS_MODE_NONE;

  break;
  }

drivers/scsi/device_handler/scsi_dh_alua.c +1 -1

-	return err;
+	return tpgs;

 }

 /*

drivers/scsi/device_handler/scsi_dh_alua.c +0 -0

@@ -627,10 +624,10 @@ static int alua_rtpg(struct scsi_device *sdev,

struct alua_dh_data *h, int wait_
  */
 static int alua_initialize(struct scsi_device *sdev, struct alua_dh_data *h)
 {

drivers/scsi/device_handler/scsi_dh_alua.c +1 -1

-	int err;
+	int err = SCSI_DH_DEV_UNSUPP;

drivers/scsi/device_handler/scsi_dh_alua.c +2 -2

-	err = alua_check_tpgs(sdev, h);
-	if (err != SCSI_DH_OK)
+	h->tpgs = alua_check_tpgs(sdev);
+	if (h->tpgs == TPGS_MODE_NONE)

  goto out;

  err = alua_check_vpd(sdev, h);

Most sense code is already handled in the generic code, so we shouldn't be adding special cases here. However, when doing so we need to check for unit attention whenever we're sending an internal command.

Reviewed-by Ewan Milne <[email protected]>

Reviewed-by Christoph Hellwig <[email protected]>

Signed-off-by Hannes Reinecke <[email protected]>

---
 drivers/scsi/device_handler/scsi_dh_alua.c | 45 +++++++++++---------
----------

 1 file changed, 17 insertions(+), 28 deletions(-)

drivers/scsi/device_handler/scsi_dh_alua.c +0 -0

index c9751c9..abf05b4 100644
@@ -85,7 +85,6 @@ struct alua_dh_data {
  #define ALUA_POLICY_SWITCH_ALL		1

 static char print_alua_state(int);
-static int alua_check_sense(struct scsi_device *, struct scsi_sense_hdr *);

 static int realloc_buffer(struct alua_dh_data *h, unsigned len) {

drivers/scsi/device_handler/scsi_dh_alua.c +0 -0

@@ -189,8 +188,13 @@ static void stpg_endio(struct request *req, int
 error)

  if (scsi_normalize_sense(h->sense, SCSI_SENSE_BUFFERSIZE, &sense_hdr)) {

drivers/scsi/device_handler/scsi_dh_alua.c +2 -2

-		err = alua_check_sense(h->sdev, &sense_hdr);
-		if (err == ADD_TO_MLQUEUE) {
+		if (sense_hdr.sense_key == NOT_READY &&
+		    sense_hdr.asc == 0x04 && sense_hdr.ascq == 0x0a)

{

drivers/scsi/device_handler/scsi_dh_alua.c +1 -0

+			/* ALUA state transition already in progress

*/

drivers/scsi/device_handler/scsi_dh_alua.c +4 -0

+			err = SCSI_DH_OK;
+			goto done;
+		}
+		if (sense_hdr.sense_key == UNIT_ATTENTION) {

  err = SCSI_DH_RETRY; goto done;
  }

drivers/scsi/device_handler/scsi_dh_alua.c +0 -0

@@ -399,28 +403,6 @@ static int alua_check_sense(struct scsi_device
 *sdev,
  			 * LUN Not Accessible - ALUA state

transition
   */
  return ADD_TO_MLQUEUE;

drivers/scsi/device_handler/scsi_dh_alua.c +0 -1

-		if (sense_hdr->asc == 0x04 && sense_hdr->ascq ==

0x0b)

drivers/scsi/device_handler/scsi_dh_alua.c +0 -2

-			/*
-			 * LUN Not Accessible -- Target port in

standby state

drivers/scsi/device_handler/scsi_dh_alua.c +0 -3

-			 */
-			return SUCCESS;
-		if (sense_hdr->asc == 0x04 && sense_hdr->ascq ==

0x0c)

drivers/scsi/device_handler/scsi_dh_alua.c +0 -2

-			/*
-			 * LUN Not Accessible -- Target port in

unavailable state

drivers/scsi/device_handler/scsi_dh_alua.c +0 -3

-			 */
-			return SUCCESS;
-		if (sense_hdr->asc == 0x04 && sense_hdr->ascq ==

0x12)

drivers/scsi/device_handler/scsi_dh_alua.c +0 -6

-			/*
-			 * LUN Not Ready -- Offline
-			 */
-			return SUCCESS;
-		if (sdev->allow_restart &&
-		    sense_hdr->asc == 0x04 && sense_hdr->ascq ==

0x02)

drivers/scsi/device_handler/scsi_dh_alua.c +0 -2

-			/*
-			 * if the device is not started, we need to

wake

drivers/scsi/device_handler/scsi_dh_alua.c +0 -3

-			 * the error handler to start the motor
-			 */
-			return FAILED;

  break;
  case UNIT_ATTENTION:
  if (sense_hdr->asc == 0x29 && sense_hdr->ascq == 0x00)

drivers/scsi/device_handler/scsi_dh_alua.c +0 -0

@@ -517,9 +499,16 @@ static int alua_rtpg(struct scsi_device *sdev,

struct alua_dh_data *h, int wait_
  h->flags |= ALUA_RTPG_EXT_HDR_UNSUPP; goto retry;
  }

drivers/scsi/device_handler/scsi_dh_alua.c +0 -3

-
-		err = alua_check_sense(sdev, &sense_hdr);
-		if (err == ADD_TO_MLQUEUE && time_before(jiffies,

expiry)) {

drivers/scsi/device_handler/scsi_dh_alua.c +10 -0

+		/*
+		 * Retry on ALUA state transition or if any
+		 * UNIT ATTENTION occurred.
+		 */
+		if (sense_hdr.sense_key == NOT_READY &&
+		    sense_hdr.asc == 0x04 && sense_hdr.ascq == 0x0a)
+			err = SCSI_DH_RETRY;
+		else if (sense_hdr.sense_key == UNIT_ATTENTION)
+			err = SCSI_DH_RETRY;
+		if (err == SCSI_DH_RETRY && time_before(jiffies,

expiry)) {
  sdev_printk(KERN_ERR, sdev, "%s: rtpg retry\n",
      ALUA_DH_NAME); scsi_print_sense_hdr(sdev, ALUA_DH_NAME, &sense_hdr);

Add a function scsi_vpd_lun_id() to return a unique device identifcation based on the designation descriptors of VPD page 0x83.

As devices might implement several descriptors the order of preference is:
- NAA IEE Registered Extended
- EUI-64 based 16-byte
- EUI-64 based 12-byte
- NAA IEEE Registered
- NAA IEEE Extended
A SCSI name string descriptor is preferred to all of them if the identification is longer than 16 bytes.

The returned unique device identification will be formatted as a SCSI Name string to avoid clashes between different designator types.

Reviewed-by Ewan Milne <[email protected]>

Reviewed-by Christoph Hellwig <[email protected]>

Signed-off-by Hannes Reinecke <[email protected]>

---
 drivers/scsi/scsi_lib.c    | 140
+++++++++++++++++++++++++++++++++++++++++++++
 include/scsi/scsi_device.h |   1 +

 2 files changed, 141 insertions(+)

drivers/scsi/scsi_lib.c +0 -0

index cbfc599..3cb295c 100644
@@ -3154,3 +3154,143 @@ void sdev_enable_disk_events(struct
 scsi_device *sdev)
  	atomic_dec(&sdev->disk_events_disable_depth);
  }

 EXPORT_SYMBOL(sdev_enable_disk_events);

drivers/scsi/scsi_lib.c +16 -0

+
+/*
+ * scsi_vpd_lun_id - return a unique device identification
+ * @sdev: SCSI device
+ * @id:   buffer for the identification
+ * @id_len:  length of the buffer
+ *
+ * Copies a unique device identification into @id based
+ * on the information in the VPD page 0x83 of the device.
+ * The string will be formatted as a SCSI name string.
+ *
+ * Returns the length of the identification or error on failure.
+ * If the identifier is longer than the supplied buffer the actual
+ * identifier length is returned and the buffer is not zero-padded.
+ */
+int scsi_vpd_lun_id(struct scsi_device *sdev, char *id, size_t

id_len)

drivers/scsi/scsi_lib.c +28 -0

+{
+	u8 cur_id_type = 0xff;
+	u8 cur_id_size = 0;
+	unsigned char *d, *cur_id_str;
+	unsigned char __rcu *vpd_pg83;
+	int id_size = -EINVAL;
+
+	rcu_read_lock();
+	vpd_pg83 = rcu_dereference(sdev->vpd_pg83);
+	if (!vpd_pg83) {
+		rcu_read_unlock();
+		return -ENXIO;
+	}
+
+	/*
+	 * Look for the correct descriptor.
+	 * Order of preference for lun descriptor:
+	 * - SCSI name string
+	 * - NAA IEEE Registered Extended
+	 * - EUI-64 based 16-byte
+	 * - EUI-64 based 12-byte
+	 * - NAA IEEE Registered
+	 * - NAA IEEE Extended
+	 * as longer descriptors reduce the likelyhood
+	 * of identification clashes.
+	 */
+
+	/* The id string must be at least 20 bytes + terminating

NULL byte */

drivers/scsi/scsi_lib.c +82 -0

+	if (id_len < 21) {
+		rcu_read_unlock();
+		return -EINVAL;
+	}
+
+	memset(id, 0, id_len);
+	d = vpd_pg83 + 4;
+	while (d < vpd_pg83 + sdev->vpd_pg83_len) {
+		/* Skip designators not referring to the LUN */
+		if ((d[1] & 0x30) != 0x00)
+			goto next_desig;
+
+		switch (d[1] & 0xf) {
+		case 0x2:
+			/* EUI-64 */
+			if (cur_id_size > d[3])
+				break;
+			/* Prefer NAA IEEE Registered Extended */
+			if (cur_id_type == 0x3 &&
+			    cur_id_size == d[3])
+				break;
+			cur_id_size = d[3];
+			cur_id_str = d + 4;
+			cur_id_type = d[1] & 0xf;
+			switch (cur_id_size) {
+			case 8:
+				id_size = snprintf(id, id_len,
+						   "eui.%8phN",
+						   cur_id_str);
+				break;
+			case 12:
+				id_size = snprintf(id, id_len,
+						   "eui.%12phN",
+						   cur_id_str);
+				break;
+			case 16:
+				id_size = snprintf(id, id_len,
+						   "eui.%16phN",
+						   cur_id_str);
+				break;
+			default:
+				cur_id_size = 0;
+				break;
+			}
+			break;
+		case 0x3:
+			/* NAA */
+			if (cur_id_size > d[3])
+				break;
+			cur_id_size = d[3];
+			cur_id_str = d + 4;
+			cur_id_type = d[1] & 0xf;
+			switch (cur_id_size) {
+			case 8:
+				id_size = snprintf(id, id_len,
+						   "naa.%8phN",
+						   cur_id_str);
+				break;
+			case 16:
+				id_size = snprintf(id, id_len,
+						   "naa.%16phN",
+						   cur_id_str);
+				break;
+			default:
+				cur_id_size = 0;
+				break;
+			}
+			break;
+		case 0x8:
+			/* SCSI name string */
+			if (cur_id_size + 4 > d[3])
+				break;
+			/* Prefer others for truncated descriptor */
+			if (cur_id_size && d[3] > id_len)
+				break;
+			cur_id_size = id_size = d[3];
+			cur_id_str = d + 4;
+			cur_id_type = d[1] & 0xf;
+			if (cur_id_size >= id_len)
+				cur_id_size = id_len - 1;
+			memcpy(id, cur_id_str, cur_id_size);
+			/* Decrease priority for truncated

descriptor */

drivers/scsi/scsi_lib.c +14 -0

+			if (cur_id_size != id_size)
+				cur_id_size = 6;
+			break;
+		default:
+			break;
+		}
+next_desig:
+		d += d[3] + 4;
+	}
+	rcu_read_unlock();
+
+	return id_size;
+}
+EXPORT_SYMBOL(scsi_vpd_lun_id);

include/scsi/scsi_device.h +1 -0

index bde4077..4c49cfa 100644
@@ -415,6 +415,7 @@ static inline int scsi_execute_req(struct
 scsi_device *sdev,
  }
  extern void sdev_disable_disk_events(struct scsi_device *sdev);
  extern void sdev_enable_disk_events(struct scsi_device *sdev);
+extern int scsi_vpd_lun_id(struct scsi_device *, char *, size_t);

 #ifdef CONFIG_PM
 extern int scsi_autopm_get_device(struct scsi_device *);

Use scsi_vpd_lun_id() to export the device id to sysfs.

Signed-off-by Hannes Reinecke <[email protected]>

---
 drivers/scsi/scsi_sysfs.c | 17 +++++++++++++++++

 1 file changed, 17 insertions(+)

drivers/scsi/scsi_sysfs.c +0 -0

index 7b41b2c..aaa38c2 100644
@@ -903,6 +903,22 @@ sdev_show_function(queue_depth, "%d\n");
  static DEVICE_ATTR(queue_depth, S_IRUGO | S_IWUSR,
 sdev_show_queue_depth,
  		   sdev_store_queue_depth);

drivers/scsi/scsi_sysfs.c +2 -0

+static ssize_t
+sdev_show_device_id(struct device *dev, struct device_attribute

*attr,

drivers/scsi/scsi_sysfs.c +14 -0

+		    char *buf)
+{
+	struct scsi_device *sdev = to_scsi_device(dev);
+	ssize_t count;
+
+	count = scsi_vpd_lun_id(sdev, buf, PAGE_SIZE);
+	if (count > 0) {
+		buf[count] = '\n';
+		count++;
+	}
+	return count;
+}
+static DEVICE_ATTR(device_id, S_IRUGO, sdev_show_device_id, NULL);
+

 #ifdef CONFIG_SCSI_DH
 static ssize_t
 sdev_show_dh_state(struct device *dev, struct device_attribute *attr,

Implement scsi_vpd_tpg_id() to extract the target
port group id and the relative port id from
SCSI VPD page 0x83.

Signed-off-by Hannes Reinecke <[email protected]>

---
 drivers/scsi/scsi_lib.c    | 48
++++++++++++++++++++++++++++++++++++++++++++++
 include/scsi/scsi_device.h |  1 +

 2 files changed, 49 insertions(+)

drivers/scsi/scsi_lib.c +1 -0

index 3cb295c..d12edc8 100644
@@ -23,6 +23,7 @@
  #include <linux/scatterlist.h>
  #include <linux/blk-mq.h>
  #include <linux/ratelimit.h>
+#include <asm/unaligned.h>

 #include <scsi/scsi.h>
 #include <scsi/scsi_cmnd.h>

drivers/scsi/scsi_lib.c +47 -0

@@ -3294,3 +3295,50 @@ next_desig:
  	return id_size;
  }
  EXPORT_SYMBOL(scsi_vpd_lun_id);
+
+/*
+ * scsi_vpd_tpg_id - return a target port group identifier
+ * @sdev: SCSI device
+ *
+ * Returns the Target Port Group identifier from the information
+ * froom VPD page 0x83 of the device.
+ *
+ * Returns the identifier or error on failure.
+ */
+int scsi_vpd_tpg_id(struct scsi_device *sdev, int *rel_id)
+{
+	unsigned char *d;
+	unsigned char __rcu *vpd_pg83;
+	int group_id = -EAGAIN, rel_port = -1;
+
+	rcu_read_lock();
+	vpd_pg83 = rcu_dereference(sdev->vpd_pg83);
+	if (!vpd_pg83) {
+		rcu_read_unlock();
+		return -ENXIO;
+	}
+
+	d = sdev->vpd_pg83 + 4;
+	while (d < sdev->vpd_pg83 + sdev->vpd_pg83_len) {
+		switch (d[1] & 0xf) {
+		case 0x4:
+			/* Relative target port */
+			rel_port = get_unaligned_be16(&d[6]);
+			break;
+		case 0x5:
+			/* Target port group */
+			group_id = get_unaligned_be16(&d[6]);
+			break;
+		default:
+			break;
+		}
+		d += d[3] + 4;
+	}
+	rcu_read_unlock();
+
+	if (group_id >= 0 && rel_id && rel_port != -1)
+		*rel_id = rel_port;
+
+	return group_id;
+}
+EXPORT_SYMBOL(scsi_vpd_tpg_id);

include/scsi/scsi_device.h +1 -0

index 4c49cfa..f63a167 100644
@@ -416,6 +416,7 @@ static inline int scsi_execute_req(struct
 scsi_device *sdev,
  extern void sdev_disable_disk_events(struct scsi_device *sdev);
  extern void sdev_enable_disk_events(struct scsi_device *sdev);
  extern int scsi_vpd_lun_id(struct scsi_device *, char *, size_t);
+extern int scsi_vpd_tpg_id(struct scsi_device *, int *);

 #ifdef CONFIG_PM
 extern int scsi_autopm_get_device(struct scsi_device *);

Signed-off-by Hannes Reinecke <[email protected]>

---
 drivers/scsi/device_handler/scsi_dh_alua.c | 39 +++++---------------
----------

 1 file changed, 6 insertions(+), 33 deletions(-)

drivers/scsi/device_handler/scsi_dh_alua.c +0 -0

index abf05b4..20fe981 100644
@@ -320,38 +320,10 @@ static int alua_check_tpgs(struct scsi_device
 *sdev)
   */

 static int alua_check_vpd(struct scsi_device *sdev, struct alua_dh_data *h)
 {

drivers/scsi/device_handler/scsi_dh_alua.c +1 -2

-	unsigned char *d;
-	unsigned char __rcu *vpd_pg83;
+	int rel_port = -1, group_id;

drivers/scsi/device_handler/scsi_dh_alua.c +2 -29

-	rcu_read_lock();
-	if (!rcu_dereference(sdev->vpd_pg83)){
-		rcu_read_unlock();
-		return SCSI_DH_DEV_UNSUPP;
-	}
-
-	/*
-	 * Look for the correct descriptor.
-	 */
-	vpd_pg83 = rcu_dereference(sdev->vpd_pg83);
-	d = vpd_pg83 + 4;
-	while (d < vpd_pg83 + sdev->vpd_pg83_len) {
-		switch (d[1] & 0xf) {
-		case 0x4:
-			/* Relative target port */
-			h->rel_port = get_unaligned_be16(&d[6]);
-			break;
-		case 0x5:
-			/* Target port group */
-			h->group_id = get_unaligned_be16(&d[6]);
-			break;
-		default:
-			break;
-		}
-		d += d[3] + 4;
-	}
-	rcu_read_unlock();
-
-	if (h->group_id == -1) {
+	group_id = scsi_vpd_tpg_id(sdev, &rel_port);
+	if (group_id < 0) {

  /*
   * Internal error; TPGS supported but required
   * VPD identification descriptors not present.

drivers/scsi/device_handler/scsi_dh_alua.c +0 -0

@@ -360,10 +332,11 @@ static int alua_check_vpd(struct scsi_device
 *sdev, struct alua_dh_data *h)
  		sdev_printk(KERN_INFO, sdev,

      "%s: No target port descriptors found\n",
      ALUA_DH_NAME);

drivers/scsi/device_handler/scsi_dh_alua.c +0 -2

-		h->state = TPGS_STATE_OPTIMIZED;
-		h->tpgs = TPGS_MODE_NONE;

  return SCSI_DH_DEV_UNSUPP;
  }

drivers/scsi/device_handler/scsi_dh_alua.c +3 -0

+	h->rel_port = rel_port;
+	h->group_id = group_id;
+

  sdev_printk(KERN_INFO, sdev,
      "%s: port group %02x rel port %02x\n", ALUA_DH_NAME, h->group_id, h->rel_port);

Can you push out a git branch with this code for easier review as well?

Hi all,

here's the first part of my ALUA device handler update. It's mainly bugfixes and minor improvements; the two important things are the addition of VPD parsing functions scsi_vpd_lun_id() and scsi_vpd_tpg_id().
This series has been split off from the original 'Asynchronous ALUA' patchset, as these bits are pretty uncontroversial and have a good chance of being merged reasonably soon.

Quoted 9 lines · Hannes Reinecke

Hi all,

here's the first part of my ALUA device handler update. It's mainly bugfixes and minor improvements; the two important things are the addition of VPD parsing functions scsi_vpd_lun_id() and scsi_vpd_tpg_id().
This series has been split off from the original 'Asynchronous ALUA' patchset, as these bits are pretty uncontroversial and have a good chance of being merged reasonably soon.

I've already reviewed most patches, but the other ones looks fine as well:

Reviewed-by Christoph Hellwig <[email protected]>

Let's get this into the 4.5 queue ASAP an then tackle the actually interesting bits next!

One thing, though: I don't really agree with Barts objection that moving to a workqueue would tie in too many resources. Thing is, I'm not convinces that using a work queue is allocating too many resources (we're speaking of 460 vs 240 bytes here). Also we have to retry commands for quite some time (cite the infamous NetApp takeover/giveback, which can take minutes). If we were to handle that without workqueue we'd have to initiate the retry from the end_io callback, causing a quite deep stack recursion. Which I'm not really fond of.

But if anyone has a better idea on how to handle retries without the need for workqueues I'm all ears :-)

Quoted 10 lines · Hannes Reinecke

One thing, though: I don't really agree with Barts objection that moving to a workqueue would tie in too many resources. Thing is, I'm not convinces that using a work queue is allocating too many resources (we're speaking of 460 vs 240 bytes here). Also we have to retry commands for quite some time (cite the infamous NetApp takeover/giveback, which can take minutes). If we were to handle that without workqueue we'd have to initiate the retry from the end_io callback, causing a quite deep stack recursion. Which I'm not really fond of.

Quoted 2 lines

But if anyone has a better idea on how to handle retries without the need for workqueues I'm all ears :-)

I tend to agree with you, but you better warm up that discussion again on the old thread that actually has Bart on Cc. Or just resend once Martin has merged this patch, and have discussion around that version.

One thing, though: I don't really agree with Barts objection that moving to a workqueue would tie in too many resources. Thing is, I'm not convinces that using a work queue is allocating too many resources (we're speaking of 460 vs 240 bytes here). Also we have to retry commands for quite some time (cite the infamous NetApp takeover/giveback, which can take minutes). If we were to handle that without workqueue we'd have to initiate the retry from the end_io callback, causing a quite deep stack recursion. Which I'm not really fond of.

Quoted 9 lines · Hannes Reinecke

One thing, though: I don't really agree with Barts objection that moving to a workqueue would tie in too many resources. Thing is, I'm not convinces that using a work queue is allocating too many resources (we're speaking of 460 vs 240 bytes here). Also we have to retry commands for quite some time (cite the infamous NetApp takeover/giveback, which can take minutes). If we were to handle that without workqueue we'd have to initiate the retry from the end_io callback, causing a quite deep stack recursion. Which I'm not really fond of.

Hello Hannes,

Sorry if I wasn't clear enough in my previous e-mail about this topic but I'm more concerned about the additional memory needed for thread stacks and thread control data structures than about the additional memory needed for the workqueue. I'd like to see the ALUA device handler implementation scale to thousands of LUNs and target port groups. In case all connections between an initiator and a target port group fail, with a synchronous implementation of STPG we will either need a large number of threads (in case of one thread per STPG command) or the STPG commands will be serialized (if there are fewer threads than portal groups). Neither alternative looks attractive to me.

BTW, not all storage arrays need STPG retries. Some arrays are able to process an STPG command quickly (this means within a few seconds).

A previous discussion about this topic is available e.g. at
http://thread.gmane.org/gmane.linux.scsi/105340/focus=105601.

Quoted 25 lines · Bart Van Assche

Quoted 9 lines · Hannes Reinecke

One thing, though: I don't really agree with Barts objection that moving to a workqueue would tie in too many resources. Thing is, I'm not convinces that using a work queue is allocating too many resources (we're speaking of 460 vs 240 bytes here). Also we have to retry commands for quite some time (cite the infamous NetApp takeover/giveback, which can take minutes). If we were to handle that without workqueue we'd have to initiate the retry from the end_io callback, causing a quite deep stack recursion. Which I'm not really fond of.

Hello Hannes,

Sorry if I wasn't clear enough in my previous e-mail about this topic but I'm more concerned about the additional memory needed for thread stacks and thread control data structures than about the additional memory needed for the workqueue. I'd like to see the ALUA device handler implementation scale to thousands of LUNs and target port groups. In case all connections between an initiator and a target port group fail, with a synchronous implementation of STPG we will either need a large number of threads (in case of one thread per STPG command) or the STPG commands will be serialized (if there are fewer threads than portal groups). Neither alternative looks attractive to me.

Quoted 6 lines

BTW, not all storage arrays need STPG retries. Some arrays are able to process an STPG command quickly (this means within a few seconds).

A previous discussion about this topic is available e.g. at
http://thread.gmane.org/gmane.linux.scsi/105340/focus=105601.

Well, one could argue that the whole point of this patchset is to allow you to serialize STPGs :-)

We definitely need to serialize STPGs for the same target port group; the current implementation is far too limited to take that into account.

But the main problem I'm facing with the current implementation is that we cannot handle retries. An RTPG or an STPG might fail, at which point we need to re-run RTPG to figure out the current status. (We also need to send RTPGs when we receive an "ALUA state changed"
UA, but that's slightly beside the point).
The retry cannot be send directly, as we're evaluating the status from end_io context. So to instantiate a retry we need to move it over to a workqueue.