| Message ID | 20190221155951.19855-1-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | staging/vboxvideo: Another FIXME item | expand |
On Thu, Feb 21, 2019 at 04:59:51PM +0100, Daniel Vetter wrote: > Found while grepping around. > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: Hans de Goede <hdegoede@redhat.com> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: Sam Ravnborg <sam@ravnborg.org> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: Liviu Dudau <liviu.dudau@arm.com> > --- > drivers/staging/vboxvideo/vbox_irq.c | 5 +++++ > 1 file changed, 5 insertions(+) Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Hi, On 21-02-19 16:59, Daniel Vetter wrote: > Found while grepping around. > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: Hans de Goede <hdegoede@redhat.com> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: Sam Ravnborg <sam@ravnborg.org> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: Liviu Dudau <liviu.dudau@arm.com> > --- > drivers/staging/vboxvideo/vbox_irq.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/staging/vboxvideo/vbox_irq.c b/drivers/staging/vboxvideo/vbox_irq.c > index 195484713365..89944134ea86 100644 > --- a/drivers/staging/vboxvideo/vbox_irq.c > +++ b/drivers/staging/vboxvideo/vbox_irq.c > @@ -123,6 +123,11 @@ static void vbox_update_mode_hints(struct vbox_private *vbox) > > validate_or_set_position_hints(vbox); > drm_modeset_lock_all(dev); > + /* > + * FIXME: this needs to use drm_connector_list_iter and some real > + * locking for the actual data it changes, not the deprecated > + * drm_modeset_lock_all() shotgun approach. > + */ Question, are the locking expectations from the drm's core pov (for modesetting-drivers) *fully* (and clearly) documented somewhere? Regards, Hans > list_for_each_entry(connector, &dev->mode_config.connector_list, head) { > vbox_conn = to_vbox_connector(connector); > >
On Thu, Feb 21, 2019 at 05:40:05PM +0100, Hans de Goede wrote: > Hi, > > On 21-02-19 16:59, Daniel Vetter wrote: > > Found while grepping around. > > > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > Cc: Hans de Goede <hdegoede@redhat.com> > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > Cc: Sam Ravnborg <sam@ravnborg.org> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > Cc: Liviu Dudau <liviu.dudau@arm.com> > > --- > > drivers/staging/vboxvideo/vbox_irq.c | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/drivers/staging/vboxvideo/vbox_irq.c b/drivers/staging/vboxvideo/vbox_irq.c > > index 195484713365..89944134ea86 100644 > > --- a/drivers/staging/vboxvideo/vbox_irq.c > > +++ b/drivers/staging/vboxvideo/vbox_irq.c > > @@ -123,6 +123,11 @@ static void vbox_update_mode_hints(struct vbox_private *vbox) > > validate_or_set_position_hints(vbox); > > drm_modeset_lock_all(dev); > > + /* > > + * FIXME: this needs to use drm_connector_list_iter and some real > > + * locking for the actual data it changes, not the deprecated > > + * drm_modeset_lock_all() shotgun approach. > > + */ > > Question, are the locking expectations from the drm's core pov (for modesetting-drivers) > *fully* (and clearly) documented somewhere? All the things I've touched the past few years wrt locking should have kerneldoc comments explaining the rules. For most drivers you don't need much if any locking though, because the core+helpers take care of everything. So yeah exists, but spread thinly around everywhere. Above is probably fine since you don't hotplug connectors and modeset_lock_all gives you a good chance you have enough locking. But modeset_lock_all is deprecated for atomic drivers, because it makes it unclear what exactly you're protecting against. The usual BKL considered harmful reasons. I just noticed that qxl has the same pattern, probably similarly grown through fairly long history. -Daniel > > Regards, > > Hans > > > > > > list_for_each_entry(connector, &dev->mode_config.connector_list, head) { > > vbox_conn = to_vbox_connector(connector); > >
diff --git a/drivers/staging/vboxvideo/vbox_irq.c b/drivers/staging/vboxvideo/vbox_irq.c index 195484713365..89944134ea86 100644 --- a/drivers/staging/vboxvideo/vbox_irq.c +++ b/drivers/staging/vboxvideo/vbox_irq.c @@ -123,6 +123,11 @@ static void vbox_update_mode_hints(struct vbox_private *vbox) validate_or_set_position_hints(vbox); drm_modeset_lock_all(dev); + /* + * FIXME: this needs to use drm_connector_list_iter and some real + * locking for the actual data it changes, not the deprecated + * drm_modeset_lock_all() shotgun approach. + */ list_for_each_entry(connector, &dev->mode_config.connector_list, head) { vbox_conn = to_vbox_connector(connector);
Found while grepping around. Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> Cc: Hans de Goede <hdegoede@redhat.com> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: Sam Ravnborg <sam@ravnborg.org> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Cc: Liviu Dudau <liviu.dudau@arm.com> --- drivers/staging/vboxvideo/vbox_irq.c | 5 +++++ 1 file changed, 5 insertions(+)