Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

staging: vc04_services: Fix bulk cache maintenance #1987

Merged
merged 1 commit into from May 3, 2017

Conversation

pelwell
Copy link
Contributor

@pelwell pelwell commented May 3, 2017

vchiq_arm supports transfers less than one page and at arbitrary
alignment, using the dma-mapping API to perform its cache maintenance
(even though the VPU drives the DMA hardware). Read (DMA_FROM_DEVICE)
operations use cache invalidation for speed, falling back to
clean+invalidate on partial cache lines, with writes (DMA_TO_DEVICE)
using flushes.

If a read transfer has ends which aren't page-aligned, performing cache
maintenance as if they were whole pages can lead to memory corruption
since the partial cache lines at the ends (and any cache lines before or
after the transfer area) will be invalidated. This bug was masked until
the disabling of the cache flush in flush_dcache_page().

Honouring the requested transfer start- and end-points prevents the
corruption.

See: #1977

Signed-off-by: Phil Elwell phil@raspberrypi.org

vchiq_arm supports transfers less than one page and at arbitrary
alignment, using the dma-mapping API to perform its cache maintenance
(even though the VPU drives the DMA hardware). Read (DMA_FROM_DEVICE)
operations use cache invalidation for speed, falling back to
clean+invalidate on partial cache lines, with writes (DMA_TO_DEVICE)
using flushes.

If a read transfer has ends which aren't page-aligned, performing cache
maintenance as if they were whole pages can lead to memory corruption
since the partial cache lines at the ends (and any cache lines before or
after the transfer area) will be invalidated. This bug was masked until
the disabling of the cache flush in flush_dcache_page().

Honouring the requested transfer start- and end-points prevents the
corruption.

See: raspberrypi#1977

Signed-off-by: Phil Elwell <phil@raspberrypi.org>
@pelwell
Copy link
Contributor Author

pelwell commented May 3, 2017

Tested downstream builds on B+, Pi2 and Pi3.

@popcornmix
Copy link
Collaborator

Looks good. vchiq_test is happy for me and kodi still runs.

@popcornmix popcornmix merged commit d8479ab into raspberrypi:rpi-4.11.y May 3, 2017
@ED6E0F17
Copy link

ED6E0F17 commented May 4, 2017

My bulk test is not happy. "vchiq_test -b 16 1"

[ 1417.084275] WARNING: CPU: 0 PID: 267 at /hardware/bsp/android/linux/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
:537 vchiq_prepare_bulk_data+0x378/0x6fc
[ 1417.102780] Modules linked in:
[ 1417.111382] CPU: 0 PID: 267 Comm: VCHIQ completio Tainted: G        W       4.11.0-armv6+ #65
[ 1417.123517] Hardware name: BCM2835
[ 1417.127988] [<c010f0cc>] (unwind_backtrace) from [<c010c764>] (show_stack+0x20/0x24)
[ 1417.134794] [<c010c764>] (show_stack) from [<c04c2ad4>] (dump_stack+0x20/0x28)
[ 1417.138377] [<c04c2ad4>] (dump_stack) from [<c011cbfc>] (__warn+0xe4/0x110)
[ 1417.141784] [<c011cbfc>] (__warn) from [<c011ccf4>] (warn_slowpath_null+0x30/0x38)
[ 1417.148316] [<c011ccf4>] (warn_slowpath_null) from [<c07b0cb8>] (vchiq_prepare_bulk_data+0x378/0x6fc)
[ 1417.154862] [<c07b0cb8>] (vchiq_prepare_bulk_data) from [<c07a8998>] (vchiq_bulk_transfer+0x2d8/0x554)
[ 1417.161498] [<c07a8998>] (vchiq_bulk_transfer) from [<c07ad73c>] (vchiq_ioctl+0x4c4/0x1b70)
[ 1417.168227] [<c07ad73c>] (vchiq_ioctl) from [<c0295e70>] (do_vfs_ioctl+0x9c/0x820)
[ 1417.175146] [<c0295e70>] (do_vfs_ioctl) from [<c0296670>] (SyS_ioctl+0x7c/0x8c)
[ 1417.178806] [<c0296670>] (SyS_ioctl) from [<c0108200>] (ret_fast_syscall+0x0/0x1c)
[ 1417.185802] ---[ end trace 1166ab1eee92f6f3 ]---

@pelwell
Copy link
Contributor Author

pelwell commented May 4, 2017

I can't reproduce that. Can you replace the WARN_ON with a WARN that logs k, dma_buffers and len? For bonus marks, dump out the whole scatterlist.

@pelwell
Copy link
Contributor Author

pelwell commented May 4, 2017

Never mind - I've got one.

pelwell pushed a commit to pelwell/linux that referenced this pull request May 4, 2017
create_pagelist uses a number of WARNs to sanity-check check the input
user pages, verifying that only the first and last entries in the
array are not integral pages. These WARNs use the output array index
when it should be input array index. Although the difference is only
significant in one instance, change all the indices for consistency.

See: raspberrypi#1987

Signed-off-by: Phil Elwell <phil@raspberrypi.org>
@pelwell
Copy link
Contributor Author

pelwell commented May 4, 2017

Found it - see #1990.

popcornmix pushed a commit that referenced this pull request May 4, 2017
create_pagelist uses a number of WARNs to sanity-check check the input
user pages, verifying that only the first and last entries in the
array are not integral pages. These WARNs use the output array index
when it should be input array index. Although the difference is only
significant in one instance, change all the indices for consistency.

See: #1987

Signed-off-by: Phil Elwell <phil@raspberrypi.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants