Re: [PATCH v2] dmaengine: Create symlinks between DMA channels and slaves

[Vinod Koul <vkoul@kernel.org>]

Hey Geert,

On 21-01-20, 21:22, Geert Uytterhoeven wrote:
> On Mon, Jan 20, 2020 at 1:06 PM Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:
> > On 20/01/2020 12.51, Geert Uytterhoeven wrote:
> > > On Mon, Jan 20, 2020 at 11:16 AM Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:
> > >> On 20/01/2020 11.01, Geert Uytterhoeven wrote:
> > >>> On Fri, Jan 17, 2020 at 9:08 PM Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:
> > >>>> On 1/17/20 5:30 PM, Geert Uytterhoeven wrote:
> > >>>>> Currently it is not easy to find out which DMA channels are in use, and
> > >>>>> which slave devices are using which channels.
> > >>>>>
> > >>>>> Fix this by creating two symlinks between the DMA channel and the actual
> > >>>>> slave device when a channel is requested:
> > >>>>>   1. A "slave" symlink from DMA channel to slave device,
> > >>>>
> > >>>> Have you considered similar link name as on the slave device:
> > >>>> slave:<name>
> > >>>>
> > >>>> That way it would be easier to grasp which channel is used for what
> > >>>> purpose by only looking under /sys/class/dma/ and no need to check the
> > >>>> slave device.
> > >>>
> > >>> Would this really provide more information?
> > >>> The device name is already provided in the target of the symlink:
> > >>>
> > >>> root@koelsch:~# readlink
> > >>> /sys/devices/platform/soc/e6720000.dma-controller/dma/dma1chan2/slave
> > >>> ../../../ee140000.sd
> > >>
> > >> e6720000.dma-controller/dma/dma1chan2/slave -> ../../../ee140000.sd
> > >> e6720000.dma-controller/dma/dma1chan3/slave -> ../../../ee140000.sd
> > >>
> > >> It is hard to tell which one is the tx and RX channel without looking
> > >> under the ee140000.sd:
> > >>
> > >> ee140000.sd/dma:rx -> ../e6720000.dma-controller/dma/dma1chan3
> > >> ee140000.sd/dma:tx -> ../e6720000.dma-controller/dma/dma1chan2
> > >
> > > Oh, you meant the name of the channel, not the name of the device.
> > > My mistake.
> > >
> > > As this name is a property of the slave device, not of the DMA channel,
> > > I don't think it belongs under dma*chan*.
> >
> > Right, but it gives me only half the information I need to be a link useful.
> > I know that device X is using two channels but I need to check the
> > device X's directory to know which channel is used for what purpose.

I gave the patch a spin on my board, I like some things and I dont like
few things :)

Having name of slaves is a good thing, but i had to resort to search of
channels, the controller I have has 18 channels and only 4 were in use,
so took a bit of time to find things.

Start with /sys/class/dma/ to find the controller node
then from controller node find the channels with slave
and then get to the clients!

So i would say it is better than what we have today, but we could do
better :)

> >
> > >> Another option would be to not have symlinks, but a debugfs file where
> > >> this information can be extracted and would only compiled if debugfs is
> > >> enabled.
> > >
> > > Like /proc/interrupts?
> >
> > More like /sys/kernel/debug/gpio
> >
> > > That brings the complexity of traversing all channels etc.
> >
> > Sure, but only when the file is read.
> > You can add
> > #ifdef CONFIG_DEBUG_FS
> > #endif
> >
> > around the slave_device and name in struct dma_chan {}
> >
> > and when user reads the file you print out something like this:
> > cat /sys/kernel/debug/dmaengine
> >
> > e6700000.dma-controller:
> > dma0chan0               e6e20000.spi:tx
> > dma0chan1               e6e20000.spi:rx
> > dma0chan2               ee100000.sd:tx
> > dma0chan3               ee100000.sd:rx
> > ...
> > dma0chan14              non slave
> > ...
> >
> > e6720000.dma-controller:
> > dma1chan0               e6b10000.spi:tx
> > dma1chan1               e6b10000.spi:rx
> > ...

I like the idea of adding this in debugfs and giving more info, I would
actually love to add bytes_transferred and few more info (descriptors
submitted etc) to it...

> > This way we will have all the information in one place, easy to look up
> > and you don't need to manage symlinks dynamically, just check all
> > channels if they have slave_device/name when they are in_use (in_use w/o
> > slave_device is 'non slave')
> >
> > Some drivers are requesting and releasing the DMA channel per transfer
> > or when they are opened/closed or other variations.
> >
> > > What do other people think?
> 
> Vinod: do you have some guidance for your minions? ;-)


That said, I am not against merging this patch while we add more
(debugfs)... So do my minions agree or they have better ideas :-)

-- 
~Vinod