Patchwork [4/5] mvsas: remove mvs_slave_{alloc|configure}

login
register
mail settings
Submitter Dan Williams
Date Sept. 20, 2011, 10:10 p.m.
Message ID <20110920221051.1094.39578.stgit@localhost6.localdomain6>
Download mbox | patch
Permalink /patch/115637/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Dan Williams - Sept. 20, 2011, 10:10 p.m.
libsas now handles:
1/ limiting ata scanning to lun0
2/ maximizing the queue_depth of sas devices (up to 256, mvsas only
   supports 64)
3/ changes to /sys/block/<sdX>/device/queue_depth for ata devices

Cc: Xiangliang Yu <yuxiangl@marvell.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/scsi/mvsas/mv_init.c |    4 ++--
 drivers/scsi/mvsas/mv_sas.c  |   30 ------------------------------
 drivers/scsi/mvsas/mv_sas.h  |    2 --
 3 files changed, 2 insertions(+), 34 deletions(-)


--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Xiangliang Yu - Sept. 22, 2011, 2:26 a.m.
> Subject: [PATCH 4/5] mvsas: remove mvs_slave_{alloc|configure}
> 
> libsas now handles:
> 1/ limiting ata scanning to lun0
> 2/ maximizing the queue_depth of sas devices (up to 256, mvsas only
>    supports 64)
> 3/ changes to /sys/block/<sdX>/device/queue_depth for ata devices
> 
> Cc: Xiangliang Yu <yuxiangl@marvell.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  drivers/scsi/mvsas/mv_init.c |    4 ++--
>  drivers/scsi/mvsas/mv_sas.c  |   30 ------------------------------
>  drivers/scsi/mvsas/mv_sas.h  |    2 --
>  3 files changed, 2 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/scsi/mvsas/mv_init.c b/drivers/scsi/mvsas/mv_init.c
> index 4e9af66..052861b 100644
> --- a/drivers/scsi/mvsas/mv_init.c
> +++ b/drivers/scsi/mvsas/mv_init.c
> @@ -59,7 +59,7 @@ static struct scsi_host_template mvs_sht = {
>  	.name			= DRV_NAME,
>  	.queuecommand		= sas_queuecommand,
>  	.target_alloc		= sas_target_alloc,
> -	.slave_configure	= mvs_slave_configure,
> +	.slave_configure	= sas_slave_configure,
>  	.slave_destroy		= sas_slave_destroy,
>  	.scan_finished		= mvs_scan_finished,
>  	.scan_start		= mvs_scan_start,
> @@ -74,7 +74,7 @@ static struct scsi_host_template mvs_sht = {
>  	.use_clustering		= ENABLE_CLUSTERING,
>  	.eh_device_reset_handler = sas_eh_device_reset_handler,
>  	.eh_bus_reset_handler	= sas_eh_bus_reset_handler,
> -	.slave_alloc		= mvs_slave_alloc,
> +	.slave_alloc		= sas_slave_alloc,
>  	.target_destroy		= sas_target_destroy,
> -int mvs_slave_configure(struct scsi_device *sdev)
> -{
> -	struct domain_device *dev = sdev_to_domain_dev(sdev);
> -	int ret = sas_slave_configure(sdev);
> -
> -	if (ret)
> -		return ret;
> -	if (!dev_is_sata(dev)) {
> -		sas_change_queue_depth(sdev,
> -			MVS_QUEUE_SIZE,
> -			SCSI_QDEPTH_DEFAULT);
> -	}
> -	return 0;
Can you don't remove mvs_slave_configure function? Someday it will be useful, such as fixing some issue.
Thanks!

--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jeff Garzik - Sept. 22, 2011, 2:31 a.m.
On 09/21/2011 10:26 PM, Xiangliang Yu wrote:
>
>> Subject: [PATCH 4/5] mvsas: remove mvs_slave_{alloc|configure}
>>
>> libsas now handles:
>> 1/ limiting ata scanning to lun0
>> 2/ maximizing the queue_depth of sas devices (up to 256, mvsas only
>>     supports 64)
>> 3/ changes to /sys/block/<sdX>/device/queue_depth for ata devices
>>
>> Cc: Xiangliang Yu<yuxiangl@marvell.com>
>> Signed-off-by: Dan Williams<dan.j.williams@intel.com>
>> ---
>>   drivers/scsi/mvsas/mv_init.c |    4 ++--
>>   drivers/scsi/mvsas/mv_sas.c  |   30 ------------------------------
>>   drivers/scsi/mvsas/mv_sas.h  |    2 --
>>   3 files changed, 2 insertions(+), 34 deletions(-)
>>
>> diff --git a/drivers/scsi/mvsas/mv_init.c b/drivers/scsi/mvsas/mv_init.c
>> index 4e9af66..052861b 100644
>> --- a/drivers/scsi/mvsas/mv_init.c
>> +++ b/drivers/scsi/mvsas/mv_init.c
>> @@ -59,7 +59,7 @@ static struct scsi_host_template mvs_sht = {
>>   	.name			= DRV_NAME,
>>   	.queuecommand		= sas_queuecommand,
>>   	.target_alloc		= sas_target_alloc,
>> -	.slave_configure	= mvs_slave_configure,
>> +	.slave_configure	= sas_slave_configure,
>>   	.slave_destroy		= sas_slave_destroy,
>>   	.scan_finished		= mvs_scan_finished,
>>   	.scan_start		= mvs_scan_start,
>> @@ -74,7 +74,7 @@ static struct scsi_host_template mvs_sht = {
>>   	.use_clustering		= ENABLE_CLUSTERING,
>>   	.eh_device_reset_handler = sas_eh_device_reset_handler,
>>   	.eh_bus_reset_handler	= sas_eh_bus_reset_handler,
>> -	.slave_alloc		= mvs_slave_alloc,
>> +	.slave_alloc		= sas_slave_alloc,
>>   	.target_destroy		= sas_target_destroy,
>> -int mvs_slave_configure(struct scsi_device *sdev)
>> -{
>> -	struct domain_device *dev = sdev_to_domain_dev(sdev);
>> -	int ret = sas_slave_configure(sdev);
>> -
>> -	if (ret)
>> -		return ret;
>> -	if (!dev_is_sata(dev)) {
>> -		sas_change_queue_depth(sdev,
>> -			MVS_QUEUE_SIZE,
>> -			SCSI_QDEPTH_DEFAULT);
>> -	}
>> -	return 0;
> Can you don't remove mvs_slave_configure function? Someday it will be useful, such as fixing some issue.
> Thanks!

There is no reason to keep a no-op function there.

	Jeff





--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dan Williams - Sept. 22, 2011, 3:17 a.m.
On Wed, Sep 21, 2011 at 7:26 PM, Xiangliang Yu <yuxiangl@marvell.com> wrote:
>
>> Subject: [PATCH 4/5] mvsas: remove mvs_slave_{alloc|configure}
>>
>> libsas now handles:
>> 1/ limiting ata scanning to lun0
>> 2/ maximizing the queue_depth of sas devices (up to 256, mvsas only
>>    supports 64)
>> 3/ changes to /sys/block/<sdX>/device/queue_depth for ata devices
>>
>> Cc: Xiangliang Yu <yuxiangl@marvell.com>
>> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>> ---
>>  drivers/scsi/mvsas/mv_init.c |    4 ++--
>>  drivers/scsi/mvsas/mv_sas.c  |   30 ------------------------------
>>  drivers/scsi/mvsas/mv_sas.h  |    2 --
>>  3 files changed, 2 insertions(+), 34 deletions(-)
>>
>> diff --git a/drivers/scsi/mvsas/mv_init.c b/drivers/scsi/mvsas/mv_init.c
>> index 4e9af66..052861b 100644
>> --- a/drivers/scsi/mvsas/mv_init.c
>> +++ b/drivers/scsi/mvsas/mv_init.c
>> @@ -59,7 +59,7 @@ static struct scsi_host_template mvs_sht = {
>>       .name                   = DRV_NAME,
>>       .queuecommand           = sas_queuecommand,
>>       .target_alloc           = sas_target_alloc,
>> -     .slave_configure        = mvs_slave_configure,
>> +     .slave_configure        = sas_slave_configure,
>>       .slave_destroy          = sas_slave_destroy,
>>       .scan_finished          = mvs_scan_finished,
>>       .scan_start             = mvs_scan_start,
>> @@ -74,7 +74,7 @@ static struct scsi_host_template mvs_sht = {
>>       .use_clustering         = ENABLE_CLUSTERING,
>>       .eh_device_reset_handler = sas_eh_device_reset_handler,
>>       .eh_bus_reset_handler   = sas_eh_bus_reset_handler,
>> -     .slave_alloc            = mvs_slave_alloc,
>> +     .slave_alloc            = sas_slave_alloc,
>>       .target_destroy         = sas_target_destroy,
>> -int mvs_slave_configure(struct scsi_device *sdev)
>> -{
>> -     struct domain_device *dev = sdev_to_domain_dev(sdev);
>> -     int ret = sas_slave_configure(sdev);
>> -
>> -     if (ret)
>> -             return ret;
>> -     if (!dev_is_sata(dev)) {
>> -             sas_change_queue_depth(sdev,
>> -                     MVS_QUEUE_SIZE,
>> -                     SCSI_QDEPTH_DEFAULT);
>> -     }
>> -     return 0;
> Can you don't remove mvs_slave_configure function? Someday it will be useful, such as fixing some issue.

That's exactly the point of the patch.  The isci driver encountered a
bug that the mvsas driver had fixed in its private mvs_slave_alloc()
routine.  If I had taken the same approach and "fixed" it with a
isci_slave_alloc() routine that would still leave aic94xx or any other
future libsas driver to rediscover the fix.

If you later find a problem with sas_slave_configure() there's a good
chance the fix will be applicable to all libsas drivers.  But if it is
truly a mvsas specific issue you can always re-introduce
mvs_slave_configure().

--
Dan
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Xiangliang Yu - Sept. 22, 2011, 3:19 a.m.
> Subject: Re: [PATCH 4/5] mvsas: remove mvs_slave_{alloc|configure}
> 
> On Wed, Sep 21, 2011 at 7:26 PM, Xiangliang Yu <yuxiangl@marvell.com> wrote:
> >
> >> Subject: [PATCH 4/5] mvsas: remove mvs_slave_{alloc|configure}
> >>
> >> libsas now handles:
> >> 1/ limiting ata scanning to lun0
> >> 2/ maximizing the queue_depth of sas devices (up to 256, mvsas only
> >>    supports 64)
> >> 3/ changes to /sys/block/<sdX>/device/queue_depth for ata devices
> >>
> >> Cc: Xiangliang Yu <yuxiangl@marvell.com>
> >> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> >> ---
> >>  drivers/scsi/mvsas/mv_init.c |    4 ++--
> >>  drivers/scsi/mvsas/mv_sas.c  |   30 ------------------------------
> >>  drivers/scsi/mvsas/mv_sas.h  |    2 --
> >>  3 files changed, 2 insertions(+), 34 deletions(-)
> >>
> >> diff --git a/drivers/scsi/mvsas/mv_init.c b/drivers/scsi/mvsas/mv_init.c
> >> index 4e9af66..052861b 100644
> >> --- a/drivers/scsi/mvsas/mv_init.c
> >> +++ b/drivers/scsi/mvsas/mv_init.c
> >> @@ -59,7 +59,7 @@ static struct scsi_host_template mvs_sht = {
> >>       .name                   = DRV_NAME,
> >>       .queuecommand           = sas_queuecommand,
> >>       .target_alloc           = sas_target_alloc,
> >> -     .slave_configure        = mvs_slave_configure,
> >> +     .slave_configure        = sas_slave_configure,
> >>       .slave_destroy          = sas_slave_destroy,
> >>       .scan_finished          = mvs_scan_finished,
> >>       .scan_start             = mvs_scan_start,
> >> @@ -74,7 +74,7 @@ static struct scsi_host_template mvs_sht = {
> >>       .use_clustering         = ENABLE_CLUSTERING,
> >>       .eh_device_reset_handler = sas_eh_device_reset_handler,
> >>       .eh_bus_reset_handler   = sas_eh_bus_reset_handler,
> >> -     .slave_alloc            = mvs_slave_alloc,
> >> +     .slave_alloc            = sas_slave_alloc,
> >>       .target_destroy         = sas_target_destroy,
> >> -int mvs_slave_configure(struct scsi_device *sdev)
> >> -{
> >> -     struct domain_device *dev = sdev_to_domain_dev(sdev);
> >> -     int ret = sas_slave_configure(sdev);
> >> -
> >> -     if (ret)
> >> -             return ret;
> >> -     if (!dev_is_sata(dev)) {
> >> -             sas_change_queue_depth(sdev,
> >> -                     MVS_QUEUE_SIZE,
> >> -                     SCSI_QDEPTH_DEFAULT);
> >> -     }
> >> -     return 0;
> > Can you don't remove mvs_slave_configure function? Someday it will be useful,
> such as fixing some issue.
> 
> That's exactly the point of the patch.  The isci driver encountered a
> bug that the mvsas driver had fixed in its private mvs_slave_alloc()
> routine.  If I had taken the same approach and "fixed" it with a
> isci_slave_alloc() routine that would still leave aic94xx or any other
> future libsas driver to rediscover the fix.
> 
> If you later find a problem with sas_slave_configure() there's a good
> chance the fix will be applicable to all libsas drivers.  But if it is
> truly a mvsas specific issue you can always re-introduce
> mvs_slave_configure().


Sound good.

--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/scsi/mvsas/mv_init.c b/drivers/scsi/mvsas/mv_init.c
index 4e9af66..052861b 100644
--- a/drivers/scsi/mvsas/mv_init.c
+++ b/drivers/scsi/mvsas/mv_init.c
@@ -59,7 +59,7 @@  static struct scsi_host_template mvs_sht = {
 	.name			= DRV_NAME,
 	.queuecommand		= sas_queuecommand,
 	.target_alloc		= sas_target_alloc,
-	.slave_configure	= mvs_slave_configure,
+	.slave_configure	= sas_slave_configure,
 	.slave_destroy		= sas_slave_destroy,
 	.scan_finished		= mvs_scan_finished,
 	.scan_start		= mvs_scan_start,
@@ -74,7 +74,7 @@  static struct scsi_host_template mvs_sht = {
 	.use_clustering		= ENABLE_CLUSTERING,
 	.eh_device_reset_handler = sas_eh_device_reset_handler,
 	.eh_bus_reset_handler	= sas_eh_bus_reset_handler,
-	.slave_alloc		= mvs_slave_alloc,
+	.slave_alloc		= sas_slave_alloc,
 	.target_destroy		= sas_target_destroy,
 	.ioctl			= sas_ioctl,
 	.shost_attrs		= mvst_host_attrs,
diff --git a/drivers/scsi/mvsas/mv_sas.c b/drivers/scsi/mvsas/mv_sas.c
index 4196eee..f2ac01f 100644
--- a/drivers/scsi/mvsas/mv_sas.c
+++ b/drivers/scsi/mvsas/mv_sas.c
@@ -276,36 +276,6 @@  static void mvs_bytes_dmaed(struct mvs_info *mvi, int i)
 				   PORTE_BYTES_DMAED);
 }
 
-int mvs_slave_alloc(struct scsi_device *scsi_dev)
-{
-	struct domain_device *dev = sdev_to_domain_dev(scsi_dev);
-	if (dev_is_sata(dev)) {
-		/* We don't need to rescan targets
-		 * if REPORT_LUNS request is failed
-		 */
-		if (scsi_dev->lun > 0)
-			return -ENXIO;
-		scsi_dev->tagged_supported = 1;
-	}
-
-	return sas_slave_alloc(scsi_dev);
-}
-
-int mvs_slave_configure(struct scsi_device *sdev)
-{
-	struct domain_device *dev = sdev_to_domain_dev(sdev);
-	int ret = sas_slave_configure(sdev);
-
-	if (ret)
-		return ret;
-	if (!dev_is_sata(dev)) {
-		sas_change_queue_depth(sdev,
-			MVS_QUEUE_SIZE,
-			SCSI_QDEPTH_DEFAULT);
-	}
-	return 0;
-}
-
 void mvs_scan_start(struct Scsi_Host *shost)
 {
 	int i, j;
diff --git a/drivers/scsi/mvsas/mv_sas.h b/drivers/scsi/mvsas/mv_sas.h
index 44d7885..21ee8b0 100644
--- a/drivers/scsi/mvsas/mv_sas.h
+++ b/drivers/scsi/mvsas/mv_sas.h
@@ -459,8 +459,6 @@  int mvs_phy_control(struct asd_sas_phy *sas_phy, enum phy_func func,
 			void *funcdata);
 void __devinit mvs_set_sas_addr(struct mvs_info *mvi, int port_id,
 				u32 off_lo, u32 off_hi, u64 sas_addr);
-int mvs_slave_alloc(struct scsi_device *scsi_dev);
-int mvs_slave_configure(struct scsi_device *sdev);
 void mvs_scan_start(struct Scsi_Host *shost);
 int mvs_scan_finished(struct Scsi_Host *shost, unsigned long time);
 int mvs_queue_command(struct sas_task *task, const int num,