| Message ID | 20220218100403.7028-7-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: Review of mode copies | expand |
On 18.02.2022 11:03, Ville Syrjala wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > struct drm_display_mode embeds a list head, so overwriting > the full struct with another one will corrupt the list > (if the destination mode is on a list). Use drm_mode_copy() > instead which explicitly preserves the list head of > the destination mode. > > Even if we know the destination mode is not on any list > using drm_mode_copy() seems decent as it sets a good > example. Bad examples of not using it might eventually > get copied into code where preserving the list head > actually matters. > > Obviously one case not covered here is when the mode > itself is embedded in a larger structure and the whole > structure is copied. But if we are careful when copying > into modes embedded in structures I think we can be a > little more reassured that bogus list heads haven't been > propagated in. > > @is_mode_copy@ > @@ > drm_mode_copy(...) > { > ... > } > > @depends on !is_mode_copy@ > struct drm_display_mode *mode; > expression E, S; > @@ > ( > - *mode = E > + drm_mode_copy(mode, &E) > | > - memcpy(mode, E, S) > + drm_mode_copy(mode, E) > ) > > @depends on !is_mode_copy@ > struct drm_display_mode mode; > expression E; > @@ > ( > - mode = E > + drm_mode_copy(&mode, &E) > | > - memcpy(&mode, E, S) > + drm_mode_copy(&mode, E) > ) > > @@ > struct drm_display_mode *mode; > @@ > - &*mode > + mode > > Cc: Andrzej Hajda <andrzej.hajda@intel.com> > Cc: Neil Armstrong <narmstrong@baylibre.com> > Cc: Robert Foss <robert.foss@linaro.org> > Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com> > Cc: Jonas Karlman <jonas@kwiboo.se> > Cc: Jernej Skrabec <jernej.skrabec@gmail.com> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com> Regards Andrzej > --- > drivers/gpu/drm/bridge/nwl-dsi.c | 2 +- > drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 2 +- > drivers/gpu/drm/bridge/tc358767.c | 2 +- > 3 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/nwl-dsi.c b/drivers/gpu/drm/bridge/nwl-dsi.c > index 963a6794735f..881cf338d5cf 100644 > --- a/drivers/gpu/drm/bridge/nwl-dsi.c > +++ b/drivers/gpu/drm/bridge/nwl-dsi.c > @@ -857,7 +857,7 @@ nwl_dsi_bridge_mode_set(struct drm_bridge *bridge, > /* Save the new desired phy config */ > memcpy(&dsi->phy_cfg, &new_cfg, sizeof(new_cfg)); > > - memcpy(&dsi->mode, adjusted_mode, sizeof(dsi->mode)); > + drm_mode_copy(&dsi->mode, adjusted_mode); > drm_mode_debug_printmodeline(adjusted_mode); > > if (pm_runtime_resume_and_get(dev) < 0) > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > index 4befc104d220..a563460f8d20 100644 > --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > @@ -2830,7 +2830,7 @@ static void dw_hdmi_bridge_mode_set(struct drm_bridge *bridge, > mutex_lock(&hdmi->mutex); > > /* Store the display mode for plugin/DKMS poweron events */ > - memcpy(&hdmi->previous_mode, mode, sizeof(hdmi->previous_mode)); > + drm_mode_copy(&hdmi->previous_mode, mode); > > mutex_unlock(&hdmi->mutex); > } > diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c > index c23e0abc65e8..7f9574b17caa 100644 > --- a/drivers/gpu/drm/bridge/tc358767.c > +++ b/drivers/gpu/drm/bridge/tc358767.c > @@ -1312,7 +1312,7 @@ static void tc_bridge_mode_set(struct drm_bridge *bridge, > { > struct tc_data *tc = bridge_to_tc(bridge); > > - tc->mode = *mode; > + drm_mode_copy(&tc->mode, mode); > } > > static struct edid *tc_get_edid(struct drm_bridge *bridge,
Hi Ville, Thank you for the patch. On Fri, Feb 18, 2022 at 12:03:47PM +0200, Ville Syrjala wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > struct drm_display_mode embeds a list head, so overwriting > the full struct with another one will corrupt the list > (if the destination mode is on a list). Use drm_mode_copy() > instead which explicitly preserves the list head of > the destination mode. > > Even if we know the destination mode is not on any list > using drm_mode_copy() seems decent as it sets a good > example. Bad examples of not using it might eventually > get copied into code where preserving the list head > actually matters. > > Obviously one case not covered here is when the mode > itself is embedded in a larger structure and the whole > structure is copied. But if we are careful when copying > into modes embedded in structures I think we can be a > little more reassured that bogus list heads haven't been > propagated in. > > @is_mode_copy@ > @@ > drm_mode_copy(...) > { > ... > } > > @depends on !is_mode_copy@ > struct drm_display_mode *mode; > expression E, S; > @@ > ( > - *mode = E > + drm_mode_copy(mode, &E) > | > - memcpy(mode, E, S) > + drm_mode_copy(mode, E) > ) > > @depends on !is_mode_copy@ > struct drm_display_mode mode; > expression E; > @@ > ( > - mode = E > + drm_mode_copy(&mode, &E) > | > - memcpy(&mode, E, S) > + drm_mode_copy(&mode, E) > ) > > @@ > struct drm_display_mode *mode; > @@ > - &*mode > + mode > > Cc: Andrzej Hajda <andrzej.hajda@intel.com> > Cc: Neil Armstrong <narmstrong@baylibre.com> > Cc: Robert Foss <robert.foss@linaro.org> > Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com> > Cc: Jonas Karlman <jonas@kwiboo.se> > Cc: Jernej Skrabec <jernej.skrabec@gmail.com> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/gpu/drm/bridge/nwl-dsi.c | 2 +- > drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 2 +- > drivers/gpu/drm/bridge/tc358767.c | 2 +- > 3 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/nwl-dsi.c b/drivers/gpu/drm/bridge/nwl-dsi.c > index 963a6794735f..881cf338d5cf 100644 > --- a/drivers/gpu/drm/bridge/nwl-dsi.c > +++ b/drivers/gpu/drm/bridge/nwl-dsi.c > @@ -857,7 +857,7 @@ nwl_dsi_bridge_mode_set(struct drm_bridge *bridge, > /* Save the new desired phy config */ > memcpy(&dsi->phy_cfg, &new_cfg, sizeof(new_cfg)); > > - memcpy(&dsi->mode, adjusted_mode, sizeof(dsi->mode)); > + drm_mode_copy(&dsi->mode, adjusted_mode); > drm_mode_debug_printmodeline(adjusted_mode); > > if (pm_runtime_resume_and_get(dev) < 0) > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > index 4befc104d220..a563460f8d20 100644 > --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > @@ -2830,7 +2830,7 @@ static void dw_hdmi_bridge_mode_set(struct drm_bridge *bridge, > mutex_lock(&hdmi->mutex); > > /* Store the display mode for plugin/DKMS poweron events */ > - memcpy(&hdmi->previous_mode, mode, sizeof(hdmi->previous_mode)); > + drm_mode_copy(&hdmi->previous_mode, mode); > > mutex_unlock(&hdmi->mutex); > } > diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c > index c23e0abc65e8..7f9574b17caa 100644 > --- a/drivers/gpu/drm/bridge/tc358767.c > +++ b/drivers/gpu/drm/bridge/tc358767.c > @@ -1312,7 +1312,7 @@ static void tc_bridge_mode_set(struct drm_bridge *bridge, > { > struct tc_data *tc = bridge_to_tc(bridge); > > - tc->mode = *mode; > + drm_mode_copy(&tc->mode, mode); > } > > static struct edid *tc_get_edid(struct drm_bridge *bridge,
diff --git a/drivers/gpu/drm/bridge/nwl-dsi.c b/drivers/gpu/drm/bridge/nwl-dsi.c index 963a6794735f..881cf338d5cf 100644 --- a/drivers/gpu/drm/bridge/nwl-dsi.c +++ b/drivers/gpu/drm/bridge/nwl-dsi.c @@ -857,7 +857,7 @@ nwl_dsi_bridge_mode_set(struct drm_bridge *bridge, /* Save the new desired phy config */ memcpy(&dsi->phy_cfg, &new_cfg, sizeof(new_cfg)); - memcpy(&dsi->mode, adjusted_mode, sizeof(dsi->mode)); + drm_mode_copy(&dsi->mode, adjusted_mode); drm_mode_debug_printmodeline(adjusted_mode); if (pm_runtime_resume_and_get(dev) < 0) diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index 4befc104d220..a563460f8d20 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c @@ -2830,7 +2830,7 @@ static void dw_hdmi_bridge_mode_set(struct drm_bridge *bridge, mutex_lock(&hdmi->mutex); /* Store the display mode for plugin/DKMS poweron events */ - memcpy(&hdmi->previous_mode, mode, sizeof(hdmi->previous_mode)); + drm_mode_copy(&hdmi->previous_mode, mode); mutex_unlock(&hdmi->mutex); } diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c index c23e0abc65e8..7f9574b17caa 100644 --- a/drivers/gpu/drm/bridge/tc358767.c +++ b/drivers/gpu/drm/bridge/tc358767.c @@ -1312,7 +1312,7 @@ static void tc_bridge_mode_set(struct drm_bridge *bridge, { struct tc_data *tc = bridge_to_tc(bridge); - tc->mode = *mode; + drm_mode_copy(&tc->mode, mode); } static struct edid *tc_get_edid(struct drm_bridge *bridge,