[PATCH -next] crypto: qat - remove redundant null pointer checks in adf_dbgfs_init()

[PATCH -next] crypto: qat - remove redundant null pointer checks in adf_dbgfs_init()

[Li Zetao]

Since the debugfs_create_dir() never returns a null pointer, checking
the return value for a null pointer is redundant, and using IS_ERR is
safe enough.

Signed-off-by: Li Zetao <lizetao1@huawei.com>
---
 drivers/crypto/intel/qat/qat_common/adf_dbgfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/crypto/intel/qat/qat_common/adf_dbgfs.c b/drivers/crypto/intel/qat/qat_common/adf_dbgfs.c
index c42f5c25aabd..ec2c712b9006 100644
--- a/drivers/crypto/intel/qat/qat_common/adf_dbgfs.c
+++ b/drivers/crypto/intel/qat/qat_common/adf_dbgfs.c
@@ -30,7 +30,7 @@ void adf_dbgfs_init(struct adf_accel_dev *accel_dev)
 		 pci_name(accel_dev->accel_pci_dev.pci_dev));
 
 	ret = debugfs_create_dir(name, NULL);
-	if (IS_ERR_OR_NULL(ret))
+	if (IS_ERR(ret))
 		return;
 
 	accel_dev->debugfs_dir = ret;
-- 
2.34.1

Re: [PATCH -next] crypto: qat - remove redundant null pointer checks in adf_dbgfs_init()

[Herbert Xu]

On Tue, Sep 03, 2024 at 10:42:30PM +0800, Li Zetao wrote:
> Since the debugfs_create_dir() never returns a null pointer, checking
> the return value for a null pointer is redundant, and using IS_ERR is
> safe enough.
> 
> Signed-off-by: Li Zetao <lizetao1@huawei.com>
> ---
>  drivers/crypto/intel/qat/qat_common/adf_dbgfs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/crypto/intel/qat/qat_common/adf_dbgfs.c b/drivers/crypto/intel/qat/qat_common/adf_dbgfs.c
> index c42f5c25aabd..ec2c712b9006 100644
> --- a/drivers/crypto/intel/qat/qat_common/adf_dbgfs.c
> +++ b/drivers/crypto/intel/qat/qat_common/adf_dbgfs.c
> @@ -30,7 +30,7 @@ void adf_dbgfs_init(struct adf_accel_dev *accel_dev)
>  		 pci_name(accel_dev->accel_pci_dev.pci_dev));
>  
>  	ret = debugfs_create_dir(name, NULL);
> -	if (IS_ERR_OR_NULL(ret))
> +	if (IS_ERR(ret))
>  		return;

There is no point in creating patches like this.  It doesn't
make the code better at all.  IS_ERR_OR_NULL usually compiles
to a single branch just like IS_ERR.

However, I have to say that this code is actually buggy.  Surely
this function should be passing the error back up so that it does
not try to create anything under the non-existant dbgfs directory?

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH -next] crypto: qat - remove redundant null pointer checks in adf_dbgfs_init()

[Cabiddu, Giovanni]

On Fri, Sep 13, 2024 at 06:19:20PM +0800, Herbert Xu wrote:
> On Tue, Sep 03, 2024 at 10:42:30PM +0800, Li Zetao wrote:
> > Since the debugfs_create_dir() never returns a null pointer, checking
> > the return value for a null pointer is redundant, and using IS_ERR is
> > safe enough.
> > 
> > Signed-off-by: Li Zetao <lizetao1@huawei.com>
> > ---
> >  drivers/crypto/intel/qat/qat_common/adf_dbgfs.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/crypto/intel/qat/qat_common/adf_dbgfs.c b/drivers/crypto/intel/qat/qat_common/adf_dbgfs.c
> > index c42f5c25aabd..ec2c712b9006 100644
> > --- a/drivers/crypto/intel/qat/qat_common/adf_dbgfs.c
> > +++ b/drivers/crypto/intel/qat/qat_common/adf_dbgfs.c
> > @@ -30,7 +30,7 @@ void adf_dbgfs_init(struct adf_accel_dev *accel_dev)
> >  		 pci_name(accel_dev->accel_pci_dev.pci_dev));
> >  
> >  	ret = debugfs_create_dir(name, NULL);
> > -	if (IS_ERR_OR_NULL(ret))
> > +	if (IS_ERR(ret))
> >  		return;
> 
> There is no point in creating patches like this.  It doesn't
> make the code better at all.  IS_ERR_OR_NULL usually compiles
> to a single branch just like IS_ERR.
> 
> However, I have to say that this code is actually buggy.  Surely
> this function should be passing the error back up so that it does
> not try to create anything under the non-existant dbgfs directory?
As I understand it, there is no need to check the return value of
debugfs_create_*() functions. See f0fcf9ade46a ("crypto: qat - no need to check
return value of debugfs_create functions"), where all checks after the
debugfs_create_*() were removed.

In this particular case, the check is present only to avoid attempting to
create attributes if the directory is missing, since we know such an
attempt will fail.


Regards,

-- 
Giovanni

Re: [PATCH -next] crypto: qat - remove redundant null pointer checks in adf_dbgfs_init()

[Herbert Xu]

On Fri, Sep 13, 2024 at 03:52:52PM +0100, Cabiddu, Giovanni wrote:
>
> As I understand it, there is no need to check the return value of
> debugfs_create_*() functions. See f0fcf9ade46a ("crypto: qat - no need to check
> return value of debugfs_create functions"), where all checks after the
> debugfs_create_*() were removed.

Right.

> In this particular case, the check is present only to avoid attempting to
> create attributes if the directory is missing, since we know such an
> attempt will fail.

I think this is still buggy.  That if statement should be removed
as otherwise subsequent calls to debugfs_create_file will provide a
NULL parent dentry instead of an error parent dentry.  This causes
debugfs to do things differently.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH -next] crypto: qat - remove redundant null pointer checks in adf_dbgfs_init()

[Greg Kroah-Hartman]

On Sat, Sep 14, 2024 at 04:40:05PM +0800, Herbert Xu wrote:
> On Fri, Sep 13, 2024 at 03:52:52PM +0100, Cabiddu, Giovanni wrote:
> >
> > As I understand it, there is no need to check the return value of
> > debugfs_create_*() functions. See f0fcf9ade46a ("crypto: qat - no need to check
> > return value of debugfs_create functions"), where all checks after the
> > debugfs_create_*() were removed.
> 
> Right.
> 
> > In this particular case, the check is present only to avoid attempting to
> > create attributes if the directory is missing, since we know such an
> > attempt will fail.
> 
> I think this is still buggy.  That if statement should be removed
> as otherwise subsequent calls to debugfs_create_file will provide a
> NULL parent dentry instead of an error parent dentry.  This causes
> debugfs to do things differently.

debugfs, if something goes wrong, will return a real error, never NULL,
so any return value from a call can be passed back in.

Ideally I want to make debugfs return values just a "opaque token" so
please just treat it like that (and ignore the fact that it's a dentry.)

thanks,

greg k-h

Re: [PATCH -next] crypto: qat - remove redundant null pointer checks in adf_dbgfs_init()

[Herbert Xu]

On Sat, Sep 14, 2024 at 10:50:33AM +0200, Greg Kroah-Hartman wrote:
>
> > I think this is still buggy.  That if statement should be removed
> > as otherwise subsequent calls to debugfs_create_file will provide a
> > NULL parent dentry instead of an error parent dentry.  This causes
> > debugfs to do things differently.
> 
> debugfs, if something goes wrong, will return a real error, never NULL,
> so any return value from a call can be passed back in.

Right, that's why we should remove the if statement so that the
error is saved and can then be passed back into the next debugfs
call.

With the error-checking if statement there, the error is discarded
and the next debugfs call from this driver will simply get a NULL
parent dentry.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH -next] crypto: qat - remove redundant null pointer checks in adf_dbgfs_init()

[Greg Kroah-Hartman]

On Sat, Sep 14, 2024 at 07:40:31PM +0800, Herbert Xu wrote:
> On Sat, Sep 14, 2024 at 10:50:33AM +0200, Greg Kroah-Hartman wrote:
> >
> > > I think this is still buggy.  That if statement should be removed
> > > as otherwise subsequent calls to debugfs_create_file will provide a
> > > NULL parent dentry instead of an error parent dentry.  This causes
> > > debugfs to do things differently.
> > 
> > debugfs, if something goes wrong, will return a real error, never NULL,
> > so any return value from a call can be passed back in.
> 
> Right, that's why we should remove the if statement so that the
> error is saved and can then be passed back into the next debugfs
> call.
> 
> With the error-checking if statement there, the error is discarded
> and the next debugfs call from this driver will simply get a NULL
> parent dentry.

Sorry, but yes, we are in agreement here, sorry, been reviewing a lot of
these "clean up" fixes that were wrong and got them confused.

thanks,

greg k-h

[PATCH] crypto: qat - remove check after debugfs_create_dir()

[Cabiddu, Giovanni]

On Sat, Sep 14, 2024 at 03:58:24PM +0200, Greg Kroah-Hartman wrote:
> On Sat, Sep 14, 2024 at 07:40:31PM +0800, Herbert Xu wrote:
> > On Sat, Sep 14, 2024 at 10:50:33AM +0200, Greg Kroah-Hartman wrote:
> > >
> > > > I think this is still buggy.  That if statement should be removed
> > > > as otherwise subsequent calls to debugfs_create_file will provide a
> > > > NULL parent dentry instead of an error parent dentry.  This causes
> > > > debugfs to do things differently.
> > > 
> > > debugfs, if something goes wrong, will return a real error, never NULL,
> > > so any return value from a call can be passed back in.
> > 
> > Right, that's why we should remove the if statement so that the
> > error is saved and can then be passed back into the next debugfs
> > call.
> > 
> > With the error-checking if statement there, the error is discarded
> > and the next debugfs call from this driver will simply get a NULL
> > parent dentry.
> 
> Sorry, but yes, we are in agreement here, sorry, been reviewing a lot of
> these "clean up" fixes that were wrong and got them confused.
Thanks Herbert and Greg for following up on this.
Here is a fix.

---8<---
The debugfs functions are guaranteed to return a valid error code
instead of NULL upon failure. Consequently, the driver can directly
propagate any error returned without additional checks.

Remove the unnecessary `if` statement after debugfs_create_dir(). If
this function fails, the error code is stored in accel_dev->debugfs_dir
and utilized in subsequent debugfs calls.

Additionally, since accel_dev->debugfs_dir is assured to be non-NULL,
remove the superfluous NULL pointer checks within the adf_dbgfs_add()
and adf_dbgfs_rm().

Fixes: 9260db6640a6 ("crypto: qat - move dbgfs init to separate file")
Signed-off-by: Giovanni Cabiddu <giovanni.cabiddu@intel.com>
---
 drivers/crypto/intel/qat/qat_common/adf_dbgfs.c | 13 +------------
 1 file changed, 1 insertion(+), 12 deletions(-)

diff --git a/drivers/crypto/intel/qat/qat_common/adf_dbgfs.c b/drivers/crypto/intel/qat/qat_common/adf_dbgfs.c
index c42f5c25aabd..4c11ad1ebcf0 100644
--- a/drivers/crypto/intel/qat/qat_common/adf_dbgfs.c
+++ b/drivers/crypto/intel/qat/qat_common/adf_dbgfs.c
@@ -22,18 +22,13 @@
 void adf_dbgfs_init(struct adf_accel_dev *accel_dev)
 {
 	char name[ADF_DEVICE_NAME_LENGTH];
-	void *ret;
 
 	/* Create dev top level debugfs entry */
 	snprintf(name, sizeof(name), "%s%s_%s", ADF_DEVICE_NAME_PREFIX,
 		 accel_dev->hw_device->dev_class->name,
 		 pci_name(accel_dev->accel_pci_dev.pci_dev));
 
-	ret = debugfs_create_dir(name, NULL);
-	if (IS_ERR_OR_NULL(ret))
-		return;
-
-	accel_dev->debugfs_dir = ret;
+	accel_dev->debugfs_dir = debugfs_create_dir(name, NULL);
 
 	adf_cfg_dev_dbgfs_add(accel_dev);
 }
@@ -59,9 +54,6 @@ EXPORT_SYMBOL_GPL(adf_dbgfs_exit);
  */
 void adf_dbgfs_add(struct adf_accel_dev *accel_dev)
 {
-	if (!accel_dev->debugfs_dir)
-		return;
-
 	if (!accel_dev->is_vf) {
 		adf_fw_counters_dbgfs_add(accel_dev);
 		adf_heartbeat_dbgfs_add(accel_dev);
@@ -77,9 +69,6 @@ void adf_dbgfs_add(struct adf_accel_dev *accel_dev)
  */
 void adf_dbgfs_rm(struct adf_accel_dev *accel_dev)
 {
-	if (!accel_dev->debugfs_dir)
-		return;
-
 	if (!accel_dev->is_vf) {
 		adf_tl_dbgfs_rm(accel_dev);
 		adf_cnv_dbgfs_rm(accel_dev);
-- 
2.46.0

Re: [PATCH] crypto: qat - remove check after debugfs_create_dir()

[Herbert Xu]

On Mon, Sep 16, 2024 at 10:42:51AM +0100, Cabiddu, Giovanni wrote:
> The debugfs functions are guaranteed to return a valid error code
> instead of NULL upon failure. Consequently, the driver can directly
> propagate any error returned without additional checks.
> 
> Remove the unnecessary `if` statement after debugfs_create_dir(). If
> this function fails, the error code is stored in accel_dev->debugfs_dir
> and utilized in subsequent debugfs calls.
> 
> Additionally, since accel_dev->debugfs_dir is assured to be non-NULL,
> remove the superfluous NULL pointer checks within the adf_dbgfs_add()
> and adf_dbgfs_rm().
> 
> Fixes: 9260db6640a6 ("crypto: qat - move dbgfs init to separate file")
> Signed-off-by: Giovanni Cabiddu <giovanni.cabiddu@intel.com>
> ---
>  drivers/crypto/intel/qat/qat_common/adf_dbgfs.c | 13 +------------
>  1 file changed, 1 insertion(+), 12 deletions(-)

Patch applied.  Thanks.
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt