On 5/22/23 20:48, John Garry wrote:
> 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
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.
> 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.
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.
>
>> 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?
Indeed. That would be nicer. And ata_link_max_devices() could use that as well.
>
>> + 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 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.
> 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.
>
> 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()
Yes. I will clean that up after sending this fix for this cycle. The cleanup
will be for 6.5.
>
> Thanks,
> John
>
--
Damien Le Moal
Western Digital Research
