| 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: <a7b301b5-bd9b-4b50-9299-e44a78fcf5a8@quicinc.com>
Date: Mon, 24 Feb 2025 18:04:49 +0800
From: Jie Gan <quic_jiegan@...cinc.com>
To: James Clark <james.clark@...aro.org>,
Suzuki K Poulose
<suzuki.poulose@....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>,
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 v12 6/7] Coresight: Add Coresight TMC Control Unit driver
On 2/24/2025 5:53 PM, James Clark wrote:
>
>
> On 24/02/2025 3:32 am, Jie Gan wrote:
>>
>>
>> On 2/21/2025 7:39 PM, Suzuki K Poulose wrote:
>>> On 17/02/2025 09:30, 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.
>>>>
>>>> Reviewed-by: James Clark <james.clark@...aro.org>
>>>> 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 | 268 +++++++++++++++
>>>> ++++
>>>> drivers/hwtracing/coresight/coresight-ctcu.h | 24 ++
>>>> include/linux/coresight.h | 3 +-
>>>> 5 files changed, 307 insertions(+), 1 deletion(-)
>>>> create mode 100644 drivers/hwtracing/coresight/coresight-ctcu.c
>>>> create mode 100644 drivers/hwtracing/coresight/coresight-ctcu.h
>>>>
>>>> diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/
>>>> hwtracing/ coresight/Kconfig
>>>> index 06f0a7594169..ecd7086a5b83 100644
>>>> --- a/drivers/hwtracing/coresight/Kconfig
>>>> +++ b/drivers/hwtracing/coresight/Kconfig
>>>> @@ -133,6 +133,18 @@ config CORESIGHT_STM
>>>> To compile this driver as a module, choose M here: the
>>>> module will be called coresight-stm.
>>>> +config CORESIGHT_CTCU
>>>> + tristate "CoreSight TMC Control Unit driver"
>>>> + depends on CORESIGHT_LINK_AND_SINK_TMC
>>>> + help
>>>> + This driver provides support for CoreSight TMC Control Unit
>>>> + that hosts miscellaneous configuration registers. This is
>>>> + primarily used for controlling the behaviors of the TMC
>>>> + ETR device.
>>>> +
>>>> + To compile this driver as a module, choose M here: the
>>>> + module will be called coresight-ctcu.
>>>> +
>>>> config CORESIGHT_CPU_DEBUG
>>>> tristate "CoreSight CPU Debug driver"
>>>> depends on ARM || ARM64
>>>> diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/
>>>> hwtracing/ coresight/Makefile
>>>> index 4ba478211b31..1b7869910a12 100644
>>>> --- a/drivers/hwtracing/coresight/Makefile
>>>> +++ b/drivers/hwtracing/coresight/Makefile
>>>> @@ -51,3 +51,4 @@ coresight-cti-y := coresight-cti-core.o coresight-
>>>> cti-platform.o \
>>>> coresight-cti-sysfs.o
>>>> obj-$(CONFIG_ULTRASOC_SMB) += ultrasoc-smb.o
>>>> obj-$(CONFIG_CORESIGHT_DUMMY) += coresight-dummy.o
>>>> +obj-$(CONFIG_CORESIGHT_CTCU) += coresight-ctcu.o
>>>> diff --git a/drivers/hwtracing/coresight/coresight-ctcu.c b/drivers/
>>>> hwtracing/coresight/coresight-ctcu.c
>>>> new file mode 100644
>>>> index 000000000000..e1460a627c4d
>>>> --- /dev/null
>>>> +++ b/drivers/hwtracing/coresight/coresight-ctcu.c
>>>> @@ -0,0 +1,268 @@
>>>> +// SPDX-License-Identifier: GPL-2.0-only
>>>> +/*
>>>> + * Copyright (c) 2024-2025 Qualcomm Innovation Center, Inc. All
>>>> rights reserved.
>>>> + */
>>>> +
>>>> +#include <linux/clk.h>
>>>> +#include <linux/coresight.h>
>>>> +#include <linux/device.h>
>>>> +#include <linux/err.h>
>>>> +#include <linux/kernel.h>
>>>> +#include <linux/init.h>
>>>> +#include <linux/io.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/mutex.h>
>>>> +#include <linux/of.h>
>>>> +#include <linux/platform_device.h>
>>>> +#include <linux/slab.h>
>>>> +
>>>> +#include "coresight-ctcu.h"
>>>> +#include "coresight-priv.h"
>>>> +
>>>> +DEFINE_CORESIGHT_DEVLIST(ctcu_devs, "ctcu");
>>>> +
>>>> +#define ctcu_writel(drvdata, val, offset) __raw_writel((val),
>>>> drvdata->base + offset)
>>>> +#define ctcu_readl(drvdata, offset) __raw_readl(drvdata-
>>>> >base + offset)
>>>> +
>>>> +/*
>>>> + * The TMC Coresight Control Unit uses four ATID registers to
>>>> control the data
>>>> + * filter function based on the trace ID for each TMC ETR sink. The
>>>> length of
>>>> + * each ATID register is 32 bits. Therefore, the ETR has a related
>>>> field in
>>>> + * CTCU that is 128 bits long. Each trace ID is represented by one
>>>> bit in that
>>>> + * filed.
>>>> + * e.g. ETR0ATID0 layout, set bit 5 for traceid 5
>>>> + * bit5
>>>> + * ------------------------------------------------------
>>>> + * | |28| |24| |20| |16| |12| |8| 1|4| |0|
>>>> + * ------------------------------------------------------
>>>> + *
>>>> + * e.g. ETR0:
>>>> + * 127 0 from ATID_offset for ETR0ATID0
>>>> + * -------------------------
>>>> + * |ATID3|ATID2|ATID1|ATID0|
>>>> + */
>>>> +#define CTCU_ATID_REG_OFFSET(traceid, atid_offset) \
>>>> + ((traceid / 32) * 4 + atid_offset)
>>>> +
>>>> +#define CTCU_ATID_REG_BIT(traceid) (traceid % 32)
>>>> +#define CTCU_ATID_REG_SIZE 0x10
>>>> +
>>>> +struct ctcu_atid_config {
>>>> + const u32 atid_offset;
>>>> + const u32 port_num;
>>>> +};
>>>> +
>>>> +struct ctcu_config {
>>>> + const struct ctcu_atid_config *atid_config;
>>>> + int num_atid_config;
>>>> +};
>>>> +
>>>> +static const struct ctcu_atid_config sa8775p_atid_cfgs[] = {
>>>> + {0xf8, 0},
>>>> + {0x108, 1},
>>>> +};
>>>> +
>>>> +static const struct ctcu_config sa8775p_cfgs = {
>>>> + .atid_config = sa8775p_atid_cfgs,
>>>> + .num_atid_config = ARRAY_SIZE(sa8775p_atid_cfgs),
>>>> +};
>>>> +
>>>> +static void ctcu_program_atid_register(struct ctcu_drvdata
>>>> *drvdata, u32 reg_offset,
>>>> + u8 bit, bool enable)
>>>> +{
>>>> + u32 val;
>>>> +
>>>> + CS_UNLOCK(drvdata->base);
>>>> + val = ctcu_readl(drvdata, reg_offset);
>>>> + val = enable? (val | BIT(bit)) : (val & ~BIT(bit));
>>>
>>> minor nit: If possible do not use the ternary operator like this. It
>>> is much better readable as:
>>>
>>> if (enable)
>>> val |= BIT(bit);
>>> else
>>> val &= ~BIT(bit);
>>>
>>
>> Will do this way.
>>
>>>> + ctcu_writel(drvdata, val, reg_offset);
>>>> + CS_LOCK(drvdata->base);
>>>> +}
>>>> +
>>>> +/*
>>>> + * __ctcu_set_etr_traceid: Set bit in the ATID register based on
>>>> trace ID when enable is true.
>>>> + * Reset the bit of the ATID register based on trace ID when enable
>>>> is false.
>>>> + *
>>>> + * @csdev: coresight_device struct related to the device
>>>> + * @traceid: trace ID of the source tracer.
>>>> + * @port_num: port number from TMC ETR sink.
>>>> + * @enable: True for set bit and false for reset bit.
>>>> + *
>>>> + * Returns 0 indicates success. Non-zero result means failure.
>>>> + */
>>>> +static int __ctcu_set_etr_traceid(struct coresight_device *csdev,
>>>> u8 traceid, int port_num,
>>>> + bool enable)
>>>> +{
>>>> + struct ctcu_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>>>> + u32 atid_offset, reg_offset;
>>>> + u8 refcnt, bit;
>>>> +
>>>> + atid_offset = drvdata->atid_offset[port_num];
>>>> + if (atid_offset == 0)
>>>> + return -EINVAL;
>>>> +
>>>> + bit = CTCU_ATID_REG_BIT(traceid);
>>>> + reg_offset = CTCU_ATID_REG_OFFSET(traceid, atid_offset);
>>>> + if (reg_offset - atid_offset > CTCU_ATID_REG_SIZE)
>>>> + return -EINVAL;
>>>> +
>>>> + guard(raw_spinlock_irqsave)(&drvdata->spin_lock);
>>>> + refcnt = drvdata->traceid_refcnt[port_num][traceid];
>>>> + /* Only program the atid register when the refcnt value is 0 or
>>>> 1 */
>>>
>>> A normal trace source won't be enabled more than once (e.g., ETM).
>>> The only odd one out is the STM, which may be driven by multiple agents.
>>> So this refcounting looks necessary.
>>>
>>
>> Besides, for the TPDMs which shared the trace_id of the TPDA also need
>> the refcnt. Consider we have TPDM1 and TPDM2 connected to the same
>> TPDA device. Once we disable one of the TPDM without checking the
>> refcnt, the filter function will be disabled for another TPDM.
>>
>>>> + if (enable && (++refcnt == 1))
>>>> + ctcu_program_atid_register(drvdata, reg_offset, bit, enable);
>>>> + else if (!enable && (--refcnt == 0))
>>>> + ctcu_program_atid_register(drvdata, reg_offset, bit, enable);
>>>
>>> minor nit:
>>>
>>> if ((enable && !refcount++) ||
>>> (!enable && --refcount))
>>> ctcu_program_atid_register(drvdata, reg_offset, bit, enable);
>>>
>>>
>>
>> I did (enable && (++refcnt == 1)) just because I think we only need
>> program the register when refcnt is equal to 1. We dont need reprogram
>> the register with same value when refcnt greater than 1. So I think
>> it's better for the performance?
>>
>>> Also, see my comment the bottom for "refcount" being u8 .
>>
>> Sure, will check.
>>
>>>
>>>
>>>> +
>>>> + drvdata->traceid_refcnt[port_num][traceid] = refcnt;
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int ctcu_get_active_port(struct coresight_device *sink,
>>>> struct coresight_device *helper)
>>>> +{
>>>> + int i;
>>>> +
>>>> + for (i = 0; i < sink->pdata->nr_outconns; ++i) {
>>>> + if (sink->pdata->out_conns[i]->dest_dev)
>>>> + return sink->pdata->out_conns[i]->dest_port;
>>>
>>> Don't we need to make sure it matches the helper ? What if there are
>>> multiple helpers ? e.g, a CATU connected to the ETR and CTCU ?
>>> Or even try the opposite ? i.e. search the helper and find the port
>>> matching the sink ?
>
> Good catch, looks like this should be done the opposite way around.
>
>>>
>>> struct coresight_platform_data *pdata = helper->pdata;
>>>
>>> for (i = 0; i < pdata->nr_inconns; ++i)
>>> if (pdata->in_conns[i]->dest_dev == sink)
>>> return pdata->in_conns[i]->src_port;
>>>
>>> Not sure if that works with the helper device connection, James ?
>
> Yeah connections are always made in both directions.
>
>> Can we check the subtype of the helper device? We should only have one
>> CTCU helper device for each ETR.
>>
>> enum coresight_dev_subtype_helper subtype;
>>
>> for (i = 0; i < sink->pdata->nr_outconns; ++i) {
>> subtype = sink->pdata->out_conns[i]->dest_dev-
>> >subtype.helper_subtype;
>> if (subtype == CORESIGHT_DEV_SUBTYPE_HELPER_CTCU)
>> return sink->pdata->out_conns[i]->dest_port;
>>
>
> I don't think we need to check the type, just search all the CTCU's
> in_conns until you find the sink.
>
> As Suzuki says, by looking at the out_conns of the sink you might find a
> different helper device. Checking that it really is connected to the
> sink is probably more robust that relying on the type anyway.
>
Hi James,
Thanks for explaination, will update per Suzuki's suggestion.
Jie
>
Powered by blists - more mailing lists