linux-kernel - Re: [PATCH v17 2/3] drivers: input: keyboard: Add mtk keypad driver

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: <CAHp75VfKckoK4facC3nMDLLUYUtd5Y66+E5xgkVpVYfyTTa9Hg@mail.gmail.com>
Date:   Thu, 13 Aug 2020 13:00:24 +0300
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc:     Fengping Yu <fengping.yu@...iatek.com>,
        Yingjoe Chen <yingjoe.chen@...iatek.com>,
        Rob Herring <robh+dt@...nel.org>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Marco Felsch <m.felsch@...gutronix.de>,
        linux-input <linux-input@...r.kernel.org>,
        devicetree <devicetree@...r.kernel.org>,
        "moderated list:ARM/Mediatek SoC support" 
        <linux-mediatek@...ts.infradead.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-arm Mailing List <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v17 2/3] drivers: input: keyboard: Add mtk keypad driver

On Thu, Aug 13, 2020 at 3:57 AM Dmitry Torokhov
<dmitry.torokhov@...il.com> wrote:
> On Mon, Aug 10, 2020 at 02:51:28PM +0800, Fengping Yu wrote:
> > From: "fengping.yu" <fengping.yu@...iatek.com>
> >
> > This patch adds matrix keypad support for Mediatek SoCs.

...

> > +static irqreturn_t kpd_irq_handler(int irq, void *dev_id)
> > +{
> > +     struct mtk_keypad *keypad = dev_id;
> > +     unsigned short *keycode = keypad->input_dev->keycode;
> > +     DECLARE_BITMAP(new_state, MTK_KPD_NUM_BITS);
> > +     DECLARE_BITMAP(change, MTK_KPD_NUM_BITS);
> > +     int bit_nr;
> > +     int pressed;
> > +     unsigned short code;
> > +
> > +     regmap_bulk_read(keypad->regmap, MTK_KPD_MEM,
> > +                     new_state, MTK_KPD_NUM_MEMS);
> > +
> > +     bitmap_xor(change, new_state, keypad->keymap_state, MTK_KPD_NUM_BITS);
> > +
> > +     for_each_set_bit(bit_nr, change, MTK_KPD_NUM_BITS) {
>
> Should we be explicit:
>
>                 if (bit_nr % 32 >= 16) // or "if ((bit_nr / 16) % 2)"
>                         continue;
>
> so that we sure we do not trip over garbage (if any) in the unused
> bits?

Shouldn't we rather rely on the fact that bitmap API explicitly takes
a bit number as an argument. What garbage are you thinking of?
If you are talking about gaps, then probably existing
for_each_set_clump8() or free size analogue (not yet in upstream,
though) should be used instead?

> > +             /* 1: not pressed, 0: pressed */
> > +             pressed = !test_bit(bit_nr, new_state);
> > +             dev_dbg(&keypad->input_dev->dev, "%s",
> > +                     pressed ? "pressed" : "released");
> > +
> > +             /* 32bit register only use low 16bit as keypad mem register */
> > +             code = keycode[bit_nr - 16 * (BITS_TO_U32(bit_nr) - 1)];
>
> This will give index of 16 for (0,0).

I was also puzzled by this in one of the review rounds, but I don't
remember what was the explanation.

> Is this what we want? Hmm, this is
> all weird... I think we need:
>
>                 row = bit_nr / 32;
>                 col = bit_nr % 32;
>                 if (col > 15)
>                         continue;
>
>                 // set row_shift in probe() as:
>                 // keypad_data->row_shift =
>                 //              get_count_order(keypad_data->n_cols);
>                 code = keycode[MATRIX_SCAN_CODE(row, col,
>                                                 keypad_data->row_shift)];
>
> This will properly unpack the keymap built by
> matrix_keypad_build_keymap().
>
> > +
> > +             input_report_key(keypad->input_dev, code, pressed);
> > +             input_sync(keypad->input_dev);
> > +
> > +             dev_dbg(&keypad->input_dev->dev,
> > +                     "report Linux keycode = %d\n", code);
> > +     }
> > +
> > +     bitmap_copy(keypad->keymap_state, new_state, MTK_KPD_NUM_BITS);
> > +
> > +     return IRQ_HANDLED;
> > +}

-- 
With Best Regards,
Andy Shevchenko