Re: [PATCH] i2c: core: ratelimit 'transfer when suspended' errors

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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





[Index of Archives]     [Linux GPIO]     [Linux SPI]     [Linux Hardward Monitoring]     [LM Sensors]     [Linux USB Devel]     [Linux Media]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux