Message-ID: <35f3901c-45cf-dacc-c25e-67c2b9ca61f0@molgen.mpg.de>
Date:   Tue, 31 May 2022 18:21:38 +0200
From:   Paul Menzel <pmenzel@...gen.mpg.de>
To:     Damien Le Moal <damien.lemoal@...nsource.wdc.com>
Cc:     Mario Limonciello <Mario.Limonciello@....com>,
        Hans de Goede <hdegoede@...hat.com>,
        linux-ide@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>,
        Nehal-bakulchandra Shah <Nehal-bakulchandra.Shah@....com>
Subject: Re: [PATCH v2 3/3] ata: ahci: Skip 200 ms debounce delay for AMD 300
 Series Chipset SATA Controller

[Cc: -Nehal-bakulchandra (undeliverable)

Am 31.05.22 um 18:18 schrieb Paul Menzel:
> Dear Damien,
> 
> 
> Am 01.04.22 um 09:23 schrieb Damien Le Moal:
>> On 4/1/22 14:18, Paul Menzel wrote:
> 
> […]
> 
>>> Am 01.04.22 um 01:04 schrieb Damien Le Moal:
>>>> On 3/31/22 23:42, Paul Menzel wrote:
>>>
>>>>> Am 23.03.22 um 09:36 schrieb Paul Menzel:
>>>>>
>>>>>> Am 23.03.22 um 09:24 schrieb Damien Le Moal:
>>>>>>> On 3/23/22 15:55, Paul Menzel wrote:
>>>>>>
>>>>>>>> Am 23.03.22 um 06:01 schrieb Damien Le Moal:
>>>>>>>>> On 3/22/22 06:51, Limonciello, Mario wrote:
>>>>>>
>>>>>>>>>>> -----Original Message-----
>>>>>>>>>>> From: Paul Menzel <pmenzel@...gen.mpg.de>
>>>>>>>>>>> Sent: Monday, March 21, 2022 16:25
>>>>>>>>
>>>>>>>> […]
>>>>>>>>
>>>>>>>>>> I seem to recall that we were talking about trying to drop the
>>>>>>>>>> debounce delay for everything, weren't we?
>>>>>>>>>>
>>>>>>>>>> So perhaps it would be right to add a 4th patch in the series 
>>>>>>>>>> to do
>>>>>>>>>> just that.  Then If this turns out to be problematic for
>>>>>>>>>> anything other than the controllers in the series that you
>>>>>>>>>> identified as not problematic then that 4th patch can
>>>>>>>>>> potentially be reverted alone?
>>>>>>>>>
>>>>>>>>> Not quite everything :) But you are right, let's try to switch the
>>>>>>>>> default to no delay. I will be posting patches today for that.
>>>>>>>>> With these patches, your patches are not necessary anymore as the AMD
>>>>>>>>> chipset falls under the default no-delay.
>>>>>>>>
>>>>>>>> I am all for improving the situation for all devices, but I am unable to
>>>>>>>> judge the regression potential of changing this, as it affects a lot of
>>>>>>>> devices. I guess it’d would go through the next tree, and hopefully the
>>>>>>>> company QA teams can give it a good spin. I hoped that my patches, as I
>>>>>>>> have tested them, and AMD will hopefully too, could go into the current
>>>>>>>> merge window.
>>>>>>>
>>>>>>> Yes, correct, the plan is to get the generic series queued as soon
>>>>>>> as rc1 so that it can spend plenty of time in linux-next for people
>>>>>>> to test. That will hopefully reduce the risk of breaking things in
>>>>>>> the field. Same for  the default LPM change.
>>>>>>
>>>>>> But 5.18 or 5.19? If 5.18, sounds good to me, if 5.19, I’d be great if
>>>>>> my patches go into 5.18 cycle, as they have been tested, and it would
>>>>>> mean the whole change gets tested more widely already.
>>>>>>
>>>>>>> With the default removal of the debounce delay, your patches addressing
>>>>>>> only the AMD adapter are not needed anymore: this adapter will not have a
>>>>>>> debounce delay unless the ATA_LFLAG_DEBOUNCE_DELAY flag is set.
>>>>>>
>>>>>> Yes, I understand.
>>>>>
>>>>> The merge window for Linux 5.18 is going to close in three days this
>>>>> Sunday. It’d be really great if my patches, tested on hardware, 
>>>>> could go into that.
>>>>>
>>>>>>>>> It would be nice if you can test though.
>>>>>>>>
>>>>>>>> Of course, I am going to that either way.
>>>>>>>
>>>>>>> Series posted with you on CC. Please test !
>>>>>>
>>>>>> Thank you. I am going to test it in the coming days, and report back.
>>>>>>
>>>>>> Maybe more people should be put in Cc (Dell, Lenovo, IBM, x86 
>>>>>> subsystem) with a request to test this?
>>>>> Thank you for the patches, which are a big improvement. Let’s hope, you
>>>>> can re-roll them, so they get into Linux very soon for everyone’s 
>>>>> benefit.
>>>>
>>>> I am waiting for 5.18-rc1 to rebase the patches and re-post them. Given
>>>> reviewed-by and tested-by tags, I will queue them for 5.19.
>>>
>>> As discussed in the other thread, it’s impossible to be 100 % certain,
>>> it won’t break anything.
>>
>> Yes, that is why I want to push the patches early in the cycle to be able
>> to revert if too many problems are reported.
>>
>>>> With that in mind, I am not planning to apply your previous patches
>>>> for 5.18, as they would conflict and would only end up being churn
>>>> since the delay removal by default will undo your changes.
>>> Obviously, I do not agree, as this would give the a little bit more
>>> testing already, if changing the default is a good idea. Also, if the
>>> conflict will be hard to resolve, I happily do it (the patches could
>>> even be reverted on top – git commits are cheap and easy to handle).
>>
>> The conflict is not hard to resolve. The point is that my patches changing
>> the default to no debounce delay completely remove the changes of your
>> patch to do the same for one or some adapters. So adding your patches now
>> and then my patches on top does not make much sense at all.
>>
>> If too many problems show up and I end up reverting/removing the patches,
>> then I will be happy to take your patches for the adapter you tested. Note
>> that *all* the machines I have tested so far are OK without a debounce
>> delay too. So we could add them too... And endup with a long list of
>> adapters that use the default ahci driver without debounce delay. The goal
>> of changing the default to no delay is to avoid that. So far, the adapters
>> I have identified that need the delay have their own declaration, so we
>> only need to add a flag there. Simpler change that listing up adapters
>> that are OK without the delay.
>>
>>> Anyway, I wrote my piece, but you are the maintainer, so it’s your call
>>> and I stop bothering you.
> 
> I just wanted to inquire about the status of your changes? I do not find 
> them in your `for-5.19` branch. As they should be tested in linux-next 
> before the merge window opens, if these are not ready yet, could you 
> please apply my (tested) patches?
> 
> 
> Kind regards,
> 
> Paul

Powered by blists - more mailing lists