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: <Z8awGBW8obpG1QPN@ishi>
Date: Tue, 4 Mar 2025 16:47:36 +0900
From: William Breathitt Gray <wbg@...nel.org>
To: Bence Csókás <csokas.bence@...lan.hu>
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

On Thu, Feb 27, 2025 at 03:40:20PM +0100, Bence Csókás wrote:
> +static int mchp_tc_count_cap_read(struct counter_device *counter,
> +				  struct counter_count *count, size_t idx, u64 *val)
> +{
> +	struct mchp_tc_data *const priv = counter_priv(counter);
> +	u32 cnt;
> +	int ret;
> +
> +	switch (idx) {
> +	case COUNTER_MCHP_EXCAP_RA:
> +		ret = regmap_read(priv->regmap, ATMEL_TC_REG(priv->channel[0], RA), &cnt);
> +		break;
> +	case COUNTER_MCHP_EXCAP_RB:
> +		ret = regmap_read(priv->regmap, ATMEL_TC_REG(priv->channel[0], RB), &cnt);
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	if (!ret)
> +		*val = cnt;
> +
> +	return ret;
> +}

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.

> diff --git a/include/uapi/linux/counter/microchip-tcb-capture.h b/include/uapi/linux/counter/microchip-tcb-capture.h
> index ee72f1463594..5c015fafe42c 100644
> --- a/include/uapi/linux/counter/microchip-tcb-capture.h
> +++ b/include/uapi/linux/counter/microchip-tcb-capture.h
> @@ -12,6 +12,9 @@
>   * Count 0
>   * \__  Synapse 0 -- Signal 0 (Channel A, i.e. TIOA)
>   * \__  Synapse 1 -- Signal 1 (Channel B, i.e. TIOB)
> + * \__  Extension capture0    (RA register)
> + * \__  Extension capture1    (RB register)
> + * \__  Extension capture2    (RC register)

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].

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.

William Breathitt Gray

[^1] https://lore.kernel.org/all/Z8alaOTjZeRuXnUI@ishi/

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)