From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.3 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id B03C4C3F2C6 for ; Sat, 29 Feb 2020 11:17:17 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 91D6A246AE for ; Sat, 29 Feb 2020 11:17:17 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 91D6A246AE Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=ravnborg.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=dri-devel-bounces@lists.freedesktop.org Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id B86A06E2ED; Sat, 29 Feb 2020 11:17:15 +0000 (UTC) Received: from asavdk4.altibox.net (asavdk4.altibox.net [109.247.116.15]) by gabe.freedesktop.org (Postfix) with ESMTPS id 28CCE6E225; Sat, 29 Feb 2020 11:17:14 +0000 (UTC) Received: from ravnborg.org (unknown [158.248.194.18]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by asavdk4.altibox.net (Postfix) with ESMTPS id 7D6E580650; Sat, 29 Feb 2020 12:17:11 +0100 (CET) Date: Sat, 29 Feb 2020 12:17:10 +0100 From: Sam Ravnborg To: Daniel Vetter Subject: Re: [PATCH 03/51] drm: add managed resources tied to drm_device Message-ID: <20200229111710.GB3674@ravnborg.org> References: <20200227181522.2711142-1-daniel.vetter@ffwll.ch> <20200227181522.2711142-4-daniel.vetter@ffwll.ch> <20200228224504.GA23961@ravnborg.org> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.10.1 (2018-07-13) X-CMAE-Score: 0 X-CMAE-Analysis: v=2.3 cv=XpTUx2N9 c=1 sm=1 tr=0 a=UWs3HLbX/2nnQ3s7vZ42gw==:117 a=UWs3HLbX/2nnQ3s7vZ42gw==:17 a=jpOVt7BSZ2e4Z31A5e1TngXxSK0=:19 a=kj9zAlcOel0A:10 a=DRRiGCmsZTF5usyg21AA:9 a=CjuIK1q_8ugA:10 X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: "Rafael J. Wysocki" , Greg Kroah-Hartman , Intel Graphics Development , Marco Felsch , DRI Development , Laurent Pinchart , Daniel Vetter Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" Hi Daniel. > > > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c > > > index 9fcd6ab3c154..3e5627d6eba6 100644 > > > --- a/drivers/gpu/drm/drm_drv.c > > > +++ b/drivers/gpu/drm/drm_drv.c > > > @@ -629,6 +629,9 @@ int drm_dev_init(struct drm_device *dev, > > > dev->dev = get_device(parent); > > > dev->driver = driver; > > > > > > + INIT_LIST_HEAD(&dev->managed.resources); > > > + spin_lock_init(&dev->managed.lock); > > > + > > > /* no per-device feature limits by default */ > > > dev->driver_features = ~0u; > > > > > > @@ -828,8 +831,16 @@ static void drm_dev_release(struct kref *ref) > > > dev->driver->release(dev); > > > } else { > > > drm_dev_fini(dev); > > > - kfree(dev); > > > + if (!dev->managed.final_kfree) { > > > + WARN_ON(!list_empty(&dev->managed.resources)); > > > + kfree(dev); > > > + } > > > > This looks sub-optimal. > > We cannot be sure a driver have used drmm_add_final_kfree() if it makes > > use of drmm_. > > So we may not WARN in all relavant cases. > > Also, we cannot expect all drivers that uses devmm_ to have managed > > to get rid of their ->release call-back. > > The above is purely transition code. It gets cleaned up once all > drivers call drmm_add_final_kfree(). This all disappears again, but > indeed looks like the interim state isn't quite what we want. > > > So the right thing looks to me like we should move it out to be > > unconditional. Se we will WARN_ON(!list_empty(&dev->managed.resources)) > > always. > > Until the driver has set drmm_add_final_kfree it's actually dangerous > to use the drmm stuff. Exactly because of the use-after-free you point > out below. Hence the warning to make sure there's no release actions. > I'll shuffle this around to make sure we call kfree last for all > possible paths and make sure this bisects all correctly. I was just reviewing the code I had on hand, and did not look further in the set of patches. Very good if we can keep is bisectable. > > > + * > > > + * Based on drivers/base/devres.c > > > + */ > > > + > > > +#include > > > + > > > +#include > > > +#include > > > +#include > > > + > > > +#include > > > +#include > > > > It is good practice to group the include files. > > And drm/ comes after linux/ > > I try to put the main header first to make sure it's stand-alone, but > I guess that works with the header check now? Do I need to do anything > to get that checked? The header-check infrastructure was dropped again - see: fcbb8461fd2376ba3782b5b8bd440c929b8e4980 So including it as the first header in the implmentation file is likely the best way to keep it self contained. We will spot errors sooner. > > > +static __always_inline struct drmres * alloc_dr(drmres_release_t release, > > > + size_t size, gfp_t gfp, int nid) > > Why do we force the compiler to inline this? > > Seems a little agressive. > > It's not for performance, but for kmalloc_trace_caller. No point if > our caller is always some boring function from drm_managed.c that > calls alloc_dr. If we force alloc_dr to inline, then we get the caller > of the drm_managed.c function traced as allocator. Much better. > > (I stole that trick from devres.c) > > I'll add a comment to explain this. Thanks. > > > All the two users so far uses dev_to_node(dev->dev) for the nid. > > Maybe let this function take a drm_device * and thus move the > > calculation to this function? > > Copypastes like that :-) I feel somewhat meh here ... Well - keep the diff for devres smaller for now and leave it. It was just an observation. > > > + /** > > > + * @managed: > > > + * > > > + * Managed resources linked to the lifetime of this &drm_device as > > > + * tracked by @ref. > > > + */ > > > + struct { > > > + struct list_head resources; > > > + void *final_kfree; > > > + spinlock_t lock; > > > + } managed; > > > > I am missing kernel-doc here. > > At least document that lock is used to guard access to resources. > > (s/lock/lock_resources/ ?) > > Dunno why, but the support for name sub-structures seems to have > broken in kerneldoc. So I can type it, but it's not showing up, so I > didn't bother. Well I had it, but deleted it again. It's still > documented to work, but I have no idea what I'm doing wrong. Most readers prefer the .c files as the source. I personally read the generated kernel doc when I google and when I check that my own stuff looks good in kernel-doc format. So comments are still valueable despite not being picked up by kernel-doc. You know this - but I just wanted to encourage you to write the few lines that may help me and others :-) Sam _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel