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@xxxxxxxxxx>
Fixes: 41bda9c98035 ("libata-link: update hotplug to handle PMP links")
Cc: stable@xxxxxxxxxxxxxxx
Signed-off-by: Damien Le Moal <dlemoal@xxxxxxxxxx>
Reviewed-by: John Garry <john.g.garry@xxxxxxxxxx>
---
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;
}
