linux-kernel - Re: [PATCH] serio: PS2 gpio bit banging driver for the serio bus

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]

Date:   Fri, 11 Aug 2017 11:16:20 +0200
From:   Linus Walleij <linus.walleij@...aro.org>
To:     Danilo Krummrich <danilokrummrich@...develop.de>,
        Russell King <linux@...linux.org.uk>,
        Will Deacon <will.deacon@....com>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>
Cc:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Linux Input <linux-input@...r.kernel.org>,
        Dmitry Torokhov <dmitry.torokhov@...il.com>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>
Subject: Re: [PATCH] serio: PS2 gpio bit banging driver for the serio bus

On Thu, Aug 10, 2017 at 4:38 PM, Danilo Krummrich
<danilokrummrich@...develop.de> wrote:
> On 2017-08-07 18:22, Danilo Krummrich wrote:
>>>
>>> > +static int ps2_gpio_write(struct serio *serio, unsigned char val)
>>> > +{
>>> > +       struct ps2_gpio_data *drvdata = serio->port_data;
>>> > +
>>> > +       drvdata->mode = PS2_MODE_TX;
>>> > +       drvdata->tx_byte = val;
>>> > +       /* Make sure ISR running on other CPU notice changes. */
>>> > +       barrier();
>>>
>>> This seems overengineered, is this really needed?
>>>
>>> If we have races like this, the error is likely elsewhere, and should be
>>> fixed in the GPIO driver MMIO access or so.
>>>
>> Yes, seems it can be removed. I didn't saw any explicit barriers in the
>> GPIO
>> driver (I'm testing on bcm2835), but it seems MMIO operations on SMP archs
>> does contain barriers. Not sure if all do. If some do not this barrier
>> might
>> be needed to ensure ISR on other CPU notice the correct mode and byte to
>> send.
>>
> I couldn't find any guarantee that the mode and tx_byte change is implicitly
> covered by a barrier in this case. E.g. the bcm2835 driver does not make
> sure stores are completed before the particular interrupt is enabled, except by
> the fact that writel on ARM contains a wmb(). But this is nothing to rely on.
> (Please tell me if I miss something.)

writel() should be guaranteeing that the values hit the hardware, wmb() is
spelled out "write memory barrier" I don't see what you're after here.

If you think writel() doesn't do its job on some platform, then fix writel()
on that platform.

We can't randomly sprinkle things like this all over the kernel it makes
no sense.

> Therefore I would like to keep this barrier and replace it with smp_wmb() if
> you are fine with that.

I do not think this is proper.

Yours,
Linus Walleij