[PATCH 0/4] PM / Domains: Handle errors from ->attach_dev() callback

[PATCH 0/4] PM / Domains: Handle errors from ->attach_dev() callback

[Ulf Hansson]

This patchset enabled the recently added ->attach_dev() callback to return an
error code.

Typically these callbacks will fetch some PM resourses, like clocks through a
clk_get() which may fail. To be able to handle the errors we need to return an
error code.

Do note, the buses which invokes dev_pm_domain_attach() are already handling
-EPROBE_DEFER, which means the ->attach_dev() callbacks may also return this
if needed.


Ulf Hansson (4):
  PM / Domains: Remove reference counting for the generic_pm_domain_data
  PM / Domains: Don't allow an existing generic PM domain data
  PM / Domains: Improve error handling while adding/removing devices
  PM / Domains: Let the ->attach_dev() callback return an error code

 drivers/base/power/domain.c | 143 +++++++++++++++++++++++---------------------
 include/linux/pm_domain.h   |   3 +-
 2 files changed, 77 insertions(+), 69 deletions(-)

-- 
1.9.1

[PATCH 1/4] PM / Domains: Remove reference counting for the generic_pm_domain_data

[Ulf Hansson]

The reference counting isn't needed, let's remove it.

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

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 40bc2f4..7546242 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1430,7 +1430,6 @@ int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
 		gpd_data = gpd_data_new;
 		dev->power.subsys_data->domain_data = &gpd_data->base;
 	}
-	gpd_data->refcount++;
 	if (td)
 		gpd_data->td = *td;
 
@@ -1478,7 +1477,6 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd,
 {
 	struct generic_pm_domain_data *gpd_data;
 	struct pm_domain_data *pdd;
-	bool remove = false;
 	int ret = 0;
 
 	dev_dbg(dev, "%s()\n", __func__);
@@ -1507,10 +1505,7 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd,
 	pdd = dev->power.subsys_data->domain_data;
 	list_del_init(&pdd->list_node);
 	gpd_data = to_gpd_data(pdd);
-	if (--gpd_data->refcount == 0) {
-		dev->power.subsys_data->domain_data = NULL;
-		remove = true;
-	}
+	dev->power.subsys_data->domain_data = NULL;
 
 	spin_unlock_irq(&dev->power.lock);
 
@@ -1521,8 +1516,7 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd,
 	genpd_release_lock(genpd);
 
 	dev_pm_put_subsys_data(dev);
-	if (remove)
-		__pm_genpd_free_dev_data(dev, gpd_data);
+	__pm_genpd_free_dev_data(dev, gpd_data);
 
 	return 0;
 
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index 73e938b..e4edde1 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -103,7 +103,6 @@ struct generic_pm_domain_data {
 	struct gpd_timing_data td;
 	struct notifier_block nb;
 	struct mutex lock;
-	unsigned int refcount;
 	bool need_restore;
 };
 
-- 
1.9.1

[PATCH 1/4] PM / Domains: Remove reference counting for the generic_pm_domain_data

[Kevin Hilman]

Ulf Hansson <ulf.hansson@linaro.org> writes:

> The reference counting isn't needed, let's remove it.

After reading the commit that added this feature[1], I think this
changelog needs a more detail explaining why it's not needed anymore.

Kevin

[1]
commit 1d5fcfec22ce5f69db0d29284d2b65ff8ab1bfaa
Author: Rafael J. Wysocki <rjw@sisk.pl>
Date:   Thu Jul 5 22:12:32 2012 +0200

    PM / Domains: Add device domain data reference counter

    Add a mechanism for counting references to the
    struct generic_pm_domain_data object pointed to by
    dev->power.subsys_data->domain_data if the device in question
    belongs to a generic PM domain.

    This change is necessary for a subsequent patch making it possible to
    allocate that object from within pm_genpd_add_callbacks(), so that
    drivers can attach their PM domain device callbacks to devices before
    those devices are added to PM domains.

    This patch has been tested on the SH7372 Mackerel board.

    Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

[PATCH 2/4] PM / Domains: Don't allow an existing generic PM domain data

[Ulf Hansson]

While adding devices to the generic PM domain we allocate data for the
struct generic_pm_domain_data.

Don't allow existing generic_pm_domain_data in this case, since that
indicates the device is already being added from another context. Let's
instead return an error.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/base/power/domain.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 7546242..9d511c7 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1423,9 +1423,9 @@ int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
 
 	spin_lock_irq(&dev->power.lock);
 
-	dev->pm_domain = &genpd->domain;
 	if (dev->power.subsys_data->domain_data) {
-		gpd_data = to_gpd_data(dev->power.subsys_data->domain_data);
+		ret = -EINVAL;
+		goto out;
 	} else {
 		gpd_data = gpd_data_new;
 		dev->power.subsys_data->domain_data = &gpd_data->base;
@@ -1433,6 +1433,8 @@ int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
 	if (td)
 		gpd_data->td = *td;
 
+	dev->pm_domain = &genpd->domain;
+
 	spin_unlock_irq(&dev->power.lock);
 
 	if (genpd->attach_dev)
-- 
1.9.1

[PATCH 2/4] PM / Domains: Don't allow an existing generic PM domain data

[Kevin Hilman]

Ulf Hansson <ulf.hansson@linaro.org> writes:

> While adding devices to the generic PM domain we allocate data for the
> struct generic_pm_domain_data.
>
> Don't allow existing generic_pm_domain_data in this case, since that
> indicates the device is already being added from another context. Let's
> instead return an error.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

Similar to PATCH 1/4, it would be good to summarize/remind why this
feature was added in the first place, and why it's not longer needed.

Kevin

[PATCH 3/4] PM / Domains: Improve error handling while adding/removing devices

[Ulf Hansson]

To improve error handling while adding/removing devices from their PM
domains, we need to restructure the code a bit. Let's do this by moving
the device specific parts into a separate function.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/base/power/domain.c | 132 +++++++++++++++++++++++---------------------
 1 file changed, 69 insertions(+), 63 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 9d511c7..4e5fcd7 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1358,25 +1358,81 @@ EXPORT_SYMBOL_GPL(pm_genpd_syscore_poweron);
 
 #endif /* CONFIG_PM_SLEEP */
 
-static struct generic_pm_domain_data *__pm_genpd_alloc_dev_data(struct device *dev)
+static int genpd_alloc_dev_data(struct generic_pm_domain *genpd,
+				struct device *dev, struct gpd_timing_data *td)
 {
 	struct generic_pm_domain_data *gpd_data;
+	int ret;
+
+	dev_dbg(dev, "%s()\n", __func__);
+
+	ret = dev_pm_get_subsys_data(dev);
+	if (ret)
+		return ret;
 
 	gpd_data = kzalloc(sizeof(*gpd_data), GFP_KERNEL);
-	if (!gpd_data)
-		return NULL;
+	if (!gpd_data) {
+		ret = -ENOMEM;
+		goto err_alloc;
+	}
 
 	mutex_init(&gpd_data->lock);
+	gpd_data->base.dev = dev;
+	gpd_data->td.constraint_changed = true;
+	gpd_data->td.effective_constraint_ns = -1;
 	gpd_data->nb.notifier_call = genpd_dev_pm_qos_notifier;
+	if (td)
+		gpd_data->td = *td;
+
+	spin_lock_irq(&dev->power.lock);
+	if (!dev->power.subsys_data->domain_data)
+		dev->power.subsys_data->domain_data = &gpd_data->base;
+	else
+		ret = -EINVAL;
+	spin_unlock_irq(&dev->power.lock);
+
+	if (ret)
+		goto err_data;
+
+	if (genpd->attach_dev)
+		genpd->attach_dev(dev);
+
 	dev_pm_qos_add_notifier(dev, &gpd_data->nb);
-	return gpd_data;
+	return 0;
+
+ err_data:
+	kfree(gpd_data);
+ err_alloc:
+	dev_pm_put_subsys_data(dev);
+	return ret;
 }
 
-static void __pm_genpd_free_dev_data(struct device *dev,
-				     struct generic_pm_domain_data *gpd_data)
+static void genpd_free_dev_data(struct generic_pm_domain *genpd,
+				struct device *dev)
 {
+	struct generic_pm_domain_data *gpd_data;
+	struct pm_domain_data *pdd;
+
+	dev_dbg(dev, "%s()\n", __func__);
+
+	if (genpd->detach_dev)
+		genpd->detach_dev(dev);
+
+	spin_lock_irq(&dev->power.lock);
+	dev->pm_domain = NULL;
+	pdd = dev->power.subsys_data->domain_data;
+	list_del_init(&pdd->list_node);
+	gpd_data = to_gpd_data(pdd);
+	dev->power.subsys_data->domain_data = NULL;
+	spin_unlock_irq(&dev->power.lock);
+
+	mutex_lock(&gpd_data->lock);
+	pdd->dev = NULL;
+	mutex_unlock(&gpd_data->lock);
+
 	dev_pm_qos_remove_notifier(dev, &gpd_data->nb);
 	kfree(gpd_data);
+	dev_pm_put_subsys_data(dev);
 }
 
 /**
@@ -1388,7 +1444,7 @@ static void __pm_genpd_free_dev_data(struct device *dev,
 int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
 			  struct gpd_timing_data *td)
 {
-	struct generic_pm_domain_data *gpd_data_new, *gpd_data = NULL;
+	struct generic_pm_domain_data *gpd_data;
 	struct pm_domain_data *pdd;
 	int ret = 0;
 
@@ -1397,10 +1453,6 @@ int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
 	if (IS_ERR_OR_NULL(genpd) || IS_ERR_OR_NULL(dev))
 		return -EINVAL;
 
-	gpd_data_new = __pm_genpd_alloc_dev_data(dev);
-	if (!gpd_data_new)
-		return -ENOMEM;
-
 	genpd_acquire_lock(genpd);
 
 	if (genpd->prepared_count > 0) {
@@ -1414,46 +1466,25 @@ int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
 			goto out;
 		}
 
-	ret = dev_pm_get_subsys_data(dev);
+	ret = genpd_alloc_dev_data(genpd, dev, td);
 	if (ret)
 		goto out;
 
-	genpd->device_count++;
-	genpd->max_off_time_changed = true;
-
 	spin_lock_irq(&dev->power.lock);
-
-	if (dev->power.subsys_data->domain_data) {
-		ret = -EINVAL;
-		goto out;
-	} else {
-		gpd_data = gpd_data_new;
-		dev->power.subsys_data->domain_data = &gpd_data->base;
-	}
-	if (td)
-		gpd_data->td = *td;
-
 	dev->pm_domain = &genpd->domain;
-
+	gpd_data = to_gpd_data(dev->power.subsys_data->domain_data);
 	spin_unlock_irq(&dev->power.lock);
 
-	if (genpd->attach_dev)
-		genpd->attach_dev(dev);
-
 	mutex_lock(&gpd_data->lock);
-	gpd_data->base.dev = dev;
 	list_add_tail(&gpd_data->base.list_node, &genpd->dev_list);
 	gpd_data->need_restore = genpd->status == GPD_STATE_POWER_OFF;
-	gpd_data->td.constraint_changed = true;
-	gpd_data->td.effective_constraint_ns = -1;
 	mutex_unlock(&gpd_data->lock);
 
+	genpd->device_count++;
+	genpd->max_off_time_changed = true;
+
  out:
 	genpd_release_lock(genpd);
-
-	if (gpd_data != gpd_data_new)
-		__pm_genpd_free_dev_data(dev, gpd_data_new);
-
 	return ret;
 }
 
@@ -1477,8 +1508,6 @@ int __pm_genpd_name_add_device(const char *domain_name, struct device *dev,
 int pm_genpd_remove_device(struct generic_pm_domain *genpd,
 			   struct device *dev)
 {
-	struct generic_pm_domain_data *gpd_data;
-	struct pm_domain_data *pdd;
 	int ret = 0;
 
 	dev_dbg(dev, "%s()\n", __func__);
@@ -1498,33 +1527,10 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd,
 	genpd->device_count--;
 	genpd->max_off_time_changed = true;
 
-	if (genpd->detach_dev)
-		genpd->detach_dev(dev);
-
-	spin_lock_irq(&dev->power.lock);
-
-	dev->pm_domain = NULL;
-	pdd = dev->power.subsys_data->domain_data;
-	list_del_init(&pdd->list_node);
-	gpd_data = to_gpd_data(pdd);
-	dev->power.subsys_data->domain_data = NULL;
-
-	spin_unlock_irq(&dev->power.lock);
-
-	mutex_lock(&gpd_data->lock);
-	pdd->dev = NULL;
-	mutex_unlock(&gpd_data->lock);
-
-	genpd_release_lock(genpd);
-
-	dev_pm_put_subsys_data(dev);
-	__pm_genpd_free_dev_data(dev, gpd_data);
-
-	return 0;
+	genpd_free_dev_data(genpd, dev);
 
  out:
 	genpd_release_lock(genpd);
-
 	return ret;
 }
 
-- 
1.9.1

[PATCH 3/4] PM / Domains: Improve error handling while adding/removing devices

[Kevin Hilman]

Ulf Hansson <ulf.hansson@linaro.org> writes:

> To improve error handling while adding/removing devices from their PM
> domains, we need to restructure the code a bit. Let's do this by moving
> the device specific parts into a separate function.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

This looks like just restructuring, but with these kinds of patches, its
hard to be sure that it's just refactoring, with no functional changes,
so it's nice to be clear in the changelog whether there are (meant to
be) any functional changes.

Kevin

[PATCH 3/4] PM / Domains: Improve error handling while adding/removing devices

[Ulf Hansson]

On 30 October 2014 00:53, Kevin Hilman <khilman@kernel.org> wrote:
> Ulf Hansson <ulf.hansson@linaro.org> writes:
>
>> To improve error handling while adding/removing devices from their PM
>> domains, we need to restructure the code a bit. Let's do this by moving
>> the device specific parts into a separate function.
>>
>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>
> This looks like just restructuring, but with these kinds of patches, its
> hard to be sure that it's just refactoring, with no functional changes,
> so it's nice to be clear in the changelog whether there are (meant to
> be) any functional changes.

According to the commit header it's not just restructuring. I will
update the commit message to reflect this better.

Br
Uffe

[PATCH 3/4] PM / Domains: Improve error handling while adding/removing devices

[Kevin Hilman]

Ulf Hansson <ulf.hansson@linaro.org> writes:

> To improve error handling while adding/removing devices from their PM
> domains, we need to restructure the code a bit. Let's do this by moving
> the device specific parts into a separate function.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

[...]

> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 9d511c7..4e5fcd7 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -1358,25 +1358,81 @@ EXPORT_SYMBOL_GPL(pm_genpd_syscore_poweron);
>  
>  #endif /* CONFIG_PM_SLEEP */
>  
> -static struct generic_pm_domain_data *__pm_genpd_alloc_dev_data(struct device *dev)
> +static int genpd_alloc_dev_data(struct generic_pm_domain *genpd,
> +				struct device *dev, struct gpd_timing_data *td)
>  {
>  	struct generic_pm_domain_data *gpd_data;
> +	int ret;
> +
> +	dev_dbg(dev, "%s()\n", __func__);
> +
> +	ret = dev_pm_get_subsys_data(dev);
> +	if (ret)
> +		return ret;
>  
>  	gpd_data = kzalloc(sizeof(*gpd_data), GFP_KERNEL);
> -	if (!gpd_data)
> -		return NULL;
> +	if (!gpd_data) {
> +		ret = -ENOMEM;
> +		goto err_alloc;
> +	}
>  
>  	mutex_init(&gpd_data->lock);
> +	gpd_data->base.dev = dev;
> +	gpd_data->td.constraint_changed = true;
> +	gpd_data->td.effective_constraint_ns = -1;
>  	gpd_data->nb.notifier_call = genpd_dev_pm_qos_notifier;
> +	if (td)
> +		gpd_data->td = *td;
> +
> +	spin_lock_irq(&dev->power.lock);
> +	if (!dev->power.subsys_data->domain_data)
> +		dev->power.subsys_data->domain_data = &gpd_data->base;
> +	else
> +		ret = -EINVAL;
> +	spin_unlock_irq(&dev->power.lock);
> +
> +	if (ret)
> +		goto err_data;
> +
> +	if (genpd->attach_dev)
> +		genpd->attach_dev(dev);

To me, it doesn't seem right that the attach is done in the 'alloc'
function.  IMO, the attach should stay in _add_device()

Kevin

[PATCH 3/4] PM / Domains: Improve error handling while adding/removing devices

[Ulf Hansson]

On 30 October 2014 00:57, Kevin Hilman <khilman@kernel.org> wrote:
> Ulf Hansson <ulf.hansson@linaro.org> writes:
>
>> To improve error handling while adding/removing devices from their PM
>> domains, we need to restructure the code a bit. Let's do this by moving
>> the device specific parts into a separate function.
>>
>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>
> [...]
>
>> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>> index 9d511c7..4e5fcd7 100644
>> --- a/drivers/base/power/domain.c
>> +++ b/drivers/base/power/domain.c
>> @@ -1358,25 +1358,81 @@ EXPORT_SYMBOL_GPL(pm_genpd_syscore_poweron);
>>
>>  #endif /* CONFIG_PM_SLEEP */
>>
>> -static struct generic_pm_domain_data *__pm_genpd_alloc_dev_data(struct device *dev)
>> +static int genpd_alloc_dev_data(struct generic_pm_domain *genpd,
>> +                             struct device *dev, struct gpd_timing_data *td)
>>  {
>>       struct generic_pm_domain_data *gpd_data;
>> +     int ret;
>> +
>> +     dev_dbg(dev, "%s()\n", __func__);
>> +
>> +     ret = dev_pm_get_subsys_data(dev);
>> +     if (ret)
>> +             return ret;
>>
>>       gpd_data = kzalloc(sizeof(*gpd_data), GFP_KERNEL);
>> -     if (!gpd_data)
>> -             return NULL;
>> +     if (!gpd_data) {
>> +             ret = -ENOMEM;
>> +             goto err_alloc;
>> +     }
>>
>>       mutex_init(&gpd_data->lock);
>> +     gpd_data->base.dev = dev;
>> +     gpd_data->td.constraint_changed = true;
>> +     gpd_data->td.effective_constraint_ns = -1;
>>       gpd_data->nb.notifier_call = genpd_dev_pm_qos_notifier;
>> +     if (td)
>> +             gpd_data->td = *td;
>> +
>> +     spin_lock_irq(&dev->power.lock);
>> +     if (!dev->power.subsys_data->domain_data)
>> +             dev->power.subsys_data->domain_data = &gpd_data->base;
>> +     else
>> +             ret = -EINVAL;
>> +     spin_unlock_irq(&dev->power.lock);
>> +
>> +     if (ret)
>> +             goto err_data;
>> +
>> +     if (genpd->attach_dev)
>> +             genpd->attach_dev(dev);
>
> To me, it doesn't seem right that the attach is done in the 'alloc'
> function.  IMO, the attach should stay in _add_device()

That's right! I fix in a v2.

Kind regards
Uffe

[PATCH 3/4] PM / Domains: Improve error handling while adding/removing devices

[Geert Uytterhoeven]

On Tue, Oct 28, 2014 at 3:38 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -1358,25 +1358,81 @@ EXPORT_SYMBOL_GPL(pm_genpd_syscore_poweron);
>
>  #endif /* CONFIG_PM_SLEEP */
>
> -static struct generic_pm_domain_data *__pm_genpd_alloc_dev_data(struct device *dev)
> +static int genpd_alloc_dev_data(struct generic_pm_domain *genpd,
> +                               struct device *dev, struct gpd_timing_data *td)
>  {

[...]

> +       if (genpd->attach_dev)
> +               genpd->attach_dev(dev);

Note that dev->pm_domain is not yet set at this point, so the callee
can no longer
know to which domain the device is being attached.
Should we re-add the parameter, or move the attach_dev() back to
__pm_genpd_add_device(), like Kevin suggested.

[...]

>  }

>  /**
> @@ -1388,7 +1444,7 @@ static void __pm_genpd_free_dev_data(struct device *dev,
>  int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
>                           struct gpd_timing_data *td)
>  {

[...]

> -       ret = dev_pm_get_subsys_data(dev);
> +       ret = genpd_alloc_dev_data(genpd, dev, td);

[...]

>         dev->pm_domain = &genpd->domain;
> -
> +       gpd_data = to_gpd_data(dev->power.subsys_data->domain_data);
>         spin_unlock_irq(&dev->power.lock);
>
> -       if (genpd->attach_dev)
> -               genpd->attach_dev(dev);
> -

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at 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

[PATCH 3/4] PM / Domains: Improve error handling while adding/removing devices

[Ulf Hansson]

On 5 November 2014 08:47, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Tue, Oct 28, 2014 at 3:38 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> --- a/drivers/base/power/domain.c
>> +++ b/drivers/base/power/domain.c
>> @@ -1358,25 +1358,81 @@ EXPORT_SYMBOL_GPL(pm_genpd_syscore_poweron);
>>
>>  #endif /* CONFIG_PM_SLEEP */
>>
>> -static struct generic_pm_domain_data *__pm_genpd_alloc_dev_data(struct device *dev)
>> +static int genpd_alloc_dev_data(struct generic_pm_domain *genpd,
>> +                               struct device *dev, struct gpd_timing_data *td)
>>  {
>
> [...]
>
>> +       if (genpd->attach_dev)
>> +               genpd->attach_dev(dev);
>
> Note that dev->pm_domain is not yet set at this point, so the callee
> can no longer
> know to which domain the device is being attached.
> Should we re-add the parameter, or move the attach_dev() back to
> __pm_genpd_add_device(), like Kevin suggested.

I agree.

I am working on a new version, which adopts to your suggestions.

Kind regards
Uffe

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

[Ulf Hansson]

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

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);
 };
 
-- 
1.9.1

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

[Geert Uytterhoeven]

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

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

[Ulf Hansson]

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

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

[Geert Uytterhoeven]

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

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

[Ulf Hansson]

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

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

[Geert Uytterhoeven]

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

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

[Kevin Hilman]

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

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

[Geert Uytterhoeven]

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

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

[Ulf Hansson]

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