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

[Niklas Cassel <Niklas.Cassel@wdc.com>]

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.

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