diff mbox series

[PATCHv4] i2c: cadence: Fix the driver in interrupt flurry case

Message ID 1551069922-15896-1-git-send-email-shubhrajyoti.datta@gmail.com
State Superseded
Headers show
Series [PATCHv4] i2c: cadence: Fix the driver in interrupt flurry case | expand

Commit Message

Shubhrajyoti Datta Feb. 25, 2019, 4:45 a.m. UTC 
From: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>

In case there are a lot of interrupts then the hold bit is not
released. Protect the code by making it atomic by disabling interrupts.

Fixes: df8eb5691c4 ("i2c: Add driver for Cadence I2C controller")
Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
---
v2: Update the wording
v3: Add fixes tag
v4: update fixes tag

 drivers/i2c/busses/i2c-cadence.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Michal Simek Feb. 25, 2019, 6:56 a.m. UTC | #1 
On 25. 02. 19 5:45, shubhrajyoti.datta@gmail.com wrote:
> From: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
> 
> In case there are a lot of interrupts then the hold bit is not
> released. Protect the code by making it atomic by disabling interrupts.
> 
> Fixes: df8eb5691c4 ("i2c: Add driver for Cadence I2C controller")
> Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
> ---
> v2: Update the wording
> v3: Add fixes tag
> v4: update fixes tag
> 
>  drivers/i2c/busses/i2c-cadence.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-cadence.c b/drivers/i2c/busses/i2c-cadence.c
> index b136057..92829be 100644
> --- a/drivers/i2c/busses/i2c-cadence.c
> +++ b/drivers/i2c/busses/i2c-cadence.c
> @@ -365,6 +365,7 @@ static void cdns_i2c_mrecv(struct cdns_i2c *id)
>  {
>  	unsigned int ctrl_reg;
>  	unsigned int isr_status;
> +	unsigned long flags;
>  
>  	id->p_recv_buf = id->p_msg->buf;
>  	id->recv_count = id->p_msg->len;
> @@ -405,6 +406,7 @@ static void cdns_i2c_mrecv(struct cdns_i2c *id)
>  		cdns_i2c_writereg(id->recv_count, CDNS_I2C_XFER_SIZE_OFFSET);
>  	}
>  
> +	local_irq_save(flags);
>  	/* Set the slave address in address register - triggers operation */
>  	cdns_i2c_writereg(id->p_msg->addr & CDNS_I2C_ADDR_MASK,
>  						CDNS_I2C_ADDR_OFFSET);
> @@ -413,6 +415,7 @@ static void cdns_i2c_mrecv(struct cdns_i2c *id)
>  		((id->p_msg->flags & I2C_M_RECV_LEN) != I2C_M_RECV_LEN) &&
>  		(id->recv_count <= CDNS_I2C_FIFO_DEPTH))
>  			cdns_i2c_clear_bus_hold(id);
> +	local_irq_restore(flags);
>  	cdns_i2c_writereg(CDNS_I2C_ENABLED_INTR_MASK, CDNS_I2C_IER_OFFSET);
>  }
>  
> 

Acked-by: Michal Simek <michal.simek@xilinx.com>

M
Wolfram Sang March 9, 2019, 9:52 a.m. UTC | #2 
On Mon, Feb 25, 2019 at 07:56:28AM +0100, Michal Simek wrote:
> On 25. 02. 19 5:45, shubhrajyoti.datta@gmail.com wrote:
> > From: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
> > 
> > In case there are a lot of interrupts then the hold bit is not
> > released. Protect the code by making it atomic by disabling interrupts.
> > 
> > Fixes: df8eb5691c4 ("i2c: Add driver for Cadence I2C controller")
> > Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
> > ---
> > v2: Update the wording
> > v3: Add fixes tag
> > v4: update fixes tag
> > 
> >  drivers/i2c/busses/i2c-cadence.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/i2c/busses/i2c-cadence.c b/drivers/i2c/busses/i2c-cadence.c
> > index b136057..92829be 100644
> > --- a/drivers/i2c/busses/i2c-cadence.c
> > +++ b/drivers/i2c/busses/i2c-cadence.c
> > @@ -365,6 +365,7 @@ static void cdns_i2c_mrecv(struct cdns_i2c *id)
> >  {
> >  	unsigned int ctrl_reg;
> >  	unsigned int isr_status;
> > +	unsigned long flags;
> >  
> >  	id->p_recv_buf = id->p_msg->buf;
> >  	id->recv_count = id->p_msg->len;
> > @@ -405,6 +406,7 @@ static void cdns_i2c_mrecv(struct cdns_i2c *id)
> >  		cdns_i2c_writereg(id->recv_count, CDNS_I2C_XFER_SIZE_OFFSET);
> >  	}
> >  
> > +	local_irq_save(flags);
> >  	/* Set the slave address in address register - triggers operation */
> >  	cdns_i2c_writereg(id->p_msg->addr & CDNS_I2C_ADDR_MASK,
> >  						CDNS_I2C_ADDR_OFFSET);
> > @@ -413,6 +415,7 @@ static void cdns_i2c_mrecv(struct cdns_i2c *id)
> >  		((id->p_msg->flags & I2C_M_RECV_LEN) != I2C_M_RECV_LEN) &&
> >  		(id->recv_count <= CDNS_I2C_FIFO_DEPTH))
> >  			cdns_i2c_clear_bus_hold(id);
> > +	local_irq_restore(flags);
> >  	cdns_i2c_writereg(CDNS_I2C_ENABLED_INTR_MASK, CDNS_I2C_IER_OFFSET);
> >  }
> >  
> > 
> 
> Acked-by: Michal Simek <michal.simek@xilinx.com>

What if another CPU core processes the interrupt handler?
Shubhrajyoti Datta March 14, 2019, 8:57 a.m. UTC | #3 
> > >     /* Set the slave address in address register - triggers operation */
> > >     cdns_i2c_writereg(id->p_msg->addr & CDNS_I2C_ADDR_MASK,
> > >                                             CDNS_I2C_ADDR_OFFSET);
> > > @@ -413,6 +415,7 @@ static void cdns_i2c_mrecv(struct cdns_i2c *id)
> > >             ((id->p_msg->flags & I2C_M_RECV_LEN) != I2C_M_RECV_LEN) &&
> > >             (id->recv_count <= CDNS_I2C_FIFO_DEPTH))
> > >                     cdns_i2c_clear_bus_hold(id);
> > > +   local_irq_restore(flags);
> > >     cdns_i2c_writereg(CDNS_I2C_ENABLED_INTR_MASK, CDNS_I2C_IER_OFFSET);
> > >  }
> > >
> > >
> >
> > Acked-by: Michal Simek <michal.simek@xilinx.com>
>
> What if another CPU core processes the interrupt handler?
>
The issue is that  due to  non i2c interrupts the  clear is not
released and the timeout is reached.
If those interrupts are serviced on the other core should  not be an issue.
Wolfram Sang March 14, 2019, 9:06 a.m. UTC | #4 
> > What if another CPU core processes the interrupt handler?
> >
> The issue is that  due to  non i2c interrupts the  clear is not
> released and the timeout is reached.
> If those interrupts are serviced on the other core should  not be an issue.

Makes sense. Can you add a comment describing this to the patch?
Shubhrajyoti Datta Nov. 21, 2019, 10:42 a.m. UTC | #5 
Hi Wolfram,

On Thu, Mar 14, 2019 at 2:36 PM Wolfram Sang <wsa@the-dreams.de> wrote:
>
>
> > > What if another CPU core processes the interrupt handler?
> > >
> > The issue is that  due to  non i2c interrupts the  clear is not
> > released and the timeout is reached.
> > If those interrupts are serviced on the other core should  not be an issue.
>
> Makes sense. Can you add a comment describing this to the patch?
>
Somehow the patch didnot reach the list resent now.

thanks and Regards,
Shubhrajyoti
diff mbox series

Patch

diff --git a/drivers/i2c/busses/i2c-cadence.c b/drivers/i2c/busses/i2c-cadence.c
index b136057..92829be 100644
--- a/drivers/i2c/busses/i2c-cadence.c
+++ b/drivers/i2c/busses/i2c-cadence.c
@@ -365,6 +365,7 @@  static void cdns_i2c_mrecv(struct cdns_i2c *id)
 {
 	unsigned int ctrl_reg;
 	unsigned int isr_status;
+	unsigned long flags;
 
 	id->p_recv_buf = id->p_msg->buf;
 	id->recv_count = id->p_msg->len;
@@ -405,6 +406,7 @@  static void cdns_i2c_mrecv(struct cdns_i2c *id)
 		cdns_i2c_writereg(id->recv_count, CDNS_I2C_XFER_SIZE_OFFSET);
 	}
 
+	local_irq_save(flags);
 	/* Set the slave address in address register - triggers operation */
 	cdns_i2c_writereg(id->p_msg->addr & CDNS_I2C_ADDR_MASK,
 						CDNS_I2C_ADDR_OFFSET);
@@ -413,6 +415,7 @@  static void cdns_i2c_mrecv(struct cdns_i2c *id)
 		((id->p_msg->flags & I2C_M_RECV_LEN) != I2C_M_RECV_LEN) &&
 		(id->recv_count <= CDNS_I2C_FIFO_DEPTH))
 			cdns_i2c_clear_bus_hold(id);
+	local_irq_restore(flags);
 	cdns_i2c_writereg(CDNS_I2C_ENABLED_INTR_MASK, CDNS_I2C_IER_OFFSET);
 }