Re: [PATCH 3/8] counter/ti-eqep: add support for unit timer

[William Breathitt Gray]

On Wed, Oct 27, 2021 at 10:28:59AM -0500, David Lechner wrote:
> On 10/25/21 3:48 AM, William Breathitt Gray wrote:
> > On Sat, Oct 16, 2021 at 08:33:38PM -0500, David Lechner wrote:
> >> This adds support to the TI eQEP counter driver for the Unit Timer.
> >> The Unit Timer is a device-level extension that provides a timer to be
> >> used for speed calculations. The sysfs interface for the Unit Timer is
> >> new and will be documented in a later commit. It contains a R/W time
> >> attribute for the current time, a R/W period attribute for the timeout
> >> period and a R/W enable attribute to start/stop the timer. It also
> >> implements a timeout event on the chrdev interface that is triggered
> >> each time the period timeout is reached.
> >>
> >> Signed-off-by: David Lechner <david@xxxxxxxxxxxxxx>
> > 
> > I'll comment on the sysfs interface in the respective docs patch. Some
> > comments regarding this patch below.
> > 
> 
> ...
> 
> >> +static int ti_eqep_unit_timer_period_write(struct counter_device *counter,
> >> +					   u64 value)
> >> +{
> >> +	struct ti_eqep_cnt *priv = counter->priv;
> >> +	u32 quprd;
> >> +
> >> +	/* convert nanoseconds to timer ticks */
> >> +	quprd = value = mul_u64_u32_div(value, priv->sysclkout_rate, NSEC_PER_SEC);
> >> +	if (quprd != value)
> >> +		return -ERANGE;
> >> +
> >> +	/* protect against infinite unit timeout interrupts */
> >> +	if (quprd == 0)
> >> +		return -EINVAL;
> > 
> > I doubt there's any practical reason for a user to set the timer period
> > to 0, but perhaps we should not try to protect users from themselves
> > here. It's very obvious and expected that setting the timer period to 0
> > results in timeouts as quickly as possible, so really the user should be
> > left to reap the fruits of their decision regardless of how asinine that
> > decision is.
> 
> Even if the operating system ceases operation because the interrupt
> handler keeps running because clearing the interrupt has no effect
> in this condition?

I don't disagree with you that configuring the device to repeatedly
timeout without pause would be a waste of system resources. However, it
is more appropriate for this protection to be located in a userspace
application rather than the driver code here.

The purpose of a driver is to expose the functionality of a device in an
understandable and consistent manner. Drivers should not dictate what a
user does with their device, but rather should help facilitate the
user's control so that the device behaves as would be expected given
such an interface.

For this particular case, the device is capable of sending an interrupt
when a timeout events occurs, and the timeout period can be adjusted;
setting the timeout period lower and lower results in less and less time
between timeout events. The behavior and result of setting the timeout
period lower is well-defined and predictable; it is intuitive that
configuring the timeout period to 0, the lowest value possible, results
in the shortest time possible between timeouts: no pause at all.

As long as the functionality of this device is exposed in such an
understandable and consistent manner, the driver succeeds in serving its
purpose. So I believe a timeout period of 0 is a valid configuration
for this driver to allow, albeit a seemingly pointless one for users to
actually choose. To that end, simply set the default value of QUPRD to
non-zero on probe() as you do already in this patch and let the user be
free to adjust if they so decide.

William Breathitt Gray

Attachment: signature.asc
Description: PGP signature