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

[PATCH v2 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.

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):
  ACPI: Enable driver and firmware hints to control power at probe time
  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   | 50 ++++++++++++++++++++++++++++++++++++--
 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/device.h     |  7 ++++++
 6 files changed, 109 insertions(+), 30 deletions(-)

-- 
2.20.1

[PATCH v2 1/5] ACPI: Enable driver and firmware hints to control power at probe time

[Sakari Ailus]

Allow drivers and firmware tell ACPI that there's no need to power on a
device for probe. This requires both a hint from the firmware as well as
an indication from a driver to leave the device off.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/acpi/device_pm.c | 15 +++++++++++++--
 include/linux/device.h   |  7 +++++++
 2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
index f616b16c1f0be..adcdf78ce4de8 100644
--- a/drivers/acpi/device_pm.c
+++ b/drivers/acpi/device_pm.c
@@ -1276,7 +1276,12 @@ static void acpi_dev_pm_detach(struct device *dev, bool power_off)
 	if (adev && dev->pm_domain == &acpi_general_pm_domain) {
 		dev_pm_domain_set(dev, NULL);
 		acpi_remove_pm_notifier(adev);
-		if (power_off) {
+		if (power_off
+#ifdef CONFIG_PM
+		    && !(dev->driver->probe_low_power &&
+			 device_property_present(dev, "probe-low-power"))
+#endif
+			) {
 			/*
 			 * If the device's PM QoS resume latency limit or flags
 			 * have been exposed to user space, they have to be
@@ -1324,7 +1329,13 @@ int acpi_dev_pm_attach(struct device *dev, bool power_on)
 
 	acpi_add_pm_notifier(adev, dev, acpi_pm_notify_work_func);
 	dev_pm_domain_set(dev, &acpi_general_pm_domain);
-	if (power_on) {
+
+	if (power_on
+#ifdef CONFIG_PM
+	    && !(dev->driver->probe_low_power &&
+		 device_property_present(dev, "probe-low-power"))
+#endif
+		) {
 		acpi_dev_pm_full_power(adev);
 		acpi_device_wakeup_disable(adev);
 	}
diff --git a/include/linux/device.h b/include/linux/device.h
index 6717adee33f01..4bc0ea4a3201a 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -248,6 +248,12 @@ enum probe_type {
  * @owner:	The module owner.
  * @mod_name:	Used for built-in modules.
  * @suppress_bind_attrs: Disables bind/unbind via sysfs.
+ * @probe_low_power: The driver supports its probe function being called while
+ *		     the device is in a low power state, independently of the
+ *		     expected behaviour on combination of a given bus and
+ *		     firmware interface etc. The driver is responsible for
+ *		     powering the device on using runtime PM in such case.
+ *		     This configuration has no effect if CONFIG_PM is disabled.
  * @probe_type:	Type of the probe (synchronous or asynchronous) to use.
  * @of_match_table: The open firmware table.
  * @acpi_match_table: The ACPI match table.
@@ -285,6 +291,7 @@ struct device_driver {
 	const char		*mod_name;	/* used for built-in modules */
 
 	bool suppress_bind_attrs;	/* disables bind/unbind via sysfs */
+	bool probe_low_power;
 	enum probe_type probe_type;
 
 	const struct of_device_id	*of_match_table;
-- 
2.20.1

[PATCH v2 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 adcdf78ce4de8..7d6b40396ea71 100644
--- a/drivers/acpi/device_pm.c
+++ b/drivers/acpi/device_pm.c
@@ -1344,4 +1344,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 9426b9aaed86f..a20b4246107ac 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -912,6 +912,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; }
@@ -921,6 +922,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 v2 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..760fce93337cd 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 */ }
 };
 
@@ -2556,6 +2560,7 @@ MODULE_DEVICE_TABLE(acpi, ov5670_acpi_ids);
 static struct i2c_driver ov5670_i2c_driver = {
 	.driver = {
 		.name = "ov5670",
+		.probe_low_power = true,
 		.pm = &ov5670_pm_ops,
 		.acpi_match_table = ACPI_PTR(ov5670_acpi_ids),
 	},
-- 
2.20.1

[PATCH v2 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..1a01946f4d5bf 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);
@@ -2544,6 +2548,7 @@ MODULE_DEVICE_TABLE(acpi, imx319_acpi_ids);
 static struct i2c_driver imx319_i2c_driver = {
 	.driver = {
 		.name = "imx319",
+		.probe_low_power = true,
 		.pm = &imx319_pm_ops,
 		.acpi_match_table = ACPI_PTR(imx319_acpi_ids),
 	},
-- 
2.20.1

[PATCH v2 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 518945b2f7374..a56cd6a6105c0 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -565,6 +565,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;
@@ -702,19 +703,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;
+		}
 	}
 
 	dev_info(dev, "%u byte %s EEPROM, %s, %u bytes/write\n",
@@ -726,8 +732,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;
 }
@@ -735,6 +745,7 @@ static int at24_remove(struct i2c_client *client)
 static struct i2c_driver at24_driver = {
 	.driver = {
 		.name = "at24",
+		.probe_low_power = true,
 		.of_match_table = at24_of_match,
 		.acpi_match_table = ACPI_PTR(at24_acpi_ids),
 	},
-- 
2.20.1

Re: [PATCH v2 1/5] ACPI: Enable driver and firmware hints to control power at probe time

[Greg Kroah-Hartman]

On Mon, Aug 26, 2019 at 11:31:08AM +0300, Sakari Ailus wrote:
> Allow drivers and firmware tell ACPI that there's no need to power on a
> device for probe. This requires both a hint from the firmware as well as
> an indication from a driver to leave the device off.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/acpi/device_pm.c | 15 +++++++++++++--
>  include/linux/device.h   |  7 +++++++
>  2 files changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
> index f616b16c1f0be..adcdf78ce4de8 100644
> --- a/drivers/acpi/device_pm.c
> +++ b/drivers/acpi/device_pm.c
> @@ -1276,7 +1276,12 @@ static void acpi_dev_pm_detach(struct device *dev, bool power_off)
>  	if (adev && dev->pm_domain == &acpi_general_pm_domain) {
>  		dev_pm_domain_set(dev, NULL);
>  		acpi_remove_pm_notifier(adev);
> -		if (power_off) {
> +		if (power_off
> +#ifdef CONFIG_PM
> +		    && !(dev->driver->probe_low_power &&
> +			 device_property_present(dev, "probe-low-power"))
> +#endif
> +			) {
>  			/*
>  			 * If the device's PM QoS resume latency limit or flags
>  			 * have been exposed to user space, they have to be
> @@ -1324,7 +1329,13 @@ int acpi_dev_pm_attach(struct device *dev, bool power_on)
>  
>  	acpi_add_pm_notifier(adev, dev, acpi_pm_notify_work_func);
>  	dev_pm_domain_set(dev, &acpi_general_pm_domain);
> -	if (power_on) {
> +
> +	if (power_on
> +#ifdef CONFIG_PM
> +	    && !(dev->driver->probe_low_power &&
> +		 device_property_present(dev, "probe-low-power"))
> +#endif
> +		) {
>  		acpi_dev_pm_full_power(adev);
>  		acpi_device_wakeup_disable(adev);
>  	}
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 6717adee33f01..4bc0ea4a3201a 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -248,6 +248,12 @@ enum probe_type {
>   * @owner:	The module owner.
>   * @mod_name:	Used for built-in modules.
>   * @suppress_bind_attrs: Disables bind/unbind via sysfs.
> + * @probe_low_power: The driver supports its probe function being called while
> + *		     the device is in a low power state, independently of the
> + *		     expected behaviour on combination of a given bus and
> + *		     firmware interface etc. The driver is responsible for
> + *		     powering the device on using runtime PM in such case.
> + *		     This configuration has no effect if CONFIG_PM is disabled.
>   * @probe_type:	Type of the probe (synchronous or asynchronous) to use.
>   * @of_match_table: The open firmware table.
>   * @acpi_match_table: The ACPI match table.
> @@ -285,6 +291,7 @@ struct device_driver {
>  	const char		*mod_name;	/* used for built-in modules */
>  
>  	bool suppress_bind_attrs;	/* disables bind/unbind via sysfs */
> +	bool probe_low_power;

Ick, no, this should be a bus-specific thing to handle such messed up
hardware.  Why polute this in the driver core?

thanks,

greg k-h

Re: [PATCH v2 1/5] ACPI: Enable driver and firmware hints to control power at probe time

[Greg Kroah-Hartman]

On Mon, Aug 26, 2019 at 11:31:08AM +0300, Sakari Ailus wrote:
> Allow drivers and firmware tell ACPI that there's no need to power on a
> device for probe. This requires both a hint from the firmware as well as
> an indication from a driver to leave the device off.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/acpi/device_pm.c | 15 +++++++++++++--
>  include/linux/device.h   |  7 +++++++
>  2 files changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
> index f616b16c1f0be..adcdf78ce4de8 100644
> --- a/drivers/acpi/device_pm.c
> +++ b/drivers/acpi/device_pm.c
> @@ -1276,7 +1276,12 @@ static void acpi_dev_pm_detach(struct device *dev, bool power_off)
>  	if (adev && dev->pm_domain == &acpi_general_pm_domain) {
>  		dev_pm_domain_set(dev, NULL);
>  		acpi_remove_pm_notifier(adev);
> -		if (power_off) {
> +		if (power_off
> +#ifdef CONFIG_PM
> +		    && !(dev->driver->probe_low_power &&
> +			 device_property_present(dev, "probe-low-power"))
> +#endif

As proof of the "only a bus-specific thing", why is probe_low_power even
needed?  Why not just always trigger off of this crazy device_property?
That makes the driver changes less.

Also, is this #ifdef really needed?

thanks,

greg k-h

Re: [PATCH v2 1/5] ACPI: Enable driver and firmware hints to control power at probe time

[Sakari Ailus]

Hi Greg,

Thanks for the comments.

On Mon, Aug 26, 2019 at 10:46:34AM +0200, Greg Kroah-Hartman wrote:
> On Mon, Aug 26, 2019 at 11:31:08AM +0300, Sakari Ailus wrote:
> > Allow drivers and firmware tell ACPI that there's no need to power on a
> > device for probe. This requires both a hint from the firmware as well as
> > an indication from a driver to leave the device off.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> >  drivers/acpi/device_pm.c | 15 +++++++++++++--
> >  include/linux/device.h   |  7 +++++++
> >  2 files changed, 20 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
> > index f616b16c1f0be..adcdf78ce4de8 100644
> > --- a/drivers/acpi/device_pm.c
> > +++ b/drivers/acpi/device_pm.c
> > @@ -1276,7 +1276,12 @@ static void acpi_dev_pm_detach(struct device *dev, bool power_off)
> >  	if (adev && dev->pm_domain == &acpi_general_pm_domain) {
> >  		dev_pm_domain_set(dev, NULL);
> >  		acpi_remove_pm_notifier(adev);
> > -		if (power_off) {
> > +		if (power_off
> > +#ifdef CONFIG_PM
> > +		    && !(dev->driver->probe_low_power &&
> > +			 device_property_present(dev, "probe-low-power"))
> > +#endif
> 
> As proof of the "only a bus-specific thing", why is probe_low_power even
> needed?  Why not just always trigger off of this crazy device_property?
> That makes the driver changes less.

That's an option, too, but firmware having this property for a device the
driver of which doesn't expect it will fail to power on the device for
probe. This leaves some room for unexpected failures that admittedly are
easy to fix, but could be harder to debug.

> 
> Also, is this #ifdef really needed?

I thought it was but it seems if CONFIG_PM is disabled,
dev_pm_domain_attach() has a nop implementation. So I agree it is not.

-- 
Regards,

Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH v2 1/5] ACPI: Enable driver and firmware hints to control power at probe time

[Sakari Ailus]

Hi Greg,

On Mon, Aug 26, 2019 at 10:43:43AM +0200, Greg Kroah-Hartman wrote:

...

> > diff --git a/include/linux/device.h b/include/linux/device.h
> > index 6717adee33f01..4bc0ea4a3201a 100644
> > --- a/include/linux/device.h
> > +++ b/include/linux/device.h
> > @@ -248,6 +248,12 @@ enum probe_type {
> >   * @owner:	The module owner.
> >   * @mod_name:	Used for built-in modules.
> >   * @suppress_bind_attrs: Disables bind/unbind via sysfs.
> > + * @probe_low_power: The driver supports its probe function being called while
> > + *		     the device is in a low power state, independently of the
> > + *		     expected behaviour on combination of a given bus and
> > + *		     firmware interface etc. The driver is responsible for
> > + *		     powering the device on using runtime PM in such case.
> > + *		     This configuration has no effect if CONFIG_PM is disabled.
> >   * @probe_type:	Type of the probe (synchronous or asynchronous) to use.
> >   * @of_match_table: The open firmware table.
> >   * @acpi_match_table: The ACPI match table.
> > @@ -285,6 +291,7 @@ struct device_driver {
> >  	const char		*mod_name;	/* used for built-in modules */
> >  
> >  	bool suppress_bind_attrs;	/* disables bind/unbind via sysfs */
> > +	bool probe_low_power;
> 
> Ick, no, this should be a bus-specific thing to handle such messed up
> hardware.  Why polute this in the driver core?

The alternative could be to make it I²C specific indeed; the vast majority
of camera sensors are I²C devices these days.

-- 
Regards,

Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH v2 1/5] ACPI: Enable driver and firmware hints to control power at probe time

[Greg Kroah-Hartman]

On Mon, Aug 26, 2019 at 01:32:00PM +0300, Sakari Ailus wrote:
> Hi Greg,
> 
> On Mon, Aug 26, 2019 at 10:43:43AM +0200, Greg Kroah-Hartman wrote:
> 
> ...
> 
> > > diff --git a/include/linux/device.h b/include/linux/device.h
> > > index 6717adee33f01..4bc0ea4a3201a 100644
> > > --- a/include/linux/device.h
> > > +++ b/include/linux/device.h
> > > @@ -248,6 +248,12 @@ enum probe_type {
> > >   * @owner:	The module owner.
> > >   * @mod_name:	Used for built-in modules.
> > >   * @suppress_bind_attrs: Disables bind/unbind via sysfs.
> > > + * @probe_low_power: The driver supports its probe function being called while
> > > + *		     the device is in a low power state, independently of the
> > > + *		     expected behaviour on combination of a given bus and
> > > + *		     firmware interface etc. The driver is responsible for
> > > + *		     powering the device on using runtime PM in such case.
> > > + *		     This configuration has no effect if CONFIG_PM is disabled.
> > >   * @probe_type:	Type of the probe (synchronous or asynchronous) to use.
> > >   * @of_match_table: The open firmware table.
> > >   * @acpi_match_table: The ACPI match table.
> > > @@ -285,6 +291,7 @@ struct device_driver {
> > >  	const char		*mod_name;	/* used for built-in modules */
> > >  
> > >  	bool suppress_bind_attrs;	/* disables bind/unbind via sysfs */
> > > +	bool probe_low_power;
> > 
> > Ick, no, this should be a bus-specific thing to handle such messed up
> > hardware.  Why polute this in the driver core?
> 
> The alternative could be to make it I²C specific indeed; the vast majority
> of camera sensors are I²C devices these days.

Why is this even needed to be a bus/device attribute at all?  You are
checking the firmware property in the probe function, just do the logic
there as you are, what needs to be saved to the bus's logic?

thanks,

greg k-h

Re: [PATCH v2 1/5] ACPI: Enable driver and firmware hints to control power at probe time

[Sakari Ailus]

On Mon, Aug 26, 2019 at 03:34:39PM +0200, Greg Kroah-Hartman wrote:
> On Mon, Aug 26, 2019 at 01:32:00PM +0300, Sakari Ailus wrote:
> > Hi Greg,
> > 
> > On Mon, Aug 26, 2019 at 10:43:43AM +0200, Greg Kroah-Hartman wrote:
> > 
> > ...
> > 
> > > > diff --git a/include/linux/device.h b/include/linux/device.h
> > > > index 6717adee33f01..4bc0ea4a3201a 100644
> > > > --- a/include/linux/device.h
> > > > +++ b/include/linux/device.h
> > > > @@ -248,6 +248,12 @@ enum probe_type {
> > > >   * @owner:	The module owner.
> > > >   * @mod_name:	Used for built-in modules.
> > > >   * @suppress_bind_attrs: Disables bind/unbind via sysfs.
> > > > + * @probe_low_power: The driver supports its probe function being called while
> > > > + *		     the device is in a low power state, independently of the
> > > > + *		     expected behaviour on combination of a given bus and
> > > > + *		     firmware interface etc. The driver is responsible for
> > > > + *		     powering the device on using runtime PM in such case.
> > > > + *		     This configuration has no effect if CONFIG_PM is disabled.
> > > >   * @probe_type:	Type of the probe (synchronous or asynchronous) to use.
> > > >   * @of_match_table: The open firmware table.
> > > >   * @acpi_match_table: The ACPI match table.
> > > > @@ -285,6 +291,7 @@ struct device_driver {
> > > >  	const char		*mod_name;	/* used for built-in modules */
> > > >  
> > > >  	bool suppress_bind_attrs;	/* disables bind/unbind via sysfs */
> > > > +	bool probe_low_power;
> > > 
> > > Ick, no, this should be a bus-specific thing to handle such messed up
> > > hardware.  Why polute this in the driver core?
> > 
> > The alternative could be to make it I²C specific indeed; the vast majority
> > of camera sensors are I²C devices these days.
> 
> Why is this even needed to be a bus/device attribute at all?  You are
> checking the firmware property in the probe function, just do the logic
> there as you are, what needs to be saved to the bus's logic?

By the time the driver gets hold of the device, or gets to control its
power state, the I²C framework has already called dev_pm_domain_attach() to
power on the device. The I²C drivers on ACPI based systems expect the
device to be powered on for probe.

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH v2 1/5] ACPI: Enable driver and firmware hints to control power at probe time

[Rafael J. Wysocki]

On Mon, Aug 26, 2019 at 3:34 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Mon, Aug 26, 2019 at 01:32:00PM +0300, Sakari Ailus wrote:
> > Hi Greg,
> >
> > On Mon, Aug 26, 2019 at 10:43:43AM +0200, Greg Kroah-Hartman wrote:
> >
> > ...
> >
> > > > diff --git a/include/linux/device.h b/include/linux/device.h
> > > > index 6717adee33f01..4bc0ea4a3201a 100644
> > > > --- a/include/linux/device.h
> > > > +++ b/include/linux/device.h
> > > > @@ -248,6 +248,12 @@ enum probe_type {
> > > >   * @owner:       The module owner.
> > > >   * @mod_name:    Used for built-in modules.
> > > >   * @suppress_bind_attrs: Disables bind/unbind via sysfs.
> > > > + * @probe_low_power: The driver supports its probe function being called while
> > > > + *                    the device is in a low power state, independently of the
> > > > + *                    expected behaviour on combination of a given bus and
> > > > + *                    firmware interface etc. The driver is responsible for
> > > > + *                    powering the device on using runtime PM in such case.
> > > > + *                    This configuration has no effect if CONFIG_PM is disabled.
> > > >   * @probe_type:  Type of the probe (synchronous or asynchronous) to use.
> > > >   * @of_match_table: The open firmware table.
> > > >   * @acpi_match_table: The ACPI match table.
> > > > @@ -285,6 +291,7 @@ struct device_driver {
> > > >   const char              *mod_name;      /* used for built-in modules */
> > > >
> > > >   bool suppress_bind_attrs;       /* disables bind/unbind via sysfs */
> > > > + bool probe_low_power;
> > >
> > > Ick, no, this should be a bus-specific thing to handle such messed up
> > > hardware.  Why polute this in the driver core?
> >
> > The alternative could be to make it I²C specific indeed; the vast majority
> > of camera sensors are I²C devices these days.
>
> Why is this even needed to be a bus/device attribute at all?  You are
> checking the firmware property in the probe function, just do the logic
> there as you are, what needs to be saved to the bus's logic?

The situation today is that all devices are put into D0 by the ACPI
layer before driver probing since drivers generally expect devices to
be in D0 when their probe routines run.  If the driver is prepared to
cope with devices in low-power states, though, powering them up before
probing for a driver may not be necessary, but still the core (or
generally the code invoking the driver probe) needs to know that the
driver really is prepared for that.  Hence the driver flag AFAICS.

Now, in theory there may be some platform requirements regarding the
power states of devices during driver probe, although admittedly it is
not entirely clear to me why that would be the case) and hence the
property.  I would think that if the driver could cope with devices in
low-power states during probe, the platform wouldn't need to worry
about that.

Re: [PATCH v2 1/5] ACPI: Enable driver and firmware hints to control power at probe time

[Sakari Ailus]

Hi Rafael,

On Wed, Aug 28, 2019 at 10:55:42AM +0200, Rafael J. Wysocki wrote:
> On Mon, Aug 26, 2019 at 3:34 PM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Mon, Aug 26, 2019 at 01:32:00PM +0300, Sakari Ailus wrote:
> > > Hi Greg,
> > >
> > > On Mon, Aug 26, 2019 at 10:43:43AM +0200, Greg Kroah-Hartman wrote:
> > >
> > > ...
> > >
> > > > > diff --git a/include/linux/device.h b/include/linux/device.h
> > > > > index 6717adee33f01..4bc0ea4a3201a 100644
> > > > > --- a/include/linux/device.h
> > > > > +++ b/include/linux/device.h
> > > > > @@ -248,6 +248,12 @@ enum probe_type {
> > > > >   * @owner:       The module owner.
> > > > >   * @mod_name:    Used for built-in modules.
> > > > >   * @suppress_bind_attrs: Disables bind/unbind via sysfs.
> > > > > + * @probe_low_power: The driver supports its probe function being called while
> > > > > + *                    the device is in a low power state, independently of the
> > > > > + *                    expected behaviour on combination of a given bus and
> > > > > + *                    firmware interface etc. The driver is responsible for
> > > > > + *                    powering the device on using runtime PM in such case.
> > > > > + *                    This configuration has no effect if CONFIG_PM is disabled.
> > > > >   * @probe_type:  Type of the probe (synchronous or asynchronous) to use.
> > > > >   * @of_match_table: The open firmware table.
> > > > >   * @acpi_match_table: The ACPI match table.
> > > > > @@ -285,6 +291,7 @@ struct device_driver {
> > > > >   const char              *mod_name;      /* used for built-in modules */
> > > > >
> > > > >   bool suppress_bind_attrs;       /* disables bind/unbind via sysfs */
> > > > > + bool probe_low_power;
> > > >
> > > > Ick, no, this should be a bus-specific thing to handle such messed up
> > > > hardware.  Why polute this in the driver core?
> > >
> > > The alternative could be to make it I²C specific indeed; the vast majority
> > > of camera sensors are I²C devices these days.
> >
> > Why is this even needed to be a bus/device attribute at all?  You are
> > checking the firmware property in the probe function, just do the logic
> > there as you are, what needs to be saved to the bus's logic?
> 
> The situation today is that all devices are put into D0 by the ACPI
> layer before driver probing since drivers generally expect devices to
> be in D0 when their probe routines run.  If the driver is prepared to
> cope with devices in low-power states, though, powering them up before
> probing for a driver may not be necessary, but still the core (or
> generally the code invoking the driver probe) needs to know that the
> driver really is prepared for that.  Hence the driver flag AFAICS.
> 
> Now, in theory there may be some platform requirements regarding the
> power states of devices during driver probe, although admittedly it is
> not entirely clear to me why that would be the case) and hence the

Please see the cover page of the set (also here):

<URL:https://lkml.org/lkml/2019/8/26/175>

> property.  I would think that if the driver could cope with devices in
> low-power states during probe, the platform wouldn't need to worry
> about that.

I understand this as driver deciding whether it'd power on the device
during probe.

That way there's no way to judge whether the device is accessible, and
probe would succeed without an error, which then manifests itself on the
first access of the device. Such as on the at24 EEPROM driver, the error
would take place on first read of the contents, not in probe.

Somebody might consider that as a driver bug.

-- 
Kind regards,

Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH v2 1/5] ACPI: Enable driver and firmware hints to control power at probe time

[Rafael J. Wysocki]

On Wed, Aug 28, 2019 at 11:57 AM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Hi Rafael,
>
> On Wed, Aug 28, 2019 at 10:55:42AM +0200, Rafael J. Wysocki wrote:
> > On Mon, Aug 26, 2019 at 3:34 PM Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Mon, Aug 26, 2019 at 01:32:00PM +0300, Sakari Ailus wrote:
> > > > Hi Greg,
> > > >
> > > > On Mon, Aug 26, 2019 at 10:43:43AM +0200, Greg Kroah-Hartman wrote:
> > > >
> > > > ...
> > > >
> > > > > > diff --git a/include/linux/device.h b/include/linux/device.h
> > > > > > index 6717adee33f01..4bc0ea4a3201a 100644
> > > > > > --- a/include/linux/device.h
> > > > > > +++ b/include/linux/device.h
> > > > > > @@ -248,6 +248,12 @@ enum probe_type {
> > > > > >   * @owner:       The module owner.
> > > > > >   * @mod_name:    Used for built-in modules.
> > > > > >   * @suppress_bind_attrs: Disables bind/unbind via sysfs.
> > > > > > + * @probe_low_power: The driver supports its probe function being called while
> > > > > > + *                    the device is in a low power state, independently of the
> > > > > > + *                    expected behaviour on combination of a given bus and
> > > > > > + *                    firmware interface etc. The driver is responsible for
> > > > > > + *                    powering the device on using runtime PM in such case.
> > > > > > + *                    This configuration has no effect if CONFIG_PM is disabled.
> > > > > >   * @probe_type:  Type of the probe (synchronous or asynchronous) to use.
> > > > > >   * @of_match_table: The open firmware table.
> > > > > >   * @acpi_match_table: The ACPI match table.
> > > > > > @@ -285,6 +291,7 @@ struct device_driver {
> > > > > >   const char              *mod_name;      /* used for built-in modules */
> > > > > >
> > > > > >   bool suppress_bind_attrs;       /* disables bind/unbind via sysfs */
> > > > > > + bool probe_low_power;
> > > > >
> > > > > Ick, no, this should be a bus-specific thing to handle such messed up
> > > > > hardware.  Why polute this in the driver core?
> > > >
> > > > The alternative could be to make it I²C specific indeed; the vast majority
> > > > of camera sensors are I²C devices these days.
> > >
> > > Why is this even needed to be a bus/device attribute at all?  You are
> > > checking the firmware property in the probe function, just do the logic
> > > there as you are, what needs to be saved to the bus's logic?
> >
> > The situation today is that all devices are put into D0 by the ACPI
> > layer before driver probing since drivers generally expect devices to
> > be in D0 when their probe routines run.  If the driver is prepared to
> > cope with devices in low-power states, though, powering them up before
> > probing for a driver may not be necessary, but still the core (or
> > generally the code invoking the driver probe) needs to know that the
> > driver really is prepared for that.  Hence the driver flag AFAICS.
> >
> > Now, in theory there may be some platform requirements regarding the
> > power states of devices during driver probe, although admittedly it is
> > not entirely clear to me why that would be the case) and hence the
>
> Please see the cover page of the set (also here):
>
> <URL:https://lkml.org/lkml/2019/8/26/175>
>
> > property.  I would think that if the driver could cope with devices in
> > low-power states during probe, the platform wouldn't need to worry
> > about that.
>
> I understand this as driver deciding whether it'd power on the device
> during probe.
>
> That way there's no way to judge whether the device is accessible, and
> probe would succeed without an error, which then manifests itself on the
> first access of the device.

OK, so the property really represents the platform preference in that
respect and the presence of it by no means guarantees that there won't
be any problems on the first device access.

> Such as on the at24 EEPROM driver, the error
> would take place on first read of the contents, not in probe.
>
> Somebody might consider that as a driver bug.

Well, I guess you can argue that the safer thing, which therefore
should be the default, is to power up the device before probing and to
check whether or not it is accessible at that time.  However, in some
cases it may be desirable to avoid powering up the device at that
point, whatever the reason, and the property provides a hint about
that.

Fair enough to me, but honestly I'm not sure about the example in the
cover letter. :-)