Linux-ARM-MSM Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 6/6] dma: qcom: hidma: no need to check return value of debugfs_create functions
       [not found] <20190612122557.24158-1-gregkh@linuxfoundation.org>
@ 2019-06-12 12:25 ` Greg Kroah-Hartman
  2019-06-12 15:24   ` Sinan Kaya
  0 siblings, 1 reply; 5+ messages in thread
From: Greg Kroah-Hartman @ 2019-06-12 12:25 UTC (permalink / raw)
  To: dan.j.williams, vkoul
  Cc: Greg Kroah-Hartman, Sinan Kaya, Andy Gross, David Brown,
	linux-arm-kernel, linux-arm-msm, dmaengine, linux-kernel

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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 6/6] dma: qcom: hidma: no need to check return value of debugfs_create functions
  2019-06-12 12:25 ` [PATCH 6/6] dma: qcom: hidma: no need to check return value of debugfs_create functions Greg Kroah-Hartman
@ 2019-06-12 15:24   ` Sinan Kaya
  2019-06-12 15:39     ` Greg Kroah-Hartman
  0 siblings, 1 reply; 5+ messages in thread
From: Sinan Kaya @ 2019-06-12 15:24 UTC (permalink / raw)
  To: Greg Kroah-Hartman, dan.j.williams, vkoul
  Cc: Andy Gross, David Brown, linux-arm-kernel, linux-arm-msm,
	dmaengine, linux-kernel

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 6/6] dma: qcom: hidma: no need to check return value of debugfs_create functions
  2019-06-12 15:24   ` Sinan Kaya
@ 2019-06-12 15:39     ` Greg Kroah-Hartman
  2019-06-12 16:17       ` Sinan Kaya
  0 siblings, 1 reply; 5+ messages in thread
From: Greg Kroah-Hartman @ 2019-06-12 15:39 UTC (permalink / raw)
  To: Sinan Kaya
  Cc: dan.j.williams, vkoul, Andy Gross, David Brown, linux-arm-kernel,
	linux-arm-msm, dmaengine, linux-kernel

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 6/6] dma: qcom: hidma: no need to check return value of debugfs_create functions
  2019-06-12 15:39     ` Greg Kroah-Hartman
@ 2019-06-12 16:17       ` Sinan Kaya
  2019-06-12 16:47         ` Greg Kroah-Hartman
  0 siblings, 1 reply; 5+ messages in thread
From: Sinan Kaya @ 2019-06-12 16:17 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: dan.j.williams, vkoul, Andy Gross, David Brown, linux-arm-kernel,
	linux-arm-msm, dmaengine, linux-kernel

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>

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 6/6] dma: qcom: hidma: no need to check return value of debugfs_create functions
  2019-06-12 16:17       ` Sinan Kaya
@ 2019-06-12 16:47         ` Greg Kroah-Hartman
  0 siblings, 0 replies; 5+ messages in thread
From: Greg Kroah-Hartman @ 2019-06-12 16:47 UTC (permalink / raw)
  To: Sinan Kaya
  Cc: dan.j.williams, vkoul, Andy Gross, David Brown, linux-arm-kernel,
	linux-arm-msm, dmaengine, linux-kernel

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20190612122557.24158-1-gregkh@linuxfoundation.org>
2019-06-12 12:25 ` [PATCH 6/6] dma: qcom: hidma: no need to check return value of debugfs_create functions Greg Kroah-Hartman
2019-06-12 15:24   ` Sinan Kaya
2019-06-12 15:39     ` Greg Kroah-Hartman
2019-06-12 16:17       ` Sinan Kaya
2019-06-12 16:47         ` Greg Kroah-Hartman

Linux-ARM-MSM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-arm-msm/0 linux-arm-msm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-arm-msm linux-arm-msm/ https://lore.kernel.org/linux-arm-msm \
		linux-arm-msm@vger.kernel.org linux-arm-msm@archiver.kernel.org
	public-inbox-index linux-arm-msm


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-arm-msm


AGPL code for this site: git clone https://public-inbox.org/ public-inbox