[PATCH] Driver 'sd' needs updating

[PATCH] Driver 'sd' needs updating

[Hannes Reinecke]

If a SCSI ULD driver sets blk_queue_prep_rq(), it should clean it
up itself on remove(), and not from the bus callbacks. This
removes the need to hook into bus->remove(), which should not
be used at the same time as driver->remove().

Signed-off-by: Hannes Reinecke <hare@suse.de>
Signed-off-by: Kay Sievers <kay.sievers@vrfy.org>
---
 drivers/scsi/scsi_lib.c    |    1 +
 drivers/scsi/scsi_priv.h   |    1 -
 drivers/scsi/scsi_sysfs.c  |   17 -----------------
 drivers/scsi/sd.c          |    1 +
 drivers/scsi/sr.c          |    1 +
 include/scsi/scsi_driver.h |    1 +
 6 files changed, 4 insertions(+), 18 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 30f3275..f3c4089 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1207,6 +1207,7 @@ int scsi_prep_fn(struct request_queue *q, struct request *req)
 		ret = scsi_setup_blk_pc_cmnd(sdev, req);
 	return scsi_prep_return(q, req, ret);
 }
+EXPORT_SYMBOL(scsi_prep_fn);
 
 /*
  * scsi_dev_queue_ready: if we can send requests to sdev, return 1 else
diff --git a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h
index fbc83be..1ade801 100644
--- a/drivers/scsi/scsi_priv.h
+++ b/drivers/scsi/scsi_priv.h
@@ -71,7 +71,6 @@ extern int scsi_init_queue(void);
 extern void scsi_exit_queue(void);
 struct request_queue;
 struct request;
-extern int scsi_prep_fn(struct request_queue *, struct request *);
 extern struct kmem_cache *scsi_sdb_cache;
 
 /* scsi_proc.c */
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index fa4711d..91482f2 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -420,29 +420,12 @@ static int scsi_bus_resume(struct device * dev)
 	return err;
 }
 
-static int scsi_bus_remove(struct device *dev)
-{
-	struct device_driver *drv = dev->driver;
-	struct scsi_device *sdev = to_scsi_device(dev);
-	int err = 0;
-
-	/* reset the prep_fn back to the default since the
-	 * driver may have altered it and it's being removed */
-	blk_queue_prep_rq(sdev->request_queue, scsi_prep_fn);
-
-	if (drv && drv->remove)
-		err = drv->remove(dev);
-
-	return 0;
-}
-
 struct bus_type scsi_bus_type = {
         .name		= "scsi",
         .match		= scsi_bus_match,
 	.uevent		= scsi_bus_uevent,
 	.suspend	= scsi_bus_suspend,
 	.resume		= scsi_bus_resume,
-	.remove		= scsi_bus_remove,
 };
 EXPORT_SYMBOL_GPL(scsi_bus_type);
 
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 878b17a..93c4bed 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2053,6 +2053,7 @@ static int sd_remove(struct device *dev)
 	struct scsi_disk *sdkp;
 
 	async_synchronize_full();
+	blk_queue_prep_rq(sdkp->device->request_queue, scsi_prep_fn);
 	sdkp = dev_get_drvdata(dev);
 	device_del(&sdkp->dev);
 	del_gendisk(sdkp->disk);
diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index cd350df..cce0fe4 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -881,6 +881,7 @@ static int sr_remove(struct device *dev)
 {
 	struct scsi_cd *cd = dev_get_drvdata(dev);
 
+	blk_queue_prep_rq(cd->device->request_queue, scsi_prep_fn);
 	del_gendisk(cd->disk);
 
 	mutex_lock(&sr_ref_mutex);
diff --git a/include/scsi/scsi_driver.h b/include/scsi/scsi_driver.h
index 1f5ca7f..9fd6702 100644
--- a/include/scsi/scsi_driver.h
+++ b/include/scsi/scsi_driver.h
@@ -32,5 +32,6 @@ int scsi_setup_blk_pc_cmnd(struct scsi_device *sdev, struct request *req);
 int scsi_setup_fs_cmnd(struct scsi_device *sdev, struct request *req);
 int scsi_prep_state_check(struct scsi_device *sdev, struct request *req);
 int scsi_prep_return(struct request_queue *q, struct request *req, int ret);
+int scsi_prep_fn(struct request_queue *, struct request *);
 
 #endif /* _SCSI_SCSI_DRIVER_H */
-- 
1.5.3.2

Re: [PATCH] Driver 'sd' needs updating

[James Bottomley]

On Thu, 2009-06-18 at 09:57 +0200, Hannes Reinecke wrote:
> If a SCSI ULD driver sets blk_queue_prep_rq(), it should clean it
> up itself on remove(), and not from the bus callbacks. This
> removes the need to hook into bus->remove(), which should not
> be used at the same time as driver->remove().
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> Signed-off-by: Kay Sievers <kay.sievers@vrfy.org>
> ---
>  drivers/scsi/scsi_lib.c    |    1 +
>  drivers/scsi/scsi_priv.h   |    1 -
>  drivers/scsi/scsi_sysfs.c  |   17 -----------------
>  drivers/scsi/sd.c          |    1 +
>  drivers/scsi/sr.c          |    1 +
>  include/scsi/scsi_driver.h |    1 +
>  6 files changed, 4 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 30f3275..f3c4089 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -1207,6 +1207,7 @@ int scsi_prep_fn(struct request_queue *q, struct request *req)
>  		ret = scsi_setup_blk_pc_cmnd(sdev, req);
>  	return scsi_prep_return(q, req, ret);
>  }
> +EXPORT_SYMBOL(scsi_prep_fn);
>  
>  /*
>   * scsi_dev_queue_ready: if we can send requests to sdev, return 1 else
> diff --git a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h
> index fbc83be..1ade801 100644
> --- a/drivers/scsi/scsi_priv.h
> +++ b/drivers/scsi/scsi_priv.h
> @@ -71,7 +71,6 @@ extern int scsi_init_queue(void);
>  extern void scsi_exit_queue(void);
>  struct request_queue;
>  struct request;
> -extern int scsi_prep_fn(struct request_queue *, struct request *);
>  extern struct kmem_cache *scsi_sdb_cache;
>  
>  /* scsi_proc.c */
> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
> index fa4711d..91482f2 100644
> --- a/drivers/scsi/scsi_sysfs.c
> +++ b/drivers/scsi/scsi_sysfs.c
> @@ -420,29 +420,12 @@ static int scsi_bus_resume(struct device * dev)
>  	return err;
>  }
>  
> -static int scsi_bus_remove(struct device *dev)
> -{
> -	struct device_driver *drv = dev->driver;
> -	struct scsi_device *sdev = to_scsi_device(dev);
> -	int err = 0;
> -
> -	/* reset the prep_fn back to the default since the
> -	 * driver may have altered it and it's being removed */
> -	blk_queue_prep_rq(sdev->request_queue, scsi_prep_fn);
> -
> -	if (drv && drv->remove)
> -		err = drv->remove(dev);
> -
> -	return 0;
> -}
> -
>  struct bus_type scsi_bus_type = {
>          .name		= "scsi",
>          .match		= scsi_bus_match,
>  	.uevent		= scsi_bus_uevent,
>  	.suspend	= scsi_bus_suspend,
>  	.resume		= scsi_bus_resume,
> -	.remove		= scsi_bus_remove,
>  };
>  EXPORT_SYMBOL_GPL(scsi_bus_type);
>  
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 878b17a..93c4bed 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -2053,6 +2053,7 @@ static int sd_remove(struct device *dev)
>  	struct scsi_disk *sdkp;
>  
>  	async_synchronize_full();
> +	blk_queue_prep_rq(sdkp->device->request_queue, scsi_prep_fn);
>  	sdkp = dev_get_drvdata(dev);

This isn't really going to work ... you're using sdkp before it gets
initialised ... did this actually get tested?

James

Re: [PATCH] Driver 'sd' needs updating

[Boaz Harrosh]

On 06/19/2009 06:41 AM, James Bottomley wrote:
> On Thu, 2009-06-18 at 09:57 +0200, Hannes Reinecke wrote:
>> If a SCSI ULD driver sets blk_queue_prep_rq(), it should clean it
>> up itself on remove(), and not from the bus callbacks. This
>> removes the need to hook into bus->remove(), which should not
>> be used at the same time as driver->remove().
>>
>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>> Signed-off-by: Kay Sievers <kay.sievers@vrfy.org>
>> ---
>>  drivers/scsi/scsi_lib.c    |    1 +
>>  drivers/scsi/scsi_priv.h   |    1 -
>>  drivers/scsi/scsi_sysfs.c  |   17 -----------------
>>  drivers/scsi/sd.c          |    1 +
>>  drivers/scsi/sr.c          |    1 +
>>  include/scsi/scsi_driver.h |    1 +
>>  6 files changed, 4 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
>> index 30f3275..f3c4089 100644
>> --- a/drivers/scsi/scsi_lib.c
>> +++ b/drivers/scsi/scsi_lib.c
>> @@ -1207,6 +1207,7 @@ int scsi_prep_fn(struct request_queue *q, struct request *req)
>>  		ret = scsi_setup_blk_pc_cmnd(sdev, req);
>>  	return scsi_prep_return(q, req, ret);
>>  }
>> +EXPORT_SYMBOL(scsi_prep_fn);
>>  
>>  /*
>>   * scsi_dev_queue_ready: if we can send requests to sdev, return 1 else
>> diff --git a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h
>> index fbc83be..1ade801 100644
>> --- a/drivers/scsi/scsi_priv.h
>> +++ b/drivers/scsi/scsi_priv.h
>> @@ -71,7 +71,6 @@ extern int scsi_init_queue(void);
>>  extern void scsi_exit_queue(void);
>>  struct request_queue;
>>  struct request;
>> -extern int scsi_prep_fn(struct request_queue *, struct request *);
>>  extern struct kmem_cache *scsi_sdb_cache;
>>  
>>  /* scsi_proc.c */
>> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
>> index fa4711d..91482f2 100644
>> --- a/drivers/scsi/scsi_sysfs.c
>> +++ b/drivers/scsi/scsi_sysfs.c
>> @@ -420,29 +420,12 @@ static int scsi_bus_resume(struct device * dev)
>>  	return err;
>>  }
>>  
>> -static int scsi_bus_remove(struct device *dev)
>> -{
>> -	struct device_driver *drv = dev->driver;
>> -	struct scsi_device *sdev = to_scsi_device(dev);
>> -	int err = 0;
>> -
>> -	/* reset the prep_fn back to the default since the
>> -	 * driver may have altered it and it's being removed */
>> -	blk_queue_prep_rq(sdev->request_queue, scsi_prep_fn);
>> -
>> -	if (drv && drv->remove)
>> -		err = drv->remove(dev);
>> -
>> -	return 0;
>> -}
>> -
>>  struct bus_type scsi_bus_type = {
>>          .name		= "scsi",
>>          .match		= scsi_bus_match,
>>  	.uevent		= scsi_bus_uevent,
>>  	.suspend	= scsi_bus_suspend,
>>  	.resume		= scsi_bus_resume,
>> -	.remove		= scsi_bus_remove,
>>  };
>>  EXPORT_SYMBOL_GPL(scsi_bus_type);
>>  
>> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
>> index 878b17a..93c4bed 100644
>> --- a/drivers/scsi/sd.c
>> +++ b/drivers/scsi/sd.c
>> @@ -2053,6 +2053,7 @@ static int sd_remove(struct device *dev)
>>  	struct scsi_disk *sdkp;
>>  
>>  	async_synchronize_full();
>> +	blk_queue_prep_rq(sdkp->device->request_queue, scsi_prep_fn);
>>  	sdkp = dev_get_drvdata(dev);
> 
> This isn't really going to work ... you're using sdkp before it gets
> initialised ... did this actually get tested?
> 

I have tested the previous iteration heavily. But yes this looks like an
uncareful rebase fallout. The last two lines should just be exchanged.

> James
> 

Thanks
Boaz

Re: [PATCH] Driver 'sd' needs updating

[Boaz Harrosh]

On 06/21/2009 11:41 AM, Boaz Harrosh wrote:
> On 06/19/2009 06:41 AM, James Bottomley wrote:
>> On Thu, 2009-06-18 at 09:57 +0200, Hannes Reinecke wrote:
>>> If a SCSI ULD driver sets blk_queue_prep_rq(), it should clean it
>>> up itself on remove(), and not from the bus callbacks. This
>>> removes the need to hook into bus->remove(), which should not
>>> be used at the same time as driver->remove().
>>>
>>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>>> Signed-off-by: Kay Sievers <kay.sievers@vrfy.org>
>>> ---
<snip>

>>> @@ -2053,6 +2053,7 @@ static int sd_remove(struct device *dev)
>>>  	struct scsi_disk *sdkp;
>>>  
>>>  	async_synchronize_full();
>>> +	blk_queue_prep_rq(sdkp->device->request_queue, scsi_prep_fn);
>>>  	sdkp = dev_get_drvdata(dev);
>> This isn't really going to work ... you're using sdkp before it gets
>> initialised ... did this actually get tested?
>>
> 
> I have tested the previous iteration heavily. But yes this looks like an
> uncareful rebase fallout. The last two lines should just be exchanged.
> 

I have now *tested* with the above blk_queue_prep_rq() call just after the
"sdkp = dev_get_drvdata(dev);" 

Thanks
Boaz
---
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 93c4bed..a6bb147 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2053,8 +2053,8 @@ static int sd_remove(struct device *dev)
 	struct scsi_disk *sdkp;
 
 	async_synchronize_full();
-	blk_queue_prep_rq(sdkp->device->request_queue, scsi_prep_fn);
 	sdkp = dev_get_drvdata(dev);
+	blk_queue_prep_rq(sdkp->device->request_queue, scsi_prep_fn);
 	device_del(&sdkp->dev);
 	del_gendisk(sdkp->disk);
 	sd_shutdown(dev);

[PATCH] Driver 'sd' needs updating

[Hannes Reinecke]

If a driver sets blk_queue_prep_rq() it should clean up itself
and not rely on the bus callbacks to handle this. This removes
the need to hook into bus->remove() as these should not be used
at the same time as driver->remove().

Signed-off-by: Hannes Reinecke <hare@suse.de>
Signed-off-by: Kay Sievers <kay.sievers@vrfy.org>
---
 drivers/scsi/scsi_lib.c    |    6 ++++++
 drivers/scsi/scsi_sysfs.c  |   17 -----------------
 drivers/scsi/sd.c          |    2 ++
 drivers/scsi/sr.c          |    1 +
 include/scsi/scsi_driver.h |    1 +
 5 files changed, 10 insertions(+), 17 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 4b13e36..73df41b 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1222,6 +1222,12 @@ int scsi_prep_fn(struct request_queue *q, struct request *req)
 	return scsi_prep_return(q, req, ret);
 }
 
+void scsi_reset_prep_fn(struct request_queue *q)
+{
+	blk_queue_prep_rq(q, scsi_prep_fn);
+}
+EXPORT_SYMBOL(scsi_reset_prep_fn);
+
 /*
  * scsi_dev_queue_ready: if we can send requests to sdev, return 1 else
  * return 0.
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index fa4711d..91482f2 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -420,29 +420,12 @@ static int scsi_bus_resume(struct device * dev)
 	return err;
 }
 
-static int scsi_bus_remove(struct device *dev)
-{
-	struct device_driver *drv = dev->driver;
-	struct scsi_device *sdev = to_scsi_device(dev);
-	int err = 0;
-
-	/* reset the prep_fn back to the default since the
-	 * driver may have altered it and it's being removed */
-	blk_queue_prep_rq(sdev->request_queue, scsi_prep_fn);
-
-	if (drv && drv->remove)
-		err = drv->remove(dev);
-
-	return 0;
-}
-
 struct bus_type scsi_bus_type = {
         .name		= "scsi",
         .match		= scsi_bus_match,
 	.uevent		= scsi_bus_uevent,
 	.suspend	= scsi_bus_suspend,
 	.resume		= scsi_bus_resume,
-	.remove		= scsi_bus_remove,
 };
 EXPORT_SYMBOL_GPL(scsi_bus_type);
 
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index aeab5d9..64e88e2 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2068,6 +2068,8 @@ static int sd_remove(struct device *dev)
 {
 	struct scsi_disk *sdkp = dev_get_drvdata(dev);
 
+	scsi_reset_prep_fn(sdkp->device->request_queue);
+
 	device_del(&sdkp->dev);
 	del_gendisk(sdkp->disk);
 	sd_shutdown(dev);
diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index e7fa3ca..914733a 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -889,6 +889,7 @@ static int sr_remove(struct device *dev)
 {
 	struct scsi_cd *cd = dev_get_drvdata(dev);
 
+	scsi_reset_prep_fn(cd->device->request_queue);
 	del_gendisk(cd->disk);
 
 	mutex_lock(&sr_ref_mutex);
diff --git a/include/scsi/scsi_driver.h b/include/scsi/scsi_driver.h
index 1f5ca7f..2e22929 100644
--- a/include/scsi/scsi_driver.h
+++ b/include/scsi/scsi_driver.h
@@ -32,5 +32,6 @@ int scsi_setup_blk_pc_cmnd(struct scsi_device *sdev, struct request *req);
 int scsi_setup_fs_cmnd(struct scsi_device *sdev, struct request *req);
 int scsi_prep_state_check(struct scsi_device *sdev, struct request *req);
 int scsi_prep_return(struct request_queue *q, struct request *req, int ret);
+int scsi_reset_prep_fn(struct request_queue *);
 
 #endif /* _SCSI_SCSI_DRIVER_H */
-- 
1.5.3.2

Re: [PATCH] Driver 'sd' needs updating

[Boaz Harrosh]

Hannes Reinecke wrote:
> If a driver sets blk_queue_prep_rq() it should clean up itself
> and not rely on the bus callbacks to handle this. This removes
> the need to hook into bus->remove() as these should not be used
> at the same time as driver->remove().
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> Signed-off-by: Kay Sievers <kay.sievers@vrfy.org>

Hi Hannes, please I do not understand this patch.

I was always under the impression that the scsi ULD
remove() function is called from inside scsi_bus_remove()
at the very end, please see below.

> ---
>  drivers/scsi/scsi_lib.c    |    6 ++++++
>  drivers/scsi/scsi_sysfs.c  |   17 -----------------
>  drivers/scsi/sd.c          |    2 ++
>  drivers/scsi/sr.c          |    1 +
>  include/scsi/scsi_driver.h |    1 +
>  5 files changed, 10 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 4b13e36..73df41b 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -1222,6 +1222,12 @@ int scsi_prep_fn(struct request_queue *q, struct request *req)
>  	return scsi_prep_return(q, req, ret);
>  }
>  
> +void scsi_reset_prep_fn(struct request_queue *q)
> +{
> +	blk_queue_prep_rq(q, scsi_prep_fn);
> +}
> +EXPORT_SYMBOL(scsi_reset_prep_fn);
> +
>  /*
>   * scsi_dev_queue_ready: if we can send requests to sdev, return 1 else
>   * return 0.
> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
> index fa4711d..91482f2 100644
> --- a/drivers/scsi/scsi_sysfs.c
> +++ b/drivers/scsi/scsi_sysfs.c
> @@ -420,29 +420,12 @@ static int scsi_bus_resume(struct device * dev)
>  	return err;
>  }
>  
> -static int scsi_bus_remove(struct device *dev)
> -{
> -	struct device_driver *drv = dev->driver;
> -	struct scsi_device *sdev = to_scsi_device(dev);
> -	int err = 0;
> -
> -	/* reset the prep_fn back to the default since the
> -	 * driver may have altered it and it's being removed */
> -	blk_queue_prep_rq(sdev->request_queue, scsi_prep_fn);
> -
> -	if (drv && drv->remove)
> -		err = drv->remove(dev);

This here is where my osd_uld.c::osd_remove() is called.

If this vector is not used will the drv-core call drv->remove(dev)
in behalf of scsi?

> -
> -	return 0;
> -}
> -
>  struct bus_type scsi_bus_type = {
>          .name		= "scsi",
>          .match		= scsi_bus_match,
>  	.uevent		= scsi_bus_uevent,
>  	.suspend	= scsi_bus_suspend,
>  	.resume		= scsi_bus_resume,
> -	.remove		= scsi_bus_remove,
>  };
>  EXPORT_SYMBOL_GPL(scsi_bus_type);
>  
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index aeab5d9..64e88e2 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -2068,6 +2068,8 @@ static int sd_remove(struct device *dev)

Until today this was called from within scsi_bus_remove
what happens after your patch?

>  {
>  	struct scsi_disk *sdkp = dev_get_drvdata(dev);
>  
> +	scsi_reset_prep_fn(sdkp->device->request_queue);
> +
>  	device_del(&sdkp->dev);
>  	del_gendisk(sdkp->disk);
>  	sd_shutdown(dev);
> diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
> index e7fa3ca..914733a 100644
> --- a/drivers/scsi/sr.c
> +++ b/drivers/scsi/sr.c
> @@ -889,6 +889,7 @@ static int sr_remove(struct device *dev)
>  {
>  	struct scsi_cd *cd = dev_get_drvdata(dev);
>  
> +	scsi_reset_prep_fn(cd->device->request_queue);
>  	del_gendisk(cd->disk);
>  
>  	mutex_lock(&sr_ref_mutex);
> diff --git a/include/scsi/scsi_driver.h b/include/scsi/scsi_driver.h
> index 1f5ca7f..2e22929 100644
> --- a/include/scsi/scsi_driver.h
> +++ b/include/scsi/scsi_driver.h
> @@ -32,5 +32,6 @@ int scsi_setup_blk_pc_cmnd(struct scsi_device *sdev, struct request *req);
>  int scsi_setup_fs_cmnd(struct scsi_device *sdev, struct request *req);
>  int scsi_prep_state_check(struct scsi_device *sdev, struct request *req);
>  int scsi_prep_return(struct request_queue *q, struct request *req, int ret);
> +int scsi_reset_prep_fn(struct request_queue *);
>  
>  #endif /* _SCSI_SCSI_DRIVER_H */

I did not test this patch with osd_uld yet, but I will tomorrow. If my assumption
is right then drv-core should call my osd_remove just the same, right?

Are there any different timing issue for example the presence of scsi_device
is still true?

Thanks
Boaz

Re: [PATCH] Driver 'sd' needs updating

[Hannes Reinecke]

Hi Boaz,

Boaz Harrosh wrote:
> Hannes Reinecke wrote:
>> If a driver sets blk_queue_prep_rq() it should clean up itself
>> and not rely on the bus callbacks to handle this. This removes
>> the need to hook into bus->remove() as these should not be used
>> at the same time as driver->remove().
>>
>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>> Signed-off-by: Kay Sievers <kay.sievers@vrfy.org>
> 
> Hi Hannes, please I do not understand this patch.
> 
> I was always under the impression that the scsi ULD
> remove() function is called from inside scsi_bus_remove()
> at the very end, please see below.
> 
>> ---
>>  drivers/scsi/scsi_lib.c    |    6 ++++++
>>  drivers/scsi/scsi_sysfs.c  |   17 -----------------
>>  drivers/scsi/sd.c          |    2 ++
>>  drivers/scsi/sr.c          |    1 +
>>  include/scsi/scsi_driver.h |    1 +
>>  5 files changed, 10 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
>> index 4b13e36..73df41b 100644
>> --- a/drivers/scsi/scsi_lib.c
>> +++ b/drivers/scsi/scsi_lib.c
>> @@ -1222,6 +1222,12 @@ int scsi_prep_fn(struct request_queue *q, struct request *req)
>>  	return scsi_prep_return(q, req, ret);
>>  }
>>  
>> +void scsi_reset_prep_fn(struct request_queue *q)
>> +{
>> +	blk_queue_prep_rq(q, scsi_prep_fn);
>> +}
>> +EXPORT_SYMBOL(scsi_reset_prep_fn);
>> +
>>  /*
>>   * scsi_dev_queue_ready: if we can send requests to sdev, return 1 else
>>   * return 0.
>> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
>> index fa4711d..91482f2 100644
>> --- a/drivers/scsi/scsi_sysfs.c
>> +++ b/drivers/scsi/scsi_sysfs.c
>> @@ -420,29 +420,12 @@ static int scsi_bus_resume(struct device * dev)
>>  	return err;
>>  }
>>  
>> -static int scsi_bus_remove(struct device *dev)
>> -{
>> -	struct device_driver *drv = dev->driver;
>> -	struct scsi_device *sdev = to_scsi_device(dev);
>> -	int err = 0;
>> -
>> -	/* reset the prep_fn back to the default since the
>> -	 * driver may have altered it and it's being removed */
>> -	blk_queue_prep_rq(sdev->request_queue, scsi_prep_fn);
>> -
>> -	if (drv && drv->remove)
>> -		err = drv->remove(dev);
> 
> This here is where my osd_uld.c::osd_remove() is called.
> 
> If this vector is not used will the drv-core call drv->remove(dev)
> in behalf of scsi?
> 
From drivers/base/dd.c:__device_release_driver()

		if (dev->bus && dev->bus->remove)
			dev->bus->remove(dev);
		else if (drv->remove)
			drv->remove(dev);

So the answer will be yes.

>> -
>> -	return 0;
>> -}
>> -
>>  struct bus_type scsi_bus_type = {
>>          .name		= "scsi",
>>          .match		= scsi_bus_match,
>>  	.uevent		= scsi_bus_uevent,
>>  	.suspend	= scsi_bus_suspend,
>>  	.resume		= scsi_bus_resume,
>> -	.remove		= scsi_bus_remove,
>>  };
>>  EXPORT_SYMBOL_GPL(scsi_bus_type);
>>  
>> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
>> index aeab5d9..64e88e2 100644
>> --- a/drivers/scsi/sd.c
>> +++ b/drivers/scsi/sd.c
>> @@ -2068,6 +2068,8 @@ static int sd_remove(struct device *dev)
> 
> Until today this was called from within scsi_bus_remove
> what happens after your patch?
> 
Driver core calls it (see above).

>>  {
>>  	struct scsi_disk *sdkp = dev_get_drvdata(dev);
>>  
>> +	scsi_reset_prep_fn(sdkp->device->request_queue);
>> +
>>  	device_del(&sdkp->dev);
>>  	del_gendisk(sdkp->disk);
>>  	sd_shutdown(dev);
>> diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
>> index e7fa3ca..914733a 100644
>> --- a/drivers/scsi/sr.c
>> +++ b/drivers/scsi/sr.c
>> @@ -889,6 +889,7 @@ static int sr_remove(struct device *dev)
>>  {
>>  	struct scsi_cd *cd = dev_get_drvdata(dev);
>>  
>> +	scsi_reset_prep_fn(cd->device->request_queue);
>>  	del_gendisk(cd->disk);
>>  
>>  	mutex_lock(&sr_ref_mutex);
>> diff --git a/include/scsi/scsi_driver.h b/include/scsi/scsi_driver.h
>> index 1f5ca7f..2e22929 100644
>> --- a/include/scsi/scsi_driver.h
>> +++ b/include/scsi/scsi_driver.h
>> @@ -32,5 +32,6 @@ int scsi_setup_blk_pc_cmnd(struct scsi_device *sdev, struct request *req);
>>  int scsi_setup_fs_cmnd(struct scsi_device *sdev, struct request *req);
>>  int scsi_prep_state_check(struct scsi_device *sdev, struct request *req);
>>  int scsi_prep_return(struct request_queue *q, struct request *req, int ret);
>> +int scsi_reset_prep_fn(struct request_queue *);
>>  
>>  #endif /* _SCSI_SCSI_DRIVER_H */
> 
> I did not test this patch with osd_uld yet, but I will tomorrow. If my assumption
> is right then drv-core should call my osd_remove just the same, right?
> 
Yes, correct.

> Are there any different timing issue for example the presence of scsi_device
> is still true?
> 
No. Or rather, none you'll notice. (We're effectively saving on indirection here,
but I doubt it'll be measureable)

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Markus Rex, 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] Driver 'sd' needs updating

[Boaz Harrosh]

Hannes Reinecke wrote:
>>>  #endif /* _SCSI_SCSI_DRIVER_H */
>> I did not test this patch with osd_uld yet, but I will tomorrow. If my assumption
>> is right then drv-core should call my osd_remove just the same, right?
>>
> Yes, correct.
> 
>> Are there any different timing issue for example the presence of scsi_device
>> is still true?
>>
> No. Or rather, none you'll notice. (We're effectively saving on indirection here,
> but I doubt it'll be measureable)
> 
> Cheers,
> 
> Hannes

Thanks, I've patched my testing Kernel, and am scheduled to run tests later
tonight. I'm sure it will all work nicely minus the warning on load.

Thanks for doing this
Boaz

Re: [PATCH] Driver 'sd' needs updating

[Boaz Harrosh]

Hannes Reinecke wrote:
> If a driver sets blk_queue_prep_rq() it should clean up itself
> and not rely on the bus callbacks to handle this. This removes
> the need to hook into bus->remove() as these should not be used
> at the same time as driver->remove().
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> Signed-off-by: Kay Sievers <kay.sievers@vrfy.org>

I spoke too soon please see below

> ---
>  drivers/scsi/scsi_lib.c    |    6 ++++++
>  drivers/scsi/scsi_sysfs.c  |   17 -----------------
>  drivers/scsi/sd.c          |    2 ++
>  drivers/scsi/sr.c          |    1 +
>  include/scsi/scsi_driver.h |    1 +
>  5 files changed, 10 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 4b13e36..73df41b 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -1222,6 +1222,12 @@ int scsi_prep_fn(struct request_queue *q, struct request *req)
>  	return scsi_prep_return(q, req, ret);
>  }
>  
> +void scsi_reset_prep_fn(struct request_queue *q)

Here it is declared void
but

> +{
> +	blk_queue_prep_rq(q, scsi_prep_fn);
> +}
> +EXPORT_SYMBOL(scsi_reset_prep_fn);
> +
>  /*
>   * scsi_dev_queue_ready: if we can send requests to sdev, return 1 else
>   * return 0.
> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
> index fa4711d..91482f2 100644
> --- a/drivers/scsi/scsi_sysfs.c
> +++ b/drivers/scsi/scsi_sysfs.c
> @@ -420,29 +420,12 @@ static int scsi_bus_resume(struct device * dev)
>  	return err;
>  }
>  
> -static int scsi_bus_remove(struct device *dev)
> -{
> -	struct device_driver *drv = dev->driver;
> -	struct scsi_device *sdev = to_scsi_device(dev);
> -	int err = 0;
> -
> -	/* reset the prep_fn back to the default since the
> -	 * driver may have altered it and it's being removed */
> -	blk_queue_prep_rq(sdev->request_queue, scsi_prep_fn);
> -
> -	if (drv && drv->remove)
> -		err = drv->remove(dev);
> -
> -	return 0;
> -}
> -
>  struct bus_type scsi_bus_type = {
>          .name		= "scsi",
>          .match		= scsi_bus_match,
>  	.uevent		= scsi_bus_uevent,
>  	.suspend	= scsi_bus_suspend,
>  	.resume		= scsi_bus_resume,
> -	.remove		= scsi_bus_remove,
>  };
>  EXPORT_SYMBOL_GPL(scsi_bus_type);
>  
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index aeab5d9..64e88e2 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -2068,6 +2068,8 @@ static int sd_remove(struct device *dev)
>  {
>  	struct scsi_disk *sdkp = dev_get_drvdata(dev);
>  
> +	scsi_reset_prep_fn(sdkp->device->request_queue);
> +
>  	device_del(&sdkp->dev);
>  	del_gendisk(sdkp->disk);
>  	sd_shutdown(dev);
> diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
> index e7fa3ca..914733a 100644
> --- a/drivers/scsi/sr.c
> +++ b/drivers/scsi/sr.c
> @@ -889,6 +889,7 @@ static int sr_remove(struct device *dev)
>  {
>  	struct scsi_cd *cd = dev_get_drvdata(dev);
>  
> +	scsi_reset_prep_fn(cd->device->request_queue);
>  	del_gendisk(cd->disk);
>  
>  	mutex_lock(&sr_ref_mutex);
> diff --git a/include/scsi/scsi_driver.h b/include/scsi/scsi_driver.h
> index 1f5ca7f..2e22929 100644
> --- a/include/scsi/scsi_driver.h
> +++ b/include/scsi/scsi_driver.h
> @@ -32,5 +32,6 @@ int scsi_setup_blk_pc_cmnd(struct scsi_device *sdev, struct request *req);
>  int scsi_setup_fs_cmnd(struct scsi_device *sdev, struct request *req);
>  int scsi_prep_state_check(struct scsi_device *sdev, struct request *req);
>  int scsi_prep_return(struct request_queue *q, struct request *req, int ret);
> +int scsi_reset_prep_fn(struct request_queue *);

Here it is returning int.

My FC10 compiler does not like that:
  drivers/scsi/scsi_lib.c:1415: error: conflicting types for 'scsi_reset_prep_fn'
  include/scsi/scsi_driver.h:35: error: previous declaration of 'scsi_reset_prep_fn' was here

>  
>  #endif /* _SCSI_SCSI_DRIVER_H */

I fixed it to be void, will test later

Thanks
Boaz

Re: [PATCH] Driver 'sd' needs updating

[Hannes Reinecke]

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

Hi Boaz,

Boaz Harrosh wrote:
> Hannes Reinecke wrote:
>> If a driver sets blk_queue_prep_rq() it should clean up itself
>> and not rely on the bus callbacks to handle this. This removes
>> the need to hook into bus->remove() as these should not be used
>> at the same time as driver->remove().
>>
>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>> Signed-off-by: Kay Sievers <kay.sievers@vrfy.org>
> 
> I spoke too soon please see below
> 
>> ---
>>  drivers/scsi/scsi_lib.c    |    6 ++++++
>>  drivers/scsi/scsi_sysfs.c  |   17 -----------------
>>  drivers/scsi/sd.c          |    2 ++
>>  drivers/scsi/sr.c          |    1 +
>>  include/scsi/scsi_driver.h |    1 +
>>  5 files changed, 10 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
>> index 4b13e36..73df41b 100644
>> --- a/drivers/scsi/scsi_lib.c
>> +++ b/drivers/scsi/scsi_lib.c
>> @@ -1222,6 +1222,12 @@ int scsi_prep_fn(struct request_queue *q, struct request *req)
>>  	return scsi_prep_return(q, req, ret);
>>  }
>>  
>> +void scsi_reset_prep_fn(struct request_queue *q)
> 
> Here it is declared void
> but
> 
>> diff --git a/include/scsi/scsi_driver.h b/include/scsi/scsi_driver.h
>> index 1f5ca7f..2e22929 100644
>> --- a/include/scsi/scsi_driver.h
>> +++ b/include/scsi/scsi_driver.h
>> @@ -32,5 +32,6 @@ int scsi_setup_blk_pc_cmnd(struct scsi_device *sdev, struct request *req);
>>  int scsi_setup_fs_cmnd(struct scsi_device *sdev, struct request *req);
>>  int scsi_prep_state_check(struct scsi_device *sdev, struct request *req);
>>  int scsi_prep_return(struct request_queue *q, struct request *req, int ret);
>> +int scsi_reset_prep_fn(struct request_queue *);
> 
> Here it is returning int.
> 
> My FC10 compiler does not like that:
>   drivers/scsi/scsi_lib.c:1415: error: conflicting types for 'scsi_reset_prep_fn'
>   include/scsi/scsi_driver.h:35: error: previous declaration of 'scsi_reset_prep_fn' was here
> 
>>  
>>  #endif /* _SCSI_SCSI_DRIVER_H */
> 
> I fixed it to be void, will test later
> 
Argl. You are correct. New patch attached.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Markus Rex, HRB 16746 (AG Nürnberg)

[-- Attachment #2: 0001-Driver-sd-needs-updating.patch --]
[-- Type: text/x-patch, Size: 3497 bytes --]

>From 6c8f63205c81922581ebd9a73d5ff92f50910064 Mon Sep 17 00:00:00 2001
From: Hannes Reinecke <hare@suse.de>
Date: Tue, 24 Mar 2009 15:13:43 +0100
Subject: [PATCH] Driver 'sd' needs updating

If a driver sets blk_queue_prep_rq() it should clean up itself
and not rely on the bus callbacks to handle this. This removes
the need to hook into bus->remove() as these should not be used
at the same time as driver->remove().

Signed-off-by: Hannes Reinecke <hare@suse.de>
Signed-off-by: Kay Sievers <kay.sievers@vrfy.org>
---
 drivers/scsi/scsi_lib.c    |    6 ++++++
 drivers/scsi/scsi_sysfs.c  |   17 -----------------
 drivers/scsi/sd.c          |    2 ++
 drivers/scsi/sr.c          |    1 +
 include/scsi/scsi_driver.h |    1 +
 5 files changed, 10 insertions(+), 17 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 4b13e36..73df41b 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1222,6 +1222,12 @@ int scsi_prep_fn(struct request_queue *q, struct request *req)
 	return scsi_prep_return(q, req, ret);
 }
 
+void scsi_reset_prep_fn(struct request_queue *q)
+{
+	blk_queue_prep_rq(q, scsi_prep_fn);
+}
+EXPORT_SYMBOL(scsi_reset_prep_fn);
+
 /*
  * scsi_dev_queue_ready: if we can send requests to sdev, return 1 else
  * return 0.
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index fa4711d..91482f2 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -420,29 +420,12 @@ static int scsi_bus_resume(struct device * dev)
 	return err;
 }
 
-static int scsi_bus_remove(struct device *dev)
-{
-	struct device_driver *drv = dev->driver;
-	struct scsi_device *sdev = to_scsi_device(dev);
-	int err = 0;
-
-	/* reset the prep_fn back to the default since the
-	 * driver may have altered it and it's being removed */
-	blk_queue_prep_rq(sdev->request_queue, scsi_prep_fn);
-
-	if (drv && drv->remove)
-		err = drv->remove(dev);
-
-	return 0;
-}
-
 struct bus_type scsi_bus_type = {
         .name		= "scsi",
         .match		= scsi_bus_match,
 	.uevent		= scsi_bus_uevent,
 	.suspend	= scsi_bus_suspend,
 	.resume		= scsi_bus_resume,
-	.remove		= scsi_bus_remove,
 };
 EXPORT_SYMBOL_GPL(scsi_bus_type);
 
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index aeab5d9..64e88e2 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2068,6 +2068,8 @@ static int sd_remove(struct device *dev)
 {
 	struct scsi_disk *sdkp = dev_get_drvdata(dev);
 
+	scsi_reset_prep_fn(sdkp->device->request_queue);
+
 	device_del(&sdkp->dev);
 	del_gendisk(sdkp->disk);
 	sd_shutdown(dev);
diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index e7fa3ca..914733a 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -889,6 +889,7 @@ static int sr_remove(struct device *dev)
 {
 	struct scsi_cd *cd = dev_get_drvdata(dev);
 
+	scsi_reset_prep_fn(cd->device->request_queue);
 	del_gendisk(cd->disk);
 
 	mutex_lock(&sr_ref_mutex);
diff --git a/include/scsi/scsi_driver.h b/include/scsi/scsi_driver.h
index 1f5ca7f..1f06be0 100644
--- a/include/scsi/scsi_driver.h
+++ b/include/scsi/scsi_driver.h
@@ -32,5 +32,6 @@ int scsi_setup_blk_pc_cmnd(struct scsi_device *sdev, struct request *req);
 int scsi_setup_fs_cmnd(struct scsi_device *sdev, struct request *req);
 int scsi_prep_state_check(struct scsi_device *sdev, struct request *req);
 int scsi_prep_return(struct request_queue *q, struct request *req, int ret);
+void scsi_reset_prep_fn(struct request_queue *);
 
 #endif /* _SCSI_SCSI_DRIVER_H */
-- 
1.5.3.2