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

[Jie Gan <quic_jiegan@quicinc.com>]

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@quicinc.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