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;
From:   "Fabio M. De Francesco" <fmdefrancesco@gmail.com>
To:     Dan Carpenter <dan.carpenter@oracle.com>
Cc:     David Kershner <david.kershner@unisys.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        sparmaintainer@unisys.com, linux-staging@lists.linux.dev,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] staging: unisys: Properly test debugfs_create_dir() return values
Date:   Tue, 22 Mar 2022 10:09:22 +0100
Message-ID: <3166283.44csPzL39Z@leap>
In-Reply-To: <20220322084928.GM3293@kadam>
References: <20220322083858.16887-1-fmdefrancesco@gmail.com> <20220322084928.GM3293@kadam>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"
Precedence: bulk

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 <fmdefrancesco@gmail.com>
> > ---
> >  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 :)