Re: [PATCH 4/4] ata: libata-core: do not retry reading the log on timeout

[Damien Le Moal <damien.lemoal@opensource.wdc.com>]

On 10/8/22 08:47, Niklas Cassel wrote:
> On Sat, Oct 08, 2022 at 07:33:54AM +0900, Damien Le Moal wrote:
>> On 10/7/22 22:23, Niklas Cassel wrote:
>>> ata_read_log_page() first tries to read the log using READ LOG DMA EXT.
>>> If that fails it will instead try to read the log using READ LOG EXT.
>>>
>>> ata_exec_internal_sg() is synchronous, so it will wait for the command to
>>> finish. If we actually got an error back from the device, it is correct
>>> to retry. However, if the command timed out, ata_exec_internal_sg() will
>>> freeze the port.
>>>
>>> There is no point in retrying if the port is frozen, as
>>> ata_exec_internal_sg() will return AC_ERR_SYSTEM on a frozen port,
>>> without ever sending the command down to the drive.
>>>
>>> Therefore, avoid retrying if the first command froze the port, as that
>>> will result in a misleading AC_ERR_SYSTEM error print, instead of printing
>>> the error that actually caused the port to be frozen (AC_ERR_TIMEOUT).
>>
>> Beside ata_read_log_page(), are there any other path that do a retry after
>> ata_exec_internal_sg() fails ?
> 
> Let me check and get back to you.
> 
>>
>> Another note: this is patch 4/4 but I did not get any patch 3/4...
> 
> It is here:
> https://lore.kernel.org/linux-scsi/20221007132342.1590367-1-niklas.cassel@wdc.com/T/#t
> 
> My scripts probably didn't add you since you are not a maintainer or
> reviewer for drivers/scsi. I should probably have added you manually.

Yes please, always send full series.

> 
>>
>>>
>>> Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
>>> ---
>>>  drivers/ata/libata-core.c | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
>>> index 1cf326dd7c41..6ae5787103e7 100644
>>> --- a/drivers/ata/libata-core.c
>>> +++ b/drivers/ata/libata-core.c
>>> @@ -2000,7 +2000,8 @@ unsigned int ata_read_log_page(struct ata_device *dev, u8 log,
>>>  	if (err_mask) {
>>>  		if (dma) {
>>>  			dev->horkage |= ATA_HORKAGE_NO_DMA_LOG;
>>> -			goto retry;
>>> +			if (!ata_port_is_frozen(dev->link->ap))
>>> +				goto retry;
>>>  		}
>>>  		ata_dev_err(dev,
>>>  			    "Read log 0x%02x page 0x%02x failed, Emask 0x%x\n",
>>
>> -- 
>> Damien Le Moal
>> Western Digital Research

-- 
Damien Le Moal
Western Digital Research