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

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

[Damien Le Moal]

For devices not attached to a port multiplier and managed directly by
libata, the device number passed to ata_find_dev() must always be lower
than the maximum number of devices returned by ata_link_max_devices().
That is 1 for SATA devices or 2 for an IDE link with master+slave
devices. This device number is the scsi device ID which matches these
constraint as the ID are generated per port and so never exceed the
link maximum.

However, for libsas managed devices, scsi device IDs are assigned per
scsi host, leading to device IDs for SATA devices that can be well in
excess of libata per-link maximum number of devices. This results in
ata_find_dev() always returning NULL for libsas managed devices except
for the first device of the host with ID (device number) 0. This issue
is visible by executing hdparm command, which fails:

hdparm -i /dev/sdX
/dev/sdX:
  HDIO_GET_IDENTITY failed: No message of desired type

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, that is SATA
devices on SATA ports. 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. Also, to be consistant with
the fact that scsi device IDs and channel numbers used as device numbers
are both unsigned int, change the devno argument of ata_find_dev() to
unsinged int.

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>
---

Changes from v2:
 * Change ata_find_dev() devno argument type to unsigned int

Changes from v1:
 * Simplify code change (remove uneeded check and remove switch-case)
 * Reword and improve comments in ata_find_dev()
 * Reword commit message

 drivers/ata/libata-scsi.c | 34 ++++++++++++++++++++++++++--------
 1 file changed, 26 insertions(+), 8 deletions(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 7bb12deab70c..6878ddf49880 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -2694,18 +2694,36 @@ static unsigned int atapi_xlat(struct ata_queued_cmd *qc)
 	return 0;
 }
 
-static struct ata_device *ata_find_dev(struct ata_port *ap, int devno)
+static struct ata_device *ata_find_dev(struct ata_port *ap, unsigned int devno)
 {
-	if (!sata_pmp_attached(ap)) {
-		if (likely(devno >= 0 &&
-			   devno < ata_link_max_devices(&ap->link)))
+	/*
+	 * For the non PMP case, link_max_devices is 1 (e.g. SATA case),
+	 * or 2 (IDE master + slave). However, the former case includes
+	 * libsas hosted devices which are numbered per host, leading
+	 * to devno potentially being larger than 0 but with each ata device
+	 * having its own ata port and ata link. To accommodate these, ignore
+	 * devno and always use device number 0.
+	 */
+	if (likely(!sata_pmp_attached(ap))) {
+		int link_max_devices = ata_link_max_devices(&ap->link);
+
+		if (link_max_devices == 1)
+			return &ap->link.device[0];
+
+		if (devno < link_max_devices)
 			return &ap->link.device[devno];
-	} else {
-		if (likely(devno >= 0 &&
-			   devno < ap->nr_pmp_links))
-			return &ap->pmp_link[devno].device[0];
+
+		return NULL;
 	}
 
+	/*
+	 * For PMP-attached devices, the device number corresponds to C
+	 * (channel) of SCSI [H:C:I:L], indicating the port pmp link
+	 * for the device.
+	 */
+	if (devno < ap->nr_pmp_links)
+		return &ap->pmp_link[devno].device[0];
+
 	return NULL;
 }
 
-- 
2.40.1

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

[John Garry]

On 23/05/2023 09:04, Damien Le Moal wrote:
> For devices not attached to a port multiplier and managed directly by
> libata, the device number passed to ata_find_dev() must always be lower
> than the maximum number of devices returned by ata_link_max_devices().
> That is 1 for SATA devices or 2 for an IDE link with master+slave
> devices. This device number is the scsi device ID which matches these
> constraint as the ID are generated per port and so never exceed the
> link maximum.
> 
> However, for libsas managed devices, scsi device IDs are assigned per
> scsi host, leading to device IDs for SATA devices that can be well in
> excess of libata per-link maximum number of devices. This results in
> ata_find_dev() always returning NULL for libsas managed devices except
> for the first device of the host with ID (device number) 0. This issue
> is visible by executing hdparm command, which fails:
> 
> hdparm -i /dev/sdX
> /dev/sdX:
>    HDIO_GET_IDENTITY failed: No message of desired type
> 
> 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, that is SATA
> devices on SATA ports. 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. Also, to be consistant with
> the fact that scsi device IDs and channel numbers used as device numbers
> are both unsigned int, change the devno argument of ata_find_dev() to
> unsinged int.
> 
> 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>

Reviewed-by: John Garry <john.g.garry@oracle.com>

> ---
> 
> Changes from v2:
>   * Change ata_find_dev() devno argument type to unsigned int
> 
> Changes from v1:
>   * Simplify code change (remove uneeded check and remove switch-case)
>   * Reword and improve comments in ata_find_dev()
>   * Reword commit message
> 
>   drivers/ata/libata-scsi.c | 34 ++++++++++++++++++++++++++--------
>   1 file changed, 26 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 7bb12deab70c..6878ddf49880 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -2694,18 +2694,36 @@ static unsigned int atapi_xlat(struct ata_queued_cmd *qc)
>   	return 0;
>   }
>   
> -static struct ata_device *ata_find_dev(struct ata_port *ap, int devno)
> +static struct ata_device *ata_find_dev(struct ata_port *ap, unsigned int devno)
>   {
> -	if (!sata_pmp_attached(ap)) {
> -		if (likely(devno >= 0 &&
> -			   devno < ata_link_max_devices(&ap->link)))
> +	/*
> +	 * For the non PMP case, link_max_devices is 1 (e.g. SATA case),

mega nit: non-PMP

> +	 * or 2 (IDE master + slave). However, the former case includes
> +	 * libsas hosted devices which are numbered per host, leading
> +	 * to devno potentially being larger than 0 but with each ata device
> +	 * having its own ata port and ata link. To accommodate these, ignore
> +	 * devno and always use device number 0.
> +	 */
> +	if (likely(!sata_pmp_attached(ap))) {

Is this function ever used in fastpath? I doubt it, but I suppose having 
likely() can't do any damage.

> +		int link_max_devices = ata_link_max_devices(&ap->link);
> +
> +		if (link_max_devices == 1)
> +			return &ap->link.device[0];
> +
> +		if (devno < link_max_devices)
>   			return &ap->link.device[devno];
> -	} else {
> -		if (likely(devno >= 0 &&
> -			   devno < ap->nr_pmp_links))
> -			return &ap->pmp_link[devno].device[0];
> +
> +		return NULL;
>   	}
>   
> +	/*
> +	 * For PMP-attached devices, the device number corresponds to C
> +	 * (channel) of SCSI [H:C:I:L], indicating the port pmp link
> +	 * for the device.
> +	 */
> +	if (devno < ap->nr_pmp_links)
> +		return &ap->pmp_link[devno].device[0];
> +
>   	return NULL;
>   }
>   

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

[Damien Le Moal]

On 5/23/23 17:29, John Garry wrote:

[...]

>> +	 * or 2 (IDE master + slave). However, the former case includes
>> +	 * libsas hosted devices which are numbered per host, leading
>> +	 * to devno potentially being larger than 0 but with each ata device
>> +	 * having its own ata port and ata link. To accommodate these, ignore
>> +	 * devno and always use device number 0.
>> +	 */
>> +	if (likely(!sata_pmp_attached(ap))) {
> 
> Is this function ever used in fastpath? I doubt it, but I suppose having 
> likely() can't do any damage.

Yes, we could drop that.


-- 
Damien Le Moal
Western Digital Research

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

[Jason Yan]

On 2023/5/23 16:29, John Garry wrote:
> On 23/05/2023 09:04, Damien Le Moal wrote:
>> For devices not attached to a port multiplier and managed directly by
>> libata, the device number passed to ata_find_dev() must always be lower
>> than the maximum number of devices returned by ata_link_max_devices().
>> That is 1 for SATA devices or 2 for an IDE link with master+slave
>> devices. This device number is the scsi device ID which matches these
>> constraint as the ID are generated per port and so never exceed the
>> link maximum.
>>
>> However, for libsas managed devices, scsi device IDs are assigned per
>> scsi host, leading to device IDs for SATA devices that can be well in
>> excess of libata per-link maximum number of devices. This results in
>> ata_find_dev() always returning NULL for libsas managed devices except
>> for the first device of the host with ID (device number) 0. This issue
>> is visible by executing hdparm command, which fails:
>>
>> hdparm -i /dev/sdX
>> /dev/sdX:
>>    HDIO_GET_IDENTITY failed: No message of desired type
>>
>> 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, that is SATA
>> devices on SATA ports. 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. Also, to be consistant with
>> the fact that scsi device IDs and channel numbers used as device numbers
>> are both unsigned int, change the devno argument of ata_find_dev() to
>> unsinged int.
>>
>> 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>
> 
> Reviewed-by: John Garry <john.g.garry@oracle.com>

Hi Damien & John,

I think we may missed something. What about if we do this:

echo "scsi add-single-device 0 0 100 0" >/proc/scsi/scsi

Then in ata_scsi_user_scan() we will return device "0 0 0 0 " and rescan 
this device, which is wrong?

Thanks,
Jason

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

[Damien Le Moal]

On 5/23/23 18:23, Jason Yan wrote:
> On 2023/5/23 16:29, John Garry wrote:
>> On 23/05/2023 09:04, Damien Le Moal wrote:
>>> For devices not attached to a port multiplier and managed directly by
>>> libata, the device number passed to ata_find_dev() must always be lower
>>> than the maximum number of devices returned by ata_link_max_devices().
>>> That is 1 for SATA devices or 2 for an IDE link with master+slave
>>> devices. This device number is the scsi device ID which matches these
>>> constraint as the ID are generated per port and so never exceed the
>>> link maximum.
>>>
>>> However, for libsas managed devices, scsi device IDs are assigned per
>>> scsi host, leading to device IDs for SATA devices that can be well in
>>> excess of libata per-link maximum number of devices. This results in
>>> ata_find_dev() always returning NULL for libsas managed devices except
>>> for the first device of the host with ID (device number) 0. This issue
>>> is visible by executing hdparm command, which fails:
>>>
>>> hdparm -i /dev/sdX
>>> /dev/sdX:
>>>    HDIO_GET_IDENTITY failed: No message of desired type
>>>
>>> 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, that is SATA
>>> devices on SATA ports. 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. Also, to be consistant with
>>> the fact that scsi device IDs and channel numbers used as device numbers
>>> are both unsigned int, change the devno argument of ata_find_dev() to
>>> unsinged int.
>>>
>>> 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>
>>
>> Reviewed-by: John Garry <john.g.garry@oracle.com>
> 
> Hi Damien & John,
> 
> I think we may missed something. What about if we do this:
> 
> echo "scsi add-single-device 0 0 100 0" >/proc/scsi/scsi
> 
> Then in ata_scsi_user_scan() we will return device "0 0 0 0 " and rescan 
> this device, which is wrong?

On first review, I think it is OK because there is one scsi host per port. So
when ata_scsi_user_scan() with ID 100, the shost passed to it corresponds to an
ata_port that is for that device ID 100, and then ata_find_dev() changing devno
from 100 to 0 is correct.

But that "one shost == one port" seems to be correct only for pure libata, that
is, AHCI and other SATA/PATA adapters. For libsas, I am not so sure: the initial
"ap = ata_shost_to_port(shost)" in ata_scsi_user_scan() seems totally bogus for
the libsas case, while it is clearly OK for AHCI... Hmm... Needs more digging.


> 
> Thanks,
> Jason

-- 
Damien Le Moal
Western Digital Research

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

[Damien Le Moal]

On 5/23/23 18:23, Jason Yan wrote:
> On 2023/5/23 16:29, John Garry wrote:
>> On 23/05/2023 09:04, Damien Le Moal wrote:
>>> For devices not attached to a port multiplier and managed directly by
>>> libata, the device number passed to ata_find_dev() must always be lower
>>> than the maximum number of devices returned by ata_link_max_devices().
>>> That is 1 for SATA devices or 2 for an IDE link with master+slave
>>> devices. This device number is the scsi device ID which matches these
>>> constraint as the ID are generated per port and so never exceed the
>>> link maximum.
>>>
>>> However, for libsas managed devices, scsi device IDs are assigned per
>>> scsi host, leading to device IDs for SATA devices that can be well in
>>> excess of libata per-link maximum number of devices. This results in
>>> ata_find_dev() always returning NULL for libsas managed devices except
>>> for the first device of the host with ID (device number) 0. This issue
>>> is visible by executing hdparm command, which fails:
>>>
>>> hdparm -i /dev/sdX
>>> /dev/sdX:
>>>    HDIO_GET_IDENTITY failed: No message of desired type
>>>
>>> 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, that is SATA
>>> devices on SATA ports. 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. Also, to be consistant with
>>> the fact that scsi device IDs and channel numbers used as device numbers
>>> are both unsigned int, change the devno argument of ata_find_dev() to
>>> unsinged int.
>>>
>>> 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>
>>
>> Reviewed-by: John Garry <john.g.garry@oracle.com>
> 
> Hi Damien & John,
> 
> I think we may missed something. What about if we do this:
> 
> echo "scsi add-single-device 0 0 100 0" >/proc/scsi/scsi
> 
> Then in ata_scsi_user_scan() we will return device "0 0 0 0 " and rescan
> this device, which is wrong?

I did some more digging into this. And I do not think there are any
issues (I tested and it works). The reason is that the "shost" passed to
ata_scsi_user_scan() corresponds to the scsi host for the ata_port of
the ata_device in libsas (dev->sata_dev.ap). It is not the scsi_host
representing the HBA itself (which has multiple devices).

So changing the devno to 0 has no effect and deleting+rescanning
particular devices leads to the correct IDs being used. E.g., on my
system, I have 4 drives connected to the pm80xx:

# lsscsi -g
[0:0:0:0]    disk    ATA      WDC  WUH721818AL W232  /dev/sdc   /dev/sg3
[0:0:1:0]    disk    ATA      WDC  WUH721818AL WTW2  /dev/sdb   /dev/sg1
[0:0:2:0]    disk    ATA      WDC  WUH722222AL Wf86  /dev/sdd   /dev/sg2
[0:0:3:0]    zbc     ATA      WDC  WSH722020AL W803  /dev/sda   /dev/sg0

If I remove the first 0 and 2:

# echo 1 > /sys/class/scsi_device/0:0:0:0/device/delete
# echo 1 > /sys/class/scsi_device/0:0:2:0/device/delete
# lsscsi -g
[0:0:1:0]    disk    ATA      WDC  WUH721818AL WTW2  /dev/sdb   /dev/sg1
[0:0:3:0]    zbc     ATA      WDC  WSH722020AL W803  /dev/sda   /dev/sg0

And then manually rescan in reverse order of the removal:

# echo "scsi add-single-device 0 0 2 0" > /proc/scsi/scsi
# lsscsi -g
[0:0:1:0]    disk    ATA      WDC  WUH721818AL WTW2  /dev/sdb   /dev/sg1
[0:0:2:0]    disk    ATA      WDC  WUH722222AL Wf86  /dev/sdc   /dev/sg2
[0:0:3:0]    zbc     ATA      WDC  WSH722020AL W803  /dev/sda   /dev/sg0

# echo "scsi add-single-device 0 0 0 0" > /proc/scsi/scsi
# lsscsi -g
[0:0:0:0]    disk    ATA      WDC  WUH721818AL W232  /dev/sdd   /dev/sg3
[0:0:1:0]    disk    ATA      WDC  WUH721818AL WTW2  /dev/sdb   /dev/sg1
[0:0:2:0]    disk    ATA      WDC  WUH722222AL Wf86  /dev/sdc   /dev/sg2
[0:0:3:0]    zbc     ATA      WDC  WSH722020AL W803  /dev/sda   /dev/sg0

I get back all devices with the correct IDs.

Note that I tried John's suggestion as well using an ata_for_each_dev()
loop. That does work well for libsas as we do have ata_dev->sdev set to
the scsi device already when scanning, but does not work for AHCI as we
do not. I will keep doing some more tests with v3 (and correct typos and
suggested) but I think this is all good.

-- 
Damien Le Moal
Western Digital Research

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

[Jason Yan]

On 2023/5/29 14:45, Damien Le Moal wrote:
> On 5/23/23 18:23, Jason Yan wrote:
>> On 2023/5/23 16:29, John Garry wrote:
>>> On 23/05/2023 09:04, Damien Le Moal wrote:
>>>> For devices not attached to a port multiplier and managed directly by
>>>> libata, the device number passed to ata_find_dev() must always be lower
>>>> than the maximum number of devices returned by ata_link_max_devices().
>>>> That is 1 for SATA devices or 2 for an IDE link with master+slave
>>>> devices. This device number is the scsi device ID which matches these
>>>> constraint as the ID are generated per port and so never exceed the
>>>> link maximum.
>>>>
>>>> However, for libsas managed devices, scsi device IDs are assigned per
>>>> scsi host, leading to device IDs for SATA devices that can be well in
>>>> excess of libata per-link maximum number of devices. This results in
>>>> ata_find_dev() always returning NULL for libsas managed devices except
>>>> for the first device of the host with ID (device number) 0. This issue
>>>> is visible by executing hdparm command, which fails:
>>>>
>>>> hdparm -i /dev/sdX
>>>> /dev/sdX:
>>>>     HDIO_GET_IDENTITY failed: No message of desired type
>>>>
>>>> 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, that is SATA
>>>> devices on SATA ports. 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. Also, to be consistant with
>>>> the fact that scsi device IDs and channel numbers used as device numbers
>>>> are both unsigned int, change the devno argument of ata_find_dev() to
>>>> unsinged int.
>>>>
>>>> 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>
>>>
>>> Reviewed-by: John Garry <john.g.garry@oracle.com>
>>
>> Hi Damien & John,
>>
>> I think we may missed something. What about if we do this:
>>
>> echo "scsi add-single-device 0 0 100 0" >/proc/scsi/scsi
>>
>> Then in ata_scsi_user_scan() we will return device "0 0 0 0 " and rescan
>> this device, which is wrong?
> 
> I did some more digging into this. And I do not think there are any
> issues (I tested and it works). The reason is that the "shost" passed to
> ata_scsi_user_scan() corresponds to the scsi host for the ata_port of
> the ata_device in libsas (dev->sata_dev.ap). It is not the scsi_host
> representing the HBA itself (which has multiple devices).
> 
> So changing the devno to 0 has no effect and deleting+rescanning
> particular devices leads to the correct IDs being used. E.g., on my
> system, I have 4 drives connected to the pm80xx:
> 
> # lsscsi -g
> [0:0:0:0]    disk    ATA      WDC  WUH721818AL W232  /dev/sdc   /dev/sg3
> [0:0:1:0]    disk    ATA      WDC  WUH721818AL WTW2  /dev/sdb   /dev/sg1
> [0:0:2:0]    disk    ATA      WDC  WUH722222AL Wf86  /dev/sdd   /dev/sg2
> [0:0:3:0]    zbc     ATA      WDC  WSH722020AL W803  /dev/sda   /dev/sg0
> 
> If I remove the first 0 and 2:
> 
> # echo 1 > /sys/class/scsi_device/0:0:0:0/device/delete
> # echo 1 > /sys/class/scsi_device/0:0:2:0/device/delete
> # lsscsi -g
> [0:0:1:0]    disk    ATA      WDC  WUH721818AL WTW2  /dev/sdb   /dev/sg1
> [0:0:3:0]    zbc     ATA      WDC  WSH722020AL W803  /dev/sda   /dev/sg0
> 
> And then manually rescan in reverse order of the removal:
> 
> # echo "scsi add-single-device 0 0 2 0" > /proc/scsi/scsi
> # lsscsi -g
> [0:0:1:0]    disk    ATA      WDC  WUH721818AL WTW2  /dev/sdb   /dev/sg1
> [0:0:2:0]    disk    ATA      WDC  WUH722222AL Wf86  /dev/sdc   /dev/sg2
> [0:0:3:0]    zbc     ATA      WDC  WSH722020AL W803  /dev/sda   /dev/sg0
> 
> # echo "scsi add-single-device 0 0 0 0" > /proc/scsi/scsi
> # lsscsi -g
> [0:0:0:0]    disk    ATA      WDC  WUH721818AL W232  /dev/sdd   /dev/sg3
> [0:0:1:0]    disk    ATA      WDC  WUH721818AL WTW2  /dev/sdb   /dev/sg1
> [0:0:2:0]    disk    ATA      WDC  WUH722222AL Wf86  /dev/sdc   /dev/sg2
> [0:0:3:0]    zbc     ATA      WDC  WSH722020AL W803  /dev/sda   /dev/sg0
> 
> I get back all devices with the correct IDs.
> 
> Note that I tried John's suggestion as well using an ata_for_each_dev()
> loop. That does work well for libsas as we do have ata_dev->sdev set to
> the scsi device already when scanning, but does not work for AHCI as we
> do not. I will keep doing some more tests with v3 (and correct typos and
> suggested) but I think this is all good.

Thanks to clarify this:

Reviewed-by: Jason Yan <yanaijie@huawei.com>

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

[Sergey Shtylyov]

On 5/23/23 11:04 AM, Damien Le Moal wrote:

> For devices not attached to a port multiplier and managed directly by
> libata, the device number passed to ata_find_dev() must always be lower
> than the maximum number of devices returned by ata_link_max_devices().
> That is 1 for SATA devices or 2 for an IDE link with master+slave
> devices. This device number is the scsi device ID which matches these
> constraint as the ID are generated per port and so never exceed the
> link maximum.
> 
> However, for libsas managed devices, scsi device IDs are assigned per
> scsi host, leading to device IDs for SATA devices that can be well in
> excess of libata per-link maximum number of devices. This results in
> ata_find_dev() always returning NULL for libsas managed devices except
> for the first device of the host with ID (device number) 0. This issue
> is visible by executing hdparm command, which fails:
> 
> hdparm -i /dev/sdX
> /dev/sdX:
>   HDIO_GET_IDENTITY failed: No message of desired type
> 
> 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, that is SATA
> devices on SATA ports. 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. Also, to be consistant with
> the fact that scsi device IDs and channel numbers used as device numbers
> are both unsigned int, change the devno argument of ata_find_dev() to
> unsinged int.
> 
> 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>
> ---
> 
> Changes from v2:
>  * Change ata_find_dev() devno argument type to unsigned int
> 
> Changes from v1:
>  * Simplify code change (remove uneeded check and remove switch-case)
>  * Reword and improve comments in ata_find_dev()
>  * Reword commit message
> 
>  drivers/ata/libata-scsi.c | 34 ++++++++++++++++++++++++++--------
>  1 file changed, 26 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 7bb12deab70c..6878ddf49880 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -2694,18 +2694,36 @@ static unsigned int atapi_xlat(struct ata_queued_cmd *qc)
>  	return 0;
>  }
>  
> -static struct ata_device *ata_find_dev(struct ata_port *ap, int devno)
> +static struct ata_device *ata_find_dev(struct ata_port *ap, unsigned int devno)
>  {
> -	if (!sata_pmp_attached(ap)) {
> -		if (likely(devno >= 0 &&
> -			   devno < ata_link_max_devices(&ap->link)))
> +	/*
> +	 * For the non PMP case, link_max_devices is 1 (e.g. SATA case),
> +	 * or 2 (IDE master + slave). However, the former case includes
> +	 * libsas hosted devices which are numbered per host, leading
> +	 * to devno potentially being larger than 0 but with each ata device
> +	 * having its own ata port and ata link. To accommodate these, ignore

   Hm, you use PMP, SATA, and IDE upper-cased but not ATA? :-)

> +	 * devno and always use device number 0.
> +	 */
> +	if (likely(!sata_pmp_attached(ap))) {
> +		int link_max_devices = ata_link_max_devices(&ap->link);
> +
> +		if (link_max_devices == 1)
> +			return &ap->link.device[0];
> +
> +		if (devno < link_max_devices)
>  			return &ap->link.device[devno];
> -	} else {
> -		if (likely(devno >= 0 &&
> -			   devno < ap->nr_pmp_links))
> -			return &ap->pmp_link[devno].device[0];
> +
> +		return NULL;
>  	}
>  
> +	/*
> +	 * For PMP-attached devices, the device number corresponds to C
> +	 * (channel) of SCSI [H:C:I:L], indicating the port pmp link

   1st time you type PMP, 2nd time pmp? :-)

[...]

MBR, Sergey