Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756099AbcC2ITh (ORCPT ); Tue, 29 Mar 2016 04:19:37 -0400 Received: from mail-wm0-f49.google.com ([74.125.82.49]:35499 "EHLO mail-wm0-f49.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751217AbcC2ITd (ORCPT ); Tue, 29 Mar 2016 04:19:33 -0400 Date: Tue, 29 Mar 2016 10:19:35 +0200 From: Daniel Vetter To: Alexey Brodkin Cc: "daniel@ffwll.ch" , "dh.herrmann@gmail.com" , "dri-devel@lists.freedesktop.org" , "linux-kernel@vger.kernel.org" , "airlied@linux.ie" Subject: Re: [PATCH 2/4 v3] drm: Introduce drm_connector_register_all() helper Message-ID: <20160329081935.GK2510@phenom.ffwll.local> Mail-Followup-To: Alexey Brodkin , "dh.herrmann@gmail.com" , "dri-devel@lists.freedesktop.org" , "linux-kernel@vger.kernel.org" , "airlied@linux.ie" References: <1458722577-20283-1-git-send-email-abrodkin@synopsys.com> <1458722577-20283-3-git-send-email-abrodkin@synopsys.com> <1458740464.3213.9.camel@synopsys.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1458740464.3213.9.camel@synopsys.com> X-Operating-System: Linux phenom 4.4.0-1-amd64 User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4202 Lines: 105 On Wed, Mar 23, 2016 at 01:41:05PM +0000, Alexey Brodkin wrote: > Hi David, > > On Wed, 2016-03-23 at 12:13 +0100, David Herrmann wrote: > > Hi > > > > On Wed, Mar 23, 2016 at 9:42 AM, Alexey Brodkin > > wrote: > > > > > > As a pair to already existing drm_connector_unregister_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 the generic one. > > > > > > Signed-off-by: Alexey Brodkin > > > Cc: Daniel Vetter > > > Cc: David Airlie > > > --- > > > > > > Changes v2 -> v3: > > > ?* Updated title with capital after colon > > > ?* Simplified failure path with direct and unconditional invocation of > > > ???unregister_all() > > > ?* Updated kerneldoc description of the drm_connector_register_all() > > > > > > Changes v1 -> v2: > > > ?* Rename drm_connector_unplug_all() to drm_connector_unregister_all() > > > ?* Use drm_for_each_connector() instead of list_for_each_entry() > > > ?* Updated kerneldoc for drm_dev_register() > > > > > > ?drivers/gpu/drm/drm_crtc.c | 43 +++++++++++++++++++++++++++++++++++++++++++ > > > ?drivers/gpu/drm/drm_drv.c??|??6 +++++- > > > ?include/drm/drm_crtc.h?????|??3 ++- > > > ?3 files changed, 50 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > > > index 65488a6..21eea11 100644 > > > --- a/drivers/gpu/drm/drm_crtc.c > > > +++ b/drivers/gpu/drm/drm_crtc.c > > > @@ -1081,6 +1081,49 @@ void drm_connector_unregister(struct drm_connector *connector) > > > ?EXPORT_SYMBOL(drm_connector_unregister); > > > > > > ?/** > > > + * drm_connector_register_all - register all connectors > > > + * @dev: drm device > > > + * > > > + * This function registers all connectors in sysfs and other places so that > > > + * userspace can start to access them. Drivers can call it after calling > > > + * drm_dev_register() to complete the device registration, if they don't call > > > + * drm_connector_register() on each connector individually. > > > + * > > > + * When a device is unplugged and should be removed from userspace access, > > > + * call drm_connector_unregister_all(), which is the inverse of this > > > + * function. > > > + * > > > + * Returns: > > > + * Zero on success, error code on failure. > > > + */ > > > +int drm_connector_register_all(struct drm_device *dev) > > > +{ > > > +???????struct drm_connector *connector; > > > +???????int ret; > > > + > > > +???????mutex_lock(&dev->mode_config.mutex); > > > + > > > +???????drm_for_each_connector(connector, dev) { > > > +???????????????ret = drm_connector_register(connector); > > > +???????????????if (ret) { > > > +???????????????????????/* > > > +????????????????????????* We may safely call unregister_all() here within > > > +????????????????????????* area locked with mutex because unregister_all() > > > +????????????????????????* doesn't use locks inside (see a comment in that > > > +????????????????????????* function). > > > +????????????????????????*/ > > Ugh? unregister_all() says: > > > > /* FIXME: taking the mode config mutex ends up in a clash with sysfs */ > > > > This strongly contradicts your comment. Anyway, regardless how you > > want to fix it: You better unlock the mode-config mutex before > > returning below. > > So good catch. > But what I really meant since we didn't get any further after registering > all "good" connections (see we're not releasing mutex still) the will be > no clashes with sysfs. > > Still I;d like Daniel to comment on that separately. I think doing the unregister_all outside of the loop&locked section is better for future-proofing. My long-term plan for connector lifetimes and the connector list is: - refcounting for connectors (Dave has wip patches already). - separate lock for the connector list (and only that). Doing it entirely separate would make things easier and more robust. I merged patch 1 meanwhile to drm-misc. Thanks, Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch