Re: [PATCH] ata: libata-scsi: Fix get identity data failed

[yangxingui]

On 2023/5/4 22:24, John Garry wrote:
> On 04/05/2023 14:15, 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.
> >
> > Fix this by replacing the pointer to the scsi_device struct argument 
> > with a
> > pointer to the ata_device struct in ata_sas_scsi_ioctl() and
> > ata_get_identity(). This pointer is provided by ata_scsi_ioctl() using
> > ata_scsi_find_dev() in the case of a libata managed device and by
> > sas_ioctl() using sas_to_ata_dev() in the case of a libsas managed ata
> > device.
>
> We made a similar change in commit 141f3d6256e5 ("ata: libata-sata: Fix 
> device queue depth control"). Is there some better way to do this to 
> make it all more scalable such that we don't need to keep replacing 
> calls to ata_scsi_find_dev()?
Yes, We do have a way to return the correct value in 
ata_scsi_find_dev(), and we don't need to keep replacing calls to 
ata_scsi_find_dev(). I'll update with a new version.

Thanks,
Xingui
> This was discussed earlier, along with queue depth issue (which does not 
> seem fixed, AFAIK):
> https://lore.kernel.org/linux-scsi/13e5e5e5-7dc2-8f14-3dd2-43366343842d@xxxxxxxxxx/ 
>
>
> Thanks,
> John
>
>
> > Signed-off-by: Xingui Yang <yangxingui@xxxxxxxxxx>
> > ---
> >   drivers/ata/libata-scsi.c           | 22 +++++++++++-----------
> >   drivers/scsi/libsas/sas_scsi_host.c |  3 ++-
> >   include/linux/libata.h              |  2 +-
> >   3 files changed, 14 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> > index 7bb12deab70c..68f2404e61d0 100644
> > --- a/drivers/ata/libata-scsi.c
> > +++ b/drivers/ata/libata-scsi.c
> > @@ -327,8 +327,7 @@ EXPORT_SYMBOL_GPL(ata_scsi_unlock_native_capacity);
> >   /**
> >    *    ata_get_identity - Handler for HDIO_GET_IDENTITY ioctl
> > - *    @ap: target port
> > - *    @sdev: SCSI device to get identify data for
> > + *    @dev: ATA device to get identify data for
> >    *    @arg: User buffer area for identify data
> >    *
> >    *    LOCKING:
> > @@ -337,10 +336,8 @@ EXPORT_SYMBOL_GPL(ata_scsi_unlock_native_capacity);
> >    *    RETURNS:
> >    *    Zero on success, negative errno on error.
> >    */
> > -static int ata_get_identity(struct ata_port *ap, struct scsi_device 
> > *sdev,
> > -                void __user *arg)
> > +static int ata_get_identity(struct ata_device *dev, void __user *arg)
> >   {
> > -    struct ata_device *dev = ata_scsi_find_dev(ap, sdev);
> >       u16 __user *dst = arg;
> >       char buf[40];
> > @@ -573,7 +570,7 @@ static bool ata_ioc32(struct ata_port *ap)
> >    * This handles both native and compat commands, so anything added
> >    * here must have a compatible argument, or check in_compat_syscall()
> >    */
> > -int ata_sas_scsi_ioctl(struct ata_port *ap, struct scsi_device *scsidev,
> > +int ata_sas_scsi_ioctl(struct ata_port *ap, struct ata_device *dev,
> >                unsigned int cmd, void __user *arg)
> >   {
> >       unsigned long val;
> > @@ -608,17 +605,17 @@ int ata_sas_scsi_ioctl(struct ata_port *ap, 
> > struct scsi_device *scsidev,
> >           return rc;
> >       case HDIO_GET_IDENTITY:
> > -        return ata_get_identity(ap, scsidev, arg);
> > +        return ata_get_identity(dev, arg);
> >       case HDIO_DRIVE_CMD:
> >           if (!capable(CAP_SYS_ADMIN) || !capable(CAP_SYS_RAWIO))
> >               return -EACCES;
> > -        return ata_cmd_ioctl(scsidev, arg);
> > +        return ata_cmd_ioctl(dev->sdev, arg);
> >       case HDIO_DRIVE_TASK:
> >           if (!capable(CAP_SYS_ADMIN) || !capable(CAP_SYS_RAWIO))
> >               return -EACCES;
> > -        return ata_task_ioctl(scsidev, arg);
> > +        return ata_task_ioctl(dev->sdev, arg);
> >       default:
> >           rc = -ENOTTY;
> > @@ -632,8 +629,11 @@ EXPORT_SYMBOL_GPL(ata_sas_scsi_ioctl);
> >   int ata_scsi_ioctl(struct scsi_device *scsidev, unsigned int cmd,
> >              void __user *arg)
> >   {
> > -    return ata_sas_scsi_ioctl(ata_shost_to_port(scsidev->host),
> > -                scsidev, cmd, arg);
> > +    struct ata_port *ap = ata_shost_to_port(scsidev->host);
> > +
> > +    return ata_sas_scsi_ioctl(ap,
> > +            ata_scsi_find_dev(ap, scsidev),
> > +            cmd, arg);
> >   }
> >   EXPORT_SYMBOL_GPL(ata_scsi_ioctl);
> > diff --git a/drivers/scsi/libsas/sas_scsi_host.c 
> > b/drivers/scsi/libsas/sas_scsi_host.c
> > index a36fa1c128a8..c7a44ce7b2e2 100644
> > --- a/drivers/scsi/libsas/sas_scsi_host.c
> > +++ b/drivers/scsi/libsas/sas_scsi_host.c
> > @@ -789,7 +789,8 @@ int sas_ioctl(struct scsi_device *sdev, unsigned 
> > int cmd, void __user *arg)
> >       struct domain_device *dev = sdev_to_domain_dev(sdev);
> >       if (dev_is_sata(dev))
> > -        return ata_sas_scsi_ioctl(dev->sata_dev.ap, sdev, cmd, arg);
> > +        return ata_sas_scsi_ioctl(dev->sata_dev.ap,
> > +                sas_to_ata_dev(dev), cmd, arg);
> >       return -EINVAL;
> >   }
> > diff --git a/include/linux/libata.h b/include/linux/libata.h
> > index 311cd93377c7..d5dd60530a24 100644
> > --- a/include/linux/libata.h
> > +++ b/include/linux/libata.h
> > @@ -1085,7 +1085,7 @@ bool ata_scsi_dma_need_drain(struct request *rq);
> >   #else
> >   #define ata_scsi_dma_need_drain NULL
> >   #endif
> > -extern int ata_sas_scsi_ioctl(struct ata_port *ap, struct scsi_device 
> > *dev,
> > +extern int ata_sas_scsi_ioctl(struct ata_port *ap, struct ata_device 
> > *dev,
> >                   unsigned int cmd, void __user *arg);
> >   extern bool ata_link_online(struct ata_link *link);
> >   extern bool ata_link_offline(struct ata_link *link);
>
> .