[PATCH v2 0/3] scsi: ufs & ums-*: fix module reference counting

[PATCH v2 0/3] scsi: ufs & ums-*: 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 some scsi drivers (ufs and unusual usb storage drivers).  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 ufs and unusual usb storage drivers
by adjusting module reference after scsi host allocation.

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

Akinobu Mita (3):
  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

 drivers/scsi/hosts.c             | 1 +
 drivers/scsi/scsi.c              | 4 ++--
 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 +
 8 files changed, 16 insertions(+), 8 deletions(-)

Cc: Vinayak Holikatti <vinholikatti-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Dolev Raviv <draviv-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
Cc: Sujit Reddy Thumma <sthumma-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
Cc: Subhash Jadavani <subhashj-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
Cc: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
Cc: "James E.J. Bottomley" <JBottomley-bzQdu9zFT3WakBO8gow8eQ@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: linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: usb-storage-ijkIwGHArpdIPJnuZ7Njw4oP9KaGy4wf@public.gmane.org
Cc: linux-scsi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
-- 
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 v2 1/3] 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 some scsi drivers (ufs and unusual usb storage drivers).  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>
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: linux-usb@vger.kernel.org
Cc: usb-storage@lists.one-eyed-alien.net
Cc: linux-scsi@vger.kernel.org
---
No change from v1

 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 e028854..5905b83 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -988,7 +988,7 @@ int scsi_device_get(struct scsi_device *sdev)
 		return -ENXIO;
 	/* We can fail this if we're doing SCSI operations
 	 * from module exit (like cache flush) */
-	try_module_get(sdev->host->hostt->module);
+	try_module_get(sdev->host->module);
 
 	return 0;
 }
@@ -1005,7 +1005,7 @@ EXPORT_SYMBOL(scsi_device_get);
 void scsi_device_put(struct scsi_device *sdev)
 {
 #ifdef CONFIG_MODULE_UNLOAD
-	struct module *module = sdev->host->hostt->module;
+	struct module *module = sdev->host->module;
 
 	/* The module refcount will be zero if scsi_device_get()
 	 * was called from a module removal routine */
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index 019e668..5133f2f 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -617,6 +617,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 v2 2/3] 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
---
No change from v1

 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 95b64e0..ea3ca99 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 2e26025..f0aff90 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -4202,7 +4202,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 v2 3/3] 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() to
adjust it after scsi host allocation.

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.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: Christoph Hellwig <hch@lst.de>
Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
Cc: linux-usb@vger.kernel.org
Cc: usb-storage@lists.one-eyed-alien.net
Cc: linux-scsi@vger.kernel.org
---
* v2:
- Pass correct module reference to usb_stor_probe1() instead of touching
  all ums-* drivers, suggested by Alan Stern

 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 d468d02..3bb2558 100644
--- a/drivers/usb/storage/usb.c
+++ b/drivers/usb/storage/usb.c
@@ -911,10 +911,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;
@@ -937,6 +938,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);
@@ -969,7 +971,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

Re: [PATCH v2 0/3] scsi: ufs & ums-*: fix module reference counting

[Christoph Hellwig]

On Wed, Jan 07, 2015 at 11:02:59PM +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 some scsi drivers (ufs and unusual usb storage drivers).  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.

Why don't ufs and usb-storage define the host templates in the sub drivers?
That's what libata or the mpt fusion driver do.

Re: [PATCH v2 3/3] usb: storage: adjust module reference for scsi host

[Alan Stern]

On Wed, 7 Jan 2015, Akinobu Mita wrote:

> 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() to
> adjust it after scsi host allocation.
> 
> Signed-off-by: Akinobu Mita <akinobu.mita-Re5JQEeQqe8AvxtiuMwx3w@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
> ---
> * v2:
> - Pass correct module reference to usb_stor_probe1() instead of touching
>   all ums-* drivers, suggested by Alan Stern

Acked-by: Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org>

--
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 v2 0/3] scsi: ufs & ums-*: fix module reference counting

[Alan Stern]

On Wed, 7 Jan 2015, Christoph Hellwig wrote:

> On Wed, Jan 07, 2015 at 11:02:59PM +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 some scsi drivers (ufs and unusual usb storage drivers).  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.
> 
> Why don't ufs and usb-storage define the host templates in the sub drivers?
> That's what libata or the mpt fusion driver do.

Originally the subdrivers were all part of usb-storage.  When they were
split out into separate modules, there didn't seem to be any need to
copy the host template into each one.  (Especially since some of the
method pointers in the host template refer to routines in usb-storage
itself, not in the subdrivers.)  The fact that the .module member was
no longer adequate escaped our notice at the time.

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 v2 0/3] scsi: ufs & ums-*: fix module reference counting

[Akinobu Mita]

2015-01-08 0:09 GMT+09:00 Alan Stern <stern@rowland.harvard.edu>:
> On Wed, 7 Jan 2015, Christoph Hellwig wrote:
>
>> On Wed, Jan 07, 2015 at 11:02:59PM +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 some scsi drivers (ufs and unusual usb storage drivers).  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.
>>
>> Why don't ufs and usb-storage define the host templates in the sub drivers?
>> That's what libata or the mpt fusion driver do.
>
> Originally the subdrivers were all part of usb-storage.  When they were
> split out into separate modules, there didn't seem to be any need to
> copy the host template into each one.  (Especially since some of the
> method pointers in the host template refer to routines in usb-storage
> itself, not in the subdrivers.)  The fact that the .module member was
> no longer adequate escaped our notice at the time.

For ufs, the reason why a host template is shared by sub drivers is similar
to usb-storage.  Originally there was only a single pci driver for ufs.
When platform driver for ufs was introduced, it was split into core driver
(ufshcd) and sub drivers (ufshcd-pci and ufshcd-pltfrm).  They require
exactly the same host template.

Is there any other preferable way to fix this problem?  The first time I
found this in ufs driver, I was trying to fix it by merging core and sub
drivers into a single compound module.  But I realized that usm-* drivers
have the same problem, then I decided to propose what this patch series do
instead.

Re: [PATCH v2 0/3] scsi: ufs & ums-*: fix module reference counting

[Alan Stern]

On Thu, 8 Jan 2015, Akinobu Mita wrote:

> >> Why don't ufs and usb-storage define the host templates in the sub drivers?
> >> That's what libata or the mpt fusion driver do.
> >
> > Originally the subdrivers were all part of usb-storage.  When they were
> > split out into separate modules, there didn't seem to be any need to
> > copy the host template into each one.  (Especially since some of the
> > method pointers in the host template refer to routines in usb-storage
> > itself, not in the subdrivers.)  The fact that the .module member was
> > no longer adequate escaped our notice at the time.
> 
> For ufs, the reason why a host template is shared by sub drivers is similar
> to usb-storage.  Originally there was only a single pci driver for ufs.
> When platform driver for ufs was introduced, it was split into core driver
> (ufshcd) and sub drivers (ufshcd-pci and ufshcd-pltfrm).  They require
> exactly the same host template.
> 
> Is there any other preferable way to fix this problem?  The first time I
> found this in ufs driver, I was trying to fix it by merging core and sub
> drivers into a single compound module.  But I realized that usm-* drivers
> have the same problem, then I decided to propose what this patch series do
> instead.

I can't think of any other ways to fix it.  The only options seem to be
to duplicate the host template in each subdriver or to add the .module
field to the host structure.  I prefer the second option because it
doesn't involve so much code duplication.

Alan Stern