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: <20251209121916.GT724103@e132581.arm.com>
Date: Tue, 9 Dec 2025 12:19:16 +0000
From: Leo Yan <leo.yan@....com>
To: Yingchao Deng <yingchao.deng@....qualcomm.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, quic_yingdeng@...cinc.com,
	suzuki.poulose@....com, tingwei.zhang@....qualcomm.com
Subject: Re: [PATCH v6 2/2] coresight: cti: Add Qualcomm extended CTI support

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);
  }

>     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