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=-10.0 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED 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 15D87C433E1 for ; Thu, 23 Jul 2020 18:20:09 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (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 D917E20714 for ; Thu, 23 Jul 2020 18:20:08 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="eUx/XyVS"; dkim=fail reason="signature verification failed" (1024-bit key) header.d=kernel.org header.i=@kernel.org header.b="v7NIXoMd" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org D917E20714 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=linuxfoundation.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: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=IA2/vH23G7dof4p/9BbQ7b0c6Xp2/O0+g5xp4/8qsIc=; b=eUx/XyVSxL+IgKa3guje6gNCs wPF+ltwq1ZMssDM+kGPBbV2drTk2n6P7Dq3WtCgGtpjzLysYQMxg42g3G2AobUwSEmYMIxP4TRvjr vzk+WRjNuAjFeop/HeYBS6Kv+HAo2wWC+K2m71CSHG5cw2sQxzM0mHf367rx03BY0CJfCWmpCOudm /gtyoljjLCXUCmliJBzb99NJQ/+N9hactq1STwbSDZQYnb6YNRZW6+yFc+vQ9oKm38XTGxIMlr8Bj eciclY84R9CiceAP2Ql81KQHK8ml5zArhAzm4arHZ5cgYeZIUuONmvbDt7FQWZd4A7/fcRBqFoeHt Z5lBHNa6g==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1jyfnh-0004SA-Qu; Thu, 23 Jul 2020 18:18:37 +0000 Received: from mail.kernel.org ([198.145.29.99]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1jyfne-0004RC-BM for linux-arm-kernel@lists.infradead.org; Thu, 23 Jul 2020 18:18:35 +0000 Received: from localhost (83-86-89-107.cable.dynamic.v4.ziggo.nl [83.86.89.107]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id ED0A120714; Thu, 23 Jul 2020 18:18:32 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1595528313; bh=JIqZJqb1+Do9N9YcDPQqhhMfkwIMKAsj0AjJesPA83I=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=v7NIXoMdo0JKV0rMpvPnh0YwIU1d4/3TrV2gqkJwZzJ+/9XNDBbrr9baQBUERIC5X 5SG3z1UmWFVB4ueB4jfSlgKE5e4UPMrSpJ0vWMiDCcC//czcrl3OEooSEVVgwyrPph 8WosFtmdV6rlCW84f7WUWM07YV89ob6Wi2QsFipo= Date: Thu, 23 Jul 2020 20:18:37 +0200 From: Greg Kroah-Hartman To: Mathieu Poirier Subject: Re: [PATCH v4 06/20] coresight: add try_get_module() in coresight_grab_device() Message-ID: <20200723181837.GA2944775@kroah.com> References: <20200723042802.22511-1-tingwei@codeaurora.org> <20200723042802.22511-7-tingwei@codeaurora.org> <20200723110707.GA1960107@kroah.com> <20200723180447.GA1323936@xps15> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20200723180447.GA1323936@xps15> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200723_141834_778928_9C7AF23A X-CRM114-Status: GOOD ( 34.64 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: tsoni@codeaurora.org, Sai Prakash Ranjan , Kim Phillips , Mao Jinlong , Suzuki K Poulose , Alexander Shishkin , coresight@lists.linaro.org, Randy Dunlap , Mian Yousaf Kaukab , Russell King , Tingwei Zhang , Leo Yan , linux-arm-kernel@lists.infradead.org, Mike Leach Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Thu, Jul 23, 2020 at 12:04:47PM -0600, Mathieu Poirier wrote: > Hi Greg, > > On Thu, Jul 23, 2020 at 01:07:07PM +0200, Greg Kroah-Hartman wrote: > > On Thu, Jul 23, 2020 at 12:27:48PM +0800, Tingwei Zhang wrote: > > > When coresight device is in an active session, driver module of > > > that device should not be removed. Use try_get_module() in > > > coresight_grab_device() to prevent module to be unloaded. > > > > Are you sure this works? Why is it needed at all? Why not just tear > > down the children properly when a module is removed so that you don't > > need this at all? > > Using the terms parent and child is somewhat ambiguous... This is not a > parent-child relationship but simply an association between devices, something > like port 1 on device "parent" is connected to port 2 on device "child". The > parent-child nomenclature was chosen to reflect that a device appears before > another in a coresight path. Otherwise there is no other relation between > devices, hence the choice of using try_get_module()/put_module() to prevent > drivers from being taken away. I'd be happy to proceed differently but haven't > found better options. > > Going back to parent/child, we could have chosen left/right, up/down or A/B, all > of which are just as confusion. Ok, thanks. But this causes confusion for everyone as seen below: > > > Signed-off-by: Tingwei Zhang > > > --- > > > drivers/hwtracing/coresight/coresight.c | 27 +++++++++++++++++++++---- > > > 1 file changed, 23 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c > > > index b7151c5f81b1..17bc76ea86ae 100644 > > > --- a/drivers/hwtracing/coresight/coresight.c > > > +++ b/drivers/hwtracing/coresight/coresight.c > > > @@ -640,7 +640,7 @@ struct coresight_device *coresight_get_sink_by_id(u32 id) > > > * don't appear on the trace path, they should be handled along with the > > > * the master device. > > > */ > > > -static void coresight_grab_device(struct coresight_device *csdev) > > > +static int coresight_grab_device(struct coresight_device *csdev) > > > { > > > int i; > > > > > > @@ -648,10 +648,25 @@ static void coresight_grab_device(struct coresight_device *csdev) > > > struct coresight_device *child; > > > > > > child = csdev->pdata->conns[i].child_dev; > > > - if (child && child->type == CORESIGHT_DEV_TYPE_HELPER) > > > + if (child && child->type == CORESIGHT_DEV_TYPE_HELPER) { > > > + if (!try_module_get(child->dev.parent->driver->owner)) > > > > Why the child's parent? Why not the child itself? > > The device structure of each coresight_device is not associated with a driver. > It is there to take advantages of device goodies such as dev.type, dev.group, > dev.release and dev.bus. Coresight IP blocks are discovered on the AMBA bus and as > such amba_device::dev::driver holds the driver itself. In coresight_register() > the association coresigth::dev::parent = amba_device::dev is made. > > > > > > > > + goto err; > > > > What about the error given to you here? Why throw that away? > > > > > pm_runtime_get_sync(child->dev.parent); > > > + } > > > } > > > + if (!try_module_get(csdev->dev.parent->driver->owner)) > > > + goto err; > > > > You don't reduce the child's parent's driver owner module reference > > here? > > Here @parent is referencing the current device. Now that helper devices > connected to any of its outgoing ports have been enabled (and a reference count > to the helper device driver incremented), a reference count to the current device > driver can also be incremented. I mean the fact that your error handling does not seem to roll back the module reference count you got up above in the other loop. Or if it does, it's really really not obvious, and should at the very least, be commented as to how it's all cleaning up properly. thanks, greg k-h _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel