debugfs_create_dir() returns a pointers to a dentry objects. On failures
it returns errors. Currently the values returned to visornic_probe()
seem to be tested for being equal to NULL in case of failures.
Properly test with "if (IS_ERR())" and then assign the correct error
value to the "err" variable.
Signed-off-by: Fabio M. De Francesco <[email protected]>
---
drivers/staging/unisys/visornic/visornic_main.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/unisys/visornic/visornic_main.c b/drivers/staging/unisys/visornic/visornic_main.c
index 643432458105..58d03f3d3173 100644
--- a/drivers/staging/unisys/visornic/visornic_main.c
+++ b/drivers/staging/unisys/visornic/visornic_main.c
@@ -1922,11 +1922,11 @@ static int visornic_probe(struct visor_device *dev)
/* create debug/sysfs directories */
devdata->eth_debugfs_dir = debugfs_create_dir(netdev->name,
visornic_debugfs_dir);
- if (!devdata->eth_debugfs_dir) {
+ if (IS_ERR(devdata->eth_debugfs_dir)) {
dev_err(&dev->device,
"%s debugfs_create_dir %s failed\n",
__func__, netdev->name);
- err = -ENOMEM;
+ err = PTR_ERR(devdata->eth_debugfs_dir);
goto cleanup_register_netdev;
}
--
2.34.1
On Tue, Mar 22, 2022 at 09:38:58AM +0100, Fabio M. De Francesco wrote:
> debugfs_create_dir() returns a pointers to a dentry objects. On failures
> it returns errors. Currently the values returned to visornic_probe()
> seem to be tested for being equal to NULL in case of failures.
>
> Properly test with "if (IS_ERR())" and then assign the correct error
> value to the "err" variable.
>
> Signed-off-by: Fabio M. De Francesco <[email protected]>
> ---
> drivers/staging/unisys/visornic/visornic_main.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/unisys/visornic/visornic_main.c b/drivers/staging/unisys/visornic/visornic_main.c
> index 643432458105..58d03f3d3173 100644
> --- a/drivers/staging/unisys/visornic/visornic_main.c
> +++ b/drivers/staging/unisys/visornic/visornic_main.c
> @@ -1922,11 +1922,11 @@ static int visornic_probe(struct visor_device *dev)
> /* create debug/sysfs directories */
> devdata->eth_debugfs_dir = debugfs_create_dir(netdev->name,
> visornic_debugfs_dir);
> - if (!devdata->eth_debugfs_dir) {
> + if (IS_ERR(devdata->eth_debugfs_dir)) {
We really shouldn't be checking this value at all. There's no reason to
check the return value of a debugfs_* call. Can you fix up the code to
do that instead?
thanks,
greg k-h
On Tue, Mar 22, 2022 at 09:38:58AM +0100, Fabio M. De Francesco wrote:
> debugfs_create_dir() returns a pointers to a dentry objects. On failures
> it returns errors. Currently the values returned to visornic_probe()
> seem to be tested for being equal to NULL in case of failures.
>
> Properly test with "if (IS_ERR())" and then assign the correct error
> value to the "err" variable.
>
> Signed-off-by: Fabio M. De Francesco <[email protected]>
> ---
> drivers/staging/unisys/visornic/visornic_main.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/unisys/visornic/visornic_main.c b/drivers/staging/unisys/visornic/visornic_main.c
> index 643432458105..58d03f3d3173 100644
> --- a/drivers/staging/unisys/visornic/visornic_main.c
> +++ b/drivers/staging/unisys/visornic/visornic_main.c
> @@ -1922,11 +1922,11 @@ static int visornic_probe(struct visor_device *dev)
> /* create debug/sysfs directories */
> devdata->eth_debugfs_dir = debugfs_create_dir(netdev->name,
> visornic_debugfs_dir);
> - if (!devdata->eth_debugfs_dir) {
> + if (IS_ERR(devdata->eth_debugfs_dir)) {
Normally I would say to just delete the error handling. But in this
case you can delete the whole devdata->eth_debugfs_dir and the related
code. It's not used at all.
regards,
dan carpenter
On Tue, Mar 22, 2022 at 09:38:58AM +0100, Fabio M. De Francesco wrote:
> debugfs_create_dir() returns a pointers to a dentry objects. On failures
> it returns errors. Currently the values returned to visornic_probe()
> seem to be tested for being equal to NULL in case of failures.
>
> Properly test with "if (IS_ERR())" and then assign the correct error
> value to the "err" variable.
>
> Signed-off-by: Fabio M. De Francesco <[email protected]>
> ---
> drivers/staging/unisys/visornic/visornic_main.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/unisys/visornic/visornic_main.c b/drivers/staging/unisys/visornic/visornic_main.c
> index 643432458105..58d03f3d3173 100644
> --- a/drivers/staging/unisys/visornic/visornic_main.c
> +++ b/drivers/staging/unisys/visornic/visornic_main.c
> @@ -1922,11 +1922,11 @@ static int visornic_probe(struct visor_device *dev)
> /* create debug/sysfs directories */
> devdata->eth_debugfs_dir = debugfs_create_dir(netdev->name,
> visornic_debugfs_dir);
> - if (!devdata->eth_debugfs_dir) {
> + if (IS_ERR(devdata->eth_debugfs_dir)) {
> dev_err(&dev->device,
> "%s debugfs_create_dir %s failed\n",
> __func__, netdev->name);
> - err = -ENOMEM;
> + err = PTR_ERR(devdata->eth_debugfs_dir);
> goto cleanup_register_netdev;
> }
Also, why does this variable have to be saved off at all? It should
only be used later when removing the directory, and we can just look it
up at that point in time, right?
Also, what happens if the network device name changes?
thanks,
greg k-h
On Tue, Mar 22, 2022 at 11:49:28AM +0300, Dan Carpenter wrote:
> On Tue, Mar 22, 2022 at 09:38:58AM +0100, Fabio M. De Francesco wrote:
> > debugfs_create_dir() returns a pointers to a dentry objects. On failures
> > it returns errors. Currently the values returned to visornic_probe()
> > seem to be tested for being equal to NULL in case of failures.
> >
> > Properly test with "if (IS_ERR())" and then assign the correct error
> > value to the "err" variable.
> >
> > Signed-off-by: Fabio M. De Francesco <[email protected]>
> > ---
> > drivers/staging/unisys/visornic/visornic_main.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/staging/unisys/visornic/visornic_main.c b/drivers/staging/unisys/visornic/visornic_main.c
> > index 643432458105..58d03f3d3173 100644
> > --- a/drivers/staging/unisys/visornic/visornic_main.c
> > +++ b/drivers/staging/unisys/visornic/visornic_main.c
> > @@ -1922,11 +1922,11 @@ static int visornic_probe(struct visor_device *dev)
> > /* create debug/sysfs directories */
> > devdata->eth_debugfs_dir = debugfs_create_dir(netdev->name,
> > visornic_debugfs_dir);
> > - if (!devdata->eth_debugfs_dir) {
> > + if (IS_ERR(devdata->eth_debugfs_dir)) {
>
> Normally I would say to just delete the error handling.
In the normal case checking debugfs functions for errors is not
required. The users all have checks built in. The exception is when
the driver dereferences devdata->eth_debugfs_dir itself. So when you
make these kinds of changes do a grep for eth_debugfs_dir to make sure
it's not dereferenced, but normally it is not.
But again, here, delete everything.
regards,
dan carpenter
On marted? 22 marzo 2022 09:49:28 CET Dan Carpenter wrote:
> On Tue, Mar 22, 2022 at 09:38:58AM +0100, Fabio M. De Francesco wrote:
> > debugfs_create_dir() returns a pointers to a dentry objects. On failures
> > it returns errors. Currently the values returned to visornic_probe()
> > seem to be tested for being equal to NULL in case of failures.
> >
> > Properly test with "if (IS_ERR())" and then assign the correct error
> > value to the "err" variable.
> >
> > Signed-off-by: Fabio M. De Francesco <[email protected]>
> > ---
> > drivers/staging/unisys/visornic/visornic_main.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/staging/unisys/visornic/visornic_main.c b/drivers/staging/unisys/visornic/visornic_main.c
> > index 643432458105..58d03f3d3173 100644
> > --- a/drivers/staging/unisys/visornic/visornic_main.c
> > +++ b/drivers/staging/unisys/visornic/visornic_main.c
> > @@ -1922,11 +1922,11 @@ static int visornic_probe(struct visor_device *dev)
> > /* create debug/sysfs directories */
> > devdata->eth_debugfs_dir = debugfs_create_dir(netdev->name,
> > visornic_debugfs_dir);
> > - if (!devdata->eth_debugfs_dir) {
> > + if (IS_ERR(devdata->eth_debugfs_dir)) {
>
> Normally I would say to just delete the error handling. But in this
> case you can delete the whole devdata->eth_debugfs_dir and the related
> code. It's not used at all.
>
> regards,
> dan carpenter
>
Dear Dan,
I think that you are right and the whole thing should be deleted. There are
a couple more of these kinds of bad error handling in the Unisys driver.
However, soon after sending this patch I noticed that David Kershner email
account at unisys.com is not working anymore. Furthermore, I am recalling
that in 2021 I made a conversion from IDA to XArray for the whole visorhba
driver and nobody at Unisys replied. After one month in queue, Greg decided
to apply my patches while noticing that nobody from Unisys cares.
In summation, probably I'll follow your suggestion for this case and the
other bugs that I was about to fix but I'm not sure at all at the moment.
What I mean is: if people at Unisys don't care, why should I?
Thanks for your review, Dan.
Regards,
Fabio M. De Francesco
P.S.: I found this and the other bugs in this Unisys driver with the help
of Smatch. Thank you for this precious tool :)
On marted? 22 marzo 2022 09:47:29 CET Greg Kroah-Hartman wrote:
> On Tue, Mar 22, 2022 at 09:38:58AM +0100, Fabio M. De Francesco wrote:
> > debugfs_create_dir() returns a pointers to a dentry objects. On failures
> > it returns errors. Currently the values returned to visornic_probe()
> > seem to be tested for being equal to NULL in case of failures.
> >
> > Properly test with "if (IS_ERR())" and then assign the correct error
> > value to the "err" variable.
> >
> > Signed-off-by: Fabio M. De Francesco <[email protected]>
> > ---
> > drivers/staging/unisys/visornic/visornic_main.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/staging/unisys/visornic/visornic_main.c b/drivers/staging/unisys/visornic/visornic_main.c
> > index 643432458105..58d03f3d3173 100644
> > --- a/drivers/staging/unisys/visornic/visornic_main.c
> > +++ b/drivers/staging/unisys/visornic/visornic_main.c
> > @@ -1922,11 +1922,11 @@ static int visornic_probe(struct visor_device *dev)
> > /* create debug/sysfs directories */
> > devdata->eth_debugfs_dir = debugfs_create_dir(netdev->name,
> > visornic_debugfs_dir);
> > - if (!devdata->eth_debugfs_dir) {
> > + if (IS_ERR(devdata->eth_debugfs_dir)) {
>
> We really shouldn't be checking this value at all. There's no reason to
> check the return value of a debugfs_* call. Can you fix up the code to
> do that instead?
>
> thanks,
>
> greg k-h
>
Yes I'll do the work that you requested by this weekend, notwithstanding
what I replied to Dan.
While I reiterate all my considerations, it seems that, if not the people
from Unisys, someone else still cares about this driver :)
Thanks,
Fabio
On Tue, Mar 29, 2022 at 07:00:49PM +0200, Fabio M. De Francesco wrote:
> On marted? 22 marzo 2022 09:38:58 CEST Fabio M. De Francesco wrote:
> > debugfs_create_dir() returns a pointers to a dentry objects. On failures
> > it returns errors. Currently the values returned to visornic_probe()
> > seem to be tested for being equal to NULL in case of failures.
> >
> > Properly test with "if (IS_ERR())" and then assign the correct error
> > value to the "err" variable.
> >
> > Signed-off-by: Fabio M. De Francesco <[email protected]>
> > ---
> > drivers/staging/unisys/visornic/visornic_main.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/staging/unisys/visornic/visornic_main.c b/drivers/staging/unisys/visornic/visornic_main.c
> > index 643432458105..58d03f3d3173 100644
> > --- a/drivers/staging/unisys/visornic/visornic_main.c
> > +++ b/drivers/staging/unisys/visornic/visornic_main.c
> > @@ -1922,11 +1922,11 @@ static int visornic_probe(struct visor_device *dev)
> > /* create debug/sysfs directories */
> > devdata->eth_debugfs_dir = debugfs_create_dir(netdev->name,
> > visornic_debugfs_dir);
> > - if (!devdata->eth_debugfs_dir) {
> > + if (IS_ERR(devdata->eth_debugfs_dir)) {
> > dev_err(&dev->device,
> > "%s debugfs_create_dir %s failed\n",
> > __func__, netdev->name);
> > - err = -ENOMEM;
> > + err = PTR_ERR(devdata->eth_debugfs_dir);
> > goto cleanup_register_netdev;
> > }
> >
> > --
> > 2.34.1
> >
> Hi Greg, Dan,
>
> Now I have time to rework this patch but, if I'm not misunderstanding, you
> asked for two contrasting works to do here...
>
> Dan wrote that "[in] this case you can delete the whole devdata->eth_debugfs_dir
> and the related code.".
>
> Greg wrote that "We really shouldn't be checking this value at all. There's
> no reason to check the return value of a debugfs_* call. Can you fix up the code to
> do that instead?".
>
> I'm confused because they look like two incompatible requests. What should I do?
Greg is saying delete the tests and the error handling. But I am
saying delete the tests, the error handling, the devdata->eth_debugfs_dir
related code and the call to debugfs_create_dir().
There is no conflict. Delete it all.
regards,
dan carpenter
On marted? 22 marzo 2022 09:38:58 CEST Fabio M. De Francesco wrote:
> debugfs_create_dir() returns a pointers to a dentry objects. On failures
> it returns errors. Currently the values returned to visornic_probe()
> seem to be tested for being equal to NULL in case of failures.
>
> Properly test with "if (IS_ERR())" and then assign the correct error
> value to the "err" variable.
>
> Signed-off-by: Fabio M. De Francesco <[email protected]>
> ---
> drivers/staging/unisys/visornic/visornic_main.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/unisys/visornic/visornic_main.c b/drivers/staging/unisys/visornic/visornic_main.c
> index 643432458105..58d03f3d3173 100644
> --- a/drivers/staging/unisys/visornic/visornic_main.c
> +++ b/drivers/staging/unisys/visornic/visornic_main.c
> @@ -1922,11 +1922,11 @@ static int visornic_probe(struct visor_device *dev)
> /* create debug/sysfs directories */
> devdata->eth_debugfs_dir = debugfs_create_dir(netdev->name,
> visornic_debugfs_dir);
> - if (!devdata->eth_debugfs_dir) {
> + if (IS_ERR(devdata->eth_debugfs_dir)) {
> dev_err(&dev->device,
> "%s debugfs_create_dir %s failed\n",
> __func__, netdev->name);
> - err = -ENOMEM;
> + err = PTR_ERR(devdata->eth_debugfs_dir);
> goto cleanup_register_netdev;
> }
>
> --
> 2.34.1
>
Hi Greg, Dan,
Now I have time to rework this patch but, if I'm not misunderstanding, you
asked for two contrasting works to do here...
Dan wrote that "[in] this case you can delete the whole devdata->eth_debugfs_dir
and the related code.".
Greg wrote that "We really shouldn't be checking this value at all. There's
no reason to check the return value of a debugfs_* call. Can you fix up the code to
do that instead?".
I'm confused because they look like two incompatible requests. What should I do?
Thanks,
Fabio M. De Francesco