linux-kernel - Re: [PATCH] Fixes: 1d7afc9 i2c: omap: ack IRQ in parts

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20141118163921.GB6179@saruman>
Date:	Tue, 18 Nov 2014 10:39:22 -0600
From:	Felipe Balbi <balbi@...com>
To:	Alexander Kochetkov <al.kochet@...il.com>,
	Aaro Koskinen <aaro.koskinen@....fi>
CC:	<linux-omap@...r.kernel.org>, <linux-i2c@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, <stable@...r.kernel.org>,
	Wolfram Sang <wsa@...-dreams.de>,
	Tony Lindgren <tony@...mide.com>, Felipe Balbi <balbi@...com>,
	Grygorii Strashko <grygorii.strashko@...com>
Subject: Re: [PATCH] Fixes: 1d7afc9 i2c: omap: ack IRQ in parts

On Mon, Nov 17, 2014 at 08:41:57PM +0400, Alexander Kochetkov wrote:
> commit 1d7afc95946487945cc7f5019b41255b72224b70 (i2c: omap: ack IRQ in parts)
> changed the interrupt handler to complete transfers without clearing
> XRDY (AL case) and ARDY (NACK case) flags. XRDY or ARDY interrupts will be
> fired again. As a result, ISR keep processing transfer after it was already
> complete (from the driver code point of view).
> 
> A didn't see real impacts of the 1d7afc9, but it is really bad idea to
> have ISR running on user data after transfer was complete.
> 
> It looks, what 1d7afc9 violate TI specs in what how AL and NACK should be
> handled (see Note 1, sprugn4r, Figure 17-31 and Figure 17-32).
> 
> According to specs (if I understood correctly), in case of NACK and AL driver
> must reset NACK, AL, ARDY, RDR, and RRDY (Master Receive Mode), and
> NACK, AL, ARDY, and XDR (Master Transmitter Mode).
> 
> All that is done down the code under the if condition:
> if (stat & (OMAP_I2C_STAT_ARDY | OMAP_I2C_STAT_NACK | OMAP_I2C_STAT_AL)) ...
> 
> The patch restore pre 1d7afc9 logic of handling NACK and AL interrupts, so
> no interrupts is fired after ISR informs the rest of driver what transfer
> complete.
> 
> Note: instead of removing break under NACK case, we could just replace 'break'
> with 'continue' and allow NACK transfer to finish using ARDY event. I found
> that NACK and ARDY bits usually set together. That case confirm TI wiki:
> http://processors.wiki.ti.com/index.php/I2C_Tips#Detecting_and_handling_NACK
> 
> In order if someone interested in the event traces for NACK and AL cases,
> I sent them to mailing list.
> 
> Tested on Beagleboard XM C.
> 
> Signed-off-by: Alexander Kochetkov <al.kochet@...il.com>
> Cc: <stable@...r.kernel.org> # v3.7+

This is good, but subject is wrong. That fixes line should not be the
subject, it should be here after Cc :-)

Other than that, I'd just ask for Aaro to test on his legacy systems.

> ---
>  drivers/i2c/busses/i2c-omap.c |    2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index 90dcc2e..9af7095 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -926,14 +926,12 @@ omap_i2c_isr_thread(int this_irq, void *dev_id)
>  		if (stat & OMAP_I2C_STAT_NACK) {
>  			err |= OMAP_I2C_STAT_NACK;
>  			omap_i2c_ack_stat(dev, OMAP_I2C_STAT_NACK);
> -			break;
>  		}
>  
>  		if (stat & OMAP_I2C_STAT_AL) {
>  			dev_err(dev->dev, "Arbitration lost\n");
>  			err |= OMAP_I2C_STAT_AL;
>  			omap_i2c_ack_stat(dev, OMAP_I2C_STAT_AL);
> -			break;
>  		}
>  
>  		/*
> -- 
> 1.7.9.5
> 

-- 
balbi

Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)