[1/3] i2c: designware: avoid race with interrupt handler

The interrupt handler dw_i2c_isr may be racing with i2c_dw_xfer_init.
Use disable_irq/enable_irq when setting up the adapter.

Cc: Ben Gardner <gardner.ben@gmail.com>
Cc: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Cc: Wolfram Sang <wsa@the-dreams.de>
Signed-off-by: Alexander Monakov <amonakov@ispras.ru>
---
 drivers/i2c/busses/i2c-designware-master.c | 2 ++
 1 file changed, 2 insertions(+)

Hi

On 04/21/18 22:11, Alexander Monakov wrote:
> The interrupt handler dw_i2c_isr may be racing with i2c_dw_xfer_init.
> Use disable_irq/enable_irq when setting up the adapter.
> 
> Cc: Ben Gardner <gardner.ben@gmail.com>
> Cc: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> Cc: Wolfram Sang <wsa@the-dreams.de>
> Signed-off-by: Alexander Monakov <amonakov@ispras.ru>
> ---
>   drivers/i2c/busses/i2c-designware-master.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c
> index fd36c39ddf4e..e7fd0e57ab1f 100644
> --- a/drivers/i2c/busses/i2c-designware-master.c
> +++ b/drivers/i2c/busses/i2c-designware-master.c
> @@ -181,6 +181,7 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev *dev)
>   
>   	/* Disable the adapter */
>   	__i2c_dw_enable_and_wait(dev, false);
> +	disable_irq(dev->irq);
>   
>   	/* If the slave address is ten bit address, enable 10BITADDR */
>   	ic_con = dw_readl(dev, DW_IC_CON);
> @@ -214,6 +215,7 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev *dev)
>   	/* Clear and enable interrupts */
>   	dw_readl(dev, DW_IC_CLR_INTR);
>   	dw_writel(dev, DW_IC_INTR_MASTER_MASK, DW_IC_INTR_MASK);
> +	enable_irq(dev->irq);

Are you seeing an issue here? Not sure about low level irq handling but 
thinking here can we miss an interrupt between unmasking and enable_irq()?

I'm also thinking is there actually any issue if i2c_dw_isr() runs while 
executing the i2c_dw_xfer_init() e.g. if the IRQ line is shared since 
the i2c_dw_isr() is checking the DW_IC_ENABLE which is not enabled until 
near at the end of i2c_dw_isr().

On Tue, 24 Apr 2018, Jarkko Nikula wrote:
> > --- a/drivers/i2c/busses/i2c-designware-master.c
> > +++ b/drivers/i2c/busses/i2c-designware-master.c
> > @@ -181,6 +181,7 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev *dev)
> >     	/* Disable the adapter */
> >   	__i2c_dw_enable_and_wait(dev, false);
> > +	disable_irq(dev->irq);
> >     	/* If the slave address is ten bit address, enable 10BITADDR */
> >   	ic_con = dw_readl(dev, DW_IC_CON);
> > @@ -214,6 +215,7 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev *dev)
> >   	/* Clear and enable interrupts */
> >   	dw_readl(dev, DW_IC_CLR_INTR);
> >   	dw_writel(dev, DW_IC_INTR_MASTER_MASK, DW_IC_INTR_MASK);
> > +	enable_irq(dev->irq);
> 
> Are you seeing an issue here? Not sure about low level irq handling but
> thinking here can we miss an interrupt between unmasking and enable_irq()?

Right, sorry, enable_irq should be between clearing and unmasking.

> I'm also thinking is there actually any issue if i2c_dw_isr() runs while
> executing the i2c_dw_xfer_init() e.g. if the IRQ line is shared since the
> i2c_dw_isr() is checking the DW_IC_ENABLE which is not enabled until near at
> the end of i2c_dw_isr().

Imagine that i2c_dw_isr and i2c_dw_xfer_init begin running concurrently. First
the isr observes non-zero DW_IC_ENABLE and proceeds. Then xfer_init waits until
DW_IC_ENABLE becomes zero and also proceeds. At this point both the isr and
xfer_init may access the adapter registers concurrently.

Alexander