| Message ID | 4f6344cb770047cf5808791d849bbc0cbd330e54.1560514379.git-series.maxime.ripard@bootlin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/vc4: Allow for more boot-time configuration | expand |
On Fri, 14 Jun 2019, Maxime Ripard <maxime.ripard@bootlin.com> wrote: > The drm_client_modeset_commit_atomic function uses two times the > plane_state variable in inner blocks of code, but the variable has a scope > global to this function. > > This will lead to inadvertent devs to reuse the variable in the second > block with the value left by the first, without any warning from the > compiler since value would have been initialized. > > Fix this by moving the variable declaration to the proper scope. This is an improvement, but I'd consider renaming also to not shadow variables. BR, Jani. > > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com> > --- > drivers/gpu/drm/drm_client_modeset.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c > index 006bf7390e7d..8264c3a732b0 100644 > --- a/drivers/gpu/drm/drm_client_modeset.c > +++ b/drivers/gpu/drm/drm_client_modeset.c > @@ -861,7 +861,6 @@ EXPORT_SYMBOL(drm_client_panel_rotation); > static int drm_client_modeset_commit_atomic(struct drm_client_dev *client, bool active) > { > struct drm_device *dev = client->dev; > - struct drm_plane_state *plane_state; > struct drm_plane *plane; > struct drm_atomic_state *state; > struct drm_modeset_acquire_ctx ctx; > @@ -879,6 +878,8 @@ static int drm_client_modeset_commit_atomic(struct drm_client_dev *client, bool > state->acquire_ctx = &ctx; > retry: > drm_for_each_plane(plane, dev) { > + struct drm_plane_state *plane_state; > + > plane_state = drm_atomic_get_plane_state(state, plane); > if (IS_ERR(plane_state)) { > ret = PTR_ERR(plane_state); > @@ -901,6 +902,8 @@ static int drm_client_modeset_commit_atomic(struct drm_client_dev *client, bool > unsigned int rotation; > > if (drm_client_panel_rotation(mode_set, &rotation)) { > + struct drm_plane_state *plane_state; > + > /* Cannot fail as we've already gotten the plane state above */ > plane_state = drm_atomic_get_new_plane_state(state, primary); > plane_state->rotation = rotation;
Hi Jani, On Fri, Jun 14, 2019 at 03:28:59PM +0300, Jani Nikula wrote: > On Fri, 14 Jun 2019, Maxime Ripard <maxime.ripard@bootlin.com> wrote: > > The drm_client_modeset_commit_atomic function uses two times the > > plane_state variable in inner blocks of code, but the variable has a scope > > global to this function. > > > > This will lead to inadvertent devs to reuse the variable in the second > > block with the value left by the first, without any warning from the > > compiler since value would have been initialized. > > > > Fix this by moving the variable declaration to the proper scope. > > This is an improvement, but I'd consider renaming also to not shadow > variables. > > > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com> > > --- > > drivers/gpu/drm/drm_client_modeset.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c > > index 006bf7390e7d..8264c3a732b0 100644 > > --- a/drivers/gpu/drm/drm_client_modeset.c > > +++ b/drivers/gpu/drm/drm_client_modeset.c > > @@ -861,7 +861,6 @@ EXPORT_SYMBOL(drm_client_panel_rotation); > > static int drm_client_modeset_commit_atomic(struct drm_client_dev *client, bool active) > > { > > struct drm_device *dev = client->dev; > > - struct drm_plane_state *plane_state; > > struct drm_plane *plane; > > struct drm_atomic_state *state; > > struct drm_modeset_acquire_ctx ctx; > > @@ -879,6 +878,8 @@ static int drm_client_modeset_commit_atomic(struct drm_client_dev *client, bool > > state->acquire_ctx = &ctx; > > retry: > > drm_for_each_plane(plane, dev) { > > + struct drm_plane_state *plane_state; > > + > > plane_state = drm_atomic_get_plane_state(state, plane); > > if (IS_ERR(plane_state)) { > > ret = PTR_ERR(plane_state); > > @@ -901,6 +902,8 @@ static int drm_client_modeset_commit_atomic(struct drm_client_dev *client, bool > > unsigned int rotation; > > > > if (drm_client_panel_rotation(mode_set, &rotation)) { > > + struct drm_plane_state *plane_state; > > + That's not super clear from that patch, but this variable will not shadow the first one. The code layout is pretty much this one: loop () { struct drm_plane_state *plane_state; [...] } loop () { loop () { struct drm_plane_state *plane_state; [...] } } so the shadowing doesn't exist Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
Den 14.06.2019 14.13, skrev Maxime Ripard: > The drm_client_modeset_commit_atomic function uses two times the > plane_state variable in inner blocks of code, but the variable has a scope > global to this function. > > This will lead to inadvertent devs to reuse the variable in the second > block with the value left by the first, without any warning from the > compiler since value would have been initialized. > > Fix this by moving the variable declaration to the proper scope. > > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com> > --- Reviewed-by: Noralf Trønnes <noralf@tronnes.org>
On Fri, 14 Jun 2019, Maxime Ripard <maxime.ripard@bootlin.com> wrote: > Hi Jani, > > On Fri, Jun 14, 2019 at 03:28:59PM +0300, Jani Nikula wrote: >> On Fri, 14 Jun 2019, Maxime Ripard <maxime.ripard@bootlin.com> wrote: >> > The drm_client_modeset_commit_atomic function uses two times the >> > plane_state variable in inner blocks of code, but the variable has a scope >> > global to this function. >> > >> > This will lead to inadvertent devs to reuse the variable in the second >> > block with the value left by the first, without any warning from the >> > compiler since value would have been initialized. >> > >> > Fix this by moving the variable declaration to the proper scope. >> >> This is an improvement, but I'd consider renaming also to not shadow >> variables. >> >> > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com> >> > --- >> > drivers/gpu/drm/drm_client_modeset.c | 5 ++++- >> > 1 file changed, 4 insertions(+), 1 deletion(-) >> > >> > diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c >> > index 006bf7390e7d..8264c3a732b0 100644 >> > --- a/drivers/gpu/drm/drm_client_modeset.c >> > +++ b/drivers/gpu/drm/drm_client_modeset.c >> > @@ -861,7 +861,6 @@ EXPORT_SYMBOL(drm_client_panel_rotation); >> > static int drm_client_modeset_commit_atomic(struct drm_client_dev *client, bool active) >> > { >> > struct drm_device *dev = client->dev; >> > - struct drm_plane_state *plane_state; >> > struct drm_plane *plane; >> > struct drm_atomic_state *state; >> > struct drm_modeset_acquire_ctx ctx; >> > @@ -879,6 +878,8 @@ static int drm_client_modeset_commit_atomic(struct drm_client_dev *client, bool >> > state->acquire_ctx = &ctx; >> > retry: >> > drm_for_each_plane(plane, dev) { >> > + struct drm_plane_state *plane_state; >> > + >> > plane_state = drm_atomic_get_plane_state(state, plane); >> > if (IS_ERR(plane_state)) { >> > ret = PTR_ERR(plane_state); >> > @@ -901,6 +902,8 @@ static int drm_client_modeset_commit_atomic(struct drm_client_dev *client, bool >> > unsigned int rotation; >> > >> > if (drm_client_panel_rotation(mode_set, &rotation)) { >> > + struct drm_plane_state *plane_state; >> > + > > That's not super clear from that patch, but this variable will not > shadow the first one. > > The code layout is pretty much this one: > > loop () { > struct drm_plane_state *plane_state; > > [...] > } > > loop () { > loop () { > struct drm_plane_state *plane_state; > > [...] > } > } > > so the shadowing doesn't exist Indeed, sorry for the noise. BR, Jani.
diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c index 006bf7390e7d..8264c3a732b0 100644 --- a/drivers/gpu/drm/drm_client_modeset.c +++ b/drivers/gpu/drm/drm_client_modeset.c @@ -861,7 +861,6 @@ EXPORT_SYMBOL(drm_client_panel_rotation); static int drm_client_modeset_commit_atomic(struct drm_client_dev *client, bool active) { struct drm_device *dev = client->dev; - struct drm_plane_state *plane_state; struct drm_plane *plane; struct drm_atomic_state *state; struct drm_modeset_acquire_ctx ctx; @@ -879,6 +878,8 @@ static int drm_client_modeset_commit_atomic(struct drm_client_dev *client, bool state->acquire_ctx = &ctx; retry: drm_for_each_plane(plane, dev) { + struct drm_plane_state *plane_state; + plane_state = drm_atomic_get_plane_state(state, plane); if (IS_ERR(plane_state)) { ret = PTR_ERR(plane_state); @@ -901,6 +902,8 @@ static int drm_client_modeset_commit_atomic(struct drm_client_dev *client, bool unsigned int rotation; if (drm_client_panel_rotation(mode_set, &rotation)) { + struct drm_plane_state *plane_state; + /* Cannot fail as we've already gotten the plane state above */ plane_state = drm_atomic_get_new_plane_state(state, primary); plane_state->rotation = rotation;
The drm_client_modeset_commit_atomic function uses two times the plane_state variable in inner blocks of code, but the variable has a scope global to this function. This will lead to inadvertent devs to reuse the variable in the second block with the value left by the first, without any warning from the compiler since value would have been initialized. Fix this by moving the variable declaration to the proper scope. Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com> --- drivers/gpu/drm/drm_client_modeset.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)