Re: [PATCH v3] ata: libata-scsi: Use correct device no in ata_find_dev()

[Jason Yan <yanaijie@huawei.com>]

On 2023/5/29 14:45, Damien Le Moal wrote:
> On 5/23/23 18:23, Jason Yan wrote:
>> On 2023/5/23 16:29, John Garry wrote:
>>> 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@huawei.com>
>>>> Fixes: 41bda9c98035 ("libata-link: update hotplug to handle PMP links")
>>>> Cc: stable@vger.kernel.org
>>>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
>>>
>>> Reviewed-by: John Garry <john.g.garry@oracle.com>
>>
>> Hi Damien & John,
>>
>> I think we may missed something. What about if we do this:
>>
>> echo "scsi add-single-device 0 0 100 0" >/proc/scsi/scsi
>>
>> Then in ata_scsi_user_scan() we will return device "0 0 0 0 " and rescan
>> this device, which is wrong?
> 
> I did some more digging into this. And I do not think there are any
> issues (I tested and it works). The reason is that the "shost" passed to
> ata_scsi_user_scan() corresponds to the scsi host for the ata_port of
> the ata_device in libsas (dev->sata_dev.ap). It is not the scsi_host
> representing the HBA itself (which has multiple devices).
> 
> So changing the devno to 0 has no effect and deleting+rescanning
> particular devices leads to the correct IDs being used. E.g., on my
> system, I have 4 drives connected to the pm80xx:
> 
> # lsscsi -g
> [0:0:0:0]    disk    ATA      WDC  WUH721818AL W232  /dev/sdc   /dev/sg3
> [0:0:1:0]    disk    ATA      WDC  WUH721818AL WTW2  /dev/sdb   /dev/sg1
> [0:0:2:0]    disk    ATA      WDC  WUH722222AL Wf86  /dev/sdd   /dev/sg2
> [0:0:3:0]    zbc     ATA      WDC  WSH722020AL W803  /dev/sda   /dev/sg0
> 
> If I remove the first 0 and 2:
> 
> # echo 1 > /sys/class/scsi_device/0:0:0:0/device/delete
> # echo 1 > /sys/class/scsi_device/0:0:2:0/device/delete
> # lsscsi -g
> [0:0:1:0]    disk    ATA      WDC  WUH721818AL WTW2  /dev/sdb   /dev/sg1
> [0:0:3:0]    zbc     ATA      WDC  WSH722020AL W803  /dev/sda   /dev/sg0
> 
> And then manually rescan in reverse order of the removal:
> 
> # echo "scsi add-single-device 0 0 2 0" > /proc/scsi/scsi
> # lsscsi -g
> [0:0:1:0]    disk    ATA      WDC  WUH721818AL WTW2  /dev/sdb   /dev/sg1
> [0:0:2:0]    disk    ATA      WDC  WUH722222AL Wf86  /dev/sdc   /dev/sg2
> [0:0:3:0]    zbc     ATA      WDC  WSH722020AL W803  /dev/sda   /dev/sg0
> 
> # echo "scsi add-single-device 0 0 0 0" > /proc/scsi/scsi
> # lsscsi -g
> [0:0:0:0]    disk    ATA      WDC  WUH721818AL W232  /dev/sdd   /dev/sg3
> [0:0:1:0]    disk    ATA      WDC  WUH721818AL WTW2  /dev/sdb   /dev/sg1
> [0:0:2:0]    disk    ATA      WDC  WUH722222AL Wf86  /dev/sdc   /dev/sg2
> [0:0:3:0]    zbc     ATA      WDC  WSH722020AL W803  /dev/sda   /dev/sg0
> 
> I get back all devices with the correct IDs.
> 
> Note that I tried John's suggestion as well using an ata_for_each_dev()
> loop. That does work well for libsas as we do have ata_dev->sdev set to
> the scsi device already when scanning, but does not work for AHCI as we
> do not. I will keep doing some more tests with v3 (and correct typos and
> suggested) but I think this is all good.

Thanks to clarify this:

Reviewed-by: Jason Yan <yanaijie@huawei.com>