[PATCH v3] ntb: Remove error checking for debugfs_create_dir()

[PATCH v3] ntb: Remove error checking for debugfs_create_dir()

[Wang Ming]

It is expected that most callers should _ignore_ the errors
return by debugfs_create_dir() in tool_setup_dbgfs()

Signed-off-by: Wang Ming <machel@vivo.com>
---
 drivers/ntb/test/ntb_tool.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/ntb/test/ntb_tool.c b/drivers/ntb/test/ntb_tool.c
index eeeb4b1c97d2..e0acc11d29ba 100644
--- a/drivers/ntb/test/ntb_tool.c
+++ b/drivers/ntb/test/ntb_tool.c
@@ -1495,8 +1495,6 @@ static void tool_setup_dbgfs(struct tool_ctx *tc)
 
 	tc->dbgfs_dir = debugfs_create_dir(dev_name(&tc->ntb->dev),
 					   tool_dbgfs_topdir);
-	if (!tc->dbgfs_dir)
-		return;
 
 	debugfs_create_file("port", 0600, tc->dbgfs_dir,
 			    tc, &tool_port_fops);
-- 
2.25.1

Re: [PATCH v3] ntb: Remove error checking for debugfs_create_dir()

[Dan Carpenter]

On Thu, Jul 13, 2023 at 04:56:06PM +0800, Wang Ming wrote:
> It is expected that most callers should _ignore_ the errors
> return by debugfs_create_dir() in tool_setup_dbgfs()
> 
> Signed-off-by: Wang Ming <machel@vivo.com>

Reviewed-by: Dan Carpenter <dan.carpenter@linaro.org>

regards,
dan carpenter

Re: 回复: [PATCH v3] ntb: Remove error checking for debugfs_create_dir()

[Dan Carpenter]

On Thu, Jul 13, 2023 at 10:07:45AM +0000, 王明-软件底层技术部 wrote:
> Hi dan carpenter
> Sorry to bother you again, Is there anything wrong with my patch? Can this patch be applied?
> 

I'm not the NTB maintainer.  I don't see a reason why it can't be
applied but, please, wait for two weeks for maintainer feedback.

regards,
dan carpenter

Re: [PATCH v3] ntb: Remove error checking for debugfs_create_dir()

[Dave Jiang]

On 7/13/23 01:56, Wang Ming wrote:
> It is expected that most callers should _ignore_ the errors
> return by debugfs_create_dir() in tool_setup_dbgfs()
> 
> Signed-off-by: Wang Ming <machel@vivo.com>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> ---
>   drivers/ntb/test/ntb_tool.c | 2 --
>   1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/ntb/test/ntb_tool.c b/drivers/ntb/test/ntb_tool.c
> index eeeb4b1c97d2..e0acc11d29ba 100644
> --- a/drivers/ntb/test/ntb_tool.c
> +++ b/drivers/ntb/test/ntb_tool.c
> @@ -1495,8 +1495,6 @@ static void tool_setup_dbgfs(struct tool_ctx *tc)
>   
>   	tc->dbgfs_dir = debugfs_create_dir(dev_name(&tc->ntb->dev),
>   					   tool_dbgfs_topdir);
> -	if (!tc->dbgfs_dir)
> -		return;
>   
>   	debugfs_create_file("port", 0600, tc->dbgfs_dir,
>   			    tc, &tool_port_fops);

Re: [PATCH v3] ntb: Remove error checking for debugfs_create_dir()

[Dan Carpenter]

Markus is banned from vger so only people who are on the CC list can
see his email.  A number of maintainers have asked him to stop
commenting on patches.

regards,
dan carpenter

Re: [PATCH v3] ntb: Remove error checking for debugfs_create_dir()

[Serge Semin]

On Thu, Jul 13, 2023 at 04:56:06PM +0800, Wang Ming wrote:
> It is expected that most callers should _ignore_ the errors
> return by debugfs_create_dir() in tool_setup_dbgfs()
> 
> Signed-off-by: Wang Ming <machel@vivo.com>

The patch itself is correct for sure:
Reviewed-by: Serge Semin <fancer.lancer@gmail.com>

As a side note IMO I would have converted the driver to successfully
probe an NTB device only if DebugFS is enabled and all the DebugFS-nodes
are successfully created. That's because the driver fully relies on
the DebugFS nodes to activate its functionality. Without them being
available it's absolutely useless. The way the test-driver is utilized
is demonstrated for instance in the NTB-test script:
tools/testing/selftests/ntb/ntb_test.sh .

* It concerns all the NTB test drivers...

-Serge(y)

> ---
>  drivers/ntb/test/ntb_tool.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/ntb/test/ntb_tool.c b/drivers/ntb/test/ntb_tool.c
> index eeeb4b1c97d2..e0acc11d29ba 100644
> --- a/drivers/ntb/test/ntb_tool.c
> +++ b/drivers/ntb/test/ntb_tool.c
> @@ -1495,8 +1495,6 @@ static void tool_setup_dbgfs(struct tool_ctx *tc)
>  
>  	tc->dbgfs_dir = debugfs_create_dir(dev_name(&tc->ntb->dev),
>  					   tool_dbgfs_topdir);
> -	if (!tc->dbgfs_dir)
> -		return;
>  
>  	debugfs_create_file("port", 0600, tc->dbgfs_dir,
>  			    tc, &tool_port_fops);
> -- 
> 2.25.1
> 

Re: [PATCH v3] ntb: Remove error checking for debugfs_create_dir()

[Serge Semin]

On Fri, Jul 14, 2023 at 02:44:11PM +0200, Markus Elfring wrote:
> > > It is expected that most callers should _ignore_ the errors
> > > return by debugfs_create_dir() in tool_setup_dbgfs()
> …
> > The patch itself is correct for sure:
> > Reviewed-by: Serge Semin <fancer.lancer@gmail.com>
> 

> How does such information fit to the Linux development requirement
> for imperative change descriptions?
> 
> See also:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.5-rc1#n94

Well, the patch author does follow that rule in the subject. Regarding the
patch log body it is impossible to use the "imperative mood" always in the
entire text. In this case the explanation is pretty much clear and doesn't
use the patterns like "This patch", or any personal pronouns so IMO it's
fine to have the log as is.

> 
> 
> How do you think about to add the tag “Fixes” because of the deletion
> of an inappropriate error detection?

It doesn't fix any real bug. So there is no need in the tag.

-Serge(y)

> 
> Regards,
> Markus

Re: [PATCH v3] ntb: Remove error checking for debugfs_create_dir()

[Dan Carpenter]

On Fri, Jul 14, 2023 at 02:44:11PM +0200, Markus Elfring wrote:
> > > It is expected that most callers should _ignore_ the errors
> > > return by debugfs_create_dir() in tool_setup_dbgfs()
> …
> > The patch itself is correct for sure:
> > Reviewed-by: Serge Semin <fancer.lancer@gmail.com>
> 
> How does such information fit to the Linux development requirement
> for imperative change descriptions?
> 

Markus, you already replied to this thread.

Greg, HCH, Matthew Wilcox and Krzysztof Kozlowski have all asked you to
stop.  Those are respected kernel maintainers.  You should listen to
them.

If you see a bug, that's useful.  But if you're just nitpicking the
commit message, that's not useful.  We have explained this many times as
clearly as we know how.

> See also:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.5-rc1#n94
> 
> 
> How do you think about to add the tag “Fixes” because of the deletion
> of an inappropriate error detection?

No.  It's not a bug fix so a Fixes tag is innappropriate.

regards,
dan carpenter