On 5/23/23 11:04 AM, 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@xxxxxxxxxx>
> Fixes: 41bda9c98035 ("libata-link: update hotplug to handle PMP links")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Damien Le Moal <dlemoal@xxxxxxxxxx>
> ---
>
> Changes from v2:
> * Change ata_find_dev() devno argument type to unsigned int
>
> Changes from v1:
> * Simplify code change (remove uneeded check and remove switch-case)
> * Reword and improve comments in ata_find_dev()
> * Reword commit message
>
> drivers/ata/libata-scsi.c | 34 ++++++++++++++++++++++++++--------
> 1 file changed, 26 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 7bb12deab70c..6878ddf49880 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -2694,18 +2694,36 @@ static unsigned int atapi_xlat(struct ata_queued_cmd *qc)
> return 0;
> }
>
> -static struct ata_device *ata_find_dev(struct ata_port *ap, int devno)
> +static struct ata_device *ata_find_dev(struct ata_port *ap, unsigned int devno)
> {
> - if (!sata_pmp_attached(ap)) {
> - if (likely(devno >= 0 &&
> - devno < ata_link_max_devices(&ap->link)))
> + /*
> + * For the non PMP case, link_max_devices is 1 (e.g. SATA case),
> + * or 2 (IDE master + slave). However, the former case includes
> + * libsas hosted devices which are numbered per host, leading
> + * to devno potentially being larger than 0 but with each ata device
> + * having its own ata port and ata link. To accommodate these, ignore
Hm, you use PMP, SATA, and IDE upper-cased but not ATA? :-)
> + * devno and always use device number 0.
> + */
> + if (likely(!sata_pmp_attached(ap))) {
> + int link_max_devices = ata_link_max_devices(&ap->link);
> +
> + if (link_max_devices == 1)
> + return &ap->link.device[0];
> +
> + if (devno < link_max_devices)
> return &ap->link.device[devno];
> - } else {
> - if (likely(devno >= 0 &&
> - devno < ap->nr_pmp_links))
> - return &ap->pmp_link[devno].device[0];
> +
> + return NULL;
> }
>
> + /*
> + * For PMP-attached devices, the device number corresponds to C
> + * (channel) of SCSI [H:C:I:L], indicating the port pmp link
1st time you type PMP, 2nd time pmp? :-)
[...]
MBR, Sergey
