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

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

[Peter Ujfalusi]

Hi,

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

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   | 143 +++++++++++++++++++++++++++++++++++---
 include/linux/dmaengine.h |  12 +++-
 2 files changed, 144 insertions(+), 11 deletions(-)

-- 
Peter

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

[PATCH 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>
---
 drivers/dma/dmaengine.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
index 7b1cefc3213a..75516f9fbab4 100644
--- a/drivers/dma/dmaengine.c
+++ b/drivers/dma/dmaengine.c
@@ -756,22 +756,24 @@ 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);
+	if (!chan->name) {
+		dev_warn(dev,
+			 "Cannot allocate memory for slave symlink name\n");
+		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 +832,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 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   | 120 ++++++++++++++++++++++++++++++++++++++
 include/linux/dmaengine.h |  12 +++-
 2 files changed, 131 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
index 75516f9fbab4..7573a4d0f9d7 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,13 @@ 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->slave_name = kasprintf(GFP_KERNEL, "%s:%s", dev_name(dev), name);
+	if (!chan->slave_name)
+		dev_warn(dev,
+			 "Cannot allocate memory for slave name (debugfs)\n");
+#endif
+
 	chan->name = kasprintf(GFP_KERNEL, "dma:%s", name);
 	if (!chan->name) {
 		dev_warn(dev,
@@ -840,6 +848,13 @@ void dma_release_channel(struct dma_chan *chan)
 		chan->name = NULL;
 		chan->slave = NULL;
 	}
+
+#ifdef CONFIG_DEBUG_FS
+	if (chan->slave_name) {
+		kfree(chan->slave_name);
+		chan->slave_name = NULL;
+	}
+#endif
 	mutex_unlock(&dma_list_mutex);
 }
 EXPORT_SYMBOL_GPL(dma_release_channel);
@@ -1562,3 +1577,108 @@ static int __init dma_bus_init(void)
 	return class_register(&dma_devclass);
 }
 arch_initcall(dma_bus_init);
+
+#ifdef CONFIG_DEBUG_FS
+static void *dmaengine_seq_start(struct seq_file *s, loff_t *pos)
+{
+	struct dma_device *dma_dev = NULL;
+	loff_t index = *pos;
+
+	s->private = "";
+
+	mutex_lock(&dma_list_mutex);
+	list_for_each_entry(dma_dev, &dma_device_list, global_node)
+		if (index-- == 0) {
+			mutex_unlock(&dma_list_mutex);
+			return dma_dev;
+		}
+	mutex_unlock(&dma_list_mutex);
+
+	return NULL;
+}
+
+static void *dmaengine_seq_next(struct seq_file *s, void *v, loff_t *pos)
+{
+	struct dma_device *dma_dev = v;
+	void *ret = NULL;
+
+	mutex_lock(&dma_list_mutex);
+	if (list_is_last(&dma_dev->global_node, &dma_device_list))
+		ret = NULL;
+	else
+		ret = list_entry(dma_dev->global_node.next,
+				 struct dma_device, global_node);
+	mutex_unlock(&dma_list_mutex);
+
+	s->private = "\n";
+	++*pos;
+
+	return ret;
+}
+
+static void dmaengine_seq_stop(struct seq_file *s, void *v)
+{
+}
+
+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%d:", dma_dev->dev_id,
+				   chan->chan_id);
+			if (chan->slave_name)
+				seq_printf(s, "\t\t%s\n", chan->slave_name);
+			else
+				seq_printf(s, "\t\t%s\n", "in-use");
+		}
+	}
+}
+
+static int dmaengine_seq_show(struct seq_file *s, void *v)
+{
+	struct dma_device *dma_dev = v;
+
+	seq_printf(s, "%sdma%d (%s): number of channels: %u\n",
+		   (char *)s->private, 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);
+
+	return 0;
+}
+
+static const struct seq_operations dmaengine_seq_ops = {
+	.start = dmaengine_seq_start,
+	.next = dmaengine_seq_next,
+	.stop = dmaengine_seq_stop,
+	.show = dmaengine_seq_show,
+};
+
+static int dmaengine_open(struct inode *inode, struct file *file)
+{
+	return seq_open(file, &dmaengine_seq_ops);
+}
+
+static const struct file_operations dmaengine_operations = {
+	.owner		= THIS_MODULE,
+	.open		= dmaengine_open,
+	.read		= seq_read,
+	.llseek		= seq_lseek,
+	.release	= seq_release,
+};
+
+static int __init dmaengine_debugfs_init(void)
+{
+	/* /sys/kernel/debug/dmaengine */
+	debugfs_create_file("dmaengine", S_IFREG | 0444, NULL, NULL,
+			    &dmaengine_operations);
+	return 0;
+}
+subsys_initcall(dmaengine_debugfs_init);
+
+#endif	/* DEBUG_FS */
diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
index 64461fc64e1b..5b9d6b1aa6e9 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
+ * @slave_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 *slave_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 1/2] dmaengine: Cleanups for the slave <-> channel symlink support

[Geert Uytterhoeven]

Hi Peter,

On Thu, Jan 30, 2020 at 12:41 PM Peter Ujfalusi <peter.ujfalusi@ti.com> 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.
>
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>

Thanks for your patch!

> --- a/drivers/dma/dmaengine.c
> +++ b/drivers/dma/dmaengine.c
> @@ -756,22 +756,24 @@ 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);
> +       if (!chan->name) {
> +               dev_warn(dev,
> +                        "Cannot allocate memory for slave symlink name\n");

No need to print a message, as the memory allocator core will have
screamed already.

> +               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);

With the above fixed:
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Geert Uytterhoeven]

Hi Peter,

On Thu, Jan 30, 2020 at 12:41 PM Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:
> 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>

Thanks for your patch!

On Salvator-XS with R-Car H3 ES2.0:

    dma0 (ec700000.dma-controller): number of channels: 15

    dma1 (ec720000.dma-controller): number of channels: 15

    dma2 (e65a0000.dma-controller): number of channels: 2
     dma2chan0: e6590000.usb:ch0
     dma2chan1: e6590000.usb:ch1

    dma3 (e65b0000.dma-controller): number of channels: 2
     dma3chan0: e6590000.usb:ch2
     dma3chan1: e6590000.usb:ch3

    dma4 (e6460000.dma-controller): number of channels: 2
     dma4chan0: e659c000.usb:ch0
     dma4chan1: e659c000.usb:ch1

    dma5 (e6470000.dma-controller): number of channels: 2
     dma5chan0: e659c000.usb:ch2
     dma5chan1: e659c000.usb:ch3

    dma6 (e6700000.dma-controller): number of channels: 15

    dma7 (e7300000.dma-controller): number of channels: 15
     dma7chan0: e6510000.i2c:tx

    dma8 (e7310000.dma-controller): number of channels: 15
     dma8chan0: e6550000.serial:tx
     dma8chan1: e6550000.serial:rx

> --- a/drivers/dma/dmaengine.c
> +++ b/drivers/dma/dmaengine.c
> @@ -760,6 +761,13 @@ 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->slave_name = kasprintf(GFP_KERNEL, "%s:%s", dev_name(dev), name);
> +       if (!chan->slave_name)
> +               dev_warn(dev,
> +                        "Cannot allocate memory for slave name (debugfs)\n");

No need to print a message, as the memory allocation core already takes
care of that.

But, do you really need chan->slave_name?
You already have chan->slave and chan->name.

> +#endif
> +
>         chan->name = kasprintf(GFP_KERNEL, "dma:%s", name);
>         if (!chan->name) {
>                 dev_warn(dev,

> @@ -1562,3 +1577,108 @@ static int __init dma_bus_init(void)
>         return class_register(&dma_devclass);
>  }
>  arch_initcall(dma_bus_init);
> +
> +#ifdef CONFIG_DEBUG_FS
> +static void *dmaengine_seq_start(struct seq_file *s, loff_t *pos)
> +{
> +       struct dma_device *dma_dev = NULL;
> +       loff_t index = *pos;
> +
> +       s->private = "";
> +
> +       mutex_lock(&dma_list_mutex);
> +       list_for_each_entry(dma_dev, &dma_device_list, global_node)
> +               if (index-- == 0) {
> +                       mutex_unlock(&dma_list_mutex);
> +                       return dma_dev;

Can the dma_device go away after unlocking the list?
Unlike dma_request_chan(), this doesn't increase a refcnt.

> +               }
> +       mutex_unlock(&dma_list_mutex);
> +
> +       return NULL;
> +}
> +
> +static void *dmaengine_seq_next(struct seq_file *s, void *v, loff_t *pos)
> +{
> +       struct dma_device *dma_dev = v;
> +       void *ret = NULL;
> +
> +       mutex_lock(&dma_list_mutex);
> +       if (list_is_last(&dma_dev->global_node, &dma_device_list))
> +               ret = NULL;
> +       else
> +               ret = list_entry(dma_dev->global_node.next,
> +                                struct dma_device, global_node);
> +       mutex_unlock(&dma_list_mutex);

Likewise.

> +
> +       s->private = "\n";
> +       ++*pos;
> +
> +       return ret;
> +}
> +
> +static void dmaengine_seq_stop(struct seq_file *s, void *v)
> +{
> +}
> +
> +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%d:", dma_dev->dev_id,
> +                                  chan->chan_id);
> +                       if (chan->slave_name)
> +                               seq_printf(s, "\t\t%s\n", chan->slave_name);
> +                       else
> +                               seq_printf(s, "\t\t%s\n", "in-use");

The truncated ternary operator might help here:

        seq_printf(s, "\t\t%s\n", chan->slave_name ?: "in-use");

However, you might as well just use dev_name(chan->slave) and chan->name
instead of chan->slave_name.

> +               }
> +       }
> +}
> +
> +static int dmaengine_seq_show(struct seq_file *s, void *v)
> +{
> +       struct dma_device *dma_dev = v;
> +
> +       seq_printf(s, "%sdma%d (%s): number of channels: %u\n",
> +                  (char *)s->private, dma_dev->dev_id, dev_name(dma_dev->dev),
> +                  dma_dev->chancnt);
> +
> +       if (dma_dev->dbg_show)
> +               dma_dev->dbg_show(s, dma_dev);

So providing a custom .dbg_show() means replacing the standard info, not
augmenting it?

> +       else
> +               dmaengine_dbg_show(s, dma_dev);
> +
> +       return 0;
> +}

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Peter Ujfalusi]

Hi Geert,

On 30/01/2020 17.20, Geert Uytterhoeven wrote:
> Hi Peter,
> 
> On Thu, Jan 30, 2020 at 12:41 PM Peter Ujfalusi <peter.ujfalusi@ti.com> 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.
>>
>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> 
> Thanks for your patch!
> 
>> --- a/drivers/dma/dmaengine.c
>> +++ b/drivers/dma/dmaengine.c
>> @@ -756,22 +756,24 @@ 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);
>> +       if (!chan->name) {
>> +               dev_warn(dev,
>> +                        "Cannot allocate memory for slave symlink name\n");
> 
> No need to print a message, as the memory allocator core will have
> screamed already.

Right, I tend to forget this ;)

> 
>> +               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);
> 
> With the above fixed:
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Thanks,
- Péter

> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 

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

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

[Peter Ujfalusi]

Hi Geert,

On 30/01/2020 17.42, Geert Uytterhoeven wrote:
> Hi Peter,
> 
> On Thu, Jan 30, 2020 at 12:41 PM Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:
>> 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>
> 
> Thanks for your patch!
> 
> On Salvator-XS with R-Car H3 ES2.0:
> 
>     dma0 (ec700000.dma-controller): number of channels: 15
> 
>     dma1 (ec720000.dma-controller): number of channels: 15
> 
>     dma2 (e65a0000.dma-controller): number of channels: 2
>      dma2chan0: e6590000.usb:ch0
>      dma2chan1: e6590000.usb:ch1
> 
>     dma3 (e65b0000.dma-controller): number of channels: 2
>      dma3chan0: e6590000.usb:ch2
>      dma3chan1: e6590000.usb:ch3
> 
>     dma4 (e6460000.dma-controller): number of channels: 2
>      dma4chan0: e659c000.usb:ch0
>      dma4chan1: e659c000.usb:ch1
> 
>     dma5 (e6470000.dma-controller): number of channels: 2
>      dma5chan0: e659c000.usb:ch2
>      dma5chan1: e659c000.usb:ch3
> 
>     dma6 (e6700000.dma-controller): number of channels: 15
> 
>     dma7 (e7300000.dma-controller): number of channels: 15
>      dma7chan0: e6510000.i2c:tx
> 
>     dma8 (e7310000.dma-controller): number of channels: 15
>      dma8chan0: e6550000.serial:tx
>      dma8chan1: e6550000.serial:rx

You have lots of DMAs over there ;)

>> --- a/drivers/dma/dmaengine.c
>> +++ b/drivers/dma/dmaengine.c
>> @@ -760,6 +761,13 @@ 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->slave_name = kasprintf(GFP_KERNEL, "%s:%s", dev_name(dev), name);
>> +       if (!chan->slave_name)
>> +               dev_warn(dev,
>> +                        "Cannot allocate memory for slave name (debugfs)\n");
> 
> No need to print a message, as the memory allocation core already takes
> care of that.

Right.

> But, do you really need chan->slave_name?
> You already have chan->slave and chan->name.

The chan->name is prefixed with "dma:" it would not look right.
In production this all go away as debugfs most likely disabled.
But I will change the name to dbg_client_name.

> 
>> +#endif
>> +
>>         chan->name = kasprintf(GFP_KERNEL, "dma:%s", name);
>>         if (!chan->name) {
>>                 dev_warn(dev,
> 
>> @@ -1562,3 +1577,108 @@ static int __init dma_bus_init(void)
>>         return class_register(&dma_devclass);
>>  }
>>  arch_initcall(dma_bus_init);
>> +
>> +#ifdef CONFIG_DEBUG_FS
>> +static void *dmaengine_seq_start(struct seq_file *s, loff_t *pos)
>> +{
>> +       struct dma_device *dma_dev = NULL;
>> +       loff_t index = *pos;
>> +
>> +       s->private = "";
>> +
>> +       mutex_lock(&dma_list_mutex);
>> +       list_for_each_entry(dma_dev, &dma_device_list, global_node)
>> +               if (index-- == 0) {
>> +                       mutex_unlock(&dma_list_mutex);
>> +                       return dma_dev;
> 
> Can the dma_device go away after unlocking the list?
> Unlike dma_request_chan(), this doesn't increase a refcnt.

It could, let me see what I can do. Probably locking the dma_device_list
for the duration of the show.

>> +               }
>> +       mutex_unlock(&dma_list_mutex);
>> +
>> +       return NULL;
>> +}
>> +
>> +static void *dmaengine_seq_next(struct seq_file *s, void *v, loff_t *pos)
>> +{
>> +       struct dma_device *dma_dev = v;
>> +       void *ret = NULL;
>> +
>> +       mutex_lock(&dma_list_mutex);
>> +       if (list_is_last(&dma_dev->global_node, &dma_device_list))
>> +               ret = NULL;
>> +       else
>> +               ret = list_entry(dma_dev->global_node.next,
>> +                                struct dma_device, global_node);
>> +       mutex_unlock(&dma_list_mutex);
> 
> Likewise.
> 
>> +
>> +       s->private = "\n";
>> +       ++*pos;
>> +
>> +       return ret;
>> +}
>> +
>> +static void dmaengine_seq_stop(struct seq_file *s, void *v)
>> +{
>> +}
>> +
>> +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%d:", dma_dev->dev_id,
>> +                                  chan->chan_id);
>> +                       if (chan->slave_name)
>> +                               seq_printf(s, "\t\t%s\n", chan->slave_name);
>> +                       else
>> +                               seq_printf(s, "\t\t%s\n", "in-use");
> 
> The truncated ternary operator might help here:
> 
>         seq_printf(s, "\t\t%s\n", chan->slave_name ?: "in-use");
> 
> However, you might as well just use dev_name(chan->slave) and chan->name
> instead of chan->slave_name.

"2b00000.mcasp" + "dma:tx" would be an awkward combination ;)

> 
>> +               }
>> +       }
>> +}
>> +
>> +static int dmaengine_seq_show(struct seq_file *s, void *v)
>> +{
>> +       struct dma_device *dma_dev = v;
>> +
>> +       seq_printf(s, "%sdma%d (%s): number of channels: %u\n",
>> +                  (char *)s->private, dma_dev->dev_id, dev_name(dma_dev->dev),
>> +                  dma_dev->chancnt);
>> +
>> +       if (dma_dev->dbg_show)
>> +               dma_dev->dbg_show(s, dma_dev);
> 
> So providing a custom .dbg_show() means replacing the standard info, not
> augmenting it?

Correct, if a DMA driver decides to implement it, then it is it's
responsibility to show things after the
"dma%d (%s): number of channels: %u\n" line.

The standard infor is pretty minimal and not sure if it can be more verbose.
Oh, I can add the router information if it is used.

> 
>> +       else
>> +               dmaengine_dbg_show(s, dma_dev);
>> +
>> +       return 0;
>> +}
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 

- Péter

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