Re: [PATCH 2/2] i2c: omap: fix "Too much work in one IRQ" irq handling

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

 



Hi,

On Sat, Nov 15, 2014 at 05:20:52AM +0400, Alexander Kochetkov wrote:
> commit 66b9298878742f08cb6e79b7c7d5632d782fd1e1 (i2c: omap: switch over
> to do {} while loop) changed the interrupt handler to abort transfers
> in case interrupt serviced 100 times but commit's comment states that
> "No functional changes otherwise.".

look at the patch again, the commit you describe is *not* the one giving
up on servicing interrupts after 100 times. That commit, really, *only*
switched over from while() {} to do {} while(), the only functional
change there is that the while loop will always execute at least once.

> Also, original commit could report status 0 (no error) on aborted transfers.

how ? This is an interesting bug which deserves further explanation.

> The patch restore original logic. In case interrupt serviced 100 times just
> quit interrupt handler and don't abort active transfer.
> 
> Signed-off-by: Alexander Kochetkov <al.kochet@xxxxxxxxx>
> ---
>  drivers/i2c/busses/i2c-omap.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index 9af7095..34b9011 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -920,7 +920,7 @@ omap_i2c_isr_thread(int this_irq, void *dev_id)
>  		dev_dbg(dev->dev, "IRQ (ISR = 0x%04x)\n", stat);
>  		if (count++ == 100) {
>  			dev_warn(dev->dev, "Too much work in one IRQ\n");
> -			break;
> +			goto out;

quite frankly, this looks *very* wrong. It creates the possibility for
us never completing a command which would cause several timeouts.

How have you tested this and how have you figured this was the actual
bug ? Based on commit log not matching the patch body (which 'original
logic' are you talking about ?), I have to NAK this patch.

-- 
balbi

Attachment: signature.asc
Description: Digital signature


[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