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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sun, 13 Sep 2020 11:43:41 +0300
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Jiada Wang <jiada_wang@...tor.com>
Cc:     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>,
        Dmitry Osipenko <digetx@...il.com>,
        Eugeniu Rosca <erosca@...adit-jv.com>,
        Andrew_Gabbasov@...tor.com
Subject: Re: [PATCH v4 1/1] Input: atmel_mxt_ts - implement I2C retries

On Sat, Sep 12, 2020 at 3:55 AM Jiada Wang <jiada_wang@...tor.com> wrote:
>
> From: Nick Dyer <nick.dyer@...ev.co.uk>
>
> Some maXTouch chips (eg mXT1386) will not respond on the first I2C request
> when they are in a sleep state. It must be retried after a delay for the
> chip to wake up.
>
> Signed-off-by: Nick Dyer <nick.dyer@...ev.co.uk>
> [gdavis: Forward port and fix conflicts.]
> Signed-off-by: George G. Davis <george_davis@...tor.com>
> [jiada: return exact errno when i2c_transfer & i2c_master_send fails
>         rename "retry" to "retried" and keep its order in length
>         set "ret" to correct errno before calling dev_err()

>         remove reduntant conditional]

redundant

> Signed-off-by: Jiada Wang <jiada_wang@...tor.com>
> Reviewed-by: Dmitry Osipenko <digetx@...il.com>
> Tested-by: Dmitry Osipenko <digetx@...il.com>


...

> +       bool retried = false;

I thought Dmitry wants that to be retry

>         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)) {

I'm wondering why you can't leave 2 as is and change it to ARRAY_SIZE
in a separate patch?
Also why switch from positive to negative conditional?

> +               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;
>  }

..

> +       bool retried = false;

Same comments here, in this function.

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

-- 
With Best Regards,
Andy Shevchenko