| Message ID | 20240212215534.190682-1-robdclark@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/crtc: fix uninitialized variable use even harder | expand |
On Mon, Feb 12, 2024 at 1:55 PM Rob Clark <robdclark@gmail.com> wrote: > > From: Rob Clark <robdclark@chromium.org> > > DRM_MODESET_LOCK_ALL_BEGIN() has a hidden trap-door (aka retry loop), > which means we can't rely too much on variable initializers. > > Fixes: 6e455f5dcdd1 ("drm/crtc: fix uninitialized variable use") > Signed-off-by: Rob Clark <robdclark@chromium.org> > --- > I have mixed feelings about DRM_MODESET_LOCK_ALL_BEGIN() (and friends) > magic. On one hand it simplifies the deadlock/back dance. OTOH it > conceals a nasty sharp edge. Maybe it is better to have the complicated > restart path a bit more explicit, like it was originally. I should also point out, had drm-misc-next been using gitlab MRs and gitlab CI, we would have caught this ;-) BR, -R > drivers/gpu/drm/drm_crtc.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > index cb90e70d85e8..65f9f66933bb 100644 > --- a/drivers/gpu/drm/drm_crtc.c > +++ b/drivers/gpu/drm/drm_crtc.c > @@ -904,6 +904,7 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data, > connector_set = NULL; > fb = NULL; > mode = NULL; > + num_connectors = 0; > > DRM_MODESET_LOCK_ALL_END(dev, ctx, ret); > > -- > 2.43.0 >
On 2/12/2024 1:55 PM, Rob Clark wrote: > From: Rob Clark <robdclark@chromium.org> > > DRM_MODESET_LOCK_ALL_BEGIN() has a hidden trap-door (aka retry loop), > which means we can't rely too much on variable initializers. > > Fixes: 6e455f5dcdd1 ("drm/crtc: fix uninitialized variable use") > Signed-off-by: Rob Clark <robdclark@chromium.org> > --- > I have mixed feelings about DRM_MODESET_LOCK_ALL_BEGIN() (and friends) > magic. On one hand it simplifies the deadlock/back dance. OTOH it > conceals a nasty sharp edge. Maybe it is better to have the complicated > restart path a bit more explicit, like it was originally. > > drivers/gpu/drm/drm_crtc.c | 1 + > 1 file changed, 1 insertion(+) > Nice catch !! Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
On Mon, 12 Feb 2024 at 23:55, Rob Clark <robdclark@gmail.com> wrote: > > From: Rob Clark <robdclark@chromium.org> > > DRM_MODESET_LOCK_ALL_BEGIN() has a hidden trap-door (aka retry loop), > which means we can't rely too much on variable initializers. > > Fixes: 6e455f5dcdd1 ("drm/crtc: fix uninitialized variable use") > Signed-off-by: Rob Clark <robdclark@chromium.org> > --- > I have mixed feelings about DRM_MODESET_LOCK_ALL_BEGIN() (and friends) > magic. On one hand it simplifies the deadlock/back dance. OTOH it > conceals a nasty sharp edge. Maybe it is better to have the complicated > restart path a bit more explicit, like it was originally. > > drivers/gpu/drm/drm_crtc.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > index cb90e70d85e8..65f9f66933bb 100644 > --- a/drivers/gpu/drm/drm_crtc.c > +++ b/drivers/gpu/drm/drm_crtc.c > @@ -904,6 +904,7 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data, > connector_set = NULL; > fb = NULL; > mode = NULL; > + num_connectors = 0; Nit: I think we should move all this next to the DRM_MODESET_LOCK_ALL_BEGIN() and drop initialisation from the prologue of the function, but it's definitely a separate and more intrusive story. Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
On Mon, Feb 12, 2024 at 01:55:34PM -0800, Rob Clark wrote: > From: Rob Clark <robdclark@chromium.org> > > DRM_MODESET_LOCK_ALL_BEGIN() has a hidden trap-door (aka retry loop), > which means we can't rely too much on variable initializers. > > Fixes: 6e455f5dcdd1 ("drm/crtc: fix uninitialized variable use") > Signed-off-by: Rob Clark <robdclark@chromium.org> Cc: stable@vger.kernel.org I guess at least ... > --- > I have mixed feelings about DRM_MODESET_LOCK_ALL_BEGIN() (and friends) > magic. On one hand it simplifies the deadlock/back dance. OTOH it > conceals a nasty sharp edge. Maybe it is better to have the complicated > restart path a bit more explicit, like it was originally. Alternative would be a very magic for {} macro, but those tend to get easily defeated with goto/break and friends. Maybe it works now that linux/cleanup.h has landed, but I'm not sure it's actually better and not just trading one set of sharp edges for another. Worth a shot in a prototype at least, if someone's bored. > > drivers/gpu/drm/drm_crtc.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > index cb90e70d85e8..65f9f66933bb 100644 > --- a/drivers/gpu/drm/drm_crtc.c > +++ b/drivers/gpu/drm/drm_crtc.c > @@ -904,6 +904,7 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data, > connector_set = NULL; > fb = NULL; > mode = NULL; > + num_connectors = 0; I think it'd be neater to have all these right below the DRM_MODESET_LOCK_A_BEGIN instead of duplicated here and at the beginning of the function. But it's a bit a bikeshed and in some cases (when you use it later on) gcc might get pissed too. So either way: Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> Cheers, Sima > > DRM_MODESET_LOCK_ALL_END(dev, ctx, ret); > > -- > 2.43.0 >
On Tue, 13 Feb 2024 at 15:50, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > On Mon, 12 Feb 2024 at 23:55, Rob Clark <robdclark@gmail.com> wrote: > > > > From: Rob Clark <robdclark@chromium.org> > > > > DRM_MODESET_LOCK_ALL_BEGIN() has a hidden trap-door (aka retry loop), > > which means we can't rely too much on variable initializers. > > > > Fixes: 6e455f5dcdd1 ("drm/crtc: fix uninitialized variable use") > > Signed-off-by: Rob Clark <robdclark@chromium.org> > > --- > > I have mixed feelings about DRM_MODESET_LOCK_ALL_BEGIN() (and friends) > > magic. On one hand it simplifies the deadlock/back dance. OTOH it > > conceals a nasty sharp edge. Maybe it is better to have the complicated > > restart path a bit more explicit, like it was originally. > > > > drivers/gpu/drm/drm_crtc.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > > index cb90e70d85e8..65f9f66933bb 100644 > > --- a/drivers/gpu/drm/drm_crtc.c > > +++ b/drivers/gpu/drm/drm_crtc.c > > @@ -904,6 +904,7 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data, > > connector_set = NULL; > > fb = NULL; > > mode = NULL; > > + num_connectors = 0; > > Nit: I think we should move all this next to the > DRM_MODESET_LOCK_ALL_BEGIN() and drop initialisation from the prologue > of the function, but it's definitely a separate and more intrusive > story. > > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> After running this through CI: Tested-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> #sc7180, sdm845 > > DRM_MODESET_LOCK_ALL_END(dev, ctx, ret); > > > -- > With best wishes > Dmitry
On Mon, 12 Feb 2024 13:55:34 -0800, Rob Clark wrote: > DRM_MODESET_LOCK_ALL_BEGIN() has a hidden trap-door (aka retry loop), > which means we can't rely too much on variable initializers. > > Applied to drm-misc-fixes, thanks! [1/1] drm/crtc: fix uninitialized variable use even harder commit: b6802b61a9d0e99dcfa6fff7c50db7c48a9623d3 Best regards,
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index cb90e70d85e8..65f9f66933bb 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -904,6 +904,7 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data, connector_set = NULL; fb = NULL; mode = NULL; + num_connectors = 0; DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);