[PATCH v2 2/3] staging: vchiq: Fix bulk transfers on 64-bit builds
The recent change to the bulk transfer compat function missed the fact
the relevant ioctl command is VCHIQ_IOC_QUEUE_BULK_TRANSMIT32, not
VCHIQ_IOC_QUEUE_BULK_TRANSMIT, as any attempt to send a bulk block
to the VPU would have shown.
Fixes: a4367cd2b231 ("staging: vchiq: convert compat bulk transfer")
Signed-off-by: Phil Elwell
---
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index 2a8883673ba1..2ca5805b2fce 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -1717,7 +1717,7 @@ vchiq_compat_ioctl_queue_bulk(struct file *file,
{
struct vchiq_queue_bulk_transfer32 args32;
struct vchiq_queue_bulk_transfer args;
- enum vchiq_bulk_dir dir = (cmd == VCHIQ_IOC_QUEUE_BULK_TRANSMIT) ?
+ enum vchiq_bulk_dir dir = (cmd == VCHIQ_IOC_QUEUE_BULK_TRANSMIT32) ?
VCHIQ_BULK_TRANSMIT : VCHIQ_BULK_RECEIVE;
if (copy_from_user(, argp, sizeof(args32)))
--
2.25.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 3/3] staging: vc04_services: Add a note to the TODO
Record in the TODO file that the address of ">bulk_waiter" should never be returned to userspace. Signed-off-by: Phil Elwell --- drivers/staging/vc04_services/interface/TODO | 4 1 file changed, 4 insertions(+) diff --git a/drivers/staging/vc04_services/interface/TODO b/drivers/staging/vc04_services/interface/TODO index fc2752bc95b2..0bcb8f158afc 100644 --- a/drivers/staging/vc04_services/interface/TODO +++ b/drivers/staging/vc04_services/interface/TODO @@ -91,3 +91,7 @@ The first thing one generally sees in a probe function is a memory allocation for all the device specific data. This structure is then passed all over the driver. This is good practice since it makes the driver work regardless of the number of devices probed. + +14) Clean up Sparse warnings from __user annotations. See +vchiq_irq_queue_bulk_tx_rx(). Ensure that the address of ">bulk_waiter" +is never disclosed to userspace. -- 2.25.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 1/3] staging: vchiq: Fix bulk userdata handling
The addition of the local 'userdata' pointer to
vchiq_irq_queue_bulk_tx_rx omitted the case where neither BLOCKING nor
WAITING modes are used, in which case the value provided by the
caller is not returned to them as expected, but instead it is replaced
with a NULL. This lack of a suitable context may cause the application
to crash or otherwise malfunction.
Fixes: 4184da4f316a ("staging: vchiq: fix __user annotations")
Signed-off-by: Phil Elwell
Tested-by: Stefan Wahren
---
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index f500a7043805..2a8883673ba1 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -958,7 +958,7 @@ static int vchiq_irq_queue_bulk_tx_rx(struct vchiq_instance
*instance,
struct vchiq_service *service;
struct bulk_waiter_node *waiter = NULL;
bool found = false;
- void *userdata = NULL;
+ void *userdata;
int status = 0;
int ret;
@@ -997,6 +997,8 @@ static int vchiq_irq_queue_bulk_tx_rx(struct vchiq_instance
*instance,
"found bulk_waiter %pK for pid %d", waiter,
current->pid);
userdata = >bulk_waiter;
+ } else {
+ userdata = args->userdata;
}
/*
--
2.25.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 0/3] A trio of vchiq bulk transfer fixes
The recent batch of vchiq improvements broke bulk transfers in two ways: 1. The userdata associated with a transfer was lost in the case that a non-blocking mode was used. 2. The 64-bit ioctl compatibility shim for a bulk transfer used the wrong ioctl command. This patch set fixes both of those bugs, and adds a security-related note to the TODO file. Changes in v2: - Expand the commit message on patch 1 to clarify the impact of the bug, and add Tested-by. - Add commit 3 with an additional TODO item. - Change the name of the patch set to be numerically accurate. Phil Elwell (3): staging: vchiq: Fix bulk userdata handling staging: vchiq: Fix bulk transfers on 64-bit builds staging: vc04_services: Add a note to the TODO drivers/staging/vc04_services/interface/TODO| 4 .../staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 6 -- 2 files changed, 8 insertions(+), 2 deletions(-) -- 2.25.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/2] staging: vchiq: Fix bulk userdata handling
On Tue, 5 Jan 2021 at 11:04, Dan Carpenter wrote:
>
> On Mon, Jan 04, 2021 at 07:26:42PM +, Phil Elwell wrote:
> > On 04/01/2021 18:31, Dan Carpenter wrote:
> > > On Mon, Jan 04, 2021 at 12:09:27PM +, Phil Elwell wrote:
> > > > The addition of the local 'userdata' pointer to
> > > > vchiq_irq_queue_bulk_tx_rx omitted the case where neither BLOCKING nor
> > > > WAITING modes are used, in which case the value provided by the
> > > > caller is replaced with a NULL.
> > > >
> > > > Fixes: 4184da4f316a ("staging: vchiq: fix __user annotations")
> > > >
> > > > Signed-off-by: Phil Elwell
> > > > ---
> > > > drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 4 +++-
> > > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git
> > > > a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > > > b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > > > index f500a7043805..2a8883673ba1 100644
> > > > --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > > > +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > > > @@ -958,7 +958,7 @@ static int vchiq_irq_queue_bulk_tx_rx(struct
> > > > vchiq_instance *instance,
> > > > struct vchiq_service *service;
> > > > struct bulk_waiter_node *waiter = NULL;
> > > > bool found = false;
> > > > - void *userdata = NULL;
> > > > + void *userdata;
> > > > int status = 0;
> > > > int ret;
> > > > @@ -997,6 +997,8 @@ static int vchiq_irq_queue_bulk_tx_rx(struct
> > > > vchiq_instance *instance,
> > > > "found bulk_waiter %pK for pid %d", waiter,
> > > > current->pid);
> > > > userdata = >bulk_waiter;
> > > > + } else {
> > > > + userdata = args->userdata;
> > >
> > > "args->userdata" is marked as a user pointer so we really don't want to
> > > mix user and kernel pointers here. Presumably this opens up a large
> > > security hole.
> >
> > It's an opaque, pointer-sized token that only exists to bereturned to
> > userspace (or not,
> > without this patch) - it's hard to see that as a security hole.
>
> I was assuming the bug here was a NULL dereference... Apparently that's
> not the case? The commit message needs to be updated to be more clear
> about how the bug looks like to the user.
>
> Are we using the ">bulk_waiter" as a "token to be returned to
> userspace" as well? It looks like maybe it is in vchiq_put_completion().
> That defeats KASLR and is a different sort of security problem.
>
> Mixing __user pointers and regular pointers is dangerous and has lead to
> security problems in this driver in the past. But also mixing mixing
> tokens with pointers just makes the code hard to read. Instead of
> undoing Arnd's work where he split the user space and kernel pointers
> apart we should go ahead and spit it up even more. At least add a giant
> FIXME comment and an item in the TODO list so we don't forget to do this
> before removing the code from staging.
Those all sound like valid comments to have made against the original
patch, but that seems to have received little attention.
I'll just leave this here - perhaps Arnd has the patience to finish the job.
Phil
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/2] staging: vchiq: Fix bulk userdata handling
On 04/01/2021 18:31, Dan Carpenter wrote:
On Mon, Jan 04, 2021 at 12:09:27PM +, Phil Elwell wrote:
The addition of the local 'userdata' pointer to
vchiq_irq_queue_bulk_tx_rx omitted the case where neither BLOCKING nor
WAITING modes are used, in which case the value provided by the
caller is replaced with a NULL.
Fixes: 4184da4f316a ("staging: vchiq: fix __user annotations")
Signed-off-by: Phil Elwell
---
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index f500a7043805..2a8883673ba1 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -958,7 +958,7 @@ static int vchiq_irq_queue_bulk_tx_rx(struct vchiq_instance
*instance,
struct vchiq_service *service;
struct bulk_waiter_node *waiter = NULL;
bool found = false;
- void *userdata = NULL;
+ void *userdata;
int status = 0;
int ret;
@@ -997,6 +997,8 @@ static int vchiq_irq_queue_bulk_tx_rx(struct vchiq_instance *instance,
"found bulk_waiter %pK for pid %d", waiter,
current->pid);
userdata = >bulk_waiter;
+ } else {
+ userdata = args->userdata;
"args->userdata" is marked as a user pointer so we really don't want to
mix user and kernel pointers here. Presumably this opens up a large
security hole.
It's an opaque, pointer-sized token that only exists to bereturned to userspace
(or not,
without this patch) - it's hard to see that as a security hole.
Phil
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 2/2] staging: vchiq: Fix bulk transfers on 64-bit builds
The recent change to the bulk transfer compat function missed the fact
the relevant ioctl command is VCHIQ_IOC_QUEUE_BULK_TRANSMIT32, not
VCHIQ_IOC_QUEUE_BULK_TRANSMIT, as any attempt to send a bulk block
to the VPU would have shown.
Fixes: a4367cd2b231 ("staging: vchiq: convert compat bulk transfer")
Signed-off-by: Phil Elwell
---
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index 2a8883673ba1..2ca5805b2fce 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -1717,7 +1717,7 @@ vchiq_compat_ioctl_queue_bulk(struct file *file,
{
struct vchiq_queue_bulk_transfer32 args32;
struct vchiq_queue_bulk_transfer args;
- enum vchiq_bulk_dir dir = (cmd == VCHIQ_IOC_QUEUE_BULK_TRANSMIT) ?
+ enum vchiq_bulk_dir dir = (cmd == VCHIQ_IOC_QUEUE_BULK_TRANSMIT32) ?
VCHIQ_BULK_TRANSMIT : VCHIQ_BULK_RECEIVE;
if (copy_from_user(, argp, sizeof(args32)))
--
2.25.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 0/2] A brace of vchiq bulk transfer fixes
The recent batch of vchiq improvements broke bulk transfers in two ways: 1. The userdata associated with a transfer was lost in the case that a non-blocking mode was used. 2. The 64-bit ioctl compatibility shim for a bulk transfer used the wrong ioctl command. This patch set fixes both of those bugs. Phil Elwell (2): staging: vchiq: Fix bulk userdata handling staging: vchiq: Fix bulk transfers on 64-bit builds .../staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) -- 2.25.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 1/2] staging: vchiq: Fix bulk userdata handling
The addition of the local 'userdata' pointer to
vchiq_irq_queue_bulk_tx_rx omitted the case where neither BLOCKING nor
WAITING modes are used, in which case the value provided by the
caller is replaced with a NULL.
Fixes: 4184da4f316a ("staging: vchiq: fix __user annotations")
Signed-off-by: Phil Elwell
---
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index f500a7043805..2a8883673ba1 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -958,7 +958,7 @@ static int vchiq_irq_queue_bulk_tx_rx(struct vchiq_instance
*instance,
struct vchiq_service *service;
struct bulk_waiter_node *waiter = NULL;
bool found = false;
- void *userdata = NULL;
+ void *userdata;
int status = 0;
int ret;
@@ -997,6 +997,8 @@ static int vchiq_irq_queue_bulk_tx_rx(struct vchiq_instance
*instance,
"found bulk_waiter %pK for pid %d", waiter,
current->pid);
userdata = >bulk_waiter;
+ } else {
+ userdata = args->userdata;
}
/*
--
2.25.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: vchiq: Fix local event signalling
Prior to the recent event reworking (see Fixes), thread synchronisation
was implemented using completions, the worker thread being woken with
a call to complete(). The replacement uses waitqueues, which are more
like condition variables in that the waiting thread is only woken if
the condition is true.
When the VPU signals the ARM, it first sets the event's fired flag to
indicate which event is being signalled, but the places in the
ARM-side code where the worker thread is being woken -
remote_event_signal_local via request_poll - did not do so as it
wasn't previously necessary, and since the armed flag was being
cleared this lead to a deadlock.
Fixes: 852b2876a8a8 ("staging: vchiq: rework remove_event handling")
Signed-off-by: Phil Elwell
---
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
index 9e17ec6..53f5a1c 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -446,6 +446,7 @@ remote_event_wait(wait_queue_head_t *wq, struct
remote_event *event)
static inline void
remote_event_signal_local(wait_queue_head_t *wq, struct remote_event *event)
{
+ event->fired = 1;
event->armed = 0;
wake_up_all(wq);
}
--
2.7.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 0/4] Improve VCHIQ cache line size handling
On 17/09/2018 18:51, Florian Fainelli wrote: On 09/17/2018 04:47 AM, Phil Elwell wrote: Hi Stefan, On 17/09/2018 12:39, Stefan Wahren wrote: Hi Phil, Am 17.09.2018 um 10:22 schrieb Phil Elwell: Both sides of the VCHIQ communications mechanism need to agree on the cache line size. Using an incorrect value can lead to data corruption, but having the two sides using different values is usually worse. In the absence of an obvious convenient run-time method to determine the correct value in the ARCH=arm world, the downstream Raspberry Pi trees used a Device Tree property, written by the firmware, to configure the kernel driver. This method was vetoed during the upstreaming process, so a fixed value of 32 was used instead, and some corruptions ensued. This is take 2 at arriving at the correct value. Add a new compatible string - "brcm,bcm2836-vchiq" - to indicate an SoC with a 64-byte cache line. Document the new string in the binding, and use it on the appropriate platforms. The final patch is a (seemingly cosmetic) correction of the Device Tree "reg" declaration for the device node, but it doubles as an indication to the Raspberry Pi firmware that the kernel driver is running a recent kernel driver that chooses the correct value. As such it would help if the DT patches are not merged before the driver patch. v3: Builds without errors, tested on multiple Raspberry Pi models. v2: Replaced ARM-specific logic used to determine cache line size with a new compatible string for BCM2836 and BCM2837. Phil Elwell (4): staging/vc04_services: Use correct cache line size dt-bindings: soc: Document "brcm,bcm2836-vchiq" ARM: dts: bcm283x: Correct vchiq compatible string ARM: dts: bcm283x: Correct mailbox register sizes since my pull requests are out, would it be okay to apply patch #1 for 4.20 and the DT stuff for 4.21 (with the assumption Rob is okay with these patches)? Patch 4 is the only one I'd like to be delayed, but delaying 2-4 is fine with me. Humm, did you mean you would like not to be delayed? In any case Stefan, you can send an additional pull request, and I will merge it and send a second pull request towards ARM SoC maintainers, that's not a problem. No, I meant what I wrote - I would prefer patch 1 to be merged before patch 4 (or at least in the same release) to avoid the need for another firmware change, hence delaying patch 4 is good. It makes sense for the other commits to be merged in that order, but the normal compatible-string fallback mechanism means there is no hard dependency there. Phil ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 0/4] Improve VCHIQ cache line size handling
Hi Stefan, On 17/09/2018 12:39, Stefan Wahren wrote: > Hi Phil, > > Am 17.09.2018 um 10:22 schrieb Phil Elwell: >> Both sides of the VCHIQ communications mechanism need to agree on the cache >> line size. Using an incorrect value can lead to data corruption, but having >> the >> two sides using different values is usually worse. >> >> In the absence of an obvious convenient run-time method to determine the >> correct value in the ARCH=arm world, the downstream Raspberry Pi trees used a >> Device Tree property, written by the firmware, to configure the kernel >> driver. >> This method was vetoed during the upstreaming process, so a fixed value of 32 >> was used instead, and some corruptions ensued. This is take 2 at arriving at >> the correct value. >> >> Add a new compatible string - "brcm,bcm2836-vchiq" - to indicate an SoC with >> a 64-byte cache line. Document the new string in the binding, and use it on >> the appropriate platforms. >> >> The final patch is a (seemingly cosmetic) correction of the Device Tree "reg" >> declaration for the device node, but it doubles as an indication to the >> Raspberry Pi firmware that the kernel driver is running a recent kernel >> driver >> that chooses the correct value. As such it would help if the DT patches are >> not merged before the driver patch. >> >> v3: Builds without errors, tested on multiple Raspberry Pi models. >> v2: Replaced ARM-specific logic used to determine cache line size with >> a new compatible string for BCM2836 and BCM2837. >> >> Phil Elwell (4): >> staging/vc04_services: Use correct cache line size >> dt-bindings: soc: Document "brcm,bcm2836-vchiq" >> ARM: dts: bcm283x: Correct vchiq compatible string >> ARM: dts: bcm283x: Correct mailbox register sizes > > since my pull requests are out, would it be okay to apply patch #1 for > 4.20 and the DT stuff for 4.21 (with the assumption Rob is okay with > these patches)? Patch 4 is the only one I'd like to be delayed, but delaying 2-4 is fine with me. Phil ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v3 2/4] dt-bindings: soc: Document "brcm,bcm2836-vchiq"
"brcm,bcm2836-vchiq" should be used on BCM2836 and BCM2837 to ensure correct operation. Signed-off-by: Phil Elwell Acked-by: Stefan Wahren --- Documentation/devicetree/bindings/soc/bcm/brcm,bcm2835-vchiq.txt | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Documentation/devicetree/bindings/soc/bcm/brcm,bcm2835-vchiq.txt b/Documentation/devicetree/bindings/soc/bcm/brcm,bcm2835-vchiq.txt index 8dd7b3a..f331316 100644 --- a/Documentation/devicetree/bindings/soc/bcm/brcm,bcm2835-vchiq.txt +++ b/Documentation/devicetree/bindings/soc/bcm/brcm,bcm2835-vchiq.txt @@ -2,7 +2,8 @@ Broadcom VCHIQ firmware services Required properties: -- compatible: Should be "brcm,bcm2835-vchiq" +- compatible: Should be "brcm,bcm2835-vchiq" on BCM2835, otherwise + "brcm,bcm2836-vchiq". - reg: Physical base address and length of the doorbell register pair - interrupts: The interrupt number See bindings/interrupt-controller/brcm,bcm2835-armctrl-ic.txt -- 2.7.4 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v3 4/4] ARM: dts: bcm283x: Correct mailbox register sizes
The size field in a Device Tree "reg" property is encoded in bytes, not
words.
Fixes: 614fa22119d6 ("ARM: dts: bcm2835: Add VCHIQ node to the Raspberry Pi
boards. (v3)")
Signed-off-by: Phil Elwell
Acked-by: Stefan Wahren
---
arch/arm/boot/dts/bcm2835-rpi.dtsi | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm/boot/dts/bcm2835-rpi.dtsi
b/arch/arm/boot/dts/bcm2835-rpi.dtsi
index 215d8cc..29f970f 100644
--- a/arch/arm/boot/dts/bcm2835-rpi.dtsi
+++ b/arch/arm/boot/dts/bcm2835-rpi.dtsi
@@ -32,7 +32,7 @@
vchiq: mailbox@7e00b840 {
compatible = "brcm,bcm2835-vchiq";
- reg = <0x7e00b840 0xf>;
+ reg = <0x7e00b840 0x3c>;
interrupts = <0 2>;
};
};
--
2.7.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v3 3/4] ARM: dts: bcm283x: Correct vchiq compatible string
To allow VCHIQ to determine the correct cache line size, use the new
"brcm,bcm2836-vchiq" compatible string on BCM2836 and BCM2837.
Signed-off-by: Phil Elwell
Acked-by: Stefan Wahren
---
arch/arm/boot/dts/bcm2835-rpi.dtsi | 2 +-
arch/arm/boot/dts/bcm2836-rpi-2-b.dts | 2 +-
arch/arm/boot/dts/bcm2836-rpi.dtsi | 6 ++
arch/arm/boot/dts/bcm2837-rpi-3-b-plus.dts | 2 +-
arch/arm/boot/dts/bcm2837-rpi-3-b.dts | 2 +-
arch/arm/boot/dts/bcm2837-rpi-cm3.dtsi | 2 +-
6 files changed, 11 insertions(+), 5 deletions(-)
create mode 100644 arch/arm/boot/dts/bcm2836-rpi.dtsi
diff --git a/arch/arm/boot/dts/bcm2835-rpi.dtsi
b/arch/arm/boot/dts/bcm2835-rpi.dtsi
index cb2d6d7..215d8cc 100644
--- a/arch/arm/boot/dts/bcm2835-rpi.dtsi
+++ b/arch/arm/boot/dts/bcm2835-rpi.dtsi
@@ -30,7 +30,7 @@
#power-domain-cells = <1>;
};
- mailbox@7e00b840 {
+ vchiq: mailbox@7e00b840 {
compatible = "brcm,bcm2835-vchiq";
reg = <0x7e00b840 0xf>;
interrupts = <0 2>;
diff --git a/arch/arm/boot/dts/bcm2836-rpi-2-b.dts
b/arch/arm/boot/dts/bcm2836-rpi-2-b.dts
index 2fef70a..ac4408b 100644
--- a/arch/arm/boot/dts/bcm2836-rpi-2-b.dts
+++ b/arch/arm/boot/dts/bcm2836-rpi-2-b.dts
@@ -1,7 +1,7 @@
// SPDX-License-Identifier: GPL-2.0
/dts-v1/;
#include "bcm2836.dtsi"
-#include "bcm2835-rpi.dtsi"
+#include "bcm2836-rpi.dtsi"
#include "bcm283x-rpi-smsc9514.dtsi"
#include "bcm283x-rpi-usb-host.dtsi"
diff --git a/arch/arm/boot/dts/bcm2836-rpi.dtsi
b/arch/arm/boot/dts/bcm2836-rpi.dtsi
new file mode 100644
index 000..c4c858b
--- /dev/null
+++ b/arch/arm/boot/dts/bcm2836-rpi.dtsi
@@ -0,0 +1,6 @@
+// SPDX-License-Identifier: GPL-2.0
+#include "bcm2835-rpi.dtsi"
+
+ {
+ compatible = "brcm,bcm2836-vchiq", "brcm,bcm2835-vchiq";
+};
diff --git a/arch/arm/boot/dts/bcm2837-rpi-3-b-plus.dts
b/arch/arm/boot/dts/bcm2837-rpi-3-b-plus.dts
index 4adb85e..eca36e3 100644
--- a/arch/arm/boot/dts/bcm2837-rpi-3-b-plus.dts
+++ b/arch/arm/boot/dts/bcm2837-rpi-3-b-plus.dts
@@ -1,7 +1,7 @@
// SPDX-License-Identifier: GPL-2.0
/dts-v1/;
#include "bcm2837.dtsi"
-#include "bcm2835-rpi.dtsi"
+#include "bcm2836-rpi.dtsi"
#include "bcm283x-rpi-lan7515.dtsi"
#include "bcm283x-rpi-usb-host.dtsi"
diff --git a/arch/arm/boot/dts/bcm2837-rpi-3-b.dts
b/arch/arm/boot/dts/bcm2837-rpi-3-b.dts
index c318bcb..a0ba0f6 100644
--- a/arch/arm/boot/dts/bcm2837-rpi-3-b.dts
+++ b/arch/arm/boot/dts/bcm2837-rpi-3-b.dts
@@ -1,7 +1,7 @@
// SPDX-License-Identifier: GPL-2.0
/dts-v1/;
#include "bcm2837.dtsi"
-#include "bcm2835-rpi.dtsi"
+#include "bcm2836-rpi.dtsi"
#include "bcm283x-rpi-smsc9514.dtsi"
#include "bcm283x-rpi-usb-host.dtsi"
diff --git a/arch/arm/boot/dts/bcm2837-rpi-cm3.dtsi
b/arch/arm/boot/dts/bcm2837-rpi-cm3.dtsi
index 7b7ab6a..4a89a18 100644
--- a/arch/arm/boot/dts/bcm2837-rpi-cm3.dtsi
+++ b/arch/arm/boot/dts/bcm2837-rpi-cm3.dtsi
@@ -1,7 +1,7 @@
// SPDX-License-Identifier: GPL-2.0
/dts-v1/;
#include "bcm2837.dtsi"
-#include "bcm2835-rpi.dtsi"
+#include "bcm2836-rpi.dtsi"
/ {
memory {
--
2.7.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v3 1/4] staging/vc04_services: Use correct cache line size
Use the compatible string in the DTB to select the correct cache line
size for the SoC - 32 for BCM2835, and 64 for BCM2836 and BCM2837.
Signed-off-by: Phil Elwell
---
.../interface/vchiq_arm/vchiq_2835_arm.c | 4 ++-
.../vc04_services/interface/vchiq_arm/vchiq_arm.c | 35 +++---
.../vc04_services/interface/vchiq_arm/vchiq_arm.h | 5
3 files changed, 33 insertions(+), 11 deletions(-)
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
index e767209..83d740f 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
@@ -109,7 +109,8 @@ free_pagelist(struct vchiq_pagelist_info *pagelistinfo,
int vchiq_platform_init(struct platform_device *pdev, VCHIQ_STATE_T *state)
{
struct device *dev = >dev;
- struct rpi_firmware *fw = platform_get_drvdata(pdev);
+ struct vchiq_drvdata *drvdata = platform_get_drvdata(pdev);
+ struct rpi_firmware *fw = drvdata->fw;
VCHIQ_SLOT_ZERO_T *vchiq_slot_zero;
struct resource *res;
void *slot_mem;
@@ -127,6 +128,7 @@ int vchiq_platform_init(struct platform_device *pdev,
VCHIQ_STATE_T *state)
if (err < 0)
return err;
+ g_cache_line_size = drvdata->cache_line_size;
g_fragments_size = 2 * g_cache_line_size;
/* Allocate space for the channels in coherent memory */
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index bc05c69..ea78937 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -170,6 +170,14 @@ static struct device *vchiq_dev;
static DEFINE_SPINLOCK(msg_queue_spinlock);
static struct platform_device *bcm2835_camera;
+static struct vchiq_drvdata bcm2835_drvdata = {
+ .cache_line_size = 32,
+};
+
+static struct vchiq_drvdata bcm2836_drvdata = {
+ .cache_line_size = 64,
+};
+
static const char *const ioctl_names[] = {
"CONNECT",
"SHUTDOWN",
@@ -3573,12 +3581,25 @@ void vchiq_platform_conn_state_changed(VCHIQ_STATE_T
*state,
}
}
+static const struct of_device_id vchiq_of_match[] = {
+ { .compatible = "brcm,bcm2835-vchiq", .data = _drvdata },
+ { .compatible = "brcm,bcm2836-vchiq", .data = _drvdata },
+ {},
+};
+MODULE_DEVICE_TABLE(of, vchiq_of_match);
+
static int vchiq_probe(struct platform_device *pdev)
{
struct device_node *fw_node;
- struct rpi_firmware *fw;
+ const struct of_device_id *of_id;
+ struct vchiq_drvdata *drvdata;
int err;
+ of_id = of_match_node(vchiq_of_match, pdev->dev.of_node);
+ drvdata = (struct vchiq_drvdata *)of_id->data;
+ if (!drvdata)
+ return -EINVAL;
+
fw_node = of_find_compatible_node(NULL, NULL,
"raspberrypi,bcm2835-firmware");
if (!fw_node) {
@@ -3586,12 +3607,12 @@ static int vchiq_probe(struct platform_device *pdev)
return -ENOENT;
}
- fw = rpi_firmware_get(fw_node);
+ drvdata->fw = rpi_firmware_get(fw_node);
of_node_put(fw_node);
- if (!fw)
+ if (!drvdata->fw)
return -EPROBE_DEFER;
- platform_set_drvdata(pdev, fw);
+ platform_set_drvdata(pdev, drvdata);
err = vchiq_platform_init(pdev, _state);
if (err != 0)
@@ -3661,12 +3682,6 @@ static int vchiq_remove(struct platform_device *pdev)
return 0;
}
-static const struct of_device_id vchiq_of_match[] = {
- { .compatible = "brcm,bcm2835-vchiq", },
- {},
-};
-MODULE_DEVICE_TABLE(of, vchiq_of_match);
-
static struct platform_driver vchiq_driver = {
.driver = {
.name = "bcm2835_vchiq",
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
index 40bb0c6..2f3ebc9 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
@@ -123,6 +123,11 @@ typedef struct vchiq_arm_state_struct {
} VCHIQ_ARM_STATE_T;
+struct vchiq_drvdata {
+ const unsigned int cache_line_size;
+ struct rpi_firmware *fw;
+};
+
extern int vchiq_arm_log_level;
extern int vchiq_susp_log_level;
--
2.7.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v3 0/4] Improve VCHIQ cache line size handling
Both sides of the VCHIQ communications mechanism need to agree on the cache line size. Using an incorrect value can lead to data corruption, but having the two sides using different values is usually worse. In the absence of an obvious convenient run-time method to determine the correct value in the ARCH=arm world, the downstream Raspberry Pi trees used a Device Tree property, written by the firmware, to configure the kernel driver. This method was vetoed during the upstreaming process, so a fixed value of 32 was used instead, and some corruptions ensued. This is take 2 at arriving at the correct value. Add a new compatible string - "brcm,bcm2836-vchiq" - to indicate an SoC with a 64-byte cache line. Document the new string in the binding, and use it on the appropriate platforms. The final patch is a (seemingly cosmetic) correction of the Device Tree "reg" declaration for the device node, but it doubles as an indication to the Raspberry Pi firmware that the kernel driver is running a recent kernel driver that chooses the correct value. As such it would help if the DT patches are not merged before the driver patch. v3: Builds without errors, tested on multiple Raspberry Pi models. v2: Replaced ARM-specific logic used to determine cache line size with a new compatible string for BCM2836 and BCM2837. Phil Elwell (4): staging/vc04_services: Use correct cache line size dt-bindings: soc: Document "brcm,bcm2836-vchiq" ARM: dts: bcm283x: Correct vchiq compatible string ARM: dts: bcm283x: Correct mailbox register sizes .../bindings/soc/bcm/brcm,bcm2835-vchiq.txt| 3 +- arch/arm/boot/dts/bcm2835-rpi.dtsi | 4 +-- arch/arm/boot/dts/bcm2836-rpi-2-b.dts | 2 +- arch/arm/boot/dts/bcm2836-rpi.dtsi | 6 arch/arm/boot/dts/bcm2837-rpi-3-b-plus.dts | 2 +- arch/arm/boot/dts/bcm2837-rpi-3-b.dts | 2 +- arch/arm/boot/dts/bcm2837-rpi-cm3.dtsi | 2 +- .../interface/vchiq_arm/vchiq_2835_arm.c | 4 ++- .../vc04_services/interface/vchiq_arm/vchiq_arm.c | 35 +++--- .../vc04_services/interface/vchiq_arm/vchiq_arm.h | 5 10 files changed, 47 insertions(+), 18 deletions(-) create mode 100644 arch/arm/boot/dts/bcm2836-rpi.dtsi -- 2.7.4 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 1/4] staging/vc04_services: Use correct cache line size
On 14/09/2018 18:03, Florian Fainelli wrote:
On 09/14/2018 06:22 AM, Phil Elwell wrote:
Use the compatible string in the DTB to select the correct cache line
size for the SoC - 32 for BCM2835, and 64 for BCM2836 and BCM2837.
Signed-off-by: Phil Elwell
---
[snip]
@@ -170,6 +170,14 @@ static struct device *vchiq_dev;
static DEFINE_SPINLOCK(msg_queue_spinlock);
static struct platform_device *bcm2835_camera;
+static struct vchiq_drvdata bcm2835_drvdata = {
+ .cache_line_size = 32,
+};
+
+static struct vchiq_drvdata bcm2836_drvdata = {
+ .cache_line_size = 64,
+};
Those two structures could probably be marked const. Other than that,
the approach definitively looks good to me.
The mutability is intentional - the structure pointer to the firmware is also
stored there. This isn't the only piece of mutable static data in a driver
that will only have a single instance, so allocating and initialising a
per-instance structure seems needlessly complicated.
Thanks for the feedback.
Phil
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 1/4] staging/vc04_services: Use correct cache line size
On 14/09/2018 14:22, Phil Elwell wrote:
Use the compatible string in the DTB to select the correct cache line
size for the SoC - 32 for BCM2835, and 64 for BCM2836 and BCM2837.
Signed-off-by: Phil Elwell
---
.../interface/vchiq_arm/vchiq_2835_arm.c | 4 ++-
.../vc04_services/interface/vchiq_arm/vchiq_arm.c | 36 --
.../vc04_services/interface/vchiq_arm/vchiq_arm.h | 5 +++
3 files changed, 34 insertions(+), 11 deletions(-)
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
index e767209..83d740f 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
@@ -109,7 +109,8 @@ free_pagelist(struct vchiq_pagelist_info *pagelistinfo,
int vchiq_platform_init(struct platform_device *pdev, VCHIQ_STATE_T *state)
{
struct device *dev = >dev;
- struct rpi_firmware *fw = platform_get_drvdata(pdev);
+ struct vchiq_drvdata *drvdata = platform_get_drvdata(pdev);
+ struct rpi_firmware *fw = drvdata->fw;
VCHIQ_SLOT_ZERO_T *vchiq_slot_zero;
struct resource *res;
void *slot_mem;
@@ -127,6 +128,7 @@ int vchiq_platform_init(struct platform_device *pdev,
VCHIQ_STATE_T *state)
if (err < 0)
return err;
+ g_cache_line_size = drvdata->cache_line_size;
g_fragments_size = 2 * g_cache_line_size;
/* Allocate space for the channels in coherent memory */
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index bc05c69..b2ae9259 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -170,6 +170,14 @@ static struct device *vchiq_dev;
static DEFINE_SPINLOCK(msg_queue_spinlock);
static struct platform_device *bcm2835_camera;
+static struct vchiq_drvdata bcm2835_drvdata = {
+ .cache_line_size = 32,
+};
+
+static struct vchiq_drvdata bcm2836_drvdata = {
+ .cache_line_size = 64,
+};
+
static const char *const ioctl_names[] = {
"CONNECT",
"SHUTDOWN",
@@ -3573,12 +3581,26 @@ void vchiq_platform_conn_state_changed(VCHIQ_STATE_T
*state,
}
}
+static const struct of_device_id vchiq_of_match[] = {
+ { .compatible = "brcm,bcm2835-vchiq", .data = _drvdata },
+ { .compatible = "brcm,bcm2836-vchiq", .data = _drvdata },
+ {},
+};
+MODULE_DEVICE_TABLE(of, vchiq_of_match);
+
static int vchiq_probe(struct platform_device *pdev)
{
struct device_node *fw_node;
- struct rpi_firmware *fw;
+ const struct of_device_id *of_id;
+ struct vchiq_drvdata *drvdata;
int err;
+ snd_rpi_simple.dev = >dev;
+ of_id = of_match_node(vchiq_of_match, pdev->dev.of_node);
+ drvdata = of_id->data;
+ if (!drvdata)
+ return -EINVAL;
+
fw_node = of_find_compatible_node(NULL, NULL,
"raspberrypi,bcm2835-firmware");
if (!fw_node) {
@@ -3586,12 +3608,12 @@ static int vchiq_probe(struct platform_device *pdev)
return -ENOENT;
}
- fw = rpi_firmware_get(fw_node);
+ drvdata->fw = rpi_firmware_get(fw_node);
of_node_put(fw_node);
- if (!fw)
+ if (!drvdata->fw)
return -EPROBE_DEFER;
- platform_set_drvdata(pdev, fw);
+ platform_set_drvdata(pdev, drvdata);
err = vchiq_platform_init(pdev, _state);
if (err != 0)
@@ -3661,12 +3683,6 @@ static int vchiq_remove(struct platform_device *pdev)
return 0;
}
-static const struct of_device_id vchiq_of_match[] = {
- { .compatible = "brcm,bcm2835-vchiq", },
- {},
-};
-MODULE_DEVICE_TABLE(of, vchiq_of_match);
-
static struct platform_driver vchiq_driver = {
.driver = {
.name = "bcm2835_vchiq",
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
index 40bb0c6..2f3ebc9 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
@@ -123,6 +123,11 @@ typedef struct vchiq_arm_state_struct {
} VCHIQ_ARM_STATE_T;
+struct vchiq_drvdata {
+ const unsigned int cache_line_size;
+ struct rpi_firmware *fw;
+};
+
extern int vchiq_arm_log_level;
extern int vchiq_susp_log_level;
This version doesn't compile (wrong defconfig used when building), but any
criticism of the
approach before v3 is welcome.
Phil
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 2/4] dt-bindings: soc: Document "brcm,bcm2836-vchiq"
"brcm,bcm2836-vchiq" should be used on BCM2836 and BCM2837 to ensure correct operation. Signed-off-by: Phil Elwell --- Documentation/devicetree/bindings/soc/bcm/brcm,bcm2835-vchiq.txt | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Documentation/devicetree/bindings/soc/bcm/brcm,bcm2835-vchiq.txt b/Documentation/devicetree/bindings/soc/bcm/brcm,bcm2835-vchiq.txt index 8dd7b3a..f331316 100644 --- a/Documentation/devicetree/bindings/soc/bcm/brcm,bcm2835-vchiq.txt +++ b/Documentation/devicetree/bindings/soc/bcm/brcm,bcm2835-vchiq.txt @@ -2,7 +2,8 @@ Broadcom VCHIQ firmware services Required properties: -- compatible: Should be "brcm,bcm2835-vchiq" +- compatible: Should be "brcm,bcm2835-vchiq" on BCM2835, otherwise + "brcm,bcm2836-vchiq". - reg: Physical base address and length of the doorbell register pair - interrupts: The interrupt number See bindings/interrupt-controller/brcm,bcm2835-armctrl-ic.txt -- 2.7.4 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 3/4] ARM: dts: bcm283x: Correct vchiq compatible string
To allow VCHIQ to determine the correct cache line size, use the new
"brcm,bcm2836-vchiq" compatible string on BCM2836 and BCM2837.
Signed-off-by: Phil Elwell
---
arch/arm/boot/dts/bcm2835-rpi.dtsi | 2 +-
arch/arm/boot/dts/bcm2836-rpi-2-b.dts | 2 +-
arch/arm/boot/dts/bcm2836-rpi.dtsi | 6 ++
arch/arm/boot/dts/bcm2837-rpi-3-b-plus.dts | 2 +-
arch/arm/boot/dts/bcm2837-rpi-3-b.dts | 2 +-
arch/arm/boot/dts/bcm2837-rpi-cm3.dtsi | 2 +-
6 files changed, 11 insertions(+), 5 deletions(-)
create mode 100644 arch/arm/boot/dts/bcm2836-rpi.dtsi
diff --git a/arch/arm/boot/dts/bcm2835-rpi.dtsi
b/arch/arm/boot/dts/bcm2835-rpi.dtsi
index cb2d6d7..215d8cc 100644
--- a/arch/arm/boot/dts/bcm2835-rpi.dtsi
+++ b/arch/arm/boot/dts/bcm2835-rpi.dtsi
@@ -30,7 +30,7 @@
#power-domain-cells = <1>;
};
- mailbox@7e00b840 {
+ vchiq: mailbox@7e00b840 {
compatible = "brcm,bcm2835-vchiq";
reg = <0x7e00b840 0xf>;
interrupts = <0 2>;
diff --git a/arch/arm/boot/dts/bcm2836-rpi-2-b.dts
b/arch/arm/boot/dts/bcm2836-rpi-2-b.dts
index 2fef70a..ac4408b 100644
--- a/arch/arm/boot/dts/bcm2836-rpi-2-b.dts
+++ b/arch/arm/boot/dts/bcm2836-rpi-2-b.dts
@@ -1,7 +1,7 @@
// SPDX-License-Identifier: GPL-2.0
/dts-v1/;
#include "bcm2836.dtsi"
-#include "bcm2835-rpi.dtsi"
+#include "bcm2836-rpi.dtsi"
#include "bcm283x-rpi-smsc9514.dtsi"
#include "bcm283x-rpi-usb-host.dtsi"
diff --git a/arch/arm/boot/dts/bcm2836-rpi.dtsi
b/arch/arm/boot/dts/bcm2836-rpi.dtsi
new file mode 100644
index 000..c4c858b
--- /dev/null
+++ b/arch/arm/boot/dts/bcm2836-rpi.dtsi
@@ -0,0 +1,6 @@
+// SPDX-License-Identifier: GPL-2.0
+#include "bcm2835-rpi.dtsi"
+
+ {
+ compatible = "brcm,bcm2836-vchiq", "brcm,bcm2835-vchiq";
+};
diff --git a/arch/arm/boot/dts/bcm2837-rpi-3-b-plus.dts
b/arch/arm/boot/dts/bcm2837-rpi-3-b-plus.dts
index 4adb85e..eca36e3 100644
--- a/arch/arm/boot/dts/bcm2837-rpi-3-b-plus.dts
+++ b/arch/arm/boot/dts/bcm2837-rpi-3-b-plus.dts
@@ -1,7 +1,7 @@
// SPDX-License-Identifier: GPL-2.0
/dts-v1/;
#include "bcm2837.dtsi"
-#include "bcm2835-rpi.dtsi"
+#include "bcm2836-rpi.dtsi"
#include "bcm283x-rpi-lan7515.dtsi"
#include "bcm283x-rpi-usb-host.dtsi"
diff --git a/arch/arm/boot/dts/bcm2837-rpi-3-b.dts
b/arch/arm/boot/dts/bcm2837-rpi-3-b.dts
index c318bcb..a0ba0f6 100644
--- a/arch/arm/boot/dts/bcm2837-rpi-3-b.dts
+++ b/arch/arm/boot/dts/bcm2837-rpi-3-b.dts
@@ -1,7 +1,7 @@
// SPDX-License-Identifier: GPL-2.0
/dts-v1/;
#include "bcm2837.dtsi"
-#include "bcm2835-rpi.dtsi"
+#include "bcm2836-rpi.dtsi"
#include "bcm283x-rpi-smsc9514.dtsi"
#include "bcm283x-rpi-usb-host.dtsi"
diff --git a/arch/arm/boot/dts/bcm2837-rpi-cm3.dtsi
b/arch/arm/boot/dts/bcm2837-rpi-cm3.dtsi
index 7b7ab6a..4a89a18 100644
--- a/arch/arm/boot/dts/bcm2837-rpi-cm3.dtsi
+++ b/arch/arm/boot/dts/bcm2837-rpi-cm3.dtsi
@@ -1,7 +1,7 @@
// SPDX-License-Identifier: GPL-2.0
/dts-v1/;
#include "bcm2837.dtsi"
-#include "bcm2835-rpi.dtsi"
+#include "bcm2836-rpi.dtsi"
/ {
memory {
--
2.7.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 1/4] staging/vc04_services: Use correct cache line size
Use the compatible string in the DTB to select the correct cache line
size for the SoC - 32 for BCM2835, and 64 for BCM2836 and BCM2837.
Signed-off-by: Phil Elwell
---
.../interface/vchiq_arm/vchiq_2835_arm.c | 4 ++-
.../vc04_services/interface/vchiq_arm/vchiq_arm.c | 36 --
.../vc04_services/interface/vchiq_arm/vchiq_arm.h | 5 +++
3 files changed, 34 insertions(+), 11 deletions(-)
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
index e767209..83d740f 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
@@ -109,7 +109,8 @@ free_pagelist(struct vchiq_pagelist_info *pagelistinfo,
int vchiq_platform_init(struct platform_device *pdev, VCHIQ_STATE_T *state)
{
struct device *dev = >dev;
- struct rpi_firmware *fw = platform_get_drvdata(pdev);
+ struct vchiq_drvdata *drvdata = platform_get_drvdata(pdev);
+ struct rpi_firmware *fw = drvdata->fw;
VCHIQ_SLOT_ZERO_T *vchiq_slot_zero;
struct resource *res;
void *slot_mem;
@@ -127,6 +128,7 @@ int vchiq_platform_init(struct platform_device *pdev,
VCHIQ_STATE_T *state)
if (err < 0)
return err;
+ g_cache_line_size = drvdata->cache_line_size;
g_fragments_size = 2 * g_cache_line_size;
/* Allocate space for the channels in coherent memory */
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index bc05c69..b2ae9259 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -170,6 +170,14 @@ static struct device *vchiq_dev;
static DEFINE_SPINLOCK(msg_queue_spinlock);
static struct platform_device *bcm2835_camera;
+static struct vchiq_drvdata bcm2835_drvdata = {
+ .cache_line_size = 32,
+};
+
+static struct vchiq_drvdata bcm2836_drvdata = {
+ .cache_line_size = 64,
+};
+
static const char *const ioctl_names[] = {
"CONNECT",
"SHUTDOWN",
@@ -3573,12 +3581,26 @@ void vchiq_platform_conn_state_changed(VCHIQ_STATE_T
*state,
}
}
+static const struct of_device_id vchiq_of_match[] = {
+ { .compatible = "brcm,bcm2835-vchiq", .data = _drvdata },
+ { .compatible = "brcm,bcm2836-vchiq", .data = _drvdata },
+ {},
+};
+MODULE_DEVICE_TABLE(of, vchiq_of_match);
+
static int vchiq_probe(struct platform_device *pdev)
{
struct device_node *fw_node;
- struct rpi_firmware *fw;
+ const struct of_device_id *of_id;
+ struct vchiq_drvdata *drvdata;
int err;
+ snd_rpi_simple.dev = >dev;
+ of_id = of_match_node(vchiq_of_match, pdev->dev.of_node);
+ drvdata = of_id->data;
+ if (!drvdata)
+ return -EINVAL;
+
fw_node = of_find_compatible_node(NULL, NULL,
"raspberrypi,bcm2835-firmware");
if (!fw_node) {
@@ -3586,12 +3608,12 @@ static int vchiq_probe(struct platform_device *pdev)
return -ENOENT;
}
- fw = rpi_firmware_get(fw_node);
+ drvdata->fw = rpi_firmware_get(fw_node);
of_node_put(fw_node);
- if (!fw)
+ if (!drvdata->fw)
return -EPROBE_DEFER;
- platform_set_drvdata(pdev, fw);
+ platform_set_drvdata(pdev, drvdata);
err = vchiq_platform_init(pdev, _state);
if (err != 0)
@@ -3661,12 +3683,6 @@ static int vchiq_remove(struct platform_device *pdev)
return 0;
}
-static const struct of_device_id vchiq_of_match[] = {
- { .compatible = "brcm,bcm2835-vchiq", },
- {},
-};
-MODULE_DEVICE_TABLE(of, vchiq_of_match);
-
static struct platform_driver vchiq_driver = {
.driver = {
.name = "bcm2835_vchiq",
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
index 40bb0c6..2f3ebc9 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
@@ -123,6 +123,11 @@ typedef struct vchiq_arm_state_struct {
} VCHIQ_ARM_STATE_T;
+struct vchiq_drvdata {
+ const unsigned int cache_line_size;
+ struct rpi_firmware *fw;
+};
+
extern int vchiq_arm_log_level;
extern int vchiq_susp_log_level;
--
2.7.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 resend] staging: bcm2835-audio: Fix memory corruption
The previous commit (0adbfd46) fixed a memory leak but also freed a
block in the success case, causing a stale pointer to be used with
potentially fatal results. Only free the vchi_instance block in the
case that vchi_connect fails; once connected, the instance is
retained for subsequent connections.
Simplifying the code by removing a bunch of gotos and returning errors
directly.
Signed-off-by: Phil Elwell <p...@raspberrypi.org>
Fixes: 0adbfd4694c2 ("staging: bcm2835-audio: fix memory leak in
bcm2835_audio_open_connection()")
---
v2: Simplified following feedback from Dan Carpenter.
---
.../vc04_services/bcm2835-audio/bcm2835-vchiq.c | 19 +++
1 file changed, 7 insertions(+), 12 deletions(-)
diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
index 5f3d8f2..4be864d 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
@@ -390,8 +390,7 @@ static int bcm2835_audio_open_connection(struct
bcm2835_alsa_stream *alsa_stream
__func__, instance);
instance->alsa_stream = alsa_stream;
alsa_stream->instance = instance;
- ret = 0; // xxx todo -1;
- goto err_free_mem;
+ return 0;
}
/* Initialize and create a VCHI connection */
@@ -401,16 +400,15 @@ static int bcm2835_audio_open_connection(struct
bcm2835_alsa_stream *alsa_stream
LOG_ERR("%s: failed to initialise VCHI instance
(ret=%d)\n",
__func__, ret);
- ret = -EIO;
- goto err_free_mem;
+ return -EIO;
}
ret = vchi_connect(NULL, 0, vchi_instance);
if (ret) {
LOG_ERR("%s: failed to connect VCHI instance
(ret=%d)\n",
__func__, ret);
- ret = -EIO;
- goto err_free_mem;
+ kfree(vchi_instance);
+ return -EIO;
}
initted = 1;
}
@@ -421,19 +419,16 @@ static int bcm2835_audio_open_connection(struct
bcm2835_alsa_stream *alsa_stream
if (IS_ERR(instance)) {
LOG_ERR("%s: failed to initialize audio service\n", __func__);
- ret = PTR_ERR(instance);
- goto err_free_mem;
+ /* vchi_instance is retained for use the next time. */
+ return PTR_ERR(instance);
}
instance->alsa_stream = alsa_stream;
alsa_stream->instance = instance;
LOG_DBG(" success !\n");
- ret = 0;
-err_free_mem:
- kfree(vchi_instance);
- return ret;
+ return 0;
}
int bcm2835_audio_open(struct bcm2835_alsa_stream *alsa_stream)
--
1.9.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2] staging: bcm2835-audio: Fix memory corruption
The previous commit (0adbfd46) fixed a memory leak but also freed a
block in the success case, causing a stale pointer to be used with
potentially fatal results. Only free the vchi_instance block in the
case that vchi_connect fails; once connected, the instance is
retained for subsequent connections.
Simplifying the code by removing a bunch of gotos and returning errors
directly.
Signed-off-by: Phil Elwell <p...@raspberrypi.org>
Fixes: 0adbfd4694c2 ("staging: bcm2835-audio: fix memory leak in
bcm2835_audio_open_connection()")
---
[Resend with v2 in subject]
v2: Simplified following feedback from Dan Carpenter.
---
.../vc04_services/bcm2835-audio/bcm2835-vchiq.c | 19 +++
1 file changed, 7 insertions(+), 12 deletions(-)
diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
index 5f3d8f2..4be864d 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
@@ -390,8 +390,7 @@ static int bcm2835_audio_open_connection(struct
bcm2835_alsa_stream *alsa_stream
__func__, instance);
instance->alsa_stream = alsa_stream;
alsa_stream->instance = instance;
- ret = 0; // xxx todo -1;
- goto err_free_mem;
+ return 0;
}
/* Initialize and create a VCHI connection */
@@ -401,16 +400,15 @@ static int bcm2835_audio_open_connection(struct
bcm2835_alsa_stream *alsa_stream
LOG_ERR("%s: failed to initialise VCHI instance
(ret=%d)\n",
__func__, ret);
- ret = -EIO;
- goto err_free_mem;
+ return -EIO;
}
ret = vchi_connect(NULL, 0, vchi_instance);
if (ret) {
LOG_ERR("%s: failed to connect VCHI instance
(ret=%d)\n",
__func__, ret);
- ret = -EIO;
- goto err_free_mem;
+ kfree(vchi_instance);
+ return -EIO;
}
initted = 1;
}
@@ -421,19 +419,16 @@ static int bcm2835_audio_open_connection(struct
bcm2835_alsa_stream *alsa_stream
if (IS_ERR(instance)) {
LOG_ERR("%s: failed to initialize audio service\n", __func__);
- ret = PTR_ERR(instance);
- goto err_free_mem;
+ /* vchi_instance is retained for use the next time. */
+ return PTR_ERR(instance);
}
instance->alsa_stream = alsa_stream;
alsa_stream->instance = instance;
LOG_DBG(" success !\n");
- ret = 0;
-err_free_mem:
- kfree(vchi_instance);
- return ret;
+ return 0;
}
int bcm2835_audio_open(struct bcm2835_alsa_stream *alsa_stream)
--
1.9.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: bcm2835-audio: Fix memory corruption
The previous commit (0adbfd46) fixed a memory leak but also freed a
block in the success case, causing a stale pointer to be used with
potentially fatal results. Only free the vchi_instance block in the
case that vchi_connect fails; once connected, the instance is
retained for subsequent connections.
Simplifying the code by removing a bunch of gotos and returning errors
directly.
Signed-off-by: Phil Elwell <p...@raspberrypi.org>
Fixes: 0adbfd4694c2 ("staging: bcm2835-audio: fix memory leak in
bcm2835_audio_open_connection()")
---
v2: Simplified following feedback from Dan Carpenter.
---
.../vc04_services/bcm2835-audio/bcm2835-vchiq.c | 19 +++
1 file changed, 7 insertions(+), 12 deletions(-)
diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
index 5f3d8f2..4be864d 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
@@ -390,8 +390,7 @@ static int bcm2835_audio_open_connection(struct
bcm2835_alsa_stream *alsa_stream
__func__, instance);
instance->alsa_stream = alsa_stream;
alsa_stream->instance = instance;
- ret = 0; // xxx todo -1;
- goto err_free_mem;
+ return 0;
}
/* Initialize and create a VCHI connection */
@@ -401,16 +400,15 @@ static int bcm2835_audio_open_connection(struct
bcm2835_alsa_stream *alsa_stream
LOG_ERR("%s: failed to initialise VCHI instance
(ret=%d)\n",
__func__, ret);
- ret = -EIO;
- goto err_free_mem;
+ return -EIO;
}
ret = vchi_connect(NULL, 0, vchi_instance);
if (ret) {
LOG_ERR("%s: failed to connect VCHI instance
(ret=%d)\n",
__func__, ret);
- ret = -EIO;
- goto err_free_mem;
+ kfree(vchi_instance);
+ return -EIO;
}
initted = 1;
}
@@ -421,19 +419,16 @@ static int bcm2835_audio_open_connection(struct
bcm2835_alsa_stream *alsa_stream
if (IS_ERR(instance)) {
LOG_ERR("%s: failed to initialize audio service\n", __func__);
- ret = PTR_ERR(instance);
- goto err_free_mem;
+ /* vchi_instance is retained for use the next time. */
+ return PTR_ERR(instance);
}
instance->alsa_stream = alsa_stream;
alsa_stream->instance = instance;
LOG_DBG(" success !\n");
- ret = 0;
-err_free_mem:
- kfree(vchi_instance);
- return ret;
+ return 0;
}
int bcm2835_audio_open(struct bcm2835_alsa_stream *alsa_stream)
--
1.9.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: bcm2835-audio: Fix memory corruption
On 10/08/2017 12:24, Dan Carpenter wrote:
> On Thu, Aug 10, 2017 at 11:52:42AM +0100, Phil Elwell wrote:
>> On 10/08/2017 11:21, Dan Carpenter wrote:
>>> The original patch did not go through the normal review process...
>>>
>>> On Tue, Aug 08, 2017 at 01:05:02PM +0100, Phil Elwell wrote:
>>>> I'm all for fixing memory leaks, but freeing a block while it is still
>>>> being used is a recipe for hard-to-debug kernel exceptions.
>>>>
>>>
>>> This bug completely breaks the driver doesn't it? It's not very subtle
>>> so it should be easy to diagnose with git bisect.
>>
>> It's more subtle than that - the failure isn't consistent, sometimes crashing
>> and sometimes not depending on how and when memory is reused.
>>
>>>> 1) There is already a vchi method for freeing the instance, so use it.
>>>> 2) Only call it on error, and then only before initted is false.
>>>>
>>>> Signed-off-by: Phil Elwell <p...@raspberrypi.org>
>>>> Fixes: 0adbfd4694c2 ("staging: bcm2835-audio: fix memory leak in
>>>> bcm2835_audio_open_connection()")
>>>> ---
>>>> drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
>>>> b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
>>>> index 5f3d8f2..89f96f3 100644
>>>> --- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
>>>> +++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
>>>> @@ -409,6 +409,7 @@ static int bcm2835_audio_open_connection(struct
>>>> bcm2835_alsa_stream *alsa_stream
>>>>LOG_ERR("%s: failed to connect VCHI instance
>>>> (ret=%d)\n",
>>>>__func__, ret);
>>>>
>>>> + vchi_disconnect(vchi_instance);
>>>
>>> This is ugly because why are we calling disconnect() if connect() fails?
>>> These functions should be symetric so disconnect only disconnects and
>>> we call a different function to undo vchi_initialise().
>>
>> Agreed - I'm not going to change the API, but I can add a comment.
>>
>
> Nah... Please don't do that. Just create a function:
>
> void vchiq_free(VCHIQ_INSTANCE_T *vchi_instance)
> {
> kfree(vchi_instance);
> }
>
> Really vchi_initialise() is badly named and it's just a wrapper around
> vchiq_initialise(). It should be deleted and vchiq_initialise() should
> be renamed to allocate instead of initialize... But that's for a
> separate patch.
>
> And also we should move the kfree() out of disconnect() like I said
> and instead call vchiq_free(). But again, that's for a separate patch.
>
> Change the goto to "return -EIO." Leave the last error path as-is.
> Eventually we will want to break this into two functions, one that
> allocates the first vchi_instance and one that calls vc_vchi_audio_init().
> But again, there are many patches needed to fix this code.
[ static removed ]
Shouldn't that be:
void vchi_uninitialise(VCHI_INSTANCE_T instance_handle)
{
vchiq_shutdown((VCHIQ_INSTANCE_T)instance_handle);
}
EXPORT_SYMBOL(vchi_uninitialise);
Yes, in this case the instance is just a block of memory with no users,
but if this is a general API call then you want it to work in all cases,
and calling vchiq_shutdown is the right way to free an instance. Calling
a VCHIQ method when all the other calls are to the VCHI API is grubby,
so make it a VCHI method instead.
If instead this is just another hack - a kfree with a comment in code form -
then lets keep it static within the audio driver (which is what I thought you
were initially suggesting).
Phil
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: bcm2835-audio: Fix memory corruption
On 10/08/2017 11:21, Dan Carpenter wrote:
> The original patch did not go through the normal review process...
>
> On Tue, Aug 08, 2017 at 01:05:02PM +0100, Phil Elwell wrote:
>> I'm all for fixing memory leaks, but freeing a block while it is still
>> being used is a recipe for hard-to-debug kernel exceptions.
>>
>
> This bug completely breaks the driver doesn't it? It's not very subtle
> so it should be easy to diagnose with git bisect.
It's more subtle than that - the failure isn't consistent, sometimes crashing
and sometimes not depending on how and when memory is reused.
>> 1) There is already a vchi method for freeing the instance, so use it.
>> 2) Only call it on error, and then only before initted is false.
>>
>> Signed-off-by: Phil Elwell <p...@raspberrypi.org>
>> Fixes: 0adbfd4694c2 ("staging: bcm2835-audio: fix memory leak in
>> bcm2835_audio_open_connection()")
>> ---
>> drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
>> b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
>> index 5f3d8f2..89f96f3 100644
>> --- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
>> +++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
>> @@ -409,6 +409,7 @@ static int bcm2835_audio_open_connection(struct
>> bcm2835_alsa_stream *alsa_stream
>> LOG_ERR("%s: failed to connect VCHI instance
>> (ret=%d)\n",
>> __func__, ret);
>>
>> +vchi_disconnect(vchi_instance);
>
> This is ugly because why are we calling disconnect() if connect() fails?
> These functions should be symetric so disconnect only disconnects and
> we call a different function to undo vchi_initialise().
Agreed - I'm not going to change the API, but I can add a comment.
>> ret = -EIO;
>> goto err_free_mem;
>
> This label name is out of date. There is a later error path where
> vc_vchi_audio_init() fails and we leak on that path.
Also agreed. I'll rework it.
> Also why is
> vchi_instance a static variable? Arglebargleblargleblah...
You may well think that. I couldn't possibly comment.
Phil
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: bcm2835-audio: Fix memory corruption
I'm all for fixing memory leaks, but freeing a block while it is still
being used is a recipe for hard-to-debug kernel exceptions.
1) There is already a vchi method for freeing the instance, so use it.
2) Only call it on error, and then only before initted is false.
Signed-off-by: Phil Elwell <p...@raspberrypi.org>
Fixes: 0adbfd4694c2 ("staging: bcm2835-audio: fix memory leak in
bcm2835_audio_open_connection()")
---
drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
index 5f3d8f2..89f96f3 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
@@ -409,6 +409,7 @@ static int bcm2835_audio_open_connection(struct
bcm2835_alsa_stream *alsa_stream
LOG_ERR("%s: failed to connect VCHI instance
(ret=%d)\n",
__func__, ret);
+ vchi_disconnect(vchi_instance);
ret = -EIO;
goto err_free_mem;
}
@@ -431,7 +432,6 @@ static int bcm2835_audio_open_connection(struct
bcm2835_alsa_stream *alsa_stream
LOG_DBG(" success !\n");
ret = 0;
err_free_mem:
- kfree(vchi_instance);
return ret;
}
--
1.9.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [Bug] VCHIQ functional test broken
On 15/05/2017 15:54, Stefan Wahren wrote: > 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? I'm sure you mean copy_to_user(), and the answer is "it's complicated". Depending on the relative timing, this code can also be called from a kernel thread, so I doubt that would work. Phil ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [Bug] VCHIQ functional test broken
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); Phil ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: vc04_services: Fix bulk cache maintenance
On 10/05/2017 10:06, Greg Kroah-Hartman wrote:
> On Wed, May 10, 2017 at 09:42:43AM +0100, Phil Elwell wrote:
>> On 04/05/2017 18:51, Stefan Wahren wrote:
>>>
>>>> Phil Elwell <p...@raspberrypi.org> hat am 4. Mai 2017 um 11:58 geschrieben:
>>>>
>>>>
>>>> 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.
>>>>
>>>> Fixes: cf9caf192988 ("staging: vc04_services: Replace dmac_map_area with
>>>> dmac_map_sg")
>>>> Signed-off-by: Phil Elwell <p...@raspberrypi.org>
>>>
>>> Reported-by: Stefan Wahren <stefan.wah...@i2se.com>
>>> Tested-by: Stefan Wahren <stefan.wah...@i2se.com>
>>>
>>> In order to clarify the context of this issue:
>>>
>>> http://lists.infradead.org/pipermail/linux-rpi-kernel/2017-April/006149.html
>>
>> Thanks, Stefan.
>>
>> Is there no feedback other on this patch? It's been in the rpi-4.11.y
>> downstream
>> branch for a week now with favourable results - see issue
>> https://github.com/raspberrypi/linux/issues/1977 and pr
>> https://github.com/raspberrypi/linux/pull/1987.
>
> It's the middle of the merge window, I can't do anything with this
> until after 4.12-rc1 comes out. Please be patient...
Yes, of course - that's the kind of feedback I was looking for.
Thanks,
Phil
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: vc04_services: Fix bulk cache maintenance
On 04/05/2017 18:51, Stefan Wahren wrote:
>
>> Phil Elwell <p...@raspberrypi.org> hat am 4. Mai 2017 um 11:58 geschrieben:
>>
>>
>> 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.
>>
>> Fixes: cf9caf192988 ("staging: vc04_services: Replace dmac_map_area with
>> dmac_map_sg")
>> Signed-off-by: Phil Elwell <p...@raspberrypi.org>
>
> Reported-by: Stefan Wahren <stefan.wah...@i2se.com>
> Tested-by: Stefan Wahren <stefan.wah...@i2se.com>
>
> In order to clarify the context of this issue:
>
> http://lists.infradead.org/pipermail/linux-rpi-kernel/2017-April/006149.html
Thanks, Stefan.
Is there no feedback other on this patch? It's been in the rpi-4.11.y downstream
branch for a week now with favourable results - see issue
https://github.com/raspberrypi/linux/issues/1977 and pr
https://github.com/raspberrypi/linux/pull/1987.
Phil
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: vc04_services: Fix bulk cache maintenance
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.
Fixes: cf9caf192988 ("staging: vc04_services: Replace dmac_map_area with
dmac_map_sg")
Signed-off-by: Phil Elwell <p...@raspberrypi.org>
---
.../interface/vchiq_arm/vchiq_2835_arm.c | 31 +-
1 file changed, 19 insertions(+), 12 deletions(-)
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
index 3aeffcb..02e9736 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
@@ -501,8 +501,15 @@ int vchiq_platform_init(struct platform_device *pdev,
VCHIQ_STATE_T *state)
*/
sg_init_table(scatterlist, num_pages);
/* Now set the pages for each scatterlist */
- for (i = 0; i < num_pages; i++)
- sg_set_page(scatterlist + i, pages[i], PAGE_SIZE, 0);
+ for (i = 0; i < num_pages; i++) {
+ unsigned int len = PAGE_SIZE - offset;
+
+ if (len > count)
+ len = count;
+ sg_set_page(scatterlist + i, pages[i], len, offset);
+ offset = 0;
+ count -= len;
+ }
dma_buffers = dma_map_sg(g_dev,
scatterlist,
@@ -523,20 +530,20 @@ int vchiq_platform_init(struct platform_device *pdev,
VCHIQ_STATE_T *state)
u32 addr = sg_dma_address(sg);
/* Note: addrs is the address + page_count - 1
-* The firmware expects the block to be page
+* The firmware expects blocks after the first to be page-
* aligned and a multiple of the page size
*/
WARN_ON(len == 0);
- WARN_ON(len & ~PAGE_MASK);
- WARN_ON(addr & ~PAGE_MASK);
+ WARN_ON(i && (i != (dma_buffers - 1)) && (len & ~PAGE_MASK));
+ WARN_ON(i && (addr & ~PAGE_MASK));
if (k > 0 &&
- ((addrs[k - 1] & PAGE_MASK) |
- ((addrs[k - 1] & ~PAGE_MASK) + 1) << PAGE_SHIFT)
- == addr) {
- addrs[k - 1] += (len >> PAGE_SHIFT);
- } else {
- addrs[k++] = addr | ((len >> PAGE_SHIFT) - 1);
- }
+ ((addrs[k - 1] & PAGE_MASK) +
+(((addrs[k - 1] & ~PAGE_MASK) + 1) << PAGE_SHIFT))
+ == (addr & PAGE_MASK))
+ addrs[k - 1] += ((len + PAGE_SIZE - 1) >> PAGE_SHIFT);
+ else
+ addrs[k++] = (addr & PAGE_MASK) |
+ (((len + PAGE_SIZE - 1) >> PAGE_SHIFT) - 1);
}
/* Partial cache lines (fragments) require special measures */
--
1.9.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [bug report] staging: add bcm2708 vchiq driver
On 16/11/2016 10:21, Dan Carpenter wrote: > On Tue, Nov 15, 2016 at 10:04:05PM -0500, Vince Weaver wrote: >> On Tue, 15 Nov 2016, Michael Zoran wrote: >> >>> I'm still interested to know more about the MMU statement. I would >>> think at least some of the RPI models have one because swapping appears >>> to be at least somewhat functional on the later models. >> All Raspberry Pi models have an MMU. I'm not sure why anyone thinks >> otherwise. > No idea. Someone told me that when I complained about a different > security bug on RPI. The ARM cores have MMUs between them and the SoC bus, but the VC4 VPU and its peripherals do not, leading to the need for contiguous allocations. Some VPU peripherals can only address 256MB of RAM, which further constrains allocations. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
