| Message ID | 20200117153056.31363-1-geert+renesas@glider.be (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v2] dmaengine: Create symlinks between DMA channels and slaves | expand |
Hi Geert, Thanks for your patch. On 2020-01-17 16:30:56 +0100, 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, > 2. A "dma:<name>" symlink slave device to DMA channel. > When the channel is released, the symlinks are removed again. > The latter requires keeping track of the slave device and the channel > name in the dma_chan structure. > > Note that this is limited to channel request functions for requesting an > exclusive slave channel that take a device pointer (dma_request_chan() > and dma_request_slave_channel*()). > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> I like this change, Tested-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > v2: > - Add DMA_SLAVE_NAME macro, > - Also handle channels from FIXME, > - Add backlinks from slave device to DMA channel, > > On r8a7791/koelsch, the following new symlinks are created: > > /sys/devices/platform/soc/ > ├── e6700000.dma-controller/dma/dma0chan0/slave -> ../../../e6e20000.spi > ├── e6700000.dma-controller/dma/dma0chan1/slave -> ../../../e6e20000.spi > ├── e6700000.dma-controller/dma/dma0chan2/slave -> ../../../ee100000.sd > ├── e6700000.dma-controller/dma/dma0chan3/slave -> ../../../ee100000.sd > ├── e6700000.dma-controller/dma/dma0chan4/slave -> ../../../ee160000.sd > ├── e6700000.dma-controller/dma/dma0chan5/slave -> ../../../ee160000.sd > ├── e6700000.dma-controller/dma/dma0chan6/slave -> ../../../e6e68000.serial > ├── e6700000.dma-controller/dma/dma0chan7/slave -> ../../../e6e68000.serial > ├── e6720000.dma-controller/dma/dma1chan0/slave -> ../../../e6b10000.spi > ├── e6720000.dma-controller/dma/dma1chan1/slave -> ../../../e6b10000.spi > ├── e6720000.dma-controller/dma/dma1chan2/slave -> ../../../ee140000.sd > ├── e6720000.dma-controller/dma/dma1chan3/slave -> ../../../ee140000.sd > ├── e6b10000.spi/dma:rx -> ../e6720000.dma-controller/dma/dma1chan1 > ├── e6b10000.spi/dma:tx -> ../e6720000.dma-controller/dma/dma1chan0 > ├── e6e20000.spi/dma:rx -> ../e6700000.dma-controller/dma/dma0chan1 > ├── e6e20000.spi/dma:tx -> ../e6700000.dma-controller/dma/dma0chan0 > ├── e6e68000.serial/dma:rx -> ../e6700000.dma-controller/dma/dma0chan7 > ├── e6e68000.serial/dma:tx -> ../e6700000.dma-controller/dma/dma0chan6 > ├── ee100000.sd/dma:rx -> ../e6700000.dma-controller/dma/dma0chan3 > ├── ee100000.sd/dma:tx -> ../e6700000.dma-controller/dma/dma0chan2 > ├── ee140000.sd/dma:rx -> ../e6720000.dma-controller/dma/dma1chan3 > ├── ee140000.sd/dma:tx -> ../e6720000.dma-controller/dma/dma1chan2 > ├── ee160000.sd/dma:rx -> ../e6700000.dma-controller/dma/dma0chan5 > └── ee160000.sd/dma:tx -> ../e6700000.dma-controller/dma/dma0chan4 > > On r8a77951/salvator-xs: > > /sys/devices/platform/soc/ > ├── e6460000.dma-controller/dma/dma4chan0/slave -> ../../../e659c000.usb > ├── e6460000.dma-controller/dma/dma4chan1/slave -> ../../../e659c000.usb > ├── e6470000.dma-controller/dma/dma5chan0/slave -> ../../../e659c000.usb > ├── e6470000.dma-controller/dma/dma5chan1/slave -> ../../../e659c000.usb > ├── e6510000.i2c/dma:tx -> ../e7300000.dma-controller/dma/dma7chan0 > ├── e6550000.serial/dma:rx -> ../e7310000.dma-controller/dma/dma8chan1 > ├── e6550000.serial/dma:tx -> ../e7310000.dma-controller/dma/dma8chan0 > ├── e6590000.usb/dma:ch0 -> ../e65a0000.dma-controller/dma/dma2chan0 > ├── e6590000.usb/dma:ch1 -> ../e65a0000.dma-controller/dma/dma2chan1 > ├── e6590000.usb/dma:ch2 -> ../e65b0000.dma-controller/dma/dma3chan0 > ├── e6590000.usb/dma:ch3 -> ../e65b0000.dma-controller/dma/dma3chan1 > ├── e659c000.usb/dma:ch0 -> ../e6460000.dma-controller/dma/dma4chan0 > ├── e659c000.usb/dma:ch1 -> ../e6460000.dma-controller/dma/dma4chan1 > ├── e659c000.usb/dma:ch2 -> ../e6470000.dma-controller/dma/dma5chan0 > ├── e659c000.usb/dma:ch3 -> ../e6470000.dma-controller/dma/dma5chan1 > ├── e65a0000.dma-controller/dma/dma2chan0/slave -> ../../../e6590000.usb > ├── e65a0000.dma-controller/dma/dma2chan1/slave -> ../../../e6590000.usb > ├── e65b0000.dma-controller/dma/dma3chan0/slave -> ../../../e6590000.usb > ├── e65b0000.dma-controller/dma/dma3chan1/slave -> ../../../e6590000.usb > ├── e7300000.dma-controller/dma/dma7chan0/slave -> ../../../e6510000.i2c > ├── e7310000.dma-controller/dma/dma8chan0/slave -> ../../../e6550000.serial > └── e7310000.dma-controller/dma/dma8chan1/slave -> ../../../e6550000.serial > --- > drivers/dma/dmaengine.c | 37 +++++++++++++++++++++++++++++++------ > include/linux/dmaengine.h | 4 ++++ > 2 files changed, 35 insertions(+), 6 deletions(-) > > diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c > index 56a8420c388679d3..617c84cf6800962b 100644 > --- a/drivers/dma/dmaengine.c > +++ b/drivers/dma/dmaengine.c > @@ -60,6 +60,8 @@ static long dmaengine_ref_count; > > /* --- sysfs implementation --- */ > > +#define DMA_SLAVE_NAME "slave" > + > /** > * dev_to_dma_chan - convert a device pointer to its sysfs container object > * @dev - device node > @@ -730,11 +732,11 @@ struct dma_chan *dma_request_chan(struct device *dev, const char *name) > if (has_acpi_companion(dev) && !chan) > chan = acpi_dma_request_slave_chan_by_name(dev, name); > > - if (chan) { > - /* Valid channel found or requester needs to be deferred */ > - if (!IS_ERR(chan) || PTR_ERR(chan) == -EPROBE_DEFER) > - return chan; > - } > + if (PTR_ERR(chan) == -EPROBE_DEFER) > + return chan; > + > + if (!IS_ERR_OR_NULL(chan)) > + goto found; > > /* Try to find the channel via the DMA filter map(s) */ > mutex_lock(&dma_list_mutex); > @@ -754,7 +756,23 @@ struct dma_chan *dma_request_chan(struct device *dev, const char *name) > } > mutex_unlock(&dma_list_mutex); > > - return chan ? chan : ERR_PTR(-EPROBE_DEFER); > + if (!IS_ERR_OR_NULL(chan)) > + goto found; > + > + return ERR_PTR(-EPROBE_DEFER); > + > +found: > + chan->slave = dev; > + chan->name = kasprintf(GFP_KERNEL, "dma:%s", name); > + if (!chan->name) > + return ERR_PTR(-ENOMEM); > + > + 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); > + if (sysfs_create_link(&dev->kobj, &chan->dev->device.kobj, chan->name)) > + dev_err(dev, "Cannot create DMA %s symlink\n", chan->name); > + return chan; > } > EXPORT_SYMBOL_GPL(dma_request_chan); > > @@ -812,6 +830,13 @@ 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->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); > diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h > index 2cd1d6d7ef0fcce5..2804da93e27e114b 100644 > --- a/include/linux/dmaengine.h > +++ b/include/linux/dmaengine.h > @@ -238,10 +238,12 @@ struct dma_router { > /** > * struct dma_chan - devices supply DMA channels, clients use them > * @device: ptr to the dma device who supplies this channel, always !%NULL > + * @slave: ptr to the device using this channel > * @cookie: last cookie value returned to client > * @completed_cookie: last completed cookie for this channel > * @chan_id: channel ID for sysfs > * @dev: class device for sysfs > + * @name: backlink name for sysfs > * @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 > @@ -252,12 +254,14 @@ struct dma_router { > */ > struct dma_chan { > struct dma_device *device; > + struct device *slave; > dma_cookie_t cookie; > dma_cookie_t completed_cookie; > > /* sysfs */ > int chan_id; > struct dma_chan_dev *dev; > + const char *name; > > struct list_head device_node; > struct dma_chan_percpu __percpu *local; > -- > 2.17.1 >
Hi Geert, 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. > 2. A "dma:<name>" symlink slave device to DMA channel. > When the channel is released, the symlinks are removed again. > The latter requires keeping track of the slave device and the channel > name in the dma_chan structure. > > Note that this is limited to channel request functions for requesting an > exclusive slave channel that take a device pointer (dma_request_chan() > and dma_request_slave_channel*()). > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > --- > v2: > - Add DMA_SLAVE_NAME macro, > - Also handle channels from FIXME, > - Add backlinks from slave device to DMA channel, > > On r8a7791/koelsch, the following new symlinks are created: > > /sys/devices/platform/soc/ > ├── e6700000.dma-controller/dma/dma0chan0/slave -> ../../../e6e20000.spi > ├── e6700000.dma-controller/dma/dma0chan1/slave -> ../../../e6e20000.spi > ├── e6700000.dma-controller/dma/dma0chan2/slave -> ../../../ee100000.sd > ├── e6700000.dma-controller/dma/dma0chan3/slave -> ../../../ee100000.sd > ├── e6700000.dma-controller/dma/dma0chan4/slave -> ../../../ee160000.sd > ├── e6700000.dma-controller/dma/dma0chan5/slave -> ../../../ee160000.sd > ├── e6700000.dma-controller/dma/dma0chan6/slave -> ../../../e6e68000.serial > ├── e6700000.dma-controller/dma/dma0chan7/slave -> ../../../e6e68000.serial > ├── e6720000.dma-controller/dma/dma1chan0/slave -> ../../../e6b10000.spi > ├── e6720000.dma-controller/dma/dma1chan1/slave -> ../../../e6b10000.spi > ├── e6720000.dma-controller/dma/dma1chan2/slave -> ../../../ee140000.sd > ├── e6720000.dma-controller/dma/dma1chan3/slave -> ../../../ee140000.sd > ├── e6b10000.spi/dma:rx -> ../e6720000.dma-controller/dma/dma1chan1 > ├── e6b10000.spi/dma:tx -> ../e6720000.dma-controller/dma/dma1chan0 > ├── e6e20000.spi/dma:rx -> ../e6700000.dma-controller/dma/dma0chan1 > ├── e6e20000.spi/dma:tx -> ../e6700000.dma-controller/dma/dma0chan0 > ├── e6e68000.serial/dma:rx -> ../e6700000.dma-controller/dma/dma0chan7 > ├── e6e68000.serial/dma:tx -> ../e6700000.dma-controller/dma/dma0chan6 > ├── ee100000.sd/dma:rx -> ../e6700000.dma-controller/dma/dma0chan3 > ├── ee100000.sd/dma:tx -> ../e6700000.dma-controller/dma/dma0chan2 > ├── ee140000.sd/dma:rx -> ../e6720000.dma-controller/dma/dma1chan3 > ├── ee140000.sd/dma:tx -> ../e6720000.dma-controller/dma/dma1chan2 > ├── ee160000.sd/dma:rx -> ../e6700000.dma-controller/dma/dma0chan5 > └── ee160000.sd/dma:tx -> ../e6700000.dma-controller/dma/dma0chan4 > > On r8a77951/salvator-xs: > > /sys/devices/platform/soc/ > ├── e6460000.dma-controller/dma/dma4chan0/slave -> ../../../e659c000.usb > ├── e6460000.dma-controller/dma/dma4chan1/slave -> ../../../e659c000.usb > ├── e6470000.dma-controller/dma/dma5chan0/slave -> ../../../e659c000.usb > ├── e6470000.dma-controller/dma/dma5chan1/slave -> ../../../e659c000.usb > ├── e6510000.i2c/dma:tx -> ../e7300000.dma-controller/dma/dma7chan0 > ├── e6550000.serial/dma:rx -> ../e7310000.dma-controller/dma/dma8chan1 > ├── e6550000.serial/dma:tx -> ../e7310000.dma-controller/dma/dma8chan0 > ├── e6590000.usb/dma:ch0 -> ../e65a0000.dma-controller/dma/dma2chan0 > ├── e6590000.usb/dma:ch1 -> ../e65a0000.dma-controller/dma/dma2chan1 > ├── e6590000.usb/dma:ch2 -> ../e65b0000.dma-controller/dma/dma3chan0 > ├── e6590000.usb/dma:ch3 -> ../e65b0000.dma-controller/dma/dma3chan1 > ├── e659c000.usb/dma:ch0 -> ../e6460000.dma-controller/dma/dma4chan0 > ├── e659c000.usb/dma:ch1 -> ../e6460000.dma-controller/dma/dma4chan1 > ├── e659c000.usb/dma:ch2 -> ../e6470000.dma-controller/dma/dma5chan0 > ├── e659c000.usb/dma:ch3 -> ../e6470000.dma-controller/dma/dma5chan1 > ├── e65a0000.dma-controller/dma/dma2chan0/slave -> ../../../e6590000.usb > ├── e65a0000.dma-controller/dma/dma2chan1/slave -> ../../../e6590000.usb > ├── e65b0000.dma-controller/dma/dma3chan0/slave -> ../../../e6590000.usb > ├── e65b0000.dma-controller/dma/dma3chan1/slave -> ../../../e6590000.usb > ├── e7300000.dma-controller/dma/dma7chan0/slave -> ../../../e6510000.i2c > ├── e7310000.dma-controller/dma/dma8chan0/slave -> ../../../e6550000.serial > └── e7310000.dma-controller/dma/dma8chan1/slave -> ../../../e6550000.serial > --- > drivers/dma/dmaengine.c | 37 +++++++++++++++++++++++++++++++------ > include/linux/dmaengine.h | 4 ++++ > 2 files changed, 35 insertions(+), 6 deletions(-) > > diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c > index 56a8420c388679d3..617c84cf6800962b 100644 > --- a/drivers/dma/dmaengine.c > +++ b/drivers/dma/dmaengine.c > @@ -60,6 +60,8 @@ static long dmaengine_ref_count; > > /* --- sysfs implementation --- */ > > +#define DMA_SLAVE_NAME "slave" > + > /** > * dev_to_dma_chan - convert a device pointer to its sysfs container object > * @dev - device node > @@ -730,11 +732,11 @@ struct dma_chan *dma_request_chan(struct device *dev, const char *name) > if (has_acpi_companion(dev) && !chan) > chan = acpi_dma_request_slave_chan_by_name(dev, name); > > - if (chan) { > - /* Valid channel found or requester needs to be deferred */ > - if (!IS_ERR(chan) || PTR_ERR(chan) == -EPROBE_DEFER) > - return chan; > - } > + if (PTR_ERR(chan) == -EPROBE_DEFER) > + return chan; > + > + if (!IS_ERR_OR_NULL(chan)) > + goto found; > > /* Try to find the channel via the DMA filter map(s) */ > mutex_lock(&dma_list_mutex); > @@ -754,7 +756,23 @@ struct dma_chan *dma_request_chan(struct device *dev, const char *name) > } > mutex_unlock(&dma_list_mutex); > > - return chan ? chan : ERR_PTR(-EPROBE_DEFER); > + if (!IS_ERR_OR_NULL(chan)) > + goto found; > + > + return ERR_PTR(-EPROBE_DEFER); > + > +found: > + chan->slave = dev; > + chan->name = kasprintf(GFP_KERNEL, "dma:%s", name); > + if (!chan->name) > + return ERR_PTR(-ENOMEM); > + > + 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); > + if (sysfs_create_link(&dev->kobj, &chan->dev->device.kobj, chan->name)) > + dev_err(dev, "Cannot create DMA %s symlink\n", chan->name); > + return chan; > } > EXPORT_SYMBOL_GPL(dma_request_chan); > > @@ -812,6 +830,13 @@ 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->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); > diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h > index 2cd1d6d7ef0fcce5..2804da93e27e114b 100644 > --- a/include/linux/dmaengine.h > +++ b/include/linux/dmaengine.h > @@ -238,10 +238,12 @@ struct dma_router { > /** > * struct dma_chan - devices supply DMA channels, clients use them > * @device: ptr to the dma device who supplies this channel, always !%NULL > + * @slave: ptr to the device using this channel > * @cookie: last cookie value returned to client > * @completed_cookie: last completed cookie for this channel > * @chan_id: channel ID for sysfs > * @dev: class device for sysfs > + * @name: backlink name for sysfs > * @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 > @@ -252,12 +254,14 @@ struct dma_router { > */ > struct dma_chan { > struct dma_device *device; > + struct device *slave; > dma_cookie_t cookie; > dma_cookie_t completed_cookie; > > /* sysfs */ > int chan_id; > struct dma_chan_dev *dev; > + const char *name; > > struct list_head device_node; > struct dma_chan_percpu __percpu *local; > - Peter Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Hi Peter, 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 > > 2. A "dma:<name>" symlink slave device to DMA channel. > > When the channel is released, the symlinks are removed again. > > The latter requires keeping track of the slave device and the channel > > name in the dma_chan structure. > > > > Note that this is limited to channel request functions for requesting an > > exclusive slave channel that take a device pointer (dma_request_chan() > > and dma_request_slave_channel*()). > > > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > > --- > > v2: > > - Add DMA_SLAVE_NAME macro, > > - Also handle channels from FIXME, > > - Add backlinks from slave device to DMA channel, > > > > On r8a7791/koelsch, the following new symlinks are created: > > > > /sys/devices/platform/soc/ > > ├── e6700000.dma-controller/dma/dma0chan0/slave -> ../../../e6e20000.spi > > ├── e6700000.dma-controller/dma/dma0chan1/slave -> ../../../e6e20000.spi > > ├── e6700000.dma-controller/dma/dma0chan2/slave -> ../../../ee100000.sd > > ├── e6700000.dma-controller/dma/dma0chan3/slave -> ../../../ee100000.sd > > ├── e6700000.dma-controller/dma/dma0chan4/slave -> ../../../ee160000.sd > > ├── e6700000.dma-controller/dma/dma0chan5/slave -> ../../../ee160000.sd > > ├── e6700000.dma-controller/dma/dma0chan6/slave -> ../../../e6e68000.serial > > ├── e6700000.dma-controller/dma/dma0chan7/slave -> ../../../e6e68000.serial > > ├── e6720000.dma-controller/dma/dma1chan0/slave -> ../../../e6b10000.spi > > ├── e6720000.dma-controller/dma/dma1chan1/slave -> ../../../e6b10000.spi > > ├── e6720000.dma-controller/dma/dma1chan2/slave -> ../../../ee140000.sd > > ├── e6720000.dma-controller/dma/dma1chan3/slave -> ../../../ee140000.sd > > ├── e6b10000.spi/dma:rx -> ../e6720000.dma-controller/dma/dma1chan1 > > ├── e6b10000.spi/dma:tx -> ../e6720000.dma-controller/dma/dma1chan0 > > ├── e6e20000.spi/dma:rx -> ../e6700000.dma-controller/dma/dma0chan1 > > ├── e6e20000.spi/dma:tx -> ../e6700000.dma-controller/dma/dma0chan0 > > ├── e6e68000.serial/dma:rx -> ../e6700000.dma-controller/dma/dma0chan7 > > ├── e6e68000.serial/dma:tx -> ../e6700000.dma-controller/dma/dma0chan6 > > ├── ee100000.sd/dma:rx -> ../e6700000.dma-controller/dma/dma0chan3 > > ├── ee100000.sd/dma:tx -> ../e6700000.dma-controller/dma/dma0chan2 > > ├── ee140000.sd/dma:rx -> ../e6720000.dma-controller/dma/dma1chan3 > > ├── ee140000.sd/dma:tx -> ../e6720000.dma-controller/dma/dma1chan2 > > ├── ee160000.sd/dma:rx -> ../e6700000.dma-controller/dma/dma0chan5 > > └── ee160000.sd/dma:tx -> ../e6700000.dma-controller/dma/dma0chan4 Gr{oetje,eeting}s, Geert
On 20/01/2020 11.01, Geert Uytterhoeven wrote: > Hi Peter, > > 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 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. >>> 2. A "dma:<name>" symlink slave device to DMA channel. >>> When the channel is released, the symlinks are removed again. >>> The latter requires keeping track of the slave device and the channel >>> name in the dma_chan structure. >>> >>> Note that this is limited to channel request functions for requesting an >>> exclusive slave channel that take a device pointer (dma_request_chan() >>> and dma_request_slave_channel*()). >>> >>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> >>> --- >>> v2: >>> - Add DMA_SLAVE_NAME macro, >>> - Also handle channels from FIXME, >>> - Add backlinks from slave device to DMA channel, >>> >>> On r8a7791/koelsch, the following new symlinks are created: >>> >>> /sys/devices/platform/soc/ >>> ├── e6700000.dma-controller/dma/dma0chan0/slave -> ../../../e6e20000.spi >>> ├── e6700000.dma-controller/dma/dma0chan1/slave -> ../../../e6e20000.spi >>> ├── e6700000.dma-controller/dma/dma0chan2/slave -> ../../../ee100000.sd >>> ├── e6700000.dma-controller/dma/dma0chan3/slave -> ../../../ee100000.sd >>> ├── e6700000.dma-controller/dma/dma0chan4/slave -> ../../../ee160000.sd >>> ├── e6700000.dma-controller/dma/dma0chan5/slave -> ../../../ee160000.sd >>> ├── e6700000.dma-controller/dma/dma0chan6/slave -> ../../../e6e68000.serial >>> ├── e6700000.dma-controller/dma/dma0chan7/slave -> ../../../e6e68000.serial >>> ├── e6720000.dma-controller/dma/dma1chan0/slave -> ../../../e6b10000.spi >>> ├── e6720000.dma-controller/dma/dma1chan1/slave -> ../../../e6b10000.spi >>> ├── e6720000.dma-controller/dma/dma1chan2/slave -> ../../../ee140000.sd >>> ├── e6720000.dma-controller/dma/dma1chan3/slave -> ../../../ee140000.sd >>> ├── e6b10000.spi/dma:rx -> ../e6720000.dma-controller/dma/dma1chan1 >>> ├── e6b10000.spi/dma:tx -> ../e6720000.dma-controller/dma/dma1chan0 >>> ├── e6e20000.spi/dma:rx -> ../e6700000.dma-controller/dma/dma0chan1 >>> ├── e6e20000.spi/dma:tx -> ../e6700000.dma-controller/dma/dma0chan0 >>> ├── e6e68000.serial/dma:rx -> ../e6700000.dma-controller/dma/dma0chan7 >>> ├── e6e68000.serial/dma:tx -> ../e6700000.dma-controller/dma/dma0chan6 >>> ├── ee100000.sd/dma:rx -> ../e6700000.dma-controller/dma/dma0chan3 >>> ├── ee100000.sd/dma:tx -> ../e6700000.dma-controller/dma/dma0chan2 >>> ├── ee140000.sd/dma:rx -> ../e6720000.dma-controller/dma/dma1chan3 >>> ├── ee140000.sd/dma:tx -> ../e6720000.dma-controller/dma/dma1chan2 >>> ├── ee160000.sd/dma:rx -> ../e6700000.dma-controller/dma/dma0chan5 >>> └── ee160000.sd/dma:tx -> ../e6700000.dma-controller/dma/dma0chan4 > > > Gr{oetje,eeting}s, > > Geert > - Péter Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Hi Peter, 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*. > 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? That brings the complexity of traversing all channels etc. What do other people think? Thanks! Gr{oetje,eeting}s, Geert
Hi Geert, On 20/01/2020 12.51, Geert Uytterhoeven wrote: > Hi Peter, > > 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. >> 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 ... 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? > > Thanks! > > Gr{oetje,eeting}s, > > Geert > - Péter Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
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. > > >> 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 > ... > > 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? ;-) Thanks! Gr{oetje,eeting}s, Geert
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 :-)
On 22-01-20, 15:10, Vinod Koul wrote: > 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 :-) So no new ideas, I am going to apply this and queue for 5.6, something is better than nothing. And I am looking forward for debugfs to give better picture, volunteers?
Vinod, Geert, On 24/01/2020 8.13, Vinod Koul wrote: > On 22-01-20, 15:10, Vinod Koul wrote: > >> 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 :-) > > So no new ideas, I am going to apply this and queue for 5.6, something > is better than nothing. My only issue with the symlink is that it is created/removed on some setups quite frequently as they request/release channel per transfer or open/close. It might be a small hit in performance, but it is going to be for them. > And I am looking forward for debugfs to give better picture, volunteers? Well, I still feel that the debugfs can give better view in one place and in production it can be disabled to save few bytes per channel and code is not complied in. If we have the debugfs we can remove some of the sysfs devices files probably. gpiolib have a nice implementation for us to lift and adapt. - Péter Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
On 24-01-20, 09:31, Peter Ujfalusi wrote: > Vinod, Geert, > > On 24/01/2020 8.13, Vinod Koul wrote: > > On 22-01-20, 15:10, Vinod Koul wrote: > > > >> 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 :-) > > > > So no new ideas, I am going to apply this and queue for 5.6, something > > is better than nothing. > > My only issue with the symlink is that it is created/removed on some > setups quite frequently as they request/release channel per transfer or > open/close. > It might be a small hit in performance, but it is going to be for them. > > > And I am looking forward for debugfs to give better picture, volunteers? > > Well, I still feel that the debugfs can give better view in one place > and in production it can be disabled to save few bytes per channel and > code is not complied in. > > If we have the debugfs we can remove some of the sysfs devices files > probably. Sure I dont mind if we move to something better :) We went from zero to something and can do better! Thanks
Hi Geert, On 17.01.2020 16:30, 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, > 2. A "dma:<name>" symlink slave device to DMA channel. > When the channel is released, the symlinks are removed again. > The latter requires keeping track of the slave device and the channel > name in the dma_chan structure. > > Note that this is limited to channel request functions for requesting an > exclusive slave channel that take a device pointer (dma_request_chan() > and dma_request_slave_channel*()). > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > Tested-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> This patch breaks booting on almost all Exynos based boards: https://lore.kernel.org/linux-samsung-soc/20200129161113.GE3928@sirena.org.uk/T/#u I've already sent a fix: https://lkml.org/lkml/2020/1/29/498 BTW, this patch reminds me some of my earlier work: https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1329778.html I had similar need to keep a client's struct device pointer for every requested channel, but it turned out to be much more complicated than I've initially thought. I've abandoned that, due to lack of time, but maybe some of that discussion and concerns are still valid (I hope that links to earlier versions are still working)... > ... Best regards
Hi Marek, On Wed, Jan 29, 2020 at 6:47 PM Marek Szyprowski <m.szyprowski@samsung.com> wrote: > On 17.01.2020 16:30, 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, > > 2. A "dma:<name>" symlink slave device to DMA channel. > > When the channel is released, the symlinks are removed again. > > The latter requires keeping track of the slave device and the channel > > name in the dma_chan structure. > > > > Note that this is limited to channel request functions for requesting an > > exclusive slave channel that take a device pointer (dma_request_chan() > > and dma_request_slave_channel*()). > > > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > > Tested-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > This patch breaks booting on almost all Exynos based boards: > > https://lore.kernel.org/linux-samsung-soc/20200129161113.GE3928@sirena.org.uk/T/#u Sorry for the breakage. > I've already sent a fix: > > https://lkml.org/lkml/2020/1/29/498 Thanks a lot! > BTW, this patch reminds me some of my earlier work: > > https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1329778.html > > I had similar need to keep a client's struct device pointer for every > requested channel, but it turned out to be much more complicated than > I've initially thought. I've abandoned that, due to lack of time, but > maybe some of that discussion and concerns are still valid (I hope that > links to earlier versions are still working)... Oh right, Runtime PM for DMA channels. As several DMA calls can be made from atomic context, probably the API should be split in a non-atomic and an atomic part, cfr. the difference between clk_prepare() and clk_enable(). Still, DMA slave drivers would need to be modified, to call the "prepare" to make use of this... Gr{oetje,eeting}s, Geert
Geert, On 17/01/2020 17.30, 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, > 2. A "dma:<name>" symlink slave device to DMA channel. > When the channel is released, the symlinks are removed again. > The latter requires keeping track of the slave device and the channel > name in the dma_chan structure. > > Note that this is limited to channel request functions for requesting an > exclusive slave channel that take a device pointer (dma_request_chan() > and dma_request_slave_channel*()). > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > --- > v2: > - Add DMA_SLAVE_NAME macro, > - Also handle channels from FIXME, > - Add backlinks from slave device to DMA channel, > > On r8a7791/koelsch, the following new symlinks are created: > > /sys/devices/platform/soc/ > ├── e6700000.dma-controller/dma/dma0chan0/slave -> ../../../e6e20000.spi > ├── e6700000.dma-controller/dma/dma0chan1/slave -> ../../../e6e20000.spi > ├── e6700000.dma-controller/dma/dma0chan2/slave -> ../../../ee100000.sd > ├── e6700000.dma-controller/dma/dma0chan3/slave -> ../../../ee100000.sd > ├── e6700000.dma-controller/dma/dma0chan4/slave -> ../../../ee160000.sd > ├── e6700000.dma-controller/dma/dma0chan5/slave -> ../../../ee160000.sd > ├── e6700000.dma-controller/dma/dma0chan6/slave -> ../../../e6e68000.serial > ├── e6700000.dma-controller/dma/dma0chan7/slave -> ../../../e6e68000.serial > ├── e6720000.dma-controller/dma/dma1chan0/slave -> ../../../e6b10000.spi > ├── e6720000.dma-controller/dma/dma1chan1/slave -> ../../../e6b10000.spi > ├── e6720000.dma-controller/dma/dma1chan2/slave -> ../../../ee140000.sd > ├── e6720000.dma-controller/dma/dma1chan3/slave -> ../../../ee140000.sd > ├── e6b10000.spi/dma:rx -> ../e6720000.dma-controller/dma/dma1chan1 > ├── e6b10000.spi/dma:tx -> ../e6720000.dma-controller/dma/dma1chan0 > ├── e6e20000.spi/dma:rx -> ../e6700000.dma-controller/dma/dma0chan1 > ├── e6e20000.spi/dma:tx -> ../e6700000.dma-controller/dma/dma0chan0 > ├── e6e68000.serial/dma:rx -> ../e6700000.dma-controller/dma/dma0chan7 > ├── e6e68000.serial/dma:tx -> ../e6700000.dma-controller/dma/dma0chan6 > ├── ee100000.sd/dma:rx -> ../e6700000.dma-controller/dma/dma0chan3 > ├── ee100000.sd/dma:tx -> ../e6700000.dma-controller/dma/dma0chan2 > ├── ee140000.sd/dma:rx -> ../e6720000.dma-controller/dma/dma1chan3 > ├── ee140000.sd/dma:tx -> ../e6720000.dma-controller/dma/dma1chan2 > ├── ee160000.sd/dma:rx -> ../e6700000.dma-controller/dma/dma0chan5 > └── ee160000.sd/dma:tx -> ../e6700000.dma-controller/dma/dma0chan4 > > On r8a77951/salvator-xs: > > /sys/devices/platform/soc/ > ├── e6460000.dma-controller/dma/dma4chan0/slave -> ../../../e659c000.usb > ├── e6460000.dma-controller/dma/dma4chan1/slave -> ../../../e659c000.usb > ├── e6470000.dma-controller/dma/dma5chan0/slave -> ../../../e659c000.usb > ├── e6470000.dma-controller/dma/dma5chan1/slave -> ../../../e659c000.usb > ├── e6510000.i2c/dma:tx -> ../e7300000.dma-controller/dma/dma7chan0 > ├── e6550000.serial/dma:rx -> ../e7310000.dma-controller/dma/dma8chan1 > ├── e6550000.serial/dma:tx -> ../e7310000.dma-controller/dma/dma8chan0 > ├── e6590000.usb/dma:ch0 -> ../e65a0000.dma-controller/dma/dma2chan0 > ├── e6590000.usb/dma:ch1 -> ../e65a0000.dma-controller/dma/dma2chan1 > ├── e6590000.usb/dma:ch2 -> ../e65b0000.dma-controller/dma/dma3chan0 > ├── e6590000.usb/dma:ch3 -> ../e65b0000.dma-controller/dma/dma3chan1 > ├── e659c000.usb/dma:ch0 -> ../e6460000.dma-controller/dma/dma4chan0 > ├── e659c000.usb/dma:ch1 -> ../e6460000.dma-controller/dma/dma4chan1 > ├── e659c000.usb/dma:ch2 -> ../e6470000.dma-controller/dma/dma5chan0 > ├── e659c000.usb/dma:ch3 -> ../e6470000.dma-controller/dma/dma5chan1 > ├── e65a0000.dma-controller/dma/dma2chan0/slave -> ../../../e6590000.usb > ├── e65a0000.dma-controller/dma/dma2chan1/slave -> ../../../e6590000.usb > ├── e65b0000.dma-controller/dma/dma3chan0/slave -> ../../../e6590000.usb > ├── e65b0000.dma-controller/dma/dma3chan1/slave -> ../../../e6590000.usb > ├── e7300000.dma-controller/dma/dma7chan0/slave -> ../../../e6510000.i2c > ├── e7310000.dma-controller/dma/dma8chan0/slave -> ../../../e6550000.serial > └── e7310000.dma-controller/dma/dma8chan1/slave -> ../../../e6550000.serial > --- > drivers/dma/dmaengine.c | 37 +++++++++++++++++++++++++++++++------ > include/linux/dmaengine.h | 4 ++++ > 2 files changed, 35 insertions(+), 6 deletions(-) > > diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c > index 56a8420c388679d3..617c84cf6800962b 100644 > --- a/drivers/dma/dmaengine.c > +++ b/drivers/dma/dmaengine.c > @@ -60,6 +60,8 @@ static long dmaengine_ref_count; > > /* --- sysfs implementation --- */ > > +#define DMA_SLAVE_NAME "slave" > + > /** > * dev_to_dma_chan - convert a device pointer to its sysfs container object > * @dev - device node > @@ -730,11 +732,11 @@ struct dma_chan *dma_request_chan(struct device *dev, const char *name) > if (has_acpi_companion(dev) && !chan) > chan = acpi_dma_request_slave_chan_by_name(dev, name); > > - if (chan) { > - /* Valid channel found or requester needs to be deferred */ > - if (!IS_ERR(chan) || PTR_ERR(chan) == -EPROBE_DEFER) > - return chan; > - } > + if (PTR_ERR(chan) == -EPROBE_DEFER) > + return chan; > + > + if (!IS_ERR_OR_NULL(chan)) > + goto found; > > /* Try to find the channel via the DMA filter map(s) */ > mutex_lock(&dma_list_mutex); > @@ -754,7 +756,23 @@ struct dma_chan *dma_request_chan(struct device *dev, const char *name) > } > mutex_unlock(&dma_list_mutex); > > - return chan ? chan : ERR_PTR(-EPROBE_DEFER); > + if (!IS_ERR_OR_NULL(chan)) > + goto found; > + > + return ERR_PTR(-EPROBE_DEFER); > + > +found: > + chan->slave = dev; > + chan->name = kasprintf(GFP_KERNEL, "dma:%s", name); > + if (!chan->name) > + return ERR_PTR(-ENOMEM); You will lock the channel... It is requested, but it is not released in case of failure. > + > + 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); > + if (sysfs_create_link(&dev->kobj, &chan->dev->device.kobj, chan->name)) > + dev_err(dev, "Cannot create DMA %s symlink\n", chan->name); It is not a problem if these fail? > + return chan; > } > EXPORT_SYMBOL_GPL(dma_request_chan); > > @@ -812,6 +830,13 @@ 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->slave->kobj, chan->name); > + kfree(chan->name); > + chan->name = NULL; > + chan->slave = NULL; > + } > + sysfs_remove_link(&chan->dev->device.kobj, DMA_SLAVE_NAME); If a non slave channel is released, then you remove the link you have never created? What happens if the link creation fails and here you attempt to remove the failed ones? > mutex_unlock(&dma_list_mutex); > } > EXPORT_SYMBOL_GPL(dma_release_channel); > diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h > index 2cd1d6d7ef0fcce5..2804da93e27e114b 100644 > --- a/include/linux/dmaengine.h > +++ b/include/linux/dmaengine.h > @@ -238,10 +238,12 @@ struct dma_router { > /** > * struct dma_chan - devices supply DMA channels, clients use them > * @device: ptr to the dma device who supplies this channel, always !%NULL > + * @slave: ptr to the device using this channel > * @cookie: last cookie value returned to client > * @completed_cookie: last completed cookie for this channel > * @chan_id: channel ID for sysfs > * @dev: class device for sysfs > + * @name: backlink name for sysfs > * @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 > @@ -252,12 +254,14 @@ struct dma_router { > */ > struct dma_chan { > struct dma_device *device; > + struct device *slave; > dma_cookie_t cookie; > dma_cookie_t completed_cookie; > > /* sysfs */ > int chan_id; > struct dma_chan_dev *dev; > + const char *name; > > struct list_head device_node; > struct dma_chan_percpu __percpu *local; > - Péter Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Hi Peter, On Thu, Jan 30, 2020 at 10:42 AM Peter Ujfalusi <peter.ujfalusi@ti.com> wrote: > On 17/01/2020 17.30, 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, > > 2. A "dma:<name>" symlink slave device to DMA channel. > > When the channel is released, the symlinks are removed again. > > The latter requires keeping track of the slave device and the channel > > name in the dma_chan structure. > > > > Note that this is limited to channel request functions for requesting an > > exclusive slave channel that take a device pointer (dma_request_chan() > > and dma_request_slave_channel*()). > > > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > > --- a/drivers/dma/dmaengine.c > > +++ b/drivers/dma/dmaengine.c > > @@ -60,6 +60,8 @@ static long dmaengine_ref_count; > > > > /* --- sysfs implementation --- */ > > > > +#define DMA_SLAVE_NAME "slave" > > + > > /** > > * dev_to_dma_chan - convert a device pointer to its sysfs container object > > * @dev - device node > > @@ -730,11 +732,11 @@ struct dma_chan *dma_request_chan(struct device *dev, const char *name) > > if (has_acpi_companion(dev) && !chan) > > chan = acpi_dma_request_slave_chan_by_name(dev, name); > > > > - if (chan) { > > - /* Valid channel found or requester needs to be deferred */ > > - if (!IS_ERR(chan) || PTR_ERR(chan) == -EPROBE_DEFER) > > - return chan; > > - } > > + if (PTR_ERR(chan) == -EPROBE_DEFER) > > + return chan; > > + > > + if (!IS_ERR_OR_NULL(chan)) > > + goto found; > > > > /* Try to find the channel via the DMA filter map(s) */ > > mutex_lock(&dma_list_mutex); > > @@ -754,7 +756,23 @@ struct dma_chan *dma_request_chan(struct device *dev, const char *name) > > } > > mutex_unlock(&dma_list_mutex); > > > > - return chan ? chan : ERR_PTR(-EPROBE_DEFER); > > + if (!IS_ERR_OR_NULL(chan)) > > + goto found; > > + > > + return ERR_PTR(-EPROBE_DEFER); > > + > > +found: > > + chan->slave = dev; > > + chan->name = kasprintf(GFP_KERNEL, "dma:%s", name); > > + if (!chan->name) > > + return ERR_PTR(-ENOMEM); > > You will lock the channel... It is requested, but it is not released in > case of failure. True. Perhaps this error should just be ignored, cfr. below. However, if this operation fails, chances are high the system will die very soon anyway. > > + > > + 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); > > + if (sysfs_create_link(&dev->kobj, &chan->dev->device.kobj, chan->name)) > > + dev_err(dev, "Cannot create DMA %s symlink\n", chan->name); > > It is not a problem if these fail? IMHO, a failure to create these links is not fatal for the operation of the device, and thus can be ignored. Just like for debugfs. > > + return chan; > > } > > EXPORT_SYMBOL_GPL(dma_request_chan); > > > > @@ -812,6 +830,13 @@ 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->slave->kobj, chan->name); > > + kfree(chan->name); > > + chan->name = NULL; > > + chan->slave = NULL; > > + } > > + sysfs_remove_link(&chan->dev->device.kobj, DMA_SLAVE_NAME); > > If a non slave channel is released, then you remove the link you have > never created? > > What happens if the link creation fails and here you attempt to remove > the failed ones? sysfs_remove_link() should handle removing non-existent links, and just return -ENOENT. Gr{oetje,eeting}s, Geert
Hi Geert, On 30/01/2020 11.51, Geert Uytterhoeven wrote: > Hi Peter, > > On Thu, Jan 30, 2020 at 10:42 AM Peter Ujfalusi <peter.ujfalusi@ti.com> wrote: >> On 17/01/2020 17.30, 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, >>> 2. A "dma:<name>" symlink slave device to DMA channel. >>> When the channel is released, the symlinks are removed again. >>> The latter requires keeping track of the slave device and the channel >>> name in the dma_chan structure. >>> >>> Note that this is limited to channel request functions for requesting an >>> exclusive slave channel that take a device pointer (dma_request_chan() >>> and dma_request_slave_channel*()). >>> >>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > >>> --- a/drivers/dma/dmaengine.c >>> +++ b/drivers/dma/dmaengine.c >>> @@ -60,6 +60,8 @@ static long dmaengine_ref_count; >>> >>> /* --- sysfs implementation --- */ >>> >>> +#define DMA_SLAVE_NAME "slave" >>> + >>> /** >>> * dev_to_dma_chan - convert a device pointer to its sysfs container object >>> * @dev - device node >>> @@ -730,11 +732,11 @@ struct dma_chan *dma_request_chan(struct device *dev, const char *name) >>> if (has_acpi_companion(dev) && !chan) >>> chan = acpi_dma_request_slave_chan_by_name(dev, name); >>> >>> - if (chan) { >>> - /* Valid channel found or requester needs to be deferred */ >>> - if (!IS_ERR(chan) || PTR_ERR(chan) == -EPROBE_DEFER) >>> - return chan; >>> - } >>> + if (PTR_ERR(chan) == -EPROBE_DEFER) >>> + return chan; >>> + >>> + if (!IS_ERR_OR_NULL(chan)) >>> + goto found; >>> >>> /* Try to find the channel via the DMA filter map(s) */ >>> mutex_lock(&dma_list_mutex); >>> @@ -754,7 +756,23 @@ struct dma_chan *dma_request_chan(struct device *dev, const char *name) >>> } >>> mutex_unlock(&dma_list_mutex); >>> >>> - return chan ? chan : ERR_PTR(-EPROBE_DEFER); >>> + if (!IS_ERR_OR_NULL(chan)) >>> + goto found; >>> + >>> + return ERR_PTR(-EPROBE_DEFER); >>> + >>> +found: >>> + chan->slave = dev; >>> + chan->name = kasprintf(GFP_KERNEL, "dma:%s", name); >>> + if (!chan->name) >>> + return ERR_PTR(-ENOMEM); >> >> You will lock the channel... It is requested, but it is not released in >> case of failure. > > True. Perhaps this error should just be ignored, cfr. below. > However, if this operation fails, chances are high the system will die very soon > anyway. Yeah, I'll fix it up in a series I'm preparing. > >>> + >>> + 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); >>> + if (sysfs_create_link(&dev->kobj, &chan->dev->device.kobj, chan->name)) >>> + dev_err(dev, "Cannot create DMA %s symlink\n", chan->name); >> >> It is not a problem if these fail? > > IMHO, a failure to create these links is not fatal for the operation of > the device, and thus can be ignored. Just like for debugfs. OK, then these should not be dev_err, but dev_warn. I'll include this is also in a fixup patch. > >>> + return chan; >>> } >>> EXPORT_SYMBOL_GPL(dma_request_chan); >>> >>> @@ -812,6 +830,13 @@ 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->slave->kobj, chan->name); >>> + kfree(chan->name); >>> + chan->name = NULL; >>> + chan->slave = NULL; >>> + } >>> + sysfs_remove_link(&chan->dev->device.kobj, DMA_SLAVE_NAME); >> >> If a non slave channel is released, then you remove the link you have >> never created? >> >> What happens if the link creation fails and here you attempt to remove >> the failed ones? > > sysfs_remove_link() should handle removing non-existent links, and just > return -ENOENT. True, just followed the call chain and tested as well, but the DMA_SLAVE_NAME symlink should be also removed within the if (chan->slave) {} block as it is never created for non slave channels. Also including inn my fixup patch. > > Gr{oetje,eeting}s, > > Geert > - Péter Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Hi Geert, On 30.01.2020 09:30, Geert Uytterhoeven wrote: > On Wed, Jan 29, 2020 at 6:47 PM Marek Szyprowski > <m.szyprowski@samsung.com> wrote: >> On 17.01.2020 16:30, 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, >>> 2. A "dma:<name>" symlink slave device to DMA channel. >>> When the channel is released, the symlinks are removed again. >>> The latter requires keeping track of the slave device and the channel >>> name in the dma_chan structure. >>> >>> Note that this is limited to channel request functions for requesting an >>> exclusive slave channel that take a device pointer (dma_request_chan() >>> and dma_request_slave_channel*()). >>> >>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> >>> Tested-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> >> This patch breaks booting on almost all Exynos based boards: >> >> https://lore.kernel.org/linux-samsung-soc/20200129161113.GE3928@sirena.org.uk/T/#u > Sorry for the breakage. No problem, that's why we have linux-next. >> I've already sent a fix: >> >> https://protect2.fireeye.com/url?k=797fc496-24ab78fe-797e4fd9-0cc47a3356b2-edd084a6ee90e98a&u=https://lkml.org/lkml/2020/1/29/498 > Thanks a lot! > >> BTW, this patch reminds me some of my earlier work: >> >> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1329778.html >> >> I had similar need to keep a client's struct device pointer for every >> requested channel, but it turned out to be much more complicated than >> I've initially thought. I've abandoned that, due to lack of time, but >> maybe some of that discussion and concerns are still valid (I hope that >> links to earlier versions are still working)... > Oh right, Runtime PM for DMA channels. > > As several DMA calls can be made from atomic context, probably the API > should be split in a non-atomic and an atomic part, cfr. the difference > between clk_prepare() and clk_enable(). Still, DMA slave drivers would need > to be modified, to call the "prepare" to make use of this... Well, I'm not a big fan for introducing this 2-levels of operation (prepare/enable). In most typical designs dmaengines have to operate from the atomic context anyway. I've made a workaround for that using device links. Usually dmaengine runtime PM can simply follow the runtime PM state of its client (master?) device. The main problem that time was to reliably find the device which requested the given channel. If you have some spare time, please read the thread and the discussions in the previous versions. This might be a bit related to the devices you create the symlinks. Best regards
Hi Marek, On Thu, Jan 30, 2020 at 11:33 AM Marek Szyprowski <m.szyprowski@samsung.com> wrote: > On 30.01.2020 09:30, Geert Uytterhoeven wrote: > > On Wed, Jan 29, 2020 at 6:47 PM Marek Szyprowski > > <m.szyprowski@samsung.com> wrote: > >> On 17.01.2020 16:30, 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, > >>> 2. A "dma:<name>" symlink slave device to DMA channel. > >>> When the channel is released, the symlinks are removed again. > >>> The latter requires keeping track of the slave device and the channel > >>> name in the dma_chan structure. > >>> > >>> Note that this is limited to channel request functions for requesting an > >>> exclusive slave channel that take a device pointer (dma_request_chan() > >>> and dma_request_slave_channel*()). > >>> > >>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > >>> Tested-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > >> This patch breaks booting on almost all Exynos based boards: > >> > >> https://lore.kernel.org/linux-samsung-soc/20200129161113.GE3928@sirena.org.uk/T/#u > > Sorry for the breakage. > > No problem, that's why we have linux-next. Not really: by that time it had been upstream for 2 days :-( Gr{oetje,eeting}s, Geert
diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c index 56a8420c388679d3..617c84cf6800962b 100644 --- a/drivers/dma/dmaengine.c +++ b/drivers/dma/dmaengine.c @@ -60,6 +60,8 @@ static long dmaengine_ref_count; /* --- sysfs implementation --- */ +#define DMA_SLAVE_NAME "slave" + /** * dev_to_dma_chan - convert a device pointer to its sysfs container object * @dev - device node @@ -730,11 +732,11 @@ struct dma_chan *dma_request_chan(struct device *dev, const char *name) if (has_acpi_companion(dev) && !chan) chan = acpi_dma_request_slave_chan_by_name(dev, name); - if (chan) { - /* Valid channel found or requester needs to be deferred */ - if (!IS_ERR(chan) || PTR_ERR(chan) == -EPROBE_DEFER) - return chan; - } + if (PTR_ERR(chan) == -EPROBE_DEFER) + return chan; + + if (!IS_ERR_OR_NULL(chan)) + goto found; /* Try to find the channel via the DMA filter map(s) */ mutex_lock(&dma_list_mutex); @@ -754,7 +756,23 @@ struct dma_chan *dma_request_chan(struct device *dev, const char *name) } mutex_unlock(&dma_list_mutex); - return chan ? chan : ERR_PTR(-EPROBE_DEFER); + if (!IS_ERR_OR_NULL(chan)) + goto found; + + return ERR_PTR(-EPROBE_DEFER); + +found: + chan->slave = dev; + chan->name = kasprintf(GFP_KERNEL, "dma:%s", name); + if (!chan->name) + return ERR_PTR(-ENOMEM); + + 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); + if (sysfs_create_link(&dev->kobj, &chan->dev->device.kobj, chan->name)) + dev_err(dev, "Cannot create DMA %s symlink\n", chan->name); + return chan; } EXPORT_SYMBOL_GPL(dma_request_chan); @@ -812,6 +830,13 @@ 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->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); diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h index 2cd1d6d7ef0fcce5..2804da93e27e114b 100644 --- a/include/linux/dmaengine.h +++ b/include/linux/dmaengine.h @@ -238,10 +238,12 @@ struct dma_router { /** * struct dma_chan - devices supply DMA channels, clients use them * @device: ptr to the dma device who supplies this channel, always !%NULL + * @slave: ptr to the device using this channel * @cookie: last cookie value returned to client * @completed_cookie: last completed cookie for this channel * @chan_id: channel ID for sysfs * @dev: class device for sysfs + * @name: backlink name for sysfs * @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 @@ -252,12 +254,14 @@ struct dma_router { */ struct dma_chan { struct dma_device *device; + struct device *slave; dma_cookie_t cookie; dma_cookie_t completed_cookie; /* sysfs */ int chan_id; struct dma_chan_dev *dev; + const char *name; struct list_head device_node; struct dma_chan_percpu __percpu *local;
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, 2. A "dma:<name>" symlink slave device to DMA channel. When the channel is released, the symlinks are removed again. The latter requires keeping track of the slave device and the channel name in the dma_chan structure. Note that this is limited to channel request functions for requesting an exclusive slave channel that take a device pointer (dma_request_chan() and dma_request_slave_channel*()). Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> --- v2: - Add DMA_SLAVE_NAME macro, - Also handle channels from FIXME, - Add backlinks from slave device to DMA channel, On r8a7791/koelsch, the following new symlinks are created: /sys/devices/platform/soc/ ├── e6700000.dma-controller/dma/dma0chan0/slave -> ../../../e6e20000.spi ├── e6700000.dma-controller/dma/dma0chan1/slave -> ../../../e6e20000.spi ├── e6700000.dma-controller/dma/dma0chan2/slave -> ../../../ee100000.sd ├── e6700000.dma-controller/dma/dma0chan3/slave -> ../../../ee100000.sd ├── e6700000.dma-controller/dma/dma0chan4/slave -> ../../../ee160000.sd ├── e6700000.dma-controller/dma/dma0chan5/slave -> ../../../ee160000.sd ├── e6700000.dma-controller/dma/dma0chan6/slave -> ../../../e6e68000.serial ├── e6700000.dma-controller/dma/dma0chan7/slave -> ../../../e6e68000.serial ├── e6720000.dma-controller/dma/dma1chan0/slave -> ../../../e6b10000.spi ├── e6720000.dma-controller/dma/dma1chan1/slave -> ../../../e6b10000.spi ├── e6720000.dma-controller/dma/dma1chan2/slave -> ../../../ee140000.sd ├── e6720000.dma-controller/dma/dma1chan3/slave -> ../../../ee140000.sd ├── e6b10000.spi/dma:rx -> ../e6720000.dma-controller/dma/dma1chan1 ├── e6b10000.spi/dma:tx -> ../e6720000.dma-controller/dma/dma1chan0 ├── e6e20000.spi/dma:rx -> ../e6700000.dma-controller/dma/dma0chan1 ├── e6e20000.spi/dma:tx -> ../e6700000.dma-controller/dma/dma0chan0 ├── e6e68000.serial/dma:rx -> ../e6700000.dma-controller/dma/dma0chan7 ├── e6e68000.serial/dma:tx -> ../e6700000.dma-controller/dma/dma0chan6 ├── ee100000.sd/dma:rx -> ../e6700000.dma-controller/dma/dma0chan3 ├── ee100000.sd/dma:tx -> ../e6700000.dma-controller/dma/dma0chan2 ├── ee140000.sd/dma:rx -> ../e6720000.dma-controller/dma/dma1chan3 ├── ee140000.sd/dma:tx -> ../e6720000.dma-controller/dma/dma1chan2 ├── ee160000.sd/dma:rx -> ../e6700000.dma-controller/dma/dma0chan5 └── ee160000.sd/dma:tx -> ../e6700000.dma-controller/dma/dma0chan4 On r8a77951/salvator-xs: /sys/devices/platform/soc/ ├── e6460000.dma-controller/dma/dma4chan0/slave -> ../../../e659c000.usb ├── e6460000.dma-controller/dma/dma4chan1/slave -> ../../../e659c000.usb ├── e6470000.dma-controller/dma/dma5chan0/slave -> ../../../e659c000.usb ├── e6470000.dma-controller/dma/dma5chan1/slave -> ../../../e659c000.usb ├── e6510000.i2c/dma:tx -> ../e7300000.dma-controller/dma/dma7chan0 ├── e6550000.serial/dma:rx -> ../e7310000.dma-controller/dma/dma8chan1 ├── e6550000.serial/dma:tx -> ../e7310000.dma-controller/dma/dma8chan0 ├── e6590000.usb/dma:ch0 -> ../e65a0000.dma-controller/dma/dma2chan0 ├── e6590000.usb/dma:ch1 -> ../e65a0000.dma-controller/dma/dma2chan1 ├── e6590000.usb/dma:ch2 -> ../e65b0000.dma-controller/dma/dma3chan0 ├── e6590000.usb/dma:ch3 -> ../e65b0000.dma-controller/dma/dma3chan1 ├── e659c000.usb/dma:ch0 -> ../e6460000.dma-controller/dma/dma4chan0 ├── e659c000.usb/dma:ch1 -> ../e6460000.dma-controller/dma/dma4chan1 ├── e659c000.usb/dma:ch2 -> ../e6470000.dma-controller/dma/dma5chan0 ├── e659c000.usb/dma:ch3 -> ../e6470000.dma-controller/dma/dma5chan1 ├── e65a0000.dma-controller/dma/dma2chan0/slave -> ../../../e6590000.usb ├── e65a0000.dma-controller/dma/dma2chan1/slave -> ../../../e6590000.usb ├── e65b0000.dma-controller/dma/dma3chan0/slave -> ../../../e6590000.usb ├── e65b0000.dma-controller/dma/dma3chan1/slave -> ../../../e6590000.usb ├── e7300000.dma-controller/dma/dma7chan0/slave -> ../../../e6510000.i2c ├── e7310000.dma-controller/dma/dma8chan0/slave -> ../../../e6550000.serial └── e7310000.dma-controller/dma/dma8chan1/slave -> ../../../e6550000.serial --- drivers/dma/dmaengine.c | 37 +++++++++++++++++++++++++++++++------ include/linux/dmaengine.h | 4 ++++ 2 files changed, 35 insertions(+), 6 deletions(-)