Message ID | 20190425082310.17109-1-wsa+renesas@sang-engineering.com |
---|---|
State | Superseded |
Headers | show |
Series | i2c: core: ratelimit 'transfer when suspended' errors | expand |
On 2019-04-25 10:23, Wolfram Sang wrote: > There are two problems with WARN_ON() here. One: It is not ratelimited. > Two: We don't see which adapter was used when trying to transfer > something when already suspended. Implement a custom ratelimit once per > adapter and use dev_WARN there. This fixes both issues. Drawback is that > we don't see if multiple drivers are trying to transfer with the same > adapter while suspended. They need to be discovered one after the other > now. This is better than a high CPU load because a really broken driver > might try to resend endlessly. > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > --- > > So, Simon had a point with his review comment back then, and I think we now > found a properly balanced way. Tested with a Renesas Lager board (R-Car H2). I > decided against dev_WARN_ONCE because that seems too coarse grained for my > taste (once per system) and the custom implementation is small. > > drivers/i2c/i2c-core-base.c | 5 ++++- > include/linux/i2c.h | 3 ++- Perhaps unrelated to this patch, but I expected a similar change in i2c-core-smbus.c:__i2c_smbus_xfer(), but it has no suspended check at all. At first I thought that perhaps no driver that actually did suspend had an .smbus_xfer implementation, but upon verifying that, I found that i2c-zx2967.c does. Am I missing something, or do we need to add an -ESHUTDOWN test to i2c-core-smbus.c:__i2c_smbus_xfer()? Cheers, Peter > 2 files changed, 6 insertions(+), 2 deletions(-) > > diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c > index 4e6300dc2c4e..f8e85983cb04 100644 > --- a/drivers/i2c/i2c-core-base.c > +++ b/drivers/i2c/i2c-core-base.c > @@ -1867,8 +1867,11 @@ int __i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num) > > if (WARN_ON(!msgs || num < 1)) > return -EINVAL; > - if (WARN_ON(test_bit(I2C_ALF_IS_SUSPENDED, &adap->locked_flags))) > + if (test_bit(I2C_ALF_IS_SUSPENDED, &adap->locked_flags)) { > + if (!test_and_set_bit(I2C_ALF_SUSPEND_REPORTED, &adap->locked_flags)) > + dev_WARN(&adap->dev, "Transfer while suspended\n"); > return -ESHUTDOWN; > + } > > if (adap->quirks && i2c_check_for_quirks(adap, msgs, num)) > return -EOPNOTSUPP; > diff --git a/include/linux/i2c.h b/include/linux/i2c.h > index 03755d4c9229..be27062f8ed1 100644 > --- a/include/linux/i2c.h > +++ b/include/linux/i2c.h > @@ -694,7 +694,8 @@ struct i2c_adapter { > int retries; > struct device dev; /* the adapter device */ > unsigned long locked_flags; /* owned by the I2C core */ > -#define I2C_ALF_IS_SUSPENDED 0 > +#define I2C_ALF_IS_SUSPENDED 0 > +#define I2C_ALF_SUSPEND_REPORTED 1 > > int nr; > char name[48]; >
> Am I missing something, or do we need to add an -ESHUTDOWN test > to i2c-core-smbus.c:__i2c_smbus_xfer()? I think you are right, SMBus was simply missed. I will refactor the check to an inline function and apply it to I2C and SMBus. Thanks for the heads up!
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c index 4e6300dc2c4e..f8e85983cb04 100644 --- a/drivers/i2c/i2c-core-base.c +++ b/drivers/i2c/i2c-core-base.c @@ -1867,8 +1867,11 @@ int __i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num) if (WARN_ON(!msgs || num < 1)) return -EINVAL; - if (WARN_ON(test_bit(I2C_ALF_IS_SUSPENDED, &adap->locked_flags))) + if (test_bit(I2C_ALF_IS_SUSPENDED, &adap->locked_flags)) { + if (!test_and_set_bit(I2C_ALF_SUSPEND_REPORTED, &adap->locked_flags)) + dev_WARN(&adap->dev, "Transfer while suspended\n"); return -ESHUTDOWN; + } if (adap->quirks && i2c_check_for_quirks(adap, msgs, num)) return -EOPNOTSUPP; diff --git a/include/linux/i2c.h b/include/linux/i2c.h index 03755d4c9229..be27062f8ed1 100644 --- a/include/linux/i2c.h +++ b/include/linux/i2c.h @@ -694,7 +694,8 @@ struct i2c_adapter { int retries; struct device dev; /* the adapter device */ unsigned long locked_flags; /* owned by the I2C core */ -#define I2C_ALF_IS_SUSPENDED 0 +#define I2C_ALF_IS_SUSPENDED 0 +#define I2C_ALF_SUSPEND_REPORTED 1 int nr; char name[48];
There are two problems with WARN_ON() here. One: It is not ratelimited. Two: We don't see which adapter was used when trying to transfer something when already suspended. Implement a custom ratelimit once per adapter and use dev_WARN there. This fixes both issues. Drawback is that we don't see if multiple drivers are trying to transfer with the same adapter while suspended. They need to be discovered one after the other now. This is better than a high CPU load because a really broken driver might try to resend endlessly. Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> --- So, Simon had a point with his review comment back then, and I think we now found a properly balanced way. Tested with a Renesas Lager board (R-Car H2). I decided against dev_WARN_ONCE because that seems too coarse grained for my taste (once per system) and the custom implementation is small. drivers/i2c/i2c-core-base.c | 5 ++++- include/linux/i2c.h | 3 ++- 2 files changed, 6 insertions(+), 2 deletions(-)