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

PATCH In review Damien Le Moal 4 participants 8 messages May 23, 2023 – May 29, 2023 libata

Share & tools

Damien Le Moal

author maintainer

2023-05-23 08:06 UTC #0 lore raw

PATCH v3 1/1 ata: libata-scsi: Use correct device no in ata_find_dev()

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 <[email protected]>

Fixes 41bda9c98035 ("libata-link: update hotplug to handle PMP links")

Cc [email protected]

Signed-off-by Damien Le Moal <[email protected]>

---

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

drivers/ata/libata-scsi.c +26 -8

index 7bb12deab70c..6878ddf49880 100644
@@ -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;
 }

Signature · 1 line

2.40.1

John Garry

Reviewed-by on v3 1/1

2023-05-23 08:34 UTC #1 lore raw

Quoted 29 lines · 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.

Quoted 4 lines

Reported-by Xingui Yang <[email protected]>
Fixes 41bda9c98035 ("libata-link: update hotplug to handle PMP links")
Cc [email protected]
Signed-off-by Damien Le Moal <[email protected]>

Reviewed-by John Garry <[email protected]>

Quoted 19 lines

---
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(-)
drivers/ata/libata-scsi.c +0 -0
index 7bb12deab70c..6878ddf49880 100644
@@ -2694,18 +2694,36 @@ static unsigned int atapi_xlat(struct ata_queued_cmd *qc)
  	return 0;

drivers/ata/libata-scsi.c static unsigned int atapi_xlat(struct ata_queued_cmd *qc) · line 2700

Quoted 10 lines

drivers/ata/libata-scsi.c +3 -4

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

drivers/ata/libata-scsi.c static unsigned int atapi_xlat(struct ata_queued_cmd *qc) · line 2707

Quoted 7 lines

drivers/ata/libata-scsi.c +7 -0

+	 * 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.

Quoted 26 lines

drivers/ata/libata-scsi.c +16 -4

+		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;
  }

Damien Le Moal

author maintainer

2023-05-23 08:43 UTC #2 lore raw

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

[...]

Quoted 10 lines

drivers/ata/libata-scsi.c static unsigned int atapi_xlat(struct ata_queued_cmd *qc) · line 2707
Quoted 7 lines
drivers/ata/libata-scsi.c +7 -0
+	 * 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.

Signature · 2 lines

Damien Le Moal
Western Digital Research

Jason Yan

2023-05-23 09:23 UTC #3 lore raw

In reply to John Garry

Quoted 30 lines · John Garry

Quoted 29 lines · 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.

Quoted 6 lines

Quoted 4 lines
Reported-by Xingui Yang <[email protected]>
Fixes 41bda9c98035 ("libata-link: update hotplug to handle PMP links")
Cc [email protected]
Signed-off-by Damien Le Moal <[email protected]>
Reviewed-by John Garry <[email protected]>

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

Damien Le Moal

author maintainer

2023-05-23 10:22 UTC #4 lore raw

Quoted 38 lines · Jason Yan

Quoted 30 lines · John Garry
Quoted 29 lines · 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.
Quoted 6 lines
Quoted 4 lines
Reported-by Xingui Yang <[email protected]>
Fixes 41bda9c98035 ("libata-link: update hotplug to handle PMP links")
Cc [email protected]
Signed-off-by Damien Le Moal <[email protected]>
Reviewed-by John Garry <[email protected]>

Quoted 8 lines

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.

Quoted 3 lines

Thanks,
Jason

Signature · 2 lines

Damien Le Moal
Western Digital Research

Damien Le Moal

author maintainer

2023-05-29 06:46 UTC #5 lore raw

In reply to Jason Yan

Quoted 38 lines · Jason Yan

Quoted 30 lines · John Garry
Quoted 29 lines · 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.
Quoted 6 lines
Quoted 4 lines
Reported-by Xingui Yang <[email protected]>
Fixes 41bda9c98035 ("libata-link: update hotplug to handle PMP links")
Cc [email protected]
Signed-off-by Damien Le Moal <[email protected]>
Reviewed-by John Garry <[email protected]>

Quoted 8 lines

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.

Signature · 2 lines

Damien Le Moal
Western Digital Research

Jason Yan

Reviewed-by

2023-05-29 09:11 UTC #6 lore raw

Quoted 87 lines · Damien Le Moal

Quoted 38 lines · Jason Yan
Quoted 30 lines · John Garry
Quoted 29 lines · 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.
Quoted 6 lines
Quoted 4 lines
Reported-by Xingui Yang <[email protected]>
Fixes 41bda9c98035 ("libata-link: update hotplug to handle PMP links")
Cc [email protected]
Signed-off-by Damien Le Moal <[email protected]>
Reviewed-by John Garry <[email protected]>
Quoted 8 lines
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

Quoted 7 lines

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 <[email protected]>

Sergey Shtylyov

2023-05-23 09:01 UTC #7 lore raw

In reply to Damien Le Moal

Quoted 56 lines · 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 <[email protected]>
Fixes 41bda9c98035 ("libata-link: update hotplug to handle PMP links")
Cc [email protected]
Signed-off-by Damien Le Moal <[email protected]>
---
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(-)
drivers/ata/libata-scsi.c +1 -1
index 7bb12deab70c..6878ddf49880 100644
@@ -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)

drivers/ata/libata-scsi.c static unsigned int atapi_xlat(struct ata_queued_cmd *qc) · line 2704

Quoted 10 lines

drivers/ata/libata-scsi.c +6 -3

 {
-	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? :-)

Quoted 11 lines

drivers/ata/libata-scsi.c +9 -1

+	 * 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 {

drivers/ata/libata-scsi.c static unsigned int atapi_xlat(struct ata_queued_cmd *qc) · line 2721

Quoted 10 lines

drivers/ata/libata-scsi.c +5 -3

-		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