[PATCH v3 0/5] Support running driver's probe for a device powered off

[PATCH v3 0/5] Support running driver's probe for a device powered off

[Sakari Ailus]

Hi all,

These patches enable calling (and finishing) a driver's probe function
without powering on the respective device on busses where the practice is
to power on the device for probe. While it generally is a driver's job to
check the that the device is there, there are cases where it might be
undesirable. (In this case it stems from a combination of hardware design
and user expectations; see below.) The downside with this change is that
if there is something wrong with the device, it will only be found at the
time the device is used. In this case (the camera sensors + EEPROM in a
sensor) I don't see any tangible harm from that though.

An indication both from the driver and the firmware is required to allow
the device's power state to remain off during probe (see the first patch).


The use case is such that there is a privacy LED next to an integrated
user-facing laptop camera, and this LED is there to signal the user that
the camera is recording a video or capturing images. That LED also happens
to be wired to one of the power supplies of the camera, so whenever you
power on the camera, the LED will be lit, whether images are captured from
the camera --- or not. There's no way to implement this differently
without additional software control (allowing of which is itself a
hardware design decision) on most CSI-2-connected camera sensors as they
simply have no pin to signal the camera streaming state.

This is also what happens during driver probe: the camera will be powered
on by the I²C subsystem calling dev_pm_domain_attach() and the device is
already powered on when the driver's own probe function is called. To the
user this visible during the boot process as a blink of the privacy LED,
suggesting that the camera is recording without the user having used an
application to do that. From the end user's point of view the behaviour is
not expected and for someone unfamiliar with internal workings of a
computer surely seems quite suspicious --- even if images are not being
actually captured.

v2 can be found here:

<URL:https://patchwork.kernel.org/cover/11114255/>

since v2:

- Remove extra CONFIG_PM ifdefs; these are not needed.

- Move the checks for power state hints from drivers/base/dd.c to
  drivers/i2c/i2c-base-core.c; these are I²C devices anyway.

- Move the probe_low_power field from struct device_driver to struct
  i2c_driver.

since v1:

- Rename probe_powered_off struct device field as probe_low_power and
  reflect the similar naming to the patches overall.

- Work with CONFIG_PM disabled, too.

Rajmohan Mani (1):
  media: i2c: imx319: Support probe while the device is off

Sakari Ailus (4):
  i2c: Allow driver to manage the device's power state during probe
  ACPI: Add a convenience function to tell a device is suspended in
    probe
  ov5670: Support probe whilst the device is in a low power state
  at24: Support probing while off

 drivers/acpi/device_pm.c    | 35 +++++++++++++++++++++++++++++++++++
 drivers/i2c/i2c-core-base.c | 15 ++++++++++++---
 drivers/media/i2c/imx319.c  | 23 ++++++++++++++---------
 drivers/media/i2c/ov5670.c  | 23 ++++++++++++++---------
 drivers/misc/eeprom/at24.c  | 31 +++++++++++++++++++++----------
 include/linux/acpi.h        |  5 +++++
 include/linux/i2c.h         |  3 +++
 7 files changed, 104 insertions(+), 31 deletions(-)

-- 
2.20.1

[PATCH v3 1/5] i2c: Allow driver to manage the device's power state during probe

[Sakari Ailus]

Enable drivers to tell ACPI that there's no need to power on a device for
probe. Drivers should still perform this by themselves if there's a need
to. In some cases powering on the device during probe is undesirable, and
this change enables a driver to choose what fits best for it.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/i2c/i2c-core-base.c | 15 ++++++++++++---
 include/linux/i2c.h         |  3 +++
 2 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 9f8dcd3f83850..7bf1699c9044d 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -303,6 +303,14 @@ static int i2c_smbus_host_notify_to_irq(const struct i2c_client *client)
 	return irq > 0 ? irq : -ENXIO;
 }
 
+static bool probe_low_power(struct device *dev)
+{
+	struct i2c_driver *driver = to_i2c_driver(dev->driver);
+
+	return driver->probe_low_power &&
+		device_property_present(dev, "probe-low-power");
+}
+
 static int i2c_device_probe(struct device *dev)
 {
 	struct i2c_client	*client = i2c_verify_client(dev);
@@ -375,7 +383,8 @@ static int i2c_device_probe(struct device *dev)
 	if (status < 0)
 		goto err_clear_wakeup_irq;
 
-	status = dev_pm_domain_attach(&client->dev, true);
+	status = dev_pm_domain_attach(&client->dev,
+				      !probe_low_power(&client->dev));
 	if (status)
 		goto err_clear_wakeup_irq;
 
@@ -397,7 +406,7 @@ static int i2c_device_probe(struct device *dev)
 	return 0;
 
 err_detach_pm_domain:
-	dev_pm_domain_detach(&client->dev, true);
+	dev_pm_domain_detach(&client->dev, !probe_low_power(&client->dev));
 err_clear_wakeup_irq:
 	dev_pm_clear_wake_irq(&client->dev);
 	device_init_wakeup(&client->dev, false);
@@ -419,7 +428,7 @@ static int i2c_device_remove(struct device *dev)
 		status = driver->remove(client);
 	}
 
-	dev_pm_domain_detach(&client->dev, true);
+	dev_pm_domain_detach(&client->dev, !probe_low_power(&client->dev));
 
 	dev_pm_clear_wake_irq(&client->dev);
 	device_init_wakeup(&client->dev, false);
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 582ef05ec07ed..6d0d6af393c56 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -229,6 +229,8 @@ enum i2c_alert_protocol {
  * @address_list: The I2C addresses to probe (for detect)
  * @clients: List of detected clients we created (for i2c-core use only)
  * @disable_i2c_core_irq_mapping: Tell the i2c-core to not do irq-mapping
+ * @probe_low_power: Let the driver manage the device's power state
+ *		     during probe and remove.
  *
  * The driver.owner field should be set to the module owner of this driver.
  * The driver.name field should be set to the name of this driver.
@@ -289,6 +291,7 @@ struct i2c_driver {
 	struct list_head clients;
 
 	bool disable_i2c_core_irq_mapping;
+	bool probe_low_power;
 };
 #define to_i2c_driver(d) container_of(d, struct i2c_driver, driver)
 
-- 
2.20.1

[PATCH v3 2/5] ACPI: Add a convenience function to tell a device is suspended in probe

[Sakari Ailus]

Add a convenience function to tell whether a device is suspended for probe
or remove, for busses where the custom is that drivers don't need to
resume devices in probe, or suspend them in their remove handlers.

Returns false on non-ACPI systems.

Suggested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/acpi/device_pm.c | 35 +++++++++++++++++++++++++++++++++++
 include/linux/acpi.h     |  5 +++++
 2 files changed, 40 insertions(+)

diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
index 5e4a8860a9c0c..87393020276d8 100644
--- a/drivers/acpi/device_pm.c
+++ b/drivers/acpi/device_pm.c
@@ -1348,4 +1348,39 @@ int acpi_dev_pm_attach(struct device *dev, bool power_on)
 	return 1;
 }
 EXPORT_SYMBOL_GPL(acpi_dev_pm_attach);
+
+/**
+ * acpi_dev_low_power_state_probe - Tell if a device is in a low power state
+ *				    during probe
+ * @dev: The device
+ *
+ * Tell whether a given device is in a low power state during the driver's probe
+ * or remove operation.
+ *
+ * Drivers of devices on certain busses such as I²C can generally assume (on
+ * ACPI based systems) that the devices they control are powered on without
+ * driver having to do anything about it. Using struct
+ * device_driver.probe_low_power and "probe-low-power" property, this can be
+ * negated and the driver has full control of the device power management.
+ * Always returns false on non-ACPI based systems. True is returned on ACPI
+ * based systems iff the device is in a low power state during probe or remove.
+ */
+bool acpi_dev_low_power_state_probe(struct device *dev)
+{
+	int power_state;
+	int ret;
+
+	if (!is_acpi_device_node(dev_fwnode(dev)))
+		return false;
+
+	ret = acpi_device_get_power(ACPI_COMPANION(dev), &power_state);
+	if (ret) {
+		dev_warn(dev, "Cannot obtain power state (%d)\n", ret);
+		return false;
+	}
+
+	return power_state != ACPI_STATE_D0;
+}
+EXPORT_SYMBOL_GPL(acpi_dev_low_power_state_probe);
+
 #endif /* CONFIG_PM */
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 0f37a7d5fa774..fd00853074e1a 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -926,6 +926,7 @@ int acpi_dev_resume(struct device *dev);
 int acpi_subsys_runtime_suspend(struct device *dev);
 int acpi_subsys_runtime_resume(struct device *dev);
 int acpi_dev_pm_attach(struct device *dev, bool power_on);
+bool acpi_dev_low_power_state_probe(struct device *dev);
 #else
 static inline int acpi_dev_runtime_suspend(struct device *dev) { return 0; }
 static inline int acpi_dev_runtime_resume(struct device *dev) { return 0; }
@@ -935,6 +936,10 @@ static inline int acpi_dev_pm_attach(struct device *dev, bool power_on)
 {
 	return 0;
 }
+static inline bool acpi_dev_low_power_state_probe(struct device *dev)
+{
+	return false;
+}
 #endif
 
 #if defined(CONFIG_ACPI) && defined(CONFIG_PM_SLEEP)
-- 
2.20.1

[PATCH v3 3/5] ov5670: Support probe whilst the device is in a low power state

[Sakari Ailus]

Tell ACPI device PM code that the driver supports the device being in a
low power state when the driver's probe function is entered.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/i2c/ov5670.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
index 041fcbb4eebdf..9b10b1c0e1f8f 100644
--- a/drivers/media/i2c/ov5670.c
+++ b/drivers/media/i2c/ov5670.c
@@ -2444,6 +2444,7 @@ static int ov5670_probe(struct i2c_client *client)
 	struct ov5670 *ov5670;
 	const char *err_msg;
 	u32 input_clk = 0;
+	bool low_power;
 	int ret;
 
 	device_property_read_u32(&client->dev, "clock-frequency", &input_clk);
@@ -2460,11 +2461,14 @@ static int ov5670_probe(struct i2c_client *client)
 	/* Initialize subdev */
 	v4l2_i2c_subdev_init(&ov5670->sd, client, &ov5670_subdev_ops);
 
-	/* Check module identity */
-	ret = ov5670_identify_module(ov5670);
-	if (ret) {
-		err_msg = "ov5670_identify_module() error";
-		goto error_print;
+	low_power = acpi_dev_low_power_state_probe(&client->dev);
+	if (!low_power) {
+		/* Check module identity */
+		ret = ov5670_identify_module(ov5670);
+		if (ret) {
+			err_msg = "ov5670_identify_module() error";
+			goto error_print;
+		}
 	}
 
 	mutex_init(&ov5670->mutex);
@@ -2501,10 +2505,10 @@ static int ov5670_probe(struct i2c_client *client)
 	ov5670->streaming = false;
 
 	/*
-	 * Device is already turned on by i2c-core with ACPI domain PM.
-	 * Enable runtime PM and turn off the device.
+	 * Don't set the device's state to active if it's in a low power state.
 	 */
-	pm_runtime_set_active(&client->dev);
+	if (!low_power)
+		pm_runtime_set_active(&client->dev);
 	pm_runtime_enable(&client->dev);
 	pm_runtime_idle(&client->dev);
 
@@ -2546,7 +2550,7 @@ static const struct dev_pm_ops ov5670_pm_ops = {
 
 #ifdef CONFIG_ACPI
 static const struct acpi_device_id ov5670_acpi_ids[] = {
-	{"INT3479"},
+	{ "INT3479" },
 	{ /* sentinel */ }
 };
 
@@ -2561,6 +2565,7 @@ static struct i2c_driver ov5670_i2c_driver = {
 	},
 	.probe_new = ov5670_probe,
 	.remove = ov5670_remove,
+	.probe_low_power = true,
 };
 
 module_i2c_driver(ov5670_i2c_driver);
-- 
2.20.1

[PATCH v3 4/5] media: i2c: imx319: Support probe while the device is off

[Sakari Ailus]

From: Rajmohan Mani <rajmohan.mani@intel.com>

Tell ACPI device PM code that the driver supports the device being powered
off when the driver's probe function is entered.

Signed-off-by: Rajmohan Mani <rajmohan.mani@intel.com>
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/i2c/imx319.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/drivers/media/i2c/imx319.c b/drivers/media/i2c/imx319.c
index 17c2e4b41221e..3bb87154dd2cd 100644
--- a/drivers/media/i2c/imx319.c
+++ b/drivers/media/i2c/imx319.c
@@ -2424,6 +2424,7 @@ static struct imx319_hwcfg *imx319_get_hwcfg(struct device *dev)
 static int imx319_probe(struct i2c_client *client)
 {
 	struct imx319 *imx319;
+	bool low_power;
 	int ret;
 	u32 i;
 
@@ -2436,11 +2437,14 @@ static int imx319_probe(struct i2c_client *client)
 	/* Initialize subdev */
 	v4l2_i2c_subdev_init(&imx319->sd, client, &imx319_subdev_ops);
 
-	/* Check module identity */
-	ret = imx319_identify_module(imx319);
-	if (ret) {
-		dev_err(&client->dev, "failed to find sensor: %d", ret);
-		goto error_probe;
+	low_power = acpi_dev_low_power_state_probe(&client->dev);
+	if (!low_power) {
+		/* Check module identity */
+		ret = imx319_identify_module(imx319);
+		if (ret) {
+			dev_err(&client->dev, "failed to find sensor: %d", ret);
+			goto error_probe;
+		}
 	}
 
 	imx319->hwcfg = imx319_get_hwcfg(&client->dev);
@@ -2493,10 +2497,10 @@ static int imx319_probe(struct i2c_client *client)
 		goto error_media_entity;
 
 	/*
-	 * Device is already turned on by i2c-core with ACPI domain PM.
-	 * Enable runtime PM and turn off the device.
+	 * Don't set the device's state to active if it's in a low power state.
 	 */
-	pm_runtime_set_active(&client->dev);
+	if (!low_power)
+		pm_runtime_set_active(&client->dev);
 	pm_runtime_enable(&client->dev);
 	pm_runtime_idle(&client->dev);
 
@@ -2536,7 +2540,7 @@ static const struct dev_pm_ops imx319_pm_ops = {
 };
 
 static const struct acpi_device_id imx319_acpi_ids[] = {
-	{ "SONY319A" },
+	{ "SONY319A", },
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(acpi, imx319_acpi_ids);
@@ -2549,6 +2553,7 @@ static struct i2c_driver imx319_i2c_driver = {
 	},
 	.probe_new = imx319_probe,
 	.remove = imx319_remove,
+	.probe_low_power = true,
 };
 module_i2c_driver(imx319_i2c_driver);
 
-- 
2.20.1

[PATCH v3 5/5] at24: Support probing while off

[Sakari Ailus]

In certain use cases (where the chip is part of a camera module, and the
camera module is wired together with a camera privacy LED), powering on
the device during probe is undesirable. Add support for the at24 to
execute probe while being powered off. For this to happen, a hint in form
of a device property is required from the firmware.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/misc/eeprom/at24.c | 31 +++++++++++++++++++++----------
 1 file changed, 21 insertions(+), 10 deletions(-)

diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index 0681d5fdd538a..41ac65d1e5d41 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -564,6 +564,7 @@ static int at24_probe(struct i2c_client *client)
 	bool i2c_fn_i2c, i2c_fn_block;
 	unsigned int i, num_addresses;
 	struct at24_data *at24;
+	bool low_power;
 	struct regmap *regmap;
 	bool writable;
 	u8 test_byte;
@@ -701,19 +702,24 @@ static int at24_probe(struct i2c_client *client)
 
 	i2c_set_clientdata(client, at24);
 
-	/* enable runtime pm */
-	pm_runtime_set_active(dev);
+	low_power = acpi_dev_low_power_state_probe(&client->dev);
+	if (!low_power)
+		pm_runtime_set_active(dev);
+
 	pm_runtime_enable(dev);
 
 	/*
-	 * Perform a one-byte test read to verify that the
-	 * chip is functional.
+	 * Perform a one-byte test read to verify that the chip is functional,
+	 * unless powering on the device is to be avoided during probe (i.e.
+	 * it's powered off right now).
 	 */
-	err = at24_read(at24, 0, &test_byte, 1);
-	pm_runtime_idle(dev);
-	if (err) {
-		pm_runtime_disable(dev);
-		return -ENODEV;
+	if (!low_power) {
+		err = at24_read(at24, 0, &test_byte, 1);
+		pm_runtime_idle(dev);
+		if (err) {
+			pm_runtime_disable(dev);
+			return -ENODEV;
+		}
 	}
 
 	if (writable)
@@ -728,8 +734,12 @@ static int at24_probe(struct i2c_client *client)
 
 static int at24_remove(struct i2c_client *client)
 {
+	bool low_power;
+
 	pm_runtime_disable(&client->dev);
-	pm_runtime_set_suspended(&client->dev);
+	low_power = acpi_dev_low_power_state_probe(&client->dev);
+	if (!low_power)
+		pm_runtime_set_suspended(&client->dev);
 
 	return 0;
 }
@@ -743,6 +753,7 @@ static struct i2c_driver at24_driver = {
 	.probe_new = at24_probe,
 	.remove = at24_remove,
 	.id_table = at24_ids,
+	.probe_low_power = true,
 };
 
 static int __init at24_init(void)
-- 
2.20.1

Re: [PATCH v3 5/5] at24: Support probing while off

[Bartosz Golaszewski]

czw., 9 sty 2020 o 16:44 Sakari Ailus <sakari.ailus@linux.intel.com> napisał(a):
>
> In certain use cases (where the chip is part of a camera module, and the
> camera module is wired together with a camera privacy LED), powering on
> the device during probe is undesirable. Add support for the at24 to
> execute probe while being powered off. For this to happen, a hint in form
> of a device property is required from the firmware.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

Why am I not Cc'ed on this patch?

Bart

Re: [PATCH v3 5/5] at24: Support probing while off

[Sakari Ailus]

Hi Bartosz,

On Fri, Jan 10, 2020 at 12:16:14PM +0100, Bartosz Golaszewski wrote:
> czw., 9 sty 2020 o 16:44 Sakari Ailus <sakari.ailus@linux.intel.com> napisał(a):
> >
> > In certain use cases (where the chip is part of a camera module, and the
> > camera module is wired together with a camera privacy LED), powering on
> > the device during probe is undesirable. Add support for the at24 to
> > execute probe while being powered off. For this to happen, a hint in form
> > of a device property is required from the firmware.
> >
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> 
> Why am I not Cc'ed on this patch?

I'll make sure you'll be cc'd on any future versions of the patch.

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v3 5/5] at24: Support probing while off

[Bartosz Golaszewski]

pt., 10 sty 2020 o 12:28 Sakari Ailus <sakari.ailus@linux.intel.com> napisał(a):
>
> Hi Bartosz,
>
> On Fri, Jan 10, 2020 at 12:16:14PM +0100, Bartosz Golaszewski wrote:
> > czw., 9 sty 2020 o 16:44 Sakari Ailus <sakari.ailus@linux.intel.com> napisał(a):
> > >
> > > In certain use cases (where the chip is part of a camera module, and the
> > > camera module is wired together with a camera privacy LED), powering on
> > > the device during probe is undesirable. Add support for the at24 to
> > > execute probe while being powered off. For this to happen, a hint in form
> > > of a device property is required from the firmware.
> > >
> > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> >
> > Why am I not Cc'ed on this patch?
>
> I'll make sure you'll be cc'd on any future versions of the patch.
>

Thanks. Please make sure this applies on top of my for-next branch[1].
We've had a couple changes in the driver this cycle.

Bart

[1] git://git.kernel.org/pub/scm/linux/kernel/git/brgl/linux.git

Re: [PATCH v3 2/5] ACPI: Add a convenience function to tell a device is suspended in probe

[Rafael J. Wysocki]

On Thu, Jan 9, 2020 at 4:44 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Add a convenience function to tell whether a device is suspended for probe
> or remove, for busses where the custom is that drivers don't need to
> resume devices in probe, or suspend them in their remove handlers.
>
> Returns false on non-ACPI systems.
>
> Suggested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/acpi/device_pm.c | 35 +++++++++++++++++++++++++++++++++++
>  include/linux/acpi.h     |  5 +++++
>  2 files changed, 40 insertions(+)
>
> diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
> index 5e4a8860a9c0c..87393020276d8 100644
> --- a/drivers/acpi/device_pm.c
> +++ b/drivers/acpi/device_pm.c
> @@ -1348,4 +1348,39 @@ int acpi_dev_pm_attach(struct device *dev, bool power_on)
>         return 1;
>  }
>  EXPORT_SYMBOL_GPL(acpi_dev_pm_attach);
> +
> +/**
> + * acpi_dev_low_power_state_probe - Tell if a device is in a low power state

"Check the current ACPI power state of a device."

> + *                                 during probe

Why is this limited to probe?

The function actually checks whether or not the ACPI power state of
the device is low-power at the call time (except that it is a bit racy
with respect to _set_power(), so it may not work as expected if called
in parallel with that one).

Maybe drop the "probe" part of the name (actually, I would call this
function acpi_dev_state_low_power()) and add a paragraph about the
potential race with _set_power() to the description?

> + * @dev: The device

"Physical device the ACPI power state of which to check".

> + *
> + * Tell whether a given device is in a low power state during the driver's probe
> + * or remove operation.
> + *
> + * Drivers of devices on certain busses such as I²C can generally assume (on
> + * ACPI based systems) that the devices they control are powered on without
> + * driver having to do anything about it. Using struct
> + * device_driver.probe_low_power and "probe-low-power" property, this can be
> + * negated and the driver has full control of the device power management.

The above information belongs somewhere else in my view.

> + * Always returns false on non-ACPI based systems. True is returned on ACPI

"On a system without ACPI, return false.  On a system with ACPI,
return true if the current ACPI power state of the device is not D0,
or false otherwise.

Note that the power state of a device is not well-defined after it has
been passed to acpi_device_set_power() and before that function
returns, so it is not valid to ask for the ACPI power state of the
device in that time frame."

> + * based systems iff the device is in a low power state during probe or remove.
> + */
> +bool acpi_dev_low_power_state_probe(struct device *dev)
> +{
> +       int power_state;
> +       int ret;
> +
> +       if (!is_acpi_device_node(dev_fwnode(dev)))
> +               return false;

This is (at least) inefficient, because the same check is repeated by
ACPI_COMPANION().

If you really want to print the message, it is better to do something like

struct acpi_device *adev = ACPI_COMPANION(dev);

if (!adev)
        return false;

ret = acpi_device_get_power(adev, &power_state);

> +
> +       ret = acpi_device_get_power(ACPI_COMPANION(dev), &power_state);
> +       if (ret) {
> +               dev_warn(dev, "Cannot obtain power state (%d)\n", ret);

And the log level of this message is way too high IMO.

This means a firmware bug AFAICS and so after seeing it once on a
given system it becomes noise.  I'd use pr_debug() to print it.

> +               return false;
> +       }
> +
> +       return power_state != ACPI_STATE_D0;
> +}
> +EXPORT_SYMBOL_GPL(acpi_dev_low_power_state_probe);
> +
>  #endif /* CONFIG_PM */

Re: [PATCH v3 2/5] ACPI: Add a convenience function to tell a device is suspended in probe

[Sakari Ailus]

Hi Rafael,

Thank you for the review.

On Mon, Jan 13, 2020 at 11:41:12AM +0100, Rafael J. Wysocki wrote:
> On Thu, Jan 9, 2020 at 4:44 PM Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
> >
> > Add a convenience function to tell whether a device is suspended for probe
> > or remove, for busses where the custom is that drivers don't need to
> > resume devices in probe, or suspend them in their remove handlers.
> >
> > Returns false on non-ACPI systems.
> >
> > Suggested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> >  drivers/acpi/device_pm.c | 35 +++++++++++++++++++++++++++++++++++
> >  include/linux/acpi.h     |  5 +++++
> >  2 files changed, 40 insertions(+)
> >
> > diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
> > index 5e4a8860a9c0c..87393020276d8 100644
> > --- a/drivers/acpi/device_pm.c
> > +++ b/drivers/acpi/device_pm.c
> > @@ -1348,4 +1348,39 @@ int acpi_dev_pm_attach(struct device *dev, bool power_on)
> >         return 1;
> >  }
> >  EXPORT_SYMBOL_GPL(acpi_dev_pm_attach);
> > +
> > +/**
> > + * acpi_dev_low_power_state_probe - Tell if a device is in a low power state
> 
> "Check the current ACPI power state of a device."

Sounds good.

> 
> > + *                                 during probe
> 
> Why is this limited to probe?

Well.. that was the purpose. It could be used at other times, too, I guess,
but most of the time runtime PM is the right interface for doing that.

> 
> The function actually checks whether or not the ACPI power state of
> the device is low-power at the call time (except that it is a bit racy
> with respect to _set_power(), so it may not work as expected if called
> in parallel with that one).
> 
> Maybe drop the "probe" part of the name (actually, I would call this
> function acpi_dev_state_low_power()) and add a paragraph about the
> potential race with _set_power() to the description?

Agreed, I'll use the text you provided below.

> 
> > + * @dev: The device
> 
> "Physical device the ACPI power state of which to check".

Ok.

> 
> > + *
> > + * Tell whether a given device is in a low power state during the driver's probe
> > + * or remove operation.
> > + *
> > + * Drivers of devices on certain busses such as I²C can generally assume (on
> > + * ACPI based systems) that the devices they control are powered on without
> > + * driver having to do anything about it. Using struct
> > + * device_driver.probe_low_power and "probe-low-power" property, this can be
> > + * negated and the driver has full control of the device power management.
> 
> The above information belongs somewhere else in my view.

How about putting it to the DSD ReST property documentation, perhaps with a
little bit more context? I can add another patch for that.

> 
> > + * Always returns false on non-ACPI based systems. True is returned on ACPI
> 
> "On a system without ACPI, return false.  On a system with ACPI,
> return true if the current ACPI power state of the device is not D0,
> or false otherwise.
> 
> Note that the power state of a device is not well-defined after it has
> been passed to acpi_device_set_power() and before that function
> returns, so it is not valid to ask for the ACPI power state of the
> device in that time frame."

Works for me.

> 
> > + * based systems iff the device is in a low power state during probe or remove.
> > + */
> > +bool acpi_dev_low_power_state_probe(struct device *dev)
> > +{
> > +       int power_state;
> > +       int ret;
> > +
> > +       if (!is_acpi_device_node(dev_fwnode(dev)))
> > +               return false;
> 
> This is (at least) inefficient, because the same check is repeated by
> ACPI_COMPANION().
> 
> If you really want to print the message, it is better to do something like
> 
> struct acpi_device *adev = ACPI_COMPANION(dev);
> 
> if (!adev)
>         return false;
> 
> ret = acpi_device_get_power(adev, &power_state);

Yes, makes sense.

> 
> > +
> > +       ret = acpi_device_get_power(ACPI_COMPANION(dev), &power_state);
> > +       if (ret) {
> > +               dev_warn(dev, "Cannot obtain power state (%d)\n", ret);
> 
> And the log level of this message is way too high IMO.
> 
> This means a firmware bug AFAICS and so after seeing it once on a
> given system it becomes noise.  I'd use pr_debug() to print it.

I'll switch to dev_dbg() then --- as we have the device.

> 
> > +               return false;
> > +       }
> > +
> > +       return power_state != ACPI_STATE_D0;
> > +}
> > +EXPORT_SYMBOL_GPL(acpi_dev_low_power_state_probe);
> > +
> >  #endif /* CONFIG_PM */

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v3 2/5] ACPI: Add a convenience function to tell a device is suspended in probe

[Rafael J. Wysocki]

On Tue, Jan 21, 2020 at 10:09 AM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Hi Rafael,
>
> Thank you for the review.
>
> On Mon, Jan 13, 2020 at 11:41:12AM +0100, Rafael J. Wysocki wrote:
> > On Thu, Jan 9, 2020 at 4:44 PM Sakari Ailus
> > <sakari.ailus@linux.intel.com> wrote:
> > >
> > > Add a convenience function to tell whether a device is suspended for probe
> > > or remove, for busses where the custom is that drivers don't need to
> > > resume devices in probe, or suspend them in their remove handlers.
> > >
> > > Returns false on non-ACPI systems.
> > >
> > > Suggested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > ---
> > >  drivers/acpi/device_pm.c | 35 +++++++++++++++++++++++++++++++++++
> > >  include/linux/acpi.h     |  5 +++++
> > >  2 files changed, 40 insertions(+)
> > >
> > > diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
> > > index 5e4a8860a9c0c..87393020276d8 100644
> > > --- a/drivers/acpi/device_pm.c
> > > +++ b/drivers/acpi/device_pm.c
> > > @@ -1348,4 +1348,39 @@ int acpi_dev_pm_attach(struct device *dev, bool power_on)
> > >         return 1;
> > >  }
> > >  EXPORT_SYMBOL_GPL(acpi_dev_pm_attach);
> > > +
> > > +/**
> > > + * acpi_dev_low_power_state_probe - Tell if a device is in a low power state
> >
> > "Check the current ACPI power state of a device."
>
> Sounds good.
>
> >
> > > + *                                 during probe
> >
> > Why is this limited to probe?
>
> Well.. that was the purpose. It could be used at other times, too, I guess,
> but most of the time runtime PM is the right interface for doing that.

PM-runtime is a layer above this one.

It is mostly about the coordination between devices, reference
counting etc which this is about device power states.

> >
> > The function actually checks whether or not the ACPI power state of
> > the device is low-power at the call time (except that it is a bit racy
> > with respect to _set_power(), so it may not work as expected if called
> > in parallel with that one).
> >
> > Maybe drop the "probe" part of the name (actually, I would call this
> > function acpi_dev_state_low_power()) and add a paragraph about the
> > potential race with _set_power() to the description?
>
> Agreed, I'll use the text you provided below.
>
> >
> > > + * @dev: The device
> >
> > "Physical device the ACPI power state of which to check".
>
> Ok.
>
> >
> > > + *
> > > + * Tell whether a given device is in a low power state during the driver's probe
> > > + * or remove operation.
> > > + *
> > > + * Drivers of devices on certain busses such as I涎 can generally assume (on
> > > + * ACPI based systems) that the devices they control are powered on without
> > > + * driver having to do anything about it. Using struct
> > > + * device_driver.probe_low_power and "probe-low-power" property, this can be
> > > + * negated and the driver has full control of the device power management.
> >
> > The above information belongs somewhere else in my view.
>
> How about putting it to the DSD ReST property documentation, perhaps with a
> little bit more context? I can add another patch for that.

Yes, something like that.