| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 31 Jan 2020 10:29:22 +0200
From: Peter Ujfalusi <peter.ujfalusi@...com>
To: Geert Uytterhoeven <geert@...ux-m68k.org>
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 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@...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
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
Powered by blists - more mailing lists