[PATCH] serio: PS2 gpio bit banging driver for the serio bus

[PATCH] serio: PS2 gpio bit banging driver for the serio bus

[Linus Walleij]

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.

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

[PATCH] serio: PS2 gpio bit banging driver for the serio bus

[Danilo Krummrich]

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

[PATCH] serio: PS2 gpio bit banging driver for the serio bus

[Russell King - ARM Linux]

On Fri, Aug 11, 2017 at 11:16:20AM +0200, Linus Walleij wrote:
> 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.

Incorrect.  writel() has a barrier which ensures that data written to
memory (eg, dma coherent memory) is visible to the hardware prior to
the write hitting the hardware.

There is no barrier to ensure that the write hits the hardware in a
timely manner - the write can be buffered by the buses, which will
delay it before it hits its destination.

PCI particularly buffers MMIO writes, and the requirement there has
always been that if you need the write to hit the hardware in a timely
fashion, you must perform a read-back to force the bus to deliver the
write (since a read is not allowed to overlap a write.)

The solution is never to use barrier() - barrier() is a _compiler_
barrier and does nothing for posted writes on hardware buses.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

[PATCH] serio: PS2 gpio bit banging driver for the serio bus

[Danilo Krummrich]

On 2017-08-17 11:09, Russell King - ARM Linux wrote:
> On Fri, Aug 11, 2017 at 11:16:20AM +0200, Linus Walleij wrote:
>> 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.
> 
> Incorrect.  writel() has a barrier which ensures that data written to
> memory (eg, dma coherent memory) is visible to the hardware prior to
> the write hitting the hardware.
> 
> There is no barrier to ensure that the write hits the hardware in a
> timely manner - the write can be buffered by the buses, which will
> delay it before it hits its destination.
> 
> PCI particularly buffers MMIO writes, and the requirement there has
> always been that if you need the write to hit the hardware in a timely
> fashion, you must perform a read-back to force the bus to deliver the
> write (since a read is not allowed to overlap a write.)
> 
> The solution is never to use barrier() - barrier() is a _compiler_
> barrier and does nothing for posted writes on hardware buses.

Thanks for clarification. I thought I just need a wmb() to make sure 
writel()
can not be reordered with another store operation. I wasn't aware that 
writel()
is defined to guarantee this on every arch.

That having the correct execution order is not enough on some buses 
because
of buffering is really something to be aware of, thanks again for 
pointing
this out.

So for the scenario I was concerned about I would expect the irqchip 
driver
guarantees the write actually hits the the hardware (if necessary read 
it
back) before the function (disable_irq_nosync()) returns, is that 
correct?
Though, having the need should be very unlikely.

[PATCH] serio: PS2 gpio bit banging driver for the serio bus

[Russell King - ARM Linux]

On Thu, Aug 17, 2017 at 12:51:33PM +0200, Danilo Krummrich wrote:
> That having the correct execution order is not enough on some buses because
> of buffering is really something to be aware of, thanks again for pointing
> this out.

PCI guarantees the order of writes to a device, but there are situations
on SoCs where you can't rely on that - for instance, if the writes go
over different buses to different devices (eg, write to a peripheral
vs write to an interrupt controller.)

Even then, with interrupts delivered by message (eg, MSI) there's
issues.

> So for the scenario I was concerned about I would expect the irqchip driver
> guarantees the write actually hits the the hardware (if necessary read it
> back) before the function (disable_irq_nosync()) returns, is that correct?
> Though, having the need should be very unlikely.

Well, disable_irq_nosync() doesn't guarantee that the interrupt handler
isn't running - a CPU may have just received the interrupt and is just
entering the interrupt handler when disable_irq_nosync() returns.  The
hint is the "nosync" - there's no synchronisation.  If you need to
guarantee that the interrupt handler is not running, disable_irq() does
that.  By implication, however, disable_irq() can not be called from
within the same interrupt handler for the interrupt that is being
disabled.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

[PATCH] serio: PS2 gpio bit banging driver for the serio bus

[Danilo Krummrich]

On 2017-08-17 15:01, Russell King - ARM Linux wrote:
> On Thu, Aug 17, 2017 at 12:51:33PM +0200, Danilo Krummrich wrote:
>> That having the correct execution order is not enough on some buses 
>> because
>> of buffering is really something to be aware of, thanks again for 
>> pointing
>> this out.
> 
> PCI guarantees the order of writes to a device, but there are 
> situations
> on SoCs where you can't rely on that - for instance, if the writes go
> over different buses to different devices (eg, write to a peripheral
> vs write to an interrupt controller.)
> 
> Even then, with interrupts delivered by message (eg, MSI) there's
> issues.
> 
>> So for the scenario I was concerned about I would expect the irqchip 
>> driver
>> guarantees the write actually hits the the hardware (if necessary read 
>> it
>> back) before the function (disable_irq_nosync()) returns, is that 
>> correct?
>> Though, having the need should be very unlikely.
> 
> Well, disable_irq_nosync() doesn't guarantee that the interrupt handler
> isn't running - a CPU may have just received the interrupt and is just
> entering the interrupt handler when disable_irq_nosync() returns.  The
> hint is the "nosync" - there's no synchronisation.  If you need to
> guarantee that the interrupt handler is not running, disable_irq() does
> that.  By implication, however, disable_irq() can not be called from
> within the same interrupt handler for the interrupt that is being
> disabled.
> 
Thanks again, I'm aware of that. As in my case the code could be called 
from
atomic context disable_irq() is not an option.

My main point is if it can be assumed that after disable_irq_nosync() 
returns
it is guaranteed, by convention, that the hardware was hit. But I really 
would
think so.

[PATCH] serio: PS2 gpio bit banging driver for the serio bus

[Linus Walleij]

On Thu, Aug 17, 2017 at 11:09 AM, Russell King - ARM Linux
<linux@armlinux.org.uk> wrote:
> On Fri, Aug 11, 2017 at 11:16:20AM +0200, Linus Walleij wrote:
>> 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.
>
> Incorrect.  writel() has a barrier which ensures that data written to
> memory (eg, dma coherent memory) is visible to the hardware prior to
> the write hitting the hardware.
>
> There is no barrier to ensure that the write hits the hardware in a
> timely manner - the write can be buffered by the buses, which will
> delay it before it hits its destination.
>
> PCI particularly buffers MMIO writes, and the requirement there has
> always been that if you need the write to hit the hardware in a timely
> fashion, you must perform a read-back to force the bus to deliver the
> write (since a read is not allowed to overlap a write.)
>
> The solution is never to use barrier() - barrier() is a _compiler_
> barrier and does nothing for posted writes on hardware buses.

Thanks Russell. I tend to forget things over time even though I have
some memory of this being spelled out to me in the past :(

Yours,
Linus Walleij