[PATCH v2 0/2] dmaengine: Cleanups for symlink handling and debugfs support

[PATCH v2 0/2] dmaengine: Cleanups for symlink handling and debugfs support

[Peter Ujfalusi]

Hi,

Changes since v1:
- Removed dev_warn() for kasprintf in both patch
- Added Reviewed-by from Geert to the first patch
- Use much more simplified fops for the debugfs file (via DEFINE_SHOW_ATTRIBUTE)
- do not allow modification to dma_device_list while the debugfs file is read
- rename the slave_name to dbg_client_name (it is only for debugging)
- print information about dma_router if it is used by the channel
- Formating of the output slightly changed

As I have mentioned on the symlink patch earlier I like how the gpio's debugfs
shows in one place information.

These patches are on top of Vinod's next (with the v2 fix for the symlink
support).

The first patch fixes and cleans up the symlink handling code a bit and the
second adds support for debugfs file:

On my board with audio and after a run with dmatest on 6 channels this is how
the information is presented about the DMA drivers:

# cat /sys/kernel/debug/dmaengine 
dma0 (285c0000.dma-controller): number of channels: 96

dma1 (31150000.dma-controller): number of channels: 267
 dma1chan0   | 2b00000.mcasp:tx
 dma1chan1   | 2b00000.mcasp:rx
 dma1chan2   | in-use
 dma1chan3   | in-use
 dma1chan4   | in-use
 dma1chan5   | in-use
 dma1chan6   | in-use
 dma1chan7   | in-use

On dra7-evm after boot:
# cat /sys/kernel/debug/dmaengine 
dma0 (43300000.edma): number of channels: 64
 dma0chan0   | 48468000.mcasp:tx (via router: 4a002c78.dma-router)
 dma0chan1   | 48468000.mcasp:rx (via router: 4a002c78.dma-router)

dma1 (4a056000.dma-controller): number of channels: 127
 dma1chan0   | in-use
 dma1chan1   | in-use

It shows the users (device name + channel name) of the channels. If it is not a
slave channel, then it only prints 'in-use' as no other information is
available for non save channels.

DMA drivers can implement the dbg_show callback to provide custom information
for their channels if needed.

Regards,
Peter
---
Peter Ujfalusi (2):
  dmaengine: Cleanups for the slave <-> channel symlink support
  dmaengine: Add basic debugfs support

 drivers/dma/dmaengine.c   | 84 ++++++++++++++++++++++++++++++++++-----
 include/linux/dmaengine.h | 12 +++++-
 2 files changed, 86 insertions(+), 10 deletions(-)

-- 
Peter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[PATCH v2 1/2] dmaengine: Cleanups for the slave <-> channel symlink support

[Peter Ujfalusi]

No need to use goto to jump over the
return chan ? chan : ERR_PTR(-EPROBE_DEFER);
We can just revert the check and return right there.

Do not fail the channel request if the chan->name allocation fails, but
print a warning about it.

Change the dev_err to dev_warn if sysfs_create_link() fails as it is not
fatal.

Only attempt to remove the DMA_SLAVE_NAME symlink if it is created - or it
was attempted to be created.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/dma/dmaengine.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
index 7b1cefc3213a..342d23132fca 100644
--- a/drivers/dma/dmaengine.c
+++ b/drivers/dma/dmaengine.c
@@ -756,22 +756,21 @@ struct dma_chan *dma_request_chan(struct device *dev, const char *name)
 	}
 	mutex_unlock(&dma_list_mutex);
 
-	if (!IS_ERR_OR_NULL(chan))
-		goto found;
-
-	return chan ? chan : ERR_PTR(-EPROBE_DEFER);
+	if (IS_ERR_OR_NULL(chan))
+		return chan ? chan : ERR_PTR(-EPROBE_DEFER);
 
 found:
-	chan->slave = dev;
 	chan->name = kasprintf(GFP_KERNEL, "dma:%s", name);
 	if (!chan->name)
-		return ERR_PTR(-ENOMEM);
+		return chan;
+	chan->slave = dev;
 
 	if (sysfs_create_link(&chan->dev->device.kobj, &dev->kobj,
 			      DMA_SLAVE_NAME))
-		dev_err(dev, "Cannot create DMA %s symlink\n", DMA_SLAVE_NAME);
+		dev_warn(dev, "Cannot create DMA %s symlink\n", DMA_SLAVE_NAME);
 	if (sysfs_create_link(&dev->kobj, &chan->dev->device.kobj, chan->name))
-		dev_err(dev, "Cannot create DMA %s symlink\n", chan->name);
+		dev_warn(dev, "Cannot create DMA %s symlink\n", chan->name);
+
 	return chan;
 }
 EXPORT_SYMBOL_GPL(dma_request_chan);
@@ -830,13 +829,14 @@ void dma_release_channel(struct dma_chan *chan)
 	/* drop PRIVATE cap enabled by __dma_request_channel() */
 	if (--chan->device->privatecnt == 0)
 		dma_cap_clear(DMA_PRIVATE, chan->device->cap_mask);
+
 	if (chan->slave) {
+		sysfs_remove_link(&chan->dev->device.kobj, DMA_SLAVE_NAME);
 		sysfs_remove_link(&chan->slave->kobj, chan->name);
 		kfree(chan->name);
 		chan->name = NULL;
 		chan->slave = NULL;
 	}
-	sysfs_remove_link(&chan->dev->device.kobj, DMA_SLAVE_NAME);
 	mutex_unlock(&dma_list_mutex);
 }
 EXPORT_SYMBOL_GPL(dma_release_channel);
-- 
Peter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[PATCH v2 2/2] dmaengine: Add basic debugfs support

[Peter Ujfalusi]

Via the /sys/kernel/debug/dmaengine users can get information about the
DMA devices and the used channels.

Example output on am654-evm with audio using two channels and after running
dmatest on 6 channels:

 # cat /sys/kernel/debug/dmaengine
dma0 (285c0000.dma-controller): number of channels: 96

dma1 (31150000.dma-controller): number of channels: 267
 dma1chan0:             2b00000.mcasp:tx
 dma1chan1:             2b00000.mcasp:rx
 dma1chan2:             in-use
 dma1chan3:             in-use
 dma1chan4:             in-use
 dma1chan5:             in-use
 dma1chan6:             in-use
 dma1chan7:             in-use

For slave channels we can show the device and the channel name a given
channel is requested.
For non slave devices the only information we know is that the channel is
in use.

DMA drivers can implement the optional dbg_show callback to provide
controller specific information instead of the generic one.

It is easy to extend the generic dmaengine_dbg_show() to print additional
information about the used channels.

I have taken the idea from gpiolib.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 drivers/dma/dmaengine.c   | 66 +++++++++++++++++++++++++++++++++++++++
 include/linux/dmaengine.h | 12 ++++++-
 2 files changed, 77 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
index 342d23132fca..121231300d35 100644
--- a/drivers/dma/dmaengine.c
+++ b/drivers/dma/dmaengine.c
@@ -32,6 +32,7 @@
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
 #include <linux/platform_device.h>
+#include <linux/debugfs.h>
 #include <linux/dma-mapping.h>
 #include <linux/init.h>
 #include <linux/module.h>
@@ -760,6 +761,11 @@ struct dma_chan *dma_request_chan(struct device *dev, const char *name)
 		return chan ? chan : ERR_PTR(-EPROBE_DEFER);
 
 found:
+#ifdef CONFIG_DEBUG_FS
+	chan->dbg_client_name = kasprintf(GFP_KERNEL, "%s:%s", dev_name(dev),
+					  name);
+#endif
+
 	chan->name = kasprintf(GFP_KERNEL, "dma:%s", name);
 	if (!chan->name)
 		return chan;
@@ -837,6 +843,11 @@ void dma_release_channel(struct dma_chan *chan)
 		chan->name = NULL;
 		chan->slave = NULL;
 	}
+
+#ifdef CONFIG_DEBUG_FS
+	kfree(chan->dbg_client_name);
+	chan->dbg_client_name = NULL;
+#endif
 	mutex_unlock(&dma_list_mutex);
 }
 EXPORT_SYMBOL_GPL(dma_release_channel);
@@ -1559,3 +1570,58 @@ static int __init dma_bus_init(void)
 	return class_register(&dma_devclass);
 }
 arch_initcall(dma_bus_init);
+
+#ifdef CONFIG_DEBUG_FS
+static void dmaengine_dbg_show(struct seq_file *s, struct dma_device *dma_dev)
+{
+	struct dma_chan *chan;
+
+	list_for_each_entry(chan, &dma_dev->channels, device_node) {
+		if (chan->client_count) {
+			seq_printf(s, " dma%dchan%-4d| %s",
+				   dma_dev->dev_id, chan->chan_id,
+				   chan->dbg_client_name ?: "in-use");
+
+			if (chan->router)
+				seq_printf(s, " (via router: %s)\n",
+					dev_name(chan->router->dev));
+			else
+				seq_puts(s, "\n");
+		}
+	}
+}
+
+static int dmaengine_debugfs_show(struct seq_file *s, void *data)
+{
+	struct dma_device *dma_dev = NULL;
+
+	mutex_lock(&dma_list_mutex);
+	list_for_each_entry(dma_dev, &dma_device_list, global_node) {
+		seq_printf(s, "dma%d (%s): number of channels: %u\n",
+			   dma_dev->dev_id, dev_name(dma_dev->dev),
+			   dma_dev->chancnt);
+
+		if (dma_dev->dbg_show)
+			dma_dev->dbg_show(s, dma_dev);
+		else
+			dmaengine_dbg_show(s, dma_dev);
+
+		if (!list_is_last(&dma_dev->global_node, &dma_device_list))
+			seq_puts(s, "\n");
+	}
+	mutex_unlock(&dma_list_mutex);
+
+	return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(dmaengine_debugfs);
+
+static int __init dmaengine_debugfs_init(void)
+{
+	/* /sys/kernel/debug/dmaengine */
+	debugfs_create_file("dmaengine", 0444, NULL, NULL,
+			    &dmaengine_debugfs_fops);
+	return 0;
+}
+late_initcall(dmaengine_debugfs_init);
+
+#endif	/* DEBUG_FS */
diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
index 64461fc64e1b..9f232b7618f1 100644
--- a/include/linux/dmaengine.h
+++ b/include/linux/dmaengine.h
@@ -300,6 +300,8 @@ struct dma_router {
  * @chan_id: channel ID for sysfs
  * @dev: class device for sysfs
  * @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"
  * @device_node: used to add this to the device chan list
  * @local: per-cpu pointer to a struct dma_chan_percpu
  * @client_count: how many clients are using this channel
@@ -318,6 +320,9 @@ struct dma_chan {
 	int chan_id;
 	struct dma_chan_dev *dev;
 	const char *name;
+#ifdef CONFIG_DEBUG_FS
+	char *dbg_client_name;
+#endif
 
 	struct list_head device_node;
 	struct dma_chan_percpu __percpu *local;
@@ -805,7 +810,9 @@ struct dma_filter {
  *     called and there are no further references to this structure. This
  *     must be implemented to free resources however many existing drivers
  *     do not and are therefore not safe to unbind while in use.
- *
+ * @dbg_show: optional routine to show contents in debugfs; default code
+ *     will be used when this is omitted, but custom code can show extra,
+ *     controller specific information.
  */
 struct dma_device {
 	struct kref ref;
@@ -891,6 +898,9 @@ struct dma_device {
 					    struct dma_tx_state *txstate);
 	void (*device_issue_pending)(struct dma_chan *chan);
 	void (*device_release)(struct dma_device *dev);
+#ifdef CONFIG_DEBUG_FS
+	void (*dbg_show)(struct seq_file *s, struct dma_device *dev);
+#endif
 };
 
 static inline int dmaengine_slave_config(struct dma_chan *chan,
-- 
Peter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH v2 1/2] dmaengine: Cleanups for the slave <-> channel symlink support

[Vinod Koul]

On 31-01-20, 11:38, Peter Ujfalusi wrote:
> No need to use goto to jump over the
> return chan ? chan : ERR_PTR(-EPROBE_DEFER);
> We can just revert the check and return right there.
> 
> Do not fail the channel request if the chan->name allocation fails, but
> print a warning about it.
> 
> Change the dev_err to dev_warn if sysfs_create_link() fails as it is not
> fatal.
> 
> Only attempt to remove the DMA_SLAVE_NAME symlink if it is created - or it
> was attempted to be created.

Applied, thanks

-- 
~Vinod