Re: [PATCH v7 3/7] of/platform: Add functional dependency link from DT bindings

From: Frank Rowand
Date: Mon Aug 19 2019 - 17:30:09 EST


On 8/19/19 1:49 PM, Saravana Kannan wrote:
> On Mon, Aug 19, 2019 at 10:16 AM Frank Rowand <frowand.list@xxxxxxxxx> wrote:
>>
>> On 8/15/19 6:50 PM, Saravana Kannan wrote:
>>> On Wed, Aug 7, 2019 at 7:06 PM Frank Rowand <frowand.list@xxxxxxxxx> wrote:
>>>>
>>>> On 7/23/19 5:10 PM, Saravana Kannan wrote:
>>>>> Add device-links after the devices are created (but before they are
>>>>> probed) by looking at common DT bindings like clocks and
>>>>> interconnects.
>>
>>
>> < very big snip (lots of comments that deserve answers) >
>>
>>
>>>>
>>>> /**
>>>> * of_link_property - TODO:
>>>> * dev:
>>>> * con_np:
>>>> * prop:
>>>> *
>>>> * TODO...
>>>> *
>>>> * Any failed attempt to create a link will NOT result in an immediate return.
>>>> * of_link_property() must create all possible links even when one of more
>>>> * attempts to create a link fail.
>>>>
>>>> Why? isn't one failure enough to prevent probing this device?
>>>> Continuing to scan just results in extra work... which will be
>>>> repeated every time device_link_check_waiting_consumers() is called
>>>
>>> Context:
>>> As I said in the cover letter, avoiding unnecessary probes is just one
>>> of the reasons for this patch. The other (arguably more important)
>>
>> Agree that it is more important.
>>
>>
>>> reason for this patch is to make sure suppliers know that they have
>>> consumers that are yet to be probed. That way, suppliers can leave
>>> their resource on AND in the right state if they were left on by the
>>> bootloader. For example, if a clock was left on and at 200 MHz, the
>>> clock provider needs to keep that clock ON and at 200 MHz till all the
>>> consumers are probed.
>>>
>>> Answer: Let's say a consumer device Z has suppliers A, B and C. If the
>>> linking fails at A and you return immediately, then B and C could
>>> probe and then figure that they have no more consumers (they don't see
>>> a link to Z) and turn off their resources. And Z could fail
>>> catastrophically.
>>
>> Then I think that this approach is fatally flawed in the current implementation.
>
> I'm waiting to hear how it is fatally flawed. But maybe this is just a
> misunderstanding of the problem?

Fatally flawed because it does not handle modules that add a consumer
device when the module is loaded.


>
> In the text below, I'm not sure if you mixing up two different things
> or just that your wording it a bit ambiguous. So pardon my nitpick to
> err on the side of clarity.

Please do nitpick. Clarity is good.


>
>> A device can be added by a module that is loaded.
>
> No, in the example I gave, of_platform_default_populate_init() would
> add all 3 of those devices during arch_initcall_sync().

The example you gave does not cover all use cases.

There are modules that add devices when the module is loaded. You can not
ignore systems using such modules.


>
>> In that case the device
>> was not present at late boot when the suppliers may turn off their resources.
>
> In that case, the _drivers_ for those devices aren't present at late
> boot. So that they can't request to keep the resources on for their
> consumer devices. Since there are no consumer requests on resources,
> the suppliers turn off their resources at late boot (since there isn't
> a better location as of today). The sync_state() call back added in a
> subsequent patche in this series will provide the better location.

And the sync_state() call back will not deal with modules that add consumer
devices when the module is loaded, correct?


>
>> (I am assuming the details since I have not reviewed the patches later in
>> the series that implement this part.)
>>
>> Am I missing something?
>
> I think you are mixing up devices getting added/populated with drivers
> getting loaded as modules?

Only some modules add devices when they are loaded. But these modules do
exist.

-Frank

>
>> If I am wrong, then I'll have more comments for your review replies for
>> patches 2 and 3.
>
> I'll wait for more review replies?
>
> Thanks,
> Saravana
>