Messages in this thread | | | Date | Fri, 11 Aug 2017 13:05:20 +0200 | From | Danilo Krummrich <> | Subject | Re: [PATCH] serio: PS2 gpio bit banging driver for the serio bus |
| |
On 2017-08-11 11:16, Linus Walleij wrote: > On Thu, Aug 10, 2017 at 4:38 PM, Danilo Krummrich > <danilokrummrich@dk-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. > 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. > >> 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
| |