| Message ID | 20220925230817.91542-3-damien.lemoal@opensource.wdc.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fixes for ATA device queue depth control | expand |
On 26/09/2022 00:08, Damien Le Moal wrote: > The function __ata_change_queue_depth() uses the helper > ata_scsi_find_dev() to get the ata_device structure of a scsi device and > set that device maximum queue depth. However, when the ata device is > managed by libsas, Yeah, this current code is utterly broken. Just a nit is that it would be good to mention that the sdev id is assigned by scsi_transport_sas (and not libata) for when using libsas. In my series "Allocate SCSI device earlier for ata port probe" I mentioned this problem of different id assignment scheme. It would be nice to try to commonize this. > ata_scsi_find_dev() returns NULL, turning > __ata_change_queue_depth() into a nop, which prevents the user from > setting the maximum queue depth of ATA devices used with libsas based > HBAs. > > Fix this by renaming __ata_change_queue_depth() to > ata_change_queue_depth() and adding a pointer to the ata_device > structure of the target device as argument. This pointer is provided by > ata_scsi_change_queue_depth() using ata_scsi_find_dev() in the case of > a libata managed device and by sas_change_queue_depth() using > sas_to_ata_dev() in the case of a libsas managed ata device. This seems ok. But could you alternatively use ata_for_each_dev() and match by ata_device.sdev pointer? That pointer is set quite late in the probe, though, so maybe it would not work. As an aside, one other thing I noticed is that ata_device.private_data is unused. Maybe we should delete it - apparently it was added just for symmetry of libata structures. Thanks, John > > Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com> > --- > drivers/ata/libata-sata.c | 24 ++++++++++++------------ > drivers/scsi/libsas/sas_scsi_host.c | 3 ++- > include/linux/libata.h | 4 ++-- > 3 files changed, 16 insertions(+), 15 deletions(-) > > diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c > index 7a5fe41aa5ae..13b9d0fdd42c 100644 > --- a/drivers/ata/libata-sata.c > +++ b/drivers/ata/libata-sata.c > @@ -1018,26 +1018,25 @@ DEVICE_ATTR(sw_activity, S_IWUSR | S_IRUGO, ata_scsi_activity_show, > EXPORT_SYMBOL_GPL(dev_attr_sw_activity); > > /** > - * __ata_change_queue_depth - helper for ata_scsi_change_queue_depth > - * @ap: ATA port to which the device change the queue depth > + * ata_change_queue_depth - Set a device maximum queue depth > + * @ap: ATA port of the target device > + * @dev: target ATA device > * @sdev: SCSI device to configure queue depth for > * @queue_depth: new queue depth > * > - * libsas and libata have different approaches for associating a sdev to > - * its ata_port. > + * Helper to set a device maximum queue depth, usable with both libsas > + * and libata. > * > */ > -int __ata_change_queue_depth(struct ata_port *ap, struct scsi_device *sdev, > - int queue_depth) > +int ata_change_queue_depth(struct ata_port *ap, struct ata_device *dev, > + struct scsi_device *sdev, int queue_depth) > { > - struct ata_device *dev; > unsigned long flags; > > - if (queue_depth < 1 || queue_depth == sdev->queue_depth) > + if (!dev || !ata_dev_enabled(dev)) > return sdev->queue_depth; > > - dev = ata_scsi_find_dev(ap, sdev); > - if (!dev || !ata_dev_enabled(dev)) > + if (queue_depth < 1 || queue_depth == sdev->queue_depth) > return sdev->queue_depth; > > /* NCQ enabled? */ > @@ -1059,7 +1058,7 @@ int __ata_change_queue_depth(struct ata_port *ap, struct scsi_device *sdev, > > return scsi_change_queue_depth(sdev, queue_depth); > } > -EXPORT_SYMBOL_GPL(__ata_change_queue_depth); > +EXPORT_SYMBOL_GPL(ata_change_queue_depth); > > /** > * ata_scsi_change_queue_depth - SCSI callback for queue depth config > @@ -1080,7 +1079,8 @@ int ata_scsi_change_queue_depth(struct scsi_device *sdev, int queue_depth) > { > struct ata_port *ap = ata_shost_to_port(sdev->host); > > - return __ata_change_queue_depth(ap, sdev, queue_depth); > + return ata_change_queue_depth(ap, ata_scsi_find_dev(ap, sdev), > + sdev, queue_depth); > } > EXPORT_SYMBOL_GPL(ata_scsi_change_queue_depth); > > diff --git a/drivers/scsi/libsas/sas_scsi_host.c b/drivers/scsi/libsas/sas_scsi_host.c > index 9c82e5dc4fcc..a36fa1c128a8 100644 > --- a/drivers/scsi/libsas/sas_scsi_host.c > +++ b/drivers/scsi/libsas/sas_scsi_host.c > @@ -872,7 +872,8 @@ int sas_change_queue_depth(struct scsi_device *sdev, int depth) > struct domain_device *dev = sdev_to_domain_dev(sdev); > > if (dev_is_sata(dev)) > - return __ata_change_queue_depth(dev->sata_dev.ap, sdev, depth); > + return ata_change_queue_depth(dev->sata_dev.ap, > + sas_to_ata_dev(dev), sdev, depth); > > if (!sdev->tagged_supported) > depth = 1; > diff --git a/include/linux/libata.h b/include/linux/libata.h > index 698032e5ef2d..20765d1c5f80 100644 > --- a/include/linux/libata.h > +++ b/include/linux/libata.h > @@ -1136,8 +1136,8 @@ extern int ata_scsi_slave_config(struct scsi_device *sdev); > extern void ata_scsi_slave_destroy(struct scsi_device *sdev); > extern int ata_scsi_change_queue_depth(struct scsi_device *sdev, > int queue_depth); > -extern int __ata_change_queue_depth(struct ata_port *ap, struct scsi_device *sdev, > - int queue_depth); > +extern int ata_change_queue_depth(struct ata_port *ap, struct ata_device *dev, > + struct scsi_device *sdev, int queue_depth); > extern struct ata_device *ata_dev_pair(struct ata_device *adev); > extern int ata_do_set_mode(struct ata_link *link, struct ata_device **r_failed_dev); > extern void ata_scsi_port_error_handler(struct Scsi_Host *host, struct ata_port *ap);
On 9/26/22 20:31, John Garry wrote: > On 26/09/2022 00:08, Damien Le Moal wrote: >> The function __ata_change_queue_depth() uses the helper >> ata_scsi_find_dev() to get the ata_device structure of a scsi device and >> set that device maximum queue depth. However, when the ata device is >> managed by libsas, > > Yeah, this current code is utterly broken. Just a nit is that it would > be good to mention that the sdev id is assigned by scsi_transport_sas > (and not libata) for when using libsas. > > In my series "Allocate SCSI device earlier for ata port probe" I > mentioned this problem of different id assignment scheme. It would be > nice to try to commonize this. > >> ata_scsi_find_dev() returns NULL, turning >> __ata_change_queue_depth() into a nop, which prevents the user from >> setting the maximum queue depth of ATA devices used with libsas based >> HBAs. >> >> Fix this by renaming __ata_change_queue_depth() to >> ata_change_queue_depth() and adding a pointer to the ata_device >> structure of the target device as argument. This pointer is provided by >> ata_scsi_change_queue_depth() using ata_scsi_find_dev() in the case of >> a libata managed device and by sas_change_queue_depth() using >> sas_to_ata_dev() in the case of a libsas managed ata device. > > This seems ok. But could you alternatively use ata_for_each_dev() and > match by ata_device.sdev pointer? That pointer is set quite late in the > probe, though, so maybe it would not work. Not sure I understand why we should search for the ata device again using ata_for_each_dev() when sas_to_ata_dev() gives us directly what we need for the libsas controlled device... Can you clarify ? > > As an aside, one other thing I noticed is that ata_device.private_data > is unused. Maybe we should delete it - apparently it was added just for > symmetry of libata structures. > > Thanks, > John > >> >> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com> >> --- >> drivers/ata/libata-sata.c | 24 ++++++++++++------------ >> drivers/scsi/libsas/sas_scsi_host.c | 3 ++- >> include/linux/libata.h | 4 ++-- >> 3 files changed, 16 insertions(+), 15 deletions(-) >> >> diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c >> index 7a5fe41aa5ae..13b9d0fdd42c 100644 >> --- a/drivers/ata/libata-sata.c >> +++ b/drivers/ata/libata-sata.c >> @@ -1018,26 +1018,25 @@ DEVICE_ATTR(sw_activity, S_IWUSR | S_IRUGO, ata_scsi_activity_show, >> EXPORT_SYMBOL_GPL(dev_attr_sw_activity); >> >> /** >> - * __ata_change_queue_depth - helper for ata_scsi_change_queue_depth >> - * @ap: ATA port to which the device change the queue depth >> + * ata_change_queue_depth - Set a device maximum queue depth >> + * @ap: ATA port of the target device >> + * @dev: target ATA device >> * @sdev: SCSI device to configure queue depth for >> * @queue_depth: new queue depth >> * >> - * libsas and libata have different approaches for associating a sdev to >> - * its ata_port. >> + * Helper to set a device maximum queue depth, usable with both libsas >> + * and libata. >> * >> */ >> -int __ata_change_queue_depth(struct ata_port *ap, struct scsi_device *sdev, >> - int queue_depth) >> +int ata_change_queue_depth(struct ata_port *ap, struct ata_device *dev, >> + struct scsi_device *sdev, int queue_depth) >> { >> - struct ata_device *dev; >> unsigned long flags; >> >> - if (queue_depth < 1 || queue_depth == sdev->queue_depth) >> + if (!dev || !ata_dev_enabled(dev)) >> return sdev->queue_depth; >> >> - dev = ata_scsi_find_dev(ap, sdev); >> - if (!dev || !ata_dev_enabled(dev)) >> + if (queue_depth < 1 || queue_depth == sdev->queue_depth) >> return sdev->queue_depth; >> >> /* NCQ enabled? */ >> @@ -1059,7 +1058,7 @@ int __ata_change_queue_depth(struct ata_port *ap, struct scsi_device *sdev, >> >> return scsi_change_queue_depth(sdev, queue_depth); >> } >> -EXPORT_SYMBOL_GPL(__ata_change_queue_depth); >> +EXPORT_SYMBOL_GPL(ata_change_queue_depth); >> >> /** >> * ata_scsi_change_queue_depth - SCSI callback for queue depth config >> @@ -1080,7 +1079,8 @@ int ata_scsi_change_queue_depth(struct scsi_device *sdev, int queue_depth) >> { >> struct ata_port *ap = ata_shost_to_port(sdev->host); >> >> - return __ata_change_queue_depth(ap, sdev, queue_depth); >> + return ata_change_queue_depth(ap, ata_scsi_find_dev(ap, sdev), >> + sdev, queue_depth); >> } >> EXPORT_SYMBOL_GPL(ata_scsi_change_queue_depth); >> >> diff --git a/drivers/scsi/libsas/sas_scsi_host.c b/drivers/scsi/libsas/sas_scsi_host.c >> index 9c82e5dc4fcc..a36fa1c128a8 100644 >> --- a/drivers/scsi/libsas/sas_scsi_host.c >> +++ b/drivers/scsi/libsas/sas_scsi_host.c >> @@ -872,7 +872,8 @@ int sas_change_queue_depth(struct scsi_device *sdev, int depth) >> struct domain_device *dev = sdev_to_domain_dev(sdev); >> >> if (dev_is_sata(dev)) >> - return __ata_change_queue_depth(dev->sata_dev.ap, sdev, depth); >> + return ata_change_queue_depth(dev->sata_dev.ap, >> + sas_to_ata_dev(dev), sdev, depth); >> >> if (!sdev->tagged_supported) >> depth = 1; >> diff --git a/include/linux/libata.h b/include/linux/libata.h >> index 698032e5ef2d..20765d1c5f80 100644 >> --- a/include/linux/libata.h >> +++ b/include/linux/libata.h >> @@ -1136,8 +1136,8 @@ extern int ata_scsi_slave_config(struct scsi_device *sdev); >> extern void ata_scsi_slave_destroy(struct scsi_device *sdev); >> extern int ata_scsi_change_queue_depth(struct scsi_device *sdev, >> int queue_depth); >> -extern int __ata_change_queue_depth(struct ata_port *ap, struct scsi_device *sdev, >> - int queue_depth); >> +extern int ata_change_queue_depth(struct ata_port *ap, struct ata_device *dev, >> + struct scsi_device *sdev, int queue_depth); >> extern struct ata_device *ata_dev_pair(struct ata_device *adev); >> extern int ata_do_set_mode(struct ata_link *link, struct ata_device **r_failed_dev); >> extern void ata_scsi_port_error_handler(struct Scsi_Host *host, struct ata_port *ap); >
On 27/09/2022 00:05, Damien Le Moal wrote: >>> ata_scsi_find_dev() returns NULL, turning >>> __ata_change_queue_depth() into a nop, which prevents the user from >>> setting the maximum queue depth of ATA devices used with libsas based >>> HBAs. >>> >>> Fix this by renaming __ata_change_queue_depth() to >>> ata_change_queue_depth() and adding a pointer to the ata_device >>> structure of the target device as argument. This pointer is provided by >>> ata_scsi_change_queue_depth() using ata_scsi_find_dev() in the case of >>> a libata managed device and by sas_change_queue_depth() using >>> sas_to_ata_dev() in the case of a libsas managed ata device. >> This seems ok. But could you alternatively use ata_for_each_dev() and >> match by ata_device.sdev pointer? That pointer is set quite late in the >> probe, though, so maybe it would not work. > Not sure I understand why we should search for the ata device again using > ata_for_each_dev() when sas_to_ata_dev() gives us directly what we need > for the libsas controlled device... Can you clarify ? > Sure, we can use sas_to_ata_dev() to get the ata_device. I am just suggesting my way such that we can have a consistent method to get the ata_device between all libata users and we don't need to change the ata_change_queue_depth() interface. It would be something like: struct ata_device *ata_scsi_find_dev(struct ata_port *ap, const struct scsi_device *scsidev) { struct ata_link *link; struct ata_device *dev; ata_for_each_link(link, ap, EDGE) { ata_for_each_dev(dev, link, ENABLED) { if (scsidev == dev->sdev) return dev; } } // todo: check pmp return NULL; } Thanks, John
On 9/27/22 16:05, John Garry wrote: > On 27/09/2022 00:05, Damien Le Moal wrote: >>>> ata_scsi_find_dev() returns NULL, turning >>>> __ata_change_queue_depth() into a nop, which prevents the user from >>>> setting the maximum queue depth of ATA devices used with libsas based >>>> HBAs. >>>> >>>> Fix this by renaming __ata_change_queue_depth() to >>>> ata_change_queue_depth() and adding a pointer to the ata_device >>>> structure of the target device as argument. This pointer is provided by >>>> ata_scsi_change_queue_depth() using ata_scsi_find_dev() in the case of >>>> a libata managed device and by sas_change_queue_depth() using >>>> sas_to_ata_dev() in the case of a libsas managed ata device. >>> This seems ok. But could you alternatively use ata_for_each_dev() and >>> match by ata_device.sdev pointer? That pointer is set quite late in the >>> probe, though, so maybe it would not work. >> Not sure I understand why we should search for the ata device again using >> ata_for_each_dev() when sas_to_ata_dev() gives us directly what we need >> for the libsas controlled device... Can you clarify ? >> > > Sure, we can use sas_to_ata_dev() to get the ata_device. > > I am just suggesting my way such that we can have a consistent method to > get the ata_device between all libata users and we don't need to change > the ata_change_queue_depth() interface. It would be something like: > > struct ata_device *ata_scsi_find_dev(struct ata_port *ap, const struct > scsi_device *scsidev) > { > struct ata_link *link; > struct ata_device *dev; > > ata_for_each_link(link, ap, EDGE) { > ata_for_each_dev(dev, link, ENABLED) { > if (scsidev == dev->sdev) > return dev; > } > } > // todo: check pmp > return NULL; > } I see. Need to think about this one... This may also unify the pmp case. Are you OK with the patch as is though ? We can improve with something like the above on top later. Really need to fix that qd setting as it is causing problems for testing devices with/without ncq commands. > > Thanks, > John
On 27/09/2022 10:28, Damien Le Moal wrote: >> Sure, we can use sas_to_ata_dev() to get the ata_device. >> >> I am just suggesting my way such that we can have a consistent method to >> get the ata_device between all libata users and we don't need to change >> the ata_change_queue_depth() interface. It would be something like: >> >> struct ata_device *ata_scsi_find_dev(struct ata_port *ap, const struct >> scsi_device *scsidev) >> { >> struct ata_link *link; >> struct ata_device *dev; >> >> ata_for_each_link(link, ap, EDGE) { >> ata_for_each_dev(dev, link, ENABLED) { >> if (scsidev == dev->sdev) >> return dev; >> } >> } >> // todo: check pmp >> return NULL; >> } > I see. Need to think about this one... This may also unify the pmp case. > Are you OK with the patch as is though ? I'm ok with your patchset, but let me test it and get back to you later today. We can improve with something > like the above on top later. Really need to fix that qd setting as it is > causing problems for testing devices with/without ncq commands. Out of curiosity, are you considering your patchset for 6.0? > Thanks, John
On 26/09/2022 00:08, Damien Le Moal wrote: > The function __ata_change_queue_depth() uses the helper > ata_scsi_find_dev() to get the ata_device structure of a scsi device and > set that device maximum queue depth. However, when the ata device is > managed by libsas, ata_scsi_find_dev() returns NULL, turning > __ata_change_queue_depth() into a nop, which prevents the user from > setting the maximum queue depth of ATA devices used with libsas based > HBAs. > > Fix this by renaming __ata_change_queue_depth() to > ata_change_queue_depth() and adding a pointer to the ata_device > structure of the target device as argument. This pointer is provided by > ata_scsi_change_queue_depth() using ata_scsi_find_dev() in the case of > a libata managed device and by sas_change_queue_depth() using > sas_to_ata_dev() in the case of a libsas managed ata device. > > Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com> Tested-by: John Garry <john.garry@huawei.com> However - a big however - I will note that this following behaviour is strange for a SATA device for libsas: root@(none)$ echo 33 > 0:0:2:0/device/queue_depth root@(none)$ echo 33 > 0:0:2:0/device/queue_depth sh: echo: write error: Invalid argument root@(none)$ I also note that setting a value out of range is just rejected for a SAS device, and not capped to the max range (like it is for SATA). AHCI rejects out of range values it as it exceeds the shost can_queue in sdev_store_queue_depth(). Thanks, John > --- > drivers/ata/libata-sata.c | 24 ++++++++++++------------ > drivers/scsi/libsas/sas_scsi_host.c | 3 ++- > include/linux/libata.h | 4 ++-- > 3 files changed, 16 insertions(+), 15 deletions(-) > > diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c > index 7a5fe41aa5ae..13b9d0fdd42c 100644 > --- a/drivers/ata/libata-sata.c > +++ b/drivers/ata/libata-sata.c > @@ -1018,26 +1018,25 @@ DEVICE_ATTR(sw_activity, S_IWUSR | S_IRUGO, ata_scsi_activity_show, > EXPORT_SYMBOL_GPL(dev_attr_sw_activity); > > /** > - * __ata_change_queue_depth - helper for ata_scsi_change_queue_depth > - * @ap: ATA port to which the device change the queue depth > + * ata_change_queue_depth - Set a device maximum queue depth > + * @ap: ATA port of the target device > + * @dev: target ATA device > * @sdev: SCSI device to configure queue depth for > * @queue_depth: new queue depth > * > - * libsas and libata have different approaches for associating a sdev to > - * its ata_port. > + * Helper to set a device maximum queue depth, usable with both libsas > + * and libata. > * > */ > -int __ata_change_queue_depth(struct ata_port *ap, struct scsi_device *sdev, > - int queue_depth) > +int ata_change_queue_depth(struct ata_port *ap, struct ata_device *dev, > + struct scsi_device *sdev, int queue_depth) > { > - struct ata_device *dev; > unsigned long flags; > > - if (queue_depth < 1 || queue_depth == sdev->queue_depth) > + if (!dev || !ata_dev_enabled(dev)) > return sdev->queue_depth; > > - dev = ata_scsi_find_dev(ap, sdev); > - if (!dev || !ata_dev_enabled(dev)) > + if (queue_depth < 1 || queue_depth == sdev->queue_depth) > return sdev->queue_depth; > > /* NCQ enabled? */ > @@ -1059,7 +1058,7 @@ int __ata_change_queue_depth(struct ata_port *ap, struct scsi_device *sdev, > > return scsi_change_queue_depth(sdev, queue_depth); > } > -EXPORT_SYMBOL_GPL(__ata_change_queue_depth); > +EXPORT_SYMBOL_GPL(ata_change_queue_depth); > > /** > * ata_scsi_change_queue_depth - SCSI callback for queue depth config > @@ -1080,7 +1079,8 @@ int ata_scsi_change_queue_depth(struct scsi_device *sdev, int queue_depth) > { > struct ata_port *ap = ata_shost_to_port(sdev->host); > > - return __ata_change_queue_depth(ap, sdev, queue_depth); > + return ata_change_queue_depth(ap, ata_scsi_find_dev(ap, sdev), > + sdev, queue_depth); > } > EXPORT_SYMBOL_GPL(ata_scsi_change_queue_depth); > > diff --git a/drivers/scsi/libsas/sas_scsi_host.c b/drivers/scsi/libsas/sas_scsi_host.c > index 9c82e5dc4fcc..a36fa1c128a8 100644 > --- a/drivers/scsi/libsas/sas_scsi_host.c > +++ b/drivers/scsi/libsas/sas_scsi_host.c > @@ -872,7 +872,8 @@ int sas_change_queue_depth(struct scsi_device *sdev, int depth) > struct domain_device *dev = sdev_to_domain_dev(sdev); > > if (dev_is_sata(dev)) > - return __ata_change_queue_depth(dev->sata_dev.ap, sdev, depth); > + return ata_change_queue_depth(dev->sata_dev.ap, > + sas_to_ata_dev(dev), sdev, depth); > > if (!sdev->tagged_supported) > depth = 1; > diff --git a/include/linux/libata.h b/include/linux/libata.h > index 698032e5ef2d..20765d1c5f80 100644 > --- a/include/linux/libata.h > +++ b/include/linux/libata.h > @@ -1136,8 +1136,8 @@ extern int ata_scsi_slave_config(struct scsi_device *sdev); > extern void ata_scsi_slave_destroy(struct scsi_device *sdev); > extern int ata_scsi_change_queue_depth(struct scsi_device *sdev, > int queue_depth); > -extern int __ata_change_queue_depth(struct ata_port *ap, struct scsi_device *sdev, > - int queue_depth); > +extern int ata_change_queue_depth(struct ata_port *ap, struct ata_device *dev, > + struct scsi_device *sdev, int queue_depth); > extern struct ata_device *ata_dev_pair(struct ata_device *adev); > extern int ata_do_set_mode(struct ata_link *link, struct ata_device **r_failed_dev); > extern void ata_scsi_port_error_handler(struct Scsi_Host *host, struct ata_port *ap);
On 9/27/22 18:47, John Garry wrote: > On 27/09/2022 10:28, Damien Le Moal wrote: >>> Sure, we can use sas_to_ata_dev() to get the ata_device. >>> >>> I am just suggesting my way such that we can have a consistent method to >>> get the ata_device between all libata users and we don't need to change >>> the ata_change_queue_depth() interface. It would be something like: >>> >>> struct ata_device *ata_scsi_find_dev(struct ata_port *ap, const struct >>> scsi_device *scsidev) >>> { >>> struct ata_link *link; >>> struct ata_device *dev; >>> >>> ata_for_each_link(link, ap, EDGE) { >>> ata_for_each_dev(dev, link, ENABLED) { >>> if (scsidev == dev->sdev) >>> return dev; >>> } >>> } >>> // todo: check pmp >>> return NULL; >>> } >> I see. Need to think about this one... This may also unify the pmp case. >> Are you OK with the patch as is though ? > > I'm ok with your patchset, but let me test it and get back to you later > today. > > We can improve with something >> like the above on top later. Really need to fix that qd setting as it is >> causing problems for testing devices with/without ncq commands. > > Out of curiosity, are you considering your patchset for 6.0? Yes. But I can send it for 6.1 with cc: stable too. > >> > > Thanks, > John
On 9/27/22 20:51, John Garry wrote: > On 26/09/2022 00:08, Damien Le Moal wrote: >> The function __ata_change_queue_depth() uses the helper >> ata_scsi_find_dev() to get the ata_device structure of a scsi device and >> set that device maximum queue depth. However, when the ata device is >> managed by libsas, ata_scsi_find_dev() returns NULL, turning >> __ata_change_queue_depth() into a nop, which prevents the user from >> setting the maximum queue depth of ATA devices used with libsas based >> HBAs. >> >> Fix this by renaming __ata_change_queue_depth() to >> ata_change_queue_depth() and adding a pointer to the ata_device >> structure of the target device as argument. This pointer is provided by >> ata_scsi_change_queue_depth() using ata_scsi_find_dev() in the case of >> a libata managed device and by sas_change_queue_depth() using >> sas_to_ata_dev() in the case of a libsas managed ata device. >> >> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com> > > Tested-by: John Garry <john.garry@huawei.com> > > However - a big however - I will note that this following behaviour is > strange for a SATA device for libsas: > > root@(none)$ echo 33 > 0:0:2:0/device/queue_depth > root@(none)$ echo 33 > 0:0:2:0/device/queue_depth > sh: echo: write error: Invalid argument > root@(none)$ Weird. I am not getting any error (pm80xx driver). The qd gets capped at 32 as expected. Is it something that changes per libsas driver ? > I also note that setting a value out of range is just rejected for a SAS > device, and not capped to the max range (like it is for SATA). Not sure where that come from. A quick look does not reveal anything obvious. Need to dig into that one. > > AHCI rejects out of range values it as it exceeds the shost can_queue in > sdev_store_queue_depth(). Indeed: echo 1 > /sys/block/sdk/device/queue_depth echo 33 > /sys/block/sdk/device/queue_depth cat /sys/block/sdk/device/queue_depth 1 But for the libsas SATA device: echo 1 > /sys/block/sdd/device/queue_depth cat /sys/block/sdd/device/queue_depth 1 echo 33 > /sys/block/sdd/device/queue_depth cat /sys/block/sdd/device/queue_depth 32 As one would expect... Need to dig into that one. > > Thanks, > John > >> --- >> drivers/ata/libata-sata.c | 24 ++++++++++++------------ >> drivers/scsi/libsas/sas_scsi_host.c | 3 ++- >> include/linux/libata.h | 4 ++-- >> 3 files changed, 16 insertions(+), 15 deletions(-) >> >> diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c >> index 7a5fe41aa5ae..13b9d0fdd42c 100644 >> --- a/drivers/ata/libata-sata.c >> +++ b/drivers/ata/libata-sata.c >> @@ -1018,26 +1018,25 @@ DEVICE_ATTR(sw_activity, S_IWUSR | S_IRUGO, ata_scsi_activity_show, >> EXPORT_SYMBOL_GPL(dev_attr_sw_activity); >> >> /** >> - * __ata_change_queue_depth - helper for ata_scsi_change_queue_depth >> - * @ap: ATA port to which the device change the queue depth >> + * ata_change_queue_depth - Set a device maximum queue depth >> + * @ap: ATA port of the target device >> + * @dev: target ATA device >> * @sdev: SCSI device to configure queue depth for >> * @queue_depth: new queue depth >> * >> - * libsas and libata have different approaches for associating a sdev to >> - * its ata_port. >> + * Helper to set a device maximum queue depth, usable with both libsas >> + * and libata. >> * >> */ >> -int __ata_change_queue_depth(struct ata_port *ap, struct scsi_device *sdev, >> - int queue_depth) >> +int ata_change_queue_depth(struct ata_port *ap, struct ata_device *dev, >> + struct scsi_device *sdev, int queue_depth) >> { >> - struct ata_device *dev; >> unsigned long flags; >> >> - if (queue_depth < 1 || queue_depth == sdev->queue_depth) >> + if (!dev || !ata_dev_enabled(dev)) >> return sdev->queue_depth; >> >> - dev = ata_scsi_find_dev(ap, sdev); >> - if (!dev || !ata_dev_enabled(dev)) >> + if (queue_depth < 1 || queue_depth == sdev->queue_depth) >> return sdev->queue_depth; >> >> /* NCQ enabled? */ >> @@ -1059,7 +1058,7 @@ int __ata_change_queue_depth(struct ata_port *ap, struct scsi_device *sdev, >> >> return scsi_change_queue_depth(sdev, queue_depth); >> } >> -EXPORT_SYMBOL_GPL(__ata_change_queue_depth); >> +EXPORT_SYMBOL_GPL(ata_change_queue_depth); >> >> /** >> * ata_scsi_change_queue_depth - SCSI callback for queue depth config >> @@ -1080,7 +1079,8 @@ int ata_scsi_change_queue_depth(struct scsi_device *sdev, int queue_depth) >> { >> struct ata_port *ap = ata_shost_to_port(sdev->host); >> >> - return __ata_change_queue_depth(ap, sdev, queue_depth); >> + return ata_change_queue_depth(ap, ata_scsi_find_dev(ap, sdev), >> + sdev, queue_depth); >> } >> EXPORT_SYMBOL_GPL(ata_scsi_change_queue_depth); >> >> diff --git a/drivers/scsi/libsas/sas_scsi_host.c b/drivers/scsi/libsas/sas_scsi_host.c >> index 9c82e5dc4fcc..a36fa1c128a8 100644 >> --- a/drivers/scsi/libsas/sas_scsi_host.c >> +++ b/drivers/scsi/libsas/sas_scsi_host.c >> @@ -872,7 +872,8 @@ int sas_change_queue_depth(struct scsi_device *sdev, int depth) >> struct domain_device *dev = sdev_to_domain_dev(sdev); >> >> if (dev_is_sata(dev)) >> - return __ata_change_queue_depth(dev->sata_dev.ap, sdev, depth); >> + return ata_change_queue_depth(dev->sata_dev.ap, >> + sas_to_ata_dev(dev), sdev, depth); >> >> if (!sdev->tagged_supported) >> depth = 1; >> diff --git a/include/linux/libata.h b/include/linux/libata.h >> index 698032e5ef2d..20765d1c5f80 100644 >> --- a/include/linux/libata.h >> +++ b/include/linux/libata.h >> @@ -1136,8 +1136,8 @@ extern int ata_scsi_slave_config(struct scsi_device *sdev); >> extern void ata_scsi_slave_destroy(struct scsi_device *sdev); >> extern int ata_scsi_change_queue_depth(struct scsi_device *sdev, >> int queue_depth); >> -extern int __ata_change_queue_depth(struct ata_port *ap, struct scsi_device *sdev, >> - int queue_depth); >> +extern int ata_change_queue_depth(struct ata_port *ap, struct ata_device *dev, >> + struct scsi_device *sdev, int queue_depth); >> extern struct ata_device *ata_dev_pair(struct ata_device *adev); >> extern int ata_do_set_mode(struct ata_link *link, struct ata_device **r_failed_dev); >> extern void ata_scsi_port_error_handler(struct Scsi_Host *host, struct ata_port *ap); >
On 27/09/2022 16:03, Damien Le Moal wrote: > On 9/27/22 20:51, John Garry wrote: >> On 26/09/2022 00:08, Damien Le Moal wrote: >>> The function __ata_change_queue_depth() uses the helper >>> ata_scsi_find_dev() to get the ata_device structure of a scsi device and >>> set that device maximum queue depth. However, when the ata device is >>> managed by libsas, ata_scsi_find_dev() returns NULL, turning >>> __ata_change_queue_depth() into a nop, which prevents the user from >>> setting the maximum queue depth of ATA devices used with libsas based >>> HBAs. >>> >>> Fix this by renaming __ata_change_queue_depth() to >>> ata_change_queue_depth() and adding a pointer to the ata_device >>> structure of the target device as argument. This pointer is provided by >>> ata_scsi_change_queue_depth() using ata_scsi_find_dev() in the case of >>> a libata managed device and by sas_change_queue_depth() using >>> sas_to_ata_dev() in the case of a libsas managed ata device. >>> >>> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com> >> >> Tested-by: John Garry <john.garry@huawei.com> >> >> However - a big however - I will note that this following behaviour is >> strange for a SATA device for libsas: >> >> root@(none)$ echo 33 > 0:0:2:0/device/queue_depth >> root@(none)$ echo 33 > 0:0:2:0/device/queue_depth >> sh: echo: write error: Invalid argument >> root@(none)$ > > Weird. I am not getting any error (pm80xx driver). The qd gets capped at > 32 as expected. Is it something that changes per libsas driver ? That is with my pm8001 card. What happens here is that the first store of 33 gets through to ata_change_queue_depth() as it does not exceed the SAS shost can_queue, which is >> 32, and then we cap this to 32 and store it in sdev->queue_depth. And then the 2nd store of 33 also gets through, but this following expression not evaluate true in ata_change_queue_depth(): queue_depth < 1 || queue_depth == sdev->queue_depth So we don't return. However the following subsequent test does evaluate true in ata_change_queue_depth(): if (sdev->queue_depth == queue_depth) return -EINVAL; And we error. > >> I also note that setting a value out of range is just rejected for a SAS >> device, and not capped to the max range (like it is for SATA). > > Not sure where that come from. A quick look does not reveal anything > obvious. Need to dig into that one. >> >> AHCI rejects out of range values it as it exceeds the shost can_queue in >> sdev_store_queue_depth(). > > Indeed: > > echo 1 > /sys/block/sdk/device/queue_depth > echo 33 > /sys/block/sdk/device/queue_depth Hmmmm... why no error message? is the printk silenced? > cat /sys/block/sdk/device/queue_depth > 1 > > But for the libsas SATA device: > > echo 1 > /sys/block/sdd/device/queue_depth > cat /sys/block/sdd/device/queue_depth > 1 > echo 33 > /sys/block/sdd/device/queue_depth > cat /sys/block/sdd/device/queue_depth > 32 > > As one would expect... > Need to dig into that one. > thanks, John
On 9/28/22 01:09, John Garry wrote: > On 27/09/2022 16:03, Damien Le Moal wrote: >> On 9/27/22 20:51, John Garry wrote: >>> On 26/09/2022 00:08, Damien Le Moal wrote: >>>> The function __ata_change_queue_depth() uses the helper >>>> ata_scsi_find_dev() to get the ata_device structure of a scsi device and >>>> set that device maximum queue depth. However, when the ata device is >>>> managed by libsas, ata_scsi_find_dev() returns NULL, turning >>>> __ata_change_queue_depth() into a nop, which prevents the user from >>>> setting the maximum queue depth of ATA devices used with libsas based >>>> HBAs. >>>> >>>> Fix this by renaming __ata_change_queue_depth() to >>>> ata_change_queue_depth() and adding a pointer to the ata_device >>>> structure of the target device as argument. This pointer is provided by >>>> ata_scsi_change_queue_depth() using ata_scsi_find_dev() in the case of >>>> a libata managed device and by sas_change_queue_depth() using >>>> sas_to_ata_dev() in the case of a libsas managed ata device. >>>> >>>> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com> >>> >>> Tested-by: John Garry <john.garry@huawei.com> >>> >>> However - a big however - I will note that this following behaviour is >>> strange for a SATA device for libsas: >>> >>> root@(none)$ echo 33 > 0:0:2:0/device/queue_depth >>> root@(none)$ echo 33 > 0:0:2:0/device/queue_depth >>> sh: echo: write error: Invalid argument >>> root@(none)$ >> >> Weird. I am not getting any error (pm80xx driver). The qd gets capped at >> 32 as expected. Is it something that changes per libsas driver ? > > That is with my pm8001 card. > > What happens here is that the first store of 33 gets through to > ata_change_queue_depth() as it does not exceed the SAS shost can_queue, > which is >> 32, and then we cap this to 32 and store it in > sdev->queue_depth. And then the 2nd store of 33 also gets through, but > this following expression not evaluate true in ata_change_queue_depth(): > > queue_depth < 1 || queue_depth == sdev->queue_depth > > So we don't return. However the following subsequent test does evaluate > true in ata_change_queue_depth(): > > if (sdev->queue_depth == queue_depth) > return -EINVAL; > > And we error. Gotcha. This needs to be changed to: if (sdev->queue_depth == queue_depth) return queue_depth; And the non-sensical error will go away. Will send another patch for that. > >> >>> I also note that setting a value out of range is just rejected for a SAS >>> device, and not capped to the max range (like it is for SATA). >> >> Not sure where that come from. A quick look does not reveal anything >> obvious. Need to dig into that one. >>> >>> AHCI rejects out of range values it as it exceeds the shost can_queue in >>> sdev_store_queue_depth(). >> >> Indeed: >> >> echo 1 > /sys/block/sdk/device/queue_depth >> echo 33 > /sys/block/sdk/device/queue_depth > > Hmmmm... why no error message? is the printk silenced? > >> cat /sys/block/sdk/device/queue_depth >> 1 >> >> But for the libsas SATA device: >> >> echo 1 > /sys/block/sdd/device/queue_depth >> cat /sys/block/sdd/device/queue_depth >> 1 >> echo 33 > /sys/block/sdd/device/queue_depth >> cat /sys/block/sdd/device/queue_depth >> 32 >> >> As one would expect... > Need to dig into that one. >> > > thanks, > John >
On 9/28/22 01:09, John Garry wrote: > On 27/09/2022 16:03, Damien Le Moal wrote: >> On 9/27/22 20:51, John Garry wrote: >>> On 26/09/2022 00:08, Damien Le Moal wrote: >>>> The function __ata_change_queue_depth() uses the helper >>>> ata_scsi_find_dev() to get the ata_device structure of a scsi device and >>>> set that device maximum queue depth. However, when the ata device is >>>> managed by libsas, ata_scsi_find_dev() returns NULL, turning >>>> __ata_change_queue_depth() into a nop, which prevents the user from >>>> setting the maximum queue depth of ATA devices used with libsas based >>>> HBAs. >>>> >>>> Fix this by renaming __ata_change_queue_depth() to >>>> ata_change_queue_depth() and adding a pointer to the ata_device >>>> structure of the target device as argument. This pointer is provided by >>>> ata_scsi_change_queue_depth() using ata_scsi_find_dev() in the case of >>>> a libata managed device and by sas_change_queue_depth() using >>>> sas_to_ata_dev() in the case of a libsas managed ata device. >>>> >>>> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com> >>> >>> Tested-by: John Garry <john.garry@huawei.com> >>> >>> However - a big however - I will note that this following behaviour is >>> strange for a SATA device for libsas: >>> >>> root@(none)$ echo 33 > 0:0:2:0/device/queue_depth >>> root@(none)$ echo 33 > 0:0:2:0/device/queue_depth >>> sh: echo: write error: Invalid argument >>> root@(none)$ >> >> Weird. I am not getting any error (pm80xx driver). The qd gets capped at >> 32 as expected. Is it something that changes per libsas driver ? > > That is with my pm8001 card. > > What happens here is that the first store of 33 gets through to > ata_change_queue_depth() as it does not exceed the SAS shost can_queue, > which is >> 32, and then we cap this to 32 and store it in > sdev->queue_depth. And then the 2nd store of 33 also gets through, but > this following expression not evaluate true in ata_change_queue_depth(): > > queue_depth < 1 || queue_depth == sdev->queue_depth > > So we don't return. However the following subsequent test does evaluate > true in ata_change_queue_depth(): > > if (sdev->queue_depth == queue_depth) > return -EINVAL; > > And we error. I dug further into this. For AHCI, I still get an error when trying to set 33. No capping and defaulting to 32. The reason is I believe that sdev_store_queue_depth() has the check: if (depth < 1 || depth > sdev->host->can_queue) return -EINVAL; as you mentioned. So all good. So changing that last "if" in ata_change_queue_depth() to if (sdev->queue_depth == queue_depth) return sdev->queue_depth; has no effect. The error remains. Now, for a libsas SATA drive, if I add the above change, I do indeed get a cap to 32 and the QD changes, no error. That is bothering me as that is really inconsistent. Instead of suppressing the error, shouldn't we unify AHCI and libsas behavior and error if the user is attempting to set a value larger than what the *device* supports (the host can_queue was checked already). In a nutshell, the difference comes form sdev->host->can_queue being equal to the device max qd for AHCI but not necessarily for libsas. I am tempted to leave things as is for now (not changin gthe current weird behavior) and cleaning that up during the next round. Thoughts ? > >> >>> I also note that setting a value out of range is just rejected for a SAS >>> device, and not capped to the max range (like it is for SATA). >> >> Not sure where that come from. A quick look does not reveal anything >> obvious. Need to dig into that one. >>> >>> AHCI rejects out of range values it as it exceeds the shost can_queue in >>> sdev_store_queue_depth(). >> >> Indeed: >> >> echo 1 > /sys/block/sdk/device/queue_depth >> echo 33 > /sys/block/sdk/device/queue_depth > > Hmmmm... why no error message? is the printk silenced? > >> cat /sys/block/sdk/device/queue_depth >> 1 >> >> But for the libsas SATA device: >> >> echo 1 > /sys/block/sdd/device/queue_depth >> cat /sys/block/sdd/device/queue_depth >> 1 >> echo 33 > /sys/block/sdd/device/queue_depth >> cat /sys/block/sdd/device/queue_depth >> 32 >> >> As one would expect... > Need to dig into that one. >> > > thanks, > John >
On 28/09/2022 08:00, Damien Le Moal wrote: >> So we don't return. However the following subsequent test does evaluate >> true in ata_change_queue_depth(): >> >> if (sdev->queue_depth == queue_depth) >> return -EINVAL; >> >> And we error. > I dug further into this. For AHCI, I still get an error when trying to set > 33. No capping and defaulting to 32. The reason is I believe that > sdev_store_queue_depth() has the check: > > if (depth < 1 || depth > sdev->host->can_queue) > return -EINVAL; > > as you mentioned. So all good. > > So changing that last "if" in ata_change_queue_depth() to > > if (sdev->queue_depth == queue_depth) > return sdev->queue_depth; > > has no effect. The error remains. > > Now, for a libsas SATA drive, if I add the above change, I do indeed get a > cap to 32 and the QD changes, no error. That is bothering me as that is > really inconsistent. Instead of suppressing the error, shouldn't we unify > AHCI and libsas behavior and error if the user is attempting to set a > value larger than what the*device* supports (the host can_queue was > checked already). In a nutshell, the difference comes form > sdev->host->can_queue being equal to the device max qd for AHCI but not > necessarily for libsas. Yes, I think consistent behaviour would be good. I suppose we just need the same check to reject QD of > 32 in ata_change_queue_depth() (and not just cap to 32 there). Having said all that, scsi_device_max_queue_depth() does introduce some capping. But let's just consider SATA behaviour now. > > I am tempted to leave things as is for now (not changin gthe current weird > behavior) and cleaning that up during the next round. Thoughts ? > It's up to you. Obviously we are making an improvement in this series, but if we are going to backport then it's better to backport something fully working first time. Thanks, John
On 9/28/22 16:53, John Garry wrote: > On 28/09/2022 08:00, Damien Le Moal wrote: >>> So we don't return. However the following subsequent test does evaluate >>> true in ata_change_queue_depth(): >>> >>> if (sdev->queue_depth == queue_depth) >>> return -EINVAL; >>> >>> And we error. >> I dug further into this. For AHCI, I still get an error when trying to set >> 33. No capping and defaulting to 32. The reason is I believe that >> sdev_store_queue_depth() has the check: >> >> if (depth < 1 || depth > sdev->host->can_queue) >> return -EINVAL; >> >> as you mentioned. So all good. >> >> So changing that last "if" in ata_change_queue_depth() to >> >> if (sdev->queue_depth == queue_depth) >> return sdev->queue_depth; >> >> has no effect. The error remains. >> >> Now, for a libsas SATA drive, if I add the above change, I do indeed get a >> cap to 32 and the QD changes, no error. That is bothering me as that is >> really inconsistent. Instead of suppressing the error, shouldn't we unify >> AHCI and libsas behavior and error if the user is attempting to set a >> value larger than what the*device* supports (the host can_queue was >> checked already). In a nutshell, the difference comes form >> sdev->host->can_queue being equal to the device max qd for AHCI but not >> necessarily for libsas. > > Yes, I think consistent behaviour would be good. I suppose we just need > the same check to reject QD of > 32 in ata_change_queue_depth() (and not > just cap to 32 there). > > Having said all that, scsi_device_max_queue_depth() does introduce some > capping. But let's just consider SATA behaviour now. > >> >> I am tempted to leave things as is for now (not changin gthe current weird >> behavior) and cleaning that up during the next round. Thoughts ? >> > > It's up to you. Obviously we are making an improvement in this series, > but if we are going to backport then it's better to backport something > fully working first time. OK. Since the current behavior has been in place for a long time, no urgency to change anything now I think. I will push the current 2 patches for 6.0-fixes and cook a full cleanup & improvement for 6.1. > > Thanks, > John
diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c index 7a5fe41aa5ae..13b9d0fdd42c 100644 --- a/drivers/ata/libata-sata.c +++ b/drivers/ata/libata-sata.c @@ -1018,26 +1018,25 @@ DEVICE_ATTR(sw_activity, S_IWUSR | S_IRUGO, ata_scsi_activity_show, EXPORT_SYMBOL_GPL(dev_attr_sw_activity); /** - * __ata_change_queue_depth - helper for ata_scsi_change_queue_depth - * @ap: ATA port to which the device change the queue depth + * ata_change_queue_depth - Set a device maximum queue depth + * @ap: ATA port of the target device + * @dev: target ATA device * @sdev: SCSI device to configure queue depth for * @queue_depth: new queue depth * - * libsas and libata have different approaches for associating a sdev to - * its ata_port. + * Helper to set a device maximum queue depth, usable with both libsas + * and libata. * */ -int __ata_change_queue_depth(struct ata_port *ap, struct scsi_device *sdev, - int queue_depth) +int ata_change_queue_depth(struct ata_port *ap, struct ata_device *dev, + struct scsi_device *sdev, int queue_depth) { - struct ata_device *dev; unsigned long flags; - if (queue_depth < 1 || queue_depth == sdev->queue_depth) + if (!dev || !ata_dev_enabled(dev)) return sdev->queue_depth; - dev = ata_scsi_find_dev(ap, sdev); - if (!dev || !ata_dev_enabled(dev)) + if (queue_depth < 1 || queue_depth == sdev->queue_depth) return sdev->queue_depth; /* NCQ enabled? */ @@ -1059,7 +1058,7 @@ int __ata_change_queue_depth(struct ata_port *ap, struct scsi_device *sdev, return scsi_change_queue_depth(sdev, queue_depth); } -EXPORT_SYMBOL_GPL(__ata_change_queue_depth); +EXPORT_SYMBOL_GPL(ata_change_queue_depth); /** * ata_scsi_change_queue_depth - SCSI callback for queue depth config @@ -1080,7 +1079,8 @@ int ata_scsi_change_queue_depth(struct scsi_device *sdev, int queue_depth) { struct ata_port *ap = ata_shost_to_port(sdev->host); - return __ata_change_queue_depth(ap, sdev, queue_depth); + return ata_change_queue_depth(ap, ata_scsi_find_dev(ap, sdev), + sdev, queue_depth); } EXPORT_SYMBOL_GPL(ata_scsi_change_queue_depth); diff --git a/drivers/scsi/libsas/sas_scsi_host.c b/drivers/scsi/libsas/sas_scsi_host.c index 9c82e5dc4fcc..a36fa1c128a8 100644 --- a/drivers/scsi/libsas/sas_scsi_host.c +++ b/drivers/scsi/libsas/sas_scsi_host.c @@ -872,7 +872,8 @@ int sas_change_queue_depth(struct scsi_device *sdev, int depth) struct domain_device *dev = sdev_to_domain_dev(sdev); if (dev_is_sata(dev)) - return __ata_change_queue_depth(dev->sata_dev.ap, sdev, depth); + return ata_change_queue_depth(dev->sata_dev.ap, + sas_to_ata_dev(dev), sdev, depth); if (!sdev->tagged_supported) depth = 1; diff --git a/include/linux/libata.h b/include/linux/libata.h index 698032e5ef2d..20765d1c5f80 100644 --- a/include/linux/libata.h +++ b/include/linux/libata.h @@ -1136,8 +1136,8 @@ extern int ata_scsi_slave_config(struct scsi_device *sdev); extern void ata_scsi_slave_destroy(struct scsi_device *sdev); extern int ata_scsi_change_queue_depth(struct scsi_device *sdev, int queue_depth); -extern int __ata_change_queue_depth(struct ata_port *ap, struct scsi_device *sdev, - int queue_depth); +extern int ata_change_queue_depth(struct ata_port *ap, struct ata_device *dev, + struct scsi_device *sdev, int queue_depth); extern struct ata_device *ata_dev_pair(struct ata_device *adev); extern int ata_do_set_mode(struct ata_link *link, struct ata_device **r_failed_dev); extern void ata_scsi_port_error_handler(struct Scsi_Host *host, struct ata_port *ap);
The function __ata_change_queue_depth() uses the helper ata_scsi_find_dev() to get the ata_device structure of a scsi device and set that device maximum queue depth. However, when the ata device is managed by libsas, ata_scsi_find_dev() returns NULL, turning __ata_change_queue_depth() into a nop, which prevents the user from setting the maximum queue depth of ATA devices used with libsas based HBAs. Fix this by renaming __ata_change_queue_depth() to ata_change_queue_depth() and adding a pointer to the ata_device structure of the target device as argument. This pointer is provided by ata_scsi_change_queue_depth() using ata_scsi_find_dev() in the case of a libata managed device and by sas_change_queue_depth() using sas_to_ata_dev() in the case of a libsas managed ata device. Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com> --- drivers/ata/libata-sata.c | 24 ++++++++++++------------ drivers/scsi/libsas/sas_scsi_host.c | 3 ++- include/linux/libata.h | 4 ++-- 3 files changed, 16 insertions(+), 15 deletions(-)