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

[PATCH v2 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: linux-i2c@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Ricardo Ribalda <ribalda@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 | 7 ++++---
 include/linux/i2c.h         | 3 +++
 2 files changed, 7 insertions(+), 3 deletions(-)
---
base-commit: f141df371335645ce29a87d9683a3f79fba7fd67
change-id: 20221109-i2c-waive-ae97fea1f1b5

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

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

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..96623e0647bd 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -545,8 +545,9 @@ static int i2c_device_probe(struct device *dev)
 	if (status < 0)
 		goto err_clear_wakeup_irq;
 
+	client->turn_off_on_remove = !i2c_acpi_waive_d0_probe(dev);
 	status = dev_pm_domain_attach(&client->dev,
-				      !i2c_acpi_waive_d0_probe(dev));
+				      client->turn_off_on_remove);
 	if (status)
 		goto err_clear_wakeup_irq;
 
@@ -585,7 +586,7 @@ static int i2c_device_probe(struct device *dev)
 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, client->turn_off_on_remove);
 err_clear_wakeup_irq:
 	dev_pm_clear_wake_irq(&client->dev);
 	device_init_wakeup(&client->dev, false);
@@ -610,7 +611,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->turn_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..6b2dacb0bae1 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.
+ * @turn_off_on_remove: Record if we have turned on the device before probing
+ *	so we can restore the initial state after remove/probe error.
  *
  * 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,7 @@ struct i2c_client {
 	i2c_slave_cb_t slave_cb;	/* callback for slave mode	*/
 #endif
 	void *devres_group_id;		/* ID of probe devres group	*/
+	bool turn_off_on_remove;	/* power state when done	*/
 };
 #define to_i2c_client(d) container_of(d, struct i2c_client, dev)
 

-- 
b4 0.11.0-dev-d93f8

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

[Hidenori Kobayashi]

Hi Ricardo,

On Wed, Nov 09, 2022 at 04:17:06PM +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.
> 
> 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..96623e0647bd 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -545,8 +545,9 @@ static int i2c_device_probe(struct device *dev)
>  	if (status < 0)
>  		goto err_clear_wakeup_irq;
>  
> +	client->turn_off_on_remove = !i2c_acpi_waive_d0_probe(dev);
>  	status = dev_pm_domain_attach(&client->dev,
> -				      !i2c_acpi_waive_d0_probe(dev));
> +				      client->turn_off_on_remove);
>  	if (status)
>  		goto err_clear_wakeup_irq;
>  
> @@ -585,7 +586,7 @@ static int i2c_device_probe(struct device *dev)
>  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, client->turn_off_on_remove);
>  err_clear_wakeup_irq:
>  	dev_pm_clear_wake_irq(&client->dev);
>  	device_init_wakeup(&client->dev, false);
> @@ -610,7 +611,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->turn_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..6b2dacb0bae1 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.
> + * @turn_off_on_remove: Record if we have turned on the device before probing
> + *	so we can restore the initial state after remove/probe error.
>   *
>   * 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,7 @@ struct i2c_client {
>  	i2c_slave_cb_t slave_cb;	/* callback for slave mode	*/
>  #endif
>  	void *devres_group_id;		/* ID of probe devres group	*/
> +	bool turn_off_on_remove;	/* power state when done	*/

Can we have a different name that also makes sense for attach()?
To me, it's kind of hard to see immediately what the second argument to
attach() meant.

Since this is used for both power_on and power_off, I think something
more neutral would be easier to read? For example: power_flag?

Or I guess we could name it like full_power and provide a wrapper for
the detach() cases?

>  };
>  #define to_i2c_client(d) container_of(d, struct i2c_client, dev)
>  
> 
> -- 
> b4 0.11.0-dev-d93f8
> 

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

[Ricardo Ribalda]

Hi Hidenori

On Thu, 10 Nov 2022 at 10:25, Hidenori Kobayashi <hidenorik@chromium.org> wrote:
>
> Hi Ricardo,
>
> On Wed, Nov 09, 2022 at 04:17:06PM +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.
> >
> > 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..96623e0647bd 100644
> > --- a/drivers/i2c/i2c-core-base.c
> > +++ b/drivers/i2c/i2c-core-base.c
> > @@ -545,8 +545,9 @@ static int i2c_device_probe(struct device *dev)
> >       if (status < 0)
> >               goto err_clear_wakeup_irq;
> >
> > +     client->turn_off_on_remove = !i2c_acpi_waive_d0_probe(dev);
> >       status = dev_pm_domain_attach(&client->dev,
> > -                                   !i2c_acpi_waive_d0_probe(dev));
> > +                                   client->turn_off_on_remove);
> >       if (status)
> >               goto err_clear_wakeup_irq;
> >
> > @@ -585,7 +586,7 @@ static int i2c_device_probe(struct device *dev)
> >  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, client->turn_off_on_remove);
> >  err_clear_wakeup_irq:
> >       dev_pm_clear_wake_irq(&client->dev);
> >       device_init_wakeup(&client->dev, false);
> > @@ -610,7 +611,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->turn_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..6b2dacb0bae1 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.
> > + * @turn_off_on_remove: Record if we have turned on the device before probing
> > + *   so we can restore the initial state after remove/probe error.
> >   *
> >   * 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,7 @@ struct i2c_client {
> >       i2c_slave_cb_t slave_cb;        /* callback for slave mode      */
> >  #endif
> >       void *devres_group_id;          /* ID of probe devres group     */
> > +     bool turn_off_on_remove;        /* power state when done        */
>
> Can we have a different name that also makes sense for attach()?
> To me, it's kind of hard to see immediately what the second argument to
> attach() meant.

I was trying to be super-clever and not adding a new variable. :P

Let me send a new version.

Thanks for your review!

>
> Since this is used for both power_on and power_off, I think something
> more neutral would be easier to read? For example: power_flag?
>
> Or I guess we could name it like full_power and provide a wrapper for
> the detach() cases?
>
> >  };
> >  #define to_i2c_client(d) container_of(d, struct i2c_client, dev)
> >
> >
> > --
> > b4 0.11.0-dev-d93f8
> >



-- 
Ricardo Ribalda