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=-11.5 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,USER_AGENT_SANE_1 autolearn=ham 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 50AA5C433E0 for ; Sun, 26 Jul 2020 01:34: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 06CCD206D8 for ; Sun, 26 Jul 2020 01:34:09 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="YB5zegH5"; dkim=fail reason="signature verification failed" (1024-bit key) header.d=mg.codeaurora.org header.i=@mg.codeaurora.org header.b="Y1YKrbdj" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 06CCD206D8 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=codeaurora.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=Mbl8lEurRb6a0YST+5wYq3CYrYfrjVA0DfbOcs0xM1Q=; b=YB5zegH5s/pCCK3GaANTVTu7u 8ue8iK60VxZ7mFSAsHyHaMGGv1fuoaRyI3EX1YYMQ859+zhFKR9VSQXJ0f9Fc01j4z7yc+0t0VRBl rLxHD4mRceIWY8YR9AhJQHI6P+JNPpIvsE9AapqWBbHSQt7Nrkhec3+0OEaHR2GhRcCSBwynXEAKd ttZrbN4tVhjnKLDaiqcnHItTL197pjiMdgrDQx6YZZU1hKlG4vyPAKALZ/ggyQPurf4A3Plo3j83r UK6hj481Jt3+RaD8bcPhlCd6eLI86UNBIb81ycliqSyZcfxCDSUFW9DVlPsZaVj5ijsbP93UUnxUt ObnpYTOHQ==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1jzVWp-00067p-5u; Sun, 26 Jul 2020 01:32:39 +0000 Received: from m43-7.mailgun.net ([69.72.43.7]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1jzVWj-00066S-DI for linux-arm-kernel@lists.infradead.org; Sun, 26 Jul 2020 01:32:37 +0000 DKIM-Signature: a=rsa-sha256; v=1; c=relaxed/relaxed; d=mg.codeaurora.org; q=dns/txt; s=smtp; t=1595727156; h=In-Reply-To: Content-Type: MIME-Version: References: Message-ID: Subject: Cc: To: From: Date: Sender; bh=9NWW2zqdfCR6YYuiU9tTYo7+j+tyi7zWYhCXUZy/C8Q=; b=Y1YKrbdj9BKISGMGTWOVOmGFhk+1q219Pr4TYW1W5xDOEkmisVfQlm/Fj9H++iE6xC0rnEij l6N1FtdfJRiPmqeXao4hymqMDF/J6elrUqh1GDYIU/m05JljmTxjck/FpStI2gfpzeNkcLv1 bAPssYa3YPYwoAvXTWt42qZKed4= X-Mailgun-Sending-Ip: 69.72.43.7 X-Mailgun-Sid: WyJiYzAxZiIsICJsaW51eC1hcm0ta2VybmVsQGxpc3RzLmluZnJhZGVhZC5vcmciLCAiYmU5ZTRhIl0= Received: from smtp.codeaurora.org (ec2-35-166-182-171.us-west-2.compute.amazonaws.com [35.166.182.171]) by smtp-out-n04.prod.us-west-2.postgun.com with SMTP id 5f1cdd2049176bd382cfd1b4 (version=TLS1.2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256); Sun, 26 Jul 2020 01:32:16 GMT Received: by smtp.codeaurora.org (Postfix, from userid 1001) id 2EE0BC43391; Sun, 26 Jul 2020 01:32:16 +0000 (UTC) Received: from codeaurora.org (unknown [180.166.53.21]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) (Authenticated sender: tingwei) by smtp.codeaurora.org (Postfix) with ESMTPSA id ADCE4C433C6; Sun, 26 Jul 2020 01:32:11 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org ADCE4C433C6 Authentication-Results: aws-us-west-2-caf-mail-1.web.codeaurora.org; dmarc=none (p=none dis=none) header.from=codeaurora.org Authentication-Results: aws-us-west-2-caf-mail-1.web.codeaurora.org; spf=none smtp.mailfrom=tingweiz@codeaurora.org Date: Sun, 26 Jul 2020 09:32:05 +0800 From: Tingwei Zhang To: Mike Leach Subject: Re: [PATCH v4 06/20] coresight: add try_get_module() in coresight_grab_device() Message-ID: <20200726013205.GA23391@codeaurora.org> References: <20200723042802.22511-1-tingwei@codeaurora.org> <20200723042802.22511-7-tingwei@codeaurora.org> <20200725100516.GA18120@codeaurora.org> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.24 (2015-08-30) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200725_213236_937873_82C98FCB X-CRM114-Status: GOOD ( 43.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 , Mathieu Poirier , Suzuki K Poulose , Alexander Shishkin , Greg Kroah-Hartman , Coresight ML , Randy Dunlap , Mian Yousaf Kaukab , Russell King , Mao Jinlong , Tingwei Zhang , Leo Yan , linux-arm-kernel 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 Mike, On Sat, Jul 25, 2020 at 09:51:31PM +0800, Mike Leach wrote: > Hi Tingwei, > > On Sat, 25 Jul 2020 at 11:05, Tingwei Zhang > wrote: > > > > Hi Mike, > > > > Thu, Jul 23, 2020 at 06:55:47PM +0800, Mike Leach wrote: > > > Hi Tingwei, > > > > > > On Thu, 23 Jul 2020 at 05:28, 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. > > > > > > > > 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)) > > > > + goto err; > > > > pm_runtime_get_sync(child->dev.parent); > > > > + } > > > > } > > > > + if (!try_module_get(csdev->dev.parent->driver->owner)) > > > > + goto err; > > > > 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; > > > > node->csdev = csdev; > > > > list_add(&node->link, path); > > > > > > > > -- > > > > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora > > > Forum, > > > > a Linux Foundation Collaborative Project > > > > > > > > > > The CTI devices are associated with other coresight components using > > > csdev->ect_dev; These are not handled in the main linear trace path as > > > these have a star topology interlinking all components. > > > However, when a component has an associated ect_dev then is enabled at > > > the same time as the other component is enabled. > > > > > > So a module_get/put will be needed for this association to prevent the > > > CTI being removed while a trace session is active. > > > I suggest in coresight.c : coresight_control_assoc_ectdev() > > > > > > > In module unload process, devices of that module will be removed. In > > this case, cti_remove() is called on all cti device when unloading > > cti module. All cti connections is cleaned at that time. > > csdev->ect_dev is set to NULL. coresight_control_assoc_ectdev() > > will return since since ect_csdev is NULL. > > > > I think we are safe without holding module reference since cti > > driver has done a pretty good job on clean up. > > > > The issue here is not about clean up. We need to keep all the > programmed coresight modules loaded and operational while a coresight > trace session is in progress. The CTI can be used to control the > generation of trace, trigger trace related events etc. > If the module is removed while a session is in progress then this > programming is lost and the trace data collected may not be what is > expected. > Got your point now. In my opinion, CTI is kinda different to other coresight components since it's optional. For other components, they have to been there when path is built or the enablement fails. Use can either get a successfully return which means it's good and all devices/modules are there until path is released, or a failed return which means trace session can't be setup. In CTI case, there's no garrantee that CTI related to the trace session is there even the enablement is successful. For example, One cti is required for ETM tracing session. If cti module is unloaded before enable that trace session. The enablement will still be successfully done since there's no ect_dev connected at all. My point is holding cti module reference in enable path won't fix all the problem. Altenatively, user should be aware that unload cti module leads to all cti functioniol failure. With current design, the behavior is consistant on cti. User can unload cti module whenever he wants but it will break the function. Thanks, Tingwei > Regards > > Mike > > > Let me know if you still think we need hold the module reference. > > > > Thanks, Tingwei > > > Regards > > > > > > Mike > > > > > > > > > -- > > > Mike Leach > > > Principal Engineer, ARM Ltd. > > > Manchester Design Centre. UK > > > > > > _______________________________________________ > > > linux-arm-kernel mailing list > > > linux-arm-kernel@lists.infradead.org > > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > > > > -- > Mike Leach > Principal Engineer, ARM Ltd. > Manchester Design Centre. UK > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel