| Message ID | 20220218100403.7028-13-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | drm: Review of mode copies | expand |
On 18/02/2022 13: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: Rob Clark <robdclark@gmail.com> > Cc: Sean Paul <sean@poorly.run> > Cc: Abhinav Kumar <quic_abhinavk@quicinc.com> > Cc: linux-arm-msm@vger.kernel.org > Cc: freedreno@lists.freedesktop.org > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > ---
On 2/18/2022 2:03 AM, 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: Rob Clark <robdclark@gmail.com> > Cc: Sean Paul <sean@poorly.run> > Cc: Abhinav Kumar <quic_abhinavk@quicinc.com> > Cc: linux-arm-msm@vger.kernel.org > Cc: freedreno@lists.freedesktop.org > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com> > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 2 +- > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 2 +- > drivers/gpu/drm/msm/dp/dp_display.c | 2 +- > 3 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c > index 34a6940d12c5..57592052af23 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c > @@ -157,7 +157,7 @@ static void dpu_encoder_phys_cmd_mode_set( > DPU_ERROR("invalid args\n"); > return; > } > - phys_enc->cached_mode = *adj_mode; > + drm_mode_copy(&phys_enc->cached_mode, adj_mode); > DPU_DEBUG_CMDENC(cmd_enc, "caching mode:\n"); > drm_mode_debug_printmodeline(adj_mode); > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c > index e7813c6f7bd9..d5deca07b65a 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c > @@ -370,7 +370,7 @@ static void dpu_encoder_phys_vid_mode_set( > struct dpu_encoder_irq *irq; > > if (adj_mode) { > - phys_enc->cached_mode = *adj_mode; > + drm_mode_copy(&phys_enc->cached_mode, adj_mode); > drm_mode_debug_printmodeline(adj_mode); > DPU_DEBUG_VIDENC(phys_enc, "caching mode:\n"); > } > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c > index 7cc4d21f2091..2ed6028ca8d6 100644 > --- a/drivers/gpu/drm/msm/dp/dp_display.c > +++ b/drivers/gpu/drm/msm/dp/dp_display.c > @@ -825,7 +825,7 @@ static int dp_display_set_mode(struct msm_dp *dp_display, > > dp = container_of(dp_display, struct dp_display_private, dp_display); > > - dp->panel->dp_mode.drm_mode = mode->drm_mode; > + drm_mode_copy(&dp->panel->dp_mode.drm_mode, &mode->drm_mode); > dp->panel->dp_mode.bpp = mode->bpp; > dp->panel->dp_mode.capabilities = mode->capabilities; > dp_panel_init_panel_info(dp->panel);
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c index 34a6940d12c5..57592052af23 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c @@ -157,7 +157,7 @@ static void dpu_encoder_phys_cmd_mode_set( DPU_ERROR("invalid args\n"); return; } - phys_enc->cached_mode = *adj_mode; + drm_mode_copy(&phys_enc->cached_mode, adj_mode); DPU_DEBUG_CMDENC(cmd_enc, "caching mode:\n"); drm_mode_debug_printmodeline(adj_mode); diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c index e7813c6f7bd9..d5deca07b65a 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c @@ -370,7 +370,7 @@ static void dpu_encoder_phys_vid_mode_set( struct dpu_encoder_irq *irq; if (adj_mode) { - phys_enc->cached_mode = *adj_mode; + drm_mode_copy(&phys_enc->cached_mode, adj_mode); drm_mode_debug_printmodeline(adj_mode); DPU_DEBUG_VIDENC(phys_enc, "caching mode:\n"); } diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c index 7cc4d21f2091..2ed6028ca8d6 100644 --- a/drivers/gpu/drm/msm/dp/dp_display.c +++ b/drivers/gpu/drm/msm/dp/dp_display.c @@ -825,7 +825,7 @@ static int dp_display_set_mode(struct msm_dp *dp_display, dp = container_of(dp_display, struct dp_display_private, dp_display); - dp->panel->dp_mode.drm_mode = mode->drm_mode; + drm_mode_copy(&dp->panel->dp_mode.drm_mode, &mode->drm_mode); dp->panel->dp_mode.bpp = mode->bpp; dp->panel->dp_mode.capabilities = mode->capabilities; dp_panel_init_panel_info(dp->panel);