| 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
| ||
|
Message-ID: <90f53256-8c75-4b43-aac6-5cb9084efe1c@linaro.org>
Date: Wed, 29 Jan 2025 14:07:53 +0000
From: James Clark <james.clark@...aro.org>
To: Jie Gan <quic_jiegan@...cinc.com>
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 29/01/2025 1:02 pm, Jie Gan wrote:
>
>
> On 1/29/2025 6:35 PM, James Clark wrote:
>>
>>
>> On 29/01/2025 12:46 am, Jie Gan wrote:
>>>
>>>
>>> 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
>>>
>>>
>>
>> No it can't become negative, but the refcount can be a different state
>> to the one that was actually written:
>>
>>
>> CPU0 CPU1
>> ---- ----
>> ctcu_set_etr_traceid(enable)
>> ctcu_set_etr_traceid(disable)
>> atomic_inc()
>> recount == 1
>> atomic_dec()
>> recount == 0
>>
>> __ctcu_set_etr_traceid(disable)
>> Lock and write disable state to
>> device
>>
>> __ctcu_set_etr_traceid(enable)
>> Lock and write enable state to
>> device
>>
>>
>> As you can see this leaves the device in an enabled state but the
>> refcount is 0.
> Yes, you are right. I didnt consider this scenario. We definitely need
> spinlock here.
>
>>
>> This is also quite large if you use atomic types:
>>
>> /* refcnt for each traceid of each sink */
>> atomic_t traceid_refcnt[ATID_MAX_NUM][CORESIGHT_TRACE_ID_RES_TOP];
>>
>> Presumably you can't have the refcount for each ID be higher than the
>> max number of TPDMs connected? If you make the locked area a bit wider
>> you don't need atomic types and also solve the above problem. So you
>> could do u8, or DECLARE_BITMAP() and bitmap_read() etc to read 3 bit
>> values. Or however wide it needs to be.
> The original purpose of using atomic here is trying to narrow the locked
> area.
>
> I think u8 is ok here.
> u8 traceid_refcnt[ATID_MAX_NUM][CORESIGHT_TRACE_ID_RES_TOP] will cost
> 224 bytes, I think it's acceptable here.
>
> Thanks,
> Jie
>
Yep u8 sounds ok then
Powered by blists - more mailing lists