PM / Domains: Handle errors from ->attach_dev() callback

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

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 <[email protected]>

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 <[email protected]>

Quoted 4 lines · 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.

Quoted 1 line

Signed-off-by Ulf Hansson <[email protected]>

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.

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 <[email protected]>

drivers/base/power/domain.c +21 -3

index 9d511c7..4e5fcd7 100644
@@ -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)

Quoted 5 lines · 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 <[email protected]>

[...]

Quoted 44 lines

drivers/base/power/domain.c +26 -3

index 9d511c7..4e5fcd7 100644
@@ -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);
+

drivers/base/power/domain.c EXPORT_SYMBOL_GPL(pm_genpd_syscore_poweron); · line 1398

Quoted 5 lines

drivers/base/power/domain.c +5 -0

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

drivers/base/power/domain.c +2 -1

@@ -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)
 {

}

/**

drivers/base/power/domain.c +0 -0

@@ -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)
 {

drivers/base/power/domain.c +1 -1

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

Quoted 10 lines · Ulf Hansson

drivers/base/power/domain.c +2 -1

@@ -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)
 {

[...]

drivers/base/power/domain.c EXPORT_SYMBOL_GPL(pm_genpd_syscore_poweron); · line 1398

Quoted 2 lines

drivers/base/power/domain.c +2 -0

+       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.

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 <[email protected]>

Hi Ulf, Rafael,

Quoted 7 lines · 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.

Quoted 1 line

Signed-off-by Ulf Hansson <[email protected]>

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?

Quoted 8 lines · Geert Uytterhoeven

Quoted 7 lines · 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.

Quoted 7 lines

Quoted 1 line

Signed-off-by Ulf Hansson <[email protected]>

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.

Hi Ulf,

Quoted 17 lines · Ulf Hansson

Quoted 8 lines · Geert Uytterhoeven

Quoted 7 lines · 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.

Quoted 7 lines

Quoted 1 line

Signed-off-by Ulf Hansson <[email protected]>

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?

Quoted 6 lines

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.

Quoted 2 lines

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.

Quoted 18 lines

Quoted 6 lines

Quoted 5 lines

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?

Quoted 6 lines

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.

Quoted 2 lines

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

Quoted 6 lines

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.

Hi Ulf, Rafael,

Quoted 7 lines · 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.

Quoted 1 line

Signed-off-by Ulf Hansson <[email protected]>

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?

Quoted 8 lines · Geert Uytterhoeven

Quoted 7 lines · 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.

Quoted 4 lines

Quoted 1 line

Signed-off-by Ulf Hansson <[email protected]>

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? :)

Quoted 10 lines · Geert Uytterhoeven

Hi Ulf, Rafael,

Quoted 7 lines · 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.

Quoted 4 lines

Quoted 1 line

Signed-off-by Ulf Hansson <[email protected]>

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? :)

Quoted 1 line

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.

Quoted 2 lines

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.