[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

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

[王明-软件底层技术部]

Hi dan carpenter
Sorry to bother you again, Is there anything wrong with my patch? Can this patch be applied?

Sincere thanks to you,
Wang Ming

-----邮件原件-----
发件人: Dan Carpenter <dan.carpenter@linaro.org> 
发送时间: 2023年7月13日 17:27
收件人: 王明-软件底层技术部 <machel@vivo.com>
抄送: Jon Mason <jdmason@kudzu.us>; Dave Jiang <dave.jiang@intel.com>; Allen Hubbe <allenbh@gmail.com>; Serge Semin <fancer.lancer@gmail.com>; Dan Carpenter <error27@gmail.com>; Jiasheng Jiang <jiasheng@iscas.ac.cn>; ntb@lists.linux.dev; linux-kernel@vger.kernel.org; opensource.kernel <opensource.kernel@vivo.com>
主题: Re: [PATCH v3] ntb: Remove error checking for debugfs_create_dir()

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

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

[王明-软件底层技术部]

Ok, thank you 😊
regards,
Ming

-----邮件原件-----
发件人: Dan Carpenter <dan.carpenter@linaro.org> 
发送时间: 2023年7月13日 18:14
收件人: 王明-软件底层技术部 <machel@vivo.com>
抄送: Jon Mason <jdmason@kudzu.us>; Dave Jiang <dave.jiang@intel.com>; Allen Hubbe <allenbh@gmail.com>; Serge Semin <fancer.lancer@gmail.com>; Dan Carpenter <error27@gmail.com>; Jiasheng Jiang <jiasheng@iscas.ac.cn>; ntb@lists.linux.dev; linux-kernel@vger.kernel.org; opensource.kernel <opensource.kernel@vivo.com>
主题: Re: 回复: [PATCH v3] ntb: Remove error checking for debugfs_create_dir()

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

[Markus Elfring]

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

Please choose a corresponding imperative change suggestion.

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 an omitted error detection?


Would a subject like “[PATCH v4] NTB: ntb_tool: Remove an error check in tool_setup_dbgfs()”
be more appropriate?

Regards,
Markus

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

[Markus Elfring]

> Markus is banned from vger so only people who are on the CC list can
> see his email.

I hope that circumstances will evolve in ways which will help
to reconsider this setting.


>                 A number of maintainers have asked him to stop
> commenting on patches.

Can remaining development concerns and communication difficulties
be adjusted any further?

Regards,
Markus

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

[Markus Elfring]

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


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

Regards,
Markus

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

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

[Markus Elfring]

>>>> 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.

Yes.

Would any other subsystem specification variant or patch title become preferred?


>                                                              Regarding the
> patch log body it is impossible to use the "imperative mood" always in the
> entire text.

This is usual.


>              In this case the explanation is pretty much clear

A terse hint is provided.

I interpret the referenced Linux development documentation in the way
that further imperative actions should be specified also in the “changelog”.


>                                                                and doesn't
> use the patterns like "This patch",

Such a wording occurred also in a few other patches by Vivo developers.


>                                   , or any personal pronouns

This detail seems to be fine (in just one sentence here).


>                                                              so IMO it's
> fine to have the log as is.

I find the change description still improvable.


>> 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.

Do you care if the previously used null pointer check was wrong
(for the variable “tool_dbgfs_topdir”)?
https://elixir.bootlin.com/linux/v6.5-rc1/source/drivers/ntb/test/ntb_tool.c#L1485

Will the source code comment “This modules is useless without dbgfs...”
be reconsidered any more?

Regards,
Markus

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

[Markus Elfring]

>>>> 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.

Yes.

Some patch versions were similarly affected.


> Greg, HCH, Matthew Wilcox and Krzysztof Kozlowski have all asked you to stop.

They responded in this way because of some communication factors
(which might be unclear to some degree).


> Those are respected kernel maintainers.

Yes, of course.


> You should listen to them.

This happens.

But several development discussions provide opportunities for different ideas
and corresponding advices.


> If you see a bug, that's useful.

The change acceptance can grow accordingly.


>                                   But if you're just nitpicking the
> commit message, that's not useful.

I dare to present additional patch review concerns.

It seems that further contributors occasionally dare also to support
such open issues.


>                                     We have explained this many times as
> clearly as we know how.

We came along recurring communication concerns
(for which a consensus is not directly achieved so far).


>> 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.

Would you eventually prefer any other solution for the discussed patch approach?

Regards,
Markus