Re: [PATCH] drm/vmwgfx: switch from ioremap_cache to memremap

[Thomas Hellstrom <thellstrom@vmware.com>]

On 10/13/2015 06:35 PM, Dan Williams wrote:
> On Mon, Oct 12, 2015 at 10:18 PM, Thomas Hellstrom
> <thellstrom@vmware.com> wrote:
>> Hi!
>>
>> On 10/13/2015 12:35 AM, Dan Williams wrote:
>>> Per commit 2e586a7e017a "drm/vmwgfx: Map the fifo as cached" the driver
>>> expects the fifo registers to be cacheable.  In preparation for
>>> deprecating ioremap_cache() convert its usage in vmwgfx to memremap().
>>>
>>> Cc: David Airlie <airlied@linux.ie>
>>> Cc: Thomas Hellstrom <thellstrom@vmware.com>
>>> Cc: Sinclair Yeh <syeh@vmware.com>
>>> Cc: dri-devel@lists.freedesktop.org
>>> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>> While I have nothing against the conversion, what's stopping the
>> compiler from reordering writes on a generic architecture and caching
>> and reordering reads on x86 in particular? At the very least it looks to
>> me like the memory accesses of the memremap'd memory needs to be
>> encapsulated within READ_ONCE and WRITE_ONCE.
> Hmm, currently the code is using ioread32/iowrite32 which only do
> volatile accesses, whereas READ_ONCE / WRITE_ONCE have a memory
> clobber on entry and exit.  So, I'm assuming all you need is the
> guarantee of "no compiler re-ordering" and not the stronger
> READ_ONCE/WRITE_ONCE guarantees, but that still seems broken compared
> to explicit fencing where it matters.

I'm not quite sure I follow you here, it looks to me like READ_ONCE()
and WRITE_ONCE() are implemented as
volatile accesses,

http://lxr.free-electrons.com/source/include/linux/compiler.h#L215

just like ioread32 and iowrite32

http://lxr.free-electrons.com/source/include/asm-generic/io.h#L54

which would minimize any potential impact of this change.
IMO optimizing the memory accesses can be done as a later step.

/Thomas