[v3,4/4] dmaengine: add union chan_dev for dma_chan::dev for clarity
Commit Message
From: Frank Li <Frank.Li@nxp.com>
The current dma_chan structure contains both "device" and "dev". So
chan->device->dev refers to the DMA engine device.
chan->dev->device refers to the per-channel device instance.
Their similar naming makes the distinction unclear and increases reader
confusion.
Add union dma_chan::chan_dev to make its purpose explicit and clearly
identify it as the per-channel device. After all user switch to chan_dev,
union and dma_chan::dev will be removed.
Update the kernel-doc accordingly. Besides its sysfs usage, the per-channel
device is also used by some DMA engine drivers for IOMMU mapping and
therefore deserves a more accurate description.
No functional change intended.
Reviewed-by: Logan Gunthorpe <logang@deltatee.com>
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
there are other user use it outside drivers/dma/ directory.
---
drivers/dma/dmaengine.c | 14 +++++++-------
drivers/dma/tegra186-gpc-dma.c | 2 +-
drivers/dma/ti/k3-udma.c | 4 ++--
include/linux/dmaengine.h | 18 +++++++++++++-----
4 files changed, 23 insertions(+), 15 deletions(-)
Comments
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues:
- [High] Missing put_device() on device_register() failure leads to resource leak and potential Use-After-Free.
- [High] Modifying device bus after registration circumvents the device core lifecycle and causes mismatched bus notifications.
- [High] Missing synchronize_rcu() in DMA device unregistration path leads to Use-After-Free in dma_issue_pending_all().
--
commit 99f7cae9d62138cb68f0b59f43bd77ab9e9bb860
Author: Frank Li <Frank.Li@nxp.com>
dmaengine: add union chan_dev for dma_chan::dev for clarity
The current dma_chan structure contains both "device" and "dev", which
increases reader confusion. Add union dma_chan::chan_dev to make its
purpose explicit and clearly identify it as the per-channel device.
> diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
> index a7ac2729e4d9d..bb30a9aa54d3f 100644
> --- a/drivers/dma/dmaengine.c
> +++ b/drivers/dma/dmaengine.c
[ ... ]
> @@ -1122,7 +1122,7 @@ static int __dma_async_device_channel_register(struct dma_device *device,
> err_out_ida:
> ida_free(&device->chan_ida, chan->chan_id);
> err_free_dev:
> - kfree(chan->dev);
> + kfree(chan->chan_dev);
[Severity: High]
This is a pre-existing issue, but is it correct to use kfree() directly on the
device structure here instead of put_device() after device_register() fails?
In __dma_async_device_channel_register(), if device_register() fails (e.g.
due to memory pressure or name collision), the error path jumps to
err_free_dev. Using kfree() directly circumvents the device core reference
counting.
Does this leak the device structure's associated resources, such as its
allocated name? Also, if the device's kobject was already linked into sysfs
ksets, can this lead to a Use-After-Free?
[ ... ]
> diff --git a/drivers/dma/tegra186-gpc-dma.c b/drivers/dma/tegra186-gpc-dma.c
> index b3327aa35de40..e76c58f213d84 100644
> --- a/drivers/dma/tegra186-gpc-dma.c
> +++ b/drivers/dma/tegra186-gpc-dma.c
> @@ -1554,7 +1554,7 @@ static int tegra_dma_probe(struct platform_device *pdev)
> return dev_err_probe(chdev, -EINVAL,
> "Failed to get stream ID for channel %d\n", tdc->id);
>
> - chan->dev->chan_dma_dev = true;
> + chan->chan_dev->chan_dma_dev = true;
> }
[Severity: High]
This is a pre-existing issue, but does modifying the device bus after
registration circumvent the device core lifecycle and cause mismatched bus
notifications?
During device probe in tegra_dma_probe(), the channels are registered via
dmaenginem_async_device_register(), and then chdev->bus = pdev->dev.bus
is set manually in this use_iommu_map block:
drivers/dma/tegra186-gpc-dma.c:tegra_dma_probe() {
...
if (use_iommu_map) {
chdev->bus = pdev->dev.bus;
...
}
Does bypassing bus_add_device() during initialization but later triggering
bus_remove_device() on teardown lead to missing add notifications? During
driver unbind or probe rollback, device_del() sees the non-NULL bus and blindly
triggers bus_remove_device(). Can this crash or corrupt listeners such as the
IOMMU subsystem that expect properly ordered device lifecycle notifications?
While reviewing this patch, I also noticed a pre-existing issue in
drivers/dma/dmaengine.c that isn't directly touched by this commit:
[Severity: High]
This is a pre-existing issue, but can the missing synchronize_rcu() in the
DMA device unregistration path lead to a Use-After-Free in
dma_issue_pending_all()?
When a DMA controller is unbound, dma_async_device_unregister() puts the
device reference. Then dma_device_release() unlinks the device using
list_del_rcu(&device->global_node) but fails to wait for an RCU grace period
before allowing devres to automatically free the backing memory:
drivers/dma/dmaengine.c:dma_device_release() {
...
list_del_rcu(&device->global_node);
dma_channel_rebalance();
...
}
Concurrently, the RCU reader iterates over the list in dma_issue_pending_all()
and accesses the freed device memory:
drivers/dma/dmaengine.c:dma_issue_pending_all() {
...
list_for_each_entry_rcu(device, &dma_device_list, global_node) {
if (dma_has_cap(DMA_PRIVATE, device->cap_mask))
...
}
Could concurrent RCU readers dereferencing the freed device pointer here
experience memory corruption or a kernel panic?
On Fri, Sep 04, 2026 at 05:49:46PM -0400, Frank.Li@oss.nxp.com wrote:
> The current dma_chan structure contains both "device" and "dev". So
>
> chan->device->dev refers to the DMA engine device.
> chan->dev->device refers to the per-channel device instance.
>
> Their similar naming makes the distinction unclear and increases reader
> confusion.
>
> Add union dma_chan::chan_dev to make its purpose explicit and clearly
> identify it as the per-channel device. After all user switch to chan_dev,
> union and dma_chan::dev will be removed.
>
> Update the kernel-doc accordingly. Besides its sysfs usage, the per-channel
> device is also used by some DMA engine drivers for IOMMU mapping and
> therefore deserves a more accurate description.
>
> No functional change intended.
I was almost ready to give a tag for the entire series, but found a minor
issue here...
...
> +++ b/include/linux/dmaengine.h
> struct dma_router {
> * @lock: protect between config and prepare transfer when driver have not
> * implemented callback device_prep_config_sg().
> * @chan_id: channel ID for sysfs
> - * @dev: class device for sysfs
> + * @chan_dev: class channel device for sysfs, some device use it for per-channel
> + * iommu mapping.
IOMMU
> * @name: backlink name for sysfs
> * @dbg_client_name: slave name for debugfs in format:
> * dev_name(requester's dev):channel name, for example: "2b00000.mcasp:tx"
> struct dma_chan {
>
> /* sysfs */
> int chan_id;
> - struct dma_chan_dev *dev;
> + union {
> + struct dma_chan_dev *chan_dev;
> + /*
> + * please use chan_dev, dev will be removed after all user
> + switch to chan_dev
> + */
Something went wrong with this comment style. It also need to respect English
grammar and punctuation as we do for multi-line comments.
> + struct dma_chan_dev *dev;
> + };
> const char *name;
> #ifdef CONFIG_DEBUG_FS
> char *dbg_client_name;
@@ -1083,8 +1083,8 @@ static int __dma_async_device_channel_register(struct dma_device *device,
chan->local = alloc_percpu(typeof(*chan->local));
if (!chan->local)
return -ENOMEM;
- chan->dev = kzalloc_obj(*chan->dev);
- if (!chan->dev) {
+ chan->chan_dev = kzalloc_obj(*chan->chan_dev);
+ if (!chan->chan_dev) {
rc = -ENOMEM;
goto err_free_local;
}
@@ -1103,8 +1103,8 @@ static int __dma_async_device_channel_register(struct dma_device *device,
dmaengine_chan_dev(chan)->class = &dma_devclass;
dmaengine_chan_dev(chan)->parent = device->dev;
- chan->dev->chan = chan;
- chan->dev->dev_id = device->dev_id;
+ chan->chan_dev->chan = chan;
+ chan->chan_dev->dev_id = device->dev_id;
spin_lock_init(&chan->lock);
if (!name)
@@ -1122,7 +1122,7 @@ static int __dma_async_device_channel_register(struct dma_device *device,
err_out_ida:
ida_free(&device->chan_ida, chan->chan_id);
err_free_dev:
- kfree(chan->dev);
+ kfree(chan->chan_dev);
err_free_local:
free_percpu(chan->local);
chan->local = NULL;
@@ -1155,7 +1155,7 @@ static void __dma_async_device_channel_unregister(struct dma_device *device,
__func__, chan->client_count);
mutex_lock(&dma_list_mutex);
device->chancnt--;
- chan->dev->chan = NULL;
+ chan->chan_dev->chan = NULL;
mutex_unlock(&dma_list_mutex);
ida_free(&device->chan_ida, chan->chan_id);
device_unregister(dmaengine_chan_dev(chan));
@@ -1290,7 +1290,7 @@ int dma_async_device_register(struct dma_device *device)
if (chan->local == NULL)
continue;
mutex_lock(&dma_list_mutex);
- chan->dev->chan = NULL;
+ chan->chan_dev->chan = NULL;
mutex_unlock(&dma_list_mutex);
device_unregister(dmaengine_chan_dev(chan));
free_percpu(chan->local);
@@ -1554,7 +1554,7 @@ static int tegra_dma_probe(struct platform_device *pdev)
return dev_err_probe(chdev, -EINVAL,
"Failed to get stream ID for channel %d\n", tdc->id);
- chan->dev->chan_dma_dev = true;
+ chan->chan_dev->chan_dma_dev = true;
}
/* program stream-id for this channel */
@@ -426,12 +426,12 @@ static void k3_configure_chan_coherency(struct dma_chan *chan, u32 asel)
if (asel == 0) {
/* No special handling for the channel */
- chan->dev->chan_dma_dev = false;
+ chan->chan_dev->chan_dma_dev = false;
dev_clear_dma_coherent(chan_dev);
chan_dev->dma_parms = NULL;
} else if (asel == 14 || asel == 15) {
- chan->dev->chan_dma_dev = true;
+ chan->chan_dev->chan_dma_dev = true;
dev_set_dma_coherent(chan_dev);
dma_coerce_mask_and_coherent(chan_dev, DMA_BIT_MASK(48));
@@ -325,7 +325,8 @@ struct dma_router {
* @lock: protect between config and prepare transfer when driver have not
* implemented callback device_prep_config_sg().
* @chan_id: channel ID for sysfs
- * @dev: class device for sysfs
+ * @chan_dev: class channel device for sysfs, some device use it for per-channel
+ * iommu mapping.
* @name: backlink name for sysfs
* @dbg_client_name: slave name for debugfs in format:
* dev_name(requester's dev):channel name, for example: "2b00000.mcasp:tx"
@@ -351,7 +352,14 @@ struct dma_chan {
/* sysfs */
int chan_id;
- struct dma_chan_dev *dev;
+ union {
+ struct dma_chan_dev *chan_dev;
+ /*
+ * please use chan_dev, dev will be removed after all user
+ switch to chan_dev
+ */
+ struct dma_chan_dev *dev;
+ };
const char *name;
#ifdef CONFIG_DEBUG_FS
char *dbg_client_name;
@@ -532,7 +540,7 @@ struct dma_slave_caps {
static inline const char *dma_chan_name(struct dma_chan *chan)
{
- return dev_name(&chan->dev->device);
+ return dev_name(&chan->chan_dev->device);
}
/**
@@ -1805,12 +1813,12 @@ dmaengine_get_direction_text(enum dma_transfer_direction dir)
static inline struct device *dmaengine_chan_dev(struct dma_chan *chan)
{
- return &chan->dev->device;
+ return &chan->chan_dev->device;
}
static inline struct device *dmaengine_get_dma_device(struct dma_chan *chan)
{
- if (chan->dev->chan_dma_dev)
+ if (chan->chan_dev->chan_dma_dev)
return dmaengine_chan_dev(chan);
return chan->device->dev;