linux-kernel - Re: [PATCH v4 1/1] Input: atmel_mxt_ts

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

Message-ID: <CAHp75VdTv-uCQue3VU=czZJd4iTG+XBVe2kFtnP+fZ1XQuFbzA@mail.gmail.com>
Date:   Mon, 14 Sep 2020 16:49:08 +0300
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Dmitry Osipenko <digetx@...il.com>
Cc:     Jiada Wang <jiada_wang@...tor.com>, nick@...anahar.org,
        Dmitry Torokhov <dmitry.torokhov@...il.com>,
        linux-input <linux-input@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Eugeniu Rosca <erosca@...adit-jv.com>,
        Andrew_Gabbasov@...tor.com
Subject: Re: [PATCH v4 1/1] Input: atmel_mxt_ts - implement I2C retries

On Sun, Sep 13, 2020 at 3:57 PM Dmitry Osipenko <digetx@...il.com> wrote:
>
> 13.09.2020 11:43, Andy Shevchenko пишет:
> > ...
> >
> >> +       bool retried = false;
>
> > I thought Dmitry wants that to be retry
>
> In the comment to v2 you suggested to negate the condition,

Where? I just checked a few messages before and I found that I asked
the same question: why is negative conditional used instead of
positive.

> hence I
> thought it's YOU who wants it to be retried.

I see. Let's see how it goes with positive conditionals first.


> The "retried" is a very common form among kernel drivers, so it's good
> to me.
>
> >>         u8 buf[2];
> >>         int ret;
> >
> >> -       ret = i2c_transfer(client->adapter, xfer, 2);
> >> -       if (ret == 2) {
> >> -               ret = 0;
> >> -       } else {
> >> -               if (ret >= 0)
> >> -                       ret = -EIO;
> >> +retry_read:
> >
> >> +       ret = i2c_transfer(client->adapter, xfer, ARRAY_SIZE(xfer));
> >> +       if (ret != ARRAY_SIZE(xfer)) {
> ...> Also why switch from positive to negative conditional?
>
> This will make code less readable because of the goto, and thus, there
> will be two branches for handling of the returned value instead of one +
> goto. Hence this part is good to me as-is.

But it's not the purpose of this patch, right?
Style changes should be really separated from the fix.
And since it's a fix it should have a Fixes tag.

>
> >> +               if (!retried) {
> >> +                       dev_dbg(&client->dev, "i2c retry\n");
> >> +                       msleep(MXT_WAKEUP_TIME);
> >> +                       retried = true;
> >> +                       goto retry_read;
> >> +               }
> >> +               ret = ret < 0 ? ret : -EIO;
> >>                 dev_err(&client->dev, "%s: i2c transfer failed (%d)\n",
> >>                         __func__, ret);
> >> +               return ret;
> >>         }
> >>
> >> -       return ret;
> >> +       return 0;
> >>  }



-- 
With Best Regards,
Andy Shevchenko