[PATCH 1/6] dma: amba-pl08x: no need to cast away call to debugfs_create_file()

[PATCH 1/6] dma: amba-pl08x: no need to cast away call to debugfs_create_file()

[Greg Kroah-Hartman]

No need to check the return value of debugfs_create_file(), so no need
to provide a fake "cast away" of the return value either.

Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Vinod Koul <vkoul@kernel.org>
Cc: dmaengine@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/dma/amba-pl08x.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
index 464725dcad00..9adc7a2fa3d3 100644
--- a/drivers/dma/amba-pl08x.c
+++ b/drivers/dma/amba-pl08x.c
@@ -2508,9 +2508,8 @@ DEFINE_SHOW_ATTRIBUTE(pl08x_debugfs);
 static void init_pl08x_debugfs(struct pl08x_driver_data *pl08x)
 {
 	/* Expose a simple debugfs interface to view all clocks */
-	(void) debugfs_create_file(dev_name(&pl08x->adev->dev),
-			S_IFREG | S_IRUGO, NULL, pl08x,
-			&pl08x_debugfs_fops);
+	debugfs_create_file(dev_name(&pl08x->adev->dev), S_IFREG | S_IRUGO,
+			    NULL, pl08x, &pl08x_debugfs_fops);
 }
 
 #else
-- 
2.22.0

[PATCH 2/6] dma: bcm-sba-raid: no need to check return value of debugfs_create functions

[Greg Kroah-Hartman]

When calling debugfs functions, there is no need to ever check the
return value.  The function can work or not, but the code logic should
never do something different based on this.

Also, because there is no need to save the file dentry, remove the
variable that was saving it as it was never even being used once set.

Cc: Vinod Koul <vkoul@kernel.org>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: dmaengine@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/dma/bcm-sba-raid.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/drivers/dma/bcm-sba-raid.c b/drivers/dma/bcm-sba-raid.c
index fa81d0177765..275e90fa829d 100644
--- a/drivers/dma/bcm-sba-raid.c
+++ b/drivers/dma/bcm-sba-raid.c
@@ -164,7 +164,6 @@ struct sba_device {
 	struct list_head reqs_free_list;
 	/* DebugFS directory entries */
 	struct dentry *root;
-	struct dentry *stats;
 };
 
 /* ====== Command helper routines ===== */
@@ -1716,17 +1715,11 @@ static int sba_probe(struct platform_device *pdev)
 
 	/* Create debugfs root entry */
 	sba->root = debugfs_create_dir(dev_name(sba->dev), NULL);
-	if (IS_ERR_OR_NULL(sba->root)) {
-		dev_err(sba->dev, "failed to create debugfs root entry\n");
-		sba->root = NULL;
-		goto skip_debugfs;
-	}
 
 	/* Create debugfs stats entry */
-	sba->stats = debugfs_create_devm_seqfile(sba->dev, "stats", sba->root,
-						 sba_debugfs_stats_show);
-	if (IS_ERR_OR_NULL(sba->stats))
-		dev_err(sba->dev, "failed to create debugfs stats file\n");
+	debugfs_create_devm_seqfile(sba->dev, "stats", sba->root,
+				    sba_debugfs_stats_show);
+
 skip_debugfs:
 
 	/* Register DMA device with Linux async framework */
-- 
2.22.0

[PATCH 3/6] dma: coh901318: no need to cast away call to debugfs_create_file()

[Greg Kroah-Hartman]

No need to check the return value of debugfs_create_file(), so no need
to provide a fake "cast away" of the return value either.

Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Vinod Koul <vkoul@kernel.org>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: dmaengine@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/dma/coh901318.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/dma/coh901318.c b/drivers/dma/coh901318.c
index 547786ac342b..e51d836afcc7 100644
--- a/drivers/dma/coh901318.c
+++ b/drivers/dma/coh901318.c
@@ -1378,10 +1378,8 @@ static int __init init_coh901318_debugfs(void)
 
 	dma_dentry = debugfs_create_dir("dma", NULL);
 
-	(void) debugfs_create_file("status",
-				   S_IFREG | S_IRUGO,
-				   dma_dentry, NULL,
-				   &coh901318_debugfs_status_operations);
+	debugfs_create_file("status", S_IFREG | S_IRUGO, dma_dentry, NULL,
+			    &coh901318_debugfs_status_operations);
 	return 0;
 }
 
-- 
2.22.0

[PATCH 4/6] dma: pxa_dma: no need to check return value of debugfs_create functions

[Greg Kroah-Hartman]

When calling debugfs functions, there is no need to ever check the
return value.  The function can work or not, but the code logic should
never do something different based on this.

Also, because there is no need to save the file dentry, remove the
variable that was saving it as it was never even being used once set.

Cc: Daniel Mack <daniel@zonque.org>
Cc: Haojian Zhuang <haojian.zhuang@gmail.com>
Cc: Robert Jarzmik <robert.jarzmik@free.fr>
Cc: Vinod Koul <vkoul@kernel.org>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: dmaengine@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/dma/pxa_dma.c | 56 +++++++++----------------------------------
 1 file changed, 11 insertions(+), 45 deletions(-)

diff --git a/drivers/dma/pxa_dma.c b/drivers/dma/pxa_dma.c
index b429642f3e7a..0f698f49ee26 100644
--- a/drivers/dma/pxa_dma.c
+++ b/drivers/dma/pxa_dma.c
@@ -132,7 +132,6 @@ struct pxad_device {
 	spinlock_t			phy_lock;	/* Phy association */
 #ifdef CONFIG_DEBUG_FS
 	struct dentry			*dbgfs_root;
-	struct dentry			*dbgfs_state;
 	struct dentry			**dbgfs_chan;
 #endif
 };
@@ -326,31 +325,18 @@ static struct dentry *pxad_dbg_alloc_chan(struct pxad_device *pdev,
 					     int ch, struct dentry *chandir)
 {
 	char chan_name[11];
-	struct dentry *chan, *chan_state = NULL, *chan_descr = NULL;
-	struct dentry *chan_reqs = NULL;
+	struct dentry *chan;
 	void *dt;
 
 	scnprintf(chan_name, sizeof(chan_name), "%d", ch);
 	chan = debugfs_create_dir(chan_name, chandir);
 	dt = (void *)&pdev->phys[ch];
 
-	if (chan)
-		chan_state = debugfs_create_file("state", 0400, chan, dt,
-						 &chan_state_fops);
-	if (chan_state)
-		chan_descr = debugfs_create_file("descriptors", 0400, chan, dt,
-						 &descriptors_fops);
-	if (chan_descr)
-		chan_reqs = debugfs_create_file("requesters", 0400, chan, dt,
-						&requester_chan_fops);
-	if (!chan_reqs)
-		goto err_state;
+	debugfs_create_file("state", 0400, chan, dt, &chan_state_fops);
+	debugfs_create_file("descriptors", 0400, chan, dt, &descriptors_fops);
+	debugfs_create_file("requesters", 0400, chan, dt, &requester_chan_fops);
 
 	return chan;
-
-err_state:
-	debugfs_remove_recursive(chan);
-	return NULL;
 }
 
 static void pxad_init_debugfs(struct pxad_device *pdev)
@@ -358,40 +344,20 @@ static void pxad_init_debugfs(struct pxad_device *pdev)
 	int i;
 	struct dentry *chandir;
 
-	pdev->dbgfs_root = debugfs_create_dir(dev_name(pdev->slave.dev), NULL);
-	if (IS_ERR(pdev->dbgfs_root) || !pdev->dbgfs_root)
-		goto err_root;
-
-	pdev->dbgfs_state = debugfs_create_file("state", 0400, pdev->dbgfs_root,
-						pdev, &state_fops);
-	if (!pdev->dbgfs_state)
-		goto err_state;
-
 	pdev->dbgfs_chan =
-		kmalloc_array(pdev->nr_chans, sizeof(*pdev->dbgfs_state),
+		kmalloc_array(pdev->nr_chans, sizeof(struct dentry *),
 			      GFP_KERNEL);
 	if (!pdev->dbgfs_chan)
-		goto err_alloc;
+		return;
+
+	pdev->dbgfs_root = debugfs_create_dir(dev_name(pdev->slave.dev), NULL);
+
+	debugfs_create_file("state", 0400, pdev->dbgfs_root, pdev, &state_fops);
 
 	chandir = debugfs_create_dir("channels", pdev->dbgfs_root);
-	if (!chandir)
-		goto err_chandir;
 
-	for (i = 0; i < pdev->nr_chans; i++) {
+	for (i = 0; i < pdev->nr_chans; i++)
 		pdev->dbgfs_chan[i] = pxad_dbg_alloc_chan(pdev, i, chandir);
-		if (!pdev->dbgfs_chan[i])
-			goto err_chans;
-	}
-
-	return;
-err_chans:
-err_chandir:
-	kfree(pdev->dbgfs_chan);
-err_alloc:
-err_state:
-	debugfs_remove_recursive(pdev->dbgfs_root);
-err_root:
-	pr_err("pxad: debugfs is not available\n");
 }
 
 static void pxad_cleanup_debugfs(struct pxad_device *pdev)
-- 
2.22.0

[PATCH 5/6] dma: mic_x100_dma: no need to check return value of debugfs_create functions

[Greg Kroah-Hartman]

When calling debugfs functions, there is no need to ever check the
return value.  The function can work or not, but the code logic should
never do something different based on this.

Cc: Sudeep Dutt <sudeep.dutt@intel.com>
Cc: Ashutosh Dixit <ashutosh.dixit@intel.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Vinod Koul <vkoul@kernel.org>
Cc: dmaengine@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/dma/mic_x100_dma.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/dma/mic_x100_dma.c b/drivers/dma/mic_x100_dma.c
index 6a91e28d537d..584e09661507 100644
--- a/drivers/dma/mic_x100_dma.c
+++ b/drivers/dma/mic_x100_dma.c
@@ -728,10 +728,8 @@ static int mic_dma_driver_probe(struct mbus_device *mbdev)
 	if (mic_dma_dbg) {
 		mic_dma_dev->dbg_dir = debugfs_create_dir(dev_name(&mbdev->dev),
 							  mic_dma_dbg);
-		if (mic_dma_dev->dbg_dir)
-			debugfs_create_file("mic_dma_reg", 0444,
-					    mic_dma_dev->dbg_dir, mic_dma_dev,
-					    &mic_dma_reg_fops);
+		debugfs_create_file("mic_dma_reg", 0444, mic_dma_dev->dbg_dir,
+				    mic_dma_dev, &mic_dma_reg_fops);
 	}
 	return 0;
 }
-- 
2.22.0

[PATCH 6/6] dma: qcom: hidma: no need to check return value of debugfs_create functions

[Greg Kroah-Hartman]

When calling debugfs functions, there is no need to ever check the
return value.  The function can work or not, but the code logic should
never do something different based on this.

Also, because there is no need to save the file dentry, remove the
variables that were saving them as they were never even being used once
set.

Cc: Sinan Kaya <okaya@kernel.org>
Cc: Andy Gross <agross@kernel.org>
Cc: David Brown <david.brown@linaro.org>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Vinod Koul <vkoul@kernel.org>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-arm-msm@vger.kernel.org
Cc: dmaengine@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/dma/qcom/hidma.h     |  5 +----
 drivers/dma/qcom/hidma_dbg.c | 37 +++++++-----------------------------
 2 files changed, 8 insertions(+), 34 deletions(-)

diff --git a/drivers/dma/qcom/hidma.h b/drivers/dma/qcom/hidma.h
index 5f9966e82c0b..36357d02333a 100644
--- a/drivers/dma/qcom/hidma.h
+++ b/drivers/dma/qcom/hidma.h
@@ -101,8 +101,6 @@ struct hidma_chan {
 	 * It is used by the DMA complete notification to
 	 * locate the descriptor that initiated the transfer.
 	 */
-	struct dentry			*debugfs;
-	struct dentry			*stats;
 	struct hidma_dev		*dmadev;
 	struct hidma_desc		*running;
 
@@ -134,7 +132,6 @@ struct hidma_dev {
 	struct dma_device		ddev;
 
 	struct dentry			*debugfs;
-	struct dentry			*stats;
 
 	/* sysfs entry for the channel id */
 	struct device_attribute		*chid_attrs;
@@ -166,6 +163,6 @@ irqreturn_t hidma_ll_inthandler(int irq, void *arg);
 irqreturn_t hidma_ll_inthandler_msi(int irq, void *arg, int cause);
 void hidma_cleanup_pending_tre(struct hidma_lldev *llhndl, u8 err_info,
 				u8 err_code);
-int hidma_debug_init(struct hidma_dev *dmadev);
+void hidma_debug_init(struct hidma_dev *dmadev);
 void hidma_debug_uninit(struct hidma_dev *dmadev);
 #endif
diff --git a/drivers/dma/qcom/hidma_dbg.c b/drivers/dma/qcom/hidma_dbg.c
index 9523faf7acdc..994f448b64d8 100644
--- a/drivers/dma/qcom/hidma_dbg.c
+++ b/drivers/dma/qcom/hidma_dbg.c
@@ -146,17 +146,13 @@ void hidma_debug_uninit(struct hidma_dev *dmadev)
 	debugfs_remove_recursive(dmadev->debugfs);
 }
 
-int hidma_debug_init(struct hidma_dev *dmadev)
+void hidma_debug_init(struct hidma_dev *dmadev)
 {
-	int rc = 0;
 	int chidx = 0;
 	struct list_head *position = NULL;
+	struct dentry *dir;
 
 	dmadev->debugfs = debugfs_create_dir(dev_name(dmadev->ddev.dev), NULL);
-	if (!dmadev->debugfs) {
-		rc = -ENODEV;
-		return rc;
-	}
 
 	/* walk through the virtual channel list */
 	list_for_each(position, &dmadev->ddev.channels) {
@@ -165,32 +161,13 @@ int hidma_debug_init(struct hidma_dev *dmadev)
 		chan = list_entry(position, struct hidma_chan,
 				  chan.device_node);
 		sprintf(chan->dbg_name, "chan%d", chidx);
-		chan->debugfs = debugfs_create_dir(chan->dbg_name,
+		dir = debugfs_create_dir(chan->dbg_name,
 						   dmadev->debugfs);
-		if (!chan->debugfs) {
-			rc = -ENOMEM;
-			goto cleanup;
-		}
-		chan->stats = debugfs_create_file("stats", S_IRUGO,
-						  chan->debugfs, chan,
-						  &hidma_chan_fops);
-		if (!chan->stats) {
-			rc = -ENOMEM;
-			goto cleanup;
-		}
+		debugfs_create_file("stats", S_IRUGO, dir, chan,
+				    &hidma_chan_fops);
 		chidx++;
 	}
 
-	dmadev->stats = debugfs_create_file("stats", S_IRUGO,
-					    dmadev->debugfs, dmadev,
-					    &hidma_dma_fops);
-	if (!dmadev->stats) {
-		rc = -ENOMEM;
-		goto cleanup;
-	}
-
-	return 0;
-cleanup:
-	hidma_debug_uninit(dmadev);
-	return rc;
+	debugfs_create_file("stats", S_IRUGO, dmadev->debugfs, dmadev,
+			    &hidma_dma_fops);
 }
-- 
2.22.0

Re: [PATCH 3/6] dma: coh901318: no need to cast away call to debugfs_create_file()

[Linus Walleij]

On Wed, Jun 12, 2019 at 2:26 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:

> No need to check the return value of debugfs_create_file(), so no need
> to provide a fake "cast away" of the return value either.
>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Vinod Koul <vkoul@kernel.org>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: dmaengine@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH 6/6] dma: qcom: hidma: no need to check return value of debugfs_create functions

[Sinan Kaya]

On 6/12/2019 8:25 AM, Greg Kroah-Hartman wrote:
> When calling debugfs functions, there is no need to ever check the
> return value.  The function can work or not, but the code logic should
> never do something different based on this.
> 
> Also, because there is no need to save the file dentry, remove the
> variables that were saving them as they were never even being used once
> set.
> 
> Cc: Sinan Kaya <okaya@kernel.org>
> Cc: Andy Gross <agross@kernel.org>
> Cc: David Brown <david.brown@linaro.org>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Vinod Koul <vkoul@kernel.org>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-arm-msm@vger.kernel.org
> Cc: dmaengine@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Interesting. Wouldn't debugfs_create_file() blow up if dir is NULL
for some reason?


+		debugfs_create_file("stats", S_IRUGO, dir, chan,
+				    &hidma_chan_fops);

Note that code ignores the return value of hidma_debug_init();
It was just trying to do clean up on debugfs failure by calling

	debugfs_remove_recursive(dmadev->debugfs);

Re: [PATCH 6/6] dma: qcom: hidma: no need to check return value of debugfs_create functions

[Greg Kroah-Hartman]

On Wed, Jun 12, 2019 at 11:24:51AM -0400, Sinan Kaya wrote:
> On 6/12/2019 8:25 AM, Greg Kroah-Hartman wrote:
> > When calling debugfs functions, there is no need to ever check the
> > return value.  The function can work or not, but the code logic should
> > never do something different based on this.
> > 
> > Also, because there is no need to save the file dentry, remove the
> > variables that were saving them as they were never even being used once
> > set.
> > 
> > Cc: Sinan Kaya <okaya@kernel.org>
> > Cc: Andy Gross <agross@kernel.org>
> > Cc: David Brown <david.brown@linaro.org>
> > Cc: Dan Williams <dan.j.williams@intel.com>
> > Cc: Vinod Koul <vkoul@kernel.org>
> > Cc: linux-arm-kernel@lists.infradead.org
> > Cc: linux-arm-msm@vger.kernel.org
> > Cc: dmaengine@vger.kernel.org
> > Cc: linux-kernel@vger.kernel.org
> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> 
> Interesting. Wouldn't debugfs_create_file() blow up if dir is NULL
> for some reason?

It will create a file in the root of debugfs.  But how will that happen?
debugfs_create_dir() can not return NULL.

> +		debugfs_create_file("stats", S_IRUGO, dir, chan,
> +				    &hidma_chan_fops);
> 
> Note that code ignores the return value of hidma_debug_init();
> It was just trying to do clean up on debugfs failure by calling
> 
> 	debugfs_remove_recursive(dmadev->debugfs);

Is that a problem?

thanks,

greg k-h

Re: [PATCH 5/6] dma: mic_x100_dma: no need to check return value of debugfs_create functions

[Sudeep Dutt]

On Wed, 2019-06-12 at 14:25 +0200, Greg Kroah-Hartman wrote:
> When calling debugfs functions, there is no need to ever check the
> return value.  The function can work or not, but the code logic should
> never do something different based on this.
> 

Thanks Greg.

Reviewed-by: Sudeep Dutt <sudeep.dutt@intel.com>

> Cc: Sudeep Dutt <sudeep.dutt@intel.com>
> Cc: Ashutosh Dixit <ashutosh.dixit@intel.com>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Vinod Koul <vkoul@kernel.org>
> Cc: dmaengine@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>  drivers/dma/mic_x100_dma.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/dma/mic_x100_dma.c b/drivers/dma/mic_x100_dma.c
> index 6a91e28d537d..584e09661507 100644
> --- a/drivers/dma/mic_x100_dma.c
> +++ b/drivers/dma/mic_x100_dma.c
> @@ -728,10 +728,8 @@ static int mic_dma_driver_probe(struct mbus_device *mbdev)
>  	if (mic_dma_dbg) {
>  		mic_dma_dev->dbg_dir = debugfs_create_dir(dev_name(&mbdev->dev),
>  							  mic_dma_dbg);
> -		if (mic_dma_dev->dbg_dir)
> -			debugfs_create_file("mic_dma_reg", 0444,
> -					    mic_dma_dev->dbg_dir, mic_dma_dev,
> -					    &mic_dma_reg_fops);
> +		debugfs_create_file("mic_dma_reg", 0444, mic_dma_dev->dbg_dir,
> +				    mic_dma_dev, &mic_dma_reg_fops);
>  	}
>  	return 0;
>  }

Re: [PATCH 6/6] dma: qcom: hidma: no need to check return value of debugfs_create functions

[Sinan Kaya]

On 6/12/2019 11:39 AM, Greg Kroah-Hartman wrote:
>> Interesting. Wouldn't debugfs_create_file() blow up if dir is NULL
>> for some reason?
> It will create a file in the root of debugfs.  But how will that happen?
> debugfs_create_dir() can not return NULL.

I see.

> 
>> +		debugfs_create_file("stats", S_IRUGO, dir, chan,
>> +				    &hidma_chan_fops);
>>
>> Note that code ignores the return value of hidma_debug_init();
>> It was just trying to do clean up on debugfs failure by calling
>>
>> 	debugfs_remove_recursive(dmadev->debugfs);
> Is that a problem?

I just wanted to double check. You probably want to remove the return
value on debugfs_create_file() to prevent others from doing the same
thing.

Acked-by: Sinan Kaya <okaya@kernel.org>

Re: [PATCH 6/6] dma: qcom: hidma: no need to check return value of debugfs_create functions

[Greg Kroah-Hartman]

On Wed, Jun 12, 2019 at 12:17:31PM -0400, Sinan Kaya wrote:
> On 6/12/2019 11:39 AM, Greg Kroah-Hartman wrote:
> >> Interesting. Wouldn't debugfs_create_file() blow up if dir is NULL
> >> for some reason?
> > It will create a file in the root of debugfs.  But how will that happen?
> > debugfs_create_dir() can not return NULL.
> 
> I see.
> 
> > 
> >> +		debugfs_create_file("stats", S_IRUGO, dir, chan,
> >> +				    &hidma_chan_fops);
> >>
> >> Note that code ignores the return value of hidma_debug_init();
> >> It was just trying to do clean up on debugfs failure by calling
> >>
> >> 	debugfs_remove_recursive(dmadev->debugfs);
> > Is that a problem?
> 
> I just wanted to double check. You probably want to remove the return
> value on debugfs_create_file() to prevent others from doing the same
> thing.

That is my long-term goal, I have a ways to go still :)

> Acked-by: Sinan Kaya <okaya@kernel.org>

Great, thanks for the review.

greg k-h

Re: [PATCH 1/6] dma: amba-pl08x: no need to cast away call to debugfs_create_file()

[Vinod Koul]

On 12-06-19, 14:25, Greg Kroah-Hartman wrote:
> No need to check the return value of debugfs_create_file(), so no need
> to provide a fake "cast away" of the return value either.

Applied all after fixing the subsystem tag (dmaengine), thanks

-- 
~Vinod

Re: [PATCH 1/6] dma: amba-pl08x: no need to cast away call to debugfs_create_file()

[Greg Kroah-Hartman]

On Fri, Jun 14, 2019 at 11:16:13AM +0530, Vinod Koul wrote:
> On 12-06-19, 14:25, Greg Kroah-Hartman wrote:
> > No need to check the return value of debugfs_create_file(), so no need
> > to provide a fake "cast away" of the return value either.
> 
> Applied all after fixing the subsystem tag (dmaengine), thanks

Sorry about that, and thanks!

greg k-h

Re: [PATCH 1/6] dma: amba-pl08x: no need to cast away call to debugfs_create_file()

[Greg Kroah-Hartman]

On Fri, Jun 14, 2019 at 11:16:13AM +0530, Vinod Koul wrote:
> On 12-06-19, 14:25, Greg Kroah-Hartman wrote:
> > No need to check the return value of debugfs_create_file(), so no need
> > to provide a fake "cast away" of the return value either.
> 
> Applied all after fixing the subsystem tag (dmaengine), thanks

Oops, messed that up, sorry.  Thanks for applying them!

greg k-h

Re: [PATCH 4/6] dma: pxa_dma: no need to check return value of debugfs_create functions

[Robert Jarzmik]

Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:

Hi Greg,

> When calling debugfs functions, there is no need to ever check the
> return value.  The function can work or not, but the code logic should
> never do something different based on this.
>
> Also, because there is no need to save the file dentry, remove the
> variable that was saving it as it was never even being used once set.
>
> Cc: Daniel Mack <daniel@zonque.org>
> Cc: Haojian Zhuang <haojian.zhuang@gmail.com>
> Cc: Robert Jarzmik <robert.jarzmik@free.fr>
> Cc: Vinod Koul <vkoul@kernel.org>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: dmaengine@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>  drivers/dma/pxa_dma.c | 56 +++++++++----------------------------------
>  1 file changed, 11 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/dma/pxa_dma.c b/drivers/dma/pxa_dma.c
> index b429642f3e7a..0f698f49ee26 100644
> --- a/drivers/dma/pxa_dma.c
> +++ b/drivers/dma/pxa_dma.c
> @@ -132,7 +132,6 @@ struct pxad_device {
>  	spinlock_t			phy_lock;	/* Phy association */
>  #ifdef CONFIG_DEBUG_FS
>  	struct dentry			*dbgfs_root;
> -	struct dentry			*dbgfs_state;
>  	struct dentry			**dbgfs_chan;
>  #endif
>  };
> @@ -326,31 +325,18 @@ static struct dentry *pxad_dbg_alloc_chan(struct pxad_device *pdev,
>  					     int ch, struct dentry *chandir)
>  {
>  	char chan_name[11];
> -	struct dentry *chan, *chan_state = NULL, *chan_descr = NULL;
> -	struct dentry *chan_reqs = NULL;
> +	struct dentry *chan;
>  	void *dt;
>  
>  	scnprintf(chan_name, sizeof(chan_name), "%d", ch);
>  	chan = debugfs_create_dir(chan_name, chandir);
>  	dt = (void *)&pdev->phys[ch];
>  
> -	if (chan)
> -		chan_state = debugfs_create_file("state", 0400, chan, dt,
> -						 &chan_state_fops);
> -	if (chan_state)
> -		chan_descr = debugfs_create_file("descriptors", 0400, chan, dt,
> -						 &descriptors_fops);
> -	if (chan_descr)
> -		chan_reqs = debugfs_create_file("requesters", 0400, chan, dt,
> -						&requester_chan_fops);
> -	if (!chan_reqs)
> -		goto err_state;
> +	debugfs_create_file("state", 0400, chan, dt, &chan_state_fops);
> +	debugfs_create_file("descriptors", 0400, chan, dt, &descriptors_fops);
> +	debugfs_create_file("requesters", 0400, chan, dt, &requester_chan_fops);

This is not strictly equivalent.
Imagine that the debugfs_create_dir() fails and returns NULL :
 - in the former case, neither "state", "descriptors" nor "requesters" would be
   created
 - in the new code, "state", "descriptors" nor "requesters" will be created in
   the debugfs root directory

Apart from that it looks fine.

Cheers.

-- 
Robert

Re: [PATCH 4/6] dma: pxa_dma: no need to check return value of debugfs_create functions

[Greg Kroah-Hartman]

On Sat, Aug 10, 2019 at 09:27:26PM +0200, Robert Jarzmik wrote:
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
> 
> Hi Greg,
> 
> > When calling debugfs functions, there is no need to ever check the
> > return value.  The function can work or not, but the code logic should
> > never do something different based on this.
> >
> > Also, because there is no need to save the file dentry, remove the
> > variable that was saving it as it was never even being used once set.
> >
> > Cc: Daniel Mack <daniel@zonque.org>
> > Cc: Haojian Zhuang <haojian.zhuang@gmail.com>
> > Cc: Robert Jarzmik <robert.jarzmik@free.fr>
> > Cc: Vinod Koul <vkoul@kernel.org>
> > Cc: Dan Williams <dan.j.williams@intel.com>
> > Cc: linux-arm-kernel@lists.infradead.org
> > Cc: dmaengine@vger.kernel.org
> > Cc: linux-kernel@vger.kernel.org
> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > ---
> >  drivers/dma/pxa_dma.c | 56 +++++++++----------------------------------
> >  1 file changed, 11 insertions(+), 45 deletions(-)
> >
> > diff --git a/drivers/dma/pxa_dma.c b/drivers/dma/pxa_dma.c
> > index b429642f3e7a..0f698f49ee26 100644
> > --- a/drivers/dma/pxa_dma.c
> > +++ b/drivers/dma/pxa_dma.c
> > @@ -132,7 +132,6 @@ struct pxad_device {
> >  	spinlock_t			phy_lock;	/* Phy association */
> >  #ifdef CONFIG_DEBUG_FS
> >  	struct dentry			*dbgfs_root;
> > -	struct dentry			*dbgfs_state;
> >  	struct dentry			**dbgfs_chan;
> >  #endif
> >  };
> > @@ -326,31 +325,18 @@ static struct dentry *pxad_dbg_alloc_chan(struct pxad_device *pdev,
> >  					     int ch, struct dentry *chandir)
> >  {
> >  	char chan_name[11];
> > -	struct dentry *chan, *chan_state = NULL, *chan_descr = NULL;
> > -	struct dentry *chan_reqs = NULL;
> > +	struct dentry *chan;
> >  	void *dt;
> >  
> >  	scnprintf(chan_name, sizeof(chan_name), "%d", ch);
> >  	chan = debugfs_create_dir(chan_name, chandir);
> >  	dt = (void *)&pdev->phys[ch];
> >  
> > -	if (chan)
> > -		chan_state = debugfs_create_file("state", 0400, chan, dt,
> > -						 &chan_state_fops);
> > -	if (chan_state)
> > -		chan_descr = debugfs_create_file("descriptors", 0400, chan, dt,
> > -						 &descriptors_fops);
> > -	if (chan_descr)
> > -		chan_reqs = debugfs_create_file("requesters", 0400, chan, dt,
> > -						&requester_chan_fops);
> > -	if (!chan_reqs)
> > -		goto err_state;
> > +	debugfs_create_file("state", 0400, chan, dt, &chan_state_fops);
> > +	debugfs_create_file("descriptors", 0400, chan, dt, &descriptors_fops);
> > +	debugfs_create_file("requesters", 0400, chan, dt, &requester_chan_fops);
> 
> This is not strictly equivalent.
> Imagine that the debugfs_create_dir() fails and returns NULL :

How can that happen?

>  - in the former case, neither "state", "descriptors" nor "requesters" would be
>    created
>  - in the new code, "state", "descriptors" nor "requesters" will be created in
>    the debugfs root directory

I agree, but debugfs_create_dir() does not return a NULL on an error
since many kernel releases.  Neither can debugfs_create_file() so really
this test is not working at all as-is :)

thanks,

greg k-h

Re: [PATCH 4/6] dma: pxa_dma: no need to check return value of debugfs_create functions

[Robert Jarzmik]

Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:

> On Sat, Aug 10, 2019 at 09:27:26PM +0200, Robert Jarzmik wrote:
>> Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
>> 
>> This is not strictly equivalent.
>> Imagine that the debugfs_create_dir() fails and returns NULL :
> How can that happen?
Well in v5.0-rc1 that could happen ... unfortunately that's also the code I
checked ...

>>  - in the former case, neither "state", "descriptors" nor "requesters" would be
>>    created
>>  - in the new code, "state", "descriptors" nor "requesters" will be created in
>>    the debugfs root directory
>
> I agree, but debugfs_create_dir() does not return a NULL on an error
> since many kernel releases.  Neither can debugfs_create_file() so really
> this test is not working at all as-is :)
Ah yes, you're right, I wasn't aware of the debugfs changes ...

But checking a bit further, your original mail is 2 monthes old, and this patch
was already merged in v5.2. I probably fell in a time-space anomaly, as I
received this mail only a couple of days ago.

Have a nice day.

-- 
Robert