dmaengine: pl08x: Create debugfs stats file under device folder
PATCH 1/1 dmaengine: pl08x: Create debugfs stats file under device folder
If parent folder for a debug file is not set, then this file is created right in top debugfs mountpoint directory, which is undesired. However for every DMA controller device its dedicated folder below 'dmaengine' is created by dma_async_device_register(), and this particular folder is expected to store all device specific debug files.
The new name "stats" is selected as the most popular debugfs file name found in other DMA controller device drivers, and the macro constant for file mode is replaced by the octal value.
--- drivers/dma/amba-pl08x.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/dma/amba-pl08x.c +2 -2
@@ -2500,8 +2500,8 @@ DEFINE_SHOW_ATTRIBUTE(pl08x_debugfs);static void init_pl08x_debugfs (struct pl08x_driver_data *pl08x) {/* Expose a simple debugfs interface to view all clocks */ -debugfs_create_file (dev_name (&pl08x->adev->dev),S_IFREG |S_IRUGO , -NULL , pl08x, &pl08x_debugfs_fops); +debugfs_create_file ("stats" ,0444 , pl08x->memcpy.dbg_dev_root, + pl08x, &pl08x_debugfs_fops); }#else
Signature · 1 line
2.51.0
[email protected]
AI reviewThank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] The amba-pl08x driver does not implement a .remove callback, breaking teardown symmetry on unbind and causing severe resource leaks and Use-After-Free vulnerabilities. --
--- Patch [1]: [PATCH] dmaengine: pl08x: Create debugfs stats file under device folder --- commit 178237cec2bb84665705001630a35ef76e87cfb1
Author: Vladimir Zapolskiy <[email protected]>
dmaengine: pl08x: Create debugfs stats file under device folder
This commit changes the debugfs initialization for the pl08x driver. It creates a stats file under the dedicated dmaengine device folder instead of the top-level debugfs mountpoint.
Quoted 4 lines
drivers/dma/amba-pl08x.c +0
-0
[ ... ]
drivers/dma/amba-pl08x.c DEFINE_SHOW_ATTRIBUTE(pl08x_debugfs); · line 2505
Quoted 9 lines
drivers/dma/amba-pl08x.c +2
-2@@ -2500,8 +2500,8 @@ DEFINE_SHOW_ATTRIBUTE(pl08x_debugfs);static void init_pl08x_debugfs (struct pl08x_driver_data *pl08x) {/* Expose a simple debugfs interface to view all clocks */ -debugfs_create_file (dev_name (&pl08x->adev->dev),S_IFREG |S_IRUGO , -NULL , pl08x, &pl08x_debugfs_fops); +debugfs_create_file ("stats" ,0444 , pl08x->memcpy.dbg_dev_root, + pl08x, &pl08x_debugfs_fops); }
While reviewing this patch, I noticed an issue in the amba_driver struct further down in the file:
drivers/dma/amba-pl08x.c:pl08x_amba_driver
static struct amba_driver pl08x_amba_driver = {
.drv.name = DRIVER_NAME,
.id_table = pl08x_ids,
.probe = pl08x_probe,
};[Severity: High]
This is a pre-existing issue, but does pl08x_amba_driver need a .remove callback to handle unbinding safely?
If an administrator unbinds the driver via sysfs or unloads the module, the AMBA bus detaches the device without invoking any driver-specific cleanup.
Because there is no .remove callback, wouldn't this leave non-devres resources and global subsystem registrations like dma_async_device_register active while the module is unloaded?
Could this lead to a use-after-free or kernel panic if the dmaengine core accesses the leaked device from the global list, or if the hardware asserts an interrupt after the module is unloaded?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
Quoted 6 lines · Vladimir Zapolskiy
If parent folder for a debug file is not set, then this file is created right in top debugfs mountpoint directory, which is undesired. However for every DMA controller device its dedicated folder below 'dmaengine' is created by
dma_async_device_register(), and this particular folder is expected to store all device specific debug files.
Quoted 6 lines
The new name "stats" is selected as the most popular debugfs file name found in other DMA controller device drivers, and the macro constant for file mode is replaced by the octal value.
Signed-off-by Vladimir Zapolskiy <[email protected]>---
Quoted 21 lines
drivers/dma/amba-pl08x.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)drivers/dma/amba-pl08x.c +2
-3@@ -2500,8 +2500,8 @@ DEFINE_SHOW_ATTRIBUTE(pl08x_debugfs);static void init_pl08x_debugfs (struct pl08x_driver_data *pl08x) {/* Expose a simple debugfs interface to view all clocks */ -debugfs_create_file (dev_name (&pl08x->adev->dev),S_IFREG |S_IRUGO , -NULL , pl08x, &pl08x_debugfs_fops); +debugfs_create_file ("stats" ,0444 , pl08x->memcpy.dbg_dev_root, + pl08x, &pl08x_debugfs_fops); }#else --2.51.0
Quoted 11 lines · Vladimir Zapolskiy
If parent folder for a debug file is not set, then this file is created right in top debugfs mountpoint directory, which is undesired. However for every DMA controller device its dedicated folder below 'dmaengine' is created by
dma_async_device_register(), and this particular folder is expected to store all device specific debug files.The new name "stats" is selected as the most popular debugfs file name found in other DMA controller device drivers, and the macro constant for file mode is replaced by the octal value.
[...]
Applied, thanks!
[1/1] dmaengine: pl08x: Create debugfs stats file under device folder
commit: 68e26bc0b7b26c614639f1d13d160c51ff81799f
Best regards,
Signature · 1 line
~Vinod