linux-kernel - Re: [PATCH] staging/vboxvideo: Another FIXME item

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Thu, 21 Feb 2019 17:49:52 +0100
From:   Daniel Vetter <daniel@...ll.ch>
To:     Hans de Goede <hdegoede@...hat.com>
Cc:     Daniel Vetter <daniel.vetter@...ll.ch>,
        DRI Development <dri-devel@...ts.freedesktop.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Sam Ravnborg <sam@...nborg.org>,
        Liviu Dudau <liviu.dudau@....com>
Subject: Re: [PATCH] staging/vboxvideo: Another FIXME item

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@...ll.ch>
> > Cc: Hans de Goede <hdegoede@...hat.com>
> > Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> > Cc: Sam Ravnborg <sam@...nborg.org>
> > Cc: Daniel Vetter <daniel.vetter@...ll.ch>
> > Cc: Liviu Dudau <liviu.dudau@....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