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@xxxxxxxxxx>
Fixes: 41bda9c98035 ("libata-link: update hotplug to handle PMP links")
Cc: stable@xxxxxxxxxxxxxxx
Signed-off-by: Damien Le Moal <dlemoal@xxxxxxxxxx>
---
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
