On 2023/05/05 11:57, Xingui Yang wrote:
> The function ata_get_identity() uses the helper ata_scsi_find_dev() to get
> the ata_device structure of a scsi device. However, when the ata device is
> managed by libsas, ata_scsi_find_dev() returns NULL, turning
> ata_get_identity() into a nop and always returns -ENOMSG.
What do you do to hit the issue ? A while back for me it was the queue depth
setting causing problems. As Garry mentioned, this led to patch 141f3d6256e5
("ata: libata-sata: Fix device queue depth control").
>
> Fix this by checking whether ATA_FLAG_SAS_HOST is set for ap->flags in
> ata_scsi_find_dev(), as the flag is only used in libsas. If
> ATA_FLAG_SAS_HOST is set, use sas_to_ata_dev() to find associated ATA
> device.
>
> Signed-off-by: Xingui Yang <yangxingui@xxxxxxxxxx>
> ---
> Changes to v1
> - Let ata_scsi_find_dev() return the correct value and don't keep replacing
> calls to ata_scsi_find_dev().
>
> drivers/ata/libata-scsi.c | 12 ++++++++++--
> drivers/ata/libata.h | 2 +-
> 2 files changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 7bb12deab70c..aa580ea341fa 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -26,6 +26,7 @@
> #include <scsi/scsi_device.h>
> #include <scsi/scsi_tcq.h>
> #include <scsi/scsi_transport.h>
> +#include <scsi/libsas.h>
> #include <linux/libata.h>
> #include <linux/hdreg.h>
> #include <linux/uaccess.h>
> @@ -2745,10 +2746,17 @@ static struct ata_device *__ata_scsi_find_dev(struct ata_port *ap,
> * Associated ATA device, or %NULL if not found.
> */
> struct ata_device *
> -ata_scsi_find_dev(struct ata_port *ap, const struct scsi_device *scsidev)
Why drop the const ?
> +ata_scsi_find_dev(struct ata_port *ap, struct scsi_device *scsidev)
> {
> - struct ata_device *dev = __ata_scsi_find_dev(ap, scsidev);
> + struct ata_device *dev;
> +
> + if (ap->flags & ATA_FLAG_SAS_HOST) {
> + struct domain_device *ddev = sdev_to_domain_dev(scsidev);
> +
> + return sas_to_ata_dev(ddev);
Do you really need the ddev variable ? Also, this really should be a libsas
helper. I beleive this pattern is repeated in several places in libsas, so that
would nicely clean things up.
> + }
>
> + dev = __ata_scsi_find_dev(ap, scsidev);
> if (unlikely(!dev || !ata_dev_enabled(dev)))
> return NULL;
>
> diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
> index 926d0d33cd29..6d66f46da064 100644
> --- a/drivers/ata/libata.h
> +++ b/drivers/ata/libata.h
> @@ -109,7 +109,7 @@ static inline void ata_acpi_bind_dev(struct ata_device *dev) {}
>
> /* libata-scsi.c */
> extern struct ata_device *ata_scsi_find_dev(struct ata_port *ap,
> - const struct scsi_device *scsidev);
> + struct scsi_device *scsidev);
> extern int ata_scsi_add_hosts(struct ata_host *host,
> const struct scsi_host_template *sht);
> extern void ata_scsi_scan_host(struct ata_port *ap, int sync);
--
Damien Le Moal
Western Digital Research
