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,URIBL_BLOCKED, 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 0CE00C433E1 for ; Fri, 14 Aug 2020 11:02:24 +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 B90BB206DA for ; Fri, 14 Aug 2020 11:02:23 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="C9VFHR13"; dkim=fail reason="signature verification failed" (1024-bit key) header.d=mg.codeaurora.org header.i=@mg.codeaurora.org header.b="gY1g7J0H" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org B90BB206DA 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=tfUzjsGLi8CUffPb6cZ8GtBkKJ5fJu5KCwRy6MkG1c4=; b=C9VFHR13gcgJUXc4iqd7G9+X2 Y1OGPSoaCBcnshvNiExwpRfX0K+oQF4y9YtzxoBubGdd4fgxGcpUtt65gAVf3JLNaxjZcV1Tv6f/T qTmgoGc7yHRrl59Q263M+IsoqEZT/oZVSP6tOZ917bkNsHXtRlSjIRum9qR/Gx/wZgTsGgNguox2j A0F8/12nXwg0LhDKpdXMyIdeHrJTJ8k2OjAl6/Q+7fX48rmvgYC7sk+WsZTHqq9OikrEJfpNSTXsM ITXpo25qqSaABRV1YOTydBW5i1sfoogE2peGBkXoPu83gDbUUSaq/2FN9Ey7aeBTcYles/9H7OUfM S6YmyKTLQ==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1k6XSA-0000Zg-KP; Fri, 14 Aug 2020 11:00:54 +0000 Received: from mail29.static.mailgun.info ([104.130.122.29]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1k6XS5-0000ZK-CS for linux-arm-kernel@lists.infradead.org; Fri, 14 Aug 2020 11:00:52 +0000 DKIM-Signature: a=rsa-sha256; v=1; c=relaxed/relaxed; d=mg.codeaurora.org; q=dns/txt; s=smtp; t=1597402851; h=In-Reply-To: Content-Type: MIME-Version: References: Message-ID: Subject: Cc: To: From: Date: Sender; bh=lV38CUZn5wxfw3cZtVyXOf0ws2IK+PQS5NhS4jyDsM8=; b=gY1g7J0H3du4gt7XLR+d4DfSygkaqxOBRFt2Di5yJ7FjrRISifdXrfb2cMhOiaXnwmHuwCL4 nYjAHlm0ybKRlVRcLUGDSZww3jfDOzv9TW5D43yiu99JDr590JbVevz0PH72YmmiFv/bBg5s RoL7HQFIC79ZUwjnnPTuKAjNsto= X-Mailgun-Sending-Ip: 104.130.122.29 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 5f366ecc03528d4024fd91b5 (version=TLS1.2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256); Fri, 14 Aug 2020 11:00:28 GMT Received: by smtp.codeaurora.org (Postfix, from userid 1001) id 1807AC43395; Fri, 14 Aug 2020 11:00:27 +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 00F84C433C9; Fri, 14 Aug 2020 11:00:22 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org 00F84C433C9 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: Fri, 14 Aug 2020 19:00:18 +0800 From: Tingwei Zhang To: Suzuki K Poulose Subject: Re: [PATCH v8 10/24] coresight: etm4x: allow etm4x to be built as a module Message-ID: <20200814110017.GB5803@codeaurora.org> References: <20200807111153.7784-1-tingwei@codeaurora.org> <20200807111153.7784-11-tingwei@codeaurora.org> <20200813193912.GH3393195@xps15> <20200814095224.GA27567@codeaurora.org> <20200814104124.GA24873@ewhatever.cambridge.arm.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20200814104124.GA24873@ewhatever.cambridge.arm.com> 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-20200814_070051_568682_11FCE790 X-CRM114-Status: GOOD ( 49.39 ) 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, saiprakash.ranjan@codeaurora.org, kim.phillips@arm.com, mathieu.poirier@linaro.org, alexander.shishkin@linux.intel.com, gregkh@linuxfoundation.org, coresight@lists.linaro.org, jinlmao@codeaurora.org, ykaukab@suse.de, linux@armlinux.org.uk, rdunlap@infradead.org, tingwei@codeaurora.org, leo.yan@linaro.org, linux-arm-kernel@lists.infradead.org, mike.leach@linaro.org 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 Fri, Aug 14, 2020 at 06:42:01PM +0800, Suzuki K Poulose wrote: > On Fri, Aug 14, 2020 at 05:52:24PM +0800, Tingwei Zhang wrote: > > On Fri, Aug 14, 2020 at 07:46:12AM +0800, Suzuki K Poulose wrote: > > > On 08/13/2020 08:39 PM, Mathieu Poirier wrote: > > > >On Fri, Aug 07, 2020 at 07:11:39PM +0800, Tingwei Zhang wrote: > > > >>From: Kim Phillips > > > >> > > > >>Allow to build coresight-etm4x as a module, for ease of development. > > > >> > > > >>- Kconfig becomes a tristate, to allow =m > > > >>- append -core to source file name to allow module to > > > >> be called coresight-etm4x by the Makefile > > > >>- add an etm4_remove function, for module unload > > > >>- add a MODULE_DEVICE_TABLE for autoloading on boot > > > >>- protect etmdrvdata[] with mutex lock from racing > > > >> between module unload and CPU hotplug > > > >> > > > >>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 > > > >>Signed-off-by: Tingwei Zhang > > > >>Tested-by: Mike Leach > > > >>--- > > > >> drivers/hwtracing/coresight/Kconfig | 5 +- > > > >> drivers/hwtracing/coresight/Makefile | 4 +- > > > >> ...resight-etm4x.c => coresight-etm4x-core.c} | 118 > > > +++++++++++++----- > > > >> 3 files changed, 92 insertions(+), 35 deletions(-) > > > >> rename drivers/hwtracing/coresight/{coresight-etm4x.c => > > > coresight-etm4x-core.c} (95%) > > > >> > > > >>diff --git a/drivers/hwtracing/coresight/Kconfig > > > b/drivers/hwtracing/coresight/Kconfig > > > >>index 8fd9fd139cf3..d6e107bbd30b 100644 > > > >>--- a/drivers/hwtracing/coresight/Kconfig > > > >>+++ b/drivers/hwtracing/coresight/Kconfig > > > >>@@ -78,7 +78,7 @@ config CORESIGHT_SOURCE_ETM3X > > > >> module will be called coresight-etm3x. > > > >> config CORESIGHT_SOURCE_ETM4X > > > >>- bool "CoreSight Embedded Trace Macrocell 4.x driver" > > > >>+ tristate "CoreSight Embedded Trace Macrocell 4.x driver" > > > >> depends on ARM64 > > > >> select CORESIGHT_LINKS_AND_SINKS > > > >> select PID_IN_CONTEXTIDR > > > >>@@ -88,6 +88,9 @@ config CORESIGHT_SOURCE_ETM4X > > > >> for instruction level tracing. Depending on the > implemented > > > version > > > >> data tracing may also be available. > > > >>+ To compile this driver as a module, choose M here: the > > > >>+ module will be called coresight-etm4x. > > > >>+ > > > >> config CORESIGHT_STM > > > >> tristate "CoreSight System Trace Macrocell driver" > > > >> depends on (ARM && !(CPU_32v3 || CPU_32v4 || CPU_32v4T)) > || ARM64 > > > >>diff --git a/drivers/hwtracing/coresight/Makefile > > > b/drivers/hwtracing/coresight/Makefile > > > >>index d619cfd0abd8..271dc255454f 100644 > > > >>--- a/drivers/hwtracing/coresight/Makefile > > > >>+++ b/drivers/hwtracing/coresight/Makefile > > > >>@@ -14,8 +14,8 @@ obj-$(CONFIG_CORESIGHT_LINKS_AND_SINKS) += > > > coresight-funnel.o \ > > > >> obj-$(CONFIG_CORESIGHT_SOURCE_ETM3X) += coresight-etm3x.o > > > >> coresight-etm3x-y := coresight-etm3x-core.o coresight-etm-cp14.o \ > > > >> coresight-etm3x-sysfs.o > > > >>-obj-$(CONFIG_CORESIGHT_SOURCE_ETM4X) += coresight-etm4x.o \ > > > >>- coresight-etm4x-sysfs.o > > > >>+obj-$(CONFIG_CORESIGHT_SOURCE_ETM4X) += coresight-etm4x.o > > > >>+coresight-etm4x-y := coresight-etm4x-core.o coresight-etm4x-sysfs.o > > > >> obj-$(CONFIG_CORESIGHT_STM) += coresight-stm.o > > > >> obj-$(CONFIG_CORESIGHT_CPU_DEBUG) += coresight-cpu-debug.o > > > >> obj-$(CONFIG_CORESIGHT_CATU) += coresight-catu.o > > > >>diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c > > > b/drivers/hwtracing/coresight/coresight-etm4x-core.c > > > >>similarity index 95% > > > >>rename from drivers/hwtracing/coresight/coresight-etm4x.c > > > >>rename to drivers/hwtracing/coresight/coresight-etm4x-core.c > > > >>index fddfd93b9a7b..ae9847e194a3 100644 > > > >>--- a/drivers/hwtracing/coresight/coresight-etm4x.c > > > >>+++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c > > > >>@@ -48,6 +48,7 @@ module_param(pm_save_enable, int, 0444); > > > >> MODULE_PARM_DESC(pm_save_enable, > > > >> "Save/restore state on power down: 1 = never, 2 = > self-hosted"); > > > >>+static DEFINE_PER_CPU(struct mutex, etmdrvdata_lock); > > > >> static struct etmv4_drvdata *etmdrvdata[NR_CPUS]; > > > >> static void etm4_set_default_config(struct etmv4_config *config); > > > >> static int etm4_set_event_filters(struct etmv4_drvdata *drvdata, > > > >>@@ -1089,18 +1090,25 @@ void etm4_config_trace_mode(struct > etmv4_config > > > *config) > > > >> static int etm4_online_cpu(unsigned int cpu) > > > >> { > > > >>- if (!etmdrvdata[cpu]) > > > >>+ mutex_lock(&per_cpu(etmdrvdata_lock, cpu)); > > > >>+ if (!etmdrvdata[cpu]) { > > > >>+ mutex_unlock(&per_cpu(etmdrvdata_lock, cpu)); > > > >> return 0; > > > >>+ } > > > >> if (etmdrvdata[cpu]->boot_enable && > > > !etmdrvdata[cpu]->sticky_enable) > > > >> coresight_enable(etmdrvdata[cpu]->csdev); > > > >>+ mutex_unlock(&per_cpu(etmdrvdata_lock, cpu)); > > > >> return 0; > > > >> } > > > >> static int etm4_starting_cpu(unsigned int cpu) > > > >> { > > > >>- if (!etmdrvdata[cpu]) > > > >>+ mutex_lock(&per_cpu(etmdrvdata_lock, cpu)); > > > >>+ if (!etmdrvdata[cpu]) { > > > >>+ mutex_unlock(&per_cpu(etmdrvdata_lock, cpu)); > > > >> return 0; > > > >>+ } > > > >> spin_lock(&etmdrvdata[cpu]->spinlock); > > > >> if (!etmdrvdata[cpu]->os_unlock) > > > >>@@ -1109,18 +1117,23 @@ static int etm4_starting_cpu(unsigned int > cpu) > > > >> if (local_read(&etmdrvdata[cpu]->mode)) > > > >> etm4_enable_hw(etmdrvdata[cpu]); > > > >> spin_unlock(&etmdrvdata[cpu]->spinlock); > > > >>+ mutex_unlock(&per_cpu(etmdrvdata_lock, cpu)); > > > > > > > >Ouch! > > > > > > > >A mutex wrapping a spinlock to protect the exact same drvdata > structure. > > > > > > Actually this mutex is for "protecting" etmdrvdata[cpu], not the > > > drvdata struct as such. But as you said, it becomes like a double > lock. > > > > > > Having mutex is an overkill for sure and can't be used replace > > > spin_lock, especially if needed from atomic context. Having looked > > > at the code, we only ever access/modify the etmdrvdata[cpu] on a > > > different CPU is when we probe and there are chances of races. > > > One of such is described here > > > > > > > http://lists.infradead.org/pipermail/linux-arm-kernel/2020-July/589941.htm > > > l > > > > > > I will see if I can spin a couple of patches to address these. > > > Once we make sure that the etmdrvdata[cpu] is only modified by "cpu" > > > then we don't need this mutex and stick with what we have. > > > > > > Suzuki > > > > > > > With Suzuki's idea, I made some change to remove mutex lock and change > > etmdrvdata[i] on CPU i. I didn't change the part in probe to assign > > drvdata to etmdrvdata. That's after all drvdata is prepared and > > coresight device is registered. Once hotplug/PM call back see the > > value, it can use it directly. If we have racing in probe and these > > call backs, the worse case is call backs finds out etmdrvdata is NULL > > and return immediately. Does this make sense? > > > > > > static void __exit clear_etmdrvdata(void *info) > > { > > int cpu = *(int *)info; > > etmdrvdata[cpu] = NULL; > > } > > > > static int __exit etm4_remove(struct amba_device *adev) > > { > > struct etmv4_drvdata *drvdata = dev_get_drvdata(&adev->dev); > > > > etm_perf_symlink(drvdata->csdev, false); > > > > /* > > * Taking hotplug lock here to avoid racing between etm4_remove > and > > * CPU hotplug call backs. > > */ > > cpus_read_lock(); > > if (cpu_online(drvdata->cpu)) > > /* > > * The readers for etmdrvdata[] are CPU hotplug call > backs > > * and PM notification call backs. Change etmdrvdata[i] > on > > * CPU i ensures these call backs has consistent view > > * inside one call back function. > > */ > > smp_call_function_single(drvdata->cpu, clear_etmdrvdata, > &drvdata->cpu, 1); > > You should check the error here to confirm if this was really complete. > Otherwise, > fallback to the manual clearing here. > Yes. I don't need to check cpu_online since it's checked in smp_call_function_single(). I can just check return value and fallback to manual clearing. > > else > > etmdrvdata[drvdata->cpu] = NULL; > > > > cpus_read_unlock(); > > > > coresight_unregister(drvdata->csdev); > > > > return 0; > > } > > Yes, this is exactly what I was looking for. Additionally we should > fix the races mentioned in the link above. I believe, we can solve > that by the following : > I already did the same thing in this patch if you ignore the mutex part. :) Thanks, Tingwei > ---8>--- > > coresight: etm4x: Delay advertising per-cpu drvdata > > Delay advertising the per-cpu etmdrvdata until we have > successfully initialised. This is to avoid races and > unnecessary save/restore of the ETM state, before the > device is properly setup. > > Signed-off-by: Suzuki K Poulose > --- > drivers/hwtracing/coresight/coresight-etm4x.c | 9 +++------ > 1 file changed, 3 insertions(+), 6 deletions(-) > > diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c > b/drivers/hwtracing/coresight/coresight-etm4x.c > index 6d7d2169bfb2..30f7ffa07eb6 100644 > --- a/drivers/hwtracing/coresight/coresight-etm4x.c > +++ b/drivers/hwtracing/coresight/coresight-etm4x.c > @@ -1499,8 +1499,6 @@ static int etm4_probe(struct amba_device *adev, > const struct amba_id *id) > return -ENOMEM; > > cpus_read_lock(); > - etmdrvdata[drvdata->cpu] = drvdata; > - > if (smp_call_function_single(drvdata->cpu, > etm4_init_arch_data, drvdata, 1)) > dev_err(dev, "ETM arch init failed\n"); > @@ -1509,10 +1507,8 @@ static int etm4_probe(struct amba_device *adev, > const struct amba_id *id) > cpus_read_unlock(); > > /* etm4_pm_setup_cpuslocked() does its own cleanup - exit on error > */ > - if (ret) { > - etmdrvdata[drvdata->cpu] = NULL; > + if (ret) > return ret; > - } > > if (etm4_arch_supported(drvdata->arch) == false) { > ret = -EINVAL; > @@ -1547,6 +1543,8 @@ static int etm4_probe(struct amba_device *adev, > const struct amba_id *id) > goto err_arch_supported; > } > > + /* Advertise this after we have successfully initialised */ > + etmdrvdata[drvdata->cpu] = drvdata; > pm_runtime_put(&adev->dev); > dev_info(&drvdata->csdev->dev, "CPU%d: ETM v%d.%d initialized\n", > drvdata->cpu, drvdata->arch >> 4, drvdata->arch & 0xf); > @@ -1559,7 +1557,6 @@ static int etm4_probe(struct amba_device *adev, > const struct amba_id *id) > return 0; > > err_arch_supported: > - etmdrvdata[drvdata->cpu] = NULL; > etm4_pm_clear(); > return ret; > } > -- > 2.24.1 > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel