linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* device handler cleanups
@ 2014-10-19 15:59 Christoph Hellwig
  2014-10-19 15:59 ` [PATCH 1/6] scsi_dh: get module reference outside of device handler Christoph Hellwig
                   ` (5 more replies)
  0 siblings, 6 replies; 17+ messages in thread
From: Christoph Hellwig @ 2014-10-19 15:59 UTC (permalink / raw)
  To: Chandra Seetharaman, Hannes Reinecke, Mike Christie
  Cc: Sean Stewart, Bart Van Assche, linux-kernel

This series contains a couple easy cleanups for the device handler
infrastructure.  I think some more work could be done in the longer run,
e.g. by using a class_device interface to attach to the scsi_device,
similar to how the /dev/sg devices work.


^ permalink raw reply	[flat|nested] 17+ messages in thread
* [PATCH 1/6] scsi_dh: get module reference outside of device handler
  2014-10-19 15:59 device handler cleanups Christoph Hellwig
@ 2014-10-19 15:59 ` Christoph Hellwig
  2014-10-20  6:01   ` Hannes Reinecke
  2014-10-19 16:00 ` [PATCH 2/6] scsi: use container_of to get at device handler private data Christoph Hellwig
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 17+ messages in thread
From: Christoph Hellwig @ 2014-10-19 15:59 UTC (permalink / raw)
  To: Chandra Seetharaman, Hannes Reinecke, Mike Christie
  Cc: Sean Stewart, Bart Van Assche, linux-kernel

We need to grab a reference to the module before calling the attach
routines to avoid a small race vs module removal.  It also cleans up
the code significantly as a side effect.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/scsi/device_handler/scsi_dh.c       | 31 ++++++++++++++++++++---------
 drivers/scsi/device_handler/scsi_dh_alua.c  |  4 ----
 drivers/scsi/device_handler/scsi_dh_emc.c   |  4 ----
 drivers/scsi/device_handler/scsi_dh_hp_sw.c |  4 ----
 drivers/scsi/device_handler/scsi_dh_rdac.c  |  4 ----
 5 files changed, 22 insertions(+), 25 deletions(-)

diff --git a/drivers/scsi/device_handler/scsi_dh.c b/drivers/scsi/device_handler/scsi_dh.c
index 33e422e..1a8dbf3 100644
--- a/drivers/scsi/device_handler/scsi_dh.c
+++ b/drivers/scsi/device_handler/scsi_dh.c
@@ -102,23 +102,36 @@ static int scsi_dh_handler_attach(struct scsi_device *sdev,
 
 	if (sdev->scsi_dh_data) {
 		if (sdev->scsi_dh_data->scsi_dh != scsi_dh)
-			err = -EBUSY;
-		else
-			kref_get(&sdev->scsi_dh_data->kref);
-	} else if (scsi_dh->attach) {
+			return -EBUSY;
+
+		kref_get(&sdev->scsi_dh_data->kref);
+		return 0;
+	}
+
+	if (scsi_dh->attach) {
+		if (!try_module_get(scsi_dh->module))
+			return -EINVAL;
+
 		err = scsi_dh->attach(sdev);
-		if (!err) {
-			kref_init(&sdev->scsi_dh_data->kref);
-			sdev->scsi_dh_data->sdev = sdev;
+		if (err) {
+			module_put(scsi_dh->module);
+			return err;
 		}
+
+		kref_init(&sdev->scsi_dh_data->kref);
+		sdev->scsi_dh_data->sdev = sdev;
 	}
 	return err;
 }
 
 static void __detach_handler (struct kref *kref)
 {
-	struct scsi_dh_data *scsi_dh_data = container_of(kref, struct scsi_dh_data, kref);
-	scsi_dh_data->scsi_dh->detach(scsi_dh_data->sdev);
+	struct scsi_dh_data *scsi_dh_data =
+		container_of(kref, struct scsi_dh_data, kref);
+	struct scsi_device_handler *scsi_dh = scsi_dh_data->scsi_dh;
+
+	scsi_dh->detach(scsi_dh_data->sdev);
+	module_put(scsi_dh->module);
 }
 
 /*
diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index e99507e..45b1795 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -866,9 +866,6 @@ static int alua_bus_attach(struct scsi_device *sdev)
 	if ((err != SCSI_DH_OK) && (err != SCSI_DH_DEV_OFFLINED))
 		goto failed;
 
-	if (!try_module_get(THIS_MODULE))
-		goto failed;
-
 	spin_lock_irqsave(sdev->request_queue->queue_lock, flags);
 	sdev->scsi_dh_data = scsi_dh_data;
 	spin_unlock_irqrestore(sdev->request_queue->queue_lock, flags);
@@ -901,7 +898,6 @@ static void alua_bus_detach(struct scsi_device *sdev)
 	if (h->buff && h->inq != h->buff)
 		kfree(h->buff);
 	kfree(scsi_dh_data);
-	module_put(THIS_MODULE);
 	sdev_printk(KERN_NOTICE, sdev, "%s: Detached\n", ALUA_DH_NAME);
 }
 
diff --git a/drivers/scsi/device_handler/scsi_dh_emc.c b/drivers/scsi/device_handler/scsi_dh_emc.c
index 8476538..153b4c3 100644
--- a/drivers/scsi/device_handler/scsi_dh_emc.c
+++ b/drivers/scsi/device_handler/scsi_dh_emc.c
@@ -692,9 +692,6 @@ static int clariion_bus_attach(struct scsi_device *sdev)
 	if (err != SCSI_DH_OK)
 		goto failed;
 
-	if (!try_module_get(THIS_MODULE))
-		goto failed;
-
 	spin_lock_irqsave(sdev->request_queue->queue_lock, flags);
 	sdev->scsi_dh_data = scsi_dh_data;
 	spin_unlock_irqrestore(sdev->request_queue->queue_lock, flags);
@@ -728,7 +725,6 @@ static void clariion_bus_detach(struct scsi_device *sdev)
 		    CLARIION_NAME);
 
 	kfree(scsi_dh_data);
-	module_put(THIS_MODULE);
 }
 
 static int __init clariion_init(void)
diff --git a/drivers/scsi/device_handler/scsi_dh_hp_sw.c b/drivers/scsi/device_handler/scsi_dh_hp_sw.c
index 4ee2759..367e6f5 100644
--- a/drivers/scsi/device_handler/scsi_dh_hp_sw.c
+++ b/drivers/scsi/device_handler/scsi_dh_hp_sw.c
@@ -377,9 +377,6 @@ static int hp_sw_bus_attach(struct scsi_device *sdev)
 	if (ret != SCSI_DH_OK || h->path_state == HP_SW_PATH_UNINITIALIZED)
 		goto failed;
 
-	if (!try_module_get(THIS_MODULE))
-		goto failed;
-
 	spin_lock_irqsave(sdev->request_queue->queue_lock, flags);
 	sdev->scsi_dh_data = scsi_dh_data;
 	spin_unlock_irqrestore(sdev->request_queue->queue_lock, flags);
@@ -406,7 +403,6 @@ static void hp_sw_bus_detach( struct scsi_device *sdev )
 	scsi_dh_data = sdev->scsi_dh_data;
 	sdev->scsi_dh_data = NULL;
 	spin_unlock_irqrestore(sdev->request_queue->queue_lock, flags);
-	module_put(THIS_MODULE);
 
 	sdev_printk(KERN_NOTICE, sdev, "%s: Detached\n", HP_SW_NAME);
 
diff --git a/drivers/scsi/device_handler/scsi_dh_rdac.c b/drivers/scsi/device_handler/scsi_dh_rdac.c
index 1b5bc92..b850954 100644
--- a/drivers/scsi/device_handler/scsi_dh_rdac.c
+++ b/drivers/scsi/device_handler/scsi_dh_rdac.c
@@ -878,9 +878,6 @@ static int rdac_bus_attach(struct scsi_device *sdev)
 	if (err != SCSI_DH_OK)
 		goto clean_ctlr;
 
-	if (!try_module_get(THIS_MODULE))
-		goto clean_ctlr;
-
 	spin_lock_irqsave(sdev->request_queue->queue_lock, flags);
 	sdev->scsi_dh_data = scsi_dh_data;
 	spin_unlock_irqrestore(sdev->request_queue->queue_lock, flags);
@@ -924,7 +921,6 @@ static void rdac_bus_detach( struct scsi_device *sdev )
 		kref_put(&h->ctlr->kref, release_controller);
 	spin_unlock(&list_lock);
 	kfree(scsi_dh_data);
-	module_put(THIS_MODULE);
 	sdev_printk(KERN_NOTICE, sdev, "%s: Detached\n", RDAC_NAME);
 }
 
-- 
1.9.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread
* [PATCH 2/6] scsi: use container_of to get at device handler private data
  2014-10-19 15:59 device handler cleanups Christoph Hellwig
  2014-10-19 15:59 ` [PATCH 1/6] scsi_dh: get module reference outside of device handler Christoph Hellwig
@ 2014-10-19 16:00 ` Christoph Hellwig
  2014-10-20  6:02   ` Hannes Reinecke
  2014-10-19 16:00 ` [PATCH 3/6] scsi: remove struct scsi_dh_devlist Christoph Hellwig
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 17+ messages in thread
From: Christoph Hellwig @ 2014-10-19 16:00 UTC (permalink / raw)
  To: Chandra Seetharaman, Hannes Reinecke, Mike Christie
  Cc: Sean Stewart, Bart Van Assche, linux-kernel

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/scsi/device_handler/scsi_dh_alua.c  | 25 +++++++++----------------
 drivers/scsi/device_handler/scsi_dh_emc.c   | 24 ++++++++++--------------
 drivers/scsi/device_handler/scsi_dh_hp_sw.c | 23 +++++++++--------------
 drivers/scsi/device_handler/scsi_dh_rdac.c  | 25 +++++++++----------------
 include/scsi/scsi_device.h                  |  1 -
 5 files changed, 37 insertions(+), 61 deletions(-)

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index 45b1795..40a2b0e 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -62,6 +62,7 @@
 #define ALUA_OPTIMIZE_STPG		1
 
 struct alua_dh_data {
+	struct scsi_dh_data	dh_data;
 	int			group_id;
 	int			rel_port;
 	int			tpgs;
@@ -87,9 +88,7 @@ static int alua_check_sense(struct scsi_device *, struct scsi_sense_hdr *);
 
 static inline struct alua_dh_data *get_alua_data(struct scsi_device *sdev)
 {
-	struct scsi_dh_data *scsi_dh_data = sdev->scsi_dh_data;
-	BUG_ON(scsi_dh_data == NULL);
-	return ((struct alua_dh_data *) scsi_dh_data->buf);
+	return container_of(sdev->scsi_dh_data, struct alua_dh_data, dh_data);
 }
 
 static int realloc_buffer(struct alua_dh_data *h, unsigned len)
@@ -839,21 +838,18 @@ static struct scsi_device_handler alua_dh = {
  */
 static int alua_bus_attach(struct scsi_device *sdev)
 {
-	struct scsi_dh_data *scsi_dh_data;
 	struct alua_dh_data *h;
 	unsigned long flags;
 	int err = SCSI_DH_OK;
 
-	scsi_dh_data = kzalloc(sizeof(*scsi_dh_data)
-			       + sizeof(*h) , GFP_KERNEL);
-	if (!scsi_dh_data) {
+	h = kzalloc(sizeof(*h) , GFP_KERNEL);
+	if (!h) {
 		sdev_printk(KERN_ERR, sdev, "%s: Attach failed\n",
 			    ALUA_DH_NAME);
 		return -ENOMEM;
 	}
 
-	scsi_dh_data->scsi_dh = &alua_dh;
-	h = (struct alua_dh_data *) scsi_dh_data->buf;
+	h->dh_data.scsi_dh = &alua_dh;
 	h->tpgs = TPGS_MODE_UNINITIALIZED;
 	h->state = TPGS_STATE_OPTIMIZED;
 	h->group_id = -1;
@@ -867,14 +863,14 @@ static int alua_bus_attach(struct scsi_device *sdev)
 		goto failed;
 
 	spin_lock_irqsave(sdev->request_queue->queue_lock, flags);
-	sdev->scsi_dh_data = scsi_dh_data;
+	sdev->scsi_dh_data = &h->dh_data;
 	spin_unlock_irqrestore(sdev->request_queue->queue_lock, flags);
 	sdev_printk(KERN_NOTICE, sdev, "%s: Attached\n", ALUA_DH_NAME);
 
 	return 0;
 
 failed:
-	kfree(scsi_dh_data);
+	kfree(h);
 	sdev_printk(KERN_ERR, sdev, "%s: not attached\n", ALUA_DH_NAME);
 	return -EINVAL;
 }
@@ -885,19 +881,16 @@ failed:
  */
 static void alua_bus_detach(struct scsi_device *sdev)
 {
-	struct scsi_dh_data *scsi_dh_data;
-	struct alua_dh_data *h;
+	struct alua_dh_data *h = get_alua_data(sdev);
 	unsigned long flags;
 
 	spin_lock_irqsave(sdev->request_queue->queue_lock, flags);
-	scsi_dh_data = sdev->scsi_dh_data;
 	sdev->scsi_dh_data = NULL;
 	spin_unlock_irqrestore(sdev->request_queue->queue_lock, flags);
 
-	h = (struct alua_dh_data *) scsi_dh_data->buf;
 	if (h->buff && h->inq != h->buff)
 		kfree(h->buff);
-	kfree(scsi_dh_data);
+	kfree(h);
 	sdev_printk(KERN_NOTICE, sdev, "%s: Detached\n", ALUA_DH_NAME);
 }
 
diff --git a/drivers/scsi/device_handler/scsi_dh_emc.c b/drivers/scsi/device_handler/scsi_dh_emc.c
index 153b4c3..c2e26cd 100644
--- a/drivers/scsi/device_handler/scsi_dh_emc.c
+++ b/drivers/scsi/device_handler/scsi_dh_emc.c
@@ -72,6 +72,7 @@ static const char * lun_state[] =
 };
 
 struct clariion_dh_data {
+	struct scsi_dh_data dh_data;
 	/*
 	 * Flags:
 	 *  CLARIION_SHORT_TRESPASS
@@ -116,9 +117,8 @@ struct clariion_dh_data {
 static inline struct clariion_dh_data
 			*get_clariion_data(struct scsi_device *sdev)
 {
-	struct scsi_dh_data *scsi_dh_data = sdev->scsi_dh_data;
-	BUG_ON(scsi_dh_data == NULL);
-	return ((struct clariion_dh_data *) scsi_dh_data->buf);
+	return container_of(sdev->scsi_dh_data, struct clariion_dh_data,
+			dh_data);
 }
 
 /*
@@ -665,21 +665,18 @@ static struct scsi_device_handler clariion_dh = {
 
 static int clariion_bus_attach(struct scsi_device *sdev)
 {
-	struct scsi_dh_data *scsi_dh_data;
 	struct clariion_dh_data *h;
 	unsigned long flags;
 	int err;
 
-	scsi_dh_data = kzalloc(sizeof(*scsi_dh_data)
-			       + sizeof(*h) , GFP_KERNEL);
-	if (!scsi_dh_data) {
+	h = kzalloc(sizeof(*h) , GFP_KERNEL);
+	if (!h) {
 		sdev_printk(KERN_ERR, sdev, "%s: Attach failed\n",
 			    CLARIION_NAME);
 		return -ENOMEM;
 	}
 
-	scsi_dh_data->scsi_dh = &clariion_dh;
-	h = (struct clariion_dh_data *) scsi_dh_data->buf;
+	h->dh_data.scsi_dh = &clariion_dh;
 	h->lun_state = CLARIION_LUN_UNINITIALIZED;
 	h->default_sp = CLARIION_UNBOUND_LU;
 	h->current_sp = CLARIION_UNBOUND_LU;
@@ -693,7 +690,7 @@ static int clariion_bus_attach(struct scsi_device *sdev)
 		goto failed;
 
 	spin_lock_irqsave(sdev->request_queue->queue_lock, flags);
-	sdev->scsi_dh_data = scsi_dh_data;
+	sdev->scsi_dh_data = &h->dh_data;
 	spin_unlock_irqrestore(sdev->request_queue->queue_lock, flags);
 
 	sdev_printk(KERN_INFO, sdev,
@@ -705,7 +702,7 @@ static int clariion_bus_attach(struct scsi_device *sdev)
 	return 0;
 
 failed:
-	kfree(scsi_dh_data);
+	kfree(h);
 	sdev_printk(KERN_ERR, sdev, "%s: not attached\n",
 		    CLARIION_NAME);
 	return -EINVAL;
@@ -713,18 +710,17 @@ failed:
 
 static void clariion_bus_detach(struct scsi_device *sdev)
 {
-	struct scsi_dh_data *scsi_dh_data;
+	struct clariion_dh_data *h = get_clariion_data(sdev);
 	unsigned long flags;
 
 	spin_lock_irqsave(sdev->request_queue->queue_lock, flags);
-	scsi_dh_data = sdev->scsi_dh_data;
 	sdev->scsi_dh_data = NULL;
 	spin_unlock_irqrestore(sdev->request_queue->queue_lock, flags);
 
 	sdev_printk(KERN_NOTICE, sdev, "%s: Detached\n",
 		    CLARIION_NAME);
 
-	kfree(scsi_dh_data);
+	kfree(h);
 }
 
 static int __init clariion_init(void)
diff --git a/drivers/scsi/device_handler/scsi_dh_hp_sw.c b/drivers/scsi/device_handler/scsi_dh_hp_sw.c
index 367e6f5..040998c 100644
--- a/drivers/scsi/device_handler/scsi_dh_hp_sw.c
+++ b/drivers/scsi/device_handler/scsi_dh_hp_sw.c
@@ -38,6 +38,7 @@
 #define HP_SW_PATH_PASSIVE		1
 
 struct hp_sw_dh_data {
+	struct scsi_dh_data dh_data;
 	unsigned char sense[SCSI_SENSE_BUFFERSIZE];
 	int path_state;
 	int retries;
@@ -51,9 +52,7 @@ static int hp_sw_start_stop(struct hp_sw_dh_data *);
 
 static inline struct hp_sw_dh_data *get_hp_sw_data(struct scsi_device *sdev)
 {
-	struct scsi_dh_data *scsi_dh_data = sdev->scsi_dh_data;
-	BUG_ON(scsi_dh_data == NULL);
-	return ((struct hp_sw_dh_data *) scsi_dh_data->buf);
+	return container_of(sdev->scsi_dh_data, struct hp_sw_dh_data, dh_data);
 }
 
 /*
@@ -354,21 +353,18 @@ static struct scsi_device_handler hp_sw_dh = {
 
 static int hp_sw_bus_attach(struct scsi_device *sdev)
 {
-	struct scsi_dh_data *scsi_dh_data;
 	struct hp_sw_dh_data *h;
 	unsigned long flags;
 	int ret;
 
-	scsi_dh_data = kzalloc(sizeof(*scsi_dh_data)
-			       + sizeof(*h) , GFP_KERNEL);
-	if (!scsi_dh_data) {
+	h = kzalloc(sizeof(*h), GFP_KERNEL);
+	if (!h) {
 		sdev_printk(KERN_ERR, sdev, "%s: Attach Failed\n",
 			    HP_SW_NAME);
 		return 0;
 	}
 
-	scsi_dh_data->scsi_dh = &hp_sw_dh;
-	h = (struct hp_sw_dh_data *) scsi_dh_data->buf;
+	h->dh_data.scsi_dh = &hp_sw_dh;
 	h->path_state = HP_SW_PATH_UNINITIALIZED;
 	h->retries = HP_SW_RETRIES;
 	h->sdev = sdev;
@@ -378,7 +374,7 @@ static int hp_sw_bus_attach(struct scsi_device *sdev)
 		goto failed;
 
 	spin_lock_irqsave(sdev->request_queue->queue_lock, flags);
-	sdev->scsi_dh_data = scsi_dh_data;
+	sdev->scsi_dh_data = &h->dh_data;
 	spin_unlock_irqrestore(sdev->request_queue->queue_lock, flags);
 
 	sdev_printk(KERN_INFO, sdev, "%s: attached to %s path\n",
@@ -388,7 +384,7 @@ static int hp_sw_bus_attach(struct scsi_device *sdev)
 	return 0;
 
 failed:
-	kfree(scsi_dh_data);
+	kfree(h);
 	sdev_printk(KERN_ERR, sdev, "%s: not attached\n",
 		    HP_SW_NAME);
 	return -EINVAL;
@@ -396,17 +392,16 @@ failed:
 
 static void hp_sw_bus_detach( struct scsi_device *sdev )
 {
-	struct scsi_dh_data *scsi_dh_data;
+	struct hp_sw_dh_data *h = get_hp_sw_data(sdev);
 	unsigned long flags;
 
 	spin_lock_irqsave(sdev->request_queue->queue_lock, flags);
-	scsi_dh_data = sdev->scsi_dh_data;
 	sdev->scsi_dh_data = NULL;
 	spin_unlock_irqrestore(sdev->request_queue->queue_lock, flags);
 
 	sdev_printk(KERN_NOTICE, sdev, "%s: Detached\n", HP_SW_NAME);
 
-	kfree(scsi_dh_data);
+	kfree(h);
 }
 
 static int __init hp_sw_init(void)
diff --git a/drivers/scsi/device_handler/scsi_dh_rdac.c b/drivers/scsi/device_handler/scsi_dh_rdac.c
index b850954..ef8caaa 100644
--- a/drivers/scsi/device_handler/scsi_dh_rdac.c
+++ b/drivers/scsi/device_handler/scsi_dh_rdac.c
@@ -181,6 +181,7 @@ struct c2_inquiry {
 };
 
 struct rdac_dh_data {
+	struct scsi_dh_data	dh_data;
 	struct rdac_controller	*ctlr;
 #define UNINITIALIZED_LUN	(1 << 8)
 	unsigned		lun;
@@ -261,9 +262,7 @@ do { \
 
 static inline struct rdac_dh_data *get_rdac_data(struct scsi_device *sdev)
 {
-	struct scsi_dh_data *scsi_dh_data = sdev->scsi_dh_data;
-	BUG_ON(scsi_dh_data == NULL);
-	return ((struct rdac_dh_data *) scsi_dh_data->buf);
+	return container_of(sdev->scsi_dh_data, struct rdac_dh_data, dh_data);
 }
 
 static struct request *get_rdac_req(struct scsi_device *sdev,
@@ -842,23 +841,20 @@ static struct scsi_device_handler rdac_dh = {
 
 static int rdac_bus_attach(struct scsi_device *sdev)
 {
-	struct scsi_dh_data *scsi_dh_data;
 	struct rdac_dh_data *h;
 	unsigned long flags;
 	int err;
 	char array_name[ARRAY_LABEL_LEN];
 	char array_id[UNIQUE_ID_LEN];
 
-	scsi_dh_data = kzalloc(sizeof(*scsi_dh_data)
-			       + sizeof(*h) , GFP_KERNEL);
-	if (!scsi_dh_data) {
+	h = kzalloc(sizeof(*h) , GFP_KERNEL);
+	if (!h) {
 		sdev_printk(KERN_ERR, sdev, "%s: Attach failed\n",
 			    RDAC_NAME);
 		return -ENOMEM;
 	}
 
-	scsi_dh_data->scsi_dh = &rdac_dh;
-	h = (struct rdac_dh_data *) scsi_dh_data->buf;
+	h->dh_data.scsi_dh = &rdac_dh;
 	h->lun = UNINITIALIZED_LUN;
 	h->state = RDAC_STATE_ACTIVE;
 
@@ -879,7 +875,7 @@ static int rdac_bus_attach(struct scsi_device *sdev)
 		goto clean_ctlr;
 
 	spin_lock_irqsave(sdev->request_queue->queue_lock, flags);
-	sdev->scsi_dh_data = scsi_dh_data;
+	sdev->scsi_dh_data = &h->dh_data;
 	spin_unlock_irqrestore(sdev->request_queue->queue_lock, flags);
 
 	sdev_printk(KERN_NOTICE, sdev,
@@ -895,7 +891,7 @@ clean_ctlr:
 	spin_unlock(&list_lock);
 
 failed:
-	kfree(scsi_dh_data);
+	kfree(h);
 	sdev_printk(KERN_ERR, sdev, "%s: not attached\n",
 		    RDAC_NAME);
 	return -EINVAL;
@@ -903,12 +899,9 @@ failed:
 
 static void rdac_bus_detach( struct scsi_device *sdev )
 {
-	struct scsi_dh_data *scsi_dh_data;
-	struct rdac_dh_data *h;
+	struct rdac_dh_data *h = get_rdac_data(sdev);
 	unsigned long flags;
 
-	scsi_dh_data = sdev->scsi_dh_data;
-	h = (struct rdac_dh_data *) scsi_dh_data->buf;
 	if (h->ctlr && h->ctlr->ms_queued)
 		flush_workqueue(kmpath_rdacd);
 
@@ -920,7 +913,7 @@ static void rdac_bus_detach( struct scsi_device *sdev )
 	if (h->ctlr)
 		kref_put(&h->ctlr->kref, release_controller);
 	spin_unlock(&list_lock);
-	kfree(scsi_dh_data);
+	kfree(h);
 	sdev_printk(KERN_NOTICE, sdev, "%s: Detached\n", RDAC_NAME);
 }
 
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 27ecee7..fb46864 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -228,7 +228,6 @@ struct scsi_dh_data {
 	struct scsi_device_handler *scsi_dh;
 	struct scsi_device *sdev;
 	struct kref kref;
-	char buf[0];
 };
 
 #define	to_scsi_device(d)	\
-- 
1.9.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread
* [PATCH 3/6] scsi: remove struct scsi_dh_devlist
  2014-10-19 15:59 device handler cleanups Christoph Hellwig
  2014-10-19 15:59 ` [PATCH 1/6] scsi_dh: get module reference outside of device handler Christoph Hellwig
  2014-10-19 16:00 ` [PATCH 2/6] scsi: use container_of to get at device handler private data Christoph Hellwig
@ 2014-10-19 16:00 ` Christoph Hellwig
  2014-10-20  6:03   ` Hannes Reinecke
  2014-10-19 16:00 ` [PATCH 4/6] scsi: device handlers must have attach and detach methods Christoph Hellwig
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 17+ messages in thread
From: Christoph Hellwig @ 2014-10-19 16:00 UTC (permalink / raw)
  To: Chandra Seetharaman, Hannes Reinecke, Mike Christie
  Cc: Sean Stewart, Bart Van Assche, linux-kernel

All drivers now do their own matching, so there is no more need to expose
a device list as part of the interface.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/scsi/device_handler/scsi_dh_emc.c   | 6 ++++--
 drivers/scsi/device_handler/scsi_dh_hp_sw.c | 6 ++++--
 drivers/scsi/device_handler/scsi_dh_rdac.c  | 6 ++++--
 include/scsi/scsi_device.h                  | 6 ------
 4 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/scsi/device_handler/scsi_dh_emc.c b/drivers/scsi/device_handler/scsi_dh_emc.c
index c2e26cd..800deb7 100644
--- a/drivers/scsi/device_handler/scsi_dh_emc.c
+++ b/drivers/scsi/device_handler/scsi_dh_emc.c
@@ -622,7 +622,10 @@ done:
 	return result;
 }
 
-static const struct scsi_dh_devlist clariion_dev_list[] = {
+static const struct {
+	char *vendor;
+	char *model;
+} clariion_dev_list[] = {
 	{"DGC", "RAID"},
 	{"DGC", "DISK"},
 	{"DGC", "VRAID"},
@@ -653,7 +656,6 @@ static void clariion_bus_detach(struct scsi_device *sdev);
 static struct scsi_device_handler clariion_dh = {
 	.name		= CLARIION_NAME,
 	.module		= THIS_MODULE,
-	.devlist	= clariion_dev_list,
 	.attach		= clariion_bus_attach,
 	.detach		= clariion_bus_detach,
 	.check_sense	= clariion_check_sense,
diff --git a/drivers/scsi/device_handler/scsi_dh_hp_sw.c b/drivers/scsi/device_handler/scsi_dh_hp_sw.c
index 040998c..aa40fcb 100644
--- a/drivers/scsi/device_handler/scsi_dh_hp_sw.c
+++ b/drivers/scsi/device_handler/scsi_dh_hp_sw.c
@@ -311,7 +311,10 @@ static int hp_sw_activate(struct scsi_device *sdev,
 	return 0;
 }
 
-static const struct scsi_dh_devlist hp_sw_dh_data_list[] = {
+static const struct {
+	char *vendor;
+	char *model;
+} hp_sw_dh_data_list[] = {
 	{"COMPAQ", "MSA1000 VOLUME"},
 	{"COMPAQ", "HSV110"},
 	{"HP", "HSV100"},
@@ -343,7 +346,6 @@ static void hp_sw_bus_detach(struct scsi_device *sdev);
 static struct scsi_device_handler hp_sw_dh = {
 	.name		= HP_SW_NAME,
 	.module		= THIS_MODULE,
-	.devlist	= hp_sw_dh_data_list,
 	.attach		= hp_sw_bus_attach,
 	.detach		= hp_sw_bus_detach,
 	.activate	= hp_sw_activate,
diff --git a/drivers/scsi/device_handler/scsi_dh_rdac.c b/drivers/scsi/device_handler/scsi_dh_rdac.c
index ef8caaa..8b09528 100644
--- a/drivers/scsi/device_handler/scsi_dh_rdac.c
+++ b/drivers/scsi/device_handler/scsi_dh_rdac.c
@@ -778,7 +778,10 @@ static int rdac_check_sense(struct scsi_device *sdev,
 	return SCSI_RETURN_NOT_HANDLED;
 }
 
-static const struct scsi_dh_devlist rdac_dev_list[] = {
+static const struct {
+	char *vendor;
+	char *model;
+} rdac_dev_list[] = {
 	{"IBM", "1722"},
 	{"IBM", "1724"},
 	{"IBM", "1726"},
@@ -830,7 +833,6 @@ static void rdac_bus_detach(struct scsi_device *sdev);
 static struct scsi_device_handler rdac_dh = {
 	.name = RDAC_NAME,
 	.module = THIS_MODULE,
-	.devlist = rdac_dev_list,
 	.prep_fn = rdac_prep_fn,
 	.check_sense = rdac_check_sense,
 	.attach = rdac_bus_attach,
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index fb46864..ba6c9b7 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -201,11 +201,6 @@ struct scsi_device {
 	unsigned long		sdev_data[0];
 } __attribute__((aligned(sizeof(unsigned long))));
 
-struct scsi_dh_devlist {
-	char *vendor;
-	char *model;
-};
-
 typedef void (*activate_complete)(void *, int);
 struct scsi_device_handler {
 	/* Used by the infrastructure */
@@ -214,7 +209,6 @@ struct scsi_device_handler {
 	/* Filled by the hardware handler */
 	struct module *module;
 	const char *name;
-	const struct scsi_dh_devlist *devlist;
 	int (*check_sense)(struct scsi_device *, struct scsi_sense_hdr *);
 	int (*attach)(struct scsi_device *);
 	void (*detach)(struct scsi_device *);
-- 
1.9.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread
* [PATCH 4/6] scsi: device handlers must have attach and detach methods
  2014-10-19 15:59 device handler cleanups Christoph Hellwig
                   ` (2 preceding siblings ...)
  2014-10-19 16:00 ` [PATCH 3/6] scsi: remove struct scsi_dh_devlist Christoph Hellwig
@ 2014-10-19 16:00 ` Christoph Hellwig
  2014-10-20  6:04   ` Hannes Reinecke
  2014-10-19 16:00 ` [PATCH 5/6] scsi_dh_hp_sw: fix return value on failed allocation Christoph Hellwig
  2014-10-19 16:00 ` [PATCH 6/6] scsi: handle more device handler setup/teardown in common code Christoph Hellwig
  5 siblings, 1 reply; 17+ messages in thread
From: Christoph Hellwig @ 2014-10-19 16:00 UTC (permalink / raw)
  To: Chandra Seetharaman, Hannes Reinecke, Mike Christie
  Cc: Sean Stewart, Bart Van Assche, linux-kernel

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/scsi/device_handler/scsi_dh.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/scsi/device_handler/scsi_dh.c b/drivers/scsi/device_handler/scsi_dh.c
index 1a8dbf3..d8a8aac 100644
--- a/drivers/scsi/device_handler/scsi_dh.c
+++ b/drivers/scsi/device_handler/scsi_dh.c
@@ -108,19 +108,17 @@ static int scsi_dh_handler_attach(struct scsi_device *sdev,
 		return 0;
 	}
 
-	if (scsi_dh->attach) {
-		if (!try_module_get(scsi_dh->module))
-			return -EINVAL;
-
-		err = scsi_dh->attach(sdev);
-		if (err) {
-			module_put(scsi_dh->module);
-			return err;
-		}
+	if (!try_module_get(scsi_dh->module))
+		return -EINVAL;
 
-		kref_init(&sdev->scsi_dh_data->kref);
-		sdev->scsi_dh_data->sdev = sdev;
+	err = scsi_dh->attach(sdev);
+	if (err) {
+		module_put(scsi_dh->module);
+		return err;
 	}
+
+	kref_init(&sdev->scsi_dh_data->kref);
+	sdev->scsi_dh_data->sdev = sdev;
 	return err;
 }
 
@@ -154,7 +152,7 @@ static void scsi_dh_handler_detach(struct scsi_device *sdev,
 	if (!scsi_dh)
 		scsi_dh = sdev->scsi_dh_data->scsi_dh;
 
-	if (scsi_dh && scsi_dh->detach)
+	if (scsi_dh)
 		kref_put(&sdev->scsi_dh_data->kref, __detach_handler);
 }
 
@@ -343,6 +341,9 @@ int scsi_register_device_handler(struct scsi_device_handler *scsi_dh)
 	if (get_device_handler(scsi_dh->name))
 		return -EBUSY;
 
+	if (!scsi_dh->attach || !scsi_dh->detach)
+		return -EINVAL;
+
 	spin_lock(&list_lock);
 	list_add(&scsi_dh->list, &scsi_dh_list);
 	spin_unlock(&list_lock);
-- 
1.9.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread
* [PATCH 5/6] scsi_dh_hp_sw: fix return value on failed allocation
  2014-10-19 15:59 device handler cleanups Christoph Hellwig
                   ` (3 preceding siblings ...)
  2014-10-19 16:00 ` [PATCH 4/6] scsi: device handlers must have attach and detach methods Christoph Hellwig
@ 2014-10-19 16:00 ` Christoph Hellwig
  2014-10-20  6:04   ` Hannes Reinecke
  2014-10-19 16:00 ` [PATCH 6/6] scsi: handle more device handler setup/teardown in common code Christoph Hellwig
  5 siblings, 1 reply; 17+ messages in thread
From: Christoph Hellwig @ 2014-10-19 16:00 UTC (permalink / raw)
  To: Chandra Seetharaman, Hannes Reinecke, Mike Christie
  Cc: Sean Stewart, Bart Van Assche, linux-kernel

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/scsi/device_handler/scsi_dh_hp_sw.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/device_handler/scsi_dh_hp_sw.c b/drivers/scsi/device_handler/scsi_dh_hp_sw.c
index aa40fcb..471ffd1 100644
--- a/drivers/scsi/device_handler/scsi_dh_hp_sw.c
+++ b/drivers/scsi/device_handler/scsi_dh_hp_sw.c
@@ -363,7 +363,7 @@ static int hp_sw_bus_attach(struct scsi_device *sdev)
 	if (!h) {
 		sdev_printk(KERN_ERR, sdev, "%s: Attach Failed\n",
 			    HP_SW_NAME);
-		return 0;
+		return -ENOMEM;
 	}
 
 	h->dh_data.scsi_dh = &hp_sw_dh;
-- 
1.9.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread
* [PATCH 6/6] scsi: handle more device handler setup/teardown in common code
  2014-10-19 15:59 device handler cleanups Christoph Hellwig
                   ` (4 preceding siblings ...)
  2014-10-19 16:00 ` [PATCH 5/6] scsi_dh_hp_sw: fix return value on failed allocation Christoph Hellwig
@ 2014-10-19 16:00 ` Christoph Hellwig
  2014-10-20  6:06   ` Hannes Reinecke
  2014-10-28 17:53   ` Mike Christie
  5 siblings, 2 replies; 17+ messages in thread
From: Christoph Hellwig @ 2014-10-19 16:00 UTC (permalink / raw)
  To: Chandra Seetharaman, Hannes Reinecke, Mike Christie
  Cc: Sean Stewart, Bart Van Assche, linux-kernel

Move all code to set up and tear down sdev->scsi_dh_data to common code.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/scsi/device_handler/scsi_dh.c       | 29 ++++++++++----
 drivers/scsi/device_handler/scsi_dh_alua.c  | 59 ++++++++++-------------------
 drivers/scsi/device_handler/scsi_dh_emc.c   | 58 +++++++++-------------------
 drivers/scsi/device_handler/scsi_dh_hp_sw.c | 54 ++++++++------------------
 drivers/scsi/device_handler/scsi_dh_rdac.c  | 52 +++++++------------------
 include/scsi/scsi_device.h                  |  2 +-
 6 files changed, 87 insertions(+), 167 deletions(-)

diff --git a/drivers/scsi/device_handler/scsi_dh.c b/drivers/scsi/device_handler/scsi_dh.c
index d8a8aac..1dba62c 100644
--- a/drivers/scsi/device_handler/scsi_dh.c
+++ b/drivers/scsi/device_handler/scsi_dh.c
@@ -98,7 +98,7 @@ device_handler_match(struct scsi_device_handler *scsi_dh,
 static int scsi_dh_handler_attach(struct scsi_device *sdev,
 				  struct scsi_device_handler *scsi_dh)
 {
-	int err = 0;
+	struct scsi_dh_data *d;
 
 	if (sdev->scsi_dh_data) {
 		if (sdev->scsi_dh_data->scsi_dh != scsi_dh)
@@ -111,15 +111,22 @@ static int scsi_dh_handler_attach(struct scsi_device *sdev,
 	if (!try_module_get(scsi_dh->module))
 		return -EINVAL;
 
-	err = scsi_dh->attach(sdev);
-	if (err) {
+	d = scsi_dh->attach(sdev);
+	if (IS_ERR(d)) {
+		sdev_printk(KERN_ERR, sdev, "%s: Attach failed (%ld)\n",
+			    scsi_dh->name, PTR_ERR(d));
 		module_put(scsi_dh->module);
-		return err;
+		return PTR_ERR(d);
 	}
 
-	kref_init(&sdev->scsi_dh_data->kref);
-	sdev->scsi_dh_data->sdev = sdev;
-	return err;
+	d->scsi_dh = scsi_dh;
+	kref_init(&d->kref);
+	d->sdev = sdev;
+
+	spin_lock_irq(sdev->request_queue->queue_lock);
+	sdev->scsi_dh_data = d;
+	spin_unlock_irq(sdev->request_queue->queue_lock);
+	return 0;
 }
 
 static void __detach_handler (struct kref *kref)
@@ -127,8 +134,14 @@ static void __detach_handler (struct kref *kref)
 	struct scsi_dh_data *scsi_dh_data =
 		container_of(kref, struct scsi_dh_data, kref);
 	struct scsi_device_handler *scsi_dh = scsi_dh_data->scsi_dh;
+	struct scsi_device *sdev = scsi_dh_data->sdev;
+
+	spin_lock_irq(sdev->request_queue->queue_lock);
+	sdev->scsi_dh_data = NULL;
+	spin_unlock_irq(sdev->request_queue->queue_lock);
 
-	scsi_dh->detach(scsi_dh_data->sdev);
+	scsi_dh->detach(sdev);
+	sdev_printk(KERN_NOTICE, sdev, "%s: Detached\n", scsi_dh->name);
 	module_put(scsi_dh->module);
 }
 
diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index 40a2b0e..94d1c36 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -817,39 +817,18 @@ static bool alua_match(struct scsi_device *sdev)
 	return (scsi_device_tpgs(sdev) != 0);
 }
 
-static int alua_bus_attach(struct scsi_device *sdev);
-static void alua_bus_detach(struct scsi_device *sdev);
-
-static struct scsi_device_handler alua_dh = {
-	.name = ALUA_DH_NAME,
-	.module = THIS_MODULE,
-	.attach = alua_bus_attach,
-	.detach = alua_bus_detach,
-	.prep_fn = alua_prep_fn,
-	.check_sense = alua_check_sense,
-	.activate = alua_activate,
-	.set_params = alua_set_params,
-	.match = alua_match,
-};
-
 /*
  * alua_bus_attach - Attach device handler
  * @sdev: device to be attached to
  */
-static int alua_bus_attach(struct scsi_device *sdev)
+static struct scsi_dh_data *alua_bus_attach(struct scsi_device *sdev)
 {
 	struct alua_dh_data *h;
-	unsigned long flags;
-	int err = SCSI_DH_OK;
+	int err;
 
 	h = kzalloc(sizeof(*h) , GFP_KERNEL);
-	if (!h) {
-		sdev_printk(KERN_ERR, sdev, "%s: Attach failed\n",
-			    ALUA_DH_NAME);
-		return -ENOMEM;
-	}
-
-	h->dh_data.scsi_dh = &alua_dh;
+	if (!h)
+		return ERR_PTR(-ENOMEM);
 	h->tpgs = TPGS_MODE_UNINITIALIZED;
 	h->state = TPGS_STATE_OPTIMIZED;
 	h->group_id = -1;
@@ -859,20 +838,14 @@ static int alua_bus_attach(struct scsi_device *sdev)
 	h->sdev = sdev;
 
 	err = alua_initialize(sdev, h);
-	if ((err != SCSI_DH_OK) && (err != SCSI_DH_DEV_OFFLINED))
+	if (err != SCSI_DH_OK && err != SCSI_DH_DEV_OFFLINED)
 		goto failed;
 
-	spin_lock_irqsave(sdev->request_queue->queue_lock, flags);
-	sdev->scsi_dh_data = &h->dh_data;
-	spin_unlock_irqrestore(sdev->request_queue->queue_lock, flags);
 	sdev_printk(KERN_NOTICE, sdev, "%s: Attached\n", ALUA_DH_NAME);
-
-	return 0;
-
+	return &h->dh_data;
 failed:
 	kfree(h);
-	sdev_printk(KERN_ERR, sdev, "%s: not attached\n", ALUA_DH_NAME);
-	return -EINVAL;
+	return ERR_PTR(-EINVAL);
 }
 
 /*
@@ -882,18 +855,24 @@ failed:
 static void alua_bus_detach(struct scsi_device *sdev)
 {
 	struct alua_dh_data *h = get_alua_data(sdev);
-	unsigned long flags;
-
-	spin_lock_irqsave(sdev->request_queue->queue_lock, flags);
-	sdev->scsi_dh_data = NULL;
-	spin_unlock_irqrestore(sdev->request_queue->queue_lock, flags);
 
 	if (h->buff && h->inq != h->buff)
 		kfree(h->buff);
 	kfree(h);
-	sdev_printk(KERN_NOTICE, sdev, "%s: Detached\n", ALUA_DH_NAME);
 }
 
+static struct scsi_device_handler alua_dh = {
+	.name = ALUA_DH_NAME,
+	.module = THIS_MODULE,
+	.attach = alua_bus_attach,
+	.detach = alua_bus_detach,
+	.prep_fn = alua_prep_fn,
+	.check_sense = alua_check_sense,
+	.activate = alua_activate,
+	.set_params = alua_set_params,
+	.match = alua_match,
+};
+
 static int __init alua_init(void)
 {
 	int r;
diff --git a/drivers/scsi/device_handler/scsi_dh_emc.c b/drivers/scsi/device_handler/scsi_dh_emc.c
index 800deb7..6ed1caa 100644
--- a/drivers/scsi/device_handler/scsi_dh_emc.c
+++ b/drivers/scsi/device_handler/scsi_dh_emc.c
@@ -650,35 +650,14 @@ static bool clariion_match(struct scsi_device *sdev)
 	return false;
 }
 
-static int clariion_bus_attach(struct scsi_device *sdev);
-static void clariion_bus_detach(struct scsi_device *sdev);
-
-static struct scsi_device_handler clariion_dh = {
-	.name		= CLARIION_NAME,
-	.module		= THIS_MODULE,
-	.attach		= clariion_bus_attach,
-	.detach		= clariion_bus_detach,
-	.check_sense	= clariion_check_sense,
-	.activate	= clariion_activate,
-	.prep_fn	= clariion_prep_fn,
-	.set_params	= clariion_set_params,
-	.match		= clariion_match,
-};
-
-static int clariion_bus_attach(struct scsi_device *sdev)
+static struct scsi_dh_data *clariion_bus_attach(struct scsi_device *sdev)
 {
 	struct clariion_dh_data *h;
-	unsigned long flags;
 	int err;
 
 	h = kzalloc(sizeof(*h) , GFP_KERNEL);
-	if (!h) {
-		sdev_printk(KERN_ERR, sdev, "%s: Attach failed\n",
-			    CLARIION_NAME);
-		return -ENOMEM;
-	}
-
-	h->dh_data.scsi_dh = &clariion_dh;
+	if (!h)
+		return ERR_PTR(-ENOMEM);
 	h->lun_state = CLARIION_LUN_UNINITIALIZED;
 	h->default_sp = CLARIION_UNBOUND_LU;
 	h->current_sp = CLARIION_UNBOUND_LU;
@@ -691,40 +670,37 @@ static int clariion_bus_attach(struct scsi_device *sdev)
 	if (err != SCSI_DH_OK)
 		goto failed;
 
-	spin_lock_irqsave(sdev->request_queue->queue_lock, flags);
-	sdev->scsi_dh_data = &h->dh_data;
-	spin_unlock_irqrestore(sdev->request_queue->queue_lock, flags);
-
 	sdev_printk(KERN_INFO, sdev,
 		    "%s: connected to SP %c Port %d (%s, default SP %c)\n",
 		    CLARIION_NAME, h->current_sp + 'A',
 		    h->port, lun_state[h->lun_state],
 		    h->default_sp + 'A');
-
-	return 0;
+	return &h->dh_data;
 
 failed:
 	kfree(h);
-	sdev_printk(KERN_ERR, sdev, "%s: not attached\n",
-		    CLARIION_NAME);
-	return -EINVAL;
+	return ERR_PTR(-EINVAL);
 }
 
 static void clariion_bus_detach(struct scsi_device *sdev)
 {
 	struct clariion_dh_data *h = get_clariion_data(sdev);
-	unsigned long flags;
-
-	spin_lock_irqsave(sdev->request_queue->queue_lock, flags);
-	sdev->scsi_dh_data = NULL;
-	spin_unlock_irqrestore(sdev->request_queue->queue_lock, flags);
-
-	sdev_printk(KERN_NOTICE, sdev, "%s: Detached\n",
-		    CLARIION_NAME);
 
 	kfree(h);
 }
 
+static struct scsi_device_handler clariion_dh = {
+	.name		= CLARIION_NAME,
+	.module		= THIS_MODULE,
+	.attach		= clariion_bus_attach,
+	.detach		= clariion_bus_detach,
+	.check_sense	= clariion_check_sense,
+	.activate	= clariion_activate,
+	.prep_fn	= clariion_prep_fn,
+	.set_params	= clariion_set_params,
+	.match		= clariion_match,
+};
+
 static int __init clariion_init(void)
 {
 	int r;
diff --git a/drivers/scsi/device_handler/scsi_dh_hp_sw.c b/drivers/scsi/device_handler/scsi_dh_hp_sw.c
index 471ffd1..485d995 100644
--- a/drivers/scsi/device_handler/scsi_dh_hp_sw.c
+++ b/drivers/scsi/device_handler/scsi_dh_hp_sw.c
@@ -340,33 +340,14 @@ static bool hp_sw_match(struct scsi_device *sdev)
 	return false;
 }
 
-static int hp_sw_bus_attach(struct scsi_device *sdev);
-static void hp_sw_bus_detach(struct scsi_device *sdev);
-
-static struct scsi_device_handler hp_sw_dh = {
-	.name		= HP_SW_NAME,
-	.module		= THIS_MODULE,
-	.attach		= hp_sw_bus_attach,
-	.detach		= hp_sw_bus_detach,
-	.activate	= hp_sw_activate,
-	.prep_fn	= hp_sw_prep_fn,
-	.match		= hp_sw_match,
-};
-
-static int hp_sw_bus_attach(struct scsi_device *sdev)
+static struct scsi_dh_data *hp_sw_bus_attach(struct scsi_device *sdev)
 {
 	struct hp_sw_dh_data *h;
-	unsigned long flags;
 	int ret;
 
 	h = kzalloc(sizeof(*h), GFP_KERNEL);
-	if (!h) {
-		sdev_printk(KERN_ERR, sdev, "%s: Attach Failed\n",
-			    HP_SW_NAME);
-		return -ENOMEM;
-	}
-
-	h->dh_data.scsi_dh = &hp_sw_dh;
+	if (!h)
+		return ERR_PTR(-ENOMEM);
 	h->path_state = HP_SW_PATH_UNINITIALIZED;
 	h->retries = HP_SW_RETRIES;
 	h->sdev = sdev;
@@ -375,37 +356,32 @@ static int hp_sw_bus_attach(struct scsi_device *sdev)
 	if (ret != SCSI_DH_OK || h->path_state == HP_SW_PATH_UNINITIALIZED)
 		goto failed;
 
-	spin_lock_irqsave(sdev->request_queue->queue_lock, flags);
-	sdev->scsi_dh_data = &h->dh_data;
-	spin_unlock_irqrestore(sdev->request_queue->queue_lock, flags);
-
 	sdev_printk(KERN_INFO, sdev, "%s: attached to %s path\n",
 		    HP_SW_NAME, h->path_state == HP_SW_PATH_ACTIVE?
 		    "active":"passive");
-
-	return 0;
-
+	return &h->dh_data;
 failed:
 	kfree(h);
-	sdev_printk(KERN_ERR, sdev, "%s: not attached\n",
-		    HP_SW_NAME);
-	return -EINVAL;
+	return ERR_PTR(-EINVAL);
 }
 
 static void hp_sw_bus_detach( struct scsi_device *sdev )
 {
 	struct hp_sw_dh_data *h = get_hp_sw_data(sdev);
-	unsigned long flags;
-
-	spin_lock_irqsave(sdev->request_queue->queue_lock, flags);
-	sdev->scsi_dh_data = NULL;
-	spin_unlock_irqrestore(sdev->request_queue->queue_lock, flags);
-
-	sdev_printk(KERN_NOTICE, sdev, "%s: Detached\n", HP_SW_NAME);
 
 	kfree(h);
 }
 
+static struct scsi_device_handler hp_sw_dh = {
+	.name		= HP_SW_NAME,
+	.module		= THIS_MODULE,
+	.attach		= hp_sw_bus_attach,
+	.detach		= hp_sw_bus_detach,
+	.activate	= hp_sw_activate,
+	.prep_fn	= hp_sw_prep_fn,
+	.match		= hp_sw_match,
+};
+
 static int __init hp_sw_init(void)
 {
 	return scsi_register_device_handler(&hp_sw_dh);
diff --git a/drivers/scsi/device_handler/scsi_dh_rdac.c b/drivers/scsi/device_handler/scsi_dh_rdac.c
index 8b09528..363872a 100644
--- a/drivers/scsi/device_handler/scsi_dh_rdac.c
+++ b/drivers/scsi/device_handler/scsi_dh_rdac.c
@@ -827,36 +827,16 @@ static bool rdac_match(struct scsi_device *sdev)
 	return false;
 }
 
-static int rdac_bus_attach(struct scsi_device *sdev);
-static void rdac_bus_detach(struct scsi_device *sdev);
-
-static struct scsi_device_handler rdac_dh = {
-	.name = RDAC_NAME,
-	.module = THIS_MODULE,
-	.prep_fn = rdac_prep_fn,
-	.check_sense = rdac_check_sense,
-	.attach = rdac_bus_attach,
-	.detach = rdac_bus_detach,
-	.activate = rdac_activate,
-	.match = rdac_match,
-};
-
-static int rdac_bus_attach(struct scsi_device *sdev)
+static struct scsi_dh_data *rdac_bus_attach(struct scsi_device *sdev)
 {
 	struct rdac_dh_data *h;
-	unsigned long flags;
 	int err;
 	char array_name[ARRAY_LABEL_LEN];
 	char array_id[UNIQUE_ID_LEN];
 
 	h = kzalloc(sizeof(*h) , GFP_KERNEL);
-	if (!h) {
-		sdev_printk(KERN_ERR, sdev, "%s: Attach failed\n",
-			    RDAC_NAME);
-		return -ENOMEM;
-	}
-
-	h->dh_data.scsi_dh = &rdac_dh;
+	if (!h)
+		return ERR_PTR(-ENOMEM);
 	h->lun = UNINITIALIZED_LUN;
 	h->state = RDAC_STATE_ACTIVE;
 
@@ -876,15 +856,10 @@ static int rdac_bus_attach(struct scsi_device *sdev)
 	if (err != SCSI_DH_OK)
 		goto clean_ctlr;
 
-	spin_lock_irqsave(sdev->request_queue->queue_lock, flags);
-	sdev->scsi_dh_data = &h->dh_data;
-	spin_unlock_irqrestore(sdev->request_queue->queue_lock, flags);
-
 	sdev_printk(KERN_NOTICE, sdev,
 		    "%s: LUN %d (%s) (%s)\n",
 		    RDAC_NAME, h->lun, mode[(int)h->mode],
 		    lun_state[(int)h->lun_state]);
-
 	return 0;
 
 clean_ctlr:
@@ -894,32 +869,33 @@ clean_ctlr:
 
 failed:
 	kfree(h);
-	sdev_printk(KERN_ERR, sdev, "%s: not attached\n",
-		    RDAC_NAME);
-	return -EINVAL;
+	return ERR_PTR(-EINVAL);
 }
 
 static void rdac_bus_detach( struct scsi_device *sdev )
 {
 	struct rdac_dh_data *h = get_rdac_data(sdev);
-	unsigned long flags;
 
 	if (h->ctlr && h->ctlr->ms_queued)
 		flush_workqueue(kmpath_rdacd);
 
-	spin_lock_irqsave(sdev->request_queue->queue_lock, flags);
-	sdev->scsi_dh_data = NULL;
-	spin_unlock_irqrestore(sdev->request_queue->queue_lock, flags);
-
 	spin_lock(&list_lock);
 	if (h->ctlr)
 		kref_put(&h->ctlr->kref, release_controller);
 	spin_unlock(&list_lock);
 	kfree(h);
-	sdev_printk(KERN_NOTICE, sdev, "%s: Detached\n", RDAC_NAME);
 }
 
-
+static struct scsi_device_handler rdac_dh = {
+	.name = RDAC_NAME,
+	.module = THIS_MODULE,
+	.prep_fn = rdac_prep_fn,
+	.check_sense = rdac_check_sense,
+	.attach = rdac_bus_attach,
+	.detach = rdac_bus_detach,
+	.activate = rdac_activate,
+	.match = rdac_match,
+};
 
 static int __init rdac_init(void)
 {
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index ba6c9b7..3f7e7be 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -210,7 +210,7 @@ struct scsi_device_handler {
 	struct module *module;
 	const char *name;
 	int (*check_sense)(struct scsi_device *, struct scsi_sense_hdr *);
-	int (*attach)(struct scsi_device *);
+	struct scsi_dh_data *(*attach)(struct scsi_device *);
 	void (*detach)(struct scsi_device *);
 	int (*activate)(struct scsi_device *, activate_complete, void *);
 	int (*prep_fn)(struct scsi_device *, struct request *);
-- 
1.9.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread
* Re: [PATCH 1/6] scsi_dh: get module reference outside of device handler
  2014-10-19 15:59 ` [PATCH 1/6] scsi_dh: get module reference outside of device handler Christoph Hellwig
@ 2014-10-20  6:01   ` Hannes Reinecke
  2014-10-20  6:53     ` Christoph Hellwig
  0 siblings, 1 reply; 17+ messages in thread
From: Hannes Reinecke @ 2014-10-20  6:01 UTC (permalink / raw)
  To: Christoph Hellwig, Chandra Seetharaman, Mike Christie
  Cc: Sean Stewart, Bart Van Assche, linux-kernel

On 10/19/2014 05:59 PM, Christoph Hellwig wrote:
> We need to grab a reference to the module before calling the attach
> routines to avoid a small race vs module removal.  It also cleans up
> the code significantly as a side effect.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/scsi/device_handler/scsi_dh.c       | 31 ++++++++++++++++++++---------
>  drivers/scsi/device_handler/scsi_dh_alua.c  |  4 ----
>  drivers/scsi/device_handler/scsi_dh_emc.c   |  4 ----
>  drivers/scsi/device_handler/scsi_dh_hp_sw.c |  4 ----
>  drivers/scsi/device_handler/scsi_dh_rdac.c  |  4 ----
>  5 files changed, 22 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/scsi/device_handler/scsi_dh.c b/drivers/scsi/device_handler/scsi_dh.c
> index 33e422e..1a8dbf3 100644
> --- a/drivers/scsi/device_handler/scsi_dh.c
> +++ b/drivers/scsi/device_handler/scsi_dh.c
> @@ -102,23 +102,36 @@ static int scsi_dh_handler_attach(struct scsi_device *sdev,
>  
>  	if (sdev->scsi_dh_data) {
>  		if (sdev->scsi_dh_data->scsi_dh != scsi_dh)
> -			err = -EBUSY;
> -		else
> -			kref_get(&sdev->scsi_dh_data->kref);
> -	} else if (scsi_dh->attach) {
> +			return -EBUSY;
> +
> +		kref_get(&sdev->scsi_dh_data->kref);
> +		return 0;
> +	}
> +
> +	if (scsi_dh->attach) {
> +		if (!try_module_get(scsi_dh->module))
> +			return -EINVAL;
> +
>  		err = scsi_dh->attach(sdev);
> -		if (!err) {
> -			kref_init(&sdev->scsi_dh_data->kref);
> -			sdev->scsi_dh_data->sdev = sdev;
> +		if (err) {
> +			module_put(scsi_dh->module);
> +			return err;
>  		}
> +
> +		kref_init(&sdev->scsi_dh_data->kref);
> +		sdev->scsi_dh_data->sdev = sdev;
>  	}
>  	return err;
>  }
>  
>  static void __detach_handler (struct kref *kref)
>  {
> -	struct scsi_dh_data *scsi_dh_data = container_of(kref, struct scsi_dh_data, kref);
> -	scsi_dh_data->scsi_dh->detach(scsi_dh_data->sdev);
> +	struct scsi_dh_data *scsi_dh_data =
> +		container_of(kref, struct scsi_dh_data, kref);
> +	struct scsi_device_handler *scsi_dh = scsi_dh_data->scsi_dh;
> +
> +	scsi_dh->detach(scsi_dh_data->sdev);
> +	module_put(scsi_dh->module);
>  }
>  
>  /*
Is it guaranteed that you cannot call ->attach() for devices which
already have a device_handler attached?
You've skipped the case

scsi_dh != sdev->scsi_dh_data->scsi_dh

IE someone called 'attach()' on a device which already has a
different device_handler attached to it.

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)

^ permalink raw reply	[flat|nested] 17+ messages in thread
* Re: [PATCH 2/6] scsi: use container_of to get at device handler private data
  2014-10-19 16:00 ` [PATCH 2/6] scsi: use container_of to get at device handler private data Christoph Hellwig
@ 2014-10-20  6:02   ` Hannes Reinecke
  0 siblings, 0 replies; 17+ messages in thread
From: Hannes Reinecke @ 2014-10-20  6:02 UTC (permalink / raw)
  To: Christoph Hellwig, Chandra Seetharaman, Mike Christie
  Cc: Sean Stewart, Bart Van Assche, linux-kernel

On 10/19/2014 06:00 PM, Christoph Hellwig wrote:
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Hannes Reinecke <hare@suse.de>

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)

^ permalink raw reply	[flat|nested] 17+ messages in thread
* Re: [PATCH 3/6] scsi: remove struct scsi_dh_devlist
  2014-10-19 16:00 ` [PATCH 3/6] scsi: remove struct scsi_dh_devlist Christoph Hellwig
@ 2014-10-20  6:03   ` Hannes Reinecke
  0 siblings, 0 replies; 17+ messages in thread
From: Hannes Reinecke @ 2014-10-20  6:03 UTC (permalink / raw)
  To: Christoph Hellwig, Chandra Seetharaman, Mike Christie
  Cc: Sean Stewart, Bart Van Assche, linux-kernel

On 10/19/2014 06:00 PM, Christoph Hellwig wrote:
> All drivers now do their own matching, so there is no more need to expose
> a device list as part of the interface.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/scsi/device_handler/scsi_dh_emc.c   | 6 ++++--
>  drivers/scsi/device_handler/scsi_dh_hp_sw.c | 6 ++++--
>  drivers/scsi/device_handler/scsi_dh_rdac.c  | 6 ++++--
>  include/scsi/scsi_device.h                  | 6 ------
>  4 files changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/scsi/device_handler/scsi_dh_emc.c b/drivers/scsi/device_handler/scsi_dh_emc.c
> index c2e26cd..800deb7 100644
> --- a/drivers/scsi/device_handler/scsi_dh_emc.c
> +++ b/drivers/scsi/device_handler/scsi_dh_emc.c
> @@ -622,7 +622,10 @@ done:
>  	return result;
>  }
>  
> -static const struct scsi_dh_devlist clariion_dev_list[] = {
> +static const struct {
> +	char *vendor;
> +	char *model;
> +} clariion_dev_list[] = {
>  	{"DGC", "RAID"},
>  	{"DGC", "DISK"},
>  	{"DGC", "VRAID"},
> @@ -653,7 +656,6 @@ static void clariion_bus_detach(struct scsi_device *sdev);
>  static struct scsi_device_handler clariion_dh = {
>  	.name		= CLARIION_NAME,
>  	.module		= THIS_MODULE,
> -	.devlist	= clariion_dev_list,
>  	.attach		= clariion_bus_attach,
>  	.detach		= clariion_bus_detach,
>  	.check_sense	= clariion_check_sense,
> diff --git a/drivers/scsi/device_handler/scsi_dh_hp_sw.c b/drivers/scsi/device_handler/scsi_dh_hp_sw.c
> index 040998c..aa40fcb 100644
> --- a/drivers/scsi/device_handler/scsi_dh_hp_sw.c
> +++ b/drivers/scsi/device_handler/scsi_dh_hp_sw.c
> @@ -311,7 +311,10 @@ static int hp_sw_activate(struct scsi_device *sdev,
>  	return 0;
>  }
>  
> -static const struct scsi_dh_devlist hp_sw_dh_data_list[] = {
> +static const struct {
> +	char *vendor;
> +	char *model;
> +} hp_sw_dh_data_list[] = {
>  	{"COMPAQ", "MSA1000 VOLUME"},
>  	{"COMPAQ", "HSV110"},
>  	{"HP", "HSV100"},
> @@ -343,7 +346,6 @@ static void hp_sw_bus_detach(struct scsi_device *sdev);
>  static struct scsi_device_handler hp_sw_dh = {
>  	.name		= HP_SW_NAME,
>  	.module		= THIS_MODULE,
> -	.devlist	= hp_sw_dh_data_list,
>  	.attach		= hp_sw_bus_attach,
>  	.detach		= hp_sw_bus_detach,
>  	.activate	= hp_sw_activate,
> diff --git a/drivers/scsi/device_handler/scsi_dh_rdac.c b/drivers/scsi/device_handler/scsi_dh_rdac.c
> index ef8caaa..8b09528 100644
> --- a/drivers/scsi/device_handler/scsi_dh_rdac.c
> +++ b/drivers/scsi/device_handler/scsi_dh_rdac.c
> @@ -778,7 +778,10 @@ static int rdac_check_sense(struct scsi_device *sdev,
>  	return SCSI_RETURN_NOT_HANDLED;
>  }
>  
> -static const struct scsi_dh_devlist rdac_dev_list[] = {
> +static const struct {
> +	char *vendor;
> +	char *model;
> +} rdac_dev_list[] = {
>  	{"IBM", "1722"},
>  	{"IBM", "1724"},
>  	{"IBM", "1726"},
> @@ -830,7 +833,6 @@ static void rdac_bus_detach(struct scsi_device *sdev);
>  static struct scsi_device_handler rdac_dh = {
>  	.name = RDAC_NAME,
>  	.module = THIS_MODULE,
> -	.devlist = rdac_dev_list,
>  	.prep_fn = rdac_prep_fn,
>  	.check_sense = rdac_check_sense,
>  	.attach = rdac_bus_attach,
> diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
> index fb46864..ba6c9b7 100644
> --- a/include/scsi/scsi_device.h
> +++ b/include/scsi/scsi_device.h
> @@ -201,11 +201,6 @@ struct scsi_device {
>  	unsigned long		sdev_data[0];
>  } __attribute__((aligned(sizeof(unsigned long))));
>  
> -struct scsi_dh_devlist {
> -	char *vendor;
> -	char *model;
> -};
> -
>  typedef void (*activate_complete)(void *, int);
>  struct scsi_device_handler {
>  	/* Used by the infrastructure */
> @@ -214,7 +209,6 @@ struct scsi_device_handler {
>  	/* Filled by the hardware handler */
>  	struct module *module;
>  	const char *name;
> -	const struct scsi_dh_devlist *devlist;
>  	int (*check_sense)(struct scsi_device *, struct scsi_sense_hdr *);
>  	int (*attach)(struct scsi_device *);
>  	void (*detach)(struct scsi_device *);
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

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)

^ permalink raw reply	[flat|nested] 17+ messages in thread
* Re: [PATCH 4/6] scsi: device handlers must have attach and detach methods
  2014-10-19 16:00 ` [PATCH 4/6] scsi: device handlers must have attach and detach methods Christoph Hellwig
@ 2014-10-20  6:04   ` Hannes Reinecke
  0 siblings, 0 replies; 17+ messages in thread
From: Hannes Reinecke @ 2014-10-20  6:04 UTC (permalink / raw)
  To: Christoph Hellwig, Chandra Seetharaman, Mike Christie
  Cc: Sean Stewart, Bart Van Assche, linux-kernel

On 10/19/2014 06:00 PM, Christoph Hellwig wrote:
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/scsi/device_handler/scsi_dh.c | 25 +++++++++++++------------
>  1 file changed, 13 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/scsi/device_handler/scsi_dh.c b/drivers/scsi/device_handler/scsi_dh.c
> index 1a8dbf3..d8a8aac 100644
> --- a/drivers/scsi/device_handler/scsi_dh.c
> +++ b/drivers/scsi/device_handler/scsi_dh.c
> @@ -108,19 +108,17 @@ static int scsi_dh_handler_attach(struct scsi_device *sdev,
>  		return 0;
>  	}
>  
> -	if (scsi_dh->attach) {
> -		if (!try_module_get(scsi_dh->module))
> -			return -EINVAL;
> -
> -		err = scsi_dh->attach(sdev);
> -		if (err) {
> -			module_put(scsi_dh->module);
> -			return err;
> -		}
> +	if (!try_module_get(scsi_dh->module))
> +		return -EINVAL;
>  
> -		kref_init(&sdev->scsi_dh_data->kref);
> -		sdev->scsi_dh_data->sdev = sdev;
> +	err = scsi_dh->attach(sdev);
> +	if (err) {
> +		module_put(scsi_dh->module);
> +		return err;
>  	}
> +
> +	kref_init(&sdev->scsi_dh_data->kref);
> +	sdev->scsi_dh_data->sdev = sdev;
>  	return err;
>  }
>  
> @@ -154,7 +152,7 @@ static void scsi_dh_handler_detach(struct scsi_device *sdev,
>  	if (!scsi_dh)
>  		scsi_dh = sdev->scsi_dh_data->scsi_dh;
>  
> -	if (scsi_dh && scsi_dh->detach)
> +	if (scsi_dh)
>  		kref_put(&sdev->scsi_dh_data->kref, __detach_handler);
>  }
>  
> @@ -343,6 +341,9 @@ int scsi_register_device_handler(struct scsi_device_handler *scsi_dh)
>  	if (get_device_handler(scsi_dh->name))
>  		return -EBUSY;
>  
> +	if (!scsi_dh->attach || !scsi_dh->detach)
> +		return -EINVAL;
> +
>  	spin_lock(&list_lock);
>  	list_add(&scsi_dh->list, &scsi_dh_list);
>  	spin_unlock(&list_lock);
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

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)

^ permalink raw reply	[flat|nested] 17+ messages in thread
* Re: [PATCH 5/6] scsi_dh_hp_sw: fix return value on failed allocation
  2014-10-19 16:00 ` [PATCH 5/6] scsi_dh_hp_sw: fix return value on failed allocation Christoph Hellwig
@ 2014-10-20  6:04   ` Hannes Reinecke
  0 siblings, 0 replies; 17+ messages in thread
From: Hannes Reinecke @ 2014-10-20  6:04 UTC (permalink / raw)
  To: Christoph Hellwig, Chandra Seetharaman, Mike Christie
  Cc: Sean Stewart, Bart Van Assche, linux-kernel

On 10/19/2014 06:00 PM, Christoph Hellwig wrote:
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/scsi/device_handler/scsi_dh_hp_sw.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/device_handler/scsi_dh_hp_sw.c b/drivers/scsi/device_handler/scsi_dh_hp_sw.c
> index aa40fcb..471ffd1 100644
> --- a/drivers/scsi/device_handler/scsi_dh_hp_sw.c
> +++ b/drivers/scsi/device_handler/scsi_dh_hp_sw.c
> @@ -363,7 +363,7 @@ static int hp_sw_bus_attach(struct scsi_device *sdev)
>  	if (!h) {
>  		sdev_printk(KERN_ERR, sdev, "%s: Attach Failed\n",
>  			    HP_SW_NAME);
> -		return 0;
> +		return -ENOMEM;
>  	}
>  
>  	h->dh_data.scsi_dh = &hp_sw_dh;
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

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)

^ permalink raw reply	[flat|nested] 17+ messages in thread
* Re: [PATCH 6/6] scsi: handle more device handler setup/teardown in common code
  2014-10-19 16:00 ` [PATCH 6/6] scsi: handle more device handler setup/teardown in common code Christoph Hellwig
@ 2014-10-20  6:06   ` Hannes Reinecke
  2014-10-28 17:53   ` Mike Christie
  1 sibling, 0 replies; 17+ messages in thread
From: Hannes Reinecke @ 2014-10-20  6:06 UTC (permalink / raw)
  To: Christoph Hellwig, Chandra Seetharaman, Mike Christie
  Cc: Sean Stewart, Bart Van Assche, linux-kernel

On 10/19/2014 06:00 PM, Christoph Hellwig wrote:
> Move all code to set up and tear down sdev->scsi_dh_data to common code.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Hannes Reinecke <hare@suse.de>

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)

^ permalink raw reply	[flat|nested] 17+ messages in thread
* Re: [PATCH 1/6] scsi_dh: get module reference outside of device handler
  2014-10-20  6:01   ` Hannes Reinecke
@ 2014-10-20  6:53     ` Christoph Hellwig
  2014-10-20  8:06       ` Hannes Reinecke
  0 siblings, 1 reply; 17+ messages in thread
From: Christoph Hellwig @ 2014-10-20  6:53 UTC (permalink / raw)
  To: Hannes Reinecke
  Cc: Mike Christie, Sean Stewart, Bart Van Assche, linux-kernel

On Mon, Oct 20, 2014 at 08:01:20AM +0200, Hannes Reinecke wrote:
> Is it guaranteed that you cannot call ->attach() for devices which
> already have a device_handler attached?
> You've skipped the case
> 
> scsi_dh != sdev->scsi_dh_data->scsi_dh

No.  Just instead of an if / else it's an if with an early return in the
new code.


^ permalink raw reply	[flat|nested] 17+ messages in thread
* Re: [PATCH 1/6] scsi_dh: get module reference outside of device handler
  2014-10-20  6:53     ` Christoph Hellwig
@ 2014-10-20  8:06       ` Hannes Reinecke
  0 siblings, 0 replies; 17+ messages in thread
From: Hannes Reinecke @ 2014-10-20  8:06 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Mike Christie, Sean Stewart, Bart Van Assche, linux-kernel

On 10/20/2014 08:53 AM, Christoph Hellwig wrote:
> On Mon, Oct 20, 2014 at 08:01:20AM +0200, Hannes Reinecke wrote:
>> Is it guaranteed that you cannot call ->attach() for devices which
>> already have a device_handler attached?
>> You've skipped the case
>>
>> scsi_dh != sdev->scsi_dh_data->scsi_dh
> 
> No.  Just instead of an if / else it's an if with an early return in the
> new code.
> 
right. Indeed.

Reviewed-by: Hannes Reinecke <hare@suse.de>

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)

^ permalink raw reply	[flat|nested] 17+ messages in thread
* Re: [PATCH 6/6] scsi: handle more device handler setup/teardown in common code
  2014-10-19 16:00 ` [PATCH 6/6] scsi: handle more device handler setup/teardown in common code Christoph Hellwig
  2014-10-20  6:06   ` Hannes Reinecke
@ 2014-10-28 17:53   ` Mike Christie
  2014-10-30  9:00     ` Christoph Hellwig
  1 sibling, 1 reply; 17+ messages in thread
From: Mike Christie @ 2014-10-28 17:53 UTC (permalink / raw)
  To: Christoph Hellwig, Chandra Seetharaman, Hannes Reinecke
  Cc: Sean Stewart, Bart Van Assche, linux-kernel

On 10/19/2014 11:00 AM, Christoph Hellwig wrote:
> -static int rdac_bus_attach(struct scsi_device *sdev)
> +static struct scsi_dh_data *rdac_bus_attach(struct scsi_device *sdev)
>  {
>  	struct rdac_dh_data *h;
> -	unsigned long flags;
>  	int err;
>  	char array_name[ARRAY_LABEL_LEN];
>  	char array_id[UNIQUE_ID_LEN];
>  
>  	h = kzalloc(sizeof(*h) , GFP_KERNEL);
> -	if (!h) {
> -		sdev_printk(KERN_ERR, sdev, "%s: Attach failed\n",
> -			    RDAC_NAME);
> -		return -ENOMEM;
> -	}
> -
> -	h->dh_data.scsi_dh = &rdac_dh;
> +	if (!h)
> +		return ERR_PTR(-ENOMEM);
>  	h->lun = UNINITIALIZED_LUN;
>  	h->state = RDAC_STATE_ACTIVE;
>  
> @@ -876,15 +856,10 @@ static int rdac_bus_attach(struct scsi_device *sdev)
>  	if (err != SCSI_DH_OK)
>  		goto clean_ctlr;
>  
> -	spin_lock_irqsave(sdev->request_queue->queue_lock, flags);
> -	sdev->scsi_dh_data = &h->dh_data;
> -	spin_unlock_irqrestore(sdev->request_queue->queue_lock, flags);
> -
>  	sdev_printk(KERN_NOTICE, sdev,
>  		    "%s: LUN %d (%s) (%s)\n",
>  		    RDAC_NAME, h->lun, mode[(int)h->mode],
>  		    lun_state[(int)h->lun_state]);
> -
>  	return 0;

Was this supposed to return a "struct scsi_dh_data *" instead of zero here?

^ permalink raw reply	[flat|nested] 17+ messages in thread
* Re: [PATCH 6/6] scsi: handle more device handler setup/teardown in common code
  2014-10-28 17:53   ` Mike Christie
@ 2014-10-30  9:00     ` Christoph Hellwig
  0 siblings, 0 replies; 17+ messages in thread
From: Christoph Hellwig @ 2014-10-30  9:00 UTC (permalink / raw)
  To: Mike Christie
  Cc: Christoph Hellwig, Chandra Seetharaman, Hannes Reinecke,
	Sean Stewart, Bart Van Assche, linux-kernel

On Tue, Oct 28, 2014 at 12:53:00PM -0500, Mike Christie wrote:
> Was this supposed to return a "struct scsi_dh_data *" instead of zero here?

Yes, thanks for catching it.


^ permalink raw reply	[flat|nested] 17+ messages in thread
end of thread, other threads:[~2014-10-30  9:00 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-10-19 15:59 device handler cleanups Christoph Hellwig
2014-10-19 15:59 ` [PATCH 1/6] scsi_dh: get module reference outside of device handler Christoph Hellwig
2014-10-20  6:01   ` Hannes Reinecke
2014-10-20  6:53     ` Christoph Hellwig
2014-10-20  8:06       ` Hannes Reinecke
2014-10-19 16:00 ` [PATCH 2/6] scsi: use container_of to get at device handler private data Christoph Hellwig
2014-10-20  6:02   ` Hannes Reinecke
2014-10-19 16:00 ` [PATCH 3/6] scsi: remove struct scsi_dh_devlist Christoph Hellwig
2014-10-20  6:03   ` Hannes Reinecke
2014-10-19 16:00 ` [PATCH 4/6] scsi: device handlers must have attach and detach methods Christoph Hellwig
2014-10-20  6:04   ` Hannes Reinecke
2014-10-19 16:00 ` [PATCH 5/6] scsi_dh_hp_sw: fix return value on failed allocation Christoph Hellwig
2014-10-20  6:04   ` Hannes Reinecke
2014-10-19 16:00 ` [PATCH 6/6] scsi: handle more device handler setup/teardown in common code Christoph Hellwig
2014-10-20  6:06   ` Hannes Reinecke
2014-10-28 17:53   ` Mike Christie
2014-10-30  9:00     ` Christoph Hellwig

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).