[PATCH v2 0/6] scsi: ufs: fix several issues caused by driver reloading

[PATCH v2 0/6] scsi: ufs: fix several issues caused by driver reloading

[Akinobu Mita]

This patch set addresses several issues caused by driver reloading in
ufs driver.  The first version of this series was sent on March 28.
This version gained three more fixes.

* Changes from v1
- Call pm_runtime_put_noidle() where idle callback shouldn't be called
- Prevent IRQ handler accessing already freed hostdata
- Fix unloading module while runtime suspended
- Fix module reference for scsi host

Akinobu Mita (6):
  scsi: ufs: avoid using hostdata after scsi_host_put()
  scsi: ufs: fix unbalanced power.usage_count after reloading driver
  scsi: ufs: fix unbalanced power.disable_depth after reloading driver
  scsi: ufs: prevent IRQ handler accessing already freed hostdata
  scsi: ufs: fix unloading module while runtime suspended
  scsi: ufs: fix module reference for scsi host

 drivers/scsi/ufs/ufshcd-pci.c    | 19 ++++++++++++--
 drivers/scsi/ufs/ufshcd-pltfrm.c | 24 ++++++++++++++++--
 drivers/scsi/ufs/ufshcd.c        | 54 +++++++++++++++++++++-------------------
 drivers/scsi/ufs/ufshcd.h        |  5 +++-
 4 files changed, 72 insertions(+), 30 deletions(-)

-- 
1.9.1

[PATCH v2 1/6] scsi: ufs: avoid using hostdata after scsi_host_put()

[Akinobu Mita]

The hostdata array, which is denoted by 'hba' in ufs driver, should
not be accessed after calling scsi_host_put().

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

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index b0ade73..e25f919 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -5363,12 +5363,12 @@ void ufshcd_remove(struct ufs_hba *hba)
 	ufshcd_disable_intr(hba, hba->intr_mask);
 	ufshcd_hba_stop(hba);
 
-	scsi_host_put(hba->host);
-
 	ufshcd_exit_clk_gating(hba);
 	if (ufshcd_is_clkscaling_enabled(hba))
 		devfreq_remove_device(hba->devfreq);
 	ufshcd_hba_exit(hba);
+
+	scsi_host_put(hba->host);
 }
 EXPORT_SYMBOL_GPL(ufshcd_remove);
 
@@ -5671,9 +5671,9 @@ exit_gating:
 	ufshcd_exit_clk_gating(hba);
 out_disable:
 	hba->is_irq_enabled = false;
-	scsi_host_put(host);
 	ufshcd_hba_exit(hba);
 out_error:
+	scsi_host_put(host);
 	return err;
 }
 EXPORT_SYMBOL_GPL(ufshcd_init);
-- 
1.9.1

[PATCH v2 2/6] scsi: ufs: fix unbalanced power.usage_count after reloading driver

[Akinobu Mita]

On driver removal, pm_runtime_get_sync() is called, but
pm_runtime_put_*() is missed.  So once the driver is reloaded, the
device's power.usage_count is unbalanced and the idle callback for the
device will never be called.

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Cc: Vinayak Holikatti <vinholikatti@gmail.com>
Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Dolev Raviv <draviv@codeaurora.org>
Cc: Sujit Reddy Thumma <sthumma@codeaurora.org>
Cc: Subhash Jadavani <subhashj@codeaurora.org>
Cc: Maya Erez <merez@codeaurora.org>
Cc: Sahitya Tummala <stummala@codeaurora.org>
Cc: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
Cc: linux-scsi@vger.kernel.org
---
 drivers/scsi/ufs/ufshcd-pltfrm.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/scsi/ufs/ufshcd-pltfrm.c b/drivers/scsi/ufs/ufshcd-pltfrm.c
index 7db9564..9beac71 100644
--- a/drivers/scsi/ufs/ufshcd-pltfrm.c
+++ b/drivers/scsi/ufs/ufshcd-pltfrm.c
@@ -368,6 +368,7 @@ static int ufshcd_pltfrm_remove(struct platform_device *pdev)
 
 	pm_runtime_get_sync(&(pdev)->dev);
 	ufshcd_remove(hba);
+	pm_runtime_put_noidle(&pdev->dev);
 	return 0;
 }
 
-- 
1.9.1

[PATCH v2 3/6] scsi: ufs: fix unbalanced power.disable_depth after reloading driver

[Akinobu Mita]

Every time the driver is reloaded, the warning message
"Unbalanced pm_runtime_enable!" is triggered due to unbalanced
power.disable_depth.  This is because pm_runtime_enable() is called
during driver probe but pm_runtime_disable() is missed on driver remove.

This also restores the device's runtime PM status to 'suspended' on
driver remove as it was set to 'active' during driver probe.

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Cc: Vinayak Holikatti <vinholikatti@gmail.com>
Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Dolev Raviv <draviv@codeaurora.org>
Cc: Sujit Reddy Thumma <sthumma@codeaurora.org>
Cc: Maya Erez <merez@codeaurora.org>
Cc: Raviv Shvili <rshvili@codeaurora.org>
Cc: Sahitya Tummala <stummala@codeaurora.org>
Cc: Subhash Jadavani <subhashj@codeaurora.org>
Cc: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
Cc: linux-scsi@vger.kernel.org
---
 drivers/scsi/ufs/ufshcd-pltfrm.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/scsi/ufs/ufshcd-pltfrm.c b/drivers/scsi/ufs/ufshcd-pltfrm.c
index 9beac71..12f1246 100644
--- a/drivers/scsi/ufs/ufshcd-pltfrm.c
+++ b/drivers/scsi/ufs/ufshcd-pltfrm.c
@@ -369,6 +369,10 @@ static int ufshcd_pltfrm_remove(struct platform_device *pdev)
 	pm_runtime_get_sync(&(pdev)->dev);
 	ufshcd_remove(hba);
 	pm_runtime_put_noidle(&pdev->dev);
+
+	pm_runtime_disable(&pdev->dev);
+	pm_runtime_set_suspended(&pdev->dev);
+
 	return 0;
 }
 
-- 
1.9.1

[PATCH v2 4/6] scsi: ufs: prevent IRQ handler accessing already freed hostdata

[Akinobu Mita]

As UFS driver registers IRQ handler as a shared IRQ, when
CONFIG_DEBUG_SHIRQ=y, an extra call will be made while unregistering
the IRQ handler.  Unfortunately, the extra call will accesses already
freed hostdata.  This is because devm_request_irq() is used to register
IRQ handler so that it will be unregistered automatically on driver
remove, but the hostdata has already been freed at this time.

This fixes it by explicitly registering/unregistering IRQ handler on
driver probe/remove.

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Cc: Vinayak Holikatti <vinholikatti@gmail.com>
Cc: "James E.J. Bottomley" <JBottomley@odin.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Dolev Raviv <draviv@codeaurora.org>
Cc: Sujit Reddy Thumma <sthumma@codeaurora.org>
Cc: Subhash Jadavani <subhashj@codeaurora.org>
Cc: Hannes Reinecke <hare@suse.de>
Cc: Sahitya Tummala <stummala@codeaurora.org>
Cc: Yaniv Gardi <ygardi@codeaurora.org>
Cc: linux-scsi@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/scsi/ufs/ufshcd.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index e25f919..d425816 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -5361,6 +5361,7 @@ void ufshcd_remove(struct ufs_hba *hba)
 	scsi_remove_host(hba->host);
 	/* disable interrupts */
 	ufshcd_disable_intr(hba, hba->intr_mask);
+	ufshcd_disable_irq(hba);
 	ufshcd_hba_stop(hba);
 
 	ufshcd_exit_clk_gating(hba);
@@ -5611,13 +5612,9 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
 
 	ufshcd_init_clk_gating(hba);
 	/* IRQ registration */
-	err = devm_request_irq(dev, irq, ufshcd_intr, IRQF_SHARED, UFSHCD, hba);
-	if (err) {
-		dev_err(hba->dev, "request irq failed\n");
+	err = ufshcd_enable_irq(hba);
+	if (err)
 		goto exit_gating;
-	} else {
-		hba->is_irq_enabled = true;
-	}
 
 	/* Enable SCSI tag mapping */
 	err = scsi_init_shared_tag_map(host, host->can_queue);
@@ -5668,9 +5665,9 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
 out_remove_scsi_host:
 	scsi_remove_host(hba->host);
 exit_gating:
+	ufshcd_disable_irq(hba);
 	ufshcd_exit_clk_gating(hba);
 out_disable:
-	hba->is_irq_enabled = false;
 	ufshcd_hba_exit(hba);
 out_error:
 	scsi_host_put(host);
-- 
1.9.1

[PATCH v2 5/6] scsi: ufs: fix unloading module while runtime suspended

[Akinobu Mita]

The ufs driver calls scsi_device_get() in ufshcd_set_dev_pwr_mode()
in order to avoid manual delete of UFS device W-LUN by holding the
reference.

But scsi_device_get() has been changed to fail when the LLD module is
in the process of being unloaded.  So it no longer doesn't work if the
module is unloaded while the device is runtime suspended.
(i.e. driver_detach -> ... pm_runtime_get_sync() ... ->
ufshcd_runtime_resume -> ufshcd_resume -> ufshcd_set_dev_pwr_mode ->
scsi_device_get -> try_module_get -> return -ENXIO)

As the reason for scsi_device_get() is to avoid manual delete of UFS
device W-LUN, this acquires shost->scan_mutex lock instead of
scsi_device_get() to work around the problem.

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Cc: Vinayak Holikatti <vinholikatti@gmail.com>
Cc: "James E.J. Bottomley" <JBottomley@odin.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Dolev Raviv <draviv@codeaurora.org>
Cc: Sujit Reddy Thumma <sthumma@codeaurora.org>
Cc: Akinobu Mita <akinobu.mita@gmail.com>
Cc: Subhash Jadavani <subhashj@codeaurora.org>
Cc: Sahitya Tummala <stummala@codeaurora.org>
Cc: Yaniv Gardi <ygardi@codeaurora.org>
Cc: linux-scsi@vger.kernel.org
---
 drivers/scsi/ufs/ufshcd.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index d425816..d287207 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -4792,23 +4792,19 @@ static int ufshcd_set_dev_pwr_mode(struct ufs_hba *hba,
 	struct scsi_sense_hdr sshdr;
 	struct scsi_device *sdp;
 	unsigned long flags;
-	int ret;
+	int ret = 0;
 
+	mutex_lock(&hba->host->scan_mutex);
 	spin_lock_irqsave(hba->host->host_lock, flags);
 	sdp = hba->sdev_ufs_device;
-	if (sdp) {
-		ret = scsi_device_get(sdp);
-		if (!ret && !scsi_device_online(sdp)) {
-			ret = -ENODEV;
-			scsi_device_put(sdp);
-		}
-	} else {
+	if (!sdp || !scsi_device_online(sdp))
 		ret = -ENODEV;
-	}
 	spin_unlock_irqrestore(hba->host->host_lock, flags);
 
-	if (ret)
+	if (ret) {
+		mutex_unlock(&hba->host->scan_mutex);
 		return ret;
+	}
 
 	/*
 	 * If scsi commands fail, the scsi mid-layer schedules scsi error-
@@ -4845,7 +4841,7 @@ static int ufshcd_set_dev_pwr_mode(struct ufs_hba *hba,
 	if (!ret)
 		hba->curr_dev_pwr_mode = pwr_mode;
 out:
-	scsi_device_put(sdp);
+	mutex_unlock(&hba->host->scan_mutex);
 	hba->host->eh_noresume = 0;
 	return ret;
 }
-- 
1.9.1

[PATCH v2 6/6] scsi: ufs: fix 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 preparing scsi host template which is initialized at
module_init() for each ufs glue driver.

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    | 19 +++++++++++++++++--
 drivers/scsi/ufs/ufshcd-pltfrm.c | 19 +++++++++++++++++--
 drivers/scsi/ufs/ufshcd.c        | 19 +++++++++++++++----
 drivers/scsi/ufs/ufshcd.h        |  5 ++++-
 4 files changed, 53 insertions(+), 9 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd-pci.c b/drivers/scsi/ufs/ufshcd-pci.c
index d15eaa4..961c4ad 100644
--- a/drivers/scsi/ufs/ufshcd-pci.c
+++ b/drivers/scsi/ufs/ufshcd-pci.c
@@ -106,6 +106,8 @@ static void ufshcd_pci_remove(struct pci_dev *pdev)
 	ufshcd_remove(hba);
 }
 
+static struct scsi_host_template ufshcd_pci_host_template;
+
 /**
  * ufshcd_pci_probe - probe routine of the driver
  * @pdev: pointer to PCI device handle
@@ -136,7 +138,7 @@ ufshcd_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 
 	mmio_base = pcim_iomap_table(pdev)[0];
 
-	err = ufshcd_alloc_host(&pdev->dev, &hba);
+	err = ufshcd_alloc_host(&pdev->dev, &ufshcd_pci_host_template, &hba);
 	if (err) {
 		dev_err(&pdev->dev, "Allocation failed\n");
 		return err;
@@ -183,7 +185,20 @@ static struct pci_driver ufshcd_pci_driver = {
 	},
 };
 
-module_pci_driver(ufshcd_pci_driver);
+static int __init ufshcd_pci_driver_init(void)
+{
+	ufshcd_host_template_init(&ufshcd_pci_host_template, "ufshcd-pci",
+				  THIS_MODULE);
+
+	return pci_register_driver(&ufshcd_pci_driver);
+}
+module_init(ufshcd_pci_driver_init);
+
+static void __exit ufshcd_pci_driver_exit(void)
+{
+	pci_unregister_driver(&ufshcd_pci_driver);
+}
+module_exit(ufshcd_pci_driver_exit);
 
 MODULE_AUTHOR("Santosh Yaragnavi <santosh.sy@samsung.com>");
 MODULE_AUTHOR("Vinayak Holikatti <h.vinayak@samsung.com>");
diff --git a/drivers/scsi/ufs/ufshcd-pltfrm.c b/drivers/scsi/ufs/ufshcd-pltfrm.c
index 12f1246..2a137c2 100644
--- a/drivers/scsi/ufs/ufshcd-pltfrm.c
+++ b/drivers/scsi/ufs/ufshcd-pltfrm.c
@@ -287,6 +287,8 @@ static void ufshcd_pltfrm_shutdown(struct platform_device *pdev)
 	ufshcd_shutdown((struct ufs_hba *)platform_get_drvdata(pdev));
 }
 
+static struct scsi_host_template ufshcd_pltfrm_host_template;
+
 /**
  * ufshcd_pltfrm_probe - probe routine of the driver
  * @pdev: pointer to Platform device handle
@@ -315,7 +317,7 @@ static int ufshcd_pltfrm_probe(struct platform_device *pdev)
 		goto out;
 	}
 
-	err = ufshcd_alloc_host(dev, &hba);
+	err = ufshcd_alloc_host(dev, &ufshcd_pltfrm_host_template, &hba);
 	if (err) {
 		dev_err(&pdev->dev, "Allocation failed\n");
 		goto out;
@@ -400,7 +402,20 @@ static struct platform_driver ufshcd_pltfrm_driver = {
 	},
 };
 
-module_platform_driver(ufshcd_pltfrm_driver);
+static int __init ufshcd_pltfrm_driver_init(void)
+{
+	ufshcd_host_template_init(&ufshcd_pltfrm_host_template, "ufshcd-pltfrm",
+				  THIS_MODULE);
+
+	return platform_driver_register(&ufshcd_pltfrm_driver);
+}
+module_init(ufshcd_pltfrm_driver_init);
+
+static void __exit ufshcd_pltfrm_driver_exit(void)
+{
+	platform_driver_unregister(&ufshcd_pltfrm_driver);
+}
+module_exit(ufshcd_pltfrm_driver_exit);
 
 MODULE_AUTHOR("Santosh Yaragnavi <santosh.sy@samsung.com>");
 MODULE_AUTHOR("Vinayak Holikatti <h.vinayak@samsung.com>");
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index d287207..6fa64bd 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -4338,7 +4338,7 @@ static void ufshcd_async_scan(void *data, async_cookie_t cookie)
 	ufshcd_probe_hba(hba);
 }
 
-static struct scsi_host_template ufshcd_driver_template = {
+static const struct scsi_host_template ufshcd_driver_template = {
 	.module			= THIS_MODULE,
 	.name			= UFSHCD,
 	.proc_name		= UFSHCD,
@@ -4359,6 +4359,16 @@ static struct scsi_host_template ufshcd_driver_template = {
 	.track_queue_depth	= 1,
 };
 
+void ufshcd_host_template_init(struct scsi_host_template *sht, const char *name,
+			       struct module *owner)
+{
+	*sht = ufshcd_driver_template;
+	sht->name = name;
+	sht->proc_name = name;
+	sht->module = owner;
+}
+EXPORT_SYMBOL_GPL(ufshcd_host_template_init);
+
 static int ufshcd_config_vreg_load(struct device *dev, struct ufs_vreg *vreg,
 				   int ua)
 {
@@ -5388,10 +5398,12 @@ static int ufshcd_set_dma_mask(struct ufs_hba *hba)
 /**
  * ufshcd_alloc_host - allocate Host Bus Adapter (HBA)
  * @dev: pointer to device handle
+ * @sht: scsi_host_template to use when registering the host
  * @hba_handle: driver private handle
  * Returns 0 on success, non-zero value on failure
  */
-int ufshcd_alloc_host(struct device *dev, struct ufs_hba **hba_handle)
+int ufshcd_alloc_host(struct device *dev, struct scsi_host_template *sht,
+		      struct ufs_hba **hba_handle)
 {
 	struct Scsi_Host *host;
 	struct ufs_hba *hba;
@@ -5404,8 +5416,7 @@ int ufshcd_alloc_host(struct device *dev, struct ufs_hba **hba_handle)
 		goto out_error;
 	}
 
-	host = scsi_host_alloc(&ufshcd_driver_template,
-				sizeof(struct ufs_hba));
+	host = scsi_host_alloc(sht, sizeof(struct ufs_hba));
 	if (!host) {
 		dev_err(dev, "scsi_host_alloc failed\n");
 		err = -ENOMEM;
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index c40a0e7..cad15b0 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -575,7 +575,10 @@ static inline void ufshcd_rmwl(struct ufs_hba *hba, u32 mask, u32 val, u32 reg)
 	ufshcd_writel(hba, tmp, reg);
 }
 
-int ufshcd_alloc_host(struct device *, struct ufs_hba **);
+void ufshcd_host_template_init(struct scsi_host_template *sht, const char *name,
+			       struct module *owner);
+int ufshcd_alloc_host(struct device *, struct scsi_host_template *,
+		      struct ufs_hba **);
 int ufshcd_init(struct ufs_hba * , void __iomem * , unsigned int);
 void ufshcd_remove(struct ufs_hba *);
 
-- 
1.9.1