From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-ot1-f49.google.com (mail-ot1-f49.google.com [209.85.210.49]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 734EA3CCFDB for ; Tue, 31 Mar 2026 09:26:11 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.210.49 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1774949173; cv=none; b=skCam/RXYe+wGahes1A89NDkwYQFlYZo9Ad6drZRkqSZfNC3xW8wL7rPlsp7rxcpFvfi5hyUo8RH9Jq8FrFVQdve9E1cAiq8kKkDjVlVBgAv/CZ357MrCIEyVS5rFjPMwOElds6P5LXa1OaDaPBZBDYk3UCfTfKqhqgUgwTcyeM= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1774949173; c=relaxed/simple; bh=5IpHyJJO9LqhQHe8M4HvoMN7rKAful/BwGsxmEVSuaQ=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=tuN+rdnP5RzqdtiiY1mWF0Ostxq//6UaW8AabVVGhXR3hsUrtU5Q9yjpM9ZSEv8GczPx9gJi+afopQPGOIFtTFhyncwR0fIAZqiMn8AndRKr5YhiNWPxaike6F4lo0AZJYy6whJEiAxWkkeLSohIykBiTO4dKxjFtdaZOJWVi9U= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linaro.org; spf=pass smtp.mailfrom=linaro.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b=klzy6Wjr; arc=none smtp.client-ip=209.85.210.49 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linaro.org Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b="klzy6Wjr" Received: by mail-ot1-f49.google.com with SMTP id 46e09a7af769-7d9b21d1461so4738322a34.1 for ; Tue, 31 Mar 2026 02:26:11 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1774949170; x=1775553970; darn=vger.kernel.org; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=sRqoNu4/T0A4idXwuAeIugLUOXNURLtj6A0/QlJBzQU=; b=klzy6WjrlNs7dHbd0YX2P4dze4pOifIbkSg/ga1O8sTMmfCnWxoRO4xTumKJZbfOVY eka0coeJYzJotVAi7tMKFvYGjyo7xquWZ8IJXkpEeEDsyjV0dj4fu/FeAszx3v51w6gw Dj2qW6NqfAGts6QXZwOf6IszH01gGnNX3ftkxwVoHRLQXdfJjJMsmFyshPWTy0fCus7x sawHHe6CMFfz/yfXU7nlccs08YMENpkmVY9SnQjErCjyvcdcidKqv2wG/g0lJM0Rj+kB XpY3Bq0SeuqIut3MrKloUtIANtkxTA7yFUXRqJqGa4B5oz9WywXkZh+IqXh2OJy6lliZ c3mA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1774949170; x=1775553970; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-gg:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=sRqoNu4/T0A4idXwuAeIugLUOXNURLtj6A0/QlJBzQU=; b=Eso9d+JEpCrYPOuDoiX0KYv1x71RP5qsYnU5cvbq/a5iDEDvUq71KDtOhIqlPze581 jNL/8eCHQsu4qYiwVgzKUar72h22YCimJemwghUVndwZdgqVsbgq7ixyi44Ifs89kqEM ++UfbXirz9G7L+YNbmfQmonGaghwxH/8JTW2bOJJcbiVfCZHgrNYPR5zM6fexev+rJbq NBdbZmhwTSZqq+5iJO8wJt87hhsiHge8JLGtHlJ4ymi2UUmQWgkkF2vU9HlmbH7Ek3jb OxzofV73fIj2rtGZYlviSYVzU7C2HyCeNKZXu1QK9Nksdbkhr7Lir4tgKJvU9Fey5orq zzjg== X-Forwarded-Encrypted: i=1; AJvYcCUcpaCielasfyctQwfOtGsVcEkbFOeDo/hYWaLXS1IchPhQ87JgLhJqsIYn8cF8r3gYeMPLzBqraYoeIpo=@vger.kernel.org X-Gm-Message-State: AOJu0YyMjdCayNJsWh4tzE2N8xI2V3LJ3OnJfxYqeo5Oq1xpHTvDxCFq dx/fWnnsZIj00r1xdqS4liWh49Y3CyoS1lnTCXhS/+GH/1KVNio+iqyNR7kHYNgOwjA5c95zF6K 8aKo8Bko= X-Gm-Gg: ATEYQzwobg3fxn9402TX+IUkAzXCaXA6FLldA/gvmN5QV/08AYM1TcgXCbA1inu5WJ7 cdg/tHU/Bq81kh9Oul04Cqge7Q2ODyMGPNGtEa/Spl8EHx/qzEyJ9O3pQxk84DeWeooSLAtBwlK PbcIvz/JIGHrdwq1LQt97xe46Lg+IC0ICBCx8YHdhEqTusDtpzj0PJjnFxVyF/IoD2U9hF33Py1 AkkxDeaadqLg3rSjDBk9A3w5OnhqIRDgzK9ocpuAsZRx3IROfl/z5GGNUg5Q2A7iswHiIps04vx O4Kqc0ATT2vmExQJ+JcA+bZk7Pzj0nOsxIknzCsQTukBTpi8N8o29ehbXQ+v6QroDfnCsqWDNtm n4bGdc4Z8qxNv1BQhwifNvHd9XvnrtvVI3/zMqpGz9JLdoYljZI2Yo6qQebv/xils8/a30dudYQ N/7/Ze9V8Rhdk0hTjc3PUnSOCGqqOf X-Received: by 2002:a05:6830:81f8:b0:7d9:ae73:2ff9 with SMTP id 46e09a7af769-7da37622aa3mr1639926a34.11.1774949170407; Tue, 31 Mar 2026 02:26:10 -0700 (PDT) Received: from [192.168.1.3] ([185.48.77.170]) by smtp.gmail.com with ESMTPSA id 46e09a7af769-7da0a7b45basm7968990a34.17.2026.03.31.02.26.08 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 31 Mar 2026 02:26:10 -0700 (PDT) Message-ID: <7abe93fa-3748-4077-b75a-d28296862654@linaro.org> Date: Tue, 31 Mar 2026 10:26:07 +0100 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v3] coresight: tpdm: add traceid_show for checking traceid To: Jie Gan , Suzuki K Poulose Cc: coresight@lists.linaro.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, Mike Leach , Leo Yan , Alexander Shishkin , Tingwei Zhang References: <20260325-add-traceid-show-for-tpdm-v3-1-0eb836d4ec30@oss.qualcomm.com> <95610981-ad68-4a31-a776-27894b7bca59@linaro.org> <207b78e5-ed6b-4caf-b9ce-546cf33d6dfd@oss.qualcomm.com> Content-Language: en-US From: James Clark In-Reply-To: <207b78e5-ed6b-4caf-b9ce-546cf33d6dfd@oss.qualcomm.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit On 31/03/2026 4:18 am, Jie Gan wrote: > Hi James, > > On 3/31/2026 9:29 AM, Jie Gan wrote: >> >> Hi James, >> >> On 3/30/2026 10:55 PM, James Clark wrote: >>> >>> >>> On 25/03/2026 3:10 am, Jie Gan wrote: >>>> Save the trace ID in drvdata during TPDM enablement and expose it >>>> to userspace to support trace data parsing. >>>> >>>> The TPDM device’s trace ID corresponds to the trace ID allocated >>>> to the connected TPDA device. >>>> >>>> Signed-off-by: Jie Gan >>>> --- >>>> Changes in v3: >>>> 1. Only allow user to read the traceid while the TPDM device is >>>> enabled. >>>> - Link to v2: https://lore.kernel.org/r/20260316-add-traceid-show- >>>> for- tpdm-v2-1-1dec2a67e4ed@oss.qualcomm.com >>>> >>>> Changes in V2: >>>> 1. Use sysfs_emit instead of sprintf. >>>> Link to V1 - https://lore.kernel.org/all/20260306-add-traceid-show- >>>> for-tpdm-v1-1-0658a8edb972@oss.qualcomm.com/ >>>> --- >>>>   drivers/hwtracing/coresight/coresight-tpdm.c | 34 ++++++++++++++++ >>>> + + +++++++++- >>>>   drivers/hwtracing/coresight/coresight-tpdm.h |  2 ++ >>>>   2 files changed, 35 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/ >>>> hwtracing/coresight/coresight-tpdm.c >>>> index da77bdaad0a4..c8339b973bfc 100644 >>>> --- a/drivers/hwtracing/coresight/coresight-tpdm.c >>>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c >>>> @@ -481,7 +481,7 @@ static void __tpdm_enable(struct tpdm_drvdata >>>> *drvdata) >>>>   static int tpdm_enable(struct coresight_device *csdev, struct >>>> perf_event *event, >>>>                  enum cs_mode mode, >>>> -               __maybe_unused struct coresight_path *path) >>>> +               struct coresight_path *path) >>>>   { >>>>       struct tpdm_drvdata *drvdata = dev_get_drvdata(csdev- >>>> >dev.parent); >>>> @@ -497,6 +497,7 @@ static int tpdm_enable(struct coresight_device >>>> *csdev, struct perf_event *event, >>>>       } >>>>       __tpdm_enable(drvdata); >>>> +    drvdata->traceid = path->trace_id; >>>>       drvdata->enable = true; >>>>       spin_unlock(&drvdata->spinlock); >>>> @@ -693,6 +694,29 @@ static struct attribute_group tpdm_attr_grp = { >>>>       .attrs = tpdm_attrs, >>>>   }; >>>> +static ssize_t traceid_show(struct device *dev, >>>> +                struct device_attribute *attr, char *buf) >>>> +{ >>>> +    unsigned long val; >>>> +    struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); >>>> + >>>> +    if (coresight_get_mode(drvdata->csdev) == CS_MODE_DISABLED) >>>> +        return -EINVAL; >>>> + >>>> +    val = drvdata->traceid; >>> >>> You probably need to take the coresight_mutex here otherwise you >>> could still return an invalid or stale value despite checking the mode. >>> >> >> Acked. I have missed this potential race condition. >> >>> There might also be some value in it returning the last used trace ID >>> even if the mode isn't enabled anymore. Because you can still read >>> out of the sink after disabling, so it makes more sense for a script >>> to read it at that point rather than when it's enabled. Also, you >>> probably don't want to be doing other things in your script in the >>> point between enabling and disabling. >> >> That's making sense. I shouldnt add such restriction for the read >> process. >> > > I missed one point in last message. > > Is that acceptable to export the coresight_mutex from the core module? > Currently, the coresight_mutex is used within the module only. > > Thanks, > Jie > If the plan is to only check for non-zero trace ID and not check the mode any more then you don't need to lock. Maybe lets see what Suzuki thinks about returning the last trace ID though as it was his idea to add -EINVAL. >> Scenarios for reading: >> 1. device is enabled -> trace ID is valid >> 2. device is enabled then disabled -> trace ID is valid for the last >> trace event >> 3. device is never enabled -> invalid trace ID (value 0) >> >> we only need to check the validation of the trace ID. >> >> mutex_lock(&coresight_mutex); >> val = drvdata->traceid; >> mutex_unlock(&coresight_mutex); >> >> if (!val) >>      return -EINVAL; >> >> return sysfs_emit(buf, "%#lx\n", val); >> >> Thanks, >> Jie >> >>> >>>> +    return sysfs_emit(buf, "%#lx\n", val); >>>> +} >>>> +static DEVICE_ATTR_RO(traceid); >>>> + >>>> +static struct attribute *traceid_attrs[] = { >>>> +    &dev_attr_traceid.attr, >>>> +    NULL, >>>> +}; >>>> + >>>> +static struct attribute_group traceid_attr_grp = { >>>> +    .attrs = traceid_attrs, >>>> +}; >>>> + >>>>   static ssize_t dsb_mode_show(struct device *dev, >>>>                    struct device_attribute *attr, >>>>                    char *buf) >>>> @@ -1367,6 +1391,12 @@ static const struct attribute_group >>>> *tpdm_attr_grps[] = { >>>>       &tpdm_cmb_patt_grp, >>>>       &tpdm_cmb_msr_grp, >>>>       &tpdm_mcmb_attr_grp, >>>> +    &traceid_attr_grp, >>>> +    NULL, >>>> +}; >>>> + >>>> +static const struct attribute_group *static_tpdm_attr_grps[] = { >>>> +    &traceid_attr_grp, >>>>       NULL, >>>>   }; >>>> @@ -1425,6 +1455,8 @@ static int tpdm_probe(struct device *dev, >>>> struct resource *res) >>>>       desc.access = CSDEV_ACCESS_IOMEM(base); >>>>       if (res) >>>>           desc.groups = tpdm_attr_grps; >>>> +    else >>>> +        desc.groups = static_tpdm_attr_grps; >>>>       drvdata->csdev = coresight_register(&desc); >>>>       if (IS_ERR(drvdata->csdev)) >>>>           return PTR_ERR(drvdata->csdev); >>>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h b/drivers/ >>>> hwtracing/coresight/coresight-tpdm.h >>>> index 2867f3ab8186..11da64e1ade8 100644 >>>> --- a/drivers/hwtracing/coresight/coresight-tpdm.h >>>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.h >>>> @@ -300,6 +300,7 @@ struct cmb_dataset { >>>>    * @cmb         Specifics associated to TPDM CMB. >>>>    * @dsb_msr_num Number of MSR supported by DSB TPDM >>>>    * @cmb_msr_num Number of MSR supported by CMB TPDM >>>> + * @traceid    Trace ID of the path. >>>>    */ >>>>   struct tpdm_drvdata { >>>> @@ -313,6 +314,7 @@ struct tpdm_drvdata { >>>>       struct cmb_dataset    *cmb; >>>>       u32            dsb_msr_num; >>>>       u32            cmb_msr_num; >>>> +    u8            traceid; >>>>   }; >>>>   /* Enumerate members of various datasets */ >>>> >>>> --- >>>> base-commit: b84a0ebe421ca56995ff78b66307667b62b3a900 >>>> change-id: 20260316-add-traceid-show-for-tpdm-88d040651f00 >>>> >>>> Best regards, >>> >> >