On 9/14/23 19:11, Niklas Cassel wrote:
> 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@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 ?
>
> 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.
That patch is in for next for testing only. I can remove it and change it.
>
> 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.)
I can add the 2 patches to for-6.6-fixes without fixes tag and just cc stable.
There are not that many LTS versions. Oldest still maintained is 4.14.
>
>
> Kind regards,
> Niklas
--
Damien Le Moal
Western Digital Research
