Hi William Breathitt Gray,
> Subject: Re: [PATCH v9 4/5] counter: Add Renesas RZ/G2L MTU3a counter driver
>
> 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@xxxxxxxxxxxxxx>
>
> Hello Biju,
>
> Do you need to take the ch->lock before checking ch->is_busy to ensure it
> does not change?
priv->count_is_enabled[count->id]-> true means channel is held by counter.
So pwm won't be able to change the state ch->is_busy.
priv->count_is_enabled[count->id]-> false and if there is contention for ch->busy
whoever is first calling rz_mtu3_request_channel() will get the channel.
among pwm_request and counter_enable.
So I think it is safe here. Please correct me if I am missing something.
static inline bool rz_mtu3_request_channel(struct rz_mtu3_channel *ch)
{
bool is_idle;
mutex_lock(&ch->lock);
is_idle = !ch->is_busy;
if (is_idle)
ch->is_busy = true;
mutex_unlock(&ch->lock);
return is_idle;
}
>
> 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 above check is for avoiding race between pwm and counter.
> The priv->lock must be taken because count_is_enabled could change after
> it's checked here.
OK, Will add priv->lock.
>
> 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.
OK. Will add helper function.
>
> > +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.
Agreed.
Cheers,
Biju
