Message-ID: <cc7edba3-af91-44ef-9899-18c21a3f33bd@kernel.org>
Date: Sun, 2 Nov 2025 10:30:58 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Jai Luthra <jai.luthra@...asonboard.com>,
 Florian Fainelli <florian.fainelli@...adcom.com>,
 Raspberry Pi Kernel Maintenance <kernel-list@...pberrypi.com>,
 bcm-kernel-feedback-list@...adcom.com
Cc: Dave Stevenson <dave.stevenson@...pberrypi.com>,
 Phil Elwell <phil@...pberrypi.com>, Stefan Wahren <wahrenst@....net>,
 Laurent Pinchart <laurent.pinchart@...asonboard.com>,
 Kieran Bingham <kieran.bingham@...asonboard.com>,
 Sumit Semwal <sumit.semwal@...aro.org>,
 Christian König <christian.koenig@....com>,
 linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
 Dom Cobley <popcornmix@...il.com>,
 Alexander Winkowski <dereference23@...look.com>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 Juerg Haefliger <juerg.haefliger@...onical.com>
Subject: Re: [PATCH 09/13] platform/raspberrypi: Add new vc-sm-cma driver

On 31/10/2025 18:27, Jai Luthra wrote:
> From: Dave Stevenson <dave.stevenson@...pberrypi.com>
> 
> Add Broadcom VideoCore Shared Memory support.

You will have to come with really, really good argument why CMA is not
working for you. This is how you write commit msgs. All further is not
really helpful.

> 
> This new driver allows contiguous memory blocks to be imported into the
> VideoCore VPU memory map, and manages the lifetime of those objects,
> only releasing the source dmabuf once the VPU has confirmed it has
> finished with it.
> 
> Signed-off-by: Dave Stevenson <dave.stevenson@...pberrypi.com>
> 
> staging: vcsm-cma: Fix memory leak from not detaching dmabuf
> 
> When importing there was a missing call to detach the buffer, so each
> import leaked the sg table entry.
> 
> Actually the release process for both locally allocated and imported
> buffers is identical, so fix them to both use the same
> function.
> 
> Signed-off-by: Dave Stevenson <dave.stevenson@...pberrypi.com>
> 
> staging/vc-sm-cma: Avoid log spamming on Pi0/1 over cache alias.
> 
> Pi 0/1 use the 0x80000000 cache alias as the ARM also sees the world
> through the VPU L2 cache.
> vc-sm-cma was trying to ensure it was in an uncached alias (0xc), and
> complaining on every allocation if it weren't. Reduce this logging.
> 
> Signed-off-by: Dave Stevenson <dave.stevenson@...pberrypi.com>
> 
> vc-sm-cma: Restore correct cache maintainance operations
> 
> We have been using the more expensive flush operations rather than
> invalidate and clean since kernel rpi-5.9.y
> 
> These are exposed with:
> 52f1453513ba95084ab811a030032fe605b0cbe2 Re-expose some dmi APIs for use
> in VCSM
> 
> But I believe that commit was dropped when (non-cma) vc-sm was dropped,
> and didn't get updated when the commit was restored
> 
> Signed-off-by: Dom Cobley <popcornmix@...il.com>
> 
> staging: vc04_services: Fix clang14 warning
> 
> Insert a break to fix a fallthrough warning from clang14. Since the
> fallthrough was to another break, this is a cosmetic change.
> 
> See: https://github.com/raspberrypi/linux/issues/5078
> 
> Signed-off-by: Phil Elwell <phil@...pberrypi.com>
> 
> vc04_services/vc-sm-cma: Handle upstream require vchiq_instance to be
> passed around
> 
> vc04_services/vc-sm-cma: Switch one-bit bitfields to bool
> 
> Clang 16 warns:
> 
> ../drivers/staging/vc04_services/vc-sm-cma/vc_sm.c:816:19: warning:
> implicit truncation from 'int' to a one-bit wide bit-field changes value
> from 1 to -1 [-Wsingle-bit-bitfield-constant-conversion]
>     buffer->imported = 1;
> 		     ^ ~
> ../drivers/staging/vc04_services/vc-sm-cma/vc_sm.c:822:17: warning:
> implicit truncation from 'int' to a one-bit wide bit-field changes value
> from 1 to -1 [-Wsingle-bit-bitfield-constant-conversion]
>     buffer->in_use = 1;
> 		   ^ ~
> 2 warnings generated.
> 
> Signed-off-by: Alexander Winkowski <dereference23@...look.com>
> 
> vc04_services: vcsm-cma: Detach from the correct dmabuf
> 
> Commit d3292daee319 ("dma-buf: Make locking consistent in
> dma_buf_detach()")
> added checking that the same dmabuf for which dma_buf_attach
> was called is passed into dma_buf_detach, which flagged up
> that vcsm-cma was passing in the wrong dmabuf.
> 
> Correct this so that we don't get the WARN on every dma_buf
> release.
> 
> Signed-off-by: Dave Stevenson <dave.stevenson@...pberrypi.com>
> 
> staging: vc04_services: vc-sm-cma: Remove deprecated header
> 
> The vchiq_connected.h header was removed in f875976ecf45 ("staging:
> vc04_services: Drop vchiq_connected.[ch] files") to simplify the
> implementation.
> 
> Update the vc_sm driver accordingly which can still use the same
> functions through the vchiq_arm.h header declarations.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@...asonboard.com>
> 
> staging: vc04_services: vc-sm-cma: Drop include Makefile directive
> 
> Drop the include directive. They can break the build, when one only
> wants to build a subdirectory. Replace with "../" for the includes in
> the vc_sm files instead.
> 
> The fix is equivalent to the four patches between 29d49a76c5b2
> ("staging: vc04_services: bcm2835-audio: Drop include Makefile
> directive")...2529ca211402 ("staging: vc04_services: interface: Drop
> include Makefile directive")
> 
> Suggested-by: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> Signed-off-by: Kieran Bingham <kieran.bingham@...asonboard.com>
> 
> staging: vc04_services: vc-sm-cma: Register with vchiq_bus_type
> 
> Register the vcsm rive with the vchiq_bus_type instead of useing the
> platform driver/device.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@...asonboard.com>
> 
> staging: vc04_services: vc-sm-cma: Explicitly set DMA mask
> 
> The platform model originally handled the DMA mask. Now that
> we are on the vchiq_bus we need to explicitly set this.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@...asonboard.com>

That's a complete mess of tags. All tags are going to one place. Please
write single commit msg, not consisting of squashed 10 messages.

> 
> staging: vc04_services: vc-sm-cma: Use [map|unmap]_attachment_unlocked
> 
> lockdep throws warnings when using libcamera as buffers are
> mapped and unmapped as the dmabuf->resv lock hasn't been taken.
> 
> Switch to using the _unlocked variants so that the framework takes
> the lock.
> 
> https://github.com/raspberrypi/linux/issues/6814
> 
> Signed-off-by: Dave Stevenson <dave.stevenson@...pberrypi.com>
> 
> staging: vc04_services: vc-sm-cma: Use a mutex instead of spinlock
> 
> There are no contexts where we should be calling the kernelid_map
> IDR functions where we can't sleep, so switch from using a spinlock
> to using a mutex.
> 
> https://github.com/raspberrypi/linux/issues/6815
> 
> Signed-off-by: Dave Stevenson <dave.stevenson@...pberrypi.com>
> 
> staging: vc-sm-cma: Fix field-spanning write warning
> 
> Replace one-element array with flexible-array member to fix:
> 
> [   11.725017] ------------[ cut here ]------------
> [   11.725038] memcpy: detected field-spanning write (size 4) of single field "hdr->body" at drivers/staging/vc04_services/vc-sm-cma/vc_sm_cma_vchi.c:130 (size 0)
> [   11.725113] WARNING: CPU: 3 PID: 455 at drivers/staging/vc04_services/vc-sm-cma/vc_sm_cma_vchi.c:130 vc_vchi_cmd_create+0x1a8/0x1d0 [vc_sm_cma]
> 
> Signed-off-by: Juerg Haefliger <juerg.haefliger@...onical.com>
> 
> platform/raspberrypi: vc-sm-cma: Fix smatch warnings
> 
> Fix these two smatch warnings for the vc-sm-cma driver, rest were false
> positives:
> 
> ../drivers/platform/raspberrypi/vc-sm-cma/vc_sm.c:413
> vc_sm_dma_buf_attach() warn: inconsistent returns '&buf->lock'.
>   Locked on  : 396
>   Unlocked on: 413
> ../drivers/platform/raspberrypi/vc-sm-cma/vc_sm.c:1225
> vc_sm_cma_ioctl_alloc() error: we previously assumed 'buffer' could be
> null (see line 1113)
> 
> Signed-off-by: Dave Stevenson <dave.stevenson@...pberrypi.com>
> [jai.luthra: fix checkpatch and smatch warnings, add entry in MAINTAINERS]
> Co-developed-by: Jai Luthra <jai.luthra@...asonboard.com>
> Signed-off-by: Jai Luthra <jai.luthra@...asonboard.com>
> ---
>  MAINTAINERS                                             |    7 +
>  drivers/platform/raspberrypi/Kconfig                    |    2 +
>  drivers/platform/raspberrypi/Makefile                   |    1 +
>  drivers/platform/raspberrypi/vc-sm-cma/Kconfig          |    9 +
>  drivers/platform/raspberrypi/vc-sm-cma/Makefile         |    9 +
>  drivers/platform/raspberrypi/vc-sm-cma/vc_sm.c          | 1619 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/platform/raspberrypi/vc-sm-cma/vc_sm.h          |   83 ++++++
>  drivers/platform/raspberrypi/vc-sm-cma/vc_sm_cma_vchi.c |  513 +++++++++++++++++++++++++++++++++++++
>  drivers/platform/raspberrypi/vc-sm-cma/vc_sm_cma_vchi.h |   63 +++++
>  drivers/platform/raspberrypi/vc-sm-cma/vc_sm_defs.h     |  298 +++++++++++++++++++++
>  drivers/platform/raspberrypi/vc-sm-cma/vc_sm_knl.h      |   28 ++
>  include/linux/raspberrypi/vc_sm_cma_ioctl.h             |  114 +++++++++
>  12 files changed, 2746 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index df07d1a3c28d048e14a0f65c9f9ff01cc260013a..352c29bb3b94543bcb37c62d26d4c8bae48130ff 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -5289,6 +5289,13 @@ L:	netdev@...r.kernel.org
>  S:	Maintained
>  F:	drivers/net/ethernet/broadcom/tg3.*
>  
> +BROADCOM VIDEOCORE SHARED MEMORY DRIVER
> +M:	Raspberry Pi Kernel Maintenance <kernel-list@...pberrypi.com>
> +L:	linux-kernel@...r.kernel.org
> +S:	Maintained
> +F:	drivers/platform/raspberrypi/vc-sm-cma/*
> +F:	include/linux/raspberrypi/vc_sm_cma*
> +
>  BROADCOM VK DRIVER
>  M:	Scott Branden <scott.branden@...adcom.com>
>  R:	Broadcom internal kernel review list <bcm-kernel-feedback-list@...adcom.com>
> diff --git a/drivers/platform/raspberrypi/Kconfig b/drivers/platform/raspberrypi/Kconfig
> index 2c928440a47c08e4d452fe838fe4105c608995a4..68a7a2d5701cd6821ec4b7418a86bf61011c83f6 100644
> --- a/drivers/platform/raspberrypi/Kconfig
> +++ b/drivers/platform/raspberrypi/Kconfig
> @@ -48,5 +48,7 @@ config VCHIQ_CDEV
>  endif
>  
>  source "drivers/platform/raspberrypi/vchiq-mmal/Kconfig"
> +source "drivers/platform/raspberrypi/vc-sm-cma/Kconfig"
> +
>  
>  endif
> diff --git a/drivers/platform/raspberrypi/Makefile b/drivers/platform/raspberrypi/Makefile
> index 2a7c9511e5d8bbe11c05680eea016ef40796b648..1980f618e2185228e1fe173b1e94a3ede0e15bbb 100644
> --- a/drivers/platform/raspberrypi/Makefile
> +++ b/drivers/platform/raspberrypi/Makefile
> @@ -13,3 +13,4 @@ vchiq-objs += vchiq-interface/vchiq_dev.o
>  endif
>  
>  obj-$(CONFIG_BCM2835_VCHIQ_MMAL)	+= vchiq-mmal/
> +obj-$(CONFIG_BCM_VC_SM_CMA)		+= vc-sm-cma/
> diff --git a/drivers/platform/raspberrypi/vc-sm-cma/Kconfig b/drivers/platform/raspberrypi/vc-sm-cma/Kconfig
> new file mode 100644
> index 0000000000000000000000000000000000000000..7daec14dcfc14b6a1492ca9e1670807b5b4f87d8
> --- /dev/null
> +++ b/drivers/platform/raspberrypi/vc-sm-cma/Kconfig
> @@ -0,0 +1,9 @@
> +config BCM_VC_SM_CMA
> +	tristate "VideoCore Shared Memory (CMA) driver"
> +	select BCM2835_VCHIQ

Why you do not have > +	select DMA_SHARED_BUFFER
> +	help
> +	  Say Y here to enable the shared memory interface that
> +	  supports sharing dmabufs with VideoCore.
> +	  This operates over the VCHIQ interface to a service
> +	  running on VideoCore.
> diff --git a/drivers/platform/raspberrypi/vc-sm-cma/Makefile b/drivers/platform/raspberrypi/vc-sm-cma/Makefile
> new file mode 100644
> index 0000000000000000000000000000000000000000..c12c1a13165c18927ab03e8edde762bcb7f32c9b
> --- /dev/null
> +++ b/drivers/platform/raspberrypi/vc-sm-cma/Makefile
> @@ -0,0 +1,9 @@
> +# SPDX-License-Identifier: GPL-2.0
> +vc-sm-cma-$(CONFIG_BCM_VC_SM_CMA) := \
> +	vc_sm.o vc_sm_cma_vchi.o
> +
> +obj-$(CONFIG_BCM_VC_SM_CMA) += vc-sm-cma.o
> +
> +ccflags-y += \
> +	-D__VCCOREVER__=0

Drop

> +
> diff --git a/drivers/platform/raspberrypi/vc-sm-cma/vc_sm.c b/drivers/platform/raspberrypi/vc-sm-cma/vc_sm.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..134a8fde7ebfc1325fcd2f5cc9b81ad48a22a802
> --- /dev/null
> +++ b/drivers/platform/raspberrypi/vc-sm-cma/vc_sm.c
> @@ -0,0 +1,1619 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * VideoCore Shared Memory driver using CMA.
> + *
> + * Copyright: 2018, Raspberry Pi (Trading) Ltd
> + * Dave Stevenson <dave.stevenson@...pberrypi.org>
> + *
> + * Based on vmcs_sm driver from Broadcom Corporation for some API,
> + * and taking some code for buffer allocation and dmabuf handling from
> + * videobuf2.
> + *
> + *
> + * This driver has 3 main uses:
> + * 1) Allocating buffers for the kernel or userspace that can be shared with the
> + *    VPU.
> + * 2) Importing dmabufs from elsewhere for sharing with the VPU.
> + * 3) Allocating buffers for use by the VPU.
> + *
> + * In the first and second cases the native handle is a dmabuf. Releasing the
> + * resource inherently comes from releasing the dmabuf, and this will trigger
> + * unmapping on the VPU. The underlying allocation and our buffer structure are
> + * retained until the VPU has confirmed that it has finished with it.
> + *
> + * For the VPU allocations the VPU is responsible for triggering the release,
> + * and therefore the released message decrements the dma_buf refcount (with the
> + * VPU mapping having already been marked as released).
> + */
> +
> +/* ---- Include Files ----------------------------------------------------- */

Drop such comments.

> +#include <linux/cacheflush.h>
> +#include <linux/cdev.h>
> +#include <linux/device.h>
> +#include <linux/debugfs.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/dma-buf.h>
> +#include <linux/errno.h>
> +#include <linux/fs.h>
> +#include <linux/kernel.h>
> +#include <linux/list.h>
> +#include <linux/miscdevice.h>
> +#include <linux/module.h>
> +#include <linux/mm.h>
> +#include <linux/of_device.h>

You don't use several of these headers...

> +#include <linux/proc_fs.h>
> +#include <linux/raspberrypi/vchiq_arm.h>
> +#include <linux/raspberrypi/vchiq_bus.h>
> +#include <linux/raspberrypi/vc_sm_cma_ioctl.h>
> +#include <linux/slab.h>
> +#include <linux/seq_file.h>
> +#include <linux/syscalls.h>

Syscalls? What are you going to call here? Isn't this kernel code?

> +#include <linux/types.h>
> +
> +#include "vc_sm_cma_vchi.h"
> +
> +#include "vc_sm.h"
> +#include "vc_sm_knl.h"
> +
> +MODULE_IMPORT_NS("DMA_BUF");
> +
> +/* ---- Private Constants and Types --------------------------------------- */
> +
> +#define DEVICE_NAME		"vcsm-cma"
> +#define DEVICE_MINOR		0
> +
> +#define VC_SM_RESOURCE_NAME_DEFAULT       "sm-host-resource"
> +
> +#define VC_SM_DIR_ROOT_NAME	"vcsm-cma"
> +#define VC_SM_STATE		"state"
> +

...

> +
> +/* Driver loading. */
> +static int bcm2835_vc_sm_cma_probe(struct vchiq_device *device)
> +{
> +	int err;
> +
> +	pr_info("%s: Videocore shared memory driver\n", __func__);

Drop this and all such useless entry/exit or driver success messages.
See coding style.

Also, all drivers are supposed to use dev_xxx, not pr. This is some
really old code.

> +
> +	err = dma_set_mask_and_coherent(&device->dev, DMA_BIT_MASK(32));
> +	if (err) {
> +		dev_err(&device->dev, "dma_set_mask_and_coherent failed: %d\n",
> +			err);
> +		return err;
> +	}
> +
> +	sm_state = devm_kzalloc(&device->dev, sizeof(*sm_state), GFP_KERNEL);
> +	if (!sm_state)
> +		return -ENOMEM;
> +	sm_state->device = device;
> +	mutex_init(&sm_state->map_lock);
> +
> +	mutex_init(&sm_state->kernelid_map_lock);
> +	idr_init_base(&sm_state->kernelid_map, 1);
> +
> +	device->dev.dma_parms = devm_kzalloc(&device->dev,
> +					     sizeof(*device->dev.dma_parms),
> +					     GFP_KERNEL);
> +	/* dma_set_max_seg_size checks if dma_parms is NULL. */
> +	dma_set_max_seg_size(&device->dev, 0x3FFFFFFF);
> +
> +	vchiq_add_connected_callback(device, vc_sm_connected_init);
> +	return 0;
> +}
> +
> +/* Driver unloading. */
> +static void bcm2835_vc_sm_cma_remove(struct vchiq_device *device)
> +{
> +	pr_debug("[%s]: start\n", __func__);

No

> +	if (sm_inited) {
> +		misc_deregister(&sm_state->misc_dev);
> +
> +		/* Remove all proc entries. */
> +		debugfs_remove_recursive(sm_state->dir_root);
> +
> +		/* Stop the videocore shared memory service. */
> +		vc_sm_cma_vchi_stop(sm_state->vchiq_instance, &sm_state->sm_handle);
> +	}
> +
> +	if (sm_state) {
> +		idr_destroy(&sm_state->kernelid_map);
> +
> +		/* Free the memory for the state structure. */
> +		mutex_destroy(&sm_state->map_lock);
> +	}
> +
> +	pr_debug("[%s]: end\n", __func__);

Drop all such code. This is very poor and old coding practice. Kernel
already gives you tracing for this exact purpose.> +}
> +
> +/* Kernel API calls */
> +/* Get an internal resource handle mapped from the external one. */
> +int vc_sm_cma_int_handle(void *handle)
> +{
> +	struct dma_buf *dma_buf = (struct dma_buf *)handle;
> +	struct vc_sm_buffer *buf;
> +
> +	/* Validate we can work with this device. */
> +	if (!sm_state || !handle) {
> +		pr_err("[%s]: invalid input\n", __func__);
> +		return 0;
> +	}
> +
> +	buf = (struct vc_sm_buffer *)dma_buf->priv;
> +	return buf->vc_handle;
> +}
> +EXPORT_SYMBOL_GPL(vc_sm_cma_int_handle);
> +
> +/* Free a previously allocated shared memory handle and block. */

You need kerneldocs for every EXPORT

> +int vc_sm_cma_free(void *handle)
> +{
> +	struct dma_buf *dma_buf = (struct dma_buf *)handle;
> +
> +	/* Validate we can work with this device. */
> +	if (!sm_state || !handle) {
> +		pr_err("[%s]: invalid input\n", __func__);
> +		return -EPERM;
> +	}
> +
> +	pr_debug("%s: handle %p/dmabuf %p\n", __func__, handle, dma_buf);
> +
> +	dma_buf_put(dma_buf);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(vc_sm_cma_free);
> +
> +/* Import a dmabuf to be shared with VC. */

Kerneldocs everywhere for exports.

> +int vc_sm_cma_import_dmabuf(struct dma_buf *src_dmabuf, void **handle)
> +{
> +	struct dma_buf *new_dma_buf;
> +	int ret;
> +
> +	/* Validate we can work with this device. */
> +	if (!sm_state || !src_dmabuf || !handle) {
> +		pr_err("[%s]: invalid input\n", __func__);
> +		return -EPERM;
> +	}
> +
> +	ret = vc_sm_cma_import_dmabuf_internal(sm_state->data_knl, src_dmabuf,
> +					       -1, &new_dma_buf);
> +
> +	if (!ret) {
> +		pr_debug("%s: imported to ptr %p\n", __func__, new_dma_buf);
> +
> +		/* Assign valid handle at this time.*/
> +		*handle = new_dma_buf;
> +	} else {
> +		/*
> +		 * succeeded in importing the dma_buf, but then
> +		 * failed to look it up again. How?
> +		 * Release the fd again.
> +		 */
> +		pr_err("%s: imported vc_sm_cma_get_buffer failed %d\n",
> +		       __func__, ret);
> +	}
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(vc_sm_cma_import_dmabuf);
> +
> +static struct vchiq_driver bcm2835_vcsm_cma_driver = {
> +	.probe = bcm2835_vc_sm_cma_probe,
> +	.remove = bcm2835_vc_sm_cma_remove,
> +	.driver = {
> +		.name = DEVICE_NAME,
> +		.owner = THIS_MODULE,

Please clean up your driver from 10 year old left overs. Entire driver.

Several patterns here show that you did not clean up this driver before
submitting from basic coding style issues.

> +	},
> +};
> +
> +module_vchiq_driver(bcm2835_vcsm_cma_driver);
> +
> +MODULE_AUTHOR("Dave Stevenson");
> +MODULE_DESCRIPTION("VideoCore CMA Shared Memory Driver");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("vcsm-cma");

Why don't you have proper ID table? How this driver is supposed to be
instantiated?


Best regards,
Krzysztof

Powered by blists - more mailing lists