[PATCH v5 0/1] i2c: Restore power status of device if probe fail or device is removed

[PATCH v5 0/1] i2c: Restore power status of device if probe fail or device is removed

[Ricardo Ribalda]

We have discovered that some power lines were always on even if the devices
on that power line was not used.

This happens because we failed to probe a device on the i2c bus, and the
ACPI Power Resource were never turned off.

This patch tries to fix this issue.

To: Wolfram Sang <wsa@kernel.org>
To: Sakari Ailus <sakari.ailus@linux.intel.com>
To: Tomasz Figa <tfiga@chromium.org>
To: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
Cc: Hidenori Kobayashi <hidenorik@google.com>
Cc: Sergey Senozhatsky <senozhatsky@chromium.org>
Cc: linux-i2c@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: stable@vger.kernel.org
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
Changes in v5:
- Add Cc: stable
- Add Reviewed-by Sakary (Thanks!)
- Renamed turn-off as power-off, in the name of consistency (Thanks Sergey!)
- Link to v4: https://lore.kernel.org/r/20221109-i2c-waive-v4-0-e4496462833b@chromium.org

Changes in v4:
- Rename full_power to do_power_on
- Link to v3: https://lore.kernel.org/r/20221109-i2c-waive-v3-0-d8651cb4b88d@chromium.org

Changes in v3:
- Introduce full_power variable to make more clear what we are doing.
- Link to v2: https://lore.kernel.org/r/20221109-i2c-waive-v2-0-07550bf2dacc@chromium.org

Changes in v2:
- Cover also device remove
- Link to v1: https://lore.kernel.org/r/20221109-i2c-waive-v1-0-ed70a99b990d@chromium.org

---
Ricardo Ribalda (1):
      i2c: Restore initial power state when we are done.

 drivers/i2c/i2c-core-base.c | 11 +++++++----
 include/linux/i2c.h         |  4 ++++
 2 files changed, 11 insertions(+), 4 deletions(-)
---
base-commit: f141df371335645ce29a87d9683a3f79fba7fd67
change-id: 20221109-i2c-waive-ae97fea1f1b5

Best regards,
-- 
Ricardo Ribalda <ribalda@chromium.org>

[PATCH v5 1/1] i2c: Restore initial power state when we are done.

[Ricardo Ribalda]

A driver that supports I2C_DRV_ACPI_WAIVE_D0_PROBE is not expected to
power off a device that it has not powered on previously.

For devices operating in "full_power" mode, the first call to
`i2c_acpi_waive_d0_probe` will return 0, which means that the device
will be turned on with `dev_pm_domain_attach`.

If probe fails or the device is removed the second call to
`i2c_acpi_waive_d0_probe` will return 1, which means that the device
will not be turned off. This is, it will be left in a different power
state. Lets fix it.

Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: stable@vger.kernel.org
Fixes: b18c1ad685d9 ("i2c: Allow an ACPI driver to manage the device's power state during probe")
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index b4edf10e8fd0..6f4974c76404 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -467,6 +467,7 @@ static int i2c_device_probe(struct device *dev)
 {
 	struct i2c_client	*client = i2c_verify_client(dev);
 	struct i2c_driver	*driver;
+	bool do_power_on;
 	int status;
 
 	if (!client)
@@ -545,8 +546,8 @@ static int i2c_device_probe(struct device *dev)
 	if (status < 0)
 		goto err_clear_wakeup_irq;
 
-	status = dev_pm_domain_attach(&client->dev,
-				      !i2c_acpi_waive_d0_probe(dev));
+	do_power_on = !i2c_acpi_waive_d0_probe(dev);
+	status = dev_pm_domain_attach(&client->dev, do_power_on);
 	if (status)
 		goto err_clear_wakeup_irq;
 
@@ -580,12 +581,14 @@ static int i2c_device_probe(struct device *dev)
 	if (status)
 		goto err_release_driver_resources;
 
+	client->power_off_on_remove = do_power_on;
+
 	return 0;
 
 err_release_driver_resources:
 	devres_release_group(&client->dev, client->devres_group_id);
 err_detach_pm_domain:
-	dev_pm_domain_detach(&client->dev, !i2c_acpi_waive_d0_probe(dev));
+	dev_pm_domain_detach(&client->dev, do_power_on);
 err_clear_wakeup_irq:
 	dev_pm_clear_wake_irq(&client->dev);
 	device_init_wakeup(&client->dev, false);
@@ -610,7 +613,7 @@ static void i2c_device_remove(struct device *dev)
 
 	devres_release_group(&client->dev, client->devres_group_id);
 
-	dev_pm_domain_detach(&client->dev, !i2c_acpi_waive_d0_probe(dev));
+	dev_pm_domain_detach(&client->dev, client->power_off_on_remove);
 
 	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 f7c49bbdb8a1..eba83bc5459e 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -326,6 +326,8 @@ struct i2c_driver {
  *	calls it to pass on slave events to the slave driver.
  * @devres_group_id: id of the devres group that will be created for resources
  *	acquired when probing this device.
+ * @power_off_on_remove: Record if we have turned on the device before probing
+ *	so we can turn off the device at removal.
  *
  * An i2c_client identifies a single device (i.e. chip) connected to an
  * i2c bus. The behaviour exposed to Linux is defined by the driver
@@ -355,6 +357,8 @@ struct i2c_client {
 	i2c_slave_cb_t slave_cb;	/* callback for slave mode	*/
 #endif
 	void *devres_group_id;		/* ID of probe devres group	*/
+	bool power_off_on_remove;	/* if device needs to be turned	*/
+					/* off by framework at removal	*/
 };
 #define to_i2c_client(d) container_of(d, struct i2c_client, dev)
 

-- 
b4 0.11.0-dev-d93f8

Re: [PATCH v5 1/1] i2c: Restore initial power state when we are done.

[Hidenori Kobayashi]

Hi Ricardo,

On Thu, Nov 10, 2022 at 05:20:39PM +0100, Ricardo Ribalda wrote:
> A driver that supports I2C_DRV_ACPI_WAIVE_D0_PROBE is not expected to
> power off a device that it has not powered on previously.
> 
> For devices operating in "full_power" mode, the first call to
> `i2c_acpi_waive_d0_probe` will return 0, which means that the device
> will be turned on with `dev_pm_domain_attach`.
> 
> If probe fails or the device is removed the second call to
> `i2c_acpi_waive_d0_probe` will return 1, which means that the device
> will not be turned off. This is, it will be left in a different power
> state. Lets fix it.
> 
> Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Cc: stable@vger.kernel.org
> Fixes: b18c1ad685d9 ("i2c: Allow an ACPI driver to manage the device's power state during probe")
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>

It's much easier to read now. Thanks!

Reviewed-by: Hidenori Kobayashi <hidenorik@chromium.org>

> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index b4edf10e8fd0..6f4974c76404 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -467,6 +467,7 @@ static int i2c_device_probe(struct device *dev)
>  {
>  	struct i2c_client	*client = i2c_verify_client(dev);
>  	struct i2c_driver	*driver;
> +	bool do_power_on;
>  	int status;
>  
>  	if (!client)
> @@ -545,8 +546,8 @@ static int i2c_device_probe(struct device *dev)
>  	if (status < 0)
>  		goto err_clear_wakeup_irq;
>  
> -	status = dev_pm_domain_attach(&client->dev,
> -				      !i2c_acpi_waive_d0_probe(dev));
> +	do_power_on = !i2c_acpi_waive_d0_probe(dev);
> +	status = dev_pm_domain_attach(&client->dev, do_power_on);
>  	if (status)
>  		goto err_clear_wakeup_irq;
>  
> @@ -580,12 +581,14 @@ static int i2c_device_probe(struct device *dev)
>  	if (status)
>  		goto err_release_driver_resources;
>  
> +	client->power_off_on_remove = do_power_on;
> +
>  	return 0;
>  
>  err_release_driver_resources:
>  	devres_release_group(&client->dev, client->devres_group_id);
>  err_detach_pm_domain:
> -	dev_pm_domain_detach(&client->dev, !i2c_acpi_waive_d0_probe(dev));
> +	dev_pm_domain_detach(&client->dev, do_power_on);
>  err_clear_wakeup_irq:
>  	dev_pm_clear_wake_irq(&client->dev);
>  	device_init_wakeup(&client->dev, false);
> @@ -610,7 +613,7 @@ static void i2c_device_remove(struct device *dev)
>  
>  	devres_release_group(&client->dev, client->devres_group_id);
>  
> -	dev_pm_domain_detach(&client->dev, !i2c_acpi_waive_d0_probe(dev));
> +	dev_pm_domain_detach(&client->dev, client->power_off_on_remove);
>  
>  	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 f7c49bbdb8a1..eba83bc5459e 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -326,6 +326,8 @@ struct i2c_driver {
>   *	calls it to pass on slave events to the slave driver.
>   * @devres_group_id: id of the devres group that will be created for resources
>   *	acquired when probing this device.
> + * @power_off_on_remove: Record if we have turned on the device before probing
> + *	so we can turn off the device at removal.
>   *
>   * An i2c_client identifies a single device (i.e. chip) connected to an
>   * i2c bus. The behaviour exposed to Linux is defined by the driver
> @@ -355,6 +357,8 @@ struct i2c_client {
>  	i2c_slave_cb_t slave_cb;	/* callback for slave mode	*/
>  #endif
>  	void *devres_group_id;		/* ID of probe devres group	*/
> +	bool power_off_on_remove;	/* if device needs to be turned	*/
> +					/* off by framework at removal	*/
>  };
>  #define to_i2c_client(d) container_of(d, struct i2c_client, dev)
>  
> 
> -- 
> b4 0.11.0-dev-d93f8
> 

Re: [PATCH v5 1/1] i2c: Restore initial power state when we are done.

[Sergey Senozhatsky]

On (22/11/10 17:20), Ricardo Ribalda wrote:
> A driver that supports I2C_DRV_ACPI_WAIVE_D0_PROBE is not expected to
> power off a device that it has not powered on previously.
> 
> For devices operating in "full_power" mode, the first call to
> `i2c_acpi_waive_d0_probe` will return 0, which means that the device
> will be turned on with `dev_pm_domain_attach`.
> 
> If probe fails or the device is removed the second call to
> `i2c_acpi_waive_d0_probe` will return 1, which means that the device
> will not be turned off. This is, it will be left in a different power
> state. Lets fix it.
> 
> Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Cc: stable@vger.kernel.org
> Fixes: b18c1ad685d9 ("i2c: Allow an ACPI driver to manage the device's power state during probe")
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>

Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>

Re: [PATCH v5 1/1] i2c: Restore initial power state when we are done.

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 3805 bytes --]

On Thu, Nov 10, 2022 at 05:20:39PM +0100, Ricardo Ribalda wrote:
> A driver that supports I2C_DRV_ACPI_WAIVE_D0_PROBE is not expected to
> power off a device that it has not powered on previously.
> 
> For devices operating in "full_power" mode, the first call to
> `i2c_acpi_waive_d0_probe` will return 0, which means that the device
> will be turned on with `dev_pm_domain_attach`.
> 
> If probe fails or the device is removed the second call to
> `i2c_acpi_waive_d0_probe` will return 1, which means that the device
> will not be turned off. This is, it will be left in a different power
> state. Lets fix it.
> 
> Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Cc: stable@vger.kernel.org
> Fixes: b18c1ad685d9 ("i2c: Allow an ACPI driver to manage the device's power state during probe")
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>

Adding I2C ACPI maintainer to CC. Mika, could you please help
reviewing?

> 
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index b4edf10e8fd0..6f4974c76404 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -467,6 +467,7 @@ static int i2c_device_probe(struct device *dev)
>  {
>  	struct i2c_client	*client = i2c_verify_client(dev);
>  	struct i2c_driver	*driver;
> +	bool do_power_on;
>  	int status;
>  
>  	if (!client)
> @@ -545,8 +546,8 @@ static int i2c_device_probe(struct device *dev)
>  	if (status < 0)
>  		goto err_clear_wakeup_irq;
>  
> -	status = dev_pm_domain_attach(&client->dev,
> -				      !i2c_acpi_waive_d0_probe(dev));
> +	do_power_on = !i2c_acpi_waive_d0_probe(dev);
> +	status = dev_pm_domain_attach(&client->dev, do_power_on);
>  	if (status)
>  		goto err_clear_wakeup_irq;
>  
> @@ -580,12 +581,14 @@ static int i2c_device_probe(struct device *dev)
>  	if (status)
>  		goto err_release_driver_resources;
>  
> +	client->power_off_on_remove = do_power_on;
> +
>  	return 0;
>  
>  err_release_driver_resources:
>  	devres_release_group(&client->dev, client->devres_group_id);
>  err_detach_pm_domain:
> -	dev_pm_domain_detach(&client->dev, !i2c_acpi_waive_d0_probe(dev));
> +	dev_pm_domain_detach(&client->dev, do_power_on);
>  err_clear_wakeup_irq:
>  	dev_pm_clear_wake_irq(&client->dev);
>  	device_init_wakeup(&client->dev, false);
> @@ -610,7 +613,7 @@ static void i2c_device_remove(struct device *dev)
>  
>  	devres_release_group(&client->dev, client->devres_group_id);
>  
> -	dev_pm_domain_detach(&client->dev, !i2c_acpi_waive_d0_probe(dev));
> +	dev_pm_domain_detach(&client->dev, client->power_off_on_remove);
>  
>  	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 f7c49bbdb8a1..eba83bc5459e 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -326,6 +326,8 @@ struct i2c_driver {
>   *	calls it to pass on slave events to the slave driver.
>   * @devres_group_id: id of the devres group that will be created for resources
>   *	acquired when probing this device.
> + * @power_off_on_remove: Record if we have turned on the device before probing
> + *	so we can turn off the device at removal.
>   *
>   * An i2c_client identifies a single device (i.e. chip) connected to an
>   * i2c bus. The behaviour exposed to Linux is defined by the driver
> @@ -355,6 +357,8 @@ struct i2c_client {
>  	i2c_slave_cb_t slave_cb;	/* callback for slave mode	*/
>  #endif
>  	void *devres_group_id;		/* ID of probe devres group	*/
> +	bool power_off_on_remove;	/* if device needs to be turned	*/
> +					/* off by framework at removal	*/
>  };
>  #define to_i2c_client(d) container_of(d, struct i2c_client, dev)
>  
> 
> -- 
> b4 0.11.0-dev-d93f8

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v5 1/1] i2c: Restore initial power state when we are done.

[Mika Westerberg]

Hi,

On Sat, Nov 12, 2022 at 09:24:14PM +0100, Wolfram Sang wrote:
> On Thu, Nov 10, 2022 at 05:20:39PM +0100, Ricardo Ribalda wrote:
> > A driver that supports I2C_DRV_ACPI_WAIVE_D0_PROBE is not expected to
> > power off a device that it has not powered on previously.
> > 
> > For devices operating in "full_power" mode, the first call to
> > `i2c_acpi_waive_d0_probe` will return 0, which means that the device
> > will be turned on with `dev_pm_domain_attach`.
> > 
> > If probe fails or the device is removed the second call to
> > `i2c_acpi_waive_d0_probe` will return 1, which means that the device
> > will not be turned off. This is, it will be left in a different power
> > state. Lets fix it.
> > 
> > Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > Cc: stable@vger.kernel.org
> > Fixes: b18c1ad685d9 ("i2c: Allow an ACPI driver to manage the device's power state during probe")
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> 
> Adding I2C ACPI maintainer to CC. Mika, could you please help
> reviewing?

Sure.

> > diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> > index b4edf10e8fd0..6f4974c76404 100644
> > --- a/drivers/i2c/i2c-core-base.c
> > +++ b/drivers/i2c/i2c-core-base.c
> > @@ -467,6 +467,7 @@ static int i2c_device_probe(struct device *dev)
> >  {
> >  	struct i2c_client	*client = i2c_verify_client(dev);
> >  	struct i2c_driver	*driver;
> > +	bool do_power_on;
> >  	int status;
> >  
> >  	if (!client)
> > @@ -545,8 +546,8 @@ static int i2c_device_probe(struct device *dev)
> >  	if (status < 0)
> >  		goto err_clear_wakeup_irq;
> >  
> > -	status = dev_pm_domain_attach(&client->dev,
> > -				      !i2c_acpi_waive_d0_probe(dev));
> > +	do_power_on = !i2c_acpi_waive_d0_probe(dev);
> > +	status = dev_pm_domain_attach(&client->dev, do_power_on);

I think this is fine as the driver says it is OK to see the device in
whatever power state (I assume this is what the
i2c_acpi_waive_d0_probe() is supposed to be doing but there is no
kernel-doc, though).

> >  	if (status)
> >  		goto err_clear_wakeup_irq;
> >  
> > @@ -580,12 +581,14 @@ static int i2c_device_probe(struct device *dev)
> >  	if (status)
> >  		goto err_release_driver_resources;
> >  
> > +	client->power_off_on_remove = do_power_on;
> > +
> >  	return 0;
> >  
> >  err_release_driver_resources:
> >  	devres_release_group(&client->dev, client->devres_group_id);
> >  err_detach_pm_domain:
> > -	dev_pm_domain_detach(&client->dev, !i2c_acpi_waive_d0_probe(dev));
> > +	dev_pm_domain_detach(&client->dev, do_power_on);
> >  err_clear_wakeup_irq:
> >  	dev_pm_clear_wake_irq(&client->dev);
> >  	device_init_wakeup(&client->dev, false);
> > @@ -610,7 +613,7 @@ static void i2c_device_remove(struct device *dev)
> >  
> >  	devres_release_group(&client->dev, client->devres_group_id);
> >  
> > -	dev_pm_domain_detach(&client->dev, !i2c_acpi_waive_d0_probe(dev));
> > +	dev_pm_domain_detach(&client->dev, client->power_off_on_remove);

However, on the remove path I think we should not call
i2c_acpi_waive_d0_probe() at all as that has nothing to do with remove
(it is for whether the driver accepts any power state on probe AFAICT)
so this should stil be "true" here. Unless I'm missing something.

> >  
> >  	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 f7c49bbdb8a1..eba83bc5459e 100644
> > --- a/include/linux/i2c.h
> > +++ b/include/linux/i2c.h
> > @@ -326,6 +326,8 @@ struct i2c_driver {
> >   *	calls it to pass on slave events to the slave driver.
> >   * @devres_group_id: id of the devres group that will be created for resources
> >   *	acquired when probing this device.
> > + * @power_off_on_remove: Record if we have turned on the device before probing
> > + *	so we can turn off the device at removal.
> >   *
> >   * An i2c_client identifies a single device (i.e. chip) connected to an
> >   * i2c bus. The behaviour exposed to Linux is defined by the driver
> > @@ -355,6 +357,8 @@ struct i2c_client {
> >  	i2c_slave_cb_t slave_cb;	/* callback for slave mode	*/
> >  #endif
> >  	void *devres_group_id;		/* ID of probe devres group	*/
> > +	bool power_off_on_remove;	/* if device needs to be turned	*/
> > +					/* off by framework at removal	*/
> >  };
> >  #define to_i2c_client(d) container_of(d, struct i2c_client, dev)
> >  
> > 
> > -- 
> > b4 0.11.0-dev-d93f8

Re: [PATCH v5 1/1] i2c: Restore initial power state when we are done.

[Ricardo Ribalda]

Hi Mika

Thanks for your review!

On Mon, 14 Nov 2022 at 11:33, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
>
> Hi,
>
> On Sat, Nov 12, 2022 at 09:24:14PM +0100, Wolfram Sang wrote:
> > On Thu, Nov 10, 2022 at 05:20:39PM +0100, Ricardo Ribalda wrote:
> > > A driver that supports I2C_DRV_ACPI_WAIVE_D0_PROBE is not expected to
> > > power off a device that it has not powered on previously.
> > >
> > > For devices operating in "full_power" mode, the first call to
> > > `i2c_acpi_waive_d0_probe` will return 0, which means that the device
> > > will be turned on with `dev_pm_domain_attach`.
> > >
> > > If probe fails or the device is removed the second call to
> > > `i2c_acpi_waive_d0_probe` will return 1, which means that the device
> > > will not be turned off. This is, it will be left in a different power
> > > state. Lets fix it.
> > >
> > > Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > Cc: stable@vger.kernel.org
> > > Fixes: b18c1ad685d9 ("i2c: Allow an ACPI driver to manage the device's power state during probe")
> > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> >
> > Adding I2C ACPI maintainer to CC. Mika, could you please help
> > reviewing?
>
> Sure.
>
> > > diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> > > index b4edf10e8fd0..6f4974c76404 100644
> > > --- a/drivers/i2c/i2c-core-base.c
> > > +++ b/drivers/i2c/i2c-core-base.c
> > > @@ -467,6 +467,7 @@ static int i2c_device_probe(struct device *dev)
> > >  {
> > >     struct i2c_client       *client = i2c_verify_client(dev);
> > >     struct i2c_driver       *driver;
> > > +   bool do_power_on;
> > >     int status;
> > >
> > >     if (!client)
> > > @@ -545,8 +546,8 @@ static int i2c_device_probe(struct device *dev)
> > >     if (status < 0)
> > >             goto err_clear_wakeup_irq;
> > >
> > > -   status = dev_pm_domain_attach(&client->dev,
> > > -                                 !i2c_acpi_waive_d0_probe(dev));
> > > +   do_power_on = !i2c_acpi_waive_d0_probe(dev);
> > > +   status = dev_pm_domain_attach(&client->dev, do_power_on);
>
> I think this is fine as the driver says it is OK to see the device in
> whatever power state (I assume this is what the
> i2c_acpi_waive_d0_probe() is supposed to be doing but there is no
> kernel-doc, though).
>
> > >     if (status)
> > >             goto err_clear_wakeup_irq;
> > >
> > > @@ -580,12 +581,14 @@ static int i2c_device_probe(struct device *dev)
> > >     if (status)
> > >             goto err_release_driver_resources;
> > >
> > > +   client->power_off_on_remove = do_power_on;
> > > +
> > >     return 0;
> > >
> > >  err_release_driver_resources:
> > >     devres_release_group(&client->dev, client->devres_group_id);
> > >  err_detach_pm_domain:
> > > -   dev_pm_domain_detach(&client->dev, !i2c_acpi_waive_d0_probe(dev));
> > > +   dev_pm_domain_detach(&client->dev, do_power_on);
> > >  err_clear_wakeup_irq:
> > >     dev_pm_clear_wake_irq(&client->dev);
> > >     device_init_wakeup(&client->dev, false);
> > > @@ -610,7 +613,7 @@ static void i2c_device_remove(struct device *dev)
> > >
> > >     devres_release_group(&client->dev, client->devres_group_id);
> > >
> > > -   dev_pm_domain_detach(&client->dev, !i2c_acpi_waive_d0_probe(dev));
> > > +   dev_pm_domain_detach(&client->dev, client->power_off_on_remove);
>
> However, on the remove path I think we should not call
> i2c_acpi_waive_d0_probe() at all as that has nothing to do with remove
> (it is for whether the driver accepts any power state on probe AFAICT)
> so this should stil be "true" here. Unless I'm missing something.

I guess the problem is that we would be undoing something (power-off),
that we did not do.


Sakari, can you think of a use-case where this can break something? If
not I can send a v6.

Regards!

>
> > >
> > >     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 f7c49bbdb8a1..eba83bc5459e 100644
> > > --- a/include/linux/i2c.h
> > > +++ b/include/linux/i2c.h
> > > @@ -326,6 +326,8 @@ struct i2c_driver {
> > >   * calls it to pass on slave events to the slave driver.
> > >   * @devres_group_id: id of the devres group that will be created for resources
> > >   * acquired when probing this device.
> > > + * @power_off_on_remove: Record if we have turned on the device before probing
> > > + * so we can turn off the device at removal.
> > >   *
> > >   * An i2c_client identifies a single device (i.e. chip) connected to an
> > >   * i2c bus. The behaviour exposed to Linux is defined by the driver
> > > @@ -355,6 +357,8 @@ struct i2c_client {
> > >     i2c_slave_cb_t slave_cb;        /* callback for slave mode      */
> > >  #endif
> > >     void *devres_group_id;          /* ID of probe devres group     */
> > > +   bool power_off_on_remove;       /* if device needs to be turned */
> > > +                                   /* off by framework at removal  */
> > >  };
> > >  #define to_i2c_client(d) container_of(d, struct i2c_client, dev)
> > >
> > >
> > > --
> > > b4 0.11.0-dev-d93f8
>
>


-- 
Ricardo Ribalda

Re: [PATCH v5 1/1] i2c: Restore initial power state when we are done.

[Sakari Ailus]

Hi Ricardo, Mika,

On Mon, Nov 14, 2022 at 11:51:24AM +0100, Ricardo Ribalda wrote:
> Hi Mika
> 
> Thanks for your review!
> 
> On Mon, 14 Nov 2022 at 11:33, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> >
> > Hi,
> >
> > On Sat, Nov 12, 2022 at 09:24:14PM +0100, Wolfram Sang wrote:
> > > On Thu, Nov 10, 2022 at 05:20:39PM +0100, Ricardo Ribalda wrote:
> > > > A driver that supports I2C_DRV_ACPI_WAIVE_D0_PROBE is not expected to
> > > > power off a device that it has not powered on previously.
> > > >
> > > > For devices operating in "full_power" mode, the first call to
> > > > `i2c_acpi_waive_d0_probe` will return 0, which means that the device
> > > > will be turned on with `dev_pm_domain_attach`.
> > > >
> > > > If probe fails or the device is removed the second call to
> > > > `i2c_acpi_waive_d0_probe` will return 1, which means that the device
> > > > will not be turned off. This is, it will be left in a different power
> > > > state. Lets fix it.
> > > >
> > > > Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > Cc: stable@vger.kernel.org
> > > > Fixes: b18c1ad685d9 ("i2c: Allow an ACPI driver to manage the device's power state during probe")
> > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > >
> > > Adding I2C ACPI maintainer to CC. Mika, could you please help
> > > reviewing?
> >
> > Sure.
> >
> > > > diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> > > > index b4edf10e8fd0..6f4974c76404 100644
> > > > --- a/drivers/i2c/i2c-core-base.c
> > > > +++ b/drivers/i2c/i2c-core-base.c
> > > > @@ -467,6 +467,7 @@ static int i2c_device_probe(struct device *dev)
> > > >  {
> > > >     struct i2c_client       *client = i2c_verify_client(dev);
> > > >     struct i2c_driver       *driver;
> > > > +   bool do_power_on;
> > > >     int status;
> > > >
> > > >     if (!client)
> > > > @@ -545,8 +546,8 @@ static int i2c_device_probe(struct device *dev)
> > > >     if (status < 0)
> > > >             goto err_clear_wakeup_irq;
> > > >
> > > > -   status = dev_pm_domain_attach(&client->dev,
> > > > -                                 !i2c_acpi_waive_d0_probe(dev));
> > > > +   do_power_on = !i2c_acpi_waive_d0_probe(dev);
> > > > +   status = dev_pm_domain_attach(&client->dev, do_power_on);
> >
> > I think this is fine as the driver says it is OK to see the device in
> > whatever power state (I assume this is what the
> > i2c_acpi_waive_d0_probe() is supposed to be doing but there is no
> > kernel-doc, though).
> >
> > > >     if (status)
> > > >             goto err_clear_wakeup_irq;
> > > >
> > > > @@ -580,12 +581,14 @@ static int i2c_device_probe(struct device *dev)
> > > >     if (status)
> > > >             goto err_release_driver_resources;
> > > >
> > > > +   client->power_off_on_remove = do_power_on;
> > > > +
> > > >     return 0;
> > > >
> > > >  err_release_driver_resources:
> > > >     devres_release_group(&client->dev, client->devres_group_id);
> > > >  err_detach_pm_domain:
> > > > -   dev_pm_domain_detach(&client->dev, !i2c_acpi_waive_d0_probe(dev));
> > > > +   dev_pm_domain_detach(&client->dev, do_power_on);
> > > >  err_clear_wakeup_irq:
> > > >     dev_pm_clear_wake_irq(&client->dev);
> > > >     device_init_wakeup(&client->dev, false);
> > > > @@ -610,7 +613,7 @@ static void i2c_device_remove(struct device *dev)
> > > >
> > > >     devres_release_group(&client->dev, client->devres_group_id);
> > > >
> > > > -   dev_pm_domain_detach(&client->dev, !i2c_acpi_waive_d0_probe(dev));
> > > > +   dev_pm_domain_detach(&client->dev, client->power_off_on_remove);
> >
> > However, on the remove path I think we should not call
> > i2c_acpi_waive_d0_probe() at all as that has nothing to do with remove
> > (it is for whether the driver accepts any power state on probe AFAICT)
> > so this should stil be "true" here. Unless I'm missing something.
> 
> I guess the problem is that we would be undoing something (power-off),
> that we did not do.
> 
> 
> Sakari, can you think of a use-case where this can break something? If
> not I can send a v6.

I don't think there's any harm from powering the device off here. The
driver should have done it but there could be bugs...

The genpd equivalent doesn't even use the argument but powers off the
domain in any case.

-- 
Kind regards,

Sakari Ailus