Support running driver's probe for a device powered off

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

---
 drivers/acpi/device_pm.c | 9 +++++++--
 include/linux/device.h   | 6 ++++++
 2 files changed, 13 insertions(+), 2 deletions(-)

drivers/acpi/device_pm.c +7 -2

index b859d75eaf9f6..ca2409a30d26d 100644
@@ -1225,7 +1225,9 @@ 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 &&
+		    !(dev->driver->probe_powered_off &&
+		      device_property_present(dev, "avoid-power-probe"))) {
 			/*
 			 * If the device's PM QoS resume latency limit or flags
 			 * have been exposed to user space, they have to be

@@ -1273,7 +1275,10 @@ 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 &&
+	    !(dev->driver->probe_powered_off &&
+	      device_property_present(dev, "avoid-power-probe"))) {
 		acpi_dev_pm_full_power(adev);
 		acpi_device_wakeup_disable(adev);
 	}

include/linux/device.h +5 -0

index e85264fb66161..2a459fd5b954a 100644
@@ -245,6 +245,11 @@ enum probe_type {
  * @owner:	The module owner.
  * @mod_name:	Used for built-in modules.
  * @suppress_bind_attrs: Disables bind/unbind via sysfs.
+ * @probe_powered_off: The driver supports its probe function being called while
+ *		       the device is powered off, 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.
  * @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.

include/linux/device.h +0 -0

 	enum probe_type probe_type;
 
 	const struct of_device_id	*of_match_table;

On Friday, May 10, 2019 12:09:26 PM CEST Sakari Ailus wrote:

Quoted 17 lines

include/linux/device.h +5 -0

index e85264fb66161..2a459fd5b954a 100644
@@ -245,6 +245,11 @@ enum probe_type {
  * @owner:	The module owner.
  * @mod_name:	Used for built-in modules.
  * @suppress_bind_attrs: Disables bind/unbind via sysfs.
+ * @probe_powered_off: The driver supports its probe function being called while
+ *		       the device is powered off, 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.
  * @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.

include/linux/device.h struct device_driver { · line 290

Quoted 4 lines

include/linux/device.h +1 -0

@@ -282,6 +287,7 @@ struct device_driver {
 	const char		*mod_name;	/* used for built-in modules */
 
 	bool suppress_bind_attrs;	/* disables bind/unbind via sysfs */
+	bool probe_powered_off;

This is a bit of a misnomer IMO, because it is not just about devices that are completely off. From the ACPI perspective that is about all devices not in D0, which may mean gated clocks etc.

I would call it probe_low_power or similar and analogously in patch [2/5], and apart from this I have no objections against this series, but I would suggest to CC the next iteration of it to Greg K-H and the LKML as it touches the driver core.

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

---
 drivers/media/i2c/ov5670.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

drivers/media/i2c/ov5670.c +1 -4

index 041fcbb4eebdf..57e8b92f90e09 100644
@@ -2444,6 +2444,7 @@ static int ov5670_probe(struct i2c_client *client)
 	struct ov5670 *ov5670;
 	const char *err_msg;
 	u32 input_clk = 0;
+	bool powered_off;
 	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";

Hi Sakari,

Quoted 28 lines · Sakari Ailus

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

---
 drivers/media/i2c/ov5670.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

drivers/media/i2c/ov5670.c +1 -4

index 041fcbb4eebdf..57e8b92f90e09 100644
@@ -2444,6 +2444,7 @@ static int ov5670_probe(struct i2c_client *client)
 	struct ov5670 *ov5670;
 	const char *err_msg;
 	u32 input_clk = 0;
+	bool powered_off;
 	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";

drivers/media/i2c/ov5670.c static int ov5670_probe(struct i2c_client *client) · line 2472

Quoted 10 lines

drivers/media/i2c/ov5670.c +8 -1

-		goto error_print;
+	powered_off = acpi_dev_powered_off_for_probe(&client->dev);
+	if (!powered_off) {
+		/* Check module identity */
+		ret = ov5670_identify_module(ov5670);
+		if (ret) {
+			err_msg = "ov5670_identify_module() error";
+			goto error_print;
+		}
 	}

I don't like the fact that we can't detect any hardware connection issue here anymore and we would actually get some obscure failure when we actually start streaming.

Wouldn't it be possible to still keep this behavior of not powering on the device at boot-up if no driver is bound and then have this driver built as a module and loaded later when the camera is to be used for the first time after the system boots?

Hi Tomasz,

Quoted 46 lines · Tomasz Figa

Hi Sakari,

Quoted 28 lines · Sakari Ailus

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

---
 drivers/media/i2c/ov5670.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

drivers/media/i2c/ov5670.c +1 -4

index 041fcbb4eebdf..57e8b92f90e09 100644
@@ -2444,6 +2444,7 @@ static int ov5670_probe(struct i2c_client *client)
    struct ov5670 *ov5670;
    const char *err_msg;
    u32 input_clk = 0;
+   bool powered_off;
    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";

drivers/media/i2c/ov5670.c static int ov5670_probe(struct i2c_client *client) · line 2472

Quoted 10 lines

drivers/media/i2c/ov5670.c +8 -1

-           goto error_print;
+   powered_off = acpi_dev_powered_off_for_probe(&client->dev);
+   if (!powered_off) {
+           /* Check module identity */
+           ret = ov5670_identify_module(ov5670);
+           if (ret) {
+                   err_msg = "ov5670_identify_module() error";
+                   goto error_print;
+           }
    }

I don't like the fact that we can't detect any hardware connection issue here anymore and we would actually get some obscure failure when we actually start streaming.

Quoted 4 lines

Wouldn't it be possible to still keep this behavior of not powering on the device at boot-up if no driver is bound and then have this driver built as a module and loaded later when the camera is to be used for the first time after the system boots?

That'd be a way to work around this, but the downside would be that the user space would need to know not only which drivers to load, but also which drivers _not_ to load. The user space could obtain the former from the kernel but not the latter, it'd be system specific configuration.

Moving the responsibility of loading the driver to user space would also not address figuring out whether the sensor is accessible through its control bus: you have to power it on to do that. In fact, if you want to be sure that the hardware is all right, you have to start streaming on the device first and that is not a part of a typical driver initialisation sequence. Just checking the sensor is accessible over I涎 is not enough.

The proposed solution addresses the problem without user space changes.

Quoted 67 lines · Sakari Ailus

Hi Tomasz,

Quoted 46 lines · Tomasz Figa

Hi Sakari,

Quoted 28 lines · Sakari Ailus

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

---
 drivers/media/i2c/ov5670.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

drivers/media/i2c/ov5670.c +1 -4

index 041fcbb4eebdf..57e8b92f90e09 100644
@@ -2444,6 +2444,7 @@ static int ov5670_probe(struct i2c_client *client)
    struct ov5670 *ov5670;
    const char *err_msg;
    u32 input_clk = 0;
+   bool powered_off;
    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";

drivers/media/i2c/ov5670.c static int ov5670_probe(struct i2c_client *client) · line 2472

Quoted 10 lines

drivers/media/i2c/ov5670.c +8 -1

-           goto error_print;
+   powered_off = acpi_dev_powered_off_for_probe(&client->dev);
+   if (!powered_off) {
+           /* Check module identity */
+           ret = ov5670_identify_module(ov5670);
+           if (ret) {
+                   err_msg = "ov5670_identify_module() error";
+                   goto error_print;
+           }
    }

I don't like the fact that we can't detect any hardware connection issue here anymore and we would actually get some obscure failure when we actually start streaming.

Quoted 4 lines

Wouldn't it be possible to still keep this behavior of not powering on the device at boot-up if no driver is bound and then have this driver built as a module and loaded later when the camera is to be used for the first time after the system boots?

That'd be a way to work around this, but the downside would be that the user space would need to know not only which drivers to load, but also which drivers _not_ to load. The user space could obtain the former from the kernel but not the latter, it'd be system specific configuration.

Moving the responsibility of loading the driver to user space would also not address figuring out whether the sensor is accessible through its control bus: you have to power it on to do that. In fact, if you want to be sure that the hardware is all right, you have to start streaming on the device first and that is not a part of a typical driver initialisation sequence. Just checking the sensor is accessible over I涎 is not enough.

Quoted 1 line

The proposed solution addresses the problem without user space changes.

I guess that makes sense indeed. If going this way, why not just move all the hardware access from probe to streamon and avoid any conditional checks at all?

Hi Tomasz,

Quoted 69 lines · Tomasz Figa

Quoted 67 lines · Sakari Ailus

Hi Tomasz,

Quoted 46 lines · Tomasz Figa

Hi Sakari,

Quoted 28 lines · Sakari Ailus

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

---
 drivers/media/i2c/ov5670.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

drivers/media/i2c/ov5670.c +1 -4

index 041fcbb4eebdf..57e8b92f90e09 100644
@@ -2444,6 +2444,7 @@ static int ov5670_probe(struct i2c_client *client)
    struct ov5670 *ov5670;
    const char *err_msg;
    u32 input_clk = 0;
+   bool powered_off;
    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";

drivers/media/i2c/ov5670.c static int ov5670_probe(struct i2c_client *client) · line 2472

Quoted 10 lines

drivers/media/i2c/ov5670.c +8 -1

-           goto error_print;
+   powered_off = acpi_dev_powered_off_for_probe(&client->dev);
+   if (!powered_off) {
+           /* Check module identity */
+           ret = ov5670_identify_module(ov5670);
+           if (ret) {
+                   err_msg = "ov5670_identify_module() error";
+                   goto error_print;
+           }
    }

I don't like the fact that we can't detect any hardware connection issue here anymore and we would actually get some obscure failure when we actually start streaming.

Quoted 4 lines

Wouldn't it be possible to still keep this behavior of not powering on the device at boot-up if no driver is bound and then have this driver built as a module and loaded later when the camera is to be used for the first time after the system boots?

That'd be a way to work around this, but the downside would be that the user space would need to know not only which drivers to load, but also which drivers _not_ to load. The user space could obtain the former from the kernel but not the latter, it'd be system specific configuration.

Moving the responsibility of loading the driver to user space would also not address figuring out whether the sensor is accessible through its control bus: you have to power it on to do that. In fact, if you want to be sure that the hardware is all right, you have to start streaming on the device first and that is not a part of a typical driver initialisation sequence. Just checking the sensor is accessible over I涎 is not enough.

Quoted 5 lines

Quoted 1 line

The proposed solution addresses the problem without user space changes.

I guess that makes sense indeed. If going this way, why not just move all the hardware access from probe to streamon and avoid any conditional checks at all?

My apologies for the late answer.

In that case there would be no way to verify the hardware actually is there, even on systems where there is no adverse effect from doing that. For a sensor driver this could be just fine, but I have doubts it'd be appropriate for e.g. the at24 driver.