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

Xiangliang Yu
2011-09-22T03:19:37Z

> 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.