| Message ID | 20240404203336.10454-5-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/client: Use after free and debug improvements | expand |
On Thu, Apr 04, 2024 at 11:33:28PM +0300, Ville Syrjala wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > crtc->mode is legacy junk and shouldn't really be used with > atomic drivers. > > Most (all?) atomic drivers do end up still calling > drm_atomic_helper_update_legacy_modeset_state() at some > point, so crtc->mode does still get populated, and this > does work for now. But eventually would be nice to eliminate > all the legacy stuff from atomic drivers. > > Switching to crtc->state->mode would require some bigger > changes however, as we currently drop the crtc->mutex > before we're done using the mode. So leave the junk in > for now and just add a FIXME to remind us that this > needs fixing. What about using allocated duplicate modes to fill modes[] array? This requires additional allocations, but it will solve most if not all modes lifetime issues. > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/drm_client_modeset.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c > index 2b7d0be04911..8ef03608b424 100644 > --- a/drivers/gpu/drm/drm_client_modeset.c > +++ b/drivers/gpu/drm/drm_client_modeset.c > @@ -699,6 +699,10 @@ static bool drm_client_firmware_config(struct drm_client_dev *client, > * > * This is crtc->mode and not crtc->state->mode for the > * fastboot check to work correctly. > + * > + * FIXME using legacy crtc->mode with atomic drivers > + * is dodgy. Switch to crtc->state->mode, after taking > + * care of the resulting locking/lifetime issues. > */ > DRM_DEBUG_KMS("looking for current mode on connector %s\n", > connector->name); > -- > 2.43.2 >
On Fri, Apr 05, 2024 at 06:32:46AM +0300, Dmitry Baryshkov wrote: > On Thu, Apr 04, 2024 at 11:33:28PM +0300, Ville Syrjala wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > crtc->mode is legacy junk and shouldn't really be used with > > atomic drivers. > > > > Most (all?) atomic drivers do end up still calling > > drm_atomic_helper_update_legacy_modeset_state() at some > > point, so crtc->mode does still get populated, and this > > does work for now. But eventually would be nice to eliminate > > all the legacy stuff from atomic drivers. > > > > Switching to crtc->state->mode would require some bigger > > changes however, as we currently drop the crtc->mutex > > before we're done using the mode. So leave the junk in > > for now and just add a FIXME to remind us that this > > needs fixing. > > > What about using allocated duplicate modes to fill modes[] array? This > requires additional allocations, but it will solve most if not all modes > lifetime issues. I think there are two obvious solutions: 1. drm_mode_duplicate() as you suggest upside: existing 'modes[i] != NULL' checks work as is downside: introduces more error paths due to potential kmalloc() fails 2. Make modes[] and array of structs rather than pointers up/downsides: the opposite of option 1 So neither is a trivial search and replace job. > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/drm_client_modeset.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c > > index 2b7d0be04911..8ef03608b424 100644 > > --- a/drivers/gpu/drm/drm_client_modeset.c > > +++ b/drivers/gpu/drm/drm_client_modeset.c > > @@ -699,6 +699,10 @@ static bool drm_client_firmware_config(struct drm_client_dev *client, > > * > > * This is crtc->mode and not crtc->state->mode for the > > * fastboot check to work correctly. > > + * > > + * FIXME using legacy crtc->mode with atomic drivers > > + * is dodgy. Switch to crtc->state->mode, after taking > > + * care of the resulting locking/lifetime issues. > > */ > > DRM_DEBUG_KMS("looking for current mode on connector %s\n", > > connector->name); > > -- > > 2.43.2 > > > > -- > With best wishes > Dmitry
diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c index 2b7d0be04911..8ef03608b424 100644 --- a/drivers/gpu/drm/drm_client_modeset.c +++ b/drivers/gpu/drm/drm_client_modeset.c @@ -699,6 +699,10 @@ static bool drm_client_firmware_config(struct drm_client_dev *client, * * This is crtc->mode and not crtc->state->mode for the * fastboot check to work correctly. + * + * FIXME using legacy crtc->mode with atomic drivers + * is dodgy. Switch to crtc->state->mode, after taking + * care of the resulting locking/lifetime issues. */ DRM_DEBUG_KMS("looking for current mode on connector %s\n", connector->name);