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@xxxxxxxxxxxxxxxxxxxx>
> ---
>
> 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];
>
