[PATCH] ata: libata-scsi: Use correct device no in ata_find_dev()

[PATCH] ata: libata-scsi: Use correct device no in ata_find_dev()

[Damien Le Moal]

For non-pmp attached devices managed directly by libata, the device
number is always 0 or 1 and lower to the maximum number of devices
returned by ata_link_max_devices(). However, for libsas managed devices,
devices are numbered up to the number of device scanned on an HBA port,
while each device has a regular ata/link setup supporting at most 1
device per link. This results in ata_find_dev() always returning NULL
except for the first device with device number 0.

Fix this by rewriting ata_find_dev() to ignore the device number for
non-pmp attached devices with a link with at most 1 device. For these,
device number 0 is always used to return the correct ata_device struct
of the port link. This change excludes IDE master/slave setups (maximum
number of devices per link is 2) and port-multiplier attached devices.

Reported-by: Xingui Yang <yangxingui@huawei.com>
Fixes: 41bda9c98035 ("libata-link: update hotplug to handle PMP links")
Cc: stable@vger.kernel.org
Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
 drivers/ata/libata-scsi.c | 31 ++++++++++++++++++++++++-------
 1 file changed, 24 insertions(+), 7 deletions(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 7bb12deab70c..3ba9cb258394 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -2696,16 +2696,33 @@ static unsigned int atapi_xlat(struct ata_queued_cmd *qc)
 
 static struct ata_device *ata_find_dev(struct ata_port *ap, int devno)
 {
-	if (!sata_pmp_attached(ap)) {
-		if (likely(devno >= 0 &&
-			   devno < ata_link_max_devices(&ap->link)))
+	if (unlikely(devno < 0))
+		return NULL;
+
+	if (likely(!sata_pmp_attached(ap))) {
+		/*
+		 * For the non PMP case, the maximum number of devices per link
+		 * is 1 (e.g. SATA case), or 2 (IDE master + slave). The former
+		 * case includes libsas hosted devices which are numbered up to
+		 * the number of devices scanned on an HBA port, but with each
+		 * ata device having its own ata port and link. To accommodate
+		 * these, ignore devno and always use device number 0.
+		 */
+		switch (ata_link_max_devices(&ap->link)) {
+		case 1:
+			return &ap->link.device[0];
+		case 2:
+			if (devno >= 2)
+				return NULL;
 			return &ap->link.device[devno];
-	} else {
-		if (likely(devno >= 0 &&
-			   devno < ap->nr_pmp_links))
-			return &ap->pmp_link[devno].device[0];
+		default:
+			return NULL;
+		}
 	}
 
+	if (devno < ap->nr_pmp_links)
+		return &ap->pmp_link[devno].device[0];
+
 	return NULL;
 }
 
-- 
2.40.1

Re: [PATCH] ata: libata-scsi: Use correct device no in ata_find_dev()

[John Garry]

On 22/05/2023 12:27, Damien Le Moal wrote:

Hi Damien,

Our mails just crossed...

> For non-pmp attached devices managed directly by libata, the device
> number is always 0 or 1 and lower to the maximum number of devices
> returned by ata_link_max_devices(). However, for libsas managed devices,
> devices are numbered up to the number of device scanned on an HBA port,

It's not really clear to me which number you mean. For libsas and lib 
ata, ata_device->devno is configured the same, it's just that the sdev 
per ata_device does not have the same numbering scheme for libsas. For 
libsas - or scsi_transport_sas, to be more exact - the sdev id is per shost.

> while each device has a regular ata/link setup supporting at most 1
> device per link. This results in ata_find_dev() always returning NULL
> except for the first device with device number 0.
> 
> Fix this by rewriting ata_find_dev() to ignore the device number for
> non-pmp attached devices with a link with at most 1 device. For these,
> device number 0 is always used to return the correct ata_device struct
> of the port link. This change excludes IDE master/slave setups (maximum
> number of devices per link is 2) and port-multiplier attached devices.
> 
> Reported-by: Xingui Yang <yangxingui@huawei.com>
> Fixes: 41bda9c98035 ("libata-link: update hotplug to handle PMP links")
> Cc: stable@vger.kernel.org
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
>   drivers/ata/libata-scsi.c | 31 ++++++++++++++++++++++++-------
>   1 file changed, 24 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 7bb12deab70c..3ba9cb258394 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -2696,16 +2696,33 @@ static unsigned int atapi_xlat(struct ata_queued_cmd *qc)
>   
>   static struct ata_device *ata_find_dev(struct ata_port *ap, int devno)
>   {
> -	if (!sata_pmp_attached(ap)) {
> -		if (likely(devno >= 0 &&
> -			   devno < ata_link_max_devices(&ap->link)))
> +	if (unlikely(devno < 0))
> +		return NULL;
> +
> +	if (likely(!sata_pmp_attached(ap))) {
> +		/*
> +		 * For the non PMP case, the maximum number of devices per link
> +		 * is 1 (e.g. SATA case), or 2 (IDE master + slave). The former
> +		 * case includes libsas hosted devices which are numbered up to
> +		 * the number of devices scanned on an HBA port, but with each
> +		 * ata device having its own ata port and link. To accommodate
> +		 * these, ignore devno and always use device number 0.
> +		 */
> +		switch (ata_link_max_devices(&ap->link)) {
> +		case 1:
> +			return &ap->link.device[0];
> +		case 2:
> +			if (devno >= 2)

How about ATA_MAX_DEVICES?

> +				return NULL;
>   			return &ap->link.device[devno];
> -	} else {
> -		if (likely(devno >= 0 &&
> -			   devno < ap->nr_pmp_links))
> -			return &ap->pmp_link[devno].device[0];
> +		default:
> +			return NULL;
> +		}
>   	}
>   
> +	if (devno < ap->nr_pmp_links)
> +		return &ap->pmp_link[devno].device[0];
> +
>   	return NULL;
>   }

This looks ok to me, since we have a big comment about what we're doing. 
I did send another suggestion, so I'll leave it to you.

BTW, I think that we can follow-up to this and remove the add ata_device 
arg that we added to sas_change_queue_depth()

Thanks,
John

Re: [PATCH] ata: libata-scsi: Use correct device no in ata_find_dev()

[Damien Le Moal]

On 5/22/23 20:48, John Garry wrote:
> On 22/05/2023 12:27, Damien Le Moal wrote:
> 
> Hi Damien,
> 
> Our mails just crossed...
> 
>> For non-pmp attached devices managed directly by libata, the device
>> number is always 0 or 1 and lower to the maximum number of devices
>> returned by ata_link_max_devices(). However, for libsas managed devices,
>> devices are numbered up to the number of device scanned on an HBA port,
> 
> It's not really clear to me which number you mean. For libsas and lib 
> ata, ata_device->devno is configured the same, it's just that the sdev 

devno used in ata_find_dev() is scsidev->id, which is always 0 for a non-pmp
SATA drive (and can be 1 for an IDE slave drive). But with libsas, that number
is 0, 1, 2, 3... numbering the devices found on the HBA port. Hence
ata_find_dev() return NULL always because the ata_link_max_devices() is always 1
for any libsas ata device as they all have their own ata_port & ata_link.

> per ata_device does not have the same numbering scheme for libsas. For 
> libsas - or scsi_transport_sas, to be more exact - the sdev id is per shost.

Yes. So the sdev->id goes beyond 0 and cannot be used as the devno for the
devices as they each have their own port & link.

> 
>> while each device has a regular ata/link setup supporting at most 1
>> device per link. This results in ata_find_dev() always returning NULL
>> except for the first device with device number 0.
>>
>> Fix this by rewriting ata_find_dev() to ignore the device number for
>> non-pmp attached devices with a link with at most 1 device. For these,
>> device number 0 is always used to return the correct ata_device struct
>> of the port link. This change excludes IDE master/slave setups (maximum
>> number of devices per link is 2) and port-multiplier attached devices.
>>
>> Reported-by: Xingui Yang <yangxingui@huawei.com>
>> Fixes: 41bda9c98035 ("libata-link: update hotplug to handle PMP links")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
>> ---
>>   drivers/ata/libata-scsi.c | 31 ++++++++++++++++++++++++-------
>>   1 file changed, 24 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
>> index 7bb12deab70c..3ba9cb258394 100644
>> --- a/drivers/ata/libata-scsi.c
>> +++ b/drivers/ata/libata-scsi.c
>> @@ -2696,16 +2696,33 @@ static unsigned int atapi_xlat(struct ata_queued_cmd *qc)
>>   
>>   static struct ata_device *ata_find_dev(struct ata_port *ap, int devno)
>>   {
>> -	if (!sata_pmp_attached(ap)) {
>> -		if (likely(devno >= 0 &&
>> -			   devno < ata_link_max_devices(&ap->link)))
>> +	if (unlikely(devno < 0))
>> +		return NULL;
>> +
>> +	if (likely(!sata_pmp_attached(ap))) {
>> +		/*
>> +		 * For the non PMP case, the maximum number of devices per link
>> +		 * is 1 (e.g. SATA case), or 2 (IDE master + slave). The former
>> +		 * case includes libsas hosted devices which are numbered up to
>> +		 * the number of devices scanned on an HBA port, but with each
>> +		 * ata device having its own ata port and link. To accommodate
>> +		 * these, ignore devno and always use device number 0.
>> +		 */
>> +		switch (ata_link_max_devices(&ap->link)) {
>> +		case 1:
>> +			return &ap->link.device[0];
>> +		case 2:
>> +			if (devno >= 2)
> 
> How about ATA_MAX_DEVICES?

Indeed. That would be nicer. And ata_link_max_devices() could use that as well.

> 
>> +				return NULL;
>>   			return &ap->link.device[devno];
>> -	} else {
>> -		if (likely(devno >= 0 &&
>> -			   devno < ap->nr_pmp_links))
>> -			return &ap->pmp_link[devno].device[0];
>> +		default:
>> +			return NULL;
>> +		}
>>   	}
>>   
>> +	if (devno < ap->nr_pmp_links)
>> +		return &ap->pmp_link[devno].device[0];
>> +
>>   	return NULL;
>>   }
> 
> This looks ok to me, since we have a big comment about what we're doing. 

I am not super happy about the wording of the comment. Suggestions welcome :)
I will at least reword to mention the counting of devices per shost.

> I did send another suggestion, so I'll leave it to you.

I kind of like the loop as it does not need a devno but it also implies that we
do have a scsi dev already while ata_find_dev() currently does not assume that.
Given that this function is also used in ata_scsi_user_scan() where we do not
have a sdev, it would not work for all cases.

> 
> BTW, I think that we can follow-up to this and remove the add ata_device 
> arg that we added to sas_change_queue_depth()

Yes. I will clean that up after sending this fix for this cycle. The cleanup
will be for 6.5.

> 
> Thanks,
> John
> 

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH] ata: libata-scsi: Use correct device no in ata_find_dev()

[Jason Yan]

On 2023/5/22 19:27, Damien Le Moal wrote:
> For non-pmp attached devices managed directly by libata, the device
> number is always 0 or 1 and lower to the maximum number of devices
> returned by ata_link_max_devices(). However, for libsas managed devices,
> devices are numbered up to the number of device scanned on an HBA port,
> while each device has a regular ata/link setup supporting at most 1
> device per link. This results in ata_find_dev() always returning NULL
> except for the first device with device number 0.
> 
> Fix this by rewriting ata_find_dev() to ignore the device number for
> non-pmp attached devices with a link with at most 1 device. For these,
> device number 0 is always used to return the correct ata_device struct
> of the port link. This change excludes IDE master/slave setups (maximum
> number of devices per link is 2) and port-multiplier attached devices.
> 
> Reported-by: Xingui Yang <yangxingui@huawei.com>
> Fixes: 41bda9c98035 ("libata-link: update hotplug to handle PMP links")
> Cc: stable@vger.kernel.org
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
>   drivers/ata/libata-scsi.c | 31 ++++++++++++++++++++++++-------
>   1 file changed, 24 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 7bb12deab70c..3ba9cb258394 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -2696,16 +2696,33 @@ static unsigned int atapi_xlat(struct ata_queued_cmd *qc)
>   
>   static struct ata_device *ata_find_dev(struct ata_port *ap, int devno)
>   {
> -	if (!sata_pmp_attached(ap)) {
> -		if (likely(devno >= 0 &&
> -			   devno < ata_link_max_devices(&ap->link)))
> +	if (unlikely(devno < 0))
> +		return NULL;

devno is either scsidev->id or scsidev->channel, which will never < 0. 
Actually it is unsigned int. So I doubt if we need this check here.

> +
> +	if (likely(!sata_pmp_attached(ap))) {
> +		/*
> +		 * For the non PMP case, the maximum number of devices per link
> +		 * is 1 (e.g. SATA case), or 2 (IDE master + slave). The former
> +		 * case includes libsas hosted devices which are numbered up to
> +		 * the number of devices scanned on an HBA port, but with each
> +		 * ata device having its own ata port and link. To accommodate
> +		 * these, ignore devno and always use device number 0.
> +		 */
> +		switch (ata_link_max_devices(&ap->link)) {
> +		case 1:
> +			return &ap->link.device[0];
> +		case 2:
> +			if (devno >= 2)
> +				return NULL;
>   			return &ap->link.device[devno];
> -	} else {
> -		if (likely(devno >= 0 &&
> -			   devno < ap->nr_pmp_links))
> -			return &ap->pmp_link[devno].device[0];
> +		default:
> +			return NULL;
> +		}
>   	}
>   
> +	if (devno < ap->nr_pmp_links)
> +		return &ap->pmp_link[devno].device[0];
> +
>   	return NULL;
>   }

This change looks good to me.

Thanks,
Jason