[4/4] PM / Domains: Let the ->attach_dev() callback return an error code

Message ID 1414507090-516-5-git-send-email-ulf.hansson@linaro.org
State New
Headers show

Commit Message

Ulf Hansson Oct. 28, 2014, 2:38 p.m.
Typically an ->attach_dev() callback would fetch some PM resourses.

Those operations, like for example clk_get() may fail with different
errors, including -EPROBE_DEFER. Instead of ignoring these errors and
potentially only print an error message, let's propagate them to give
callers the option to handle them.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/base/power/domain.c | 11 +++++++++--
 include/linux/pm_domain.h   |  2 +-
 2 files changed, 10 insertions(+), 3 deletions(-)

Comments

Geert Uytterhoeven Oct. 28, 2014, 8:31 p.m. | #1
Hi Ulf, Rafael,

On Tue, Oct 28, 2014 at 3:38 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> Typically an ->attach_dev() callback would fetch some PM resourses.
>
> Those operations, like for example clk_get() may fail with different
> errors, including -EPROBE_DEFER. Instead of ignoring these errors and
> potentially only print an error message, let's propagate them to give
> callers the option to handle them.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

Given that several patch series using ->attach_dev() are already floating
around and will be in -next soon, what is the plan of getting this in?
Doing it ASAP (in v3.18-rc3)?
Delaying this to v3.19-rc2, which will require an atomic fixing of its users?
Any other option?

Gr{oetje,eeting}s,

                        Geert, who's about to resubmit his pm-rmobile patches

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ulf Hansson Oct. 29, 2014, 9:26 a.m. | #2
On 28 October 2014 21:31, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> Hi Ulf, Rafael,
>
> On Tue, Oct 28, 2014 at 3:38 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> Typically an ->attach_dev() callback would fetch some PM resourses.
>>
>> Those operations, like for example clk_get() may fail with different
>> errors, including -EPROBE_DEFER. Instead of ignoring these errors and
>> potentially only print an error message, let's propagate them to give
>> callers the option to handle them.
>>
>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>
> Given that several patch series using ->attach_dev() are already floating
> around and will be in -next soon, what is the plan of getting this in?
> Doing it ASAP (in v3.18-rc3)?
> Delaying this to v3.19-rc2, which will require an atomic fixing of its users?
> Any other option?

I would prefer if we consider 3.18-rc[x|. That's applies also to the
below patchset, which actually fixes an issue. It would simplify the
process of handling other SOC specific patches which adds PM domain
support.

[PATCH v3 0/9] PM / Domains: Fix race conditions during boot

Obviously the patches needs to be reviewed, I guess we are still in
the process of doing that.

Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Geert Uytterhoeven Oct. 29, 2014, 9:32 a.m. | #3
Hi Ulf,

On Wed, Oct 29, 2014 at 10:26 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 28 October 2014 21:31, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>> On Tue, Oct 28, 2014 at 3:38 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>> Typically an ->attach_dev() callback would fetch some PM resourses.
>>>
>>> Those operations, like for example clk_get() may fail with different
>>> errors, including -EPROBE_DEFER. Instead of ignoring these errors and
>>> potentially only print an error message, let's propagate them to give
>>> callers the option to handle them.
>>>
>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>>
>> Given that several patch series using ->attach_dev() are already floating
>> around and will be in -next soon, what is the plan of getting this in?
>> Doing it ASAP (in v3.18-rc3)?
>> Delaying this to v3.19-rc2, which will require an atomic fixing of its users?
>> Any other option?
>
> I would prefer if we consider 3.18-rc[x|. That's applies also to the
> below patchset, which actually fixes an issue. It would simplify the
> process of handling other SOC specific patches which adds PM domain
> support.
>
> [PATCH v3 0/9] PM / Domains: Fix race conditions during boot

v3.18-rc[x] sounds fine.

> Obviously the patches needs to be reviewed, I guess we are still in
> the process of doing that.

Indeed.

Perhaps we can get just the prototype change of ->attach_dev() in first?
That leaves some time for reviewing the code changes to actually handle
the return value, and unblocks platform patches using ->attach_dev() soon,
which are planned to enter in v3.19-rc.

Thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ulf Hansson Oct. 29, 2014, 10:14 a.m. | #4
On 29 October 2014 10:32, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> Hi Ulf,
>
> On Wed, Oct 29, 2014 at 10:26 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> On 28 October 2014 21:31, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>> On Tue, Oct 28, 2014 at 3:38 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>>> Typically an ->attach_dev() callback would fetch some PM resourses.
>>>>
>>>> Those operations, like for example clk_get() may fail with different
>>>> errors, including -EPROBE_DEFER. Instead of ignoring these errors and
>>>> potentially only print an error message, let's propagate them to give
>>>> callers the option to handle them.
>>>>
>>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>>>
>>> Given that several patch series using ->attach_dev() are already floating
>>> around and will be in -next soon, what is the plan of getting this in?
>>> Doing it ASAP (in v3.18-rc3)?
>>> Delaying this to v3.19-rc2, which will require an atomic fixing of its users?
>>> Any other option?
>>
>> I would prefer if we consider 3.18-rc[x|. That's applies also to the
>> below patchset, which actually fixes an issue. It would simplify the
>> process of handling other SOC specific patches which adds PM domain
>> support.
>>
>> [PATCH v3 0/9] PM / Domains: Fix race conditions during boot
>
> v3.18-rc[x] sounds fine.
>
>> Obviously the patches needs to be reviewed, I guess we are still in
>> the process of doing that.
>
> Indeed.
>
> Perhaps we can get just the prototype change of ->attach_dev() in first?
> That leaves some time for reviewing the code changes to actually handle
> the return value, and unblocks platform patches using ->attach_dev() soon,
> which are planned to enter in v3.19-rc.

That's an option.

On the other hand, that would mean that the errors that the
attach_dev() callback would return from you SOC specific code, would
just be ignored until v3.19-rc. That's not so good, hiding errors. :-)

Let's see what Rafael thinks.

Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Geert Uytterhoeven Oct. 29, 2014, 10:28 a.m. | #5
Hi Ulf,

On Wed, Oct 29, 2014 at 11:14 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>>> Given that several patch series using ->attach_dev() are already floating
>>>> around and will be in -next soon, what is the plan of getting this in?
>>>> Doing it ASAP (in v3.18-rc3)?
>>>> Delaying this to v3.19-rc2, which will require an atomic fixing of its users?
>>>> Any other option?
>>>
>>> I would prefer if we consider 3.18-rc[x|. That's applies also to the
>>> below patchset, which actually fixes an issue. It would simplify the
>>> process of handling other SOC specific patches which adds PM domain
>>> support.
>>>
>>> [PATCH v3 0/9] PM / Domains: Fix race conditions during boot
>>
>> v3.18-rc[x] sounds fine.
>>
>>> Obviously the patches needs to be reviewed, I guess we are still in
>>> the process of doing that.
>>
>> Indeed.
>>
>> Perhaps we can get just the prototype change of ->attach_dev() in first?
>> That leaves some time for reviewing the code changes to actually handle
>> the return value, and unblocks platform patches using ->attach_dev() soon,
>> which are planned to enter in v3.19-rc.
>
> That's an option.
>
> On the other hand, that would mean that the errors that the
> attach_dev() callback would return from you SOC specific code, would
> just be ignored until v3.19-rc. That's not so good, hiding errors. :-)

The code to handle the return value could still get in in v3.18-rc.
So the errors would only be unhandled for a short while in -next.

> Let's see what Rafael thinks.

Right.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Kevin Hilman Oct. 29, 2014, 9:10 p.m. | #6
Geert Uytterhoeven <geert@linux-m68k.org> writes:

> Hi Ulf, Rafael,
>
> On Tue, Oct 28, 2014 at 3:38 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> Typically an ->attach_dev() callback would fetch some PM resourses.
>>
>> Those operations, like for example clk_get() may fail with different
>> errors, including -EPROBE_DEFER. Instead of ignoring these errors and
>> potentially only print an error message, let's propagate them to give
>> callers the option to handle them.
>>
>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>
> Given that several patch series using ->attach_dev() are already floating
> around and will be in -next soon, what is the plan of getting this in?

Shall we take this as a Reviewed-by or Acked-by for the series?  :)

> Doing it ASAP (in v3.18-rc3)?

IMO, this isn't at all appropriate for -rc since it's not fixing a
regression. Also, this series includes other cleanups that are not
really fixes either.  At this point of the -rc cycle, we need to focus
only on regression fixes.

> Delaying this to v3.19-rc2, which will require an atomic fixing of its users?
> Any other option?

I don't see any users of this in -next yet, so I think doing a simple
patch to the prototype and fixing up any users before they hit -next is
the right approach.  Errors will be ignored, but that's not change from
today. :)

Then the rest of this cleanup and behavior change stuff can continue to
be reviewed and get broader testing before merge.

Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Geert Uytterhoeven Oct. 29, 2014, 9:18 p.m. | #7
Hi Kevin,

On Wed, Oct 29, 2014 at 10:10 PM, Kevin Hilman <khilman@kernel.org> wrote:
> Geert Uytterhoeven <geert@linux-m68k.org> writes:
>> On Tue, Oct 28, 2014 at 3:38 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>> Typically an ->attach_dev() callback would fetch some PM resourses.
>>>
>>> Those operations, like for example clk_get() may fail with different
>>> errors, including -EPROBE_DEFER. Instead of ignoring these errors and
>>> potentially only print an error message, let's propagate them to give
>>> callers the option to handle them.
>>>
>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>>
>> Given that several patch series using ->attach_dev() are already floating
>> around and will be in -next soon, what is the plan of getting this in?
>
> Shall we take this as a Reviewed-by or Acked-by for the series?  :)

No, I still have to review/test this series.

Changing ->attach_dev() to return an error code again is fine for me.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ulf Hansson Oct. 30, 2014, 11:34 a.m. | #8
On 29 October 2014 22:10, Kevin Hilman <khilman@kernel.org> wrote:
> Geert Uytterhoeven <geert@linux-m68k.org> writes:
>
>> Hi Ulf, Rafael,
>>
>> On Tue, Oct 28, 2014 at 3:38 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>> Typically an ->attach_dev() callback would fetch some PM resourses.
>>>
>>> Those operations, like for example clk_get() may fail with different
>>> errors, including -EPROBE_DEFER. Instead of ignoring these errors and
>>> potentially only print an error message, let's propagate them to give
>>> callers the option to handle them.
>>>
>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>>
>> Given that several patch series using ->attach_dev() are already floating
>> around and will be in -next soon, what is the plan of getting this in?
>
> Shall we take this as a Reviewed-by or Acked-by for the series?  :)
>
>> Doing it ASAP (in v3.18-rc3)?
>
> IMO, this isn't at all appropriate for -rc since it's not fixing a
> regression. Also, this series includes other cleanups that are not
> really fixes either.  At this point of the -rc cycle, we need to focus
> only on regression fixes.
>
>> Delaying this to v3.19-rc2, which will require an atomic fixing of its users?
>> Any other option?
>
> I don't see any users of this in -next yet, so I think doing a simple
> patch to the prototype and fixing up any users before they hit -next is
> the right approach.  Errors will be ignored, but that's not change from
> today. :)
>
> Then the rest of this cleanup and behavior change stuff can continue to
> be reviewed and get broader testing before merge.

Okay, I will follow your suggestions and send a patch that only change
the prototype, intended as a fix for rc[n].

Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 4e5fcd7..da40769 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1394,12 +1394,19 @@  static int genpd_alloc_dev_data(struct generic_pm_domain *genpd,
 	if (ret)
 		goto err_data;
 
-	if (genpd->attach_dev)
-		genpd->attach_dev(dev);
+	if (genpd->attach_dev) {
+		ret = genpd->attach_dev(dev);
+		if (ret)
+			goto err_attach;
+	}
 
 	dev_pm_qos_add_notifier(dev, &gpd_data->nb);
 	return 0;
 
+ err_attach:
+	spin_lock_irq(&dev->power.lock);
+	dev->power.subsys_data->domain_data = NULL;
+	spin_unlock_irq(&dev->power.lock);
  err_data:
 	kfree(gpd_data);
  err_alloc:
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index e4edde1..70a3bc3 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -72,7 +72,7 @@  struct generic_pm_domain {
 	bool max_off_time_changed;
 	bool cached_power_down_ok;
 	struct gpd_cpuidle_data *cpuidle_data;
-	void (*attach_dev)(struct device *dev);
+	int (*attach_dev)(struct device *dev);
 	void (*detach_dev)(struct device *dev);
 };