[Bug] VCHIQ functional test broken

[Russell King - ARM Linux]

On Mon, Apr 24, 2017 at 06:12:09PM +0200, Stefan Wahren wrote:
> Am 20.04.2017 um 21:58 schrieb Rabin Vincent:
> > On Thu, Apr 20, 2017 at 11:27:38AM -0700, Eric Anholt wrote:
> >> I'm confused by what you're saying here.  The driver has already been
> >> converted to not use dmac_map_area (commit
> >> cf9caf1929882b66922aee698e99e6c8f357bee5), and uses dma_map_sg instead,
> >> matching the radeon driver you give as a model as far as I can see.
> >> That commit is in v4.11-rc6 from Stefan's regression report.
> > Right.  Sorry.  I must have had an old tag checked out when I looked at
> > the driver earlier.  The DMA API usage in the driver in v4.11-rc6 and
> > current master looks fine, except for one thing:
> >
> > The flush in flush_dcache_page() (from get_user_pages()) was done with a
> > v6_flush_kern_dcache_page() which always did a clean+invalidate while
> > the DMA API only does what is required by the direction, which is only a
> > invalidate for DMA_FROM_DEVICE.  Since the driver calls dma_from_sg() on
> > the entire page, even if userspace sent in an offset into the page,
> > unrelated data in userspace may be thrown away.
> >
> > Does changing the dma API calls to always use DMA_BIDIRECTIONAL make the
> > test pass?
> 
> Unfortunately not (at least not that simple).
> 
> Do we need special DMA mapping attributes here ? Or a dma_sync_sg_for_* ?

I had a look at the driver when you first reported the problem, but I
don't see an obvious problem.

In drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c, I
see it using get_user_pages(), generating a scatterlist, which it then
uses dma_map_sg() with.  It then goes on to populate the DMA coherent
buffer that it allocated with the DMA addresses of these buffers.

The tear-down looks sane too - free_pagelist() looks like it correctly
unmaps the scatterlist, marks the pages dirty if necessary, puts the
pages and frees the DMA coherent memory.

Since you're running on a PIPT cache, the only possible issue is whether
there's a lifetime mismatch between what happens here, and what you're
doing with the pages in userspace.  All the rules wrt the DMA API apply
to these userspace pages, just as these same rules apply in kernel space.
Once you have called dma_map_sg() on them, any CPU access (whether
explicit or speculative) will cause cache lines to be populated, and
possibly marked dirty, which can have the effect of corrupting the data
unless it is unmapped prior to the accesses you care about.

What I can't see is how changing flush_dcache_page() has possibly broken
this: it seems to imply that you're getting flush_dcache_page() somehow
called inbetween mapping it for DMA, using it for DMA and CPU accesses
occuring.  However, the driver doesn't call flush_dcache_page() other
than through get_user_pages() - and since dma_map_sg() is used in that
path, it should be fine.

So, I don't have much to offer.

However, flush_dcache_page() is probably still a tad heavy - it currently
flushes to the point of coherency, but it's really about making sure that
the user vs kernel mappings are coherent, not about device coherency.
That's the role of the DMA API.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.