Hi Linus,

On 2017-08-07 18:22, Danilo Krummrich wrote:

quoted

quoted

+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.)
Therefore I would like to keep this barrier and replace it with 
smp_wmb() if you
are fine with that.

Regards,
Danilo
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html