From patchwork Wed Jun 6 20:55:01 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kim Phillips X-Patchwork-Id: 10450933 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 3AFD960146 for ; Wed, 6 Jun 2018 20:55:30 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 2998C29D59 for ; Wed, 6 Jun 2018 20:55:30 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 1C66129D69; Wed, 6 Jun 2018 20:55:30 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-2.9 required=2.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,MAILING_LIST_MULTI autolearn=unavailable version=3.3.1 Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id ABE5B29D59 for ; Wed, 6 Jun 2018 20:55:29 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:Mime-Version:References:In-Reply-To: Message-Id:Subject:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=nFX4K0DIKjNDGsk7jmvpFhvB7BUt6f/VgVbkVBLerRw=; b=gX1GBt13U2/3Uv sFhwWfy4AXmx/ExxxW5G9/P34fMczuK1D9jQWrFXTMvsZoA9HEGH6bOE8xdzxuslzHVTlLAqOn/Yn fDFTXs9lD7HXJOq34ZupKmmdAhW9HlcUMn/2qkyghoqzr7sPCg4lpH4n5Z63ZzMmi6Mfr34yirTg0 a3n7wcSCDpV4lHfPe4XLFgnBUPWKBY+IFfEbozEcXnRauZbb0cX954cUQrEz7lP2GRdZAgoolS3sE rQbslmH0Bv8/3fSBVAEb4DBMX1um5spme61WN1B9SC7UWUI9cvj/aQXJqD9q6/qr9JEA+fdm8BfCN wgBR11L0qZT4BGdfTzNg==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.90_1 #2 (Red Hat Linux)) id 1fQfSi-0002rM-Hb; Wed, 06 Jun 2018 20:55:20 +0000 Received: from foss.arm.com ([217.140.101.70]) by bombadil.infradead.org with esmtp (Exim 4.90_1 #2 (Red Hat Linux)) id 1fQfSe-0001gS-Ij for linux-arm-kernel@lists.infradead.org; Wed, 06 Jun 2018 20:55:18 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id DA25680D; Wed, 6 Jun 2018 13:55:02 -0700 (PDT) Received: from dupont (dupont.austin.arm.com [10.118.16.87]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id CF7BA3F59D; Wed, 6 Jun 2018 13:55:01 -0700 (PDT) Date: Wed, 6 Jun 2018 15:55:01 -0500 From: Kim Phillips To: Suzuki K Poulose Subject: Re: [PATCH v4 05/14] coresight: get/put module in coresight_build/release_path Message-Id: <20180606155501.704583e1412996a1a2c6fa61@arm.com> In-Reply-To: References: <20180605210710.22227-1-kim.phillips@arm.com> <20180605210710.22227-6-kim.phillips@arm.com> <20180606082422.GB19727@kroah.com> Organization: Arm X-Mailer: Sylpheed 3.7.0 (GTK+ 2.24.32; x86_64-pc-linux-gnu) Mime-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20180606_135516_636681_7F840344 X-CRM114-Status: GOOD ( 26.16 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Kefeng Wang , Geert Uytterhoeven , Alexander Shishkin , Gargi Sharma , David Howells , Russell King , Pavel Tatashin , Thierry Reding , Rik van Riel , Eric Auger , Alex Williamson , Mike Rapoport , linux-arm-kernel , Mathieu Poirier , Greg Kroah-Hartman , Randy Dunlap , Oleg Nesterov , Linux Kernel Mailing List , Kirill Tkhai , Eric Biederman , Leo Yan , Andrew Morton , Robin Murphy , Todd Kjos Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Virus-Scanned: ClamAV using ClamSMTP On Wed, 6 Jun 2018 10:46:36 +0100 Suzuki K Poulose wrote: > On 06/06/2018 09:24 AM, Greg Kroah-Hartman wrote: > > On Tue, Jun 05, 2018 at 04:07:01PM -0500, Kim Phillips wrote: > >> Increment the refcnt for driver modules in current use by calling > >> module_get in coresight_build_path and module_put in release_path. > >> > >> This prevents driver modules from being unloaded when they are in use, > >> either in sysfs or perf mode. > > > > Why does it matter? Shouldn't you be allowed to remove any module at > > any point in time, much like a networking driver? > > > > > >> > >> Cc: Mathieu Poirier > >> Cc: Leo Yan > >> Cc: Alexander Shishkin > >> Cc: Randy Dunlap > >> Cc: Suzuki K Poulose > >> Cc: Greg Kroah-Hartman > >> Cc: Russell King > >> Signed-off-by: Kim Phillips > >> --- > >> drivers/hwtracing/coresight/coresight.c | 9 +++++++++ > >> 1 file changed, 9 insertions(+) > >> > >> diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c > >> index 338f1719641c..1c941351f1d1 100644 > >> --- a/drivers/hwtracing/coresight/coresight.c > >> +++ b/drivers/hwtracing/coresight/coresight.c > >> @@ -465,6 +465,12 @@ static int _coresight_build_path(struct coresight_device *csdev, > >> > >> node->csdev = csdev; > >> list_add(&node->link, path); > >> + > >> + if (!try_module_get(csdev->dev.parent->driver->owner)) { > > > > What is to keep parent->driver from going away right here? What keeps > > parent around? This feels very fragile to me, I don't see any locking > > anywhere around this code path to try to keep things in place. > > You're right. We do have coresight_mutex, which is held across the build > path and the csdev is removed when a device is unregistered. However, I > see that we don't hold the mutex while removing the connections from > coresight_unregister(). Holding the mutex should protect us from the > csdev being removed, while we build the path. OK, I'll add this for the next version: Thanks, Kim diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c index f96258de1e9b..da702507a55c 100644 --- a/drivers/hwtracing/coresight/coresight-core.c +++ b/drivers/hwtracing/coresight/coresight-core.c @@ -1040,8 +1040,12 @@ EXPORT_SYMBOL_GPL(coresight_register); void coresight_unregister(struct coresight_device *csdev) { + mutex_lock(&coresight_mutex); + /* Remove references of that device in the topology */ coresight_remove_conns(csdev); device_unregister(&csdev->dev); + + mutex_unlock(&coresight_mutex); } EXPORT_SYMBOL_GPL(coresight_unregister); > And while we are at this, I also realised that we hold references to the > parent devices for each connection (via bus_find_device() from > of_coresight_get_endpoint_device()), while parsing the platform data, > which is never released. Would this fix that?: diff --git a/drivers/hwtracing/coresight/of_coresight.c b/drivers/hwtracing/coresight/of_coresight.c index a33a92ebe74b..a43ab078c85e 100644 --- a/drivers/hwtracing/coresight/of_coresight.c +++ b/drivers/hwtracing/coresight/of_coresight.c @@ -181,6 +181,8 @@ of_get_coresight_platform_data(struct device *dev, pdata->child_names[i] = dev_name(rdev); pdata->child_ports[i] = rendpoint.id; + put_device(rdev); + i++; } while (ep); }