Message-ID: <CAPDyKFpc8GRKK8bafMU_HPaqWNrger3grPSd5rWWCTYhUGHZLA@mail.gmail.com>
Date:   Thu, 15 Jun 2017 10:11:45 +0200
From:   Ulf Hansson <ulf.hansson@...aro.org>
To:     Peter Chen <hzpeterchen@...il.com>
Cc:     Peter Chen <peter.chen@....com>,
        Mark Rutland <mark.rutland@....com>,
        Heiko Stuebner <heiko@...ech.de>,
        Stephen Boyd <stephen.boyd@...aro.org>, frank.li@....com,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Gary Bisson <gary.bisson@...ndarydevices.com>,
        Fabio Estevam <festevam@...il.com>,
        Joshua Clayton <stillcompiling@...il.com>,
        Arnd Bergmann <arnd@...db.de>,
        Dmitry Eremin-Solenikov <dbaryshkov@...il.com>,
        Vaibhav Hiremath <vaibhav.hiremath@...aro.org>,
        Krzysztof Kozlowski <krzk@...nel.org>, mka@...omium.org,
        Alan Stern <stern@...land.harvard.edu>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "Maciej S. Szmigiero" <mail@...iej.szmigiero.name>,
        Pawel Moll <pawel.moll@....com>,
        "linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>,
        Sascha Hauer <s.hauer@...gutronix.de>,
        troy.kisky@...ndarydevices.com, Rob Herring <robh+dt@...nel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>, hverkuil@...all.nl,
        oscar@...andei.net,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Linux USB List <linux-usb@...r.kernel.org>,
        "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Sebastian Reichel <sre@...nel.org>,
        Mark Brown <broonie@...nel.org>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        Shawn Guo <shawnguo@...nel.org>, jun.li@....com
Subject: Re: [PATCH v15 2/7] power: add power sequence library

On 15 June 2017 at 08:58, Peter Chen <hzpeterchen@...il.com> wrote:
> On Wed, Jun 14, 2017 at 10:53:29AM +0200, Ulf Hansson wrote:
>> On 14 June 2017 at 03:53, Peter Chen <hzpeterchen@...il.com> wrote:
>> > On Tue, Jun 13, 2017 at 12:24:42PM +0200, Ulf Hansson wrote:
>> >> [...]
>> >>
>> >> > +
>> >> > +/**
>> >> > + * of_pwrseq_on - Carry out power sequence on for device node
>> >> > + *
>> >> > + * @np: the device node would like to power on
>> >> > + *
>> >> > + * Carry out a single device power on.  If multiple devices
>> >> > + * need to be handled, use of_pwrseq_on_list() instead.
>> >> > + *
>> >> > + * Return a pointer to the power sequence instance on success,
>> >> > + * or an error code otherwise.
>> >> > + */
>> >> > +struct pwrseq *of_pwrseq_on(struct device_node *np)
>> >> > +{
>> >> > +       struct pwrseq *pwrseq;
>> >> > +       int ret;
>> >> > +
>> >> > +       pwrseq = pwrseq_find_available_instance(np);
>> >> > +       if (!pwrseq)
>> >> > +               return ERR_PTR(-ENOENT);
>> >>
>> >> In case the pwrseq instance hasn't been registered yet, then there is
>> >> no way to deal with -EPROBE_DEFER properly here.
>> >>
>> >> I haven't been following the discussions in-depth during all
>> >> iterations, so perhaps you have already discussed why doing it like
>> >> this.
>> >
>> > Yes, it has been discussed. In order to compare with compatible string
>> > at dts, we need to have one registered pwrseq instance for each
>> > pwrseq library, this pre-registered one is allocated using
>> > postcore_initcall, and the new (eg, second) instance is registered
>> > after pwrseq_get has succeeded.
>>
>> I understand you need one compatible per pwrseq library, but how does
>> that have anything to do with -EPROBE_DEFER?
>>
>> My point is that, if a driver calls of_pwrseq_on() (which calls
>> pwrseq_find_available_instance()), but the corresponding pwrseq
>> library and instance has not yet been registered for that device. Then
>> how will you handle -EPROBE_DEFER? I guess you simply can't, which is
>> why *all* pwrseq libraries needs to be registered in early boot phase,
>> like at postcore_initcall(). Right?
>>
>> If that is the case, I really don't like it.
>>
>
> Yes, you are right. This is the limitation for this power sequence
> library, the registration for the 1st power sequence instance must
> be finished before device driver uses it. I am appreciated that
> you can supply some suggestions for it.

In general this kind of problems is solved by first parsing the DTB,
which means you will find out whether there is a resource (a pwrseq)
required for the device. Then you try to fetch that resource, and if
that fails, it means the resource is not yet available, and hence you
want to retry later and should return -EPROBE_DEFER.

In this case, of_pwrseq_on() needs to be converted to start looking
for a pwrseq compatible in it's child node - I guess. Then if that is
found, you try to fetch the instance of the corresponding library.
Failing to fetch the library instance should then cause a return
-EPROBE_DEFER.

>
>> Moreover, I have found yet another severe problem but reviewing the code:
>> In the struct pwrseq, you have a "bool used", which you are setting to
>> "true" once the pwrseq has been hooked up with the device, when a
>> driver calls of_pwrseq_on(). Setting that variable to true, will also
>> prevent another driver from using the same instance of the pwrseq for
>> its device. So, to cope with multiple users, you register a new
>> instance of the same pwrseq library that got hooked up, once the
>> ->get() callback is about to complete.
>>
>> The problem the occurs, when there is another driver calling
>> of_pwrseq_on() in between, meaning that the new instance has not yet
>> been registered. This will simply fail, won't it?
>
> Yes, you are right, thanks for pointing that, I will add mutex_lock for
> of_pwrseq_on.

Another option is to entirely skip to two step approach.

In other words, make the library to cope with multiple users via the
same registered library instance.

[...]

Kind regards
Uffe

Powered by blists - more mailing lists