| 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: <0d697322-90ff-4945-a7bc-789e7029a7d5@quicinc.com>
Date: Fri, 11 Apr 2025 09:52:03 +0800
From: songchai <quic_songchai@...cinc.com>
To: Mike Leach <mike.leach@...aro.org>
CC: Suzuki K Poulose <suzuki.poulose@....com>,
James Clark
<james.clark@....com>,
Alexander Shishkin
<alexander.shishkin@...ux.intel.com>,
Andy Gross <agross@...nel.org>,
"Bjorn
Andersson" <andersson@...nel.org>,
Rob Herring <robh+dt@...nel.org>,
"Krzysztof Kozlowski" <krzk+dt@...nel.org>,
Conor Dooley
<conor+dt@...nel.org>, <linux-kernel@...r.kernel.org>,
<coresight@...ts.linaro.org>, <linux-arm-kernel@...ts.infradead.org>,
<linux-arm-msm@...r.kernel.org>, <devicetree@...r.kernel.org>,
Songwei Chai
<quic_songchai@...cinc.com>
Subject: Re: [PATCH v3 4/7] coresight-tgu: Add TGU decode support
On 4/10/2025 9:35 PM, Mike Leach wrote:
> Hi,
>
> On Thu, 10 Apr 2025 at 08:46, songchai <quic_songchai@...cinc.com> wrote:
>>
>> On 3/7/2025 6:58 PM, Mike Leach wrote:
>>
>> Hi,
>>
>> On Thu, 27 Feb 2025 at 09:27, songchai <quic_songchai@...cinc.com> wrote:
>>
>> From: Songwei Chai <quic_songchai@...cinc.com>
>>
>> Decoding is when all the potential pieces for creating a trigger
>> are brought together for a given step. Example - there may be a
>> counter keeping track of some occurrences and a priority-group that
>> is being used to detect a pattern on the sense inputs. These 2
>> inputs to condition_decode must be programmed, for a given step,
>> to establish the condition for the trigger, or movement to another
>> step.
>>
>> Signed-off-by: Songwei Chai <quic_songchai@...cinc.com>
>> Signed-off-by: songchai <quic_songchai@...cinc.com>
>> ---
>> .../testing/sysfs-bus-coresight-devices-tgu | 7 ++
>> drivers/hwtracing/coresight/coresight-tgu.c | 113 ++++++++++++++++--
>> drivers/hwtracing/coresight/coresight-tgu.h | 29 ++++-
>> 3 files changed, 136 insertions(+), 13 deletions(-)
>>
>> diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tgu b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tgu
>> index af7332153833..dd6cc1184d52 100644
>> --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tgu
>> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tgu
>> @@ -14,3 +14,10 @@ KernelVersion 6.15
>> Contact: Jinlong Mao (QUIC) <quic_jinlmao@...cinc.com>, Sam Chai (QUIC) <quic_songchai@...cinc.com>
>> Description:
>> (RW) Set/Get the sensed siganal with specific step and priority for TGU.
>> +
>> +What: /sys/bus/coresight/devices/<tgu-name>/step[0:7]_condition_decode/reg[0:3]
>> +Date: February 2025
>> +KernelVersion 6.15
>> +Contact: Jinlong Mao (QUIC) <quic_jinlmao@...cinc.com>, Sam Chai (QUIC) <quic_songchai@...cinc.com>
>> +Description:
>> + (RW) Set/Get the decode mode with specific step for TGU.
>> diff --git a/drivers/hwtracing/coresight/coresight-tgu.c b/drivers/hwtracing/coresight/coresight-tgu.c
>> index f28761619ebe..5eebf5eecbbb 100644
>> --- a/drivers/hwtracing/coresight/coresight-tgu.c
>> +++ b/drivers/hwtracing/coresight/coresight-tgu.c
>> @@ -22,9 +22,21 @@ static int calculate_array_location(struct tgu_drvdata *drvdata, int step_index,
>> {
>> int ret = -EINVAL;
>>
>> - ret = operation_index * (drvdata->max_step) *
>> - (drvdata->max_reg) + step_index * (drvdata->max_reg)
>> - + reg_index;
>> + switch (operation_index) {
>> + case TGU_PRIORITY0:
>> + case TGU_PRIORITY1:
>> + case TGU_PRIORITY2:
>> + case TGU_PRIORITY3:
>> + ret = operation_index * (drvdata->max_step) *
>> + (drvdata->max_reg) + step_index * (drvdata->max_reg)
>> + + reg_index;
>> + break;
>> + case TGU_CONDITION_DECODE:
>> + ret = step_index * (drvdata->max_condition_decode) + reg_index;
>> + break;
>> + default:
>> + break;
>> + }
>>
>> return ret;
>> }
>> @@ -36,10 +48,23 @@ static ssize_t tgu_dataset_show(struct device *dev,
>> struct tgu_attribute *tgu_attr =
>> container_of(attr, struct tgu_attribute, attr);
>>
>> - return sysfs_emit(buf, "0x%x\n",
>> - drvdata->value_table->priority[calculate_array_location(
>> + switch (tgu_attr->operation_index) {
>> + case TGU_PRIORITY0:
>> + case TGU_PRIORITY1:
>> + case TGU_PRIORITY2:
>> + case TGU_PRIORITY3:
>> + return sysfs_emit(buf, "0x%x\n",
>> + drvdata->value_table->priority[calculate_array_location(
>>
>> calculate_array_location() can return -EINVAL - you could be
>> referencing array value drvdata->value_table->priority[-EINVAL] here.
>> Test the return before referencing the array.
>>
>> Same for all following occurrences.
>>
>> Both tgu_dataset_store/show and calculate_array_location use switch cases to evaluate tgu_attr->operation_index.
>>
>> Therefore, when a case in tgu_dataset_store/show satisfies tgu_attr->operation_index, calculate_array_location
>>
>> will not return -EINVAL. This prevents the occurrence of -EINVAL within the array.
> If the function can never return -EINVAL, then do not use is at all in
> the function - it is unneeded.
> It does not matter that you believe that the current implementation
> can never produce an incorrect response - if you write a function that
> can return -EINVAL, this must be checked for - do not assume that
> future changes will retain the apparent "safety".
>
>
> Regards
>
> Mike
Thanks for your clarification, Mike.
Will add check in next version.
>
>> drvdata, tgu_attr->step_index,
>> tgu_attr->operation_index, tgu_attr->reg_num)]);
>> + case TGU_CONDITION_DECODE:
>> + return sysfs_emit(buf, "0x%x\n",
>> + drvdata->value_table->condition_decode[calculate_array_location(
>> + drvdata, tgu_attr->step_index, tgu_attr->operation_index,
>> + tgu_attr->reg_num)]);
>> +
>>
>> missing default - did this code compile without warnings?
>>
>> Will correct it in next version.
>>
>> + }
>> + return -EINVAL;
>>
>> }
>>
>> @@ -58,11 +83,25 @@ static ssize_t tgu_dataset_store(struct device *dev,
>> return ret;
>>
>> guard(spinlock)(&tgu_drvdata->spinlock);
>> - tgu_drvdata->value_table->priority[calculate_array_location(
>> - tgu_drvdata, tgu_attr->step_index, tgu_attr->operation_index,
>> - tgu_attr->reg_num)] = val;
>> - ret = size;
>> -
>> + switch (tgu_attr->operation_index) {
>> + case TGU_PRIORITY0:
>> + case TGU_PRIORITY1:
>> + case TGU_PRIORITY2:
>> + case TGU_PRIORITY3:
>> + tgu_drvdata->value_table->priority[calculate_array_location(
>> + tgu_drvdata, tgu_attr->step_index, tgu_attr->operation_index,
>> + tgu_attr->reg_num)] = val;
>> + ret = size;
>> + break;
>> + case TGU_CONDITION_DECODE:
>> + tgu_drvdata->value_table->condition_decode[calculate_array_location(
>> + tgu_drvdata, tgu_attr->step_index, tgu_attr->operation_index,
>> + tgu_attr->reg_num)] = val;
>> + ret = size;
>> + break;
>> + default:
>> + break;
>> + }
>> return ret;
>> }
>>
>> @@ -79,8 +118,23 @@ static umode_t tgu_node_visible(struct kobject *kobject, struct attribute *attr,
>> container_of(dev_attr, struct tgu_attribute, attr);
>>
>> if (tgu_attr->step_index < drvdata->max_step) {
>> - ret = (tgu_attr->reg_num < drvdata->max_reg) ?
>> - attr->mode : 0;
>> + switch (tgu_attr->operation_index) {
>> + case TGU_PRIORITY0:
>> + case TGU_PRIORITY1:
>> + case TGU_PRIORITY2:
>> + case TGU_PRIORITY3:
>> + ret = (tgu_attr->reg_num < drvdata->max_reg) ?
>> + attr->mode : 0;
>> + break;
>> + case TGU_CONDITION_DECODE:
>> + ret = (tgu_attr->reg_num <
>> + drvdata->max_condition_decode) ?
>> + attr->mode : 0;
>> + break;
>> + default:
>> + break;
>> + }
>> +
>> return ret;
>> }
>> return SYSFS_GROUP_INVISIBLE;
>> @@ -103,6 +157,17 @@ static void tgu_write_all_hw_regs(struct tgu_drvdata *drvdata)
>> }
>> }
>>
>> + for (i = 0; i < drvdata->max_step; i++) {
>> + for (j = 0; j < drvdata->max_condition_decode; j++) {
>> + tgu_writel(drvdata,
>> + drvdata->value_table
>> + ->condition_decode[calculate_array_location(
>> + drvdata, i,
>> + TGU_CONDITION_DECODE, j)],
>> + CONDITION_DECODE_STEP(i, j));
>> + }
>> + }
>> +
>> /* Enable TGU to program the triggers */
>> tgu_writel(drvdata, 1, TGU_CONTROL);
>> CS_LOCK(drvdata->base);
>> @@ -245,6 +310,14 @@ static const struct attribute_group *tgu_attr_groups[] = {
>> PRIORITY_ATTRIBUTE_GROUP_INIT(7, 1),
>> PRIORITY_ATTRIBUTE_GROUP_INIT(7, 2),
>> PRIORITY_ATTRIBUTE_GROUP_INIT(7, 3),
>> + CONDITION_DECODE_ATTRIBUTE_GROUP_INIT(0),
>> + CONDITION_DECODE_ATTRIBUTE_GROUP_INIT(1),
>> + CONDITION_DECODE_ATTRIBUTE_GROUP_INIT(2),
>> + CONDITION_DECODE_ATTRIBUTE_GROUP_INIT(3),
>> + CONDITION_DECODE_ATTRIBUTE_GROUP_INIT(4),
>> + CONDITION_DECODE_ATTRIBUTE_GROUP_INIT(5),
>> + CONDITION_DECODE_ATTRIBUTE_GROUP_INIT(6),
>> + CONDITION_DECODE_ATTRIBUTE_GROUP_INIT(7),
>> NULL,
>> };
>>
>> @@ -289,6 +362,13 @@ static int tgu_probe(struct amba_device *adev, const struct amba_id *id)
>> if (ret)
>> return -EINVAL;
>>
>> + ret = of_property_read_u32(adev->dev.of_node, "tgu-conditions",
>> + &drvdata->max_condition);
>> + if (ret)
>> + return -EINVAL;
>> +
>> + drvdata->max_condition_decode = drvdata->max_condition;
>> +
>> drvdata->value_table =
>> devm_kzalloc(dev, sizeof(*drvdata->value_table), GFP_KERNEL);
>> if (!drvdata->value_table)
>> @@ -303,6 +383,15 @@ static int tgu_probe(struct amba_device *adev, const struct amba_id *id)
>> if (!drvdata->value_table->priority)
>> return -ENOMEM;
>>
>> + drvdata->value_table->condition_decode = devm_kzalloc(
>> + dev,
>> + drvdata->max_condition_decode * drvdata->max_step *
>> + sizeof(*(drvdata->value_table->condition_decode)),
>> + GFP_KERNEL);
>> +
>> + if (!drvdata->value_table->condition_decode)
>> + return -ENOMEM;
>> +
>> drvdata->enable = false;
>> desc.type = CORESIGHT_DEV_TYPE_HELPER;
>> desc.pdata = adev->dev.platform_data;
>> diff --git a/drivers/hwtracing/coresight/coresight-tgu.h b/drivers/hwtracing/coresight/coresight-tgu.h
>> index 6e5d465117df..c2a9ce38b44f 100644
>> --- a/drivers/hwtracing/coresight/coresight-tgu.h
>> +++ b/drivers/hwtracing/coresight/coresight-tgu.h
>> @@ -46,6 +46,9 @@
>> #define PRIORITY_REG_STEP(step, priority, reg)\
>> (0x0074 + 0x60 * priority + 0x4 * reg + 0x1D8 * step)
>>
>> +#define CONDITION_DECODE_STEP(step, decode) \
>> + (0x0050 + 0x4 * decode + 0x1D8 * step)
>> +
>>
>> use #define constants with explanations of what they are rather than
>> arbitrary magic numbers.
>>
>> Done.
>>
>> #define tgu_dataset_ro(name, step_index, type, reg_num) \
>> (&((struct tgu_attribute[]){ { \
>> __ATTR(name, 0444, tgu_dataset_show, NULL), \
>> @@ -66,6 +69,9 @@
>> tgu_dataset_rw(reg##reg_num, step_index, TGU_PRIORITY##priority, \
>> reg_num)
>>
>> +#define STEP_DECODE(step_index, reg_num) \
>> + tgu_dataset_rw(reg##reg_num, step_index, TGU_CONDITION_DECODE, reg_num)
>> +
>> #define STEP_PRIORITY_LIST(step_index, priority) \
>> {STEP_PRIORITY(step_index, 0, priority), \
>> STEP_PRIORITY(step_index, 1, priority), \
>> @@ -88,6 +94,14 @@
>> NULL \
>> }
>>
>> +#define STEP_DECODE_LIST(n) \
>> + {STEP_DECODE(n, 0), \
>> + STEP_DECODE(n, 1), \
>> + STEP_DECODE(n, 2), \
>> + STEP_DECODE(n, 3), \
>> + NULL \
>> + }
>> +
>> #define PRIORITY_ATTRIBUTE_GROUP_INIT(step, priority)\
>> (&(const struct attribute_group){\
>> .attrs = (struct attribute*[])STEP_PRIORITY_LIST(step, priority),\
>> @@ -95,11 +109,19 @@
>> .name = "step" #step "_priority" #priority \
>> })
>>
>> +#define CONDITION_DECODE_ATTRIBUTE_GROUP_INIT(step)\
>> + (&(const struct attribute_group){\
>> + .attrs = (struct attribute*[])STEP_DECODE_LIST(step),\
>> + .is_visible = tgu_node_visible,\
>> + .name = "step" #step "_condition_decode" \
>> + })
>> +
>> enum operation_index {
>> TGU_PRIORITY0,
>> TGU_PRIORITY1,
>> TGU_PRIORITY2,
>> - TGU_PRIORITY3
>> + TGU_PRIORITY3,
>> + TGU_CONDITION_DECODE
>>
>> };
>>
>> @@ -115,6 +137,7 @@ struct tgu_attribute {
>>
>> struct value_table {
>> unsigned int *priority;
>> + unsigned int *condition_decode;
>> };
>>
>> /**
>> @@ -127,6 +150,8 @@ struct value_table {
>> * @value_table: Store given value based on relevant parameters.
>> * @max_reg: Maximum number of registers
>> * @max_step: Maximum step size
>> + * @max_condition: Maximum number of condition
>> + * @max_condition_decode: Maximum number of condition_decode
>> *
>> * This structure defines the data associated with a TGU device, including its base
>> * address, device pointers, clock, spinlock for synchronization, trigger data pointers,
>> @@ -141,6 +166,8 @@ struct tgu_drvdata {
>> struct value_table *value_table;
>> int max_reg;
>> int max_step;
>> + int max_condition;
>> + int max_condition_decode;
>> };
>>
>> #endif
>>
>> Regards
>>
>> Mike
>>
>
Powered by blists - more mailing lists