diff mbox series

[v2,1/3] libata: fix reading concurrent positioning ranges log

Message ID 20220602225113.10218-2-tyler.erickson@seagate.com (mailing list archive)
State Not Applicable
Headers show
Series ata,sd: Fix reading concurrent positioning ranges | expand

Commit Message

Tyler Erickson June 2, 2022, 10:51 p.m. UTC
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.

This resolves this error from the dmesg output:
    ata6.00: Read log 0x47 page 0x00 failed, Emask 0x1

Cc: stable@vger.kernel.org
Fixes: fe22e1c2f705 ("libata: support concurrent positioning ranges log")
Signed-off-by: Tyler Erickson <tyler.erickson@seagate.com>
Reviewed-by: Muhammad Ahmad <muhammad.ahmad@seagate.com>
Tested-by: Michael English <michael.english@seagate.com>
---
 drivers/ata/libata-core.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

Comments

Hannes Reinecke June 3, 2022, 6:17 a.m. UTC | #1
On 6/3/22 00:51, Tyler Erickson wrote:
> 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.
> 
> This resolves this error from the dmesg output:
>      ata6.00: Read log 0x47 page 0x00 failed, Emask 0x1
> 
> Cc: stable@vger.kernel.org
> Fixes: fe22e1c2f705 ("libata: support concurrent positioning ranges log")
> Signed-off-by: Tyler Erickson <tyler.erickson@seagate.com>
> Reviewed-by: Muhammad Ahmad <muhammad.ahmad@seagate.com>
> Tested-by: Michael English <michael.english@seagate.com>
> ---
>   drivers/ata/libata-core.c | 21 +++++++++++++--------
>   1 file changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 40e816419f48..3ea10f72cb70 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -2010,16 +2010,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)
>   {
>   	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]);
>   }
>   
Maybe we should change to name of the function here; 
'ata_log_supported()' suggests a bool return.

ata_check_log_page() ?

>   static bool ata_identify_page_supported(struct ata_device *dev, u8 page)
> @@ -2455,15 +2455,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;
>   
>   	/*
>   	 * Read the concurrent positioning ranges log (0x47). We can have at
> -	 * most 255 32B range descriptors plus a 64B header.
> +	 * most 255 32B range descriptors plus a 64B header. This log varies in
> +	 * size, so use the size reported in the GPL directory. Reading beyond
> +	 * the supported length will result in an error.
>   	 */
> -	buf_len = (64 + 255 * 32 + 511) & ~511;
> +	buf_len <<= 9;
>   	buf = kzalloc(buf_len, GFP_KERNEL);
>   	if (!buf)
>   		goto out;

I don't get it.
You just returned the actual length of the log page from the previous 
function. Why do you need to calculate the length here?

Cheers,

Hannes
Damien Le Moal June 3, 2022, 7:07 a.m. UTC | #2
On 6/3/22 15:17, Hannes Reinecke wrote:
> On 6/3/22 00:51, Tyler Erickson wrote:
>> 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.
>>
>> This resolves this error from the dmesg output:
>>      ata6.00: Read log 0x47 page 0x00 failed, Emask 0x1
>>
>> Cc: stable@vger.kernel.org
>> Fixes: fe22e1c2f705 ("libata: support concurrent positioning ranges log")
>> Signed-off-by: Tyler Erickson <tyler.erickson@seagate.com>
>> Reviewed-by: Muhammad Ahmad <muhammad.ahmad@seagate.com>
>> Tested-by: Michael English <michael.english@seagate.com>
>> ---
>>   drivers/ata/libata-core.c | 21 +++++++++++++--------
>>   1 file changed, 13 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
>> index 40e816419f48..3ea10f72cb70 100644
>> --- a/drivers/ata/libata-core.c
>> +++ b/drivers/ata/libata-core.c
>> @@ -2010,16 +2010,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)
>>   {
>>   	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]);
>>   }
>>   
> Maybe we should change to name of the function here; 
> 'ata_log_supported()' suggests a bool return.
> 
> ata_check_log_page() ?
> 
>>   static bool ata_identify_page_supported(struct ata_device *dev, u8 page)
>> @@ -2455,15 +2455,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;
>>   
>>   	/*
>>   	 * Read the concurrent positioning ranges log (0x47). We can have at
>> -	 * most 255 32B range descriptors plus a 64B header.
>> +	 * most 255 32B range descriptors plus a 64B header. This log varies in
>> +	 * size, so use the size reported in the GPL directory. Reading beyond
>> +	 * the supported length will result in an error.
>>   	 */
>> -	buf_len = (64 + 255 * 32 + 511) & ~511;
>> +	buf_len <<= 9;
>>   	buf = kzalloc(buf_len, GFP_KERNEL);
>>   	if (!buf)
>>   		goto out;
> 
> I don't get it.
> You just returned the actual length of the log page from the previous 
> function. Why do you need to calculate the length here?

Calculate ? This is only converting from 512B sectors to bytes.
The calculation was mine, a gross error :) This is what this patch is fixing.

> 
> Cheers,
> 
> Hannes
Hannes Reinecke June 3, 2022, 7:42 a.m. UTC | #3
On 6/3/22 09:07, Damien Le Moal wrote:
> On 6/3/22 15:17, Hannes Reinecke wrote:
>> On 6/3/22 00:51, Tyler Erickson wrote:
>>> 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.
>>>
>>> This resolves this error from the dmesg output:
>>>       ata6.00: Read log 0x47 page 0x00 failed, Emask 0x1
>>>
>>> Cc: stable@vger.kernel.org
>>> Fixes: fe22e1c2f705 ("libata: support concurrent positioning ranges log")
>>> Signed-off-by: Tyler Erickson <tyler.erickson@seagate.com>
>>> Reviewed-by: Muhammad Ahmad <muhammad.ahmad@seagate.com>
>>> Tested-by: Michael English <michael.english@seagate.com>
>>> ---
>>>    drivers/ata/libata-core.c | 21 +++++++++++++--------
>>>    1 file changed, 13 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
>>> index 40e816419f48..3ea10f72cb70 100644
>>> --- a/drivers/ata/libata-core.c
>>> +++ b/drivers/ata/libata-core.c
>>> @@ -2010,16 +2010,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)
>>>    {
>>>    	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]);
>>>    }
>>>    
>> Maybe we should change to name of the function here;
>> 'ata_log_supported()' suggests a bool return.
>>
>> ata_check_log_page() ?
>>
>>>    static bool ata_identify_page_supported(struct ata_device *dev, u8 page)
>>> @@ -2455,15 +2455,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;
>>>    
>>>    	/*
>>>    	 * Read the concurrent positioning ranges log (0x47). We can have at
>>> -	 * most 255 32B range descriptors plus a 64B header.
>>> +	 * most 255 32B range descriptors plus a 64B header. This log varies in
>>> +	 * size, so use the size reported in the GPL directory. Reading beyond
>>> +	 * the supported length will result in an error.
>>>    	 */
>>> -	buf_len = (64 + 255 * 32 + 511) & ~511;
>>> +	buf_len <<= 9;
>>>    	buf = kzalloc(buf_len, GFP_KERNEL);
>>>    	if (!buf)
>>>    		goto out;
>>
>> I don't get it.
>> You just returned the actual length of the log page from the previous
>> function. Why do you need to calculate the length here?
> 
> Calculate ? This is only converting from 512B sectors to bytes.
> The calculation was mine, a gross error :) This is what this patch is fixing.
> 
Sigh. Can't we have a 'bytes_to_sectors' helper? All this shifting by 9 
is getting on my nerves ...

Cheers,

Hannes
Damien Le Moal June 3, 2022, 8:18 a.m. UTC | #4
On 6/3/22 16:42, Hannes Reinecke wrote:
> On 6/3/22 09:07, Damien Le Moal wrote:
>> On 6/3/22 15:17, Hannes Reinecke wrote:
>>> On 6/3/22 00:51, Tyler Erickson wrote:
>>>> 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.
>>>>
>>>> This resolves this error from the dmesg output:
>>>>       ata6.00: Read log 0x47 page 0x00 failed, Emask 0x1
>>>>
>>>> Cc: stable@vger.kernel.org
>>>> Fixes: fe22e1c2f705 ("libata: support concurrent positioning ranges log")
>>>> Signed-off-by: Tyler Erickson <tyler.erickson@seagate.com>
>>>> Reviewed-by: Muhammad Ahmad <muhammad.ahmad@seagate.com>
>>>> Tested-by: Michael English <michael.english@seagate.com>
>>>> ---
>>>>    drivers/ata/libata-core.c | 21 +++++++++++++--------
>>>>    1 file changed, 13 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
>>>> index 40e816419f48..3ea10f72cb70 100644
>>>> --- a/drivers/ata/libata-core.c
>>>> +++ b/drivers/ata/libata-core.c
>>>> @@ -2010,16 +2010,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)
>>>>    {
>>>>    	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]);
>>>>    }
>>>>    
>>> Maybe we should change to name of the function here;
>>> 'ata_log_supported()' suggests a bool return.
>>>
>>> ata_check_log_page() ?
>>>
>>>>    static bool ata_identify_page_supported(struct ata_device *dev, u8 page)
>>>> @@ -2455,15 +2455,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;
>>>>    
>>>>    	/*
>>>>    	 * Read the concurrent positioning ranges log (0x47). We can have at
>>>> -	 * most 255 32B range descriptors plus a 64B header.
>>>> +	 * most 255 32B range descriptors plus a 64B header. This log varies in
>>>> +	 * size, so use the size reported in the GPL directory. Reading beyond
>>>> +	 * the supported length will result in an error.
>>>>    	 */
>>>> -	buf_len = (64 + 255 * 32 + 511) & ~511;
>>>> +	buf_len <<= 9;
>>>>    	buf = kzalloc(buf_len, GFP_KERNEL);
>>>>    	if (!buf)
>>>>    		goto out;
>>>
>>> I don't get it.
>>> You just returned the actual length of the log page from the previous
>>> function. Why do you need to calculate the length here?
>>
>> Calculate ? This is only converting from 512B sectors to bytes.
>> The calculation was mine, a gross error :) This is what this patch is fixing.
>>
> Sigh. Can't we have a 'bytes_to_sectors' helper? All this shifting by 9 
> is getting on my nerves ...

Haha ! Yes, we can do that. But not in this patch since that is a bug fix.
diff mbox series

Patch

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 40e816419f48..3ea10f72cb70 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -2010,16 +2010,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)
 {
 	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)
@@ -2455,15 +2455,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;
 
 	/*
 	 * Read the concurrent positioning ranges log (0x47). We can have at
-	 * most 255 32B range descriptors plus a 64B header.
+	 * most 255 32B range descriptors plus a 64B header. This log varies in
+	 * size, so use the size reported in the GPL directory. Reading beyond
+	 * the supported length will result in an error.
 	 */
-	buf_len = (64 + 255 * 32 + 511) & ~511;
+	buf_len <<= 9;
 	buf = kzalloc(buf_len, GFP_KERNEL);
 	if (!buf)
 		goto out;