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.
Signed-off-by: Xingui Yang <[email protected]>
---
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);
--
2.17.1
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()?
This was discussed earlier, along with queue depth issue (which does not
seem fixed, AFAIK):
https://lore.kernel.org/linux-scsi/[email protected]/
Thanks,
John
>
> Signed-off-by: Xingui Yang <[email protected]>
> ---
> 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);
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/[email protected]/
>
>
> Thanks,
> John
>
>
>>
>> Signed-off-by: Xingui Yang <[email protected]>
>> ---
>> 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);
>
> .