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=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 17D8EC433E1 for ; Fri, 14 Aug 2020 10:44:00 +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 C7C06206A4 for ; Fri, 14 Aug 2020 10:43:59 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="o47v4LpP" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org C7C06206A4 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=arm.com 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=/tXEi4jaMKWnRgVSZhYmHfDCsEHIcamnMeau/03BFRU=; b=o47v4LpPSXOnKDXnVKKKvbx8Y XpWsjlSyNcu67fgfp3ggyeqZg9IKCJc/HEQySqrSOurZdf7MymeyuTmGnlEPnscURc1JkQ6A60a3X z7xcVNg0GfdmaPEjfNh+MSKMBLBxvUrPQqMKsX1xBduInBjcVcYhRCjHDX84FmEZiLaynRjM9qEe4 5rNDU0JdcYRsej0vzhasu58hYkLfUaNYKe4pQp2MtGSMqDJBz48klV8hgYI857V6B1aq2HHJfEjJ3 NIt4PU9VFN+RStaXCweJQfhS6272uiSSSrw3ALQUf5pWe7wiGi/oFx51FkuNUZpmZjdDfrbXgOP6/ DGhMLnJoA==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1k6XA9-0005LT-KW; Fri, 14 Aug 2020 10:42:17 +0000 Received: from foss.arm.com ([217.140.110.172]) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1k6XA4-0005Kf-OW for linux-arm-kernel@lists.infradead.org; Fri, 14 Aug 2020 10:42:14 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id E9B401063; Fri, 14 Aug 2020 03:42:09 -0700 (PDT) Received: from ewhatever.cambridge.arm.com (ewhatever.cambridge.arm.com [10.1.197.1]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id CC8B23F6CF; Fri, 14 Aug 2020 03:42:07 -0700 (PDT) Date: Fri, 14 Aug 2020 11:42:01 +0100 From: Suzuki K Poulose To: Tingwei Zhang Subject: Re: [PATCH v8 10/24] coresight: etm4x: allow etm4x to be built as a module Message-ID: <20200814104124.GA24873@ewhatever.cambridge.arm.com> References: <20200807111153.7784-1-tingwei@codeaurora.org> <20200807111153.7784-11-tingwei@codeaurora.org> <20200813193912.GH3393195@xps15> <20200814095224.GA27567@codeaurora.org> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20200814095224.GA27567@codeaurora.org> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200814_064212_891563_855A948F X-CRM114-Status: GOOD ( 42.74 ) 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, suzuki.poulose@arm.com, 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 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. > 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 : ---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