ata: libata-scsi: Use correct device no in ata_find_dev()
Damien Le Moal
maintainerPATCH 1/1 ata: libata-scsi: Use correct device no in ata_find_dev()
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.
--- drivers/ata/libata-scsi.c | 31 ++++++++++++++++++++++++------- 1 file changed, 24 insertions(+), 7 deletions(-)
drivers/ata/libata-scsi.c +24 -7
@@ -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 ) +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 ; }
Signature · 1 line
2.40.1
On 22/05/2023 12:27, Damien Le Moal wrote:
Hi Damien,
Our mails just crossed...
Quoted 4 lines
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.
Quoted 36 lines
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 correctata_devicestruct 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 <[email protected]>Fixes 41bda9c98035 ("libata-link: update hotplug to handle PMP links")Signed-off-by Damien Le Moal <[email protected]>--- drivers/ata/libata-scsi.c | 31 ++++++++++++++++++++++++------- 1 file changed, 24 insertions(+), 7 deletions(-)drivers/ata/libata-scsi.c +7
-3@@ -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
drivers/ata/libata-scsi.c static unsigned int atapi_xlat(struct ata_queued_cmd *qc) · line 2715
Quoted 10 lines
drivers/ata/libata-scsi.c +10
-0+* 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?
Quoted 6 lines
drivers/ata/libata-scsi.c +1
-4+return NULL ;return &ap->link.device[devno]; - }else { -if (likely (devno >=0 && - devno < ap->nr_pmp_links)) -return &ap->pmp_link[devno].device[0 ];
drivers/ata/libata-scsi.c static unsigned int atapi_xlat(struct ata_queued_cmd *qc) · line 2727
Quoted 10 lines
drivers/ata/libata-scsi.c +6
-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
Damien Le Moal
maintainerQuoted 6 lines · John Garry
On 22/05/2023 12:27, Damien Le Moal wrote:
Hi Damien,
Our mails just crossed...
Quoted 7 lines
Quoted 4 lines
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->devnois configured the same, it's just that the sdev
devno used in ata_find_dev() is scsidev->id, which is always 0 for a non-pmp SATA drive (and can be 1 for an IDE slave drive). But with libsas, that number is 0, 1, 2, 3... numbering the devices found on the HBA port. Hence ata_find_dev() return NULL always because the ata_link_max_devices() is always 1 for any libsas ata device as they all have their own ata_port & ata_link.
Quoted 2 lines
per
ata_devicedoes not have the same numbering scheme for libsas. For libsas - orscsi_transport_sas, to be more exact - the sdev id is per shost.
Yes. So the sdev->id goes beyond 0 and cannot be used as the devno for the devices as they each have their own port & link.
Quoted 41 lines
Quoted 41 lines
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 correctata_devicestruct 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 <[email protected]>Fixes 41bda9c98035 ("libata-link: update hotplug to handle PMP links")Signed-off-by Damien Le Moal <[email protected]>--- drivers/ata/libata-scsi.c | 31 ++++++++++++++++++++++++------- 1 file changed, 24 insertions(+), 7 deletions(-)drivers/ata/libata-scsi.c +11
-3@@ -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.
Quoted 8 lines
drivers/ata/libata-scsi.c static unsigned int atapi_xlat(struct ata_queued_cmd *qc) · line 2715
Quoted 6 lines
drivers/ata/libata-scsi.c +6
-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?
Indeed. That would be nicer. And ata_link_max_devices() could use that as well.
Quoted 12 lines
Quoted 12 lines
drivers/ata/libata-scsi.c +4
-4+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 ; + } }
Quoted 7 lines
drivers/ata/libata-scsi.c static unsigned int atapi_xlat(struct ata_queued_cmd *qc) · line 2727
Quoted 5 lines
drivers/ata/libata-scsi.c +3
-0+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 am not super happy about the wording of the comment. Suggestions welcome :) I will at least reword to mention the counting of devices per shost.
Quoted 1 line
I did send another suggestion, so I'll leave it to you.
I kind of like the loop as it does not need a devno but it also implies that we do have a scsi dev already while ata_find_dev() currently does not assume that. Given that this function is also used in ata_scsi_user_scan() where we do not have a sdev, it would not work for all cases.
Quoted 3 lines
BTW, I think that we can follow-up to this and remove the add
ata_devicearg that we added tosas_change_queue_depth()
Yes. I will clean that up after sending this fix for this cycle. The cleanup will be for 6.5.
Quoted 4 lines
Thanks,
John
Signature · 2 lines
Damien Le Moal Western Digital Research
Quoted 27 lines · Damien Le Moal
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 inata_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 correctata_devicestruct 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 <[email protected]>Fixes 41bda9c98035 ("libata-link: update hotplug to handle PMP links")Signed-off-by Damien Le Moal <[email protected]>--- drivers/ata/libata-scsi.c | 31 ++++++++++++++++++++++++------- 1 file changed, 24 insertions(+), 7 deletions(-)drivers/ata/libata-scsi.c +0
-0
drivers/ata/libata-scsi.c static unsigned int atapi_xlat(struct ata_queued_cmd *qc) · line 2700
Quoted 8 lines
drivers/ata/libata-scsi.c +2
-3@@ -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.
Quoted 21 lines
drivers/ata/libata-scsi.c +16
-4+ +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 ];
drivers/ata/libata-scsi.c static unsigned int atapi_xlat(struct ata_queued_cmd *qc) · line 2727
Quoted 10 lines
drivers/ata/libata-scsi.c +6
-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