[PATCH 1/3] scsi: serialize ->rescan against ->remove

[PATCH 1/3] scsi: serialize ->rescan against ->remove

[Christoph Hellwig]

Lock the device embedded in the scsi_device to protect against
concurrent calls to ->remove.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/scsi/scsi_scan.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 983aed1..523faee 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -1593,16 +1593,15 @@ EXPORT_SYMBOL(scsi_add_device);
 
 void scsi_rescan_device(struct device *dev)
 {
-	if (!dev->driver)
-		return;
-
-	if (try_module_get(dev->driver->owner)) {
+	device_lock(dev);
+	if (dev->driver && try_module_get(dev->driver->owner)) {
 		struct scsi_driver *drv = to_scsi_driver(dev->driver);
 
 		if (drv->rescan)
 			drv->rescan(dev);
 		module_put(dev->driver->owner);
 	}
+	device_unlock(dev);
 }
 EXPORT_SYMBOL(scsi_rescan_device);
 
-- 
1.9.1

[PATCH 2/3] sd: don't grab a device references from driver methods

[Christoph Hellwig]

The device model already takes care of races between ->remove and
->shutdown vs its other methods, and we now take care about locking
them out for ->rescan as well.

This is a partial revert of commit 39b7f1 ("[SCSI] sd: Fix refcounting").

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/scsi/sd.c | 55 +++++++++++--------------------------------------------
 1 file changed, 11 insertions(+), 44 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 3995169..1f06274 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -564,10 +564,12 @@ static int sd_major(int major_idx)
 	}
 }
 
-static struct scsi_disk *__scsi_disk_get(struct gendisk *disk)
+static struct scsi_disk *scsi_disk_get(struct gendisk *disk)
 {
 	struct scsi_disk *sdkp = NULL;
 
+	mutex_lock(&sd_ref_mutex);
+
 	if (disk->private_data) {
 		sdkp = scsi_disk(disk);
 		if (scsi_device_get(sdkp->device) == 0)
@@ -575,27 +577,6 @@ static struct scsi_disk *__scsi_disk_get(struct gendisk *disk)
 		else
 			sdkp = NULL;
 	}
-	return sdkp;
-}
-
-static struct scsi_disk *scsi_disk_get(struct gendisk *disk)
-{
-	struct scsi_disk *sdkp;
-
-	mutex_lock(&sd_ref_mutex);
-	sdkp = __scsi_disk_get(disk);
-	mutex_unlock(&sd_ref_mutex);
-	return sdkp;
-}
-
-static struct scsi_disk *scsi_disk_get_from_dev(struct device *dev)
-{
-	struct scsi_disk *sdkp;
-
-	mutex_lock(&sd_ref_mutex);
-	sdkp = dev_get_drvdata(dev);
-	if (sdkp)
-		sdkp = __scsi_disk_get(sdkp->disk);
 	mutex_unlock(&sd_ref_mutex);
 	return sdkp;
 }
@@ -610,8 +591,6 @@ static void scsi_disk_put(struct scsi_disk *sdkp)
 	mutex_unlock(&sd_ref_mutex);
 }
 
-
-
 static unsigned char sd_setup_protect_cmnd(struct scsi_cmnd *scmd,
 					   unsigned int dix, unsigned int dif)
 {
@@ -1525,12 +1504,9 @@ static int sd_sync_cache(struct scsi_disk *sdkp)
 
 static void sd_rescan(struct device *dev)
 {
-	struct scsi_disk *sdkp = scsi_disk_get_from_dev(dev);
+	struct scsi_disk *sdkp = dev_get_drvdata(dev);
 
-	if (sdkp) {
-		revalidate_disk(sdkp->disk);
-		scsi_disk_put(sdkp);
-	}
+	revalidate_disk(sdkp->disk);
 }
 
 
@@ -3147,13 +3123,13 @@ static int sd_start_stop_device(struct scsi_disk *sdkp, int start)
  */
 static void sd_shutdown(struct device *dev)
 {
-	struct scsi_disk *sdkp = scsi_disk_get_from_dev(dev);
+	struct scsi_disk *sdkp = dev_get_drvdata(dev);
 
 	if (!sdkp)
 		return;         /* this can happen */
 
 	if (pm_runtime_suspended(dev))
-		goto exit;
+		return;
 
 	if (sdkp->WCE && sdkp->media_present) {
 		sd_printk(KERN_NOTICE, sdkp, "Synchronizing SCSI cache\n");
@@ -3164,14 +3140,11 @@ static void sd_shutdown(struct device *dev)
 		sd_printk(KERN_NOTICE, sdkp, "Stopping disk\n");
 		sd_start_stop_device(sdkp, 0);
 	}
-
-exit:
-	scsi_disk_put(sdkp);
 }
 
 static int sd_suspend_common(struct device *dev, bool ignore_stop_errors)
 {
-	struct scsi_disk *sdkp = scsi_disk_get_from_dev(dev);
+	struct scsi_disk *sdkp = dev_get_drvdata(dev);
 	int ret = 0;
 
 	if (!sdkp)
@@ -3197,7 +3170,6 @@ static int sd_suspend_common(struct device *dev, bool ignore_stop_errors)
 	}
 
 done:
-	scsi_disk_put(sdkp);
 	return ret;
 }
 
@@ -3213,18 +3185,13 @@ static int sd_suspend_runtime(struct device *dev)
 
 static int sd_resume(struct device *dev)
 {
-	struct scsi_disk *sdkp = scsi_disk_get_from_dev(dev);
-	int ret = 0;
+	struct scsi_disk *sdkp = dev_get_drvdata(dev);
 
 	if (!sdkp->device->manage_start_stop)
-		goto done;
+		return 0;
 
 	sd_printk(KERN_NOTICE, sdkp, "Starting disk\n");
-	ret = sd_start_stop_device(sdkp, 1);
-
-done:
-	scsi_disk_put(sdkp);
-	return ret;
+	return sd_start_stop_device(sdkp, 1);
 }
 
 /**
-- 
1.9.1

[PATCH 3/3] scsi: proper state checking and module refcount handling in scsi_device_get

[Christoph Hellwig]

This effectively reverts commits 85b6c7 ("[SCSI] sd: fix cache flushing on
module removal (and individual device removal)" and dc4515ea ("scsi: always
increment reference count").

We now never call scsi_device_get from the shutdown path, and the fact
that we started grabbing reference there in commit 85b6c7 turned out
turned out to create more problems than it solves, and required
workarounds for workarounds for workarounds. Move back to properly checking
the device state and carefully handle module refcounting.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/scsi/scsi.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 9b38299..95f0293 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -982,15 +982,18 @@ EXPORT_SYMBOL(scsi_report_opcode);
  */
 int scsi_device_get(struct scsi_device *sdev)
 {
-	if (sdev->sdev_state == SDEV_DEL)
-		return -ENXIO;
+	if (sdev->sdev_state == SDEV_DEL || sdev->sdev_state == SDEV_CANCEL)
+		goto fail;
 	if (!get_device(&sdev->sdev_gendev))
-		return -ENXIO;
-	/* We can fail try_module_get if we're doing SCSI operations
-	 * from module exit (like cache flush) */
-	__module_get(sdev->host->hostt->module);
-
+		goto fail;
+	if (!try_module_get(sdev->host->hostt->module))
+		goto fail_put_device;
 	return 0;
+
+fail_put_device:
+	put_device(&sdev->sdev_gendev);
+fail:
+	return -ENXIO;
 }
 EXPORT_SYMBOL(scsi_device_get);
 
-- 
1.9.1

Re: [PATCH 3/3] scsi: proper state checking and module refcount handling in scsi_device_get

[James Bottomley]

On Thu, 2015-01-29 at 00:00 +0100, Christoph Hellwig wrote:
> This effectively reverts commits 85b6c7 ("[SCSI] sd: fix cache flushing on
> module removal (and individual device removal)" and dc4515ea ("scsi: always
> increment reference count").
> 
> We now never call scsi_device_get from the shutdown path, and the fact
> that we started grabbing reference there in commit 85b6c7 turned out
> turned out to create more problems than it solves, and required
> workarounds for workarounds for workarounds. Move back to properly checking
> the device state and carefully handle module refcounting.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/scsi/scsi.c | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> index 9b38299..95f0293 100644
> --- a/drivers/scsi/scsi.c
> +++ b/drivers/scsi/scsi.c
> @@ -982,15 +982,18 @@ EXPORT_SYMBOL(scsi_report_opcode);
>   */
>  int scsi_device_get(struct scsi_device *sdev)
>  {
> -	if (sdev->sdev_state == SDEV_DEL)
> -		return -ENXIO;
> +	if (sdev->sdev_state == SDEV_DEL || sdev->sdev_state == SDEV_CANCEL)
> +		goto fail;
>  	if (!get_device(&sdev->sdev_gendev))
> -		return -ENXIO;
> -	/* We can fail try_module_get if we're doing SCSI operations
> -	 * from module exit (like cache flush) */
> -	__module_get(sdev->host->hostt->module);
> -
> +		goto fail;
> +	if (!try_module_get(sdev->host->hostt->module))
> +		goto fail_put_device;
>  	return 0;
> +
> +fail_put_device:
> +	put_device(&sdev->sdev_gendev);
> +fail:
> +	return -ENXIO;
>  }
>  EXPORT_SYMBOL(scsi_device_get);

If we can get away with this, I'm all for this approach.  However, you
need to document in a comment or above the function that it may not be
called in module exit functions and why.

Other than the comment issue, the series looks good,

Thanks,

James

Re: [PATCH 1/3] scsi: serialize ->rescan against ->remove

[Alan Stern]

On Thu, 29 Jan 2015, Christoph Hellwig wrote:

> Lock the device embedded in the scsi_device to protect against
> concurrent calls to ->remove.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/scsi/scsi_scan.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> index 983aed1..523faee 100644
> --- a/drivers/scsi/scsi_scan.c
> +++ b/drivers/scsi/scsi_scan.c
> @@ -1593,16 +1593,15 @@ EXPORT_SYMBOL(scsi_add_device);
>  
>  void scsi_rescan_device(struct device *dev)
>  {
> -	if (!dev->driver)
> -		return;
> -
> -	if (try_module_get(dev->driver->owner)) {
> +	device_lock(dev);
> +	if (dev->driver && try_module_get(dev->driver->owner)) {
>  		struct scsi_driver *drv = to_scsi_driver(dev->driver);
>  
>  		if (drv->rescan)
>  			drv->rescan(dev);
>  		module_put(dev->driver->owner);
>  	}
> +	device_unlock(dev);
>  }
>  EXPORT_SYMBOL(scsi_rescan_device);

Acked-by: Alan Stern <stern@rowland.harvard.edu>

Re: [PATCH 1/3] scsi: serialize ->rescan against ->remove

[Paolo Bonzini]

On 29/01/2015 00:00, Christoph Hellwig wrote:
> Lock the device embedded in the scsi_device to protect against
> concurrent calls to ->remove.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

I wonder if this makes this problem: https://lkml.org/lkml/2015/1/5/9 go
away.

Paolo

> ---
>  drivers/scsi/scsi_scan.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> index 983aed1..523faee 100644
> --- a/drivers/scsi/scsi_scan.c
> +++ b/drivers/scsi/scsi_scan.c
> @@ -1593,16 +1593,15 @@ EXPORT_SYMBOL(scsi_add_device);
>  
>  void scsi_rescan_device(struct device *dev)
>  {
> -	if (!dev->driver)
> -		return;
> -
> -	if (try_module_get(dev->driver->owner)) {
> +	device_lock(dev);
> +	if (dev->driver && try_module_get(dev->driver->owner)) {
>  		struct scsi_driver *drv = to_scsi_driver(dev->driver);
>  
>  		if (drv->rescan)
>  			drv->rescan(dev);
>  		module_put(dev->driver->owner);
>  	}
> +	device_unlock(dev);
>  }
>  EXPORT_SYMBOL(scsi_rescan_device);
>  
> 

Re: [PATCH 1/3] scsi: serialize ->rescan against ->remove

[Fam Zheng]

On Fri, 01/30 00:11, Paolo Bonzini wrote:
> 
> 
> On 29/01/2015 00:00, Christoph Hellwig wrote:
> > Lock the device embedded in the scsi_device to protect against
> > concurrent calls to ->remove.
> > 
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> I wonder if this makes this problem: https://lkml.org/lkml/2015/1/5/9 go
> away.

A quick test says yes.

Fam

> 
> Paolo
> 
> > ---
> >  drivers/scsi/scsi_scan.c | 7 +++----
> >  1 file changed, 3 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> > index 983aed1..523faee 100644
> > --- a/drivers/scsi/scsi_scan.c
> > +++ b/drivers/scsi/scsi_scan.c
> > @@ -1593,16 +1593,15 @@ EXPORT_SYMBOL(scsi_add_device);
> >  
> >  void scsi_rescan_device(struct device *dev)
> >  {
> > -	if (!dev->driver)
> > -		return;
> > -
> > -	if (try_module_get(dev->driver->owner)) {
> > +	device_lock(dev);
> > +	if (dev->driver && try_module_get(dev->driver->owner)) {
> >  		struct scsi_driver *drv = to_scsi_driver(dev->driver);
> >  
> >  		if (drv->rescan)
> >  			drv->rescan(dev);
> >  		module_put(dev->driver->owner);
> >  	}
> > +	device_unlock(dev);
> >  }
> >  EXPORT_SYMBOL(scsi_rescan_device);
> >  
> > 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] scsi: serialize ->rescan against ->remove

[Paolo Bonzini]

On 30/01/2015 02:08, Fam Zheng wrote:
> On Fri, 01/30 00:11, Paolo Bonzini wrote:
>>
>>
>> On 29/01/2015 00:00, Christoph Hellwig wrote:
>>> Lock the device embedded in the scsi_device to protect against
>>> concurrent calls to ->remove.
>>>
>>> Signed-off-by: Christoph Hellwig <hch@lst.de>
>>
>> I wonder if this makes this problem: https://lkml.org/lkml/2015/1/5/9 go
>> away.
> 
> A quick test says yes.

Great, we might want to revert that patch in 3.21.

Paolo

Re: [PATCH 1/3] scsi: serialize ->rescan against ->remove

[Christoph Hellwig]

On Fri, Jan 30, 2015 at 10:46:17AM +0100, Paolo Bonzini wrote:
> Great, we might want to revert that patch in 3.21.

Is that fix in any tree yet?  Seems like I missed it for the scsi
tree at least.  So unless you want it for 3.19/stable we might as well
ust skip that patch.

Re: [PATCH 1/3] scsi: serialize ->rescan against ->remove

[Paolo Bonzini]

On 02/02/2015 13:59, Christoph Hellwig wrote:
> On Fri, Jan 30, 2015 at 10:46:17AM +0100, Paolo Bonzini wrote:
>> > Great, we might want to revert that patch in 3.21.
> Is that fix in any tree yet?  Seems like I missed it for the scsi
> tree at least.  So unless you want it for 3.19/stable we might as well
> ust skip that patch.

Yes, I agree.

Paolo

Re: [PATCH 3/3] scsi: proper state checking and module refcount handling in scsi_device_get

[Christoph Hellwig]

On Wed, Apr 29, 2015 at 10:17:59AM +0900, Akinobu Mita wrote:
> This change broke ufs driver.

I'd claim the ufs driver, or rather more specifily the ufs spec
had already been broken.  That whole concept of keeping references
to scsi devices to send commands to for host state changes is just
fundamentally broken.  

> The reason for scsi_device_get() in ufshcd_set_dev_pwr_mode() is
> to avoid manual delete of UFS device W-LUN by holding the reference
> to it.  So can we acquire shost->scan_mutex lock instead of
> scsi_device_get()?  I tried attached patch and it seems to be working,
> but I would like to ask your opinion about this change.

It seems like a reasonable workaround.  But then again the concept
of the driver hanging on scsi_devices is and will stay broken.

Re: [PATCH 3/3] scsi: proper state checking and module refcount handling in scsi_device_get

[Akinobu Mita]

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

2015-02-02 22:01 GMT+09:00 Christoph Hellwig <hch@lst.de>:
> This effectively reverts commits 85b6c7 ("[SCSI] sd: fix cache flushing on
> module removal (and individual device removal)" and dc4515ea ("scsi: always
> increment reference count").
>
> We now never call scsi_device_get from the shutdown path, and the fact
> that we started grabbing reference there in commit 85b6c7 turned out
> turned out to create more problems than it solves, and required
> workarounds for workarounds for workarounds. Move back to properly checking
> the device state and carefully handle module refcounting.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/scsi/scsi.c | 20 +++++++++++++-------
>  1 file changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> index 9b38299..9b7fd0b 100644
> --- a/drivers/scsi/scsi.c
> +++ b/drivers/scsi/scsi.c
> @@ -979,18 +979,24 @@ EXPORT_SYMBOL(scsi_report_opcode);
>   * Description: Gets a reference to the scsi_device and increments the use count
>   * of the underlying LLDD module.  You must hold host_lock of the
>   * parent Scsi_Host or already have a reference when calling this.
> + *
> + * This will fail if a device is deleted or cancelled, or when the LLD module
> + * is in the process of being unloaded.
>   */
>  int scsi_device_get(struct scsi_device *sdev)

Hi Christoph,

This change broke ufs driver.

Because scsi_device_get() can be called while the module is being
unloaded with the device 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)

The reason for scsi_device_get() in ufshcd_set_dev_pwr_mode() is
to avoid manual delete of UFS device W-LUN by holding the reference
to it.  So can we acquire shost->scan_mutex lock instead of
scsi_device_get()?  I tried attached patch and it seems to be working,
but I would like to ask your opinion about this change.

>  {
> -       if (sdev->sdev_state == SDEV_DEL)
> -               return -ENXIO;
> +       if (sdev->sdev_state == SDEV_DEL || sdev->sdev_state == SDEV_CANCEL)
> +               goto fail;
>         if (!get_device(&sdev->sdev_gendev))
> -               return -ENXIO;
> -       /* We can fail try_module_get if we're doing SCSI operations
> -        * from module exit (like cache flush) */
> -       __module_get(sdev->host->hostt->module);
> -
> +               goto fail;
> +       if (!try_module_get(sdev->host->hostt->module))
> +               goto fail_put_device;
>         return 0;
> +
> +fail_put_device:
> +       put_device(&sdev->sdev_gendev);
> +fail:
> +       return -ENXIO;
>  }
>  EXPORT_SYMBOL(scsi_device_get);
>
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[-- Attachment #2: 0001-scsi-ufs-fix-ufshcd_set_dev_pwr_mode-when-unloading-.patch --]
[-- Type: text/x-patch, Size: 1492 bytes --]

From a305a5284cac23adbf7f86b3014cc2e6325c7b88 Mon Sep 17 00:00:00 2001
From: Akinobu Mita <akinobu.mita@gmail.com>
Date: Wed, 29 Apr 2015 10:02:17 +0900
Subject: [PATCH] scsi: ufs: fix ufshcd_set_dev_pwr_mode() when unloading
 module

---
 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 540e00d..91cbc04 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -4695,23 +4695,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-
@@ -4748,7 +4744,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

Re: [PATCH 3/3] scsi: proper state checking and module refcount handling in scsi_device_get

[Hannes Reinecke]

On 02/02/2015 08:01 AM, Christoph Hellwig wrote:
> This effectively reverts commits 85b6c7 ("[SCSI] sd: fix cache flushing on
> module removal (and individual device removal)" and dc4515ea ("scsi: always
> increment reference count").
> 
> We now never call scsi_device_get from the shutdown path, and the fact
> that we started grabbing reference there in commit 85b6c7 turned out
> turned out to create more problems than it solves, and required
> workarounds for workarounds for workarounds. Move back to properly checking
> the device state and carefully handle module refcounting.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

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

Re: [PATCH 3/3] scsi: proper state checking and module refcount handling in scsi_device_get

[Paolo Bonzini]

On 02/02/2015 14:01, Christoph Hellwig wrote:
> This effectively reverts commits 85b6c7 ("[SCSI] sd: fix cache flushing on
> module removal (and individual device removal)" and dc4515ea ("scsi: always
> increment reference count").
> 
> We now never call scsi_device_get from the shutdown path, and the fact
> that we started grabbing reference there in commit 85b6c7 turned out
> turned out to create more problems than it solves, and required
> workarounds for workarounds for workarounds. Move back to properly checking
> the device state and carefully handle module refcounting.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/scsi/scsi.c | 20 +++++++++++++-------
>  1 file changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> index 9b38299..9b7fd0b 100644
> --- a/drivers/scsi/scsi.c
> +++ b/drivers/scsi/scsi.c
> @@ -979,18 +979,24 @@ EXPORT_SYMBOL(scsi_report_opcode);
>   * Description: Gets a reference to the scsi_device and increments the use count
>   * of the underlying LLDD module.  You must hold host_lock of the
>   * parent Scsi_Host or already have a reference when calling this.
> + *
> + * This will fail if a device is deleted or cancelled, or when the LLD module
> + * is in the process of being unloaded.
>   */
>  int scsi_device_get(struct scsi_device *sdev)
>  {
> -	if (sdev->sdev_state == SDEV_DEL)
> -		return -ENXIO;
> +	if (sdev->sdev_state == SDEV_DEL || sdev->sdev_state == SDEV_CANCEL)
> +		goto fail;
>  	if (!get_device(&sdev->sdev_gendev))
> -		return -ENXIO;
> -	/* We can fail try_module_get if we're doing SCSI operations
> -	 * from module exit (like cache flush) */
> -	__module_get(sdev->host->hostt->module);
> -
> +		goto fail;
> +	if (!try_module_get(sdev->host->hostt->module))
> +		goto fail_put_device;
>  	return 0;
> +
> +fail_put_device:
> +	put_device(&sdev->sdev_gendev);
> +fail:
> +	return -ENXIO;
>  }
>  EXPORT_SYMBOL(scsi_device_get);
>  
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

[PATCH 3/3] scsi: proper state checking and module refcount handling in scsi_device_get

[Christoph Hellwig]

This effectively reverts commits 85b6c7 ("[SCSI] sd: fix cache flushing on
module removal (and individual device removal)" and dc4515ea ("scsi: always
increment reference count").

We now never call scsi_device_get from the shutdown path, and the fact
that we started grabbing reference there in commit 85b6c7 turned out
turned out to create more problems than it solves, and required
workarounds for workarounds for workarounds. Move back to properly checking
the device state and carefully handle module refcounting.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/scsi/scsi.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 9b38299..9b7fd0b 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -979,18 +979,24 @@ EXPORT_SYMBOL(scsi_report_opcode);
  * Description: Gets a reference to the scsi_device and increments the use count
  * of the underlying LLDD module.  You must hold host_lock of the
  * parent Scsi_Host or already have a reference when calling this.
+ *
+ * This will fail if a device is deleted or cancelled, or when the LLD module
+ * is in the process of being unloaded.
  */
 int scsi_device_get(struct scsi_device *sdev)
 {
-	if (sdev->sdev_state == SDEV_DEL)
-		return -ENXIO;
+	if (sdev->sdev_state == SDEV_DEL || sdev->sdev_state == SDEV_CANCEL)
+		goto fail;
 	if (!get_device(&sdev->sdev_gendev))
-		return -ENXIO;
-	/* We can fail try_module_get if we're doing SCSI operations
-	 * from module exit (like cache flush) */
-	__module_get(sdev->host->hostt->module);
-
+		goto fail;
+	if (!try_module_get(sdev->host->hostt->module))
+		goto fail_put_device;
 	return 0;
+
+fail_put_device:
+	put_device(&sdev->sdev_gendev);
+fail:
+	return -ENXIO;
 }
 EXPORT_SYMBOL(scsi_device_get);
 
-- 
1.9.1