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 94AC5C433E1 for ; Fri, 14 Aug 2020 09:54:50 +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 54DCA204EA for ; Fri, 14 Aug 2020 09:54:50 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="UryqAEmE"; dkim=fail reason="signature verification failed" (1024-bit key) header.d=mg.codeaurora.org header.i=@mg.codeaurora.org header.b="Tm/u6/55" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 54DCA204EA 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=fhrOLuc8bz1Ikt9+/z7Ama+xo3IU0AdExV/7X+eHtk4=; b=UryqAEmEMthiRYH4dnNOdg7br rAAel4EM6dzB6olhYnkFJDfESiuCX84GpNslIpnS+xc6P89ludQfW0U/GJGVPglJN39B7wzyVaUgZ c75O5sgul1GFGpRNhfxre36//v2sbmJ/ucI7HIH7m6tWUgOnwC1IVgaHHt4y0Ot0+m0jJKw60lCvz BAgdQjDejfsIhQ4qbR1IePlEGOW/D/OYthDEhOanwsUqFZORcqK9oJMM5n0TFkjvpMGFCsfXmZotx pVjQAOzM80BMEfPw6t8bn9IeCXWGJLbeUVC/zuBdw1aHDahJz+VDwKhmHKGH3L3XxjUDOhUDTQiZF 7QnTZyrMg==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1k6WOB-000858-3t; Fri, 14 Aug 2020 09:52:43 +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 1k6WO7-00084Q-FS for linux-arm-kernel@lists.infradead.org; Fri, 14 Aug 2020 09:52:40 +0000 DKIM-Signature: a=rsa-sha256; v=1; c=relaxed/relaxed; d=mg.codeaurora.org; q=dns/txt; s=smtp; t=1597398758; h=In-Reply-To: Content-Type: MIME-Version: References: Message-ID: Subject: Cc: To: From: Date: Sender; bh=zTpePWYBhzq4HoDEf5g2tMmgvUBVMtvD0jY0BxZzfls=; b=Tm/u6/55vmsv3/4jTKU28tzPElbDDeY6EoMXsRWQAQDD02JpR5N5JgodJ0Y26tziAavkCS+l 2fz5+eLt6h3A18NpIh2bwchr8nCKPa4M2WBS8aMbP6Za+Ng4Uff43b8Dz49gPbi6s6mXDXQr 7Ak9+EWbw4ZDJBJfAcUDTCDb1gY= 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-east-1.postgun.com with SMTP id 5f365ee5f2b697637a1a2f59 (version=TLS1.2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256); Fri, 14 Aug 2020 09:52:37 GMT Received: by smtp.codeaurora.org (Postfix, from userid 1001) id AE89DC433CB; Fri, 14 Aug 2020 09:52:36 +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 22207C433C6; Fri, 14 Aug 2020 09:52:30 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org 22207C433C6 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 17:52:24 +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: <20200814095224.GA27567@codeaurora.org> References: <20200807111153.7784-1-tingwei@codeaurora.org> <20200807111153.7784-11-tingwei@codeaurora.org> <20200813193912.GH3393195@xps15> 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-20200814_055239_562579_7ED7B4D0 X-CRM114-Status: GOOD ( 33.78 ) 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 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); else etmdrvdata[drvdata->cpu] = NULL; cpus_read_unlock(); coresight_unregister(drvdata->csdev); return 0; } Thanks, Tingwei > > _______________________________________________ > 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