| 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: <bc337e7c-42e7-4e2d-8b2d-c39174d1ddd5@linaro.org>
Date: Wed, 27 Aug 2025 11:16:01 +0100
From: James Clark <james.clark@...aro.org>
To: Jie Gan <jie.gan@....qualcomm.com>
Cc: coresight@...ts.linaro.org, linux-arm-kernel@...ts.infradead.org,
linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org,
Suzuki K Poulose <suzuki.poulose@....com>, Mike Leach
<mike.leach@...aro.org>,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
Tingwei Zhang <tingwei.zhang@....qualcomm.com>
Subject: Re: [PATCH v2 3/3] coresight: tpda: add sysfs node to flush specific
port
On 27/08/2025 10:48 am, Jie Gan wrote:
>
>
> On 8/27/2025 5:17 PM, James Clark wrote:
>>
>>
>> On 27/08/2025 5:20 am, Jie Gan wrote:
>>> From: Tao Zhang <tao.zhang@....qualcomm.com>
>>>
>>> Setting bit i in the TPDA_FLUSH_CR register initiates a flush request
>>> for port i, forcing the data to synchronize and be transmitted to the
>>> sink device.
>>>
>>> Signed-off-by: Tao Zhang <tao.zhang@....qualcomm.com>
>>> Co-developed-by: Jie Gan <jie.gan@....qualcomm.com>
>>> Signed-off-by: Jie Gan <jie.gan@....qualcomm.com>
>>> ---
>>> .../testing/sysfs-bus-coresight-devices-tpda | 7 ++++
>>> drivers/hwtracing/coresight/coresight-tpda.c | 42 +++++++++++++++++++
>>> drivers/hwtracing/coresight/coresight-tpda.h | 2 +
>>> 3 files changed, 51 insertions(+)
>>>
>>> diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices-
>>> tpda b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpda
>>> index fb651aebeb31..2cf2dcfc13c8 100644
>>> --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpda
>>> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpda
>>> @@ -41,3 +41,10 @@ Contact: Jinlong Mao
>>> <jinlong.mao@....qualcomm.com>, Tao Zhang <tao.zhang@....qu
>>> Description:
>>> (RW) Configure the CMB/MCMB channel mode for all enabled
>>> ports.
>>> Value 0 means raw channel mapping mode. Value 1 means
>>> channel pair marking mode.
>>> +
>>> +What: /sys/bus/coresight/devices/<tpda-name>/port_flush_req
>>> +Date: August 2025
>>> +KernelVersion: 6.17
>>> +Contact: Jinlong Mao <jinlong.mao@....qualcomm.com>, Tao Zhang
>>> <tao.zhang@....qualcomm.com>, Jie Gan <jie.gan@....qualcomm.com>
>>> +Description:
>>> + (RW) Configure the bit i to requests a flush operation of
>>> port i on the TPDA.
>>> diff --git a/drivers/hwtracing/coresight/coresight-tpda.c b/drivers/
>>> hwtracing/coresight/coresight-tpda.c
>>> index 430f76c559f2..8b1fe128881d 100644
>>> --- a/drivers/hwtracing/coresight/coresight-tpda.c
>>> +++ b/drivers/hwtracing/coresight/coresight-tpda.c
>>> @@ -487,6 +487,47 @@ static ssize_t cmbchan_mode_store(struct device
>>> *dev,
>>> }
>>> static DEVICE_ATTR_RW(cmbchan_mode);
>>> +static ssize_t port_flush_req_show(struct device *dev,
>>> + struct device_attribute *attr,
>>> + char *buf)
>>> +{
>>> + struct tpda_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>> + unsigned long val;
>>> +
>>> + if (!drvdata->csdev->refcnt)
>>> + return -EINVAL;
>>> +
>>> + guard(spinlock)(&drvdata->spinlock);
>>> + CS_UNLOCK(drvdata->base);
>>> + val = readl_relaxed(drvdata->base + TPDA_FLUSH_CR);
>>> + CS_LOCK(drvdata->base);
>>> + return sysfs_emit(buf, "%lx\n", val);
>>
>> Still missing the 0x prefix
>
> Will re-check rest of the codes and add prefix. Sorry I missed it during
> the review process.
>
>>
>>> +}
>>> +
>>> +static ssize_t port_flush_req_store(struct device *dev,
>>> + struct device_attribute *attr,
>>> + const char *buf,
>>> + size_t size)
>>> +{
>>> + struct tpda_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>> + unsigned long val;
>>> +
>>> + if (kstrtoul(buf, 0, &val))
>>> + return -EINVAL;
>>> +
>>> + if (!drvdata->csdev->refcnt || !val)
>>> + return -EINVAL;
>>> +
>>> + val |= FIELD_PREP(TPDA_MAX_INPORTS_MASK, val);
>>
>> Using FIELD_PREP() now that it's the full width of the register makes
>> less sense. Especially when there is no corresponding FIELD_FIT()
>> check, which is fine because everything always fits. But if you
>> didn't know the mask was the full width you'd wonder if the check is
>> missing.
>>
>> I would just write val directly to TPDA_FLUSH_CR so it's simpler.
>>
>> It should also have been val = FIELD_PREP(...)
>
> Yeah, it should have been val = FIELD_PREP(...) here... sorry for the
> mistake here..
>
> I was thinking the unsigned long here could be 64 or 32 bits and we only
> need the value of the lower 32 bits. So that's why I am using val =
> FIELD_PREP(...) here. We shouldn't write a value greater than UINT32_MAX
> to the register.
>
> Thanks,
> Jie
>
writel_relaxed() is always 32 bits though so it is a bit confusing if
you truncate the user value without an error. Also a reason to use u32
instead of unsigned long types.
Are you trying to support arm and arm64 with tpda? Or just arm64? For it
to be consistent you can use kstrtou32(), or use kstrtoull() and then
FIELD_FIT() to error on truncation. kstrtou32() is probably the cleanest.
>>
>>> + guard(spinlock)(&drvdata->spinlock);
>>> + CS_UNLOCK(drvdata->base);
>>> + writel_relaxed(val, drvdata->base + TPDA_FLUSH_CR);
>>> + CS_LOCK(drvdata->base);
>>> +
>>> + return size;
>>> +}
>>> +static DEVICE_ATTR_RW(port_flush_req);
>>> +
>>> static struct attribute *tpda_attrs[] = {
>>> &dev_attr_trig_async_enable.attr,
>>> &dev_attr_trig_flag_ts_enable.attr,
>>> @@ -494,6 +535,7 @@ static struct attribute *tpda_attrs[] = {
>>> &dev_attr_freq_ts_enable.attr,
>>> &dev_attr_global_flush_req.attr,
>>> &dev_attr_cmbchan_mode.attr,
>>> + &dev_attr_port_flush_req.attr,
>>> NULL,
>>> };
>>> diff --git a/drivers/hwtracing/coresight/coresight-tpda.h b/drivers/
>>> hwtracing/coresight/coresight-tpda.h
>>> index 8e1b66115ad1..56d3ad293e46 100644
>>> --- a/drivers/hwtracing/coresight/coresight-tpda.h
>>> +++ b/drivers/hwtracing/coresight/coresight-tpda.h
>>> @@ -10,6 +10,7 @@
>>> #define TPDA_Pn_CR(n) (0x004 + (n * 4))
>>> #define TPDA_FPID_CR (0x084)
>>> #define TPDA_SYNCR (0x08C)
>>> +#define TPDA_FLUSH_CR (0x090)
>>> /* Cross trigger FREQ packets timestamp bit */
>>> #define TPDA_CR_FREQTS BIT(2)
>>> @@ -35,6 +36,7 @@
>>> #define TPDA_SYNCR_MAX_COUNTER_VAL (0xFFF)
>>> #define TPDA_MAX_INPORTS 32
>>> +#define TPDA_MAX_INPORTS_MASK GENMASK(31, 0)
>>> /* Bits 6 ~ 12 is for atid value */
>>> #define TPDA_CR_ATID GENMASK(12, 6)
>>
>>
>
Powered by blists - more mailing lists