| 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
| ||
|
Message-ID: <20160319100258.GP14170@phenom.ffwll.local>
Date: Sat, 19 Mar 2016 11:02:58 +0100
From: Daniel Vetter <daniel@...ll.ch>
To: Alexey Brodkin <Alexey.Brodkin@...opsys.com>
Cc: "daniel@...ll.ch" <daniel@...ll.ch>,
"dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"airlied@...ux.ie" <airlied@...ux.ie>
Subject: Re: [PATCH 1/3] drm: introduce drm_connector_plug_all() helper
On Fri, Mar 18, 2016 at 09:58:49PM +0000, Alexey Brodkin wrote:
> Hi Daniel,
>
> On Fri, 2016-03-18 at 19:06 +0100, Daniel Vetter wrote:
> > On Fri, Mar 18, 2016 at 01:01:42PM +0300, Alexey Brodkin wrote:
> > >
> > > As a pair to already existing drm_connector_unplug_all() we're adding
> > > generic implementation of what is already done in some drivers.
> > >
> > > Once this helper is implemented we'll be ready to switch existing
> > > driver-specific implementations with generic one.
> > >
> > > Signed-off-by: Alexey Brodkin <abrodkin@...opsys.com>
> > > Cc: Daniel Vetter <daniel@...ll.ch>
> > > Cc: David Airlie <airlied@...ux.ie>
> > > ---
> > > drivers/gpu/drm/drm_crtc.c | 44 +++++++++++++++++++++++++++++++++++++++++++-
> > > drivers/gpu/drm/drm_drv.c | 3 ++-
> > > include/drm/drm_crtc.h | 3 ++-
> > > 3 files changed, 47 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> > > index 65258ac..ce27420 100644
> > > --- a/drivers/gpu/drm/drm_crtc.c
> > > +++ b/drivers/gpu/drm/drm_crtc.c
> > > @@ -1080,6 +1080,46 @@ void drm_connector_unregister(struct drm_connector *connector)
> > > }
> > > EXPORT_SYMBOL(drm_connector_unregister);
> > >
> > > +/**
> > > + * drm_connector_plug_all - register connector userspace interfaces
> > > + * @dev: drm device
> > > + *
> > > + * This function registers all connector userspace interfaces in sysfs. Should
> > > + * be call when the device is disconnected, e.g. from an usb driver's
> > Still talks about disconnect ;-) Please also mention that this just calls
> > drm_connector_register() exactly like this including () to generate a
> > kerneldoc hyperlink.
>
> Well I intentionally left in description of drm_connector_register_all():
> "Should be call when the device is disconnected, e.g. from an usb driver's
> ->connect callback."
You use "disconnected" for connecting stuff. That doesn't make sense to me
at all - register_all is for when you want to publish something, not for
unpublishing when the device disappears. Or maybe this is a case of lost
in translation, and you mean something else?
-Daniel
> I did mean it. Or is this statement is incorrect and example of invocation of
> drm_connector_register_all() should be different? Which one works better then?
>
> > >
> > > + * ->connect callback.
> > Returns: section is missing, specifying how this can fail. Just copy the
> > one from connector_register().
>
> Yeah, correct. Blind copy-paste doesn't work equally good always :(
>
> > >
> > > + */
> > > +int drm_connector_plug_all(struct drm_device *dev)
> > > +{
> > > + struct drm_connector *connector, *failed;
> > > + int ret;
> > > +
> > > + mutex_lock(&dev->mode_config.mutex);
> > > +
> > > + list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
> > for_each_connector here please. And the s/plug/register/ naming discussion
> > we've had.
>
> Ok.
>
> > >
> > > + ret = drm_connector_register(connector);
> > > + if (ret) {
> > > + failed = connector;
> > > + goto err;
> > > + }
> > > + }
> > > +
> > > + mutex_unlock(&dev->mode_config.mutex);
> > > +
> > > + return 0;
> > > +
> > > +err:
> > > + list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
> > > + if (failed == connector)
> > > + break;
> > > +
> > > + drm_connector_unregister(connector);
> > > + }
> > > +
> > > + mutex_unlock(&dev->mode_config.mutex);
> > > +
> > > + return ret;
> > > +}
> > > +EXPORT_SYMBOL(drm_connector_plug_all);
> > >
> > > /**
> > > * drm_connector_unplug_all - unregister connector userspace interfaces
> > > @@ -1093,10 +1133,12 @@ void drm_connector_unplug_all(struct drm_device *dev)
> > > {
> > > struct drm_connector *connector;
> > >
> > > - /* FIXME: taking the mode config mutex ends up in a clash with sysfs */
> > > + mutex_lock(&dev->mode_config.mutex);
> > You can't drop that FIXME, the bug is still there.
>
> That's clear given your explanation in the previous email.
>
> > >
> > > +
> > > list_for_each_entry(connector, &dev->mode_config.connector_list, head)
> > > drm_connector_unregister(connector);
> > >
> > > + mutex_unlock(&dev->mode_config.mutex);
> > > }
> > > EXPORT_SYMBOL(drm_connector_unplug_all);
> > >
> > > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> > > index 167c8d3..4a559c6 100644
> > > --- a/drivers/gpu/drm/drm_drv.c
> > > +++ b/drivers/gpu/drm/drm_drv.c
> > > @@ -715,7 +715,8 @@ EXPORT_SYMBOL(drm_dev_unref);
> > > *
> > > * Register the DRM device @dev with the system, advertise device to user-space
> > > * and start normal device operation. @dev must be allocated via drm_dev_alloc()
> > > - * previously.
> > > + * previously and right after drm_dev_register() driver should call
> > It'd do 2 sentences here for simplicity, not connect them with and. Also
> > "... _the_ driver should ..."
>
> Ok.
>
> > >
> > > + * drm_connector_plug_all() to register all connectors in sysfs.
> > Maybe mention why this is separate: "This is a separate call for backwards
> > compatibility with drivers still using the deprecated ->load() callback,
> > where connectors are registered from within the ->load() callback."
>
> Ok.
>
> -Alexey
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Powered by blists - more mailing lists