linux-kernel - Re: [PATCH v6 2/2] coresight: cti: Add Qualcomm extended CTI support

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: <1fbe140c-b960-4f13-83e2-b0a0733a67bc@quicinc.com>
Date: Tue, 9 Dec 2025 20:51:38 +0800
From: "Yingchao Deng (Consultant)" <quic_yingdeng@...cinc.com>
To: Leo Yan <leo.yan@....com>, Yingchao Deng <quic_yingdeng@...cinc.com>
CC: <mike.leach@...aro.org>, <alexander.shishkin@...ux.intel.com>,
        <coresight@...ts.linaro.org>, <james.clark@...aro.org>,
        <jinlong.mao@....qualcomm.com>, <linux-arm-kernel@...ts.infradead.org>,
        <linux-arm-msm@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <quic_jinlmao@...cinc.com>, <suzuki.poulose@....com>,
        <tingwei.zhang@....qualcomm.com>
Subject: Re: [PATCH v6 2/2] coresight: cti: Add Qualcomm extended CTI support

Hi Leo,

On 12/9/2025 8:19 PM, Leo Yan wrote:
> Hi Yingchao,
>
> On Tue, Dec 09, 2025 at 04:16:28PM +0800, Yingchao Deng wrote:
>> Hi Leo & Mike
>>
>> Based on Leo’s suggestions, I created a new patch, but there are three points that do not fully align with his recommendations:
>>
>>      1. The helper function for returning the register address now returns only the offset, because returning the full address would conflict with cti_write_single_reg.
> No need to change each callsite for cti_write_single_reg().  You could
> update cti_write_single_reg() instead:
>
>    void cti_write_single_reg(struct cti_drvdata *drvdata,
>                              int offset, u32 value)
>    {
>            CS_UNLOCK(drvdata->base);
>            writel_relaxed(value, cti_reg_addr(drvdata, offset));
>            CS_LOCK(drvdata->base);
>    }

However, since we also need to handle cti_reg_addr_with_nr, it will be 
necessary to add an additional parameter "nr" to cti_write_single_reg?

Thanks
Yingchao
>>      2. For registers such as triginstatus1...3, I defined additional macros CTITRIGINSTATUS1...3. This is because CTITRIGINSTATUS + 0x4 equals CTITRIGOUTSTATUS, and to avoid conflicts with existing macros, I chose numbers starting from 0x1000 for the new definitions.
> To avoid the register naming pollution, please don't define the common
> names but only used for Qcom registers.
>
> AFAIK, you even don't need to define these registers.  These registers
> are only used for sysfs knobs, we can define an extra "nr" field (e.g.,
> bits[31..28] for indexing these registers, something like:
>
>    #define CIT_REG_NR_SHIFT          28
>    #define CIT_REG_NR_MASK           GENMASK(31, 28)
>    #define CTI_REG_GET_NR(reg)       FIELD_GET(CIT_REG_NR_MASK, (reg))
>    #define CTI_REG_SET_NR(reg, nr)   ((reg) | FIELD_PREP(CIT_REG_NR_MASK, (nr))
>
>    static struct attribute *coresight_cti_regs_attrs[] = {
>    ...
>      coresight_cti_reg(triginstatus, CTITRIGINSTATUS),
>      coresight_cti_reg(triginstatus1, CTI_REG_SET_NR(CTITRIGINSTATUS, 1)),
>      coresight_cti_reg(triginstatus2, CTI_REG_SET_NR(CTITRIGINSTATUS, 2)),
>      coresight_cti_reg(triginstatus3, CTI_REG_SET_NR(CTITRIGINSTATUS, 3)),
>    ...
>
> Then, you just need to decode "nr" fields in cti_qcom_reg_off().
>
>>      3. Regarding the visibility of attributes for triginstatus1...3, since coresight_cti_reg produces an anonymous variable that cannot be directly referenced, I used coresight_cti_regs_attrs[i] to obtain the attribute corresponding to triginstatus1.
> Okay, I get the meaning for "an anonymous variable" - there have no
> field naming when define attr with the macro coresight_cti_reg().
>
> but you could comparing the attr string?
>
>    if (!strcmp(attr->name, "triginstatus1") ||
>        !strcmp(attr->name, "triginstatus2") ||
>        !strcmp(attr->name, "triginstatus3"))
>        ...
>
> Thanks,
> Leo