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@xxxxxxx>
>>>
>>> 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@xxxxxxxxxxxxxx/
>>>
>>> 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@xxxxxxxxxxxxxx/
>>>
>>> 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@xxxxxxx>
>>
>> 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 ?
>
> Do we really need a Fixes tag?
> The workaround (which broke ATA EH retry logic) has been in the kernel for
> 14 years, since then, we've only seen two complaints..
> the one by Bruce Stenning 12 years ago (see commit log for this patch),
> and the complaint from Huawei folks this year..
>
> I guess we could set the Fixes tag to 94152042eaa9 ("ata: libahci: clear
> pending interrupt status"), since we depend on that commit.
> However, that is basically a lie, since we are not fixing that commit.
>
>
> Kind regards,
> Niklas
--
Damien Le Moal
Western Digital Research
