[V2,3/7] i2c: sprd: Use global variables to record I2C ack/nack status instead of local variables

We found that when the interrupt bit of the I2C controller is cleared,
the ack/nack bit is also cleared at the same time. After clearing the
interrupt bit in sprd_i2c_isr(), incorrect ack/nack information will be
obtained in sprd_i2c_isr_thread(), resulting in incorrect communication
when nack cannot be recognized. To solve this problem, we used a global
variable to record ack/nack information before clearing the interrupt
bit instead of a local variable.

Fixes: 8b9ec0719834 ("i2c: Add Spreadtrum I2C controller driver")
Cc: <stable@vger.kernel.org> # v4.14+
Signed-off-by: Huangzheng Lai <Huangzheng.Lai@unisoc.com>
---
 drivers/i2c/busses/i2c-sprd.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

Hi Huangzheng,

On Mon, Oct 23, 2023 at 04:11:54PM +0800, Huangzheng Lai wrote:
> We found that when the interrupt bit of the I2C controller is cleared,
> the ack/nack bit is also cleared at the same time. After clearing the
> interrupt bit in sprd_i2c_isr(), incorrect ack/nack information will be
> obtained in sprd_i2c_isr_thread(), resulting in incorrect communication
> when nack cannot be recognized. To solve this problem, we used a global
> variable to record ack/nack information before clearing the interrupt
> bit instead of a local variable.
> 
> Fixes: 8b9ec0719834 ("i2c: Add Spreadtrum I2C controller driver")
> Cc: <stable@vger.kernel.org> # v4.14+
> Signed-off-by: Huangzheng Lai <Huangzheng.Lai@unisoc.com>
> ---
>  drivers/i2c/busses/i2c-sprd.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-sprd.c b/drivers/i2c/busses/i2c-sprd.c
> index aa602958d4fd..dec627ef408c 100644
> --- a/drivers/i2c/busses/i2c-sprd.c
> +++ b/drivers/i2c/busses/i2c-sprd.c
> @@ -85,6 +85,7 @@ struct sprd_i2c {
>  	struct clk *clk;
>  	u32 src_clk;
>  	u32 bus_freq;
> +	bool ack_flag;

So that you are telling me that this is not racy because we won't
receive any irq until we pull the ack down. Am I understanding
correctly?

But if this patch is fixing an unstable ack flag, how can I
believe this won't end up into a race?

>  	struct completion complete;
>  	struct reset_control *rst;
>  	u8 *buf;
> @@ -119,6 +120,7 @@ static void sprd_i2c_clear_ack(struct sprd_i2c *i2c_dev)
>  {
>  	u32 tmp = readl(i2c_dev->base + I2C_STATUS);
>  
> +	i2c_dev->ack_flag = 0;
>  	writel(tmp & ~I2C_RX_ACK, i2c_dev->base + I2C_STATUS);
>  }
>  
> @@ -393,7 +395,6 @@ static irqreturn_t sprd_i2c_isr_thread(int irq, void *dev_id)
>  {
>  	struct sprd_i2c *i2c_dev = dev_id;
>  	struct i2c_msg *msg = i2c_dev->msg;
> -	bool ack = !(readl(i2c_dev->base + I2C_STATUS) & I2C_RX_ACK);

Where exactly did you see the ack going to '0', here in the
thread or in handler?

>  	u32 i2c_tran;
>  
>  	if (msg->flags & I2C_M_RD)
> @@ -409,7 +410,7 @@ static irqreturn_t sprd_i2c_isr_thread(int irq, void *dev_id)
>  	 * For reading data, ack is always true, if i2c_tran is not 0 which
>  	 * means we still need to contine to read data from slave.
>  	 */
> -	if (i2c_tran && ack) {
> +	if (i2c_tran && i2c_dev->ack_flag) {
>  		sprd_i2c_data_transfer(i2c_dev);
>  		return IRQ_HANDLED;
>  	}
> @@ -420,7 +421,7 @@ static irqreturn_t sprd_i2c_isr_thread(int irq, void *dev_id)
>  	 * If we did not get one ACK from slave when writing data, we should
>  	 * return -EIO to notify users.
>  	 */
> -	if (!ack)
> +	if (!i2c_dev->ack_flag)
>  		i2c_dev->err = -EIO;
>  	else if (msg->flags & I2C_M_RD && i2c_dev->count)
>  		sprd_i2c_read_bytes(i2c_dev, i2c_dev->buf, i2c_dev->count);
> @@ -437,7 +438,6 @@ static irqreturn_t sprd_i2c_isr(int irq, void *dev_id)
>  {
>  	struct sprd_i2c *i2c_dev = dev_id;
>  	struct i2c_msg *msg = i2c_dev->msg;
> -	bool ack = !(readl(i2c_dev->base + I2C_STATUS) & I2C_RX_ACK);
>  	u32 i2c_tran;
>  
>  	if (msg->flags & I2C_M_RD)
> @@ -456,7 +456,8 @@ static irqreturn_t sprd_i2c_isr(int irq, void *dev_id)
>  	 * means we can read all data in one time, then we can finish this
>  	 * transmission too.
>  	 */
> -	if (!i2c_tran || !ack) {
> +	i2c_dev->ack_flag = !(readl(i2c_dev->base + I2C_STATUS) & I2C_RX_ACK);
> +	if (!i2c_tran || !i2c_dev->ack_flag) {
>  		sprd_i2c_clear_start(i2c_dev);

this clear_start() is called both here and in the thread, why?

Andi

>  		sprd_i2c_clear_irq(i2c_dev);
>  	}
> -- 
> 2.17.1
>

On 23/10/2023 10:11, Huangzheng Lai wrote:
> We found that when the interrupt bit of the I2C controller is cleared,
> the ack/nack bit is also cleared at the same time. After clearing the
> interrupt bit in sprd_i2c_isr(), incorrect ack/nack information will be
> obtained in sprd_i2c_isr_thread(), resulting in incorrect communication
> when nack cannot be recognized. To solve this problem, we used a global
> variable to record ack/nack information before clearing the interrupt
> bit instead of a local variable.
> 
> Fixes: 8b9ec0719834 ("i2c: Add Spreadtrum I2C controller driver")
> Cc: <stable@vger.kernel.org> # v4.14+
> Signed-off-by: Huangzheng Lai <Huangzheng.Lai@unisoc.com>

Fixes must be send independent of features.

Best regards,
Krzysztof

On Mon, Oct 23, 2023 at 7:32 PM Baolin Wang
<baolin.wang@linux.alibaba.com> wrote:
>
>
>
> On 10/23/2023 4:11 PM, Huangzheng Lai wrote:
> > We found that when the interrupt bit of the I2C controller is cleared,
> > the ack/nack bit is also cleared at the same time. After clearing the
> > interrupt bit in sprd_i2c_isr(), incorrect ack/nack information will be
> > obtained in sprd_i2c_isr_thread(), resulting in incorrect communication
> > when nack cannot be recognized. To solve this problem, we used a global
>
> This is a hardware bug?
>

Yes.

> > variable to record ack/nack information before clearing the interrupt
> > bit instead of a local variable.
> >
> > Fixes: 8b9ec0719834 ("i2c: Add Spreadtrum I2C controller driver")
> > Cc: <stable@vger.kernel.org> # v4.14+
> > Signed-off-by: Huangzheng Lai <Huangzheng.Lai@unisoc.com>
> > ---
> >   drivers/i2c/busses/i2c-sprd.c | 11 ++++++-----
> >   1 file changed, 6 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-sprd.c b/drivers/i2c/busses/i2c-sprd.c
> > index aa602958d4fd..dec627ef408c 100644
> > --- a/drivers/i2c/busses/i2c-sprd.c
> > +++ b/drivers/i2c/busses/i2c-sprd.c
> > @@ -85,6 +85,7 @@ struct sprd_i2c {
> >       struct clk *clk;
> >       u32 src_clk;
> >       u32 bus_freq;
> > +     bool ack_flag;
> >       struct completion complete;
> >       struct reset_control *rst;
> >       u8 *buf;
> > @@ -119,6 +120,7 @@ static void sprd_i2c_clear_ack(struct sprd_i2c *i2c_dev)
> >   {
> >       u32 tmp = readl(i2c_dev->base + I2C_STATUS);
> >
> > +     i2c_dev->ack_flag = 0;
> >       writel(tmp & ~I2C_RX_ACK, i2c_dev->base + I2C_STATUS);
> >   }
> >
> > @@ -393,7 +395,6 @@ static irqreturn_t sprd_i2c_isr_thread(int irq, void *dev_id)
> >   {
> >       struct sprd_i2c *i2c_dev = dev_id;
> >       struct i2c_msg *msg = i2c_dev->msg;
> > -     bool ack = !(readl(i2c_dev->base + I2C_STATUS) & I2C_RX_ACK);
>
> Before this patch, we will re-read the ack bit form the register, but
> now we just read it in sprd_i2c_isr(). Is it possible that we will miss
> the ack bit?
>

Yes, we will miss the 'ack' bit after clear 'irq' bit.

> >       u32 i2c_tran;
> >
> >       if (msg->flags & I2C_M_RD)
> > @@ -409,7 +410,7 @@ static irqreturn_t sprd_i2c_isr_thread(int irq, void *dev_id)
> >        * For reading data, ack is always true, if i2c_tran is not 0 which
> >        * means we still need to contine to read data from slave.
> >        */
> > -     if (i2c_tran && ack) {
> > +     if (i2c_tran && i2c_dev->ack_flag) {
> >               sprd_i2c_data_transfer(i2c_dev);
> >               return IRQ_HANDLED;
> >       }
> > @@ -420,7 +421,7 @@ static irqreturn_t sprd_i2c_isr_thread(int irq, void *dev_id)
> >        * If we did not get one ACK from slave when writing data, we should
> >        * return -EIO to notify users.
> >        */
> > -     if (!ack)
> > +     if (!i2c_dev->ack_flag)
> >               i2c_dev->err = -EIO;
> >       else if (msg->flags & I2C_M_RD && i2c_dev->count)
> >               sprd_i2c_read_bytes(i2c_dev, i2c_dev->buf, i2c_dev->count);
> > @@ -437,7 +438,6 @@ static irqreturn_t sprd_i2c_isr(int irq, void *dev_id)
> >   {
> >       struct sprd_i2c *i2c_dev = dev_id;
> >       struct i2c_msg *msg = i2c_dev->msg;
> > -     bool ack = !(readl(i2c_dev->base + I2C_STATUS) & I2C_RX_ACK);
> >       u32 i2c_tran;
> >
> >       if (msg->flags & I2C_M_RD)
> > @@ -456,7 +456,8 @@ static irqreturn_t sprd_i2c_isr(int irq, void *dev_id)
> >        * means we can read all data in one time, then we can finish this
> >        * transmission too.
> >        */
> > -     if (!i2c_tran || !ack) {
> > +     i2c_dev->ack_flag = !(readl(i2c_dev->base + I2C_STATUS) & I2C_RX_ACK);
> > +     if (!i2c_tran || !i2c_dev->ack_flag) {
> >               sprd_i2c_clear_start(i2c_dev);
> >               sprd_i2c_clear_irq(i2c_dev);
> >       }

Hi Andi,


On Wed, Oct 25, 2023 at 5:20 AM Andi Shyti <andi.shyti@kernel.org> wrote:
>
> Hi Huangzheng,
>
> On Mon, Oct 23, 2023 at 04:11:54PM +0800, Huangzheng Lai wrote:
> > We found that when the interrupt bit of the I2C controller is cleared,
> > the ack/nack bit is also cleared at the same time. After clearing the
> > interrupt bit in sprd_i2c_isr(), incorrect ack/nack information will be
> > obtained in sprd_i2c_isr_thread(), resulting in incorrect communication
> > when nack cannot be recognized. To solve this problem, we used a global
> > variable to record ack/nack information before clearing the interrupt
> > bit instead of a local variable.
> >
> > Fixes: 8b9ec0719834 ("i2c: Add Spreadtrum I2C controller driver")
> > Cc: <stable@vger.kernel.org> # v4.14+
> > Signed-off-by: Huangzheng Lai <Huangzheng.Lai@unisoc.com>
> > ---
> >  drivers/i2c/busses/i2c-sprd.c | 11 ++++++-----
> >  1 file changed, 6 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-sprd.c b/drivers/i2c/busses/i2c-sprd.c
> > index aa602958d4fd..dec627ef408c 100644
> > --- a/drivers/i2c/busses/i2c-sprd.c
> > +++ b/drivers/i2c/busses/i2c-sprd.c
> > @@ -85,6 +85,7 @@ struct sprd_i2c {
> >       struct clk *clk;
> >       u32 src_clk;
> >       u32 bus_freq;
> > +     bool ack_flag;
>
> So that you are telling me that this is not racy because we won't
> receive any irq until we pull the ack down. Am I understanding
> correctly?
>
> But if this patch is fixing an unstable ack flag, how can I
> believe this won't end up into a race?
>

In fact, the ack flag is not unstable. However, the ack state on the
hardware will
reset as the interrupt state is cleared, nd there are some unexpected coupling
relationships on the hardware.We need to obtain and save the ack flag before
clearing the interrupt state.

> >       struct completion complete;
> >       struct reset_control *rst;
> >       u8 *buf;
> > @@ -119,6 +120,7 @@ static void sprd_i2c_clear_ack(struct sprd_i2c *i2c_dev)
> >  {
> >       u32 tmp = readl(i2c_dev->base + I2C_STATUS);
> >
> > +     i2c_dev->ack_flag = 0;
> >       writel(tmp & ~I2C_RX_ACK, i2c_dev->base + I2C_STATUS);
> >  }
> >
> > @@ -393,7 +395,6 @@ static irqreturn_t sprd_i2c_isr_thread(int irq, void *dev_id)
> >  {
> >       struct sprd_i2c *i2c_dev = dev_id;
> >       struct i2c_msg *msg = i2c_dev->msg;
> > -     bool ack = !(readl(i2c_dev->base + I2C_STATUS) & I2C_RX_ACK);
>
> Where exactly did you see the ack going to '0', here in the
> thread or in handler?
>

After function sprd_i2c_irq() is executed, the ack state bit in the
hardware will be reset to the default ack.
When the slave nack, the ack flag obtained in the thread is incorrect,
which can cause
the i2c driver to mistakenly believe that the transmission was successful.

> >       u32 i2c_tran;
> >
> >       if (msg->flags & I2C_M_RD)
> > @@ -409,7 +410,7 @@ static irqreturn_t sprd_i2c_isr_thread(int irq, void *dev_id)
> >        * For reading data, ack is always true, if i2c_tran is not 0 which
> >        * means we still need to contine to read data from slave.
> >        */
> > -     if (i2c_tran && ack) {
> > +     if (i2c_tran && i2c_dev->ack_flag) {
> >               sprd_i2c_data_transfer(i2c_dev);
> >               return IRQ_HANDLED;
> >       }
> > @@ -420,7 +421,7 @@ static irqreturn_t sprd_i2c_isr_thread(int irq, void *dev_id)
> >        * If we did not get one ACK from slave when writing data, we should
> >        * return -EIO to notify users.
> >        */
> > -     if (!ack)
> > +     if (!i2c_dev->ack_flag)
> >               i2c_dev->err = -EIO;
> >       else if (msg->flags & I2C_M_RD && i2c_dev->count)
> >               sprd_i2c_read_bytes(i2c_dev, i2c_dev->buf, i2c_dev->count);
> > @@ -437,7 +438,6 @@ static irqreturn_t sprd_i2c_isr(int irq, void *dev_id)
> >  {
> >       struct sprd_i2c *i2c_dev = dev_id;
> >       struct i2c_msg *msg = i2c_dev->msg;
> > -     bool ack = !(readl(i2c_dev->base + I2C_STATUS) & I2C_RX_ACK);
> >       u32 i2c_tran;
> >
> >       if (msg->flags & I2C_M_RD)
> > @@ -456,7 +456,8 @@ static irqreturn_t sprd_i2c_isr(int irq, void *dev_id)
> >        * means we can read all data in one time, then we can finish this
> >        * transmission too.
> >        */
> > -     if (!i2c_tran || !ack) {
> > +     i2c_dev->ack_flag = !(readl(i2c_dev->base + I2C_STATUS) & I2C_RX_ACK);
> > +     if (!i2c_tran || !i2c_dev->ack_flag) {
> >               sprd_i2c_clear_start(i2c_dev);
>
> this clear_start() is called both here and in the thread, why?
>

When it is determined here that there is no remaining i2c data or slave nack,
clear_start() is called for stopping i2c controller transmission.
In the thread, clear_start() is called because all i2c data
reads/writes are completed.

> Andi
>
> >               sprd_i2c_clear_irq(i2c_dev);
> >       }
> > --
> > 2.17.1
> >