Re: [PATCH v2 1/4] dmaengine: add per-channel sysfs attribute groups via chan_groups

[Frank Li]

On Mon, Jul 27, 2026 at 12:48:41PM -0600, Logan Gunthorpe wrote:
> Each channel already gets its own struct device (dma_chan_dev),
> registered with device_register()/device_unregister() and torn down
> correctly by chan_dev_release(). Let drivers add their own sysfs
> attributes to that device directly, via a new dma_device.chan_groups
> field.
>
> This avoids drivers needing a second, separately-managed kobject to
> expose driver-specific attributes. That's a use-after-free issue:
> kobject_put() can invoke the ktype's release() and free the structure
> embedding dma_chan while the core (or the driver) still expects to
> reference it afterward, so avoiding it requires careful ordering and
> bookkeeping that's easy to get wrong. The channel's struct device
> already has the correct lifetime, and sysfs_create_group() supports
> the same named-subdirectory layout (attribute_group.name) that a bare
> kobject would provide.
>
> To let show()/store() callbacks safely recover the struct dma_chan from
> the struct device they're attached to without exposing dma_list_mutex
> itself to drivers, add a small dma_chan_from_dev_lock() /
> dma_chan_from_dev_unlock() pair that take / release the lock, and
> wrap them in a dma_chan_from_dev CLASS so callers can write:
>
> 	CLASS(dma_chan_from_dev, c)(dev);
> 	if (!c)
> 		return -ENODEV;
>
> Signed-off-by: Logan Gunthorpe <logang@xxxxxxxxxxxx>
> ---
>  drivers/dma/dmaengine.c   | 27 +++++++++++++++++++++++++++
>  drivers/dma/dmaengine.h   | 19 +++++++++++++++++++
>  include/linux/dmaengine.h |  4 ++++
>  3 files changed, 50 insertions(+)
>
> diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
> index 9049171df857..060a0a482eb3 100644
> --- a/drivers/dma/dmaengine.c
> +++ b/drivers/dma/dmaengine.c
> @@ -161,6 +161,32 @@ static struct dma_chan *dev_to_dma_chan(struct device *dev)
>  	return chan_dev->chan;
>  }
>
> +/**
> + * dma_chan_from_dev_lock - take dma_list_mutex and convert a channel's
> + *	struct device to its dma_chan
> + * @dev: the channel's struct device, embedded in struct dma_chan_dev
> + *
> + * Returns NULL if the channel has already been unregistered. Pairs with
> + * dma_chan_from_dev_unlock(); see the dma_chan_from_dev CLASS in
> + * drivers/dma/dmaengine.h.
> + */
> +struct dma_chan *dma_chan_from_dev_lock(struct device *dev)
> +{
> +	mutex_lock(&dma_list_mutex);
> +	return dev_to_dma_chan(dev);
> +}
> +EXPORT_SYMBOL_GPL(dma_chan_from_dev_lock);
> +
> +/**
> + * dma_chan_from_dev_unlock - release the lock taken by dma_chan_from_dev_lock()
> + * @chan: unused; matches the value produced by dma_chan_from_dev_lock()
> + */
> +void dma_chan_from_dev_unlock(struct dma_chan *chan)
> +{
> +	mutex_unlock(&dma_list_mutex);
> +}
> +EXPORT_SYMBOL_GPL(dma_chan_from_dev_unlock);
> +
>  static ssize_t memcpy_count_show(struct device *dev,
>  				 struct device_attribute *attr, char *buf)
>  {
> @@ -1098,6 +1124,7 @@ static int __dma_async_device_channel_register(struct dma_device *device,
>
>  	chan->dev->device.class = &dma_devclass;
>  	chan->dev->device.parent = device->dev;
> +	chan->dev->device.groups = device->chan_groups;

why not call device_add_groups() with const groups, so needn't save
"device->chan_groups" at all.

Frank

>  	chan->dev->chan = chan;
>  	chan->dev->dev_id = device->dev_id;
>  	if (!name)
> diff --git a/drivers/dma/dmaengine.h b/drivers/dma/dmaengine.h
> index 53f16d3f0029..224126e64d54 100644
> --- a/drivers/dma/dmaengine.h
> +++ b/drivers/dma/dmaengine.h
> @@ -182,6 +182,25 @@ dmaengine_desc_callback_valid(struct dmaengine_desc_callback *cb)
>  struct dma_chan *dma_get_slave_channel(struct dma_chan *chan);
>  struct dma_chan *dma_get_any_slave_channel(struct dma_device *device);
>
> +/*
> + * dma_chan_from_dev_lock() / dma_chan_from_dev_unlock() bracket a critical
> + * section across which a channel's struct device can be safely converted
> + * back to its struct dma_chan: dma_chan_from_dev_lock() returns NULL if the
> + * channel has already been unregistered, and the lock it takes must be held
> + * for as long as the returned channel (or anything derived from it) is
> + * accessed.
> + *
> + * Use these through the dma_chan_from_dev CLASS below rather than calling
> + * them directly.
> + */
> +struct dma_chan *dma_chan_from_dev_lock(struct device *dev);
> +void dma_chan_from_dev_unlock(struct dma_chan *chan);
> +
> +DEFINE_CLASS(dma_chan_from_dev, struct dma_chan *,
> +	     dma_chan_from_dev_unlock(_T),
> +	     dma_chan_from_dev_lock(dev),
> +	     struct device *dev)
> +
>  #ifdef CONFIG_DEBUG_FS
>  #include <linux/debugfs.h>
>
> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
> index b3d251c9734e..f4e7861b2507 100644
> --- a/include/linux/dmaengine.h
> +++ b/include/linux/dmaengine.h
> @@ -803,6 +803,9 @@ struct dma_filter {
>   * @dev: struct device reference for dma mapping api
>   * @owner: owner module (automatically set based on the provided dev)
>   * @chan_ida: unique channel ID
> + * @chan_groups: optional NULL-terminated array of sysfs attribute groups
> + *	added to each channel's struct device, for driver-specific per-channel
> + *	attributes
>   * @src_addr_widths: bit mask of src addr widths the device supports
>   *	Width is specified in bytes, e.g. for a device supporting
>   *	a width of 4 the mask should have BIT(4) set.
> @@ -886,6 +889,7 @@ struct dma_device {
>  	struct device *dev;
>  	struct module *owner;
>  	struct ida chan_ida;
> +	const struct attribute_group **chan_groups;
>
>  	u32 src_addr_widths;
>  	u32 dst_addr_widths;
> --
> 2.47.3
>