| Message ID | 20240903144230.2005570-1-lizetao1@huawei.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | [-next] crypto: qat - remove redundant null pointer checks in adf_dbgfs_init() | expand |
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,
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,
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,
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
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,
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
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;
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(-)