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

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

[Geert Uytterhoeven]

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

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

[Niklas Söderlund]

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
> 

-- 
Regards,
Niklas Söderlund

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

[Peter Ujfalusi]

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

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

[Geert Uytterhoeven]

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

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

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

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

[Peter Ujfalusi]

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

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

[Geert Uytterhoeven]

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

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

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

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

[Peter Ujfalusi]

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

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

[Geert Uytterhoeven]

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

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

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

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

[Vinod Koul]

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

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

[Vinod Koul]

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

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

[Peter Ujfalusi]

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

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

[Vinod Koul]

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

-- 
~Vinod

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

[Marek Szyprowski]

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
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

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

[Geert Uytterhoeven]

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 Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

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

[Peter Ujfalusi]

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

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

[Geert Uytterhoeven]

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

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

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

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

[Peter Ujfalusi]

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

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

[Marek Szyprowski]

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
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

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

[Geert Uytterhoeven]

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

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

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