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