[Bug] VCHIQ functional test broken

[Stefan Wahren]

Am 15.05.2017 um 16:29 schrieb Phil Elwell:
> On 13/05/2017 10:30, Russell King - ARM Linux wrote:
>> On Sat, May 13, 2017 at 11:07:28AM +0200, Stefan Wahren wrote:
>>> In the meantime this issue has been fixed by Phil [1].
>> Right - definitely a driver bug.  Mapping more memory for DMA than is
>> actually going to be DMA'd to and expecting data to be preserved is
>> really horrid.
> That feature was added during the upstreaming process, and as Stefan says
> there is an outstanding patch for it.
>
>>> Unfortunately i found another issue. If i enable CONFIG_HIGHMEM in
>>> the kernel config, the data during functional test gets corrupted.
>>> Phil said it's caused by the usage of get_user_pages() [2].
>> Without knowing who "Phil" is in that thread, but...
>>
>>    HIGHMEM is a problem because you can't use get_user_pages on pages in
>>    HIGHMEM.
>>
>> is an interesting statement, and without any reasoning or evidence.
>>
>> I also believe it to be incorrect.  get_user_pages() returns an array
>> of struct page pointers for the user memory, calling flush_dcache_page()
>> and flush_anon_page() on them to ensure that any kernel mapping is
>> coherent with what is in userspace.
>>
>> As far as returning the array of page pointers, get_user_pages() doesn't
>> care whether they're lowmem or highmem.
>>
>> flush_dcache_page() doesn't care either - if it wants to flush the page
>> and the page is a highmem page, it will temporarily map it before
>> flushing it.
>>
>> flush_anon_page() is a no-op for all non-aliasing caches.
>>
>> get_user_pages() works fine for whatever memory on other platforms and
>> drivers such as etnaviv, so I think this comes down to the vchip driver
>> doing things in ways that the kernel interfaces its using don't expect -
>> exactly like the "lets pass full pages to the DMA API" broken-ness.
> See previous comment.
>
>> I would like to hear the justification for that statement, but without
>> any justification, I assert that the statement is false.
> I am the Phil in question, and the off-the-cuff comment was the result of
> a hazy memory of issues encountered with VCHIQ bulk transfers as a Broadcom
> employee (which would have been on a 2.6 kernel). I suspect there may have
> been some use of kernel virtual addresses as an intermediate representation,
> but I no longer have access to that code.
>
> If get_user_pages is HIGHMEM-safe (and I can see why it would be), then the
> cause of the corruption Stefan saw is probably the special handling of
> unaligned reads, specifically:
>
> 			memcpy((char *)page_address(pages[0]) +
> 				pagelist->offset,
> 				fragments,
> 				head_bytes);


Btw shouldn't we use copy_from_user() at this place?