| Message ID | Yrqm5yYVa6xMY2vq@kili (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/bridge: Avoid uninitialized variable warning | expand |
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);
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
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
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);
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(-)