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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxxxxx
>>>> Fixes: fe22e1c2f705 ("libata: support concurrent positioning ranges log")
>>>> Signed-off-by: Tyler Erickson <tyler.erickson@xxxxxxxxxxx>
>>>> Reviewed-by: Muhammad Ahmad <muhammad.ahmad@xxxxxxxxxxx>
>>>> Tested-by: Michael English <michael.english@xxxxxxxxxxx>
>>>> ---
>>>>    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.


-- 
Damien Le Moal
Western Digital Research



[Index of Archives]     [Linux Filesystems]     [Linux SCSI]     [Linux RAID]     [Git]     [Kernel Newbies]     [Linux Newbie]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Samba]     [Device Mapper]

  Powered by Linux