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,NICE_REPLY_A,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 5D843C433E1 for ; Thu, 13 Aug 2020 23:43:45 +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 223B120771 for ; Thu, 13 Aug 2020 23:43:45 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="FVeg41v9" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 223B120771 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-Type: Content-Transfer-Encoding:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:Date:Message-ID:From: References:To:Subject:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=AsxwYQVSICmaVz1hqYcFtmUvhYAJBVd+Ts8GnI0d4as=; b=FVeg41v9jmc1tyniKwk4frOve T7T/oqYGKuiPMmF1nBD/y7WpgpErFLJUCfC03/q/mKskrvHt0ZZs/sbUj2oZH5n6yJZsK8+31RHqu AfaSWejrJnssjsuiIgCk6b0qA5Z7nbvq0m1OM5tJeft2SYyBodvk7zI6kN1fAbaMc7aOXDzZIuO/T wdV7uinfJpliAB5omJfp7cwGGlCeYofCbHK+u8Ntn1BtBR1P8uyTFnXrKZE79gNs3mDkHuwgKrLy5 UWWRAh42KKNij7+Fnl3Au9GgL9lgfCGg86QocZz39eE58srUZ9RCvv3k7D0NqlktuGQtYywPCTPlX oIAiBsIBQ==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1k6Mqf-0002l3-4c; Thu, 13 Aug 2020 23:41:29 +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 1k6Mqb-0002kS-DS for linux-arm-kernel@lists.infradead.org; Thu, 13 Aug 2020 23:41:26 +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 E60AF31B; Thu, 13 Aug 2020 16:41:17 -0700 (PDT) Received: from [10.37.8.11] (unknown [10.37.8.11]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 0155D3F22E; Thu, 13 Aug 2020 16:41:09 -0700 (PDT) Subject: Re: [PATCH v8 10/24] coresight: etm4x: allow etm4x to be built as a module To: mathieu.poirier@linaro.org, tingwei@codeaurora.org References: <20200807111153.7784-1-tingwei@codeaurora.org> <20200807111153.7784-11-tingwei@codeaurora.org> <20200813193912.GH3393195@xps15> From: Suzuki K Poulose Message-ID: Date: Fri, 14 Aug 2020 00:46:12 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: <20200813193912.GH3393195@xps15> Content-Language: en-US X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200813_194125_557084_6838F88F X-CRM114-Status: GOOD ( 27.20 ) 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, jinlmao@codeaurora.org, alexander.shishkin@linux.intel.com, gregkh@linuxfoundation.org, coresight@lists.linaro.org, rdunlap@infradead.org, ykaukab@suse.de, linux@armlinux.org.uk, leo.yan@linaro.org, linux-arm-kernel@lists.infradead.org, mike.leach@linaro.org Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org 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.html 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel