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

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

On Thu, Sep 14, 2023 at 07:02:38PM +0900, Damien Le Moal wrote:
> On 9/14/23 18:06, Niklas Cassel wrote:
> > On Thu, Sep 14, 2023 at 08:51:06AM +0900, Damien Le Moal wrote:
> >> 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.
> > 
> > The workaround introduced in commit 1e641060c4b5 ("libata: clear eh_info on
> > reset completion") broke ATA EH retry logic, so the proper commit that we
> > fix is that commit.
> > 
> > However, if we put a Fixes tag with that commit, then this patch will get
> > backported to all possible stable kernels that has that commit, something
> > that we do _not_ want.
> > 
> > We can only remove this workaround for kernels that has commit 94152042eaa9
> > ("ata: libahci: clear pending interrupt status").
> 
> Squash the 2 fixes together in a single commit ?

We can do that, but the problem would be the same.

commit 94152042eaa9 ("ata: libahci: clear pending interrupt status") is
currently in your for-next branch. Patch 1 and patch 2 in this series
depend on this commit.

Both of these fixes (patch 1 and patch 2 in this series) fix issues caused
by commit 1e641060c4b5 ("libata: clear eh_info on reset completion"),
a 14 year old commit.

We could add a Fixes that on 1e641060c4b5 ("libata: clear eh_info on reset
completion").
But might get this patch to get backported to all old kernels.
We don't want that, as we depend on 94152042eaa9 ("ata: libahci: clear
pending interrupt status").


So... skip a Fixes tag or add a Fixes against on the commit that we depend
on? (Even tough we are not "fixing" that commit.)


Kind regards,
Niklas