All of lore.kernel.org
 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; 18+ 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] 18+ 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; 18+ 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] 18+ 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; 18+ 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] 18+ 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; 18+ 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] 18+ 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; 18+ 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] 18+ 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; 18+ 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] 18+ 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; 18+ 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] 18+ 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; 18+ 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] 18+ 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; 18+ 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] 18+ 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; 18+ 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] 18+ 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; 18+ 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] 18+ 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; 18+ 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] 18+ 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; 18+ 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] 18+ 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; 18+ 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] 18+ 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; 18+ 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] 18+ 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; 18+ 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] 18+ 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; 18+ 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] 18+ messages in thread
* [PATCH 1/6] scsi_dh: get module reference outside of device handler
  2014-10-30  9:19 device handler cleanups V2 Christoph Hellwig
@ 2014-10-30  9:19 ` Christoph Hellwig
  0 siblings, 0 replies; 18+ messages in thread
From: Christoph Hellwig @ 2014-10-30  9:19 UTC (permalink / raw)
  To: Hannes Reinecke, Mike Christie; +Cc: Sean Stewart, Bart Van Assche, linux-scsi

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>
Reviewed-by: Hannes Reinecke <hare@suse.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] 18+ messages in thread
end of thread, other threads:[~2014-10-30  9:19 UTC | newest]

Thread overview: 18+ 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
  -- strict thread matches above, loose matches on Subject: below --
2014-10-30  9:19 device handler cleanups V2 Christoph Hellwig
2014-10-30  9:19 ` [PATCH 1/6] scsi_dh: get module reference outside of device handler Christoph Hellwig

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.