linux-kernel - Re: [PATCH v6 3/3] counter: microchip-tcb-capture: Add capture extensions for registers RA/RB

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: <1604dce5-7be6-4a95-a51c-0c760a6c9a76@prolan.hu>
Date: Tue, 4 Mar 2025 11:03:17 +0100
From: Csókás Bence <csokas.bence@...lan.hu>
To: William Breathitt Gray <wbg@...nel.org>
CC: <linux-arm-kernel@...ts.infradead.org>, <linux-iio@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, Kamel Bouhara <kamel.bouhara@...tlin.com>
Subject: Re: [PATCH v6 3/3] counter: microchip-tcb-capture: Add capture
 extensions for registers RA/RB

Hi,

On 2025. 03. 04. 8:47, William Breathitt Gray wrote:
> It's cleaner to exit early on an error than to carry it to the end.
> Instead of if (!ret), perform an if (ret) return ret to exit early on an
> error, then simply return 0 at the end of the funtion.

Ok.

> The capture2 extension doesn't exist in this patch so remove this
> comment line.
> 
>> @@ -30,6 +33,12 @@ enum counter_mchp_signals {
>>   	COUNTER_MCHP_SIG_TIOB,
>>   };
>>   
>> +enum counter_mchp_capture_extensions {
>> +	COUNTER_MCHP_EXCAP_RA,
>> +	COUNTER_MCHP_EXCAP_RB,
>> +	COUNTER_MCHP_EXCAP_RC,
>> +};
> 
> Remove COUNTER_MCHP_EXCAP_RC for the same reason as above.
> 
> Also, I would argue for these to be preprocessor defines rather than
> enum for the same reasons as in my other review[^1].

Ok.

> One final comment: is RA/RB the best way to differentiate these? One of
> the benefits of abstraction layers is that users won't need to be
> concerned about the hardware details, and naming the capture values
> after their respective general register hardware names feels somewhat
> antithetic to that end.
> 
> I imagine there are better ways to refer to these that would communicate
> their relationship better, such as "primary capture" and "secondary
> capture". However at that point capture0 and capture1 would seem
> obvious enough, in which case you might not even need to expose these to
> userspace at all.

Hmm. Well, RA and RB is what it says in the datasheet, and since we 
don't do much processing on their value, I'd say we're still closely 
coupled to the hardware. So, if one wants to understand what they do, 
they will have to read the datasheet anyways in which case I think it's 
best to be consistent with it naming-wise.

> William Breathitt Gray
> 
> [^1] https://lore.kernel.org/all/Z8alaOTjZeRuXnUI@ishi/

Bence