Re: [PATCH 1/2] [PATCH v1 1/2] libata: fix reading concurrent positioning ranges log

[Sergey Shtylyov <s.shtylyov@omp.ru>]

Hello!

On 5/31/22 8:50 PM, Tyler Erickson wrote:

> From: Tyler Erickson <tyler.j.erickson@seagate.com>
> 
> The concurrent positioning ranges log is not a fixed size and may depend
> on how many ranges are supported by the device. This patch uses the size
> reported in the GPL directory to determine the number of pages supported
> by the device before attempting to read this log page.
> 
> Also fixing the page length in the SCSI translation for the concurrent
> positioning ranges VPD page.
> 
> This resolves this error from the dmesg output:
>     ata6.00: Read log 0x47 page 0x00 failed, Emask 0x1
> 
> Signed-off-by: Tyler Erickson <tyler.j.erickson@seagate.com>
> Reviewed-by: Muhammad Ahmad <muhammad.ahmad@seagate.com>
> Tested-by: Michael English <michael.english@seagate.com>
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index ca64837641be..3d57fa84e2be 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -2003,16 +2003,16 @@ unsigned int ata_read_log_page(struct ata_device *dev, u8 log,
>  	return err_mask;
>  }
>  
> -static bool ata_log_supported(struct ata_device *dev, u8 log)
> +static int ata_log_supported(struct ata_device *dev, u8 log)

   Maybe *unsigned int*? The 'buf_len' variable below is 'size_t' which is *unsigned* type...

>  {
>  	struct ata_port *ap = dev->link->ap;
>  
>  	if (dev->horkage & ATA_HORKAGE_NO_LOG_DIR)
> -		return false;
> +		return 0;
>  
>  	if (ata_read_log_page(dev, ATA_LOG_DIRECTORY, 0, ap->sector_buf, 1))
> -		return false;
> -	return get_unaligned_le16(&ap->sector_buf[log * 2]) ? true : false;
> +		return 0;
> +	return get_unaligned_le16(&ap->sector_buf[log * 2]);
>  }
>  
>  static bool ata_identify_page_supported(struct ata_device *dev, u8 page)
> @@ -2448,15 +2448,20 @@ static void ata_dev_config_cpr(struct ata_device *dev)
>  	struct ata_cpr_log *cpr_log = NULL;
>  	u8 *desc, *buf = NULL;
>  
> -	if (ata_id_major_version(dev->id) < 11 ||
> -	    !ata_log_supported(dev, ATA_LOG_CONCURRENT_POSITIONING_RANGES))
> +	if (ata_id_major_version(dev->id) < 11)
> +		goto out;
> +
> +	buf_len = ata_log_supported(dev, ATA_LOG_CONCURRENT_POSITIONING_RANGES);
> +	if (buf_len == 0)
>  		goto out;
[...]

MBR, Sergey