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 >
