[PATCH v6 0/4] scsi: ufs & ums-* & esp_scsi: fix module reference counting

[PATCH v6 0/4] scsi: ufs & ums-* & esp_scsi: fix module reference counting

[Akinobu Mita]

While accessing a scsi_device, the use count of the underlying LLDD module
is incremented.  The module reference is retrieved through .module field of
struct scsi_host_template.

This mapping between scsi_device and underlying LLDD module works well
except ufs, unusual usb storage drivers, and sub drivers for esp_scsi.
These drivers consist with core driver and actual LLDDs, and
scsi_host_template is defined in the core driver.  So the actual LLDDs can
be unloaded even if the scsi_device is being accessed.

This patch series first adds ability to adjust module reference for
scsi host by LLDDs and then fixes actual LLDDs by adjusting module
reference after scsi host allocation.

* v6:
- Rebased as v5 doesn't apply cleanly to the latest tree anymore.

* v5:
- Discard v4 changes and restore to v3.  Because v4 shows that
  moving owner module field from scsi_host_template to Scsi_Host
  requires a lot of changes and introduces complication to existing
  drivers which don't have the module reference mismatch issue.
- Rebased to the 4.0-rc1

* v4:
- Patch series is almost rewritten as module reference field in
  struct scsi_host_template has been unused anymore.  So Acked-by: and
  Reviewed-by: tags that have been received are deleted.

* v3:
- Add fix for ESP SCSI drivers

* v2:
- Pass correct module reference to usb_stor_probe1() instead of touching
  all ums-* drivers, suggested by Alan Stern

Akinobu Mita (4):
  scsi: add ability to adjust module reference for scsi host
  scsi: ufs: adjust module reference for scsi host
  usb: storage: adjust module reference for scsi host
  scsi: esp_scsi: adjust module reference for scsi host

 drivers/scsi/am53c974.c          |  3 +--
 drivers/scsi/esp_scsi.c          | 16 +++++++++++++---
 drivers/scsi/esp_scsi.h          | 11 +++++++----
 drivers/scsi/hosts.c             |  1 +
 drivers/scsi/jazz_esp.c          |  3 +--
 drivers/scsi/mac_esp.c           |  3 +--
 drivers/scsi/scsi.c              |  4 ++--
 drivers/scsi/sun3x_esp.c         |  3 +--
 drivers/scsi/sun_esp.c           |  3 +--
 drivers/scsi/ufs/ufshcd-pci.c    |  1 +
 drivers/scsi/ufs/ufshcd-pltfrm.c |  1 +
 drivers/scsi/ufs/ufshcd.c        |  1 -
 drivers/usb/storage/usb.c        |  8 +++++---
 drivers/usb/storage/usb.h        |  7 +++++--
 include/scsi/scsi_host.h         |  1 +
 15 files changed, 41 insertions(+), 25 deletions(-)

Cc: Vinayak Holikatti <vinholikatti@gmail.com>
Cc: Dolev Raviv <draviv@codeaurora.org>
Cc: Sujit Reddy Thumma <sthumma@codeaurora.org>
Cc: Subhash Jadavani <subhashj@codeaurora.org>
Cc: Christoph Hellwig <hch@lst.de>
Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
Cc: Matthew Dharm <mdharm-usb@one-eyed-alien.net>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Alan Stern <stern@rowland.harvard.edu>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Hannes Reinecke <hare@suse.de>
Cc: linux-usb@vger.kernel.org
Cc: usb-storage@lists.one-eyed-alien.net
Cc: linux-scsi@vger.kernel.org
-- 
1.9.1

[PATCH v6 1/4] scsi: add ability to adjust module reference for scsi host

[Akinobu Mita]

While accessing a scsi_device, the use count of the underlying LLDD module
is incremented.  The module reference is retrieved through .module field of
struct scsi_host_template.

This mapping between scsi_device and underlying LLDD module works well
except ufs, unusual usb storage drivers, and sub drivers for esp_scsi.
These drivers consist with core driver and actual LLDDs, and
scsi_host_template is defined in the core driver.  So the actual LLDDs can
be unloaded even if the scsi_device is being accessed.

This adds .module field in struct Scsi_Host and let the module reference
be retrieved though it instead of struct scsi_host_template.  This allows
the actual LLDDs adjust module reference.

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Reviewed-by: Hannes Reinecke <hare@suse.de>
Cc: Vinayak Holikatti <vinholikatti@gmail.com>
Cc: Dolev Raviv <draviv@codeaurora.org>
Cc: Sujit Reddy Thumma <sthumma@codeaurora.org>
Cc: Subhash Jadavani <subhashj@codeaurora.org>
Cc: Christoph Hellwig <hch@lst.de>
Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
Cc: Matthew Dharm <mdharm-usb@one-eyed-alien.net>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Alan Stern <stern@rowland.harvard.edu>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Hannes Reinecke <hare@suse.de>
Cc: linux-usb@vger.kernel.org
Cc: usb-storage@lists.one-eyed-alien.net
Cc: linux-scsi@vger.kernel.org
---
* Changes from v5
- Rebased as v5 doesn't apply cleanly to the latest tree anymore.

 drivers/scsi/hosts.c     | 1 +
 drivers/scsi/scsi.c      | 4 ++--
 include/scsi/scsi_host.h | 1 +
 3 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index 8bb173e..21f1442 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -411,6 +411,7 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int privsize)
 	 */
 	shost->max_cmd_len = 12;
 	shost->hostt = sht;
+	shost->module = sht->module;
 	shost->this_id = sht->this_id;
 	shost->can_queue = sht->can_queue;
 	shost->sg_tablesize = sht->sg_tablesize;
diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 3833bf5..f7534a9 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -982,7 +982,7 @@ int scsi_device_get(struct scsi_device *sdev)
 		goto fail;
 	if (!get_device(&sdev->sdev_gendev))
 		goto fail;
-	if (!try_module_get(sdev->host->hostt->module))
+	if (!try_module_get(sdev->host->module))
 		goto fail_put_device;
 	return 0;
 
@@ -1003,7 +1003,7 @@ EXPORT_SYMBOL(scsi_device_get);
  */
 void scsi_device_put(struct scsi_device *sdev)
 {
-	module_put(sdev->host->hostt->module);
+	module_put(sdev->host->module);
 	put_device(&sdev->sdev_gendev);
 }
 EXPORT_SYMBOL(scsi_device_put);
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index e113c75..8742bfd 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -620,6 +620,7 @@ struct Scsi_Host {
 	 */
 	unsigned short max_cmd_len;
 
+	struct module *module;
 	int this_id;
 	int can_queue;
 	short cmd_per_lun;
-- 
1.9.1

[PATCH v6 2/4] scsi: ufs: adjust module reference for scsi host

[Akinobu Mita]

While accessing a UFS device, the module reference count for core driver
(ufshcd) is incremented but not incremented for the actual glue driver
(ufshcd-pci or ufshcd-pltfrm).  Because these drivers allocate scsi hosts
with scsi_host_template defined in ufshcd module.  So these drivers always
can be unloaded.

This fixes it by adjusting module reference after scsi host allocation.

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Cc: Vinayak Holikatti <vinholikatti@gmail.com>
Cc: Dolev Raviv <draviv@codeaurora.org>
Cc: Sujit Reddy Thumma <sthumma@codeaurora.org>
Cc: Subhash Jadavani <subhashj@codeaurora.org>
Cc: Christoph Hellwig <hch@lst.de>
Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
Cc: linux-scsi@vger.kernel.org
---
 drivers/scsi/ufs/ufshcd-pci.c    | 1 +
 drivers/scsi/ufs/ufshcd-pltfrm.c | 1 +
 drivers/scsi/ufs/ufshcd.c        | 1 -
 3 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/ufs/ufshcd-pci.c b/drivers/scsi/ufs/ufshcd-pci.c
index d15eaa4..9fe21b4 100644
--- a/drivers/scsi/ufs/ufshcd-pci.c
+++ b/drivers/scsi/ufs/ufshcd-pci.c
@@ -142,6 +142,7 @@ ufshcd_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 		return err;
 	}
 
+	hba->host->module = THIS_MODULE;
 	INIT_LIST_HEAD(&hba->clk_list_head);
 
 	err = ufshcd_init(hba, mmio_base, pdev->irq);
diff --git a/drivers/scsi/ufs/ufshcd-pltfrm.c b/drivers/scsi/ufs/ufshcd-pltfrm.c
index e6b29b1..d08e0bb 100644
--- a/drivers/scsi/ufs/ufshcd-pltfrm.c
+++ b/drivers/scsi/ufs/ufshcd-pltfrm.c
@@ -322,6 +322,7 @@ static int ufshcd_pltfrm_probe(struct platform_device *pdev)
 		goto out;
 	}
 
+	hba->host->module = THIS_MODULE;
 	hba->vops = get_variant_ops(&pdev->dev);
 
 	err = ufshcd_parse_clock_info(hba);
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 996030b..c499f16 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -4237,7 +4237,6 @@ static void ufshcd_async_scan(void *data, async_cookie_t cookie)
 }
 
 static struct scsi_host_template ufshcd_driver_template = {
-	.module			= THIS_MODULE,
 	.name			= UFSHCD,
 	.proc_name		= UFSHCD,
 	.queuecommand		= ufshcd_queuecommand,
-- 
1.9.1

[PATCH v6 3/4] usb: storage: adjust module reference for scsi host

[Akinobu Mita]

While accessing a unusual usb storage (ums-alauda, ums-cypress, ...),
the module reference count is not incremented.  Because these drivers
allocate scsi hosts with usb_stor_host_template defined in usb-storage
module.  So these drivers always can be unloaded.

This fixes it by passing correct module reference to usb_stor_probe1() so
that .module field in struct Scsi_Host can be adjusted.

Signed-off-by: Akinobu Mita <akinobu.mita-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Acked-by: Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org>
Cc: Matthew Dharm <mdharm-usb-JGfshJpz5UybPZpvUQj5UqxOck334EZe@public.gmane.org>
Cc: Greg Kroah-Hartman <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>
Cc: Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org>
Cc: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
Cc: "James E.J. Bottomley" <JBottomley-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
Cc: linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: usb-storage-ijkIwGHArpdIPJnuZ7Njw4oP9KaGy4wf@public.gmane.org
Cc: linux-scsi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
---
 drivers/usb/storage/usb.c | 8 +++++---
 drivers/usb/storage/usb.h | 7 +++++--
 2 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c
index 6c10c88..711cdee 100644
--- a/drivers/usb/storage/usb.c
+++ b/drivers/usb/storage/usb.c
@@ -921,10 +921,11 @@ static unsigned int usb_stor_sg_tablesize(struct usb_interface *intf)
 }
 
 /* First part of general USB mass-storage probing */
-int usb_stor_probe1(struct us_data **pus,
+int __usb_stor_probe1(struct us_data **pus,
 		struct usb_interface *intf,
 		const struct usb_device_id *id,
-		struct us_unusual_dev *unusual_dev)
+		struct us_unusual_dev *unusual_dev,
+		struct module *owner)
 {
 	struct Scsi_Host *host;
 	struct us_data *us;
@@ -947,6 +948,7 @@ int usb_stor_probe1(struct us_data **pus,
 	 */
 	host->max_cmd_len = 16;
 	host->sg_tablesize = usb_stor_sg_tablesize(intf);
+	host->module = owner;
 	*pus = us = host_to_us(host);
 	mutex_init(&(us->dev_mutex));
 	us_set_lock_class(&us->dev_mutex, intf);
@@ -979,7 +981,7 @@ BadDevice:
 	release_everything(us);
 	return result;
 }
-EXPORT_SYMBOL_GPL(usb_stor_probe1);
+EXPORT_SYMBOL_GPL(__usb_stor_probe1);
 
 /* Second part of general USB mass-storage probing */
 int usb_stor_probe2(struct us_data *us)
diff --git a/drivers/usb/storage/usb.h b/drivers/usb/storage/usb.h
index 307e339..0cb74ba 100644
--- a/drivers/usb/storage/usb.h
+++ b/drivers/usb/storage/usb.h
@@ -194,10 +194,13 @@ extern int usb_stor_reset_resume(struct usb_interface *iface);
 extern int usb_stor_pre_reset(struct usb_interface *iface);
 extern int usb_stor_post_reset(struct usb_interface *iface);
 
-extern int usb_stor_probe1(struct us_data **pus,
+extern int __usb_stor_probe1(struct us_data **pus,
 		struct usb_interface *intf,
 		const struct usb_device_id *id,
-		struct us_unusual_dev *unusual_dev);
+		struct us_unusual_dev *unusual_dev,
+		struct module *owner);
+#define usb_stor_probe1(pus, intf, id, unusual_dev) \
+	__usb_stor_probe1(pus, intf, id, unusual_dev, THIS_MODULE)
 extern int usb_stor_probe2(struct us_data *us);
 extern void usb_stor_disconnect(struct usb_interface *intf);
 
-- 
1.9.1

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

[PATCH v6 4/4] scsi: esp_scsi: adjust module reference for scsi host

[Akinobu Mita]

While accessing a scsi device on host adapter supported by sub driver for
the ESP chip (mac_esp, am53c974, sun_esp, jazz_esp, sun3x_esp), the module
reference count is not incremented.  Because these drivers allocate scsi
hosts with scsi_esp_template defined in ESP SCSI driver core module.  So
these drivers always can be unloaded.

This fixes it by passing correct module reference to newly introduced
scsi_esp_host_alloc() so that .module field in struct Scsi_Host can be
adjusted.

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Acked-by: David S. Miller <davem@davemloft.net>
Acked-by: Hannes Reinecke <hare@suse.de>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Hannes Reinecke <hare@suse.de>
Cc: Christoph Hellwig <hch@lst.de>
Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
Cc: linux-scsi@vger.kernel.org
---
 drivers/scsi/am53c974.c  |  3 +--
 drivers/scsi/esp_scsi.c  | 16 +++++++++++++---
 drivers/scsi/esp_scsi.h  | 11 +++++++----
 drivers/scsi/jazz_esp.c  |  3 +--
 drivers/scsi/mac_esp.c   |  3 +--
 drivers/scsi/sun3x_esp.c |  3 +--
 drivers/scsi/sun_esp.c   |  3 +--
 7 files changed, 25 insertions(+), 17 deletions(-)

diff --git a/drivers/scsi/am53c974.c b/drivers/scsi/am53c974.c
index beea30e..c1ba885 100644
--- a/drivers/scsi/am53c974.c
+++ b/drivers/scsi/am53c974.c
@@ -400,7 +400,6 @@ static void dc390_check_eeprom(struct esp *esp)
 static int pci_esp_probe_one(struct pci_dev *pdev,
 			      const struct pci_device_id *id)
 {
-	struct scsi_host_template *hostt = &scsi_esp_template;
 	int err = -ENODEV;
 	struct Scsi_Host *shost;
 	struct esp *esp;
@@ -417,7 +416,7 @@ static int pci_esp_probe_one(struct pci_dev *pdev,
 		goto fail_disable_device;
 	}
 
-	shost = scsi_host_alloc(hostt, sizeof(struct esp));
+	shost = scsi_esp_host_alloc(sizeof(struct esp));
 	if (!shost) {
 		dev_printk(KERN_INFO, &pdev->dev,
 			   "failed to allocate scsi host\n");
diff --git a/drivers/scsi/esp_scsi.c b/drivers/scsi/esp_scsi.c
index 065b25d..4484c90 100644
--- a/drivers/scsi/esp_scsi.c
+++ b/drivers/scsi/esp_scsi.c
@@ -2675,8 +2675,7 @@ static const char *esp_info(struct Scsi_Host *host)
 	return "esp";
 }
 
-struct scsi_host_template scsi_esp_template = {
-	.module			= THIS_MODULE,
+static struct scsi_host_template scsi_esp_template = {
 	.name			= "esp",
 	.info			= esp_info,
 	.queuecommand		= esp_queuecommand,
@@ -2696,7 +2695,18 @@ struct scsi_host_template scsi_esp_template = {
 	.skip_settle_delay	= 1,
 	.use_blk_tags		= 1,
 };
-EXPORT_SYMBOL(scsi_esp_template);
+
+struct Scsi_Host *__scsi_esp_host_alloc(int privsize, struct module *owner)
+{
+	struct Scsi_Host *shost;
+
+	shost = scsi_host_alloc(&scsi_esp_template, privsize);
+	if (shost)
+		shost->module = owner;
+
+	return shost;
+}
+EXPORT_SYMBOL(__scsi_esp_host_alloc);
 
 static void esp_get_signalling(struct Scsi_Host *host)
 {
diff --git a/drivers/scsi/esp_scsi.h b/drivers/scsi/esp_scsi.h
index 84dcbe4..9309e08 100644
--- a/drivers/scsi/esp_scsi.h
+++ b/drivers/scsi/esp_scsi.h
@@ -544,9 +544,8 @@ struct esp {
 
 /* A front-end driver for the ESP chip should do the following in
  * it's device probe routine:
- * 1) Allocate the host and private area using scsi_host_alloc()
- *    with size 'sizeof(struct esp)'.  The first argument to
- *    scsi_host_alloc() should be &scsi_esp_template.
+ * 1) Allocate the host and private area using scsi_esp_host_alloc()
+ *    with size 'sizeof(struct esp)'.
  * 2) Set host->max_id as appropriate.
  * 3) Set esp->host to the scsi_host itself, and esp->dev
  *    to the device object pointer.
@@ -573,7 +572,11 @@ struct esp {
  * 13) Check scsi_esp_register() return value, release all resources
  *     if an error was returned.
  */
-extern struct scsi_host_template scsi_esp_template;
+extern struct Scsi_Host *__scsi_esp_host_alloc(int privsize,
+						struct module *owner);
+#define scsi_esp_host_alloc(privsize)	\
+	__scsi_esp_host_alloc(privsize, THIS_MODULE)
+
 extern int scsi_esp_register(struct esp *, struct device *);
 
 extern void scsi_esp_unregister(struct esp *);
diff --git a/drivers/scsi/jazz_esp.c b/drivers/scsi/jazz_esp.c
index 9aaa74e..9c268f7 100644
--- a/drivers/scsi/jazz_esp.c
+++ b/drivers/scsi/jazz_esp.c
@@ -131,13 +131,12 @@ static const struct esp_driver_ops jazz_esp_ops = {
 
 static int esp_jazz_probe(struct platform_device *dev)
 {
-	struct scsi_host_template *tpnt = &scsi_esp_template;
 	struct Scsi_Host *host;
 	struct esp *esp;
 	struct resource *res;
 	int err;
 
-	host = scsi_host_alloc(tpnt, sizeof(struct esp));
+	host = scsi_esp_host_alloc(sizeof(struct esp));
 
 	err = -ENOMEM;
 	if (!host)
diff --git a/drivers/scsi/mac_esp.c b/drivers/scsi/mac_esp.c
index 14c0334..ad4ad71 100644
--- a/drivers/scsi/mac_esp.c
+++ b/drivers/scsi/mac_esp.c
@@ -483,7 +483,6 @@ static struct esp_driver_ops mac_esp_ops = {
 
 static int esp_mac_probe(struct platform_device *dev)
 {
-	struct scsi_host_template *tpnt = &scsi_esp_template;
 	struct Scsi_Host *host;
 	struct esp *esp;
 	int err;
@@ -495,7 +494,7 @@ static int esp_mac_probe(struct platform_device *dev)
 	if (dev->id > 1)
 		return -ENODEV;
 
-	host = scsi_host_alloc(tpnt, sizeof(struct esp));
+	host = scsi_esp_host_alloc(sizeof(struct esp));
 
 	err = -ENOMEM;
 	if (!host)
diff --git a/drivers/scsi/sun3x_esp.c b/drivers/scsi/sun3x_esp.c
index e26e81d..adb4368 100644
--- a/drivers/scsi/sun3x_esp.c
+++ b/drivers/scsi/sun3x_esp.c
@@ -196,13 +196,12 @@ static const struct esp_driver_ops sun3x_esp_ops = {
 
 static int esp_sun3x_probe(struct platform_device *dev)
 {
-	struct scsi_host_template *tpnt = &scsi_esp_template;
 	struct Scsi_Host *host;
 	struct esp *esp;
 	struct resource *res;
 	int err = -ENOMEM;
 
-	host = scsi_host_alloc(tpnt, sizeof(struct esp));
+	host = scsi_esp_host_alloc(sizeof(struct esp));
 	if (!host)
 		goto fail;
 
diff --git a/drivers/scsi/sun_esp.c b/drivers/scsi/sun_esp.c
index 7b6d4c2..1449bea 100644
--- a/drivers/scsi/sun_esp.c
+++ b/drivers/scsi/sun_esp.c
@@ -489,12 +489,11 @@ static const struct esp_driver_ops sbus_esp_ops = {
 static int esp_sbus_probe_one(struct platform_device *op,
 			      struct platform_device *espdma, int hme)
 {
-	struct scsi_host_template *tpnt = &scsi_esp_template;
 	struct Scsi_Host *host;
 	struct esp *esp;
 	int err;
 
-	host = scsi_host_alloc(tpnt, sizeof(struct esp));
+	host = scsi_esp_host_alloc(sizeof(struct esp));
 
 	err = -ENOMEM;
 	if (!host)
-- 
1.9.1

Re: [PATCH v6 0/4] scsi: ufs & ums-* & esp_scsi: fix module reference counting

[James Bottomley]

On Mon, 2015-05-04 at 23:46 +0900, Akinobu Mita wrote:
> While accessing a scsi_device, the use count of the underlying LLDD module
> is incremented.  The module reference is retrieved through .module field of
> struct scsi_host_template.
> 
> This mapping between scsi_device and underlying LLDD module works well
> except ufs, unusual usb storage drivers, and sub drivers for esp_scsi.
> These drivers consist with core driver and actual LLDDs, and
> scsi_host_template is defined in the core driver.  So the actual LLDDs can
> be unloaded even if the scsi_device is being accessed.
> 
> This patch series first adds ability to adjust module reference for
> scsi host by LLDDs and then fixes actual LLDDs by adjusting module
> reference after scsi host allocation.

This series is still missing an ack from ufs.  However, as we're on
series 6, final warning: object now or I'll take it without ufs ack.

However, it does also strike me that these three drivers have problems
because they're using the wrong initialisation pattern: the template is
supposed to be in the bus connector for compound drivers not in the
core.  To see how the pattern is supposed to work look at the 53c700
series of drivers.  They have a tiny template in the bus connector part
and then the function NCR_700_detect() fills in the rest.

53c700 is possibly not the best example because it fills in the template
for every host rather than once at module init time, but you get the
idea.  how difficult would it be to convert these things to use the
correct pattern rather than hacking it in the mid layer?

James

Re: [PATCH v6 0/4] scsi: ufs & ums-* & esp_scsi: fix module reference counting

[Hannes Reinecke]

On 05/04/2015 05:15 PM, James Bottomley wrote:
> On Mon, 2015-05-04 at 23:46 +0900, Akinobu Mita wrote:
>> While accessing a scsi_device, the use count of the underlying LLDD module
>> is incremented.  The module reference is retrieved through .module field of
>> struct scsi_host_template.
>>
>> This mapping between scsi_device and underlying LLDD module works well
>> except ufs, unusual usb storage drivers, and sub drivers for esp_scsi.
>> These drivers consist with core driver and actual LLDDs, and
>> scsi_host_template is defined in the core driver.  So the actual LLDDs can
>> be unloaded even if the scsi_device is being accessed.
>>
>> This patch series first adds ability to adjust module reference for
>> scsi host by LLDDs and then fixes actual LLDDs by adjusting module
>> reference after scsi host allocation.
> 
> This series is still missing an ack from ufs.  However, as we're on
> series 6, final warning: object now or I'll take it without ufs ack.
> 
> However, it does also strike me that these three drivers have problems
> because they're using the wrong initialisation pattern: the template is
> supposed to be in the bus connector for compound drivers not in the
> core.  To see how the pattern is supposed to work look at the 53c700
> series of drivers.  They have a tiny template in the bus connector part
> and then the function NCR_700_detect() fills in the rest.
> 
> 53c700 is possibly not the best example because it fills in the template
> for every host rather than once at module init time, but you get the
> idea.  how difficult would it be to convert these things to use the
> correct pattern rather than hacking it in the mid layer?
> 
Probably not _that_ difficult.
I'll give esp_scsi a shot.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare-l3A5Bk7waGM@public.gmane.org			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 0/4] scsi: ufs & ums-* & esp_scsi: fix module reference counting

[Alan Stern]

On Mon, 4 May 2015, James Bottomley wrote:

> However, it does also strike me that these three drivers have problems
> because they're using the wrong initialisation pattern: the template is
> supposed to be in the bus connector for compound drivers not in the
> core.

Why is it supposed to be done that way?  Isn't that less efficient?  It 
means you have to have a separate copy of the template for each bus 
connector driver, instead of letting them all share a common template 
in the core.

Alan Stern

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

Re: [PATCH v6 0/4] scsi: ufs & ums-* & esp_scsi: fix module reference counting

[James Bottomley]

On Mon, 2015-05-04 at 16:09 -0400, Alan Stern wrote:
> On Mon, 4 May 2015, James Bottomley wrote:
> 
> > However, it does also strike me that these three drivers have problems
> > because they're using the wrong initialisation pattern: the template is
> > supposed to be in the bus connector for compound drivers not in the
> > core.
> 
> Why is it supposed to be done that way?  Isn't that less efficient?  It 
> means you have to have a separate copy of the template for each bus 
> connector driver, instead of letting them all share a common template 
> in the core.

Well, no it doesn't.  The way 53c700 implements it is that there is a
common template in the core.  The drivers just initialise their variant
fields (for 53c700 it's name, proc_name and this_id) and the core fills
in the rest.  Admittedly wd33c93 doesn't quite get this right, that's
why I cited 53c700.

James

Re: [PATCH v6 0/4] scsi: ufs & ums-* & esp_scsi: fix module reference counting

[Akinobu Mita]

[-- Attachment #1: Type: text/plain, Size: 1247 bytes --]

2015-05-05 6:41 GMT+09:00 James Bottomley
<James.Bottomley-JuX6DAaQMKPCXq6kfMZ53/egYHeGw8Jk@public.gmane.org>:
> On Mon, 2015-05-04 at 16:09 -0400, Alan Stern wrote:
>> On Mon, 4 May 2015, James Bottomley wrote:
>>
>> > However, it does also strike me that these three drivers have problems
>> > because they're using the wrong initialisation pattern: the template is
>> > supposed to be in the bus connector for compound drivers not in the
>> > core.
>>
>> Why is it supposed to be done that way?  Isn't that less efficient?  It
>> means you have to have a separate copy of the template for each bus
>> connector driver, instead of letting them all share a common template
>> in the core.
>
> Well, no it doesn't.  The way 53c700 implements it is that there is a
> common template in the core.  The drivers just initialise their variant
> fields (for 53c700 it's name, proc_name and this_id) and the core fills
> in the rest.  Admittedly wd33c93 doesn't quite get this right, that's
> why I cited 53c700.

I have converted usb-storage and ums-alauda in the attached patch.
Each ums-* driver needs to expand module_usb_driver() macro as we need
to initialize their scsi host template in module_init().

Alan, how do you feel the change like this?

[-- Attachment #2: 0001-usb-storage-fix-module-reference-for-scsi-host.patch --]
[-- Type: text/x-patch, Size: 6268 bytes --]

From 4c6aa0b56c153afe331a98969a857f16d99f96b6 Mon Sep 17 00:00:00 2001
From: Akinobu Mita <akinobu.mita@gmail.com>
Date: Tue, 5 May 2015 22:30:04 +0900
Subject: [PATCH] usb: storage: fix module reference  for scsi host

---
 drivers/usb/storage/alauda.c   | 21 +++++++++++++++++++--
 drivers/usb/storage/scsiglue.c | 12 +++++++++++-
 drivers/usb/storage/scsiglue.h |  3 ++-
 drivers/usb/storage/usb.c      | 25 +++++++++++++++++++++----
 drivers/usb/storage/usb.h      |  3 ++-
 5 files changed, 55 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/storage/alauda.c b/drivers/usb/storage/alauda.c
index 4b55ab6..0baa484 100644
--- a/drivers/usb/storage/alauda.c
+++ b/drivers/usb/storage/alauda.c
@@ -42,6 +42,7 @@
 #include "transport.h"
 #include "protocol.h"
 #include "debug.h"
+#include "scsiglue.h"
 
 MODULE_DESCRIPTION("Driver for Alauda-based card readers");
 MODULE_AUTHOR("Daniel Drake <dsd@gentoo.org>");
@@ -1232,6 +1233,8 @@ static int alauda_transport(struct scsi_cmnd *srb, struct us_data *us)
 	return USB_STOR_TRANSPORT_FAILED;
 }
 
+static struct scsi_host_template alauda_host_template;
+
 static int alauda_probe(struct usb_interface *intf,
 			 const struct usb_device_id *id)
 {
@@ -1239,7 +1242,8 @@ static int alauda_probe(struct usb_interface *intf,
 	int result;
 
 	result = usb_stor_probe1(&us, intf, id,
-			(id - alauda_usb_ids) + alauda_unusual_dev_list);
+			(id - alauda_usb_ids) + alauda_unusual_dev_list,
+			&alauda_host_template);
 	if (result)
 		return result;
 
@@ -1266,4 +1270,17 @@ static struct usb_driver alauda_driver = {
 	.no_dynamic_id = 1,
 };
 
-module_usb_driver(alauda_driver);
+static int __init alauda_driver_init(void)
+{
+	usb_stor_host_template_init(&alauda_host_template, "ums-alauda",
+				    THIS_MODULE);
+
+	return usb_register(&alauda_driver);
+}
+module_init(alauda_driver_init);
+
+static void __exit alauda_driver_exit(void)
+{
+	usb_deregister(&alauda_driver);
+}
+module_exit(alauda_driver_exit);
diff --git a/drivers/usb/storage/scsiglue.c b/drivers/usb/storage/scsiglue.c
index 0e400f3..05b8fe2 100644
--- a/drivers/usb/storage/scsiglue.c
+++ b/drivers/usb/storage/scsiglue.c
@@ -540,7 +540,7 @@ static struct device_attribute *sysfs_device_attr_list[] = {
  * this defines our host template, with which we'll allocate hosts
  */
 
-struct scsi_host_template usb_stor_host_template = {
+static struct scsi_host_template usb_stor_host_template = {
 	/* basic userland interface stuff */
 	.name =				"usb-storage",
 	.proc_name =			"usb-storage",
@@ -592,6 +592,16 @@ struct scsi_host_template usb_stor_host_template = {
 	.module =			THIS_MODULE
 };
 
+void usb_stor_host_template_init(struct scsi_host_template *sht,
+				 const char *name, struct module *owner)
+{
+	memcpy(sht, &usb_stor_host_template, sizeof(*sht));
+	sht->name = name;
+	sht->proc_name = name;
+	sht->module = owner;
+}
+EXPORT_SYMBOL_GPL(usb_stor_host_template_init);
+
 /* To Report "Illegal Request: Invalid Field in CDB */
 unsigned char usb_stor_sense_invalidCDB[18] = {
 	[0]	= 0x70,			    /* current error */
diff --git a/drivers/usb/storage/scsiglue.h b/drivers/usb/storage/scsiglue.h
index ffa1cca..1909601 100644
--- a/drivers/usb/storage/scsiglue.h
+++ b/drivers/usb/storage/scsiglue.h
@@ -43,6 +43,7 @@ extern void usb_stor_report_device_reset(struct us_data *us);
 extern void usb_stor_report_bus_reset(struct us_data *us);
 
 extern unsigned char usb_stor_sense_invalidCDB[18];
-extern struct scsi_host_template usb_stor_host_template;
+extern void usb_stor_host_template_init(struct scsi_host_template *sht,
+					const char *name, struct module *owner);
 
 #endif
diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c
index 6c10c88..0f76899 100644
--- a/drivers/usb/storage/usb.c
+++ b/drivers/usb/storage/usb.c
@@ -924,7 +924,8 @@ static unsigned int usb_stor_sg_tablesize(struct usb_interface *intf)
 int usb_stor_probe1(struct us_data **pus,
 		struct usb_interface *intf,
 		const struct usb_device_id *id,
-		struct us_unusual_dev *unusual_dev)
+		struct us_unusual_dev *unusual_dev,
+		struct scsi_host_template *sht)
 {
 	struct Scsi_Host *host;
 	struct us_data *us;
@@ -936,7 +937,7 @@ int usb_stor_probe1(struct us_data **pus,
 	 * Ask the SCSI layer to allocate a host structure, with extra
 	 * space at the end for our private us_data structure.
 	 */
-	host = scsi_host_alloc(&usb_stor_host_template, sizeof(*us));
+	host = scsi_host_alloc(sht, sizeof(*us));
 	if (!host) {
 		dev_warn(&intf->dev, "Unable to allocate the scsi host\n");
 		return -ENOMEM;
@@ -1073,6 +1074,8 @@ void usb_stor_disconnect(struct usb_interface *intf)
 }
 EXPORT_SYMBOL_GPL(usb_stor_disconnect);
 
+static struct scsi_host_template usb_stor_host_template;
+
 /* The main probe routine for standard devices */
 static int storage_probe(struct usb_interface *intf,
 			 const struct usb_device_id *id)
@@ -1113,7 +1116,8 @@ static int storage_probe(struct usb_interface *intf,
 			id->idVendor, id->idProduct);
 	}
 
-	result = usb_stor_probe1(&us, intf, id, unusual_dev);
+	result = usb_stor_probe1(&us, intf, id, unusual_dev,
+				 &usb_stor_host_template);
 	if (result)
 		return result;
 
@@ -1137,4 +1141,17 @@ static struct usb_driver usb_storage_driver = {
 	.soft_unbind =	1,
 };
 
-module_usb_driver(usb_storage_driver);
+static int __init usb_storage_driver_init(void)
+{
+	usb_stor_host_template_init(&usb_stor_host_template, "usb-storage",
+				    THIS_MODULE);
+
+	return usb_register(&usb_storage_driver);
+}
+module_init(usb_storage_driver_init);
+
+static void __exit usb_storage_driver_exit(void)
+{
+	usb_deregister(&usb_storage_driver);
+}
+module_exit(usb_storage_driver_exit);
diff --git a/drivers/usb/storage/usb.h b/drivers/usb/storage/usb.h
index 307e339..a00ec4f 100644
--- a/drivers/usb/storage/usb.h
+++ b/drivers/usb/storage/usb.h
@@ -197,7 +197,8 @@ extern int usb_stor_post_reset(struct usb_interface *iface);
 extern int usb_stor_probe1(struct us_data **pus,
 		struct usb_interface *intf,
 		const struct usb_device_id *id,
-		struct us_unusual_dev *unusual_dev);
+		struct us_unusual_dev *unusual_dev,
+		struct scsi_host_template *sht);
 extern int usb_stor_probe2(struct us_data *us);
 extern void usb_stor_disconnect(struct usb_interface *intf);
 
-- 
1.9.1

Re: [PATCH v6 0/4] scsi: ufs & ums-* & esp_scsi: fix module reference counting

[James Bottomley]

On Tue, 2015-05-05 at 22:39 +0900, Akinobu Mita wrote:
> 2015-05-05 6:41 GMT+09:00 James Bottomley
> <James.Bottomley@hansenpartnership.com>:
> > On Mon, 2015-05-04 at 16:09 -0400, Alan Stern wrote:
> >> On Mon, 4 May 2015, James Bottomley wrote:
> >>
> >> > However, it does also strike me that these three drivers have problems
> >> > because they're using the wrong initialisation pattern: the template is
> >> > supposed to be in the bus connector for compound drivers not in the
> >> > core.
> >>
> >> Why is it supposed to be done that way?  Isn't that less efficient?  It
> >> means you have to have a separate copy of the template for each bus
> >> connector driver, instead of letting them all share a common template
> >> in the core.
> >
> > Well, no it doesn't.  The way 53c700 implements it is that there is a
> > common template in the core.  The drivers just initialise their variant
> > fields (for 53c700 it's name, proc_name and this_id) and the core fills
> > in the rest.  Admittedly wd33c93 doesn't quite get this right, that's
> > why I cited 53c700.
> 
> I have converted usb-storage and ums-alauda in the attached patch.
> Each ums-* driver needs to expand module_usb_driver() macro as we need
> to initialize their scsi host template in module_init().

Actually, I was more thinking something like this (mirroring 53c700).
It hides more of the internals rather than exposing them.

James

---

diff --git a/drivers/usb/storage/alauda.c b/drivers/usb/storage/alauda.c
index 4b55ab6..6cfc5c2 100644
--- a/drivers/usb/storage/alauda.c
+++ b/drivers/usb/storage/alauda.c
@@ -1232,6 +1232,12 @@ static int alauda_transport(struct scsi_cmnd *srb, struct us_data *us)
 	return USB_STOR_TRANSPORT_FAILED;
 }
 
+static struct scsi_host_template aluda_template = {
+	.name = "aluda",
+	.proc_name = "aluda",
+	.module = THIS_MODULE,
+};
+
 static int alauda_probe(struct usb_interface *intf,
 			 const struct usb_device_id *id)
 {
@@ -1239,7 +1245,8 @@ static int alauda_probe(struct usb_interface *intf,
 	int result;
 
 	result = usb_stor_probe1(&us, intf, id,
-			(id - alauda_usb_ids) + alauda_unusual_dev_list);
+			(id - alauda_usb_ids) + alauda_unusual_dev_list,
+			&aluda_template);
 	if (result)
 		return result;
 
diff --git a/drivers/usb/storage/scsiglue.c b/drivers/usb/storage/scsiglue.c
index 0e400f3..3d84a41 100644
--- a/drivers/usb/storage/scsiglue.c
+++ b/drivers/usb/storage/scsiglue.c
@@ -539,58 +539,58 @@ static struct device_attribute *sysfs_device_attr_list[] = {
 /*
  * this defines our host template, with which we'll allocate hosts
  */
-
-struct scsi_host_template usb_stor_host_template = {
+void usb_stor_template_init(struct scsi_host_template *tpnt)
+{
 	/* basic userland interface stuff */
-	.name =				"usb-storage",
-	.proc_name =			"usb-storage",
-	.show_info =			show_info,
-	.write_info =			write_info,
-	.info =				host_info,
+	if (!tpnt->name)
+		tpnt->name = "usb-storage";
+	if (!tpnt->proc_name)
+		tpnt->proc_name = "usb-storage";
+
+	tpnt->show_info = show_info;
+	tpnt->write_info = write_info;
+	tpnt->info = host_info;
 
 	/* command interface -- queued only */
-	.queuecommand =			queuecommand,
+	tpnt->queuecommand = queuecommand;
 
 	/* error and abort handlers */
-	.eh_abort_handler =		command_abort,
-	.eh_device_reset_handler =	device_reset,
-	.eh_bus_reset_handler =		bus_reset,
+	tpnt->eh_abort_handler = command_abort;
+	tpnt->eh_device_reset_handler = device_reset;
+	tpnt->eh_bus_reset_handler = bus_reset;
 
 	/* queue commands only, only one command per LUN */
-	.can_queue =			1,
-	.cmd_per_lun =			1,
+	tpnt->can_queue = 1;
+	tpnt->cmd_per_lun = 1;
 
 	/* unknown initiator id */
-	.this_id =			-1,
+	tpnt->this_id = -1;
 
-	.slave_alloc =			slave_alloc,
-	.slave_configure =		slave_configure,
-	.target_alloc =			target_alloc,
+	tpnt->slave_alloc = slave_alloc;
+	tpnt->slave_configure = slave_configure;
+	tpnt->target_alloc = target_alloc;
 
 	/* lots of sg segments can be handled */
-	.sg_tablesize =			SCSI_MAX_SG_CHAIN_SEGMENTS,
+	tpnt->sg_tablesize = SCSI_MAX_SG_CHAIN_SEGMENTS;
 
 	/* limit the total size of a transfer to 120 KB */
-	.max_sectors =                  240,
+	tpnt->max_sectors =                   240;
 
 	/* merge commands... this seems to help performance, but
 	 * periodically someone should test to see which setting is more
 	 * optimal.
 	 */
-	.use_clustering =		1,
+	tpnt->use_clustering = 1;
 
 	/* emulated HBA */
-	.emulated =			1,
+	tpnt->emulated = 1;
 
 	/* we do our own delay after a device or bus reset */
-	.skip_settle_delay =		1,
+	tpnt->skip_settle_delay = 1;
 
 	/* sysfs device attributes */
-	.sdev_attrs =			sysfs_device_attr_list,
-
-	/* module management */
-	.module =			THIS_MODULE
-};
+	tpnt->sdev_attrs = sysfs_device_attr_list;
+}
 
 /* To Report "Illegal Request: Invalid Field in CDB */
 unsigned char usb_stor_sense_invalidCDB[18] = {
diff --git a/drivers/usb/storage/scsiglue.h b/drivers/usb/storage/scsiglue.h
index ffa1cca..ae0ed71 100644
--- a/drivers/usb/storage/scsiglue.h
+++ b/drivers/usb/storage/scsiglue.h
@@ -41,8 +41,8 @@
 
 extern void usb_stor_report_device_reset(struct us_data *us);
 extern void usb_stor_report_bus_reset(struct us_data *us);
+extern void usb_stor_template_init(struct scsi_host_template *tpnt);
 
 extern unsigned char usb_stor_sense_invalidCDB[18];
-extern struct scsi_host_template usb_stor_host_template;
 
 #endif
diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c
index 5600c33..020d7ed 100644
--- a/drivers/usb/storage/usb.c
+++ b/drivers/usb/storage/usb.c
@@ -920,7 +920,8 @@ static unsigned int usb_stor_sg_tablesize(struct usb_interface *intf)
 int usb_stor_probe1(struct us_data **pus,
 		struct usb_interface *intf,
 		const struct usb_device_id *id,
-		struct us_unusual_dev *unusual_dev)
+		struct us_unusual_dev *unusual_dev,
+		struct scsi_host_template *tpnt)
 {
 	struct Scsi_Host *host;
 	struct us_data *us;
@@ -928,11 +929,13 @@ int usb_stor_probe1(struct us_data **pus,
 
 	dev_info(&intf->dev, "USB Mass Storage device detected\n");
 
+
 	/*
 	 * Ask the SCSI layer to allocate a host structure, with extra
 	 * space at the end for our private us_data structure.
 	 */
-	host = scsi_host_alloc(&usb_stor_host_template, sizeof(*us));
+	usb_stor_template_init(tpnt);
+	host = scsi_host_alloc(tpnt, sizeof(*us));
 	if (!host) {
 		dev_warn(&intf->dev, "Unable to allocate the scsi host\n");
 		return -ENOMEM;
@@ -1069,6 +1072,10 @@ void usb_stor_disconnect(struct usb_interface *intf)
 }
 EXPORT_SYMBOL_GPL(usb_stor_disconnect);
 
+static struct scsi_host_template storage_template = {
+	.module = THIS_MODULE,
+};
+
 /* The main probe routine for standard devices */
 static int storage_probe(struct usb_interface *intf,
 			 const struct usb_device_id *id)
@@ -1109,7 +1116,7 @@ static int storage_probe(struct usb_interface *intf,
 			id->idVendor, id->idProduct);
 	}
 
-	result = usb_stor_probe1(&us, intf, id, unusual_dev);
+	result = usb_stor_probe1(&us, intf, id, unusual_dev, &storage_template);
 	if (result)
 		return result;
 
diff --git a/drivers/usb/storage/usb.h b/drivers/usb/storage/usb.h
index 307e339..918cee8 100644
--- a/drivers/usb/storage/usb.h
+++ b/drivers/usb/storage/usb.h
@@ -197,7 +197,8 @@ extern int usb_stor_post_reset(struct usb_interface *iface);
 extern int usb_stor_probe1(struct us_data **pus,
 		struct usb_interface *intf,
 		const struct usb_device_id *id,
-		struct us_unusual_dev *unusual_dev);
+		struct us_unusual_dev *unusual_dev,
+		struct scsi_host_template *tpnt);
 extern int usb_stor_probe2(struct us_data *us);
 extern void usb_stor_disconnect(struct usb_interface *intf);
 

Re: [PATCH v6 0/4] scsi: ufs & ums-* & esp_scsi: fix module reference counting

[Alan Stern]

On Mon, 4 May 2015, James Bottomley wrote:

> On Mon, 2015-05-04 at 16:09 -0400, Alan Stern wrote:
> > On Mon, 4 May 2015, James Bottomley wrote:
> > 
> > > However, it does also strike me that these three drivers have problems
> > > because they're using the wrong initialisation pattern: the template is
> > > supposed to be in the bus connector for compound drivers not in the
> > > core.
> > 
> > Why is it supposed to be done that way?  Isn't that less efficient?  It 
> > means you have to have a separate copy of the template for each bus 
> > connector driver, instead of letting them all share a common template 
> > in the core.
> 
> Well, no it doesn't.  The way 53c700 implements it is that there is a
> common template in the core.  The drivers just initialise their variant
> fields (for 53c700 it's name, proc_name and this_id) and the core fills
> in the rest.  Admittedly wd33c93 doesn't quite get this right, that's
> why I cited 53c700.

You seem to be agreeing with me, even though you think you are 
disagreeing.

	"... there is a common template in the core." -- that's one
	scsi_host_template structure.

	"The drivers just initialize their variant fields ... and the
	core fills in the rest." -- that's an additional
	scsi_host_template structure for each driver.

The total comes to N+1 scsi_host_templates, where N is the number of 
drivers.

Now consider the way usb-storage does things.

	There is a common template in the core.  That's one
	scsi_host_template structure.

	The drivers use the core's template.  They have _no_ variant
	fields other than .owner.  That's why I thought Akinobu's idea
	of putting the owner field in the Scsi_Host structure was a
	good idea.

The total comes to 1 scsi_host_template.  Is that not better than N+1?

Consider the patch Akinobu just posted.  In addition to a whole bunch 
of new code, it adds this:

> --- a/drivers/usb/storage/alauda.c
> +++ b/drivers/usb/storage/alauda.c
...
> @@ -1232,6 +1233,8 @@ static int alauda_transport(struct scsi_cmnd *srb, struct us_data *us)
>  	return USB_STOR_TRANSPORT_FAILED;
>  }
>  
> +static struct scsi_host_template alauda_host_template;

alauda.c was the only driver modified in that patch, and it gained an 
new scsi_host_template.

For the case where the variants are identical in all respects other 
than .owner, doesn't it make sense to allow them to share a single 
scsi_host_template?

The original design of the SCSI stack envisioned multiple drivers, each 
in control of multiple SCSI hosts.  The idea was that 
scsi_host_template would be associated with the driver and Scsi_Host 
with the individual host.

Now the kernel has evolved, and we have multiple drivers, some of which
contain multiple subdrivers, each in control of multiple SCSI hosts.  
In this situation we should be flexible enough to allow the
scsi_host_template to be associated with either the driver or the
subdriver (decision to be made by the driver).  When the only variant
field is .owner, it makes sense to associate the scsi_host_template
with the driver, not force it to be associated with the subdriver.

The cost is duplication of the .owner field in every Scsi_Host.  The 
savings is a reduction in the number of scsi_host_templates.

Alan Stern

Re: [PATCH v6 0/4] scsi: ufs & ums-* & esp_scsi: fix module reference counting

[James Bottomley]

On Tue, 2015-05-05 at 10:25 -0400, Alan Stern wrote:
> On Mon, 4 May 2015, James Bottomley wrote:
> 
> > On Mon, 2015-05-04 at 16:09 -0400, Alan Stern wrote:
> > > On Mon, 4 May 2015, James Bottomley wrote:
> > > 
> > > > However, it does also strike me that these three drivers have problems
> > > > because they're using the wrong initialisation pattern: the template is
> > > > supposed to be in the bus connector for compound drivers not in the
> > > > core.
> > > 
> > > Why is it supposed to be done that way?  Isn't that less efficient?  It 
> > > means you have to have a separate copy of the template for each bus 
> > > connector driver, instead of letting them all share a common template 
> > > in the core.
> > 
> > Well, no it doesn't.  The way 53c700 implements it is that there is a
> > common template in the core.  The drivers just initialise their variant
> > fields (for 53c700 it's name, proc_name and this_id) and the core fills
> > in the rest.  Admittedly wd33c93 doesn't quite get this right, that's
> > why I cited 53c700.
> 
> You seem to be agreeing with me, even though you think you are 
> disagreeing.
> 
> 	"... there is a common template in the core." -- that's one
> 	scsi_host_template structure.
> 
> 	"The drivers just initialize their variant fields ... and the
> 	core fills in the rest." -- that's an additional
> 	scsi_host_template structure for each driver.
> 
> The total comes to N+1 scsi_host_templates, where N is the number of 
> drivers.
> 
> Now consider the way usb-storage does things.
> 
> 	There is a common template in the core.  That's one
> 	scsi_host_template structure.
> 
> 	The drivers use the core's template.  They have _no_ variant
> 	fields other than .owner.  That's why I thought Akinobu's idea
> 	of putting the owner field in the Scsi_Host structure was a
> 	good idea.
> 
> The total comes to 1 scsi_host_template.  Is that not better than N+1?
> 
> Consider the patch Akinobu just posted.  In addition to a whole bunch 
> of new code, it adds this:
> 
> > --- a/drivers/usb/storage/alauda.c
> > +++ b/drivers/usb/storage/alauda.c
> ...
> > @@ -1232,6 +1233,8 @@ static int alauda_transport(struct scsi_cmnd *srb, struct us_data *us)
> >  	return USB_STOR_TRANSPORT_FAILED;
> >  }
> >  
> > +static struct scsi_host_template alauda_host_template;
> 
> alauda.c was the only driver modified in that patch, and it gained an 
> new scsi_host_template.
> 
> For the case where the variants are identical in all respects other 
> than .owner, doesn't it make sense to allow them to share a single 
> scsi_host_template?
> 
> The original design of the SCSI stack envisioned multiple drivers, each 
> in control of multiple SCSI hosts.  The idea was that 
> scsi_host_template would be associated with the driver and Scsi_Host 
> with the individual host.
> 
> Now the kernel has evolved, and we have multiple drivers, some of which
> contain multiple subdrivers, each in control of multiple SCSI hosts.  
> In this situation we should be flexible enough to allow the
> scsi_host_template to be associated with either the driver or the
> subdriver (decision to be made by the driver).  When the only variant
> field is .owner, it makes sense to associate the scsi_host_template
> with the driver, not force it to be associated with the subdriver.
> 
> The cost is duplication of the .owner field in every Scsi_Host.  The 
> savings is a reduction in the number of scsi_host_templates.

So your essential objection is the host template duplication?  I know
it's a couple of hundred bytes, but surely its dwarfed by all the other
stuff you have to duplicate ... the module size of each of these is
around 0.25MB, so a couple of hundred bytes would seem a bit
insignificant.

James

Re: [PATCH v6 0/4] scsi: ufs & ums-* & esp_scsi: fix module reference counting

[Alan Stern]

On Tue, 5 May 2015, James Bottomley wrote:

> > The cost is duplication of the .owner field in every Scsi_Host.  The 
> > savings is a reduction in the number of scsi_host_templates.
> 
> So your essential objection is the host template duplication?  I know
> it's a couple of hundred bytes, but surely its dwarfed by all the other
> stuff you have to duplicate ... the module size of each of these is
> around 0.25MB, so a couple of hundred bytes would seem a bit
> insignificant.

Agreed, the size is relatively insignificant.  I drop any objection on
that account.  (But turning things around, what's wrong with
duplicating the .owner field in the Scsi_Host structure when we're
already duplicating this_id, can_queue, sg_tablesize, and a large bunch
of others?)

On the other hand, your code to populate the template copies is a mess.  
I like Akinobu's approach much better, although I would change a few
details:

	Declare usb_stor_host_template as "const", and define a new, 
	separate usb_stor_template in usb.c for actual use.

	In usb_stor_host_template_init(), do a plain structure
	assignment rather than memcpy.

Alan Stern

Re: [PATCH v6 0/4] scsi: ufs & ums-* & esp_scsi: fix module reference counting

[James Bottomley]

On Tue, 2015-05-05 at 15:14 -0400, Alan Stern wrote:
> On Tue, 5 May 2015, James Bottomley wrote:
> 
> > > The cost is duplication of the .owner field in every Scsi_Host.  The 
> > > savings is a reduction in the number of scsi_host_templates.
> > 
> > So your essential objection is the host template duplication?  I know
> > it's a couple of hundred bytes, but surely its dwarfed by all the other
> > stuff you have to duplicate ... the module size of each of these is
> > around 0.25MB, so a couple of hundred bytes would seem a bit
> > insignificant.
> 
> Agreed, the size is relatively insignificant.  I drop any objection on
> that account.  (But turning things around, what's wrong with
> duplicating the .owner field in the Scsi_Host structure when we're
> already duplicating this_id, can_queue, sg_tablesize, and a large bunch
> of others?)

It's really only the pattern wrongness.  However, as Rusty says (at
length) the best APIs are those that you can't get wrong and having the
owner in the template gives that (it's a Level 10 interface).
Duplicating the owner in the host gets us to the point where following
convention you get it right but nothing prevents abuse (it's a Level 4
interface).  I mean, it could be worse (his scale goes down to -10) but
I'd rather not move down if we don't have to.

> On the other hand, your code to populate the template copies is a mess.  
> I like Akinobu's approach much better

Unfortunately with that approach, values in the host template can't be
overridden.

> , although I would change a few
> details:
> 
> 	Declare usb_stor_host_template as "const", and define a new, 
> 	separate usb_stor_template in usb.c for actual use.
> 
> 	In usb_stor_host_template_init(), do a plain structure
> 	assignment rather than memcpy.

OK, so I'm happy with that: it solves the "can't be overridden" problem
if you mean a driver should 

declare a local template

call usb_stor_host_template_init() on the local template

override whatever they want (or nothing)

call usb_stor_probe1 with the new template.

I also note that you can

#define usb_stor_host_template_init(t, n) __usb_stor_host_template_init((t), (n), THIS_MODULE)

And make it just work for normal usage.

James

Re: [PATCH v6 0/4] scsi: ufs & ums-* & esp_scsi: fix module reference counting

[Akinobu Mita]

2015-05-06 6:42 GMT+09:00 James Bottomley
<James.Bottomley@hansenpartnership.com>:
> On Tue, 2015-05-05 at 15:14 -0400, Alan Stern wrote:
>> On Tue, 5 May 2015, James Bottomley wrote:
>>
>> > > The cost is duplication of the .owner field in every Scsi_Host.  The
>> > > savings is a reduction in the number of scsi_host_templates.
>> >
>> > So your essential objection is the host template duplication?  I know
>> > it's a couple of hundred bytes, but surely its dwarfed by all the other
>> > stuff you have to duplicate ... the module size of each of these is
>> > around 0.25MB, so a couple of hundred bytes would seem a bit
>> > insignificant.
>>
>> Agreed, the size is relatively insignificant.  I drop any objection on
>> that account.  (But turning things around, what's wrong with
>> duplicating the .owner field in the Scsi_Host structure when we're
>> already duplicating this_id, can_queue, sg_tablesize, and a large bunch
>> of others?)
>
> It's really only the pattern wrongness.  However, as Rusty says (at
> length) the best APIs are those that you can't get wrong and having the
> owner in the template gives that (it's a Level 10 interface).
> Duplicating the owner in the host gets us to the point where following
> convention you get it right but nothing prevents abuse (it's a Level 4
> interface).  I mean, it could be worse (his scale goes down to -10) but
> I'd rather not move down if we don't have to.
>
>> On the other hand, your code to populate the template copies is a mess.
>> I like Akinobu's approach much better
>
> Unfortunately with that approach, values in the host template can't be
> overridden.
>
>> , although I would change a few
>> details:
>>
>>       Declare usb_stor_host_template as "const", and define a new,
>>       separate usb_stor_template in usb.c for actual use.
>>
>>       In usb_stor_host_template_init(), do a plain structure
>>       assignment rather than memcpy.
>
> OK, so I'm happy with that: it solves the "can't be overridden" problem
> if you mean a driver should
>
> declare a local template
>
> call usb_stor_host_template_init() on the local template
>
> override whatever they want (or nothing)
>
> call usb_stor_probe1 with the new template.
>
> I also note that you can
>
> #define usb_stor_host_template_init(t, n) __usb_stor_host_template_init((t), (n), THIS_MODULE)
>
> And make it just work for normal usage.

I have just posted a formal patch.  usb_stor_host_template_init() is
now called in the newly added module_usb_stor_driver() helper macro
which is almost same as module_usb_driver() so that it doesn't need to
specify THIS_MODULE for each call.