[v4, 1/9] ACPI / PM: Let acpi_dev_pm_detach() return an error code

[Ulf Hansson]

On 18 September 2014 01:43, Dmitry Torokhov <dmitry.torokhov at gmail.com> wrote:
> On Thu, Sep 18, 2014 at 01:20:49AM +0200, Ulf Hansson wrote:
>> On 17 September 2014 22:10, Dmitry Torokhov <dmitry.torokhov at gmail.com> wrote:
>> > On Wed, Sep 17, 2014 at 08:25:44PM +0200, Ulf Hansson wrote:
>> >> On 16 September 2014 01:36, Rafael J. Wysocki <rjw at rjwysocki.net> wrote:
>> >> > On Monday, September 15, 2014 09:53:59 AM Dmitry Torokhov wrote:
>> >> >> On Sun, Sep 14, 2014 at 06:38:58PM +0200, Rafael J. Wysocki wrote:
>> >> >> > On Friday, September 12, 2014 02:05:53 PM Dmitry Torokhov wrote:
>> >> >> > > Hi Ulf,
>> >> >> > >
>> >> >> > > On Tue, Sep 09, 2014 at 01:36:02PM +0200, Ulf Hansson wrote:
>> >> >> > > > To give callers the option of acting on a errors while removing the
>> >> >> > > > pm_domain ops for the device in the ACPI PM domain, let
>> >> >> > > > acpi_dev_pm_detach() return an int to provide the error code.
>> >> >> > >
>> >> >> > > So how would callers handle the errors? As far as I can see
>> >> >> > > acpi_dev_pm_detach() is called from ->remove() and ->shutdown() methods, where
>> >> >> > > there is no meaningful strategy to handle errors as you are past the point of
>> >> >> > > no return and you keep on tearing down the device.
>> >>
>> >> The benefit is only relevant when ACPI and genpd PM domains would
>> >> co-exist. In that case we might be able to skip genpd_dev_pm_detach()
>> >> if acpi_dev_pm_detach() succeeds. So, currently there are  no benefit,
>> >> but still it doesn't hurt.
>> >
>> > It doe snot have any negative material effect, the drawback is purely
>> > from API perspective.
>> >
>> >>
>> >> >> >
>> >> >> > This is specifically for what patch [3/9] is doing AFAICS.
>> >> >> >
>> >> >> > The existing callers don't need to worry about this.
>> >> >>
>> >> >> OK, so I have the very same comment about patch 3 then: we have
>> >> >> dev_pm_domain_detach() returning error. How would the callers handle errors?
>> >> >
>> >> > Ulf?
>> >>
>> >> I see your point. How about making dev_pm_domain_detach() to be a void
>> >> function instead?
>> >
>> > Yes, please.
>>
>> OK!
>>
>> >
>> >>
>> >> >
>> >> >> WRT this patch: I'd rater we did not just return generic "error code" just
>> >> >> because we do not know who manages PD for the device. Can we add API to check
>> >> >> if we are using ACPI to manage power domains? Then patch #3 could check if it
>> >> >> needs to use ACPI or generic power domain API.
>> >>
>> >> The problem is scalability. If we have other PM domains implementation
>> >> in future, each of them need to be checked prior invoking the attach
>> >> functions.
>> >> Also, how would we distinguish between genpd and a new PM domain XYZ?
>> >
>> > I do not think that trying all available methods to detach a pm domain,
>> > i.e.
>> >
>> >         err = acpi_dev_pm_detach();
>> >         if (err)
>> >                 err = blah_dev_pm_detach();
>> >         if (err)
>> >                 err = flab_dev_pm_detach();
>> >         if (err)
>> >                 err = gen_dev_pm_detach();
>> >
>> > is any better from scalability point of view. If you need to do that you
>> > will probably have to store something like "struct pd_ops *pd_ops" in
>> > your device and call appropriate implementation via it.
>>
>> No, that's not needed. Go ahead and have look at both ACPI and genpd,
>> the interesting part is the validation of struct dev_pm_domain pointer
>> in the struct device. That's all there is to it, no additional data
>> are required.
>
> OK, so can you simply put the needed method into struct dev_pm_domain and then
> dev_pm_domain_detach() would become:
>
> void dev_pm_domain_detach(struct device *dev, bool power_off)
> {
>         if (dev->pm_domain)
>                 dev->pm_domain->detach(dev, power_off);
> }
>
> Thanks.

Ohh, didn't quite follow that this is what you meant. That would be a
great improvement, I will adopt in the next version.

Kind regards
Uffe

>
> --
> Dmitry