From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Herrmann Subject: Re: [PATCH 03/12] drm: call ->firstopen() before ->open() Date: Thu, 24 Jul 2014 11:31:05 +0200 Message-ID: References: <1406129207-1302-1-git-send-email-dh.herrmann@gmail.com> <1406129207-1302-4-git-send-email-dh.herrmann@gmail.com> <20140723192529.GZ15237@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-ie0-f169.google.com (mail-ie0-f169.google.com [209.85.223.169]) by gabe.freedesktop.org (Postfix) with ESMTP id 275666E6D5 for ; Thu, 24 Jul 2014 02:31:06 -0700 (PDT) Received: by mail-ie0-f169.google.com with SMTP id rd18so2072769iec.28 for ; Thu, 24 Jul 2014 02:31:05 -0700 (PDT) In-Reply-To: <20140723192529.GZ15237@phenom.ffwll.local> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Daniel Vetter Cc: Daniel Vetter , "dri-devel@lists.freedesktop.org" List-Id: dri-devel@lists.freedesktop.org Hi On Wed, Jul 23, 2014 at 9:25 PM, Daniel Vetter wrote: > On Wed, Jul 23, 2014 at 05:26:38PM +0200, David Herrmann wrote: >> Lets order things correctly: >> ->load() >> ->fistopen() >> ->open() >> ->close() >> ->lastclose() >> ->unload() >> >> This doesn't change much as only savage and radeon use ->firstopen() and >> they just do map-initialization. Therefore, the global drm mutex makes >> sure there cannot be any other f_op between ->open() and ->firstopen(). >> However, once we get rid of that lock, we really want ->firstopen() to >> initialize the device before ->open() is called. >> >> Furthermore, this fixes the clean-up path in drm_open(). We currently >> don't cleanup the drm_file object if ->firstopen() fails. >> >> Signed-off-by: David Herrmann >> --- >> drivers/gpu/drm/drm_fops.c | 139 +++++++++++++++++++++------------------------ >> include/drm/drmP.h | 2 +- >> 2 files changed, 66 insertions(+), 75 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c >> index 021fe5d..8e73519 100644 >> --- a/drivers/gpu/drm/drm_fops.c >> +++ b/drivers/gpu/drm/drm_fops.c >> @@ -45,7 +45,7 @@ EXPORT_SYMBOL(drm_global_mutex); >> >> static int drm_open_helper(struct file *filp, struct drm_minor *minor); >> >> -static int drm_setup(struct drm_device * dev) >> +static int drm_firstopen(struct drm_device * dev) > > All the stuff in here is only for legacy drivers. Imo we should rename > this to drm_legacy_setup and hide it harder. > > Also touching the init ordering for ums drivers is a bit risky, I'd advise > against it. firstopen is officially dead for kms driver and really there's > nothing legit you can do in there. imx abused until they've switched over > to the component framework. > > I'd just drop this one tbh. I did a driver audit when writing this patch, there're only 2 drivers using firstopen: * radeon_cp: sets two fields of dev_private and calls drm_addmap() on those. In lastclose(), it calls drm_rmmap() * savage: sets several fields in dev_private, calls arch_phys_wc_add() for some regions and requests a drm-map for those via drm_addmap(). On lastclose(), it reverts exactly those steps. Taking into account that firstopen() just runs with drm_device context, not with drm_file context, I really think we should be calling it _before_ doing ->open(). I even think the drivers would fail horribly if we don't do this patch and some other user-context sneaks in between both calls (currently impossible thanks to drm_global_mutex). Both ->firstopen() callbacks are fairly trivial. In favor of making the error-paths work in drm_open() I'd really like to get this in. This patch is preparing for my drm_file rework that can make drm_dev_unregister() stop all open files immediately. But I honestly don't care much for UMS drivers, so if you NACK this, I'd just ignore the error code and add a comment that we don't do error-handling for UMS. Cheers David