RE: [PATCH 4/6] bus: fsl-mc: fsl-mc-allocator: Improve error reporting

[Leo Li]

> -----Original Message-----
> From: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>
> Sent: Thursday, June 1, 2023 12:00 PM
> To: Nathan Chancellor <nathan@xxxxxxxxxx>; Leo Li <leoyang.li@xxxxxxx>
> Cc: Stuart Yoder <stuyoder@xxxxxxxxx>; llvm@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; kernel@xxxxxxxxxxxxxx; Laurentiu Tudor
> <laurentiu.tudor@xxxxxxx>
> Subject: Re: [PATCH 4/6] bus: fsl-mc: fsl-mc-allocator: Improve error
> reporting
> 
> Hello,
> 
> On Thu, Jun 01, 2023 at 08:41:01AM -0700, Nathan Chancellor wrote:
> > On Fri, Mar 10, 2023 at 11:41:26PM +0100, Uwe Kleine-König wrote:
> > > Instead of silently returning an error in the remove callback (which
> > > yields a generic and little informing error message), annotate each
> > > error path of
> > > fsl_mc_resource_pool_remove_device() with an error message and
> > > return zero in the remove callback to suppress the error message.
> > >
> > > Note that changing the return value has no other effect than
> > > suppressing the error message by the fsl_mc bus driver.
> > >
> > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>
> >
> > I apologize if this has already been reported or fixed somewhere, I
> > did a search of lore.kernel.org and did not find anything. This change
> > as commit b3134039c5b3 ("bus: fsl-mc: fsl-mc-allocator: Improve error
> > reporting") causes the following warning/error with clang:
> >
> >   drivers/bus/fsl-mc/fsl-mc-allocator.c:108:12: error: variable 'mc_bus_dev'
> is uninitialized when used here [-Werror,-Wuninitialized]
> >     108 |                 dev_err(&mc_bus_dev->dev, "resource mismatch\n");
> >         |                          ^~~~~~~~~~
> >   include/linux/dev_printk.h:144:44: note: expanded from macro 'dev_err'
> >     144 |         dev_printk_index_wrap(_dev_err, KERN_ERR, dev,
> dev_fmt(fmt), ##__VA_ARGS__)
> >         |                                                   ^~~
> >   include/linux/dev_printk.h:110:11: note: expanded from macro
> 'dev_printk_index_wrap'
> >     110 |                 _p_func(dev, fmt, ##__VA_ARGS__);                       \
> >         |                         ^~~
> >   drivers/bus/fsl-mc/fsl-mc-allocator.c:100:34: note: initialize the variable
> 'mc_bus_dev' to silence this warning
> >     100 |         struct fsl_mc_device *mc_bus_dev;
> >         |                                         ^
> >         |                                          = NULL
> >   1 error generated.
> >
> > Should this be using mc_dev->dev or is there a different fix?
> >
> > diff --git a/drivers/bus/fsl-mc/fsl-mc-allocator.c
> > b/drivers/bus/fsl-mc/fsl-mc-allocator.c
> > index 0ad68099684e..867ac3bbeae6 100644
> > --- a/drivers/bus/fsl-mc/fsl-mc-allocator.c
> > +++ b/drivers/bus/fsl-mc/fsl-mc-allocator.c
> > @@ -105,7 +105,7 @@ static int __must_check
> > fsl_mc_resource_pool_remove_device(struct fsl_mc_device
> >
> >  	resource = mc_dev->resource;
> >  	if (!resource || resource->data != mc_dev) {
> > -		dev_err(&mc_bus_dev->dev, "resource mismatch\n");
> > +		dev_err(&mc_dev->dev, "resource mismatch\n");
> >  		goto out;
> >  	}
> 
> Hmm, clang seems to be right, and I just confirmed that gcc (arm-linux-
> gnueabihf-gcc (Debian 12.2.0-14) 12.2.0) doesn't emit a warning. :-\
> 
> My approach would be:
> 
> diff --git a/drivers/bus/fsl-mc/fsl-mc-allocator.c b/drivers/bus/fsl-mc/fsl-mc-
> allocator.c
> index 0ad68099684e..991273f956ce 100644
> --- a/drivers/bus/fsl-mc/fsl-mc-allocator.c
> +++ b/drivers/bus/fsl-mc/fsl-mc-allocator.c
> @@ -103,14 +103,15 @@ static int __must_check
> fsl_mc_resource_pool_remove_device(struct fsl_mc_device
>  	struct fsl_mc_resource *resource;
>  	int error = -EINVAL;
> 
> +	mc_bus_dev = to_fsl_mc_device(mc_dev->dev.parent);
> +	mc_bus = to_fsl_mc_bus(mc_bus_dev);
> +
>  	resource = mc_dev->resource;
>  	if (!resource || resource->data != mc_dev) {
>  		dev_err(&mc_bus_dev->dev, "resource mismatch\n");
>  		goto out;
>  	}
> 
> -	mc_bus_dev = to_fsl_mc_device(mc_dev->dev.parent);
> -	mc_bus = to_fsl_mc_bus(mc_bus_dev);
>  	res_pool = resource->parent_pool;
>  	if (res_pool != &mc_bus->resource_pools[resource->type]) {
>  		dev_err(&mc_bus_dev->dev, "pool mismatch\n");
> 
> 
> Should I prepare a proper patch, or is it possible to squash this change into
> b3134039c5b3cf879841e3ec84c8cbf7675554ec?
> 
> @Li Yang: Please advice.

This looks fine.  Please send a new patch and I can squash it to the original commit.

Regards,
Leo