linux-kernel - Re: [PATCH v10 5/7] qcom-tgu: Add support to configure next action

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: <c1efbad6-4ea6-4261-b7b0-7cb8081ecfb1@oss.qualcomm.com>
Date: Tue, 27 Jan 2026 10:43:11 +0800
From: Songwei Chai <songwei.chai@....qualcomm.com>
To: Konrad Dybcio <konrad.dybcio@....qualcomm.com>, andersson@...nel.org,
        alexander.shishkin@...ux.intel.com, mike.leach@...aro.org,
        suzuki.poulose@....com, james.clark@....com, krzk+dt@...nel.org,
        conor+dt@...nel.org
Cc: linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-arm-msm@...r.kernel.org, coresight@...ts.linaro.org,
        devicetree@...r.kernel.org, gregkh@...uxfoundation.org
Subject: Re: [PATCH v10 5/7] qcom-tgu: Add support to configure next action



On 1/13/2026 7:15 PM, Konrad Dybcio wrote:
> On 1/9/26 3:11 AM, Songwei Chai wrote:
>> Add "select" node for each step to determine if another step is taken,
>> trigger(s) are generated, counters/timers incremented/decremented, etc.
>>
>> Signed-off-by: Songwei Chai <songwei.chai@....qualcomm.com>
>> ---
> 
> [...]
> 
>> +		case TGU_CONDITION_SELECT:
>> +		/* 'default' register is at the end of 'select' region */
>> +			if (tgu_attr->reg_num ==
>> +			    drvdata->max_condition_select - 1)
>> +				attr->name = "default";
>> +			ret = (tgu_attr->reg_num <
>> +				drvdata->max_condition_select) ?
>> +				attr->mode : 0;
> 
> similarly to my previous comments
> 
> [...]
> 
>> +	for (i = 0; i < drvdata->max_step; i++) {
>> +		for (j = 0; j < drvdata->max_condition_select; j++) {
>> +			index = check_array_location(drvdata, i,
>> +						TGU_CONDITION_SELECT, j);
>> +
>> +			if (index == -EINVAL)
> 
> stray \n
>> +				goto exit;
>> +
>> +			writel(drvdata->value_table->condition_select[index],
>> +				drvdata->base + CONDITION_SELECT_STEP(i, j));
>> +		}
>> +	}
>>   	/* Enable TGU to program the triggers */
>>   	writel(1, drvdata->base + TGU_CONTROL);
>>   exit:
>> @@ -225,6 +258,8 @@ static void tgu_set_conditions(struct tgu_drvdata *drvdata)
>>   
>>   	devid = readl(drvdata->base + TGU_DEVID);
>>   	drvdata->max_condition_decode = TGU_DEVID_CONDITIONS(devid);
>> +	/* select region has an additional 'default' register */
>> +	drvdata->max_condition_select = TGU_DEVID_CONDITIONS(devid) + 1;
>>   }
>>   
>>   static int tgu_enable(struct device *dev)
>> @@ -356,6 +391,14 @@ static const struct attribute_group *tgu_attr_groups[] = {
>>   	CONDITION_DECODE_ATTRIBUTE_GROUP_INIT(5),
>>   	CONDITION_DECODE_ATTRIBUTE_GROUP_INIT(6),
>>   	CONDITION_DECODE_ATTRIBUTE_GROUP_INIT(7),
>> +	CONDITION_SELECT_ATTRIBUTE_GROUP_INIT(0),
>> +	CONDITION_SELECT_ATTRIBUTE_GROUP_INIT(1),
>> +	CONDITION_SELECT_ATTRIBUTE_GROUP_INIT(2),
>> +	CONDITION_SELECT_ATTRIBUTE_GROUP_INIT(3),
>> +	CONDITION_SELECT_ATTRIBUTE_GROUP_INIT(4),
>> +	CONDITION_SELECT_ATTRIBUTE_GROUP_INIT(5),
>> +	CONDITION_SELECT_ATTRIBUTE_GROUP_INIT(6),
>> +	CONDITION_SELECT_ATTRIBUTE_GROUP_INIT(7),
>>   	NULL,
>>   };
>>   
>> @@ -363,8 +406,8 @@ static int tgu_probe(struct amba_device *adev, const struct amba_id *id)
>>   {
>>   	struct device *dev = &adev->dev;
>>   	struct tgu_drvdata *drvdata;
>> -	size_t priority_size, condition_size;
>> -	unsigned int *priority, *condition;
>> +	size_t priority_size, condition_size, select_size;
>> +	unsigned int *priority, *condition, *select;
>>   	int ret;
>>   
>>   	drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
>> @@ -417,6 +460,16 @@ static int tgu_probe(struct amba_device *adev, const struct amba_id *id)
>>   
>>   	drvdata->value_table->condition_decode = condition;
>>   
>> +	select_size = drvdata->max_condition_select * drvdata->max_step *
>> +		      sizeof(*(drvdata->value_table->condition_select));
>> +
>> +	select = devm_kzalloc(dev, select_size, GFP_KERNEL);
>> +
>> +	if (!select)
> 
> stray \n
Will improve this based on the comments above.
> 
>> +		return -ENOMEM;
>> +
>> +	drvdata->value_table->condition_select = select;
> 
> I don't see a need for an intemediate variable here

This was done intentionally, following the earlier suggestion in v9 to
introduce named intermediate variables for better readability when 
dealing with allocations.

I’m happy to inline the allocation if you prefer the simpler form here.

> 
> [...]
> 
>>    * @max_condition_decode: Maximum number of condition_decode
>> + * @max_condition_select: Maximum number of condition_select
> 
> Maximum value, perhaps? You haven't explained the feature very well
> so I'm not sure what this is supposed to reflect
Will add "Maximum value" description in cover letter.
> 
> Konrad