Message-ID: <CAMuHMdWXuoWX=AgB=RY=5At_yw6nZJGBOK_8TXjwgYQN27JcQQ@mail.gmail.com>
Date:   Thu, 30 Jan 2020 16:42:38 +0100
From:   Geert Uytterhoeven <geert@...ux-m68k.org>
To:     Peter Ujfalusi <peter.ujfalusi@...com>
Cc:     Vinod <vkoul@...nel.org>, dmaengine@...r.kernel.org,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Dan Williams <dan.j.williams@...el.com>
Subject: Re: [PATCH 2/2] dmaengine: Add basic debugfs support

Hi Peter,

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

Powered by blists - more mailing lists