Re: [PATCH v10 2/4] PM / Domains: add setter for dev.pm_domain

[Tomeu Vizoso]

On 25 October 2015 at 16:18, Rafael J. Wysocki <rjw@xxxxxxxxxxxxx> wrote:
> On Wednesday, October 21, 2015 05:34:12 PM Tomeu Vizoso wrote:
>> Adds a function that sets the pointer to dev_pm_domain in struct device
>> and that warns if the device has already finished probing. The reason
>> why we want to enforce that is because in the general case that can
>> cause problems and also that we can simplify code quite a bit if we can
>> always assume that.
>>
>> This patch also changes all current code that directly sets the
>> dev.pm_domain pointer.
>>
>> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@xxxxxxxxxxxxx>
>> Reviewed-by: Ulf Hansson <ulf.hansson@xxxxxxxxxx>
>> ---
>>
>> Changes in v9:
>> - Add docs noting the need for the device lock to be held before calling
>>   dev_pm_domain_set()
>>
>> Changes in v8:
>> - Add dev_pm_domain_set() and update code to use it
>>
>>  arch/arm/mach-omap2/omap_device.c |  7 ++++---
>>  drivers/acpi/acpi_lpss.c          |  5 +++--
>>  drivers/acpi/device_pm.c          |  5 +++--
>>  drivers/base/power/clock_ops.c    |  5 +++--
>>  drivers/base/power/common.c       | 21 +++++++++++++++++++++
>>  drivers/base/power/domain.c       |  4 ++--
>>  drivers/gpu/vga/vga_switcheroo.c  | 10 +++++-----
>>  drivers/misc/mei/pci-me.c         |  5 +++--
>>  drivers/misc/mei/pci-txe.c        |  5 +++--
>>  include/linux/pm_domain.h         |  3 +++
>>  10 files changed, 50 insertions(+), 20 deletions(-)
>>
>> diff --git a/arch/arm/mach-omap2/omap_device.c b/arch/arm/mach-omap2/omap_device.c
>> index 4cb8fd9f741f..204101d11632 100644
>> --- a/arch/arm/mach-omap2/omap_device.c
>> +++ b/arch/arm/mach-omap2/omap_device.c
>> @@ -32,6 +32,7 @@
>>  #include <linux/io.h>
>>  #include <linux/clk.h>
>>  #include <linux/clkdev.h>
>> +#include <linux/pm_domain.h>
>>  #include <linux/pm_runtime.h>
>>  #include <linux/of.h>
>>  #include <linux/notifier.h>
>> @@ -168,7 +169,7 @@ static int omap_device_build_from_dt(struct platform_device *pdev)
>>                       r->name = dev_name(&pdev->dev);
>>       }
>>
>> -     pdev->dev.pm_domain = &omap_device_pm_domain;
>> +     dev_pm_domain_set(&pdev->dev, &omap_device_pm_domain);
>>
>>       if (device_active) {
>>               omap_device_enable(pdev);
>> @@ -180,7 +181,7 @@ odbfd_exit1:
>>  odbfd_exit:
>>       /* if data/we are at fault.. load up a fail handler */
>>       if (ret)
>> -             pdev->dev.pm_domain = &omap_device_fail_pm_domain;
>> +             dev_pm_domain_set(&pdev->dev, &omap_device_fail_pm_domain);
>>
>>       return ret;
>>  }
>> @@ -701,7 +702,7 @@ int omap_device_register(struct platform_device *pdev)
>>  {
>>       pr_debug("omap_device: %s: registering\n", pdev->name);
>>
>> -     pdev->dev.pm_domain = &omap_device_pm_domain;
>> +     dev_pm_domain_set(&pdev->dev, &omap_device_pm_domain);
>>       return platform_device_add(pdev);
>>  }
>>
>> diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
>> index f51bd0d0bc17..cc6e1abc69b3 100644
>> --- a/drivers/acpi/acpi_lpss.c
>> +++ b/drivers/acpi/acpi_lpss.c
>> @@ -17,6 +17,7 @@
>>  #include <linux/io.h>
>>  #include <linux/platform_device.h>
>>  #include <linux/platform_data/clk-lpss.h>
>> +#include <linux/pm_domain.h>
>>  #include <linux/pm_runtime.h>
>>  #include <linux/delay.h>
>>
>> @@ -706,7 +707,7 @@ static int acpi_lpss_platform_notify(struct notifier_block *nb,
>>
>>       switch (action) {
>>       case BUS_NOTIFY_ADD_DEVICE:
>> -             pdev->dev.pm_domain = &acpi_lpss_pm_domain;
>> +             dev_pm_domain_set(&pdev->dev, &acpi_lpss_pm_domain);
>>               if (pdata->dev_desc->flags & LPSS_LTR)
>>                       return sysfs_create_group(&pdev->dev.kobj,
>>                                                 &lpss_attr_group);
>> @@ -714,7 +715,7 @@ static int acpi_lpss_platform_notify(struct notifier_block *nb,
>>       case BUS_NOTIFY_DEL_DEVICE:
>>               if (pdata->dev_desc->flags & LPSS_LTR)
>>                       sysfs_remove_group(&pdev->dev.kobj, &lpss_attr_group);
>> -             pdev->dev.pm_domain = NULL;
>> +             dev_pm_domain_set(&pdev->dev, NULL);
>>               break;
>>       default:
>>               break;
>> diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
>> index 4806b7f856c4..8c5955bf9bbf 100644
>> --- a/drivers/acpi/device_pm.c
>> +++ b/drivers/acpi/device_pm.c
>> @@ -22,6 +22,7 @@
>>  #include <linux/export.h>
>>  #include <linux/mutex.h>
>>  #include <linux/pm_qos.h>
>> +#include <linux/pm_domain.h>
>>  #include <linux/pm_runtime.h>
>>
>>  #include "internal.h"
>> @@ -1076,7 +1077,7 @@ static void acpi_dev_pm_detach(struct device *dev, bool power_off)
>>       struct acpi_device *adev = ACPI_COMPANION(dev);
>>
>>       if (adev && dev->pm_domain == &acpi_general_pm_domain) {
>> -             dev->pm_domain = NULL;
>> +             dev_pm_domain_set(dev, NULL);
>>               acpi_remove_pm_notifier(adev);
>>               if (power_off) {
>>                       /*
>> @@ -1128,7 +1129,7 @@ int acpi_dev_pm_attach(struct device *dev, bool power_on)
>>               return -EBUSY;
>>
>>       acpi_add_pm_notifier(adev, dev, acpi_pm_notify_work_func);
>> -     dev->pm_domain = &acpi_general_pm_domain;
>> +     dev_pm_domain_set(dev, &acpi_general_pm_domain);
>>       if (power_on) {
>>               acpi_dev_pm_full_power(adev);
>>               acpi_device_wakeup(adev, ACPI_STATE_S0, false);
>> diff --git a/drivers/base/power/clock_ops.c b/drivers/base/power/clock_ops.c
>> index 652b5a367c1f..e80fda6c03a9 100644
>> --- a/drivers/base/power/clock_ops.c
>> +++ b/drivers/base/power/clock_ops.c
>> @@ -15,6 +15,7 @@
>>  #include <linux/clkdev.h>
>>  #include <linux/slab.h>
>>  #include <linux/err.h>
>> +#include <linux/pm_domain.h>
>>  #include <linux/pm_runtime.h>
>>
>>  #ifdef CONFIG_PM
>> @@ -346,7 +347,7 @@ static int pm_clk_notify(struct notifier_block *nb,
>>               if (error)
>>                       break;
>>
>> -             dev->pm_domain = clknb->pm_domain;
>> +             dev_pm_domain_set(dev, clknb->pm_domain);
>>               if (clknb->con_ids[0]) {
>>                       for (con_id = clknb->con_ids; *con_id; con_id++)
>>                               pm_clk_add(dev, *con_id);
>> @@ -359,7 +360,7 @@ static int pm_clk_notify(struct notifier_block *nb,
>>               if (dev->pm_domain != clknb->pm_domain)
>>                       break;
>>
>> -             dev->pm_domain = NULL;
>> +             dev_pm_domain_set(dev, NULL);
>>               pm_clk_destroy(dev);
>>               break;
>>       }
>> diff --git a/drivers/base/power/common.c b/drivers/base/power/common.c
>> index f32b802b98f4..a70f8a5cdfd7 100644
>> --- a/drivers/base/power/common.c
>> +++ b/drivers/base/power/common.c
>> @@ -128,3 +128,24 @@ void dev_pm_domain_detach(struct device *dev, bool power_off)
>>               dev->pm_domain->detach(dev, power_off);
>>  }
>>  EXPORT_SYMBOL_GPL(dev_pm_domain_detach);
>> +
>> +/**
>> + * dev_pm_domain_set - Set PM domain of a device.
>> + * @dev: Device whose PM domain is to be set.
>> + * @pd: PM domain to be set, or NULL.
>> + *
>> + * Sets the PM domain the device belongs to. The PM domain of a device needs
>> + * to be set before its probe finishes (it's bound to a driver).
>> + *
>> + * This function must be called with the device lock held.
>
> So obviously every caller of this function that doesn't lock the device has to
> be called with the device locked already.
>
> Have you verified that this is the case?

Haven't checked that all the possible codepaths have the device lock,
but as they were modifying dev->pm_domain already, I assumed they
should.

Regards,

Tomeu
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/