Re: [PATCH v9 4/5] counter: Add Renesas RZ/G2L MTU3a counter driver

[William Breathitt Gray <william.gray@linaro.org>]

[-- Attachment #1: Type: text/plain, Size: 2460 bytes --]

On Wed, Dec 14, 2022 at 10:31:35AM +0000, Biju Das wrote:
> Add RZ/G2L MTU3a counter driver. This IP supports the following
> phase counting modes on MTU1 and MTU2 channels
> 
> 1) 16-bit phase counting modes on MTU1 and MTU2 channels.
> 2) 32-bit phase counting mode by cascading MTU1 and MTU2 channels.
> 
> This patch adds 3 counter value channels.
> 	count0: 16-bit phase counter value channel on MTU1
> 	count1: 16-bit phase counter value channel on MTU2
> 	count2: 32-bit phase counter value channel by cascading
>                 MTU1 and MTU2 channels.
> 
> The external input phase clock pin for the counter value channels
> are as follows:
> 	count0: "MTCLKA-MTCLKB"
> 	count1: "MTCLKA-MTCLKB" or "MTCLKC-MTCLKD"
> 	count2: "MTCLKA-MTCLKB" or "MTCLKC-MTCLKD"
> 
> Use the sysfs variable "external_input_phase_clock_select" to select the
> external input phase clock pin and "cascade_counts_enable" to enable/
> disable cascading of channels.
> 
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>

Hello Biju,

Do you need to take the ch->lock before checking ch->is_busy to ensure
it does not change?

Regardless, I have some race comments below.

> +static int rz_mtu3_count_function_read(struct counter_device *counter,
> +				       struct counter_count *count,
> +				       enum counter_function *function)
> +{
> +	struct rz_mtu3_channel *const ch = rz_mtu3_get_ch(counter, count->id);
> +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> +	u8 timer_mode;
> +
> +	if (ch->is_busy && !priv->count_is_enabled[count->id])
> +		return -EINVAL;

The priv->lock must be taken because count_is_enabled could change
after it's checked here.

However, you'll need to spin up a helper function because you're
currently calling rz_mtu3_count_function_read() in
rz_mtu3_action_read(). So move the implementation of this function to a
new helper function and call that here with the appropriate locks.

> +static int rz_mtu3_count_direction_read(struct counter_device *counter,
> +					struct counter_count *count,
> +					enum counter_count_direction *direction)
> +{
> +	struct rz_mtu3_channel *const ch = rz_mtu3_get_ch(counter, count->id);
> +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> +	u8 tsr;
> +
> +	if (ch->is_busy && !priv->count_is_enabled[count->id])
> +		return -EINVAL;

This needs to be locked for the same reason as above.

William Breathitt Gray

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]