Re: [PATCH 3/4] PM / Domains: Improve error handling while adding/removing devices
On 5 November 2014 08:47, Geert Uytterhoeven wrote:
> On Tue, Oct 28, 2014 at 3:38 PM, Ulf Hansson 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] PM / Domains: Improve error handling while adding/removing devices
On Tue, Oct 28, 2014 at 3:38 PM, Ulf Hansson 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 -- ge...@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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] PM / Domains: Improve error handling while adding/removing devices
On 30 October 2014 00:53, Kevin Hilman wrote: > Ulf Hansson 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 > > 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] PM / Domains: Improve error handling while adding/removing devices
On 30 October 2014 00:57, Kevin Hilman wrote:
> Ulf Hansson 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
>
> [...]
>
>> 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-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] PM / Domains: Improve error handling while adding/removing devices
Ulf Hansson 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
[...]
> 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] PM / Domains: Improve error handling while adding/removing devices
Ulf Hansson 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 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-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/4] PM / Domains: Improve error handling while adding/removing devices
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
---
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
