staging/vboxvideo: Another FIXME item
diff mbox series

Message ID 20190221155951.19855-1-daniel.vetter@ffwll.ch
State New
Headers show
Series
  • staging/vboxvideo: Another FIXME item
Related show

Commit Message

Daniel Vetter Feb. 21, 2019, 3:59 p.m. UTC
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(+)

Comments

Greg KH Feb. 21, 2019, 4:04 p.m. UTC | #1
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>
Hans de Goede Feb. 21, 2019, 4:40 p.m. UTC | #2
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);
>   
>
Daniel Vetter Feb. 21, 2019, 4:49 p.m. UTC | #3
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);
> >

Patch
diff mbox series

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);