On 2017-08-11 11:16, Linus Walleij wrote:

On Thu, Aug 10, 2017 at 4:38 PM, Danilo Krummrich
[off-list ref] wrote:

quoted

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

quoted

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

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.

Sorry for confusing wording. What I actually meant is if writel() is 
guaranteed
to make sure there's no reordering happening with other store 
operations. Of
course, in case of ARM it is sufficient as it contains a wmb. But I 
wasn't aware
that all writel() implementations guarantee this (if needed).
Thanks for clarification.

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.

quoted

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.

As you explained writel() should guarantee no reordering with other 
store operations
(like drvdata->mode = PS2_MODE_TX in my case) is happening, I totally 
agree and will
fix this.

Yours,
Linus Walleij

Thanks,
Danilo