Re: [PATCH net-next v4 01/10] net: sunhme: Fix uninitialized return code

[Simon Horman <simon.horman@corigine.com>]

Hi Sean,

On Fri, Mar 24, 2023 at 01:51:27PM -0400, Sean Anderson wrote:
> Fix an uninitialized return code if we never found a qfe slot. It would be
> a bug if we ever got into this situation, but it's good to return something
> tracable.
> 
> Fixes: acb3f35f920b ("sunhme: forward the error code from pci_enable_device()")

I think it might be,

Fixes: 5b3dc6dda6b1 ("sunhme: Regularize probe errors")

> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <error27@gmail.com>

Checkpatch requests a Link tag after Reported-by tags.

Link: https://lore.kernel.org/oe-kbuild/20230222135715.hjXBN9H5dr7nCnI_Ye2s5H--HsnWom4o9AMScThhDRM@z/T/

> Signed-off-by: Sean Anderson <seanga2@gmail.com>
> ---
> 
> Changes in v4:
> - Move this fix to its own commit
> 
>  drivers/net/ethernet/sun/sunhme.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/sun/sunhme.c b/drivers/net/ethernet/sun/sunhme.c
> index b0c7ab74a82e..7cf8210ebbec 100644
> --- a/drivers/net/ethernet/sun/sunhme.c
> +++ b/drivers/net/ethernet/sun/sunhme.c
> @@ -2834,7 +2834,7 @@ static int happy_meal_pci_probe(struct pci_dev *pdev,
>  	int i, qfe_slot = -1;
>  	char prom_name[64];
>  	u8 addr[ETH_ALEN];
> -	int err;
> +	int err = -ENODEV;
>  
>  	/* Now make sure pci_dev cookie is there. */
>  #ifdef CONFIG_SPARC

Unfortunately I don't think this is the right fix,
and indeed smatch still complains with it applied.

The reason is that a few lines further down there is:

        err = pcim_enable_device(pdev);
        if (err)
                goto err_out;

Which overrides the initialisation of err.
Before getting to the line that smatch highlights, correctly as far
as I can tell, as having a missing error code.

			if (qfe_slot == 4)
				goto err_out;

As err_out simply calls 'return err' one could just return here.
And perhaps that is a nice cleanup to roll out at some point.
But to be in keeping with the style of the function, as a minimal fix,
I think the following might be appropriate.

Note, with this applied err doesn't need to be initialised at the top of
the function (as far as my invocation of smatch is concerned).

diff --git a/drivers/net/ethernet/sun/sunhme.c b/drivers/net/ethernet/sun/sunhme.c
index b0c7ab74a82e..d6df778a0052 100644
--- a/drivers/net/ethernet/sun/sunhme.c
+++ b/drivers/net/ethernet/sun/sunhme.c
@@ -2863,8 +2863,10 @@ static int happy_meal_pci_probe(struct pci_dev *pdev,
 			if (!qp->happy_meals[qfe_slot])
 				break;
 
-		if (qfe_slot == 4)
+		if (qfe_slot == 4) {
+			err = -ENODEV;
 			goto err_out;
+		}
 	}
 
 	dev = devm_alloc_etherdev(&pdev->dev, sizeof(struct happy_meal));