Re: [PATCH v2 17/21] drivers: Remove explicit invocations of mmiowb()

[Nicholas Piggin <npiggin@gmail.com>]

Will Deacon's on April 9, 2019 11:46 pm:
> Hi Nick,
> 
> On Tue, Apr 09, 2019 at 07:00:52PM +1000, Nicholas Piggin wrote:
>> Linus Torvalds's on April 6, 2019 1:50 am:
>> > On Fri, Apr 5, 2019 at 4:01 AM Will Deacon <will.deacon@arm.com> wrote:
>> >>
>> >> mmiowb() is now implied by spin_unlock() on architectures that require
>> >> it, so there is no reason to call it from driver code. This patch was
>> >> generated using coccinelle:
>> >>
>> >>         @mmiowb@
>> >>         @@
>> >>         - mmiowb();
>> > 
>> > So I love the patch series, and think we should just do it, but I do
>> > wonder if some of the drivers involved end up relying on memory
>> > ordering things (store_release -> load_aquire) and IO ordering rather
>> > than using locking...
>> 
>> Hopefully the convention that smp_ prefix does not work for MMIO
>> ordering helps there. Drivers relying on that would be broken today
>> on powerpc, at least.
>> 
>> > Wouldn't such use now be broken on ia64 SN platforms? Do we care?
>> 
>> Hopefully not too much, what changed since last thread? :)
>> 
>> > So it might be worth noting that a lot of the mmiowb()s here weren't
>> > paired with spin_unlock?
>> 
>> I repeat myself, but the correct change is for ia64 to #define wmb to
>> mmiowb, then nothing is silently broken, nothing has to be noted, and 
>> nobody has to care. The ia64/sn2 platform will run a little slower 
>> that's all.
> 
> That's certainly something for the ia64 maintainers to consider, if they
> care about this behaviour. I still have hope that we'll drop ia64 in the
> near future :)

Well we don't need to for this reason, at least. Wouldn't cost
architecture independent code anything.

I don't have much opinion about it, but Itaniums of course are still
being sold and the latest chip released in 2017. The last Itanium
Altix seems more than 10 years old though so it might be reasonable 
to remove sn2 (if it's causing a big headache).

> 
>> But deliberately breaking sn2 I guess is implicitly acknowledging the 
>> same end result that I wanted, so fine.
>> 
>> I think it might be an idea to remove all the mmiowb() that obviously
>> come before spin_unlock in one big patch, but then submit the rest 
>> individually to driver maintainers. I could do that rather than ask
>> more work from Will, if he and you agree.
> 
> That's an option, I suppose, but I'd much rather just kill off mmiowb() in
> one fell swoop and be done with it. I've added the following message to
> the commit of the coccinelle patch so any breakage should be easily
> rectified:
> 
>  | NOTE: mmiowb() has only ever guaranteed ordering in conjunction with
>  | spin_unlock(). However, pairing each mmiowb() removal in this patch
>  | with the corresponding call to spin_unlock() is not at all trivial,
>  | so there is a small chance that this change may regress any drivers
>  | incorrectly relying on mmiowb() to order MMIO writes between CPUs using
>  | lock-free synchronisation. If you've ended up bisecting to this commit,
>  | you can reintroduce the mmiowb() calls using wmb() instead, which should
>  | restore the old behaviour on all architectures other than some esoteric
>  | ia64 systems.
> 
> That way we don't have to worry about the long tail of commits removing
> undocumented, dangling barriers.
> 
> It's not like we're losing the information about where the mmiowb()s used to
> be, so it should be easy to address any fallout (but I'm not really expecting
> anything significant, to be honest with you).

Well if you feel strongly about it I don't object.

Thanks,
Nick