Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <eae28066-9b70-bb7c-91e5-ba9fbf97672e@redhat.com>
Date:   Thu, 17 Nov 2022 20:16:01 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.3.1
Subject: Re: [PATCH v2 3/4] driver core: Add fw_devlink.timeout param to stop
 waiting for devlinks
Content-Language: en-US
To:     John Stultz <jstultz@google.com>
Cc:     linux-kernel@vger.kernel.org,
        "Rafael J . Wysocki" <rafael@kernel.org>,
        Douglas Anderson <dianders@chromium.org>,
        Saravana Kannan <saravanak@google.com>,
        Daniel Vetter <daniel.vetter@ffwll.ch>,
        linux-arm-msm@vger.kernel.org,
        Peter Robinson <pbrobinson@redhat.com>,
        Steev Klimaszewski <steev@kali.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Enric Balletbo i Serra <eballetbo@redhat.com>,
        Bjorn Andersson <andersson@kernel.org>,
        Brian Masney <bmasney@redhat.com>,
        Rob Herring <robh@kernel.org>
References: <20221116115348.517599-1-javierm@redhat.com>
 <20221116120159.519908-1-javierm@redhat.com>
 <CANDhNCr7ZwbCDK1ftigLK_S2qASj1yfenUG1WPaiYbjr5M9x3w@mail.gmail.com>
From:   Javier Martinez Canillas <javierm@redhat.com>
In-Reply-To: <CANDhNCr7ZwbCDK1ftigLK_S2qASj1yfenUG1WPaiYbjr5M9x3w@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hello John,

On 11/17/22 20:07, John Stultz wrote:
> On Wed, Nov 16, 2022 at 4:02 AM Javier Martinez Canillas
> <javierm@redhat.com> wrote:
>>
>> Currently, the probe deferral timeout does two things:
>>
>> 1) Call to fw_devlink_drivers_done() to relax the device dependencies and
>>    allow drivers to be probed if these dependencies are optional.
>>
>> 2) Disable the probe deferral mechanism so that drivers will fail to probe
>>    if the required dependencies are not present, instead of adding them to
>>    the deferred probe pending list.
>>
>> But there is no need to couple these two, for example the probe deferral
>> can be used even when the device links are disable (i.e: fw_devlink=off).
>>
>> So let's add a separate fw_devlink.timeout command line parameter to allow
>> relaxing the device links and prevent drivers to wait for these to probe.
> 
> I'm probably being dim, but it's not immediately clear from this
> description *why* this is useful. Maybe add some words on the tangible
> benefit of splitting this up?
>

Thanks for your feedback. You are right that I need to better explain
the why / goal of this patch. But basically is that it would be good
to allow timeout waiting for the optional links while still allow the
non-optional links to keep make the drivers to keep deferring.

I can make a better job at explaining the why in the next iteration.
 
> I'd also push a little bit back on why we need to split this into a
> separate boot option. Since it's not obvious as to when a user would
> want to use fw_devlink.timeout vs probe_deferral_timeout.
> The extra complexity of remembering which timeout is for what might
> become a burden to users and developers.
> 
>>
>> +       fw_devlink.timeout=
>> +                       [KNL] Debugging option to set a timeout in seconds for
>> +                       drivers to give up waiting on dependencies and to probe
>> +                       these are optional. A timeout of 0 will timeout at the
>> +                       end of initcalls. If the time out hasn't expired, it'll
>> +                       be restarted by each successful driver registration.
>> +
> 
> This sounds pretty close to like the deferred_probe_timeout option.
> I'd suggest some words to make the distinction more clear.
>

Yeah, I can think how to better explain this... but it's similar because
there is some overlapping between the two really, but are not exactly the
same even though we are tying the two and folding the disable of the two
under the same timeout.
 
I'll be OK to drop this parameter btw, and just keep it as an internal var
if it's fine to just always use the default 10 seconds or whatever timeout
it is decided.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat