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

[Damien Le Moal <dlemoal@kernel.org>]

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?

On first review, I think it is OK because there is one scsi host per port. So
when ata_scsi_user_scan() with ID 100, the shost passed to it corresponds to an
ata_port that is for that device ID 100, and then ata_find_dev() changing devno
from 100 to 0 is correct.

But that "one shost == one port" seems to be correct only for pure libata, that
is, AHCI and other SATA/PATA adapters. For libsas, I am not so sure: the initial
"ap = ata_shost_to_port(shost)" in ata_scsi_user_scan() seems totally bogus for
the libsas case, while it is clearly OK for AHCI... Hmm... Needs more digging.


> 
> Thanks,
> Jason

-- 
Damien Le Moal
Western Digital Research