[PATCH v1 0/4] dmaengine: dma_request_chan*() amendments

[PATCH v1 0/4] dmaengine: dma_request_chan*() amendments

[Andy Shevchenko]

Reduce the scope of the use of some rarely used DMA request channel APIs
in order to make the step of their removal or making static in the
future. No functional changes intended.

Andy Shevchenko (4):
  dmaengine: Replace dma_request_slave_channel() by dma_request_chan()
  dmaengine: Use dma_request_channel() instead of
    __dma_request_channel()
  dmaengine: Add a comment on why it's okay when kasprintf() fails
  dmaengine: Unify checks in dma_request_chan()

 drivers/dma/dmaengine.c   | 16 ++++++++--------
 drivers/dma/imx-sdma.c    |  5 ++---
 include/linux/dmaengine.h |  6 +++---
 3 files changed, 13 insertions(+), 14 deletions(-)

-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v1 1/4] dmaengine: Replace dma_request_slave_channel() by dma_request_chan()

[Andy Shevchenko]

Replace dma_request_slave_channel() by dma_request_chan() as suggested
since the former is deprecated.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/dma/imx-sdma.c    | 5 ++---
 include/linux/dmaengine.h | 4 ++--
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
index 72299a08af44..3a769934c984 100644
--- a/drivers/dma/imx-sdma.c
+++ b/drivers/dma/imx-sdma.c
@@ -1459,9 +1459,8 @@ static int sdma_alloc_chan_resources(struct dma_chan *chan)
 	 * dmatest, thus create 'struct imx_dma_data mem_data' for this case.
 	 * Please note in any other slave case, you have to setup chan->private
 	 * with 'struct imx_dma_data' in your own filter function if you want to
-	 * request dma channel by dma_request_channel() rather than
-	 * dma_request_slave_channel(). Othwise, 'MEMCPY in case?' will appear
-	 * to warn you to correct your filter function.
+	 * request DMA channel by dma_request_channel(), otherwise, 'MEMCPY in
+	 * case?' will appear to warn you to correct your filter function.
 	 */
 	if (!data) {
 		dev_dbg(sdmac->sdma->dev, "MEMCPY in case?\n");
diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
index b137fdb56093..b4e6de892d34 100644
--- a/include/linux/dmaengine.h
+++ b/include/linux/dmaengine.h
@@ -1632,8 +1632,8 @@ static inline struct dma_chan
 {
 	struct dma_chan *chan;
 
-	chan = dma_request_slave_channel(dev, name);
-	if (chan)
+	chan = dma_request_chan(dev, name);
+	if (!IS_ERR(chan))
 		return chan;
 
 	if (!fn || !fn_param)
-- 
2.43.0.rc1.1336.g36b5255a03ac

Re: [PATCH v1 1/4] dmaengine: Replace dma_request_slave_channel() by dma_request_chan()

[Frank Li]

On Mon, Oct 07, 2024 at 06:06:45PM +0300, Andy Shevchenko wrote:
> Replace dma_request_slave_channel() by dma_request_chan() as suggested
> since the former is deprecated.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/dma/imx-sdma.c    | 5 ++---
>  include/linux/dmaengine.h | 4 ++--
>  2 files changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
> index 72299a08af44..3a769934c984 100644
> --- a/drivers/dma/imx-sdma.c
> +++ b/drivers/dma/imx-sdma.c
> @@ -1459,9 +1459,8 @@ static int sdma_alloc_chan_resources(struct dma_chan *chan)
>  	 * dmatest, thus create 'struct imx_dma_data mem_data' for this case.
>  	 * Please note in any other slave case, you have to setup chan->private
>  	 * with 'struct imx_dma_data' in your own filter function if you want to
> -	 * request dma channel by dma_request_channel() rather than
> -	 * dma_request_slave_channel(). Othwise, 'MEMCPY in case?' will appear
> -	 * to warn you to correct your filter function.
> +	 * request DMA channel by dma_request_channel(), otherwise, 'MEMCPY in
> +	 * case?' will appear to warn you to correct your filter function.

It just change comments, why combined with dmaengine.h change.

Frank

>  	 */
>  	if (!data) {
>  		dev_dbg(sdmac->sdma->dev, "MEMCPY in case?\n");
> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
> index b137fdb56093..b4e6de892d34 100644
> --- a/include/linux/dmaengine.h
> +++ b/include/linux/dmaengine.h
> @@ -1632,8 +1632,8 @@ static inline struct dma_chan
>  {
>  	struct dma_chan *chan;
>
> -	chan = dma_request_slave_channel(dev, name);
> -	if (chan)
> +	chan = dma_request_chan(dev, name);
> +	if (!IS_ERR(chan))
>  		return chan;
>
>  	if (!fn || !fn_param)
> --
> 2.43.0.rc1.1336.g36b5255a03ac
>

Re: [PATCH v1 1/4] dmaengine: Replace dma_request_slave_channel() by dma_request_chan()

[Andy Shevchenko]

On Mon, Oct 07, 2024 at 11:52:21AM -0400, Frank Li wrote:
> On Mon, Oct 07, 2024 at 06:06:45PM +0300, Andy Shevchenko wrote:

...

> >  	 * Please note in any other slave case, you have to setup chan->private
> >  	 * with 'struct imx_dma_data' in your own filter function if you want to
> > -	 * request dma channel by dma_request_channel() rather than
> > -	 * dma_request_slave_channel(). Othwise, 'MEMCPY in case?' will appear
> > -	 * to warn you to correct your filter function.
> > +	 * request DMA channel by dma_request_channel(), otherwise, 'MEMCPY in
> > +	 * case?' will appear to warn you to correct your filter function.
> 
> It just change comments, why combined with dmaengine.h change.

Because this comment is related to what is done below.

...

> >  	struct dma_chan *chan;
> >
> > -	chan = dma_request_slave_channel(dev, name);
> > -	if (chan)
> > +	chan = dma_request_chan(dev, name);

Here is no more dma_request_slave_channel() calls as in the mentioned
comment.

> > +	if (!IS_ERR(chan))
> >  		return chan;

-- 
With Best Regards,
Andy Shevchenko

[PATCH v1 2/4] dmaengine: Use dma_request_channel() instead of __dma_request_channel()

[Andy Shevchenko]

Let's reduce the surface of the use of __dma_request_channel().
Hopefully we can make it internall to the DMA drivers or kill for good
completely.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 include/linux/dmaengine.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
index b4e6de892d34..2f46056096d6 100644
--- a/include/linux/dmaengine.h
+++ b/include/linux/dmaengine.h
@@ -1639,7 +1639,7 @@ static inline struct dma_chan
 	if (!fn || !fn_param)
 		return NULL;
 
-	return __dma_request_channel(&mask, fn, fn_param, NULL);
+	return dma_request_channel(mask, fn, fn_param);
 }
 
 static inline char *
-- 
2.43.0.rc1.1336.g36b5255a03ac

Re: [PATCH v1 2/4] dmaengine: Use dma_request_channel() instead of __dma_request_channel()

[Frank Li]

On Mon, Oct 07, 2024 at 06:06:46PM +0300, Andy Shevchenko wrote:
> Let's reduce the surface of the use of __dma_request_channel().
> Hopefully we can make it internall to the DMA drivers or kill for good
> completely.

Suggest:

Reduce use internal __dma_request_channel() function in public
dmaengine.h

I think this change is okay, but I hope the following patches, which make
__dma_request_channel() as internal only. otherwise, it looks not necessary.

Frank

>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  include/linux/dmaengine.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
> index b4e6de892d34..2f46056096d6 100644
> --- a/include/linux/dmaengine.h
> +++ b/include/linux/dmaengine.h
> @@ -1639,7 +1639,7 @@ static inline struct dma_chan
>  	if (!fn || !fn_param)
>  		return NULL;
>
> -	return __dma_request_channel(&mask, fn, fn_param, NULL);
> +	return dma_request_channel(mask, fn, fn_param);
>  }
>
>  static inline char *
> --
> 2.43.0.rc1.1336.g36b5255a03ac
>

Re: [PATCH v1 2/4] dmaengine: Use dma_request_channel() instead of __dma_request_channel()

[Andy Shevchenko]

On Mon, Oct 07, 2024 at 12:01:22PM -0400, Frank Li wrote:
> On Mon, Oct 07, 2024 at 06:06:46PM +0300, Andy Shevchenko wrote:
> > Let's reduce the surface of the use of __dma_request_channel().
> > Hopefully we can make it internall to the DMA drivers or kill for good
> > completely.
> 
> Suggest:
> 
> Reduce use internal __dma_request_channel() function in public
> dmaengine.h

Okay.

> I think this change is okay, but I hope the following patches, which make
> __dma_request_channel() as internal only. otherwise, it looks not necessary.

As explained in the commit message it's the first baby step to this
direction. But sure, when I have more time I'll continue. In any case
I think this change is good on this own as it shows the use inside the
header.  The people who want to have an example of the use may wrongly
take the existing code as "good to go" and this even might pass the
review somewhere. That said, it's not only reducing the use, but has an
educational purpose as well.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v1 3/4] dmaengine: Add a comment on why it's okay when kasprintf() fails

[Andy Shevchenko]

In dma_request_chan() one of the kasprintf() call is not checked
against NULL. This is completely fine right now, but make others
aware of this aspect by adding a comment.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/dma/dmaengine.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
index c1357d7f3dc6..dd4224d90f07 100644
--- a/drivers/dma/dmaengine.c
+++ b/drivers/dma/dmaengine.c
@@ -854,8 +854,8 @@ struct dma_chan *dma_request_chan(struct device *dev, const char *name)
 
 found:
 #ifdef CONFIG_DEBUG_FS
-	chan->dbg_client_name = kasprintf(GFP_KERNEL, "%s:%s", dev_name(dev),
-					  name);
+	chan->dbg_client_name = kasprintf(GFP_KERNEL, "%s:%s", dev_name(dev), name);
+	/* No functional issue if it fails, users are supposed to test before use */
 #endif
 
 	chan->name = kasprintf(GFP_KERNEL, "dma:%s", name);
-- 
2.43.0.rc1.1336.g36b5255a03ac

Re: [PATCH v1 3/4] dmaengine: Add a comment on why it's okay when kasprintf() fails

[Frank Li]

On Mon, Oct 07, 2024 at 06:06:47PM +0300, Andy Shevchenko wrote:
> In dma_request_chan() one of the kasprintf() call is not checked
> against NULL. This is completely fine right now, but make others
> aware of this aspect by adding a comment.

suggest:

Add comment in dma_request_chan() to clarify kasprintf() missing return
value check and it is correct funcationaly.

>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/dma/dmaengine.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
> index c1357d7f3dc6..dd4224d90f07 100644
> --- a/drivers/dma/dmaengine.c
> +++ b/drivers/dma/dmaengine.c
> @@ -854,8 +854,8 @@ struct dma_chan *dma_request_chan(struct device *dev, const char *name)
>
>  found:
>  #ifdef CONFIG_DEBUG_FS
> -	chan->dbg_client_name = kasprintf(GFP_KERNEL, "%s:%s", dev_name(dev),
> -					  name);
> +	chan->dbg_client_name = kasprintf(GFP_KERNEL, "%s:%s", dev_name(dev), name);
> +	/* No functional issue if it fails, users are supposed to test before use */

comments should above chan->dbg_client_name ...

No funcational issue if it is NULL because user always test it before use.

>  #endif
>
>  	chan->name = kasprintf(GFP_KERNEL, "dma:%s", name);
> --
> 2.43.0.rc1.1336.g36b5255a03ac
>

Re: [PATCH v1 3/4] dmaengine: Add a comment on why it's okay when kasprintf() fails

[Andy Shevchenko]

On Mon, Oct 07, 2024 at 11:51:14AM -0400, Frank Li wrote:
> On Mon, Oct 07, 2024 at 06:06:47PM +0300, Andy Shevchenko wrote:
> > In dma_request_chan() one of the kasprintf() call is not checked
> > against NULL. This is completely fine right now, but make others
> > aware of this aspect by adding a comment.
> 
> suggest:
> 
> Add comment in dma_request_chan() to clarify kasprintf() missing return
> value check and it is correct funcationaly.

Sure, thanks.

...

> >  #ifdef CONFIG_DEBUG_FS
> > -	chan->dbg_client_name = kasprintf(GFP_KERNEL, "%s:%s", dev_name(dev),
> > -					  name);
> > +	chan->dbg_client_name = kasprintf(GFP_KERNEL, "%s:%s", dev_name(dev), name);
> > +	/* No functional issue if it fails, users are supposed to test before use */
> 
> comments should above chan->dbg_client_name ...

It's placed exactly there on purpose. Because it explains 

> No funcational issue if it is NULL because user always test it before use.

I think my is better because it reveals the actual issue, ideally users
must not rely on that and the code here should assign a valid pointer.
The problem is that the code paths are a bit twisted and I only can come
up with this comment _for now_. Semantically this change is a band-aid
(and not good), but at least it describes current (broken) desing.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v1 4/4] dmaengine: Unify checks in dma_request_chan()

[Andy Shevchenko]

Use firmware node and unify checks accordingly in dma_request_chan().
As a side effect we get rid of the node dereferencing in struct device.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/dma/dmaengine.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
index dd4224d90f07..758fcd0546d8 100644
--- a/drivers/dma/dmaengine.c
+++ b/drivers/dma/dmaengine.c
@@ -40,6 +40,8 @@
 #include <linux/dmaengine.h>
 #include <linux/hardirq.h>
 #include <linux/spinlock.h>
+#include <linux/of.h>
+#include <linux/property.h>
 #include <linux/percpu.h>
 #include <linux/rcupdate.h>
 #include <linux/mutex.h>
@@ -812,15 +814,13 @@ static const struct dma_slave_map *dma_filter_match(struct dma_device *device,
  */
 struct dma_chan *dma_request_chan(struct device *dev, const char *name)
 {
+	struct fwnode_handle *fwnode = dev_fwnode(dev);
 	struct dma_device *d, *_d;
 	struct dma_chan *chan = NULL;
 
-	/* If device-tree is present get slave info from here */
-	if (dev->of_node)
-		chan = of_dma_request_slave_channel(dev->of_node, name);
-
-	/* If device was enumerated by ACPI get slave info from here */
-	if (has_acpi_companion(dev) && !chan)
+	if (is_of_node(fwnode))
+		chan = of_dma_request_slave_channel(to_of_node(fwnode), name);
+	else if (is_acpi_device_node(fwnode))
 		chan = acpi_dma_request_slave_chan_by_name(dev, name);
 
 	if (PTR_ERR(chan) == -EPROBE_DEFER)
-- 
2.43.0.rc1.1336.g36b5255a03ac

Re: [PATCH v1 4/4] dmaengine: Unify checks in dma_request_chan()

[Frank Li]

On Mon, Oct 07, 2024 at 06:06:48PM +0300, Andy Shevchenko wrote:
> Use firmware node and unify checks accordingly in dma_request_chan().
> As a side effect we get rid of the node dereferencing in struct device.

suggest:

Use dev_fwnode() to simple check logic for device tree and ACPI in
dma_request_chan().

Frank

>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/dma/dmaengine.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
> index dd4224d90f07..758fcd0546d8 100644
> --- a/drivers/dma/dmaengine.c
> +++ b/drivers/dma/dmaengine.c
> @@ -40,6 +40,8 @@
>  #include <linux/dmaengine.h>
>  #include <linux/hardirq.h>
>  #include <linux/spinlock.h>
> +#include <linux/of.h>
> +#include <linux/property.h>
>  #include <linux/percpu.h>
>  #include <linux/rcupdate.h>
>  #include <linux/mutex.h>
> @@ -812,15 +814,13 @@ static const struct dma_slave_map *dma_filter_match(struct dma_device *device,
>   */
>  struct dma_chan *dma_request_chan(struct device *dev, const char *name)
>  {
> +	struct fwnode_handle *fwnode = dev_fwnode(dev);
>  	struct dma_device *d, *_d;
>  	struct dma_chan *chan = NULL;
>
> -	/* If device-tree is present get slave info from here */
> -	if (dev->of_node)
> -		chan = of_dma_request_slave_channel(dev->of_node, name);
> -
> -	/* If device was enumerated by ACPI get slave info from here */
> -	if (has_acpi_companion(dev) && !chan)
> +	if (is_of_node(fwnode))
> +		chan = of_dma_request_slave_channel(to_of_node(fwnode), name);
> +	else if (is_acpi_device_node(fwnode))
>  		chan = acpi_dma_request_slave_chan_by_name(dev, name);
>
>  	if (PTR_ERR(chan) == -EPROBE_DEFER)
> --
> 2.43.0.rc1.1336.g36b5255a03ac
>

Re: [PATCH v1 4/4] dmaengine: Unify checks in dma_request_chan()

[Andy Shevchenko]

On Mon, Oct 07, 2024 at 12:05:56PM -0400, Frank Li wrote:
> On Mon, Oct 07, 2024 at 06:06:48PM +0300, Andy Shevchenko wrote:
> > Use firmware node and unify checks accordingly in dma_request_chan().
> > As a side effect we get rid of the node dereferencing in struct device.
> 
> suggest:
> 
> Use dev_fwnode() to simple check logic for device tree and ACPI in
> dma_request_chan().

Sure, thanks!


-- 
With Best Regards,
Andy Shevchenko