* [PATCH] drm/bridge: Avoid uninitialized variable warning
@ 2022-06-28 6:59 Dan Carpenter
2022-06-28 7:26 ` Boris Brezillon
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2022-06-28 6:59 UTC (permalink / raw)
To: Maarten Lankhorst, Boris Brezillon
Cc: Thomas Zimmermann, Neil Armstrong, David Airlie, dri-devel,
kernel-janitors, Jernej Skrabec
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
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] drm/bridge: Avoid uninitialized variable warning
2022-06-28 6:59 [PATCH] drm/bridge: Avoid uninitialized variable warning Dan Carpenter
@ 2022-06-28 7:26 ` Boris Brezillon
2022-06-28 7:45 ` Dan Carpenter
0 siblings, 1 reply; 4+ messages in thread
From: Boris Brezillon @ 2022-06-28 7:26 UTC (permalink / raw)
To: Dan Carpenter
Cc: Thomas Zimmermann, Neil Armstrong, David Airlie, kernel-janitors,
dri-devel, Jernej Skrabec
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);
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] drm/bridge: Avoid uninitialized variable warning
2022-06-28 7:26 ` Boris Brezillon
@ 2022-06-28 7:45 ` Dan Carpenter
2022-06-28 8:54 ` Boris Brezillon
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2022-06-28 7:45 UTC (permalink / raw)
To: Boris Brezillon
Cc: Thomas Zimmermann, Neil Armstrong, David Airlie, kernel-janitors,
dri-devel, Jernej Skrabec
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
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] drm/bridge: Avoid uninitialized variable warning
2022-06-28 7:45 ` Dan Carpenter
@ 2022-06-28 8:54 ` Boris Brezillon
0 siblings, 0 replies; 4+ messages in thread
From: Boris Brezillon @ 2022-06-28 8:54 UTC (permalink / raw)
To: Dan Carpenter
Cc: Thomas Zimmermann, Neil Armstrong, David Airlie, kernel-janitors,
dri-devel, Jernej Skrabec
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-06-28 8:54 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-06-28 6:59 [PATCH] drm/bridge: Avoid uninitialized variable warning Dan Carpenter
2022-06-28 7:26 ` Boris Brezillon
2022-06-28 7:45 ` Dan Carpenter
2022-06-28 8:54 ` Boris Brezillon
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).