Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 12 Jul 2023 16:50:34 +0300
From:   Dan Carpenter <dan.carpenter@linaro.org>
To:     Wang Ming <machel@vivo.com>
Cc:     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@vivo.com
Subject: Re: [PATCH v2] ntb:Fix an NULL vs IS_ERR() bug for
 debugfs_create_dir() in tool_setup_dbgfs()
Message-ID: <138ab604-27de-4ac6-88fb-0886ee8f6b6d@kadam.mountain>
References: <20230712124035.7981-1-machel@vivo.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230712124035.7981-1-machel@vivo.com>
Precedence: bulk

On Wed, Jul 12, 2023 at 08:39:59PM +0800, Wang Ming wrote:
> The debugfs_create_dir() function returns error pointers.
> It never returns NULL. Most incorrect error checks were fixed,
> but the one in tool_setup_dbgfs() was forgotten.
> 
> Fix the remaining error check.
> 
> Signed-off-by: Wang Ming <machel@vivo.com>
> ---
>  drivers/ntb/test/ntb_tool.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/ntb/test/ntb_tool.c b/drivers/ntb/test/ntb_tool.c
> index eeeb4b1c97d2..4fa69ea4331d 100644
> --- a/drivers/ntb/test/ntb_tool.c
> +++ b/drivers/ntb/test/ntb_tool.c
> @@ -1495,7 +1495,7 @@ 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)
> +	if (IS_ERR(tc->dbgfs_dir))

No, this will break the driver if debugfs is disabled in the .config.

(I haven't checked, it's possible that this code is #ifdeffed out when
CONFIG_DEBUGFS is disabled so possibly this change is harmless.  But
either way, this change is wrong).

Normally this would be the correct change, but debugfs is weird.  It's
not supposed to be checked for errors in the normal case.  If the
driver pokes around in the debugfs internals then you might need to
check but you should avoid doing that and it doesn't apply here.

As I was saying, this change would normally be the correct thing, and it
used to work.  But we changed it so that now it's impossible to check
for errors.  Making it impossible to check for errors helps people feel
better about deleting error checking.

The correct change is to delete this dead code, but it's a headache
to convince people to it.  It would be better to do it as a mass delete
so everyone can see the thread.  Trying to convince people one by one
does not work.

regards,
dan carpenter