linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Saravana Kannan <saravanak@google.com>
To: Frank Rowand <frowand.list@gmail.com>
Cc: Rob Herring <robh+dt@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	"Rafael J. Wysocki" <rafael@kernel.org>,
	Jonathan Corbet <corbet@lwn.net>,
	"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
	<devicetree@vger.kernel.org>, LKML <linux-kernel@vger.kernel.org>,
	David Collins <collinsd@codeaurora.org>,
	Android Kernel Team <kernel-team@android.com>,
	Linux Doc Mailing List <linux-doc@vger.kernel.org>
Subject: Re: [PATCH v7 3/7] of/platform: Add functional dependency link from DT bindings
Date: Mon, 19 Aug 2019 13:49:09 -0700	[thread overview]
Message-ID: <CAGETcx-XcXZq7YFHsFdzBDniQku9cxFUJL_vBoEKKhCH+cDKRw@mail.gmail.com> (raw)
In-Reply-To: <19c99a6e-51c3-68d7-d1d6-640aae754c14@gmail.com>

On Mon, Aug 19, 2019 at 10:16 AM Frank Rowand <frowand.list@gmail.com> wrote:
>
> On 8/15/19 6:50 PM, Saravana Kannan wrote:
> > On Wed, Aug 7, 2019 at 7:06 PM Frank Rowand <frowand.list@gmail.com> 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?

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.

> 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().

>  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.

> (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?

> 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

  reply	other threads:[~2019-08-19 20:49 UTC|newest]

Thread overview: 36+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-07-24  0:10 [PATCH v7 0/7] Solve postboot supplier cleanup and optimize probe ordering Saravana Kannan
2019-07-24  0:10 ` [PATCH v7 1/7] driver core: Add support for linking devices during device addition Saravana Kannan
2019-08-08  2:04   ` Frank Rowand
2019-08-16  1:50     ` Saravana Kannan
2019-08-19  3:38       ` Frank Rowand
2019-08-20  0:00         ` Saravana Kannan
2019-08-20  4:25           ` Frank Rowand
2019-08-20 22:10             ` Saravana Kannan
2019-08-21  1:06               ` Frank Rowand
2019-08-21  1:56                 ` Greg Kroah-Hartman
2019-08-21  2:04                   ` Saravana Kannan
     [not found]                   ` <CAGETcx8eVfJ0mn08E6pTDytjVE+RFcj_gOt_enide4E6G1NAWw@mail.gmail.com>
2019-08-21  4:24                     ` Frank Rowand
2019-08-21  2:22                 ` Saravana Kannan
2019-08-21 15:36               ` Frank Rowand
2019-07-24  0:10 ` [PATCH v7 2/7] driver core: Add edit_links() callback for drivers Saravana Kannan
2019-08-08  2:05   ` Frank Rowand
2019-08-16  1:50     ` Saravana Kannan
2019-07-24  0:10 ` [PATCH v7 3/7] of/platform: Add functional dependency link from DT bindings Saravana Kannan
2019-08-08  2:06   ` Frank Rowand
2019-08-16  1:50     ` Saravana Kannan
2019-08-19 17:16       ` Frank Rowand
2019-08-19 20:49         ` Saravana Kannan [this message]
2019-08-19 21:30           ` Frank Rowand
2019-08-20  0:09             ` Saravana Kannan
2019-08-20  4:26               ` Frank Rowand
2019-08-20 22:09                 ` Saravana Kannan
2019-08-21 16:39                   ` Frank Rowand
2019-07-24  0:10 ` [PATCH v7 4/7] driver core: Add sync_state driver/bus callback Saravana Kannan
2019-07-24  0:10 ` [PATCH v7 5/7] of/platform: Pause/resume sync state during init and of_platform_populate() Saravana Kannan
2019-07-24  0:10 ` [PATCH v7 6/7] of/platform: Create device links for all child-supplier depencencies Saravana Kannan
2019-07-24  0:11 ` [PATCH v7 7/7] of/platform: Don't create device links for default busses Saravana Kannan
2019-07-25 13:42 ` [PATCH v7 0/7] Solve postboot supplier cleanup and optimize probe ordering Greg Kroah-Hartman
2019-07-25 21:04   ` Frank Rowand
2019-07-26 14:32     ` Greg Kroah-Hartman
2019-07-31  2:22       ` Frank Rowand
2019-08-08  2:02 ` Frank Rowand

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAGETcx-XcXZq7YFHsFdzBDniQku9cxFUJL_vBoEKKhCH+cDKRw@mail.gmail.com \
    --to=saravanak@google.com \
    --cc=collinsd@codeaurora.org \
    --cc=corbet@lwn.net \
    --cc=devicetree@vger.kernel.org \
    --cc=frowand.list@gmail.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=kernel-team@android.com \
    --cc=linux-doc@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=rafael@kernel.org \
    --cc=robh+dt@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).