Re: [PATCH 2/2] ata: libata-eh: do not thaw the port twice in ata_eh_reset()

[Damien Le Moal <dlemoal@kernel.org>]

On 9/14/23 07:19, Niklas Cassel wrote:
> From: Niklas Cassel <niklas.cassel@wdc.com>
> 
> commit 1e641060c4b5 ("libata: clear eh_info on reset completion") added
> a workaround that broke the retry mechanism in ATA EH.
> 
> Tejun himself suggested to remove this workaround when it was identified
> to cause additional problems:
> https://lore.kernel.org/linux-ide/20110426135027.GI878@htj.dyndns.org/
> 
> He and even said:
> "Hmm... it seems I wasn't thinking straight when I added that work around."
> https://lore.kernel.org/linux-ide/20110426155229.GM878@htj.dyndns.org/
> 
> While removing the workaround solved the issue, however, the workaround was
> kept to avoid "spurious hotplug events during reset", and instead another
> workaround was added on top of the existing workaround in commit
> 8c56cacc724c ("libata: fix unexpectedly frozen port after ata_eh_reset()").
> 
> Because these IRQs happened when the port was frozen, we know that they
> were actually a side effect of PxIS and IS.IPS(x) not being cleared before
> the COMRESET. This is now done in commit 94152042eaa9 ("ata: libahci: clear
> pending interrupt status"), so these workarounds can now be removed.
> 
> Since commit 1e641060c4b5 ("libata: clear eh_info on reset completion") has
> now been reverted, the ATA EH retry mechanism is functional again, so there
> is once again no need to thaw the port more than once in ata_eh_reset().
> 
> This reverts "the workaround on top of the workaround" introduced in commit
> 8c56cacc724c ("libata: fix unexpectedly frozen port after ata_eh_reset()").
> 
> Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>

We need a fixes tag. Same for patch 1.

> ---
>  drivers/ata/libata-eh.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> index 5c493b6316eb..4cf4f57e57b8 100644
> --- a/drivers/ata/libata-eh.c
> +++ b/drivers/ata/libata-eh.c
> @@ -2803,9 +2803,6 @@ int ata_eh_reset(struct ata_link *link, int classify,
>  		slave->eh_info.serror = 0;
>  	spin_unlock_irqrestore(link->ap->lock, flags);
>  
> -	if (ata_port_is_frozen(ap))
> -		ata_eh_thaw_port(ap);
> -
>  	/*
>  	 * Make sure onlineness and classification result correspond.
>  	 * Hotplug could have happened during reset and some

-- 
Damien Le Moal
Western Digital Research