linux-kernel - Re: [PATCH] i2c: cadence: Implement timeout

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

Message-ID: <CAKfKVtFLssjC3j3yHvQ98TNGSf=DRdMO+YdUyMn0o91+Jn10Zw@mail.gmail.com>
Date:   Wed, 18 Dec 2019 16:31:44 +0530
From:   Shubhrajyoti Datta <shubhrajyoti.datta@...il.com>
To:     Russell King - ARM Linux <linux@...linux.org.uk>
Cc:     linux-i2c <linux-i2c@...r.kernel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Shubhrajyoti Datta <shubhrajyoti.datta@...inx.com>,
        Michal Simek <michal.simek@...inx.com>,
        linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH] i2c: cadence: Implement timeout

On Wed, Oct 24, 2018 at 4:29 PM Russell King - ARM Linux
<linux@...linux.org.uk> wrote:
>
> On Wed, Oct 24, 2018 at 04:20:03PM +0530, shubhrajyoti.datta@...il.com wrote:
> > From: Shubhrajyoti Datta <shubhrajyoti.datta@...inx.com>
> >
> > In some cases we are waiting in a loop. Replace the infinite wait with
> > the  timeout.
> >
> > Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@...inx.com>
> > ---
> >  drivers/i2c/busses/i2c-cadence.c | 30 ++++++++++++++++++++++++++----
> >  1 file changed, 26 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-cadence.c b/drivers/i2c/busses/i2c-cadence.c
> > index b136057..9c38278 100644
> > --- a/drivers/i2c/busses/i2c-cadence.c
> > +++ b/drivers/i2c/busses/i2c-cadence.c
> > @@ -209,6 +209,7 @@ static irqreturn_t cdns_i2c_isr(int irq, void *ptr)
> >       struct cdns_i2c *id = ptr;
> >       /* Signal completion only after everything is updated */
> >       int done_flag = 0;
> > +     unsigned int timeout;
> >       irqreturn_t status = IRQ_NONE;
> >
> >       isr_status = cdns_i2c_readreg(CDNS_I2C_ISR_OFFSET);
> > @@ -235,6 +236,7 @@ static irqreturn_t cdns_i2c_isr(int irq, void *ptr)
> >           ((isr_status & CDNS_I2C_IXR_COMP) ||
> >            (isr_status & CDNS_I2C_IXR_DATA))) {
> >               /* Read data if receive data valid is set */
> > +             timeout = 1000;
> >               while (cdns_i2c_readreg(CDNS_I2C_SR_OFFSET) &
> >                      CDNS_I2C_SR_RXDV) {
> >                       /*
> > @@ -253,6 +255,16 @@ static irqreturn_t cdns_i2c_isr(int irq, void *ptr)
> >
> >                       if (cdns_is_holdquirk(id, hold_quirk))
> >                               break;
> > +                     timeout--;
> > +                     if (timeout)
> > +                             mdelay(1);
> > +                     else
> > +                             break;
> > +             }
> > +             if (!timeout) {
> > +                     id->err_status = -ETIMEDOUT;
> > +                     complete(&id->xfer_done);
> > +                     return IRQ_HANDLED;
>
> Good kernel programming principle: Always check for the success
> condition when exiting due to timeout rather than the fact that we
> timed out.
>
> Also, is this _really_ a loop that needs a timeout condition?  Looking
> at the original code, it looks like the purpose of the loop is to read
> more than one byte, and you are introducing a 1ms delay between the
> read of each byte.
Thanks for the review.
I agree will skip this patch.