[PATCH] staging/vboxvideo: Another FIXME item

[PATCH] staging/vboxvideo: Another FIXME item

[Daniel Vetter]

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.
+	 */
 	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
 		vbox_conn = to_vbox_connector(connector);
 
-- 
2.20.1

Re: [PATCH] staging/vboxvideo: Another FIXME item

[Greg Kroah-Hartman]

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>

Re: [PATCH] staging/vboxvideo: Another FIXME item

[Hans de Goede]

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

Re: [PATCH] staging/vboxvideo: Another FIXME item

[Daniel Vetter]

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch