linux-kernel - Re: [PATCH v9 5/6] Coresight: Add Coresight TMC Control Unit driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <b3d9e4f0-cca8-45a5-a5b7-239cdeed2751@quicinc.com>
Date: Wed, 29 Jan 2025 08:46:44 +0800
From: Jie Gan <quic_jiegan@...cinc.com>
To: James Clark <james.clark@...aro.org>
CC: Tingwei Zhang <quic_tingweiz@...cinc.com>,
        Jinlong Mao
	<quic_jinlmao@...cinc.com>, <coresight@...ts.linaro.org>,
        <linux-arm-kernel@...ts.infradead.org>, <linux-kernel@...r.kernel.org>,
        <devicetree@...r.kernel.org>, <linux-arm-msm@...r.kernel.org>,
        <linux-stm32@...md-mailman.stormreply.com>,
        Suzuki K Poulose
	<suzuki.poulose@....com>,
        Mike Leach <mike.leach@...aro.org>,
        "Alexander
 Shishkin" <alexander.shishkin@...ux.intel.com>,
        Maxime Coquelin
	<mcoquelin.stm32@...il.com>,
        Alexandre Torgue <alexandre.torgue@...s.st.com>,
        Rob Herring <robh@...nel.org>,
        Krzysztof Kozlowski <krzk+dt@...nel.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Bjorn Andersson <andersson@...nel.org>,
        Konrad Dybcio <konradybcio@...nel.org>
Subject: Re: [PATCH v9 5/6] Coresight: Add Coresight TMC Control Unit driver



On 1/28/2025 7:55 PM, James Clark wrote:
> 
> 
> On 24/01/2025 7:25 am, Jie Gan wrote:
>> The Coresight TMC Control Unit hosts miscellaneous configuration 
>> registers
>> which control various features related to TMC ETR sink.
>>
>> Based on the trace ID, which is programmed in the related CTCU ATID
>> register of a specific ETR, trace data with that trace ID gets into
>> the ETR buffer, while other trace data gets dropped.
>>
>> Enabling source device sets one bit of the ATID register based on
>> source device's trace ID.
>> Disabling source device resets the bit according to the source
>> device's trace ID.
>>
>> Signed-off-by: Jie Gan <quic_jiegan@...cinc.com>
>> ---
>>   drivers/hwtracing/coresight/Kconfig          |  12 +
>>   drivers/hwtracing/coresight/Makefile         |   1 +
>>   drivers/hwtracing/coresight/coresight-ctcu.c | 276 +++++++++++++++++++
>>   drivers/hwtracing/coresight/coresight-ctcu.h |  30 ++
>>   include/linux/coresight.h                    |   3 +-
>>   5 files changed, 321 insertions(+), 1 deletion(-)
>>   create mode 100644 drivers/hwtracing/coresight/coresight-ctcu.c
>>   create mode 100644 drivers/hwtracing/coresight/coresight-ctcu.h
>  >
> 
> [...]
> 
>> +/*
>> + * ctcu_set_etr_traceid: Retrieve the ATID offset and trace ID.
>> + *
>> + * Returns 0 indicates success. None-zero result means failure.
>> + */
>> +static int ctcu_set_etr_traceid(struct coresight_device *csdev, 
>> struct coresight_path *cs_path,
>> +                bool enable)
>> +{
>> +    struct coresight_device *sink = coresight_get_sink(cs_path->path);
>> +    struct ctcu_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>> +    u8 trace_id = cs_path->trace_id;
>> +    int port_num;
>> +
>> +    if ((sink == NULL) || !IS_VALID_CS_TRACE_ID(trace_id) || 
>> IS_ERR_OR_NULL(drvdata)) {
>> +        dev_err(&csdev->dev, "Invalid parameters\n");
>> +        return -EINVAL;
>> +    }
>> +
>> +    port_num = ctcu_get_active_port(sink, csdev);
>> +    if (port_num < 0)
>> +        return -EINVAL;
>> +
>> +    /*
>> +     * Skip the disable session if more than one TPDM device that
>> +     * connected to the same TPDA device has been enabled.
>> +     */
>> +    if (enable)
>> +        atomic_inc(&drvdata->traceid_refcnt[port_num][trace_id]);
>> +    else {
>> +        if (atomic_dec_return(&drvdata->traceid_refcnt[port_num] 
>> [trace_id]) > 0) {
>> +            dev_dbg(&csdev->dev, "Skip the disable session\n");
>> +            return 0;
>> +        }
>> +    }
>> +
>> +    dev_dbg(&csdev->dev, "traceid is %d\n", cs_path->trace_id);
>> +
>> +    return __ctcu_set_etr_traceid(csdev, trace_id, port_num, enable);
> 
> Hi Jie,
> 
> Using atomic_dec_return() here doesn't prevent __ctcu_set_etr_traceid() 
> from running concurrent enable and disables. Once you pass the 
> atomic_dec_return() a second call to enable it will mess it up.
> 
> I think you need a spinlock around the whole thing and then the 
> refcounts don't need to be atomics.
> 
Hi, James
Thanks for comment. I may not fully tested my codes here. What I was 
thinking is there's no way the refcnt could become a negative number 
under current framework. So I just added spinlock in 
__ctcu_set_etr_traceid() to ensure concurrent sessions correctly 
manipulate the register.

As the trace_id related to the bit of the ATID register, I think the 
concurrent processes are working fine with spinlock around read/write 
register.

I may not fully got your point here. Please help me to correct it.

Thanks,
Jie