[PATCH 10/10] scsi_dh: move 'dh_state' sysfs attribute to generic code

[PATCH 10/10] scsi_dh: move 'dh_state' sysfs attribute to generic code

[Hannes Reinecke]

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

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/scsi_dh.c    | 65 -----------------------------------------------
 drivers/scsi/scsi_sysfs.c | 60 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 60 insertions(+), 65 deletions(-)

diff --git a/drivers/scsi/scsi_dh.c b/drivers/scsi/scsi_dh.c
index a9494f3..42f20f1 100644
--- a/drivers/scsi/scsi_dh.c
+++ b/drivers/scsi/scsi_dh.c
@@ -153,76 +153,12 @@ static void scsi_dh_handler_detach(struct scsi_device *sdev)
 	module_put(sdev->handler->module);
 }
 
-/*
- * 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
-		 */
-		if (!(scsi_dh = scsi_dh_lookup(buf)))
-			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);
-			else
-				err = 0;
-		}
-	}
-
-	return err<0?err:count;
-}
-
-static ssize_t
-show_dh_state(struct device *dev, struct device_attribute *attr, 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 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;
 	int err;
 
-	err = device_create_file(&sdev->sdev_gendev, &scsi_dh_state_attr);
-	if (err)
-		return err;
-
 	drv = scsi_dh_find_driver(sdev);
 	if (drv)
 		devinfo = scsi_dh_lookup(drv);
@@ -235,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);
 }
 
 /*
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index c8a120f..e3c3b86 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -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>
 
@@ -875,6 +876,62 @@ sdev_show_function(queue_depth, "%d\n");
 static DEVICE_ATTR(queue_depth, S_IRUGO | S_IWUSR, sdev_show_queue_depth,
 		   sdev_store_queue_depth);
 
+#ifdef CONFIG_SCSI_DH
+static ssize_t
+sdev_show_dh_state(struct device *dev, struct device_attribute *attr,
+		   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,
+		    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, "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);
+			else
+				err = 0;
+		}
+	}
+
+	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,
@@ -944,6 +1001,9 @@ static struct attribute *scsi_sdev_attrs[] = {
 	&dev_attr_modalias.attr,
 	&dev_attr_queue_depth.attr,
 	&dev_attr_queue_type.attr,
+#ifdef CONFIG_SCSI_DH
+	&dev_attr_dh_state.attr,
+#endif
 	&dev_attr_queue_ramp_up_period.attr,
 	REF_EVT(media_change),
 	REF_EVT(inquiry_change_reported),
-- 
1.8.5.2

Re: [PATCH 10/10] scsi_dh: move 'dh_state' sysfs attribute to generic code

[Christoph Hellwig]

On Wed, Jul 08, 2015 at 11:07:05AM +0200, Hannes Reinecke wrote:
> As scsi_dh.c is now always compiled in we should be moving
> the 'dh_state' attribute to the generic code.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>

Looks fine, but a few coding style nitpicks:

> +	if (!sdev->handler) {
> +		/*
> +		 * Attach to a device handler
> +		 */
> +		err = scsi_dh_attach(sdev->request_queue, buf);
> +	} else {
> +		if (!strncmp(buf, "detach", 6)) {

Switch to en else if here.  Also mayべe move the activate case up as it's
relevant while detach is dead now.

> +	return err<0?err:count;

Please add the missing spaces here.
--
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 10/10] scsi_dh: move 'dh_state' sysfs attribute to generic code

[Hannes Reinecke]

On 07/09/2015 10:24 AM, Christoph Hellwig wrote:
> On Wed, Jul 08, 2015 at 11:07:05AM +0200, Hannes Reinecke wrote:
>> As scsi_dh.c is now always compiled in we should be moving
>> the 'dh_state' attribute to the generic code.
>>
>> Signed-off-by: Hannes Reinecke <hare@suse.de>
> 
> Looks fine, but a few coding style nitpicks:
> 
>> +	if (!sdev->handler) {
>> +		/*
>> +		 * Attach to a device handler
>> +		 */
>> +		err = scsi_dh_attach(sdev->request_queue, buf);
>> +	} else {
>> +		if (!strncmp(buf, "detach", 6)) {
> 
> Switch to en else if here.  Also mayべe move the activate case up as it's
> relevant while detach is dead now.
> 
Okay.

>> +	return err<0?err:count;
> 
> Please add the missing spaces here.
> 
Will be doing so.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
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

[PATCH 00/10] Integrate scsi_dh better into the scsi core V4

[Hannes Reinecke]

This series ties scsi_dh deeper into the scsi core, and fixes all kinds
of issues in it, most importantly the race between using and detaching
device handlers.

Changes since V1:
  - updated comments / strings based on review feedback
  - moved scsi_dh.c to drivers/scsi to fix the srcdir = objdir build
  - changed the old patch 2 to have saner handling of mismatching device
    handlers in dm.
  - dropped patch 1: not having a hw handler is fine if we don't plan to
    change it anyway
  - dropped patch 3: not necessary anymore.
Changes since V2:
  - Fixup issues during attaching device_handler from dm-multipath
  - Add patch to clarify scsi_dh_activate() error codes
Changes since V3:
  - Fixup minor bugs discovered during testing
  - Fixup patch description

Christoph Hellwig (8):
  dm-mpath, scsi_dh: don't let dm detach device handlers
  dm-mpath, scsi_dh: request scsi_dh modules in scsi_dh, not dm-mpath
  scsi_dh: move to drivers/scsi
  scsi_dh: integrate into the core SCSI code
  scsi_dh: move device matching to the core code
  scsi_dh: kill struct scsi_dh_data
  scsi_dh: add a common helper to get a scsi_device from a request_queue
  scsi_dh: don't allow to detach device handlers at runtime

Hannes Reinecke (2):
  scsi_dh: return SCSI_DH_NOTCONN in scsi_dh_activate()
  scsi_dh: move 'dh_state' sysfs attribute to generic code

 drivers/md/dm-mpath.c                       |  27 +-
 drivers/scsi/Makefile                       |   1 +
 drivers/scsi/device_handler/Kconfig         |   2 +-
 drivers/scsi/device_handler/Makefile        |   1 -
 drivers/scsi/device_handler/scsi_dh.c       | 621 ----------------------------
 drivers/scsi/device_handler/scsi_dh_alua.c  |  31 +-
 drivers/scsi/device_handler/scsi_dh_emc.c   |  58 +--
 drivers/scsi/device_handler/scsi_dh_hp_sw.c |  55 +--
 drivers/scsi/device_handler/scsi_dh_rdac.c  |  80 +---
 drivers/scsi/scsi_dh.c                      | 371 +++++++++++++++++
 drivers/scsi/scsi_error.c                   |   6 +-
 drivers/scsi/scsi_lib.c                     |   6 +-
 drivers/scsi/scsi_priv.h                    |   9 +
 drivers/scsi/scsi_sysfs.c                   |  68 +++
 include/scsi/scsi_device.h                  |  27 +-
 include/scsi/scsi_dh.h                      |  29 +-
 16 files changed, 527 insertions(+), 865 deletions(-)
 delete mode 100644 drivers/scsi/device_handler/scsi_dh.c
 create mode 100644 drivers/scsi/scsi_dh.c

-- 
1.8.5.6

[PATCH 10/10] scsi_dh: move 'dh_state' sysfs attribute to generic code

[Hannes Reinecke]

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

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/scsi_dh.c    | 67 +----------------------------------------------
 drivers/scsi/scsi_sysfs.c | 58 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 59 insertions(+), 66 deletions(-)

diff --git a/drivers/scsi/scsi_dh.c b/drivers/scsi/scsi_dh.c
index 2d93bc9..1584080 100644
--- a/drivers/scsi/scsi_dh.c
+++ b/drivers/scsi/scsi_dh.c
@@ -153,75 +153,11 @@ static void scsi_dh_handler_detach(struct scsi_device *sdev)
 	module_put(sdev->handler->module);
 }
 
-/*
- * 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
-		 */
-		if (!(scsi_dh = scsi_dh_lookup(buf)))
-			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);
-			else
-				err = 0;
-		}
-	}
-
-	return err<0?err:count;
-}
-
-static ssize_t
-show_dh_state(struct device *dev, struct device_attribute *attr, 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 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;
-	int err;
-
-	err = device_create_file(&sdev->sdev_gendev, &scsi_dh_state_attr);
-	if (err)
-		return err;
+	int err = 0;
 
 	drv = scsi_dh_find_driver(sdev);
 	if (drv)
@@ -235,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);
 }
 
 /*
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index b333389..5d64c3f 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -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>
 
@@ -875,6 +876,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);
 
+#ifdef CONFIG_SCSI_DH
+static ssize_t
+sdev_show_dh_state(struct device *dev, struct device_attribute *attr,
+		   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,
+		    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);
+		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,
@@ -944,6 +999,9 @@ static struct attribute *scsi_sdev_attrs[] = {
 	&dev_attr_modalias.attr,
 	&dev_attr_queue_depth.attr,
 	&dev_attr_queue_type.attr,
+#ifdef CONFIG_SCSI_DH
+	&dev_attr_dh_state.attr,
+#endif
 	&dev_attr_queue_ramp_up_period.attr,
 	REF_EVT(media_change),
 	REF_EVT(inquiry_change_reported),
-- 
1.8.5.6

Re: [PATCH 10/10] scsi_dh: move 'dh_state' sysfs attribute to generic code

[James Bottomley]

On Thu, 2015-08-27 at 14:17 +0200, Hannes Reinecke wrote:
> As scsi_dh.c is now always compiled in we should be moving
> the 'dh_state' attribute to the generic code.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>

This looks OK, but needs another reviewer.  If you could also fix up the
rejections caused by the if (!(scsi_dh = ...)) as well, that would be
great.

It's independent, so I pulled in the rest.

Thanks,

James

Re: [PATCH 10/10] scsi_dh: move 'dh_state' sysfs attribute to generic code

[Christoph Hellwig]

On Thu, Aug 27, 2015 at 02:17:03PM +0200, Hannes Reinecke wrote:
> As scsi_dh.c is now always compiled in we should be moving
> the 'dh_state' attribute to the generic code.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>

Looks good,

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