Re: [PATCH 01/20] asm-generic/mmiowb: Add generic implementation of mmiowb() tracking

[Linus Torvalds]

On Wed, Mar 6, 2019 at 4:48 PM Michael Ellerman <mpe@xxxxxxxxxxxxxx> wrote:
>
> It's a bit hard to grep for, but I did find one case:
>
> static void netxen_nic_io_write_128M(struct netxen_adapter *adapter,
>                 void __iomem *addr, u32 data)
> {
>         read_lock(&adapter->ahw.crb_lock);
>         writel(data, addr);
>         read_unlock(&adapter->ahw.crb_lock);
> }
>
> It looks like that driver is using the rwlock to exclude cases that can
> just do a readl()/writel() (readers) vs another case that has to reconfigure a
> window or something, before doing readl()/writel() and then configuring
> the window back. So that seems like a valid use for a rwlock.

Oh, it's actually fairly sane: the IO itself is apparently windowed on
that hardware, and the *common* case is that you'd access "window 1".

So if everybody accesses window 1, they can all work in parallel - the
read case.

But if somebody needs to access any of the other special IO windows,
they need to take the write lock, then change the window pointer to
the window they want to access, do the access, and then set it back to
the default "window 1".

So yes. That driver very much relies on exclusion of the IO through an rwlock.

I'm guessing nobody uses that hardware on Power? Or maybe the "window
1 is common" is *so* common that the other cases basically never
happen and don't really end up ever causing problems?

[ Time passes, I look at it ]

Actually, the driver probably works on Power, because *setting* the
window isn't just a write to the window register, it's always
serialized by a read _from_ the window register to verify that the
write "took". Apparently the hardware itself really needs that "don't
do accesses to the window before I've settled".

And that would basically serialize the only operation that really
needs serialization, so in the case of _that_ driver, it all looks
safe. Even if it's partly by luck.

> > Perhaps more importantly, what about sleeping locks? When they
> > actually *block*, they get the barrier thanks to the scheduler, but
> > you can have a nice non-contended sequence that never does that.
>
> Yeah.
>
> The mutex unlock fast path is just:

Yup. Both lock/unlock have fast paths that should be just trivial
atomic sequences.

But the good news is that *usually* device IO is protected by a
spinlock, since you almost always have interrupt serialization needs
too whenever you have any sequence of MMIO that isn't just some "write
single word to start the engine".

So the "use mutex to serialize IO" may be fairly unusual.

                  Linus