linux-kernel - Re: [PATCH 3/8] i2c: sprd: Use global variables to record IIC ack/nack status instead of local variables

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>] [day] [month] [year] [list]

Message-ID: <CAAfSe-vwueijaYo2_rw3CO2ghZUN7hzoXEzRYS1gkB5ez2LXqw@mail.gmail.com>
Date:   Wed, 13 Sep 2023 16:28:44 +0800
From:   Chunyan Zhang <zhang.lyra@...il.com>
To:     huangzheng lai <laihuangzheng@...il.com>
Cc:     Andi Shyti <andi.shyti@...nel.org>,
        Huangzheng Lai <Huangzheng.Lai@...soc.com>,
        Orson Zhai <orsonzhai@...il.com>,
        Baolin Wang <baolin.wang@...ux.alibaba.com>,
        linux-i2c@...r.kernel.org, linux-kernel@...r.kernel.org,
        Xiongpeng Wu <xiongpeng.wu@...soc.com>
Subject: Re: [PATCH 3/8] i2c: sprd: Use global variables to record IIC
 ack/nack status instead of local variables

On Wed, 13 Sept 2023 at 15:12, huangzheng lai <laihuangzheng@...il.com> wrote:
>
> Hi Chunyan，
>
> I don't think it's necessary to clear  i2c_dev->ack_flag in sprd_i2c_clear_ack() . Because every time a new interrupt is triggered, it will retrieve the value of i2c_dev->ack_flag in sprd_i2c_isr and then use it in sprd_i2c_isr_thread.

You're assuming that ack_flag is and will be used in sprd_i2c_isr_thread() only.

If ack_flag is used to represent the ack/nack bit of interrupt status
register, I think we should clear it as well when clearing ack/nack
bit.

BTW, please make sure your email is plain-text mode so that people can
receive from the mail list, and do not top-post again.

Thanks,
Chunyan

>
> On Sun, Sep 3, 2023 at 5:05 AM Andi Shyti <andi.shyti@...nel.org> wrote:
>>
>> Hi Huangzheng,
>>
>> On Thu, Aug 17, 2023 at 05:45:15PM +0800, Huangzheng Lai wrote:
>> > We found that when the interrupt bit of the IIC 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.
>> >
>> > Signed-off-by: Huangzheng Lai <Huangzheng.Lai@...soc.com>
>>
>> Is this a fix? Then please consider adding
>>
>> Fixes: 8b9ec0719834 ("i2c: Add Spreadtrum I2C controller driver")
>> Cc: <stable@...r.kernel.org> # v4.14+
>>
>> > ---
>> >  drivers/i2c/busses/i2c-sprd.c | 10 +++++-----
>> >  1 file changed, 5 insertions(+), 5 deletions(-)
>> >
>> > diff --git a/drivers/i2c/busses/i2c-sprd.c b/drivers/i2c/busses/i2c-sprd.c
>> > index 066b3a9c30c8..549b60dd3273 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;
>>
>> smells a bit racy... however we are in the same interrupt cycle.
>>
>> Do you think we might need a spinlock around here?
>>
>> >       struct completion complete;
>> >       struct reset_control *rst;
>> >       u8 *buf;
>> > @@ -384,7 +385,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);
>> >       u32 i2c_tran;
>> >
>> >       if (msg->flags & I2C_M_RD)
>> > @@ -400,7 +400,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;
>> >       }
>> > @@ -411,7 +411,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);
>> > @@ -428,7 +428,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)
>> > @@ -447,7 +446,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);
>>
>> there is a question from Chunyan here.
>>
>> I like more
>>
>>         val = readl(...);
>>         i2c_dev->ack_flag = !(val & I2C_RX_ACK);
>>
>> a matter of taste, your choice.
>>
>> Andi
>>
>> > +     if (!i2c_tran || !i2c_dev->ack_flag) {
>> >               sprd_i2c_clear_start(i2c_dev);
>> >               sprd_i2c_clear_irq(i2c_dev);
>> >       }
>> > --
>> > 2.17.1
>> >