[PATCH] drm/bridge: Avoid uninitialized variable warning

[PATCH] drm/bridge: Avoid uninitialized variable warning

[Dan Carpenter]

This works, but technically it uses "num_in_bus_fmts" before it has been
initialized so it leads to static checker warnings and probably KMEMsan
warnings at run time.  Reverse the checks so it checks for failure first
and then check for unsupported formats next.

Fixes: f32df58acc68 ("drm/bridge: Add the necessary bits to support bus format negotiation")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 drivers/gpu/drm/drm_bridge.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index e275b4ca344b..00cbde654472 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -897,10 +897,10 @@ static int select_bus_fmt_recursive(struct drm_bridge *first_bridge,
 							conn_state,
 							out_bus_fmt,
 							&num_in_bus_fmts);
-	if (!num_in_bus_fmts)
-		return -ENOTSUPP;
-	else if (!in_bus_fmts)
+	if (!in_bus_fmts)
 		return -ENOMEM;
+	else if (!num_in_bus_fmts)
+		return -ENOTSUPP;
 
 	if (first_bridge == cur_bridge) {
 		cur_state->input_bus_cfg.format = in_bus_fmts[0];
@@ -993,10 +993,10 @@ drm_atomic_bridge_chain_select_bus_fmts(struct drm_bridge *bridge,
 							crtc_state,
 							conn_state,
 							&num_out_bus_fmts);
-		if (!num_out_bus_fmts)
-			return -ENOTSUPP;
-		else if (!out_bus_fmts)
+		if (!out_bus_fmts)
 			return -ENOMEM;
+		else if (!num_out_bus_fmts)
+			return -ENOTSUPP;
 	} else {
 		num_out_bus_fmts = 1;
 		out_bus_fmts = kmalloc(sizeof(*out_bus_fmts), GFP_KERNEL);
-- 
2.35.1

Re: [PATCH] drm/bridge: Avoid uninitialized variable warning

[Boris Brezillon]

On Tue, 28 Jun 2022 09:59:51 +0300
Dan Carpenter <dan.carpenter@oracle.com> wrote:

> This works, but technically it uses "num_in_bus_fmts" before it has been
> initialized so it leads to static checker warnings and probably KMEMsan
> warnings at run time.  Reverse the checks so it checks for failure first
> and then check for unsupported formats next.
> 
> Fixes: f32df58acc68 ("drm/bridge: Add the necessary bits to support bus format negotiation")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
>  drivers/gpu/drm/drm_bridge.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index e275b4ca344b..00cbde654472 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -897,10 +897,10 @@ static int select_bus_fmt_recursive(struct drm_bridge *first_bridge,
>  							conn_state,
>  							out_bus_fmt,
>  							&num_in_bus_fmts);
> -	if (!num_in_bus_fmts)
> -		return -ENOTSUPP;
> -	else if (!in_bus_fmts)
> +	if (!in_bus_fmts)
>  		return -ENOMEM;
> +	else if (!num_in_bus_fmts)
> +		return -ENOTSUPP;

Well, it changes the error we return when num_in_bus_fmts = 0
&& in_bus_fmts == NULL which is not an ENOMEM situation, so I'd rather
initialize num_{in,out}_bus_fmts to 0 here.

>  
>  	if (first_bridge == cur_bridge) {
>  		cur_state->input_bus_cfg.format = in_bus_fmts[0];
> @@ -993,10 +993,10 @@ drm_atomic_bridge_chain_select_bus_fmts(struct drm_bridge *bridge,
>  							crtc_state,
>  							conn_state,
>  							&num_out_bus_fmts);
> -		if (!num_out_bus_fmts)
> -			return -ENOTSUPP;
> -		else if (!out_bus_fmts)
> +		if (!out_bus_fmts)
>  			return -ENOMEM;
> +		else if (!num_out_bus_fmts)
> +			return -ENOTSUPP;
>  	} else {
>  		num_out_bus_fmts = 1;
>  		out_bus_fmts = kmalloc(sizeof(*out_bus_fmts), GFP_KERNEL);

Re: [PATCH] drm/bridge: Avoid uninitialized variable warning

[Dan Carpenter]

On Tue, Jun 28, 2022 at 09:26:09AM +0200, Boris Brezillon wrote:
> On Tue, 28 Jun 2022 09:59:51 +0300
> Dan Carpenter <dan.carpenter@oracle.com> wrote:
> 
> > This works, but technically it uses "num_in_bus_fmts" before it has been
> > initialized so it leads to static checker warnings and probably KMEMsan
> > warnings at run time.  Reverse the checks so it checks for failure first
> > and then check for unsupported formats next.
> > 
> > Fixes: f32df58acc68 ("drm/bridge: Add the necessary bits to support bus format negotiation")
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > ---
> >  drivers/gpu/drm/drm_bridge.c | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> > index e275b4ca344b..00cbde654472 100644
> > --- a/drivers/gpu/drm/drm_bridge.c
> > +++ b/drivers/gpu/drm/drm_bridge.c
> > @@ -897,10 +897,10 @@ static int select_bus_fmt_recursive(struct drm_bridge *first_bridge,
> >  							conn_state,
> >  							out_bus_fmt,
> >  							&num_in_bus_fmts);
> > -	if (!num_in_bus_fmts)
> > -		return -ENOTSUPP;
> > -	else if (!in_bus_fmts)
> > +	if (!in_bus_fmts)
> >  		return -ENOMEM;
> > +	else if (!num_in_bus_fmts)
> > +		return -ENOTSUPP;
> 
> Well, it changes the error we return when num_in_bus_fmts = 0
> && in_bus_fmts == NULL which is not an ENOMEM situation, so I'd rather
> initialize num_{in,out}_bus_fmts to 0 here.
> 

I can do that but there is no real consistency in how
->atomic_get_input_bus_fmts() functions are implemented.  Some set
*num_input_fmts = 0; before the kmalloc() and then reset it to
*num_input_fmts = 1; if the allocation succeeds.  Some just set it to
*num_input_fmts = 1 at the start.

This bug only affects the imx code like:
imx8qm_ldb_bridge_atomic_get_input_bus_fmts()
imx8qxp_pixel_link_bridge_atomic_get_input_bus_fmts

Anyway, it's not a problem to resend.

regards,
dan carpenter

Re: [PATCH] drm/bridge: Avoid uninitialized variable warning

[Boris Brezillon]

On Tue, 28 Jun 2022 10:45:19 +0300
Dan Carpenter <dan.carpenter@oracle.com> wrote:

> On Tue, Jun 28, 2022 at 09:26:09AM +0200, Boris Brezillon wrote:
> > On Tue, 28 Jun 2022 09:59:51 +0300
> > Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >   
> > > This works, but technically it uses "num_in_bus_fmts" before it has been
> > > initialized so it leads to static checker warnings and probably KMEMsan
> > > warnings at run time.  Reverse the checks so it checks for failure first
> > > and then check for unsupported formats next.
> > > 
> > > Fixes: f32df58acc68 ("drm/bridge: Add the necessary bits to support bus format negotiation")
> > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > > ---
> > >  drivers/gpu/drm/drm_bridge.c | 12 ++++++------
> > >  1 file changed, 6 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> > > index e275b4ca344b..00cbde654472 100644
> > > --- a/drivers/gpu/drm/drm_bridge.c
> > > +++ b/drivers/gpu/drm/drm_bridge.c
> > > @@ -897,10 +897,10 @@ static int 	(struct drm_bridge *first_bridge,
> > >  							conn_state,
> > >  							out_bus_fmt,
> > >  							&num_in_bus_fmts);
> > > -	if (!num_in_bus_fmts)
> > > -		return -ENOTSUPP;
> > > -	else if (!in_bus_fmts)
> > > +	if (!in_bus_fmts)
> > >  		return -ENOMEM;
> > > +	else if (!num_in_bus_fmts)
> > > +		return -ENOTSUPP;  
> > 
> > Well, it changes the error we return when num_in_bus_fmts = 0
> > && in_bus_fmts == NULL which is not an ENOMEM situation, so I'd rather
> > initialize num_{in,out}_bus_fmts to 0 here.
> >   
> 
> I can do that but there is no real consistency in how
> ->atomic_get_input_bus_fmts() functions are implemented.  Some set  
> *num_input_fmts = 0; before the kmalloc() and then reset it to
> *num_input_fmts = 1; if the allocation succeeds.  Some just set it to
> *num_input_fmts = 1 at the start.
> 
> This bug only affects the imx code like:
> imx8qm_ldb_bridge_atomic_get_input_bus_fmts()
> imx8qxp_pixel_link_bridge_atomic_get_input_bus_fmts
> 

I'd say imx8qm_ldb_bridge_atomic_get_input_bus_fmts() and
imx8qxp_pixel_link_bridge_atomic_get_input_bus_fmts() should be patched
to set *num_input_fmts = 0 when they return NULL on purpose, as
documented here [1].

[1]https://elixir.bootlin.com/linux/latest/source/include/drm/drm_bridge.h#L453