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

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

Commit Message

Ulf Hansson Oct. 28, 2014, 2:38 p.m.
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(-)

Comments

Kevin Hilman Oct. 29, 2014, 11:53 p.m. | #1
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
--
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
Kevin Hilman Oct. 29, 2014, 11:57 p.m. | #2
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
--
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:25 a.m. | #3
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
--
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. 30, 2014, 11:27 a.m. | #4
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
--
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 Nov. 5, 2014, 7:47 a.m. | #5
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@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 Nov. 5, 2014, 8:03 a.m. | #6
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
--
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 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;
 }