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

[John Garry <john.g.garry@oracle.com>]

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>

> ---
> 
> 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),

mega nit: non-PMP

> +	 * 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
> +	 * devno and always use device number 0.
> +	 */
> +	if (likely(!sata_pmp_attached(ap))) {

Is this function ever used in fastpath? I doubt it, but I suppose having 
likely() can't do any damage.

> +		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
> +	 * for the device.
> +	 */
> +	if (devno < ap->nr_pmp_links)
> +		return &ap->pmp_link[devno].device[0];
> +
>   	return NULL;
>   }
>