linux-kernel - Re: [PATCH v10 3/7] qcom-tgu: Add signal priority 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: <bcc3547c-b7d5-41a3-b291-4669c11d7f1c@oss.qualcomm.com>
Date: Tue, 27 Jan 2026 10:23:53 +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 3/7] qcom-tgu: Add signal priority support



On 1/13/2026 7:09 PM, Konrad Dybcio wrote:
> On 1/9/26 3:11 AM, Songwei Chai wrote:
>> Like circuit of a Logic analyzer, in TGU, the requirement could be
>> configured in each step and the trigger will be created once the
>> requirements are met. Add priority functionality here to sort the
>> signals into different priorities. The signal which is wanted could
>> be configured in each step's priority node, the larger number means
>> the higher priority and the signal with higher priority will be sensed
>> more preferentially.
>>
>> Signed-off-by: Songwei Chai <songwei.chai@....qualcomm.com>
>> ---
> 
> [...]
> 
>> +static ssize_t tgu_dataset_store(struct device *dev,
>> +				 struct device_attribute *attr,
>> +				 const char *buf, size_t size)
>> +{
>> +	struct tgu_drvdata *tgu_drvdata = dev_get_drvdata(dev);
>> +	struct tgu_attribute *tgu_attr =
>> +		container_of(attr, struct tgu_attribute, attr);
>> +	unsigned long val;
>> +	int index;
>> +
>> +	ret = kstrtoul(buf, 0, &val);
>> +	if (ret)
>> +		return ret;
>> +
>> +	guard(spinlock)(&tgu_drvdata->lock);
>> +	index = calculate_array_location(tgu_drvdata, tgu_attr->step_index,
>> +					 tgu_attr->operation_index,
>> +					 tgu_attr->reg_num);
>> +
>> +	tgu_drvdata->value_table->priority[index] = val;
>> +	return size;
> 
> Style: some functions have a \n before return, some don't. The former
> is preferred
Sure, Konrad.
I will update and pay more attention next time.
> 
>> +static umode_t tgu_node_visible(struct kobject *kobject,
>> +				struct attribute *attr,
>> +				int n)
>> +{
>> +	struct device *dev = kobj_to_dev(kobject);
>> +	struct tgu_drvdata *drvdata = dev_get_drvdata(dev);
>> +	struct device_attribute *dev_attr =
>> +		container_of(attr, struct device_attribute, attr);
>> +	struct tgu_attribute *tgu_attr =
>> +		container_of(dev_attr, struct tgu_attribute, attr);
>> +	int ret = SYSFS_GROUP_INVISIBLE;
>> +
>> +	if (tgu_attr->step_index < drvdata->max_step) {
>> +		ret = (tgu_attr->reg_num < drvdata->max_reg) ?
>> +			attr->mode : 0;
>> +	}
>> +	return ret;
> 
> This is very convoluted
> 
> How about:
> 
> if (tgu_attr->step_index >= drvdata->max_step)
> 	return 0;
> 
> if (tgu_attr->reg_num >= drvdata->max_reg)
> 	return 0;
> 
> return attr->mode;
> 
> ?
> 
> [...]
> 
I agree that the expanded form is clearer step-by-step, but I 
intentionally kept the current version as it keeps the bounds checks 
localized and avoids multiple early returns in this simple case.

I find the conditional expression still reasonably readable here, but 
I’m happy to revisit this if you feel strongly about the style.
>> +static void tgu_set_reg_number(struct tgu_drvdata *drvdata)
>> +{
>> +	int num_sense_input;
>> +	int num_reg;
>> +	u32 devid;
>> +
>> +	devid = readl(drvdata->base + TGU_DEVID);
>> +
>> +	num_sense_input = TGU_DEVID_SENSE_INPUT(devid);
>> +	if (((num_sense_input * NUMBER_BITS_EACH_SIGNAL) % LENGTH_REGISTER) == 0)
>> +		num_reg = (num_sense_input * NUMBER_BITS_EACH_SIGNAL) / LENGTH_REGISTER;
>> +	else
>> +		num_reg = ((num_sense_input * NUMBER_BITS_EACH_SIGNAL) / LENGTH_REGISTER) + 1;
> 
> num_reg = base case
> 
> if (num_sense_input * NUMBER_BITS_EACH_SIGNAL) % LENGTH_REGISTER)
> 	num_reg++;
> 
> ?
> 
Marked, will update.
> [...]
> 
>> @@ -112,6 +250,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;
>> +	unsigned int *priority;
> 
> reverse-Christmas-tree would be nice
> 
> 
Marked, will update.
>>   	int ret;
>>   
>>   	drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
>> @@ -127,12 +267,32 @@ static int tgu_probe(struct amba_device *adev, const struct amba_id *id)
>>   
>>   	spin_lock_init(&drvdata->lock);
>>   
>> +	tgu_set_reg_number(drvdata);
>> +	tgu_set_steps(drvdata);
>> +
>>   	ret = sysfs_create_groups(&dev->kobj, tgu_attr_groups);
>>   	if (ret) {
>>   		dev_err(dev, "failed to create sysfs groups: %d\n", ret);
>>   		return ret;
>>   	}
>>   
>> +	drvdata->value_table =
>> +		devm_kzalloc(dev, sizeof(*drvdata->value_table), GFP_KERNEL);
>> +	if (!drvdata->value_table)
>> +		return -ENOMEM;
>> +
>> +	priority_size = MAX_PRIORITY * drvdata->max_reg *
>> +			drvdata->max_step *
>> +			sizeof(*(drvdata->value_table->priority));
>> +
>> +	priority = devm_kzalloc(dev, priority_size, GFP_KERNEL);
>> +
>> +	if (!priority)
> 
> stray \n above
Marked, will update.
> 
> [...]
> 
>> +#define BMVAL(val, lsb, msb)	((val & GENMASK(msb, lsb)) >> lsb)
>> +#define TGU_DEVID_SENSE_INPUT(devid_val) ((int) BMVAL(devid_val, 10, 17))
>> +#define TGU_DEVID_STEPS(devid_val) ((int)BMVAL(devid_val, 3, 6))
>> +#define NUMBER_BITS_EACH_SIGNAL 4
> 
> "TGU_BITS_PER_SIGNAL"
Marked, will update.
> 
> Konrad