[v2] i2c: core: Disable client irq on reboot/shutdown

Message ID 20210604232744.1259150-1-swboyd@chromium.org
State New
Headers show
Series
  • [v2] i2c: core: Disable client irq on reboot/shutdown
Related show

Commit Message

Stephen Boyd June 4, 2021, 11:27 p.m.
From: Dmitry Torokhov <dmitry.torokhov@gmail.com>

If an i2c client receives an interrupt during reboot or shutdown it may
be too late to service it by making an i2c transaction on the bus
because the i2c controller has already been shutdown. This can lead to
system hangs if the i2c controller tries to make a transfer that is
doomed to fail because the access to the i2c pins is already shut down,
or an iommu translation has been torn down so i2c controller register
access doesn't work.

Let's simply disable the irq if there isn't a shutdown callback for an
i2c client when there is an irq associated with the device. This will
make sure that irqs don't come in later than the time that we can handle
it. We don't do this if the i2c client device already has a shutdown
callback because presumably they're doing the right thing and quieting
the device so irqs don't come in after the shutdown callback returns.

Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Reported-by: kernel test robot <lkp@intel.com>
[swboyd@chromium.org: Dropped newline, added commit text, added
interrupt.h for robot build error]
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---

Dmitry, please add Signed-off-by so this can be merged through i2c

This supersedes https://lore.kernel.org/r/20210510220012.2003285-1-swboyd@chromium.org

Changes from v1 (https://lore.kernel.org/r/20210604212752.3547301-1-swboyd@chromium.org)
 * Add interrupt.h include for robot

 drivers/i2c/i2c-core-base.c | 3 +++
 1 file changed, 3 insertions(+)


base-commit: 8124c8a6b35386f73523d27eacb71b5364a68c4c

Comments

Dmitry Torokhov June 5, 2021, 10:54 p.m. | #1
On Fri, Jun 04, 2021 at 04:27:44PM -0700, Stephen Boyd wrote:
> From: Dmitry Torokhov <dmitry.torokhov@gmail.com>

> 

> If an i2c client receives an interrupt during reboot or shutdown it may

> be too late to service it by making an i2c transaction on the bus

> because the i2c controller has already been shutdown. This can lead to

> system hangs if the i2c controller tries to make a transfer that is

> doomed to fail because the access to the i2c pins is already shut down,

> or an iommu translation has been torn down so i2c controller register

> access doesn't work.

> 

> Let's simply disable the irq if there isn't a shutdown callback for an

> i2c client when there is an irq associated with the device. This will

> make sure that irqs don't come in later than the time that we can handle

> it. We don't do this if the i2c client device already has a shutdown

> callback because presumably they're doing the right thing and quieting

> the device so irqs don't come in after the shutdown callback returns.

> 

> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>

> Reported-by: kernel test robot <lkp@intel.com>

> [swboyd@chromium.org: Dropped newline, added commit text, added

> interrupt.h for robot build error]

> Signed-off-by: Stephen Boyd <swboyd@chromium.org>


Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>


> ---

> 

> Dmitry, please add Signed-off-by so this can be merged through i2c

> 

> This supersedes https://lore.kernel.org/r/20210510220012.2003285-1-swboyd@chromium.org

> 

> Changes from v1 (https://lore.kernel.org/r/20210604212752.3547301-1-swboyd@chromium.org)

>  * Add interrupt.h include for robot

> 

>  drivers/i2c/i2c-core-base.c | 3 +++

>  1 file changed, 3 insertions(+)

> 

> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c

> index 5a97e4a02fa2..e314ccaf114a 100644

> --- a/drivers/i2c/i2c-core-base.c

> +++ b/drivers/i2c/i2c-core-base.c

> @@ -24,6 +24,7 @@

>  #include <linux/i2c-smbus.h>

>  #include <linux/idr.h>

>  #include <linux/init.h>

> +#include <linux/interrupt.h>

>  #include <linux/irqflags.h>

>  #include <linux/jump_label.h>

>  #include <linux/kernel.h>

> @@ -627,6 +628,8 @@ static void i2c_device_shutdown(struct device *dev)

>  	driver = to_i2c_driver(dev->driver);

>  	if (driver->shutdown)

>  		driver->shutdown(client);

> +	else if (client->irq > 0)

> +		disable_irq(client->irq);

>  }

>  

>  static void i2c_client_dev_release(struct device *dev)

> 

> base-commit: 8124c8a6b35386f73523d27eacb71b5364a68c4c

> -- 

> https://chromeos.dev

> 


-- 
Dmitry
Wolfram Sang June 20, 2021, 9:04 p.m. | #2
On Fri, Jun 04, 2021 at 04:27:44PM -0700, Stephen Boyd wrote:
> From: Dmitry Torokhov <dmitry.torokhov@gmail.com>

> 

> If an i2c client receives an interrupt during reboot or shutdown it may

> be too late to service it by making an i2c transaction on the bus

> because the i2c controller has already been shutdown. This can lead to

> system hangs if the i2c controller tries to make a transfer that is

> doomed to fail because the access to the i2c pins is already shut down,

> or an iommu translation has been torn down so i2c controller register

> access doesn't work.

> 

> Let's simply disable the irq if there isn't a shutdown callback for an

> i2c client when there is an irq associated with the device. This will

> make sure that irqs don't come in later than the time that we can handle

> it. We don't do this if the i2c client device already has a shutdown

> callback because presumably they're doing the right thing and quieting

> the device so irqs don't come in after the shutdown callback returns.

> 

> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>

> Reported-by: kernel test robot <lkp@intel.com>

> [swboyd@chromium.org: Dropped newline, added commit text, added

> interrupt.h for robot build error]

> Signed-off-by: Stephen Boyd <swboyd@chromium.org>


I think this is for-current material because it fixes hangs. Or better
for-next to check for side-effects?
Dmitry Torokhov June 21, 2021, 5:26 a.m. | #3
On Sun, Jun 20, 2021 at 11:04:23PM +0200, Wolfram Sang wrote:
> On Fri, Jun 04, 2021 at 04:27:44PM -0700, Stephen Boyd wrote:

> > From: Dmitry Torokhov <dmitry.torokhov@gmail.com>

> > 

> > If an i2c client receives an interrupt during reboot or shutdown it may

> > be too late to service it by making an i2c transaction on the bus

> > because the i2c controller has already been shutdown. This can lead to

> > system hangs if the i2c controller tries to make a transfer that is

> > doomed to fail because the access to the i2c pins is already shut down,

> > or an iommu translation has been torn down so i2c controller register

> > access doesn't work.

> > 

> > Let's simply disable the irq if there isn't a shutdown callback for an

> > i2c client when there is an irq associated with the device. This will

> > make sure that irqs don't come in later than the time that we can handle

> > it. We don't do this if the i2c client device already has a shutdown

> > callback because presumably they're doing the right thing and quieting

> > the device so irqs don't come in after the shutdown callback returns.

> > 

> > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>

> > Reported-by: kernel test robot <lkp@intel.com>

> > [swboyd@chromium.org: Dropped newline, added commit text, added

> > interrupt.h for robot build error]

> > Signed-off-by: Stephen Boyd <swboyd@chromium.org>

> 

> I think this is for-current material because it fixes hangs. Or better

> for-next to check for side-effects?

> 


We lived with this issue for many years, so letting it cook in next will
not hurt us IMO.

Thanks.

-- 
Dmitry
Wolfram Sang June 24, 2021, 8:04 p.m. | #4
On Fri, Jun 04, 2021 at 04:27:44PM -0700, Stephen Boyd wrote:
> From: Dmitry Torokhov <dmitry.torokhov@gmail.com>

> 

> If an i2c client receives an interrupt during reboot or shutdown it may

> be too late to service it by making an i2c transaction on the bus

> because the i2c controller has already been shutdown. This can lead to

> system hangs if the i2c controller tries to make a transfer that is

> doomed to fail because the access to the i2c pins is already shut down,

> or an iommu translation has been torn down so i2c controller register

> access doesn't work.

> 

> Let's simply disable the irq if there isn't a shutdown callback for an

> i2c client when there is an irq associated with the device. This will

> make sure that irqs don't come in later than the time that we can handle

> it. We don't do this if the i2c client device already has a shutdown

> callback because presumably they're doing the right thing and quieting

> the device so irqs don't come in after the shutdown callback returns.

> 

> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>

> Reported-by: kernel test robot <lkp@intel.com>

> [swboyd@chromium.org: Dropped newline, added commit text, added

> interrupt.h for robot build error]

> Signed-off-by: Stephen Boyd <swboyd@chromium.org>


Applied to for-next, thanks!

Patch

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 5a97e4a02fa2..e314ccaf114a 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -24,6 +24,7 @@ 
 #include <linux/i2c-smbus.h>
 #include <linux/idr.h>
 #include <linux/init.h>
+#include <linux/interrupt.h>
 #include <linux/irqflags.h>
 #include <linux/jump_label.h>
 #include <linux/kernel.h>
@@ -627,6 +628,8 @@  static void i2c_device_shutdown(struct device *dev)
 	driver = to_i2c_driver(dev->driver);
 	if (driver->shutdown)
 		driver->shutdown(client);
+	else if (client->irq > 0)
+		disable_irq(client->irq);
 }
 
 static void i2c_client_dev_release(struct device *dev)