[PATCH v4 00/11] scsi: fix module reference mismatch for scsi host

[PATCH v4 00/11] scsi: fix module reference mismatch 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 drivers which consist with the core driver and the actual
LLDDs and scsi_host_template is defined in the core driver.  In these
cases, the actual LLDDs can be unloaded even if the scsi_device is
being accessed.

This patch series fixes the module reference mismatch problem for
ufs, usb-storage, esp_scsi, ahci_platform, and pata_platform drivers
by moving owner module reference field from struct scsi_host_template
to struct Scsi_Host and allowing the LLDDs to set their correct module
reference.

* 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 (11):
  ata: prepare to move module reference from scsi_host_template to
    Scsi_Host
  iscsi: prepare to move module reference from scsi_host_template to
    Scsi_Host
  cxgbi: prepare to move module reference from scsi_host_template to
    Scsi_Host
  libfc: prepare to move module reference from scsi_host_template to
    Scsi_Host
  53c700: prepare move module reference from scsi_host_template to
    Scsi_Host
  scsi: legacy: prepare to move module reference from scsi_host_template
    to Scsi_Host
  scsi: move module reference from scsi_host_template to Scsi_Host
  scsi: ufs: adjust module reference for scsi host
  usb: storage: adjust module reference for scsi host
  ata: ahci_platform: adjust module reference for scsi host
  ata: pata_of_platform: adjust module reference for scsi host

 drivers/ata/libahci_platform.c | 14 +++++-----
 drivers/ata/libata-core.c      | 22 ++++++++-------
 drivers/ata/libata-scsi.c      |  2 +-
 drivers/ata/libata-sff.c       | 61 +++++++++++++++++++++++-------------------
 drivers/ata/pata_platform.c    | 18 +++++++------
 drivers/scsi/53c700.c          |  9 ++++---
 drivers/scsi/53c700.h          |  7 +++--
 drivers/scsi/cxgbi/libcxgbi.c  |  8 +++---
 drivers/scsi/cxgbi/libcxgbi.h  |  6 +++--
 drivers/scsi/hosts.c           | 16 ++++++-----
 drivers/scsi/libfc/fc_lport.c  | 26 ++++++++++++++++++
 drivers/scsi/libfc/fc_npiv.c   |  2 +-
 drivers/scsi/libiscsi.c        | 12 +++++----
 drivers/scsi/scsi.c            |  4 +--
 drivers/scsi/ufs/ufshcd.c      | 13 ++++-----
 drivers/scsi/ufs/ufshcd.h      |  5 +++-
 drivers/usb/storage/scsiglue.c |  3 ---
 drivers/usb/storage/usb.c      |  9 ++++---
 drivers/usb/storage/usb.h      |  7 +++--
 include/linux/ahci_platform.h  |  9 ++++---
 include/linux/ata_platform.h   | 16 ++++++-----
 include/linux/libata.h         | 52 ++++++++++++++++++++++++-----------
 include/scsi/libfc.h           | 27 +++----------------
 include/scsi/libiscsi.h        |  9 ++++---
 include/scsi/scsi_host.h       | 13 +++++++--
 25 files changed, 224 insertions(+), 146 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: 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: Tejun Heo <tj@kernel.org>
Cc: Hans de Goede <hdegoede@redhat.com>
Cc: Mike Christie <michaelc@cs.wisc.edu>
Cc: Karen Xie <kxie@chelsio.com>
Cc: Robert Love <robert.w.love@intel.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
Cc: open-iscsi@googlegroups.com
Cc: fcoe-devel@open-fcoe.org
Cc: linux-ide@vger.kernel.org
Cc: linux-usb@vger.kernel.org
Cc: usb-storage@lists.one-eyed-alien.net
Cc: linux-scsi@vger.kernel.org
-- 
1.9.1

[PATCH v4 01/11] ata: prepare to move module reference from scsi_host_template to Scsi_Host

[Akinobu Mita]

In preparation for moving owner module reference field from struct
scsi_host_template to struct Scsi_Host, this converts the following
functions into macros so that LLDDs can pass THIS_MODULE to
scsi_host_alloc() through them instead of scsi_host_template->module.

ata_host_alloc()
ata_host_alloc_pinfo()
ata_pci_sff_prepare_host()
ata_pci_sff_init_one()
ata_pci_bmdma_prepare_host()
ata_pci_bmdma_init_one()

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Cc: Hans de Goede <hdegoede@redhat.com>
Cc: Tejun Heo <tj@kernel.org>
Cc: Christoph Hellwig <hch@lst.de>
Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
Cc: linux-ide@vger.kernel.org
Cc: linux-scsi@vger.kernel.org
---
 drivers/ata/libata-core.c | 22 ++++++++++-------
 drivers/ata/libata-sff.c  | 61 +++++++++++++++++++++++++----------------------
 include/linux/libata.h    | 52 ++++++++++++++++++++++++++++------------
 3 files changed, 83 insertions(+), 52 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 5c84fb5..a3cc0e3 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -5700,9 +5700,10 @@ static void ata_host_release(struct device *gendev, void *res)
 }
 
 /**
- *	ata_host_alloc - allocate and init basic ATA host resources
+ *	__ata_host_alloc - allocate and init basic ATA host resources
  *	@dev: generic device this host is associated with
  *	@max_ports: maximum number of ATA ports associated with this host
+ *	@owner: module which will be the owner of the ATA host
  *
  *	Allocate and initialize basic ATA host resources.  LLD calls
  *	this function to allocate a host, initializes it fully and
@@ -5719,7 +5720,8 @@ static void ata_host_release(struct device *gendev, void *res)
  *	LOCKING:
  *	Inherited from calling layer (may sleep).
  */
-struct ata_host *ata_host_alloc(struct device *dev, int max_ports)
+struct ata_host *__ata_host_alloc(struct device *dev, int max_ports,
+				  struct module *owner)
 {
 	struct ata_host *host;
 	size_t sz;
@@ -5744,6 +5746,7 @@ struct ata_host *ata_host_alloc(struct device *dev, int max_ports)
 	mutex_init(&host->eh_mutex);
 	host->dev = dev;
 	host->n_ports = max_ports;
+	host->module = owner;
 
 	/* allocate ports bound to this host */
 	for (i = 0; i < max_ports; i++) {
@@ -5766,10 +5769,11 @@ struct ata_host *ata_host_alloc(struct device *dev, int max_ports)
 }
 
 /**
- *	ata_host_alloc_pinfo - alloc host and init with port_info array
+ *	__ata_host_alloc_pinfo - alloc host and init with port_info array
  *	@dev: generic device this host is associated with
  *	@ppi: array of ATA port_info to initialize host with
  *	@n_ports: number of ATA ports attached to this host
+ *	@owner: module which will be the owner of the ATA host
  *
  *	Allocate ATA host and initialize with info from @ppi.  If NULL
  *	terminated, @ppi may contain fewer entries than @n_ports.  The
@@ -5781,15 +5785,15 @@ struct ata_host *ata_host_alloc(struct device *dev, int max_ports)
  *	LOCKING:
  *	Inherited from calling layer (may sleep).
  */
-struct ata_host *ata_host_alloc_pinfo(struct device *dev,
-				      const struct ata_port_info * const * ppi,
-				      int n_ports)
+struct ata_host *__ata_host_alloc_pinfo(struct device *dev,
+					const struct ata_port_info * const *ppi,
+					int n_ports, struct module *owner)
 {
 	const struct ata_port_info *pi;
 	struct ata_host *host;
 	int i, j;
 
-	host = ata_host_alloc(dev, n_ports);
+	host = __ata_host_alloc(dev, n_ports, owner);
 	if (!host)
 		return NULL;
 
@@ -6862,8 +6866,8 @@ EXPORT_SYMBOL_GPL(ata_dev_next);
 EXPORT_SYMBOL_GPL(ata_std_bios_param);
 EXPORT_SYMBOL_GPL(ata_scsi_unlock_native_capacity);
 EXPORT_SYMBOL_GPL(ata_host_init);
-EXPORT_SYMBOL_GPL(ata_host_alloc);
-EXPORT_SYMBOL_GPL(ata_host_alloc_pinfo);
+EXPORT_SYMBOL_GPL(__ata_host_alloc);
+EXPORT_SYMBOL_GPL(__ata_host_alloc_pinfo);
 EXPORT_SYMBOL_GPL(ata_slave_link_init);
 EXPORT_SYMBOL_GPL(ata_host_start);
 EXPORT_SYMBOL_GPL(ata_host_register);
diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
index db90aa3..323c9d8 100644
--- a/drivers/ata/libata-sff.c
+++ b/drivers/ata/libata-sff.c
@@ -2349,10 +2349,11 @@ int ata_pci_sff_init_host(struct ata_host *host)
 EXPORT_SYMBOL_GPL(ata_pci_sff_init_host);
 
 /**
- *	ata_pci_sff_prepare_host - helper to prepare PCI PIO-only SFF ATA host
+ *	__ata_pci_sff_prepare_host - helper to prepare PCI PIO-only SFF ATA host
  *	@pdev: target PCI device
  *	@ppi: array of port_info, must be enough for two ports
  *	@r_host: out argument for the initialized ATA host
+ *	@owner: module which will be the owner of the ATA host
  *
  *	Helper to allocate PIO-only SFF ATA host for @pdev, acquire
  *	all PCI resources and initialize it accordingly in one go.
@@ -2363,9 +2364,9 @@ EXPORT_SYMBOL_GPL(ata_pci_sff_init_host);
  *	RETURNS:
  *	0 on success, -errno otherwise.
  */
-int ata_pci_sff_prepare_host(struct pci_dev *pdev,
-			     const struct ata_port_info * const *ppi,
-			     struct ata_host **r_host)
+int __ata_pci_sff_prepare_host(struct pci_dev *pdev,
+			       const struct ata_port_info * const *ppi,
+			       struct ata_host **r_host, struct module *owner)
 {
 	struct ata_host *host;
 	int rc;
@@ -2373,7 +2374,7 @@ int ata_pci_sff_prepare_host(struct pci_dev *pdev,
 	if (!devres_open_group(&pdev->dev, NULL, GFP_KERNEL))
 		return -ENOMEM;
 
-	host = ata_host_alloc_pinfo(&pdev->dev, ppi, 2);
+	host = __ata_host_alloc_pinfo(&pdev->dev, ppi, 2, owner);
 	if (!host) {
 		dev_err(&pdev->dev, "failed to allocate ATA host\n");
 		rc = -ENOMEM;
@@ -2392,7 +2393,7 @@ err_out:
 	devres_release_group(&pdev->dev, NULL);
 	return rc;
 }
-EXPORT_SYMBOL_GPL(ata_pci_sff_prepare_host);
+EXPORT_SYMBOL_GPL(__ata_pci_sff_prepare_host);
 
 /**
  *	ata_pci_sff_activate_host - start SFF host, request IRQ and register it
@@ -2500,7 +2501,7 @@ static const struct ata_port_info *ata_sff_find_valid_pi(
 static int ata_pci_init_one(struct pci_dev *pdev,
 		const struct ata_port_info * const *ppi,
 		struct scsi_host_template *sht, void *host_priv,
-		int hflags, bool bmdma)
+		int hflags, bool bmdma, struct module *owner)
 {
 	struct device *dev = &pdev->dev;
 	const struct ata_port_info *pi;
@@ -2525,11 +2526,11 @@ static int ata_pci_init_one(struct pci_dev *pdev,
 #ifdef CONFIG_ATA_BMDMA
 	if (bmdma)
 		/* prepare and activate BMDMA host */
-		rc = ata_pci_bmdma_prepare_host(pdev, ppi, &host);
+		rc = __ata_pci_bmdma_prepare_host(pdev, ppi, &host, owner);
 	else
 #endif
 		/* prepare and activate SFF host */
-		rc = ata_pci_sff_prepare_host(pdev, ppi, &host);
+		rc = __ata_pci_sff_prepare_host(pdev, ppi, &host, owner);
 	if (rc)
 		goto out;
 	host->private_data = host_priv;
@@ -2552,12 +2553,13 @@ out:
 }
 
 /**
- *	ata_pci_sff_init_one - Initialize/register PIO-only PCI IDE controller
+ *	__ata_pci_sff_init_one - Initialize/register PIO-only PCI IDE controller
  *	@pdev: Controller to be initialized
  *	@ppi: array of port_info, must be enough for two ports
  *	@sht: scsi_host_template to use when registering the host
  *	@host_priv: host private_data
  *	@hflag: host flags
+ *	@owner: module which will be the owner of the ATA host
  *
  *	This is a helper function which can be called from a driver's
  *	xxx_init_one() probe function if the hardware uses traditional
@@ -2573,13 +2575,14 @@ out:
  *	RETURNS:
  *	Zero on success, negative on errno-based value on error.
  */
-int ata_pci_sff_init_one(struct pci_dev *pdev,
-		 const struct ata_port_info * const *ppi,
-		 struct scsi_host_template *sht, void *host_priv, int hflag)
+int __ata_pci_sff_init_one(struct pci_dev *pdev,
+			   const struct ata_port_info * const *ppi,
+			   struct scsi_host_template *sht, void *host_priv,
+			   int hflag, struct module *owner)
 {
-	return ata_pci_init_one(pdev, ppi, sht, host_priv, hflag, 0);
+	return ata_pci_init_one(pdev, ppi, sht, host_priv, hflag, 0, owner);
 }
-EXPORT_SYMBOL_GPL(ata_pci_sff_init_one);
+EXPORT_SYMBOL_GPL(__ata_pci_sff_init_one);
 
 #endif /* CONFIG_PCI */
 
@@ -3245,10 +3248,11 @@ void ata_pci_bmdma_init(struct ata_host *host)
 EXPORT_SYMBOL_GPL(ata_pci_bmdma_init);
 
 /**
- *	ata_pci_bmdma_prepare_host - helper to prepare PCI BMDMA ATA host
+ *	__ata_pci_bmdma_prepare_host - helper to prepare PCI BMDMA ATA host
  *	@pdev: target PCI device
  *	@ppi: array of port_info, must be enough for two ports
  *	@r_host: out argument for the initialized ATA host
+ *	@owner: module which will be the owner of the ATA host
  *
  *	Helper to allocate BMDMA ATA host for @pdev, acquire all PCI
  *	resources and initialize it accordingly in one go.
@@ -3259,28 +3263,29 @@ EXPORT_SYMBOL_GPL(ata_pci_bmdma_init);
  *	RETURNS:
  *	0 on success, -errno otherwise.
  */
-int ata_pci_bmdma_prepare_host(struct pci_dev *pdev,
-			       const struct ata_port_info * const * ppi,
-			       struct ata_host **r_host)
+int __ata_pci_bmdma_prepare_host(struct pci_dev *pdev,
+				 const struct ata_port_info * const *ppi,
+				 struct ata_host **r_host, struct module *owner)
 {
 	int rc;
 
-	rc = ata_pci_sff_prepare_host(pdev, ppi, r_host);
+	rc = __ata_pci_sff_prepare_host(pdev, ppi, r_host, owner);
 	if (rc)
 		return rc;
 
 	ata_pci_bmdma_init(*r_host);
 	return 0;
 }
-EXPORT_SYMBOL_GPL(ata_pci_bmdma_prepare_host);
+EXPORT_SYMBOL_GPL(__ata_pci_bmdma_prepare_host);
 
 /**
- *	ata_pci_bmdma_init_one - Initialize/register BMDMA PCI IDE controller
+ *	__ata_pci_bmdma_init_one - Initialize/register BMDMA PCI IDE controller
  *	@pdev: Controller to be initialized
  *	@ppi: array of port_info, must be enough for two ports
  *	@sht: scsi_host_template to use when registering the host
  *	@host_priv: host private_data
  *	@hflags: host flags
+ *	@owner: module which will be the owner of the ATA host
  *
  *	This function is similar to ata_pci_sff_init_one() but also
  *	takes care of BMDMA initialization.
@@ -3291,14 +3296,14 @@ EXPORT_SYMBOL_GPL(ata_pci_bmdma_prepare_host);
  *	RETURNS:
  *	Zero on success, negative on errno-based value on error.
  */
-int ata_pci_bmdma_init_one(struct pci_dev *pdev,
-			   const struct ata_port_info * const * ppi,
-			   struct scsi_host_template *sht, void *host_priv,
-			   int hflags)
+int __ata_pci_bmdma_init_one(struct pci_dev *pdev,
+			     const struct ata_port_info * const *ppi,
+			     struct scsi_host_template *sht, void *host_priv,
+			     int hflags, struct module *owner)
 {
-	return ata_pci_init_one(pdev, ppi, sht, host_priv, hflags, 1);
+	return ata_pci_init_one(pdev, ppi, sht, host_priv, hflags, 1, owner);
 }
-EXPORT_SYMBOL_GPL(ata_pci_bmdma_init_one);
+EXPORT_SYMBOL_GPL(__ata_pci_bmdma_init_one);
 
 #endif /* CONFIG_PCI */
 #endif /* CONFIG_ATA_BMDMA */
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 2d18241..541ceb1 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -598,6 +598,7 @@ struct ata_host {
 	void			*private_data;
 	struct ata_port_operations *ops;
 	unsigned long		flags;
+	struct module		*module;
 
 	struct mutex		eh_mutex;
 	struct task_struct	*eh_owner;
@@ -1106,9 +1107,15 @@ extern int sata_std_hardreset(struct ata_link *link, unsigned int *class,
 			      unsigned long deadline);
 extern void ata_std_postreset(struct ata_link *link, unsigned int *classes);
 
-extern struct ata_host *ata_host_alloc(struct device *dev, int max_ports);
-extern struct ata_host *ata_host_alloc_pinfo(struct device *dev,
-			const struct ata_port_info * const * ppi, int n_ports);
+extern struct ata_host *__ata_host_alloc(struct device *dev, int max_ports,
+					struct module *owner);
+#define ata_host_alloc(dev, max_ports) \
+	__ata_host_alloc(dev, max_ports, THIS_MODULE)
+extern struct ata_host *__ata_host_alloc_pinfo(struct device *dev,
+			const struct ata_port_info * const *ppi, int n_ports,
+			struct module *owner);
+#define ata_host_alloc_pinfo(dev, ppi, n_ports) \
+	__ata_host_alloc_pinfo(dev, ppi, n_ports, THIS_MODULE)
 extern int ata_slave_link_init(struct ata_port *ap);
 extern int ata_host_start(struct ata_host *host);
 extern int ata_host_register(struct ata_host *host,
@@ -1835,15 +1842,22 @@ extern void ata_sff_error_handler(struct ata_port *ap);
 extern void ata_sff_std_ports(struct ata_ioports *ioaddr);
 #ifdef CONFIG_PCI
 extern int ata_pci_sff_init_host(struct ata_host *host);
-extern int ata_pci_sff_prepare_host(struct pci_dev *pdev,
-				    const struct ata_port_info * const * ppi,
-				    struct ata_host **r_host);
+extern int __ata_pci_sff_prepare_host(struct pci_dev *pdev,
+				      const struct ata_port_info * const *ppi,
+				      struct ata_host **r_host,
+				      struct module *owner);
+#define ata_pci_sff_prepare_host(pdev, ppi, r_host) \
+	__ata_pci_sff_prepare_host(pdev, ppi, r_host, THIS_MODULE)
 extern int ata_pci_sff_activate_host(struct ata_host *host,
 				     irq_handler_t irq_handler,
 				     struct scsi_host_template *sht);
-extern int ata_pci_sff_init_one(struct pci_dev *pdev,
+extern int __ata_pci_sff_init_one(struct pci_dev *pdev,
 		const struct ata_port_info * const * ppi,
-		struct scsi_host_template *sht, void *host_priv, int hflags);
+		struct scsi_host_template *sht, void *host_priv, int hflags,
+		struct module *owner);
+#define ata_pci_sff_init_one(pdev, ppi, sht, host_priv, hflag)	\
+	__ata_pci_sff_init_one(pdev, ppi, sht, host_priv, hflag, THIS_MODULE)
+
 #endif /* CONFIG_PCI */
 
 #ifdef CONFIG_ATA_BMDMA
@@ -1874,13 +1888,21 @@ extern int ata_bmdma_port_start32(struct ata_port *ap);
 #ifdef CONFIG_PCI
 extern int ata_pci_bmdma_clear_simplex(struct pci_dev *pdev);
 extern void ata_pci_bmdma_init(struct ata_host *host);
-extern int ata_pci_bmdma_prepare_host(struct pci_dev *pdev,
-				      const struct ata_port_info * const * ppi,
-				      struct ata_host **r_host);
-extern int ata_pci_bmdma_init_one(struct pci_dev *pdev,
-				  const struct ata_port_info * const * ppi,
-				  struct scsi_host_template *sht,
-				  void *host_priv, int hflags);
+extern int __ata_pci_bmdma_prepare_host(struct pci_dev *pdev,
+					const struct ata_port_info * const *ppi,
+					struct ata_host **r_host,
+					struct module *owner);
+#define ata_pci_bmdma_prepare_host(pdev, ppi, r_host) \
+	__ata_pci_bmdma_prepare_host(pdev, ppi, r_host, THIS_MODULE)
+
+extern int __ata_pci_bmdma_init_one(struct pci_dev *pdev,
+				    const struct ata_port_info * const *ppi,
+				    struct scsi_host_template *sht,
+				    void *host_priv, int hflags,
+				    struct module *owner);
+#define ata_pci_bmdma_init_one(pdev, ppi, sht, host_priv, hflags) \
+	__ata_pci_bmdma_init_one(pdev, ppi, sht, host_priv, hflags, THIS_MODULE)
+
 #endif /* CONFIG_PCI */
 #endif /* CONFIG_ATA_BMDMA */
 
-- 
1.9.1

[PATCH v4 07/11] scsi: move module reference from scsi_host_template to 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 drivers which consist with the core driver and the actual
LLDDs and scsi_host_template is defined in the core driver.  In these
cases, the actual LLDDs can be unloaded even if the scsi_device is
being accessed.

This moves owner module reference from struct scsi_host_template to
struct Scsi_Host, and it is passed through scsi_host_alloc().  Some
drivers which have the module reference mismatch problem described above
can pass their correct module reference by using __scsi_host_alloc()
which takes module reference argument.

Sub drivers of esp_scsi (mac_esp, am53c974, sun_esp, jazz_esp, sun3x_esp)
use a single scsi_host_template defined in esp_scsi module, so these
had module reference mismatch problem, but this change implicitly fixes
it.  Because sub drivers directly call scsi_host_alloc() and THIS_MODULE
is used for the module reference instead of scsi_host_template->module.

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: 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: Tejun Heo <tj@kernel.org>
Cc: Hans de Goede <hdegoede@redhat.com>
Cc: Mike Christie <michaelc@cs.wisc.edu>
Cc: Karen Xie <kxie@chelsio.com>
Cc: Robert Love <robert.w.love@intel.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
Cc: open-iscsi@googlegroups.com
Cc: fcoe-devel@open-fcoe.org
Cc: linux-ide@vger.kernel.org
Cc: linux-usb@vger.kernel.org
Cc: usb-storage@lists.one-eyed-alien.net
Cc: linux-scsi@vger.kernel.org
---
 drivers/ata/libata-scsi.c     |  2 +-
 drivers/scsi/53c700.c         |  2 +-
 drivers/scsi/hosts.c          | 11 +++++++----
 drivers/scsi/libfc/fc_lport.c |  2 +-
 drivers/scsi/libfc/fc_npiv.c  |  3 +--
 drivers/scsi/libiscsi.c       |  2 +-
 drivers/scsi/scsi.c           |  4 ++--
 include/scsi/scsi_host.h      |  8 +++++++-
 8 files changed, 21 insertions(+), 13 deletions(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index e364e86..20a963c 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3643,7 +3643,7 @@ int ata_scsi_add_hosts(struct ata_host *host, struct scsi_host_template *sht)
 		struct Scsi_Host *shost;
 
 		rc = -ENOMEM;
-		shost = scsi_host_alloc(sht, sizeof(struct ata_port *));
+		shost = __scsi_host_alloc(sht, sizeof(ap), host->module);
 		if (!shost)
 			goto err_alloc;
 
diff --git a/drivers/scsi/53c700.c b/drivers/scsi/53c700.c
index e70fd05..a5777f8 100644
--- a/drivers/scsi/53c700.c
+++ b/drivers/scsi/53c700.c
@@ -333,7 +333,7 @@ __NCR_700_detect(struct scsi_host_template *tpnt,
 	if(tpnt->proc_name == NULL)
 		tpnt->proc_name = "53c700";
 
-	host = scsi_host_alloc(tpnt, 4);
+	host = __scsi_host_alloc(tpnt, sizeof(hostdata), owner);
 	if (!host)
 		return NULL;
 	memset(hostdata->slots, 0, sizeof(struct NCR_700_command_slot)
diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index eb324f1..90c109e 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -354,9 +354,10 @@ static struct device_type scsi_host_type = {
 };
 
 /**
- * scsi_host_alloc - register a scsi host adapter instance.
+ * __scsi_host_alloc - register a scsi host adapter instance.
  * @sht:	pointer to scsi host template
  * @privsize:	extra bytes to allocate for driver
+ * @owner:	module which will be the owner of the scsi host
  *
  * Note:
  * 	Allocate a new Scsi_Host and perform basic initialization.
@@ -366,7 +367,8 @@ static struct device_type scsi_host_type = {
  * Return value:
  * 	Pointer to a new Scsi_Host
  **/
-struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int privsize)
+struct Scsi_Host *__scsi_host_alloc(struct scsi_host_template *sht,
+				    int privsize, struct module *owner)
 {
 	struct Scsi_Host *shost;
 	gfp_t gfp_mask = GFP_KERNEL;
@@ -411,6 +413,7 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int privsize)
 	 */
 	shost->max_cmd_len = 12;
 	shost->hostt = sht;
+	shost->module = owner;
 	shost->this_id = sht->this_id;
 	shost->can_queue = sht->can_queue;
 	shost->sg_tablesize = sht->sg_tablesize;
@@ -497,12 +500,12 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int privsize)
 	kfree(shost);
 	return NULL;
 }
-EXPORT_SYMBOL(scsi_host_alloc);
+EXPORT_SYMBOL(__scsi_host_alloc);
 
 struct Scsi_Host *__scsi_register(struct scsi_host_template *sht, int privsize,
 				  struct module *owner)
 {
-	struct Scsi_Host *shost = scsi_host_alloc(sht, privsize);
+	struct Scsi_Host *shost = __scsi_host_alloc(sht, privsize, owner);
 
 	if (!sht->detect) {
 		printk(KERN_WARNING "scsi_register() called on new-style "
diff --git a/drivers/scsi/libfc/fc_lport.c b/drivers/scsi/libfc/fc_lport.c
index d931e69..54ef1c4 100644
--- a/drivers/scsi/libfc/fc_lport.c
+++ b/drivers/scsi/libfc/fc_lport.c
@@ -2158,7 +2158,7 @@ struct fc_lport *__libfc_host_alloc(struct scsi_host_template *sht,
 	struct fc_lport *lport;
 	struct Scsi_Host *shost;
 
-	shost = scsi_host_alloc(sht, sizeof(*lport) + priv_size);
+	shost = __scsi_host_alloc(sht, sizeof(*lport) + priv_size, owner);
 	if (!shost)
 		return NULL;
 	lport = shost_priv(shost);
diff --git a/drivers/scsi/libfc/fc_npiv.c b/drivers/scsi/libfc/fc_npiv.c
index 86133f9..1cf2abb 100644
--- a/drivers/scsi/libfc/fc_npiv.c
+++ b/drivers/scsi/libfc/fc_npiv.c
@@ -36,8 +36,7 @@ struct fc_lport *libfc_vport_create(struct fc_vport *vport, int privsize)
 	struct fc_lport *n_port = shost_priv(shost);
 	struct fc_lport *vn_port;
 
-	vn_port = __libfc_host_alloc(shost->hostt, privsize,
-				     shost->hostt->module);
+	vn_port = __libfc_host_alloc(shost->hostt, privsize, shost->module);
 	if (!vn_port)
 		return vn_port;
 
diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index 4c5be6b..6c2ebf3 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -2609,7 +2609,7 @@ struct Scsi_Host *__iscsi_host_alloc(struct scsi_host_template *sht,
 	struct Scsi_Host *shost;
 	struct iscsi_host *ihost;
 
-	shost = scsi_host_alloc(sht, sizeof(struct iscsi_host) + dd_data_size);
+	shost = __scsi_host_alloc(sht, sizeof(*ihost) + dd_data_size, owner);
 	if (!shost)
 		return NULL;
 	ihost = shost_priv(shost);
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 0369b4e..acdf828 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -47,6 +47,7 @@ struct blk_queue_tags;
 #define ENABLE_CLUSTERING 1
 
 struct scsi_host_template {
+	/* This is unused and will be removed after mass conversion */
 	struct module *module;
 	const char *name;
 
@@ -617,6 +618,7 @@ struct Scsi_Host {
 	 */
 	unsigned short max_cmd_len;
 
+	struct module *module;
 	int this_id;
 	int can_queue;
 	short cmd_per_lun;
@@ -783,7 +785,11 @@ static inline bool shost_use_blk_mq(struct Scsi_Host *shost)
 extern int scsi_queue_work(struct Scsi_Host *, struct work_struct *);
 extern void scsi_flush_work(struct Scsi_Host *);
 
-extern struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *, int);
+extern struct Scsi_Host *__scsi_host_alloc(struct scsi_host_template *, int,
+					struct module *);
+#define scsi_host_alloc(sht, privsize) \
+	__scsi_host_alloc(sht, privsize, THIS_MODULE)
+
 extern int __must_check scsi_add_host_with_dma(struct Scsi_Host *,
 					       struct device *,
 					       struct device *);
-- 
1.9.1

[PATCH v4 10/11] ata: ahci_platform: adjust module reference for scsi host

[Akinobu Mita]

The ahci platform drivers depend on libahci_platform module.  The
module reference of these scsi host is initialized to libahci_platform's
one.  Because these drivers use ahci_platform_init_host() which is
defined in libahci_platform module and calls scsi_host_alloc()
internally.  So these drivers can be unloaded even if the scsi device
is being accessed.

This fixes it by converting into ahci_platform_init_host() macro so that
these drivers can pass their correct module reference through
__ata_host_alloc_pinfo().

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Cc: Hans de Goede <hdegoede@redhat.com>
Cc: Tejun Heo <tj@kernel.org>
Cc: Christoph Hellwig <hch@lst.de>
Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
Cc: linux-ide@vger.kernel.org
Cc: linux-scsi@vger.kernel.org
---
 drivers/ata/libahci_platform.c | 14 ++++++++------
 include/linux/ahci_platform.h  |  9 ++++++---
 2 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
index 0b03f90..ea75416 100644
--- a/drivers/ata/libahci_platform.c
+++ b/drivers/ata/libahci_platform.c
@@ -395,10 +395,11 @@ err_out:
 EXPORT_SYMBOL_GPL(ahci_platform_get_resources);
 
 /**
- * ahci_platform_init_host - Bring up an ahci-platform host
+ * __ahci_platform_init_host - Bring up an ahci-platform host
  * @pdev: platform device pointer for the host
  * @hpriv: ahci-host private data for the host
  * @pi_template: template for the ata_port_info to use
+ * @owner: module which will be the owner of the ahci-platform host
  *
  * This function does all the usual steps needed to bring up an
  * ahci-platform host, note any necessary resources (ie clks, phys, etc.)
@@ -407,9 +408,10 @@ EXPORT_SYMBOL_GPL(ahci_platform_get_resources);
  * RETURNS:
  * 0 on success otherwise a negative error code
  */
-int ahci_platform_init_host(struct platform_device *pdev,
-			    struct ahci_host_priv *hpriv,
-			    const struct ata_port_info *pi_template)
+int __ahci_platform_init_host(struct platform_device *pdev,
+			      struct ahci_host_priv *hpriv,
+			      const struct ata_port_info *pi_template,
+			      struct module *owner)
 {
 	struct device *dev = &pdev->dev;
 	struct ata_port_info pi = *pi_template;
@@ -443,7 +445,7 @@ int ahci_platform_init_host(struct platform_device *pdev,
 	 */
 	n_ports = max(ahci_nr_ports(hpriv->cap), fls(hpriv->port_map));
 
-	host = ata_host_alloc_pinfo(dev, ppi, n_ports);
+	host = __ata_host_alloc_pinfo(dev, ppi, n_ports, owner);
 	if (!host)
 		return -ENOMEM;
 
@@ -495,7 +497,7 @@ int ahci_platform_init_host(struct platform_device *pdev,
 
 	return ahci_host_activate(host, irq, &ahci_platform_sht);
 }
-EXPORT_SYMBOL_GPL(ahci_platform_init_host);
+EXPORT_SYMBOL_GPL(__ahci_platform_init_host);
 
 static void ahci_host_stop(struct ata_host *host)
 {
diff --git a/include/linux/ahci_platform.h b/include/linux/ahci_platform.h
index 642d6ae..0dcac84 100644
--- a/include/linux/ahci_platform.h
+++ b/include/linux/ahci_platform.h
@@ -28,9 +28,12 @@ int ahci_platform_enable_resources(struct ahci_host_priv *hpriv);
 void ahci_platform_disable_resources(struct ahci_host_priv *hpriv);
 struct ahci_host_priv *ahci_platform_get_resources(
 	struct platform_device *pdev);
-int ahci_platform_init_host(struct platform_device *pdev,
-			    struct ahci_host_priv *hpriv,
-			    const struct ata_port_info *pi_template);
+int __ahci_platform_init_host(struct platform_device *pdev,
+			      struct ahci_host_priv *hpriv,
+			      const struct ata_port_info *pi_template,
+			      struct module *owner);
+#define ahci_platform_init_host(pdev, hpriv, pi_template) \
+	__ahci_platform_init_host(pdev, hpriv, pi_template, THIS_MODULE)
 
 int ahci_platform_suspend_host(struct device *dev);
 int ahci_platform_resume_host(struct device *dev);
-- 
1.9.1

[PATCH v4 11/11] ata: pata_of_platform: adjust module reference for scsi host

[Akinobu Mita]

The pata_of_platform driver depends on pata_platform module.  The
module reference of this scsi host is initialized to pata_platform's
one.  Because this drivers use __pata_platform_probe() which is
defined in pata_platform module and calls scsi_host_alloc() internally.
So these drivers can be unloaded even if the scsi device is being
accessed.

This fixes it by converting into __pata_platform_probe() macro so that
these drivers can pass their correct module reference through
__ata_host_alloc().

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Cc: Tejun Heo <tj@kernel.org>
Cc: Christoph Hellwig <hch@lst.de>
Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
Cc: linux-ide@vger.kernel.org
Cc: linux-scsi@vger.kernel.org
---
 drivers/ata/pata_platform.c  | 18 ++++++++++--------
 include/linux/ata_platform.h | 16 ++++++++++------
 2 files changed, 20 insertions(+), 14 deletions(-)

diff --git a/drivers/ata/pata_platform.c b/drivers/ata/pata_platform.c
index 1eedfe4..c84e834 100644
--- a/drivers/ata/pata_platform.c
+++ b/drivers/ata/pata_platform.c
@@ -71,13 +71,14 @@ static void pata_platform_setup_port(struct ata_ioports *ioaddr,
 }
 
 /**
- *	__pata_platform_probe		-	attach a platform interface
+ *	___pata_platform_probe		-	attach a platform interface
  *	@dev: device
  *	@io_res: Resource representing I/O base
  *	@ctl_res: Resource representing CTL base
  *	@irq_res: Resource representing IRQ and its flags
  *	@ioport_shift: I/O port shift
  *	@__pio_mask: PIO mask
+ *	@owner: module which will be the owner of the ATA host
  *
  *	Register a platform bus IDE interface. Such interfaces are PIO and we
  *	assume do not support IRQ sharing.
@@ -97,9 +98,10 @@ static void pata_platform_setup_port(struct ata_ioports *ioaddr,
  *
  *	If no IRQ resource is present, PIO polling mode is used instead.
  */
-int __pata_platform_probe(struct device *dev, struct resource *io_res,
-			  struct resource *ctl_res, struct resource *irq_res,
-			  unsigned int ioport_shift, int __pio_mask)
+int ___pata_platform_probe(struct device *dev, struct resource *io_res,
+			   struct resource *ctl_res, struct resource *irq_res,
+			   unsigned int ioport_shift, int __pio_mask,
+			   struct module *owner)
 {
 	struct ata_host *host;
 	struct ata_port *ap;
@@ -124,7 +126,7 @@ int __pata_platform_probe(struct device *dev, struct resource *io_res,
 	/*
 	 * Now that that's out of the way, wire up the port..
 	 */
-	host = ata_host_alloc(dev, 1);
+	host = __ata_host_alloc(dev, 1, owner);
 	if (!host)
 		return -ENOMEM;
 	ap = host->ports[0];
@@ -172,7 +174,7 @@ int __pata_platform_probe(struct device *dev, struct resource *io_res,
 	return ata_host_activate(host, irq, irq ? ata_sff_interrupt : NULL,
 				 irq_flags, &pata_platform_sht);
 }
-EXPORT_SYMBOL_GPL(__pata_platform_probe);
+EXPORT_SYMBOL_GPL(___pata_platform_probe);
 
 static int pata_platform_probe(struct platform_device *pdev)
 {
@@ -215,8 +217,8 @@ static int pata_platform_probe(struct platform_device *pdev)
 	irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
 
 	return __pata_platform_probe(&pdev->dev, io_res, ctl_res, irq_res,
-				     pp_info ? pp_info->ioport_shift : 0,
-				     pio_mask);
+				   pp_info ? pp_info->ioport_shift : 0,
+				   pio_mask);
 }
 
 static struct platform_driver pata_platform_driver = {
diff --git a/include/linux/ata_platform.h b/include/linux/ata_platform.h
index 5c618a0..20bca38 100644
--- a/include/linux/ata_platform.h
+++ b/include/linux/ata_platform.h
@@ -10,12 +10,16 @@ struct pata_platform_info {
 	unsigned int ioport_shift;
 };
 
-extern int __pata_platform_probe(struct device *dev,
-				 struct resource *io_res,
-				 struct resource *ctl_res,
-				 struct resource *irq_res,
-				 unsigned int ioport_shift,
-				 int __pio_mask);
+extern int ___pata_platform_probe(struct device *dev,
+				  struct resource *io_res,
+				  struct resource *ctl_res,
+				  struct resource *irq_res,
+				  unsigned int ioport_shift,
+				  int __pio_mask, struct module *owner);
+#define __pata_platform_probe(dev, io_res, ctl_res, irq_res,		\
+			      ioport_shift, __pio_mask)			\
+	___pata_platform_probe(dev, io_res, ctl_res, irq_res,		\
+			       ioport_shift, __pio_mask, THIS_MODULE)
 
 /*
  * Marvell SATA private data
-- 
1.9.1

Re: [PATCH v4 10/11] ata: ahci_platform: adjust module reference for scsi host

[Hans de Goede]

Hi,

On 18-01-15 16:06, Akinobu Mita wrote:
> The ahci platform drivers depend on libahci_platform module.  The
> module reference of these scsi host is initialized to libahci_platform's
> one.  Because these drivers use ahci_platform_init_host() which is
> defined in libahci_platform module and calls scsi_host_alloc()
> internally.  So these drivers can be unloaded even if the scsi device
> is being accessed.
>
> This fixes it by converting into ahci_platform_init_host() macro so that
> these drivers can pass their correct module reference through
> __ata_host_alloc_pinfo().
>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> Cc: Hans de Goede <hdegoede@redhat.com>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
> Cc: linux-ide@vger.kernel.org
> Cc: linux-scsi@vger.kernel.org

Looks good to me:

Acked-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans

> ---
>   drivers/ata/libahci_platform.c | 14 ++++++++------
>   include/linux/ahci_platform.h  |  9 ++++++---
>   2 files changed, 14 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
> index 0b03f90..ea75416 100644
> --- a/drivers/ata/libahci_platform.c
> +++ b/drivers/ata/libahci_platform.c
> @@ -395,10 +395,11 @@ err_out:
>   EXPORT_SYMBOL_GPL(ahci_platform_get_resources);
>
>   /**
> - * ahci_platform_init_host - Bring up an ahci-platform host
> + * __ahci_platform_init_host - Bring up an ahci-platform host
>    * @pdev: platform device pointer for the host
>    * @hpriv: ahci-host private data for the host
>    * @pi_template: template for the ata_port_info to use
> + * @owner: module which will be the owner of the ahci-platform host
>    *
>    * This function does all the usual steps needed to bring up an
>    * ahci-platform host, note any necessary resources (ie clks, phys, etc.)
> @@ -407,9 +408,10 @@ EXPORT_SYMBOL_GPL(ahci_platform_get_resources);
>    * RETURNS:
>    * 0 on success otherwise a negative error code
>    */
> -int ahci_platform_init_host(struct platform_device *pdev,
> -			    struct ahci_host_priv *hpriv,
> -			    const struct ata_port_info *pi_template)
> +int __ahci_platform_init_host(struct platform_device *pdev,
> +			      struct ahci_host_priv *hpriv,
> +			      const struct ata_port_info *pi_template,
> +			      struct module *owner)
>   {
>   	struct device *dev = &pdev->dev;
>   	struct ata_port_info pi = *pi_template;
> @@ -443,7 +445,7 @@ int ahci_platform_init_host(struct platform_device *pdev,
>   	 */
>   	n_ports = max(ahci_nr_ports(hpriv->cap), fls(hpriv->port_map));
>
> -	host = ata_host_alloc_pinfo(dev, ppi, n_ports);
> +	host = __ata_host_alloc_pinfo(dev, ppi, n_ports, owner);
>   	if (!host)
>   		return -ENOMEM;
>
> @@ -495,7 +497,7 @@ int ahci_platform_init_host(struct platform_device *pdev,
>
>   	return ahci_host_activate(host, irq, &ahci_platform_sht);
>   }
> -EXPORT_SYMBOL_GPL(ahci_platform_init_host);
> +EXPORT_SYMBOL_GPL(__ahci_platform_init_host);
>
>   static void ahci_host_stop(struct ata_host *host)
>   {
> diff --git a/include/linux/ahci_platform.h b/include/linux/ahci_platform.h
> index 642d6ae..0dcac84 100644
> --- a/include/linux/ahci_platform.h
> +++ b/include/linux/ahci_platform.h
> @@ -28,9 +28,12 @@ int ahci_platform_enable_resources(struct ahci_host_priv *hpriv);
>   void ahci_platform_disable_resources(struct ahci_host_priv *hpriv);
>   struct ahci_host_priv *ahci_platform_get_resources(
>   	struct platform_device *pdev);
> -int ahci_platform_init_host(struct platform_device *pdev,
> -			    struct ahci_host_priv *hpriv,
> -			    const struct ata_port_info *pi_template);
> +int __ahci_platform_init_host(struct platform_device *pdev,
> +			      struct ahci_host_priv *hpriv,
> +			      const struct ata_port_info *pi_template,
> +			      struct module *owner);
> +#define ahci_platform_init_host(pdev, hpriv, pi_template) \
> +	__ahci_platform_init_host(pdev, hpriv, pi_template, THIS_MODULE)
>
>   int ahci_platform_suspend_host(struct device *dev);
>   int ahci_platform_resume_host(struct device *dev);
>

Re: [PATCH v4 00/11] scsi: fix module reference mismatch for scsi host

[Tejun Heo]

On Mon, Jan 19, 2015 at 12:05:58AM +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 drivers which consist with the core driver and the actual
> LLDDs and scsi_host_template is defined in the core driver.  In these
> cases, the actual LLDDs can be unloaded even if the scsi_device is
> being accessed.
> 
> This patch series fixes the module reference mismatch problem for
> ufs, usb-storage, esp_scsi, ahci_platform, and pata_platform drivers
> by moving owner module reference field from struct scsi_host_template
> to struct Scsi_Host and allowing the LLDDs to set their correct module
> reference.

Hmmm, can't we just move sht definitions to actual LLDDs?  libata can
do that easily.  sht, as its name implies, is the template for
creating the scsi_hosts of a given type.  We're now just moving module
ownership from sht definition site to whatever callsite the actual
instance is being created which can also be wrapped in a separate
layer requiring explicit propagation.  Why not just propagate sht's
directly?  What's the difference?

Thanks.

-- 
tejun

Re: [PATCH v4 00/11] scsi: fix module reference mismatch for scsi host

[Akinobu Mita]

2015-01-19 23:22 GMT+09:00 Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>:
> On Mon, Jan 19, 2015 at 12:05:58AM +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 drivers which consist with the core driver and the actual
>> LLDDs and scsi_host_template is defined in the core driver.  In these
>> cases, the actual LLDDs can be unloaded even if the scsi_device is
>> being accessed.
>>
>> This patch series fixes the module reference mismatch problem for
>> ufs, usb-storage, esp_scsi, ahci_platform, and pata_platform drivers
>> by moving owner module reference field from struct scsi_host_template
>> to struct Scsi_Host and allowing the LLDDs to set their correct module
>> reference.
>
> Hmmm, can't we just move sht definitions to actual LLDDs?  libata can
> do that easily.  sht, as its name implies, is the template for
> creating the scsi_hosts of a given type.  We're now just moving module
> ownership from sht definition site to whatever callsite the actual
> instance is being created which can also be wrapped in a separate
> layer requiring explicit propagation.  Why not just propagate sht's
> directly?  What's the difference?

The reason I didn't move sht from the core driver to the LLDDs for
fixing ufs and ums-* in the first place is to avoid exporting many
symbols for callbacks in sht.  But I realized that we can do it
without that many exported symbols by creating a single function that
returns a kmemdup()ed sht with a few change including ->module.

So there are three options we can take for fixing this problem.
I would like to know the opinions which one should be taken.

(1) Add owner module field to Scsi_Host for allowing LLDDs to adjust
it after scsi host allocation.  This approach is used by v1,2,3 of
this patch series and the scsi midlayer change is minimum.

(2) Move owner module field from scsi_host_template to Scsi_Host.
The owner module reference is retrieved from the callsite of
scsi_host_alloc() by passing THIS_MODULE to the extra argument.
The scsi midlayer change is small, but we need the same macro trick
for each scsi_host_alloc() wrapper and these changes are relatively
large (required by libata, libiscsi, libfc, cxgbi, 53c700, legacy).
This approach is used by v4 of this patch series.

(3) Allocate scsi host template for each module.  No scsi midlayer
change is required.  Instead of sharing a single scsi host template
defined in the core, create a single function that returns a
kmemdup()ed sht with a few change including ->module so that the sub
modules can use it as their sht.

-- 
You received this message because you are subscribed to the Google Groups "open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to open-iscsi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
To post to this group, send email to open-iscsi-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
Visit this group at http://groups.google.com/group/open-iscsi.
For more options, visit https://groups.google.com/d/optout.

Re: [PATCH v4 00/11] scsi: fix module reference mismatch for scsi host

[Tejun Heo]

Hello, Akinobu.

On Tue, Jan 20, 2015 at 11:57:37PM +0900, Akinobu Mita wrote:
> The reason I didn't move sht from the core driver to the LLDDs for
> fixing ufs and ums-* in the first place is to avoid exporting many
> symbols for callbacks in sht.  But I realized that we can do it
> without that many exported symbols by creating a single function that
> returns a kmemdup()ed sht with a few change including ->module.

Hmmm, libata already exports most of the necessary symbols.  libahci
or platform drivers might have to export more but that shouldn't be
much.  For libata, pushing sht's to the leaf drivers would make far
more sense as sht's already get inherited and modified along the
hierarchy.

Thanks.

-- 
tejun

Re: [PATCH v4 00/11] scsi: fix module reference mismatch for scsi host

[Alan Stern]

On Tue, 20 Jan 2015, Akinobu Mita wrote:

> 2015-01-19 23:22 GMT+09:00 Tejun Heo <tj@kernel.org>:
> > On Mon, Jan 19, 2015 at 12:05:58AM +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 drivers which consist with the core driver and the actual
> >> LLDDs and scsi_host_template is defined in the core driver.  In these
> >> cases, the actual LLDDs can be unloaded even if the scsi_device is
> >> being accessed.
> >>
> >> This patch series fixes the module reference mismatch problem for
> >> ufs, usb-storage, esp_scsi, ahci_platform, and pata_platform drivers
> >> by moving owner module reference field from struct scsi_host_template
> >> to struct Scsi_Host and allowing the LLDDs to set their correct module
> >> reference.
> >
> > Hmmm, can't we just move sht definitions to actual LLDDs?  libata can
> > do that easily.  sht, as its name implies, is the template for
> > creating the scsi_hosts of a given type.  We're now just moving module
> > ownership from sht definition site to whatever callsite the actual
> > instance is being created which can also be wrapped in a separate
> > layer requiring explicit propagation.  Why not just propagate sht's
> > directly?  What's the difference?
> 
> The reason I didn't move sht from the core driver to the LLDDs for
> fixing ufs and ums-* in the first place is to avoid exporting many
> symbols for callbacks in sht.  But I realized that we can do it
> without that many exported symbols by creating a single function that
> returns a kmemdup()ed sht with a few change including ->module.
> 
> So there are three options we can take for fixing this problem.
> I would like to know the opinions which one should be taken.
> 
> (1) Add owner module field to Scsi_Host for allowing LLDDs to adjust
> it after scsi host allocation.  This approach is used by v1,2,3 of
> this patch series and the scsi midlayer change is minimum.
> 
> (2) Move owner module field from scsi_host_template to Scsi_Host.
> The owner module reference is retrieved from the callsite of
> scsi_host_alloc() by passing THIS_MODULE to the extra argument.
> The scsi midlayer change is small, but we need the same macro trick
> for each scsi_host_alloc() wrapper and these changes are relatively
> large (required by libata, libiscsi, libfc, cxgbi, 53c700, legacy).
> This approach is used by v4 of this patch series.
> 
> (3) Allocate scsi host template for each module.  No scsi midlayer
> change is required.  Instead of sharing a single scsi host template
> defined in the core, create a single function that returns a
> kmemdup()ed sht with a few change including ->module so that the sub
> modules can use it as their sht.

(3) means duplicating a reasonably large data structure in order to
alter just one field.  It also means changing all the subdrivers to
make them call the new function.

(1) is the simplest.  Since the use of subdrivers in general tends to 
be a special case (most SCSI drivers don't do it), I prefer to keep the 
code optimized for it.  In other words, I prefer option (1).  If people 
think (2) is better, it can always be layered on top of (1).

Alan Stern

Re: [PATCH v4 00/11] scsi: fix module reference mismatch for scsi host

[Akinobu Mita]

2015-01-21 0:20 GMT+09:00 Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org>:
> On Tue, 20 Jan 2015, Akinobu Mita wrote:
>
>> 2015-01-19 23:22 GMT+09:00 Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>:
>> > On Mon, Jan 19, 2015 at 12:05:58AM +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 drivers which consist with the core driver and the actual
>> >> LLDDs and scsi_host_template is defined in the core driver.  In these
>> >> cases, the actual LLDDs can be unloaded even if the scsi_device is
>> >> being accessed.
>> >>
>> >> This patch series fixes the module reference mismatch problem for
>> >> ufs, usb-storage, esp_scsi, ahci_platform, and pata_platform drivers
>> >> by moving owner module reference field from struct scsi_host_template
>> >> to struct Scsi_Host and allowing the LLDDs to set their correct module
>> >> reference.
>> >
>> > Hmmm, can't we just move sht definitions to actual LLDDs?  libata can
>> > do that easily.  sht, as its name implies, is the template for
>> > creating the scsi_hosts of a given type.  We're now just moving module
>> > ownership from sht definition site to whatever callsite the actual
>> > instance is being created which can also be wrapped in a separate
>> > layer requiring explicit propagation.  Why not just propagate sht's
>> > directly?  What's the difference?
>>
>> The reason I didn't move sht from the core driver to the LLDDs for
>> fixing ufs and ums-* in the first place is to avoid exporting many
>> symbols for callbacks in sht.  But I realized that we can do it
>> without that many exported symbols by creating a single function that
>> returns a kmemdup()ed sht with a few change including ->module.
>>
>> So there are three options we can take for fixing this problem.
>> I would like to know the opinions which one should be taken.
>>
>> (1) Add owner module field to Scsi_Host for allowing LLDDs to adjust
>> it after scsi host allocation.  This approach is used by v1,2,3 of
>> this patch series and the scsi midlayer change is minimum.
>>
>> (2) Move owner module field from scsi_host_template to Scsi_Host.
>> The owner module reference is retrieved from the callsite of
>> scsi_host_alloc() by passing THIS_MODULE to the extra argument.
>> The scsi midlayer change is small, but we need the same macro trick
>> for each scsi_host_alloc() wrapper and these changes are relatively
>> large (required by libata, libiscsi, libfc, cxgbi, 53c700, legacy).
>> This approach is used by v4 of this patch series.
>>
>> (3) Allocate scsi host template for each module.  No scsi midlayer
>> change is required.  Instead of sharing a single scsi host template
>> defined in the core, create a single function that returns a
>> kmemdup()ed sht with a few change including ->module so that the sub
>> modules can use it as their sht.
>
> (3) means duplicating a reasonably large data structure in order to
> alter just one field.  It also means changing all the subdrivers to
> make them call the new function.
>
> (1) is the simplest.  Since the use of subdrivers in general tends to
> be a special case (most SCSI drivers don't do it), I prefer to keep the
> code optimized for it.  In other words, I prefer option (1).  If people
> think (2) is better, it can always be layered on top of (1).

OK, I agree that (1) is the simplest.

Christoph,

Option (2) required a lot of changes as this v4 shows that
scsi_host_alloc() is used by several libraries on scsi mid level in
various forms.  It is a bit different from pci_register_driver() or
similar registration interfaces.  Although they also use the same
macro trick to get a module reference, but they are called directly
or through thin and straightforward wrapper interfaces by lower
level drivers.

So, option (1) seems to be a better choice than (2), (3) for fixing
ufs, ums-*, and esp_scsi drivers.  For ahci_platform and pata_platform,
they can be fixed easily by moving sht definision to LLDDs as Tejun
suggested.  Do you think it is ok to prepare v5 of series in this way?

-- 
You received this message because you are subscribed to the Google Groups "open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to open-iscsi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
To post to this group, send email to open-iscsi-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
Visit this group at http://groups.google.com/group/open-iscsi.
For more options, visit https://groups.google.com/d/optout.