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 27BBDC433DF for ; Thu, 23 Jul 2020 18:06:32 +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 EB41C20714 for ; Thu, 23 Jul 2020 18:06:31 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="jW6xf8Ej"; dkim=fail reason="signature verification failed" (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b="lxASQDqP" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org EB41C20714 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linaro.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=KNN+ZGDd5JBVX580ysJuAXBTymU+vwpk/1ZbbR0yC3Y=; b=jW6xf8Ej2YnSZ22TeKQLqKPtK lfKhmVbanccJ68HxvDKTYaFySEyFg6hfh1+KDL9iEjGp8ks/8hWXJ+kUYsWa4wzvIGc0wO5S5zJHI p8iGss2JBody+JtK0zlvuDpz81qTybYBlV2meKe2MGPPIl3oJRKyBMdlyOcmNzHhJC55cgqOoB3vJ GOxHnVulvvOIwtqcdWOQQJi04OLxWPqIE+UH/OWlTRROjlQYPUXnAVyAc6Y++XPQLign+EVsmdf/A c1jrNMNlifPyJPWH0dt7tnGW/cHHqG25WyJ/7qWzPFyzSPk2aWARUFH2ltq3I+Qr8cB3Uwvhpw4oV ITROAEtvg==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1jyfaU-000383-QY; Thu, 23 Jul 2020 18:04:58 +0000 Received: from mail-pl1-x641.google.com ([2607:f8b0:4864:20::641]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1jyfaR-000371-Rh for linux-arm-kernel@lists.infradead.org; Thu, 23 Jul 2020 18:04:56 +0000 Received: by mail-pl1-x641.google.com with SMTP id q17so2894907pls.9 for ; Thu, 23 Jul 2020 11:04:53 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=i+Mu5u+5tihiKoKOr/bR/7GDWmkfnxoclB/vSUgSTVU=; b=lxASQDqPjeur7KTKFtIpyg8lKuM+0jQMfXZDxLueEpUs5xaF36ll+b3tcD/lUi3s4R KRgMDAqOJEuwDomjnQdexocI82nBgMwyjrMP/p10RWrOeQSG741ExD8N5Mr89st7cQ0O DHhf5Q/2j2A+sB34bNfaoph3nIufnz6Zd4KiEWm2akhDVd5i5nUiu9VEBqSXZcj2+I89 0u5r2h78V0RG2WAjAA9d7U93y0tKqoI1JjNJv9gY/9J9MqQ15OLZFX29PofTwREMSlrf 3uJS7SI3crUHNcRLg3HYLzpOFzWMOsZbtu17MxfKKk4BOVQlHHwRGWMX8KZ818jjcz+v eopA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=i+Mu5u+5tihiKoKOr/bR/7GDWmkfnxoclB/vSUgSTVU=; b=bsFH9jJRVmywBtP57QwsEYc5nGx7OrSQEhB/ClA4nvoaxADgDSn5XDMxsIR9K8yFB9 EJq1rhdU0cO07BcToVYmQ5xViAu6bra75gxMGiQhwN/FOND5XzzXkSKvM+AbMvTnAyyb /AzBnCovZLH2BQlLVIYHT8Ui0n0Ex1Sakqhw6EM02LPJIM34vlWu8bHzqu5pSWAUQQrU 0ZW5jB8c1UCgutS0ax5WPyEZk7aqKGJg09579rLbw5XWOw9embKGhLS1Ugnrt3XzVeVd KC/A0uo5i98gMFtod96hSRtd+p7ctxKvF9Ah3fBjO5BGT8v+GRMVLl/8dCFQ829OLYdc LYkw== X-Gm-Message-State: AOAM533e/tpEM2UPg1Wm1e3419xfZRZ16pzlANB9qhfQ6B6dTRGwGDfi t1eMBwhG6+HmFu7RojAFRaOnXg== X-Google-Smtp-Source: ABdhPJyuV9+VHo7PJyonCAR1iJaMSl4szKlPkbD64RPw+/XPrkC81jtcpFfZY2ZjPOqb2MpSODI2mw== X-Received: by 2002:a17:902:9a4b:: with SMTP id x11mr4810942plv.255.1595527491790; Thu, 23 Jul 2020 11:04:51 -0700 (PDT) Received: from xps15 (S0106002369de4dac.cg.shawcable.net. [68.147.8.254]) by smtp.gmail.com with ESMTPSA id w17sm3712266pge.10.2020.07.23.11.04.48 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 23 Jul 2020 11:04:49 -0700 (PDT) Date: Thu, 23 Jul 2020 12:04:47 -0600 From: Mathieu Poirier To: Greg Kroah-Hartman Subject: Re: [PATCH v4 06/20] coresight: add try_get_module() in coresight_grab_device() Message-ID: <20200723180447.GA1323936@xps15> References: <20200723042802.22511-1-tingwei@codeaurora.org> <20200723042802.22511-7-tingwei@codeaurora.org> <20200723110707.GA1960107@kroah.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20200723110707.GA1960107@kroah.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200723_140455_904306_F271ABB1 X-CRM114-Status: GOOD ( 30.85 ) 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 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. > > > > > 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 hope this clarify what is going on here. It is a little unorthodox but so is coresight. Thanks, Mathieu > > Ugh, that's a horid sentence :( > > > pm_runtime_get_sync(csdev->dev.parent); > > + return 0; > > +err: > > + for (i--; i >= 0; i--) { > > + struct coresight_device *child; > > + > > + child = csdev->pdata->conns[i].child_dev; > > + if (child && child->type == CORESIGHT_DEV_TYPE_HELPER) > > + pm_runtime_put(child->dev.parent); > > + } > > + return -ENODEV; > > } > > > > /* > > @@ -663,12 +678,15 @@ static void coresight_drop_device(struct coresight_device *csdev) > > int i; > > > > pm_runtime_put(csdev->dev.parent); > > + module_put(csdev->dev.parent->driver->owner); > > for (i = 0; i < csdev->pdata->nr_outport; i++) { > > 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) { > > pm_runtime_put(child->dev.parent); > > + module_put(child->dev.parent->driver->owner); > > + } > > } > > } > > > > @@ -721,7 +739,8 @@ static int _coresight_build_path(struct coresight_device *csdev, > > if (!node) > > return -ENOMEM; > > > > - coresight_grab_device(csdev); > > + if (coresight_grab_device(csdev)) > > + return -ENODEV; > > Why not return the error given to you? > > > thanks, > > greg k-h _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel