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

[William Breathitt Gray]

On Thu, Oct 28, 2021 at 08:42:49AM -0500, David Lechner wrote:
> On 10/28/21 2:48 AM, William Breathitt Gray wrote:
> > 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
> > 
> 
> I disagree. I consider this a crash. The system becomes completely
> unusable and you have to pull power to turn it off, potentially
> leading to data loss and disk corruption.

I hope I'm not being excessively pedantic here -- I'll concede to a
third opion on this if someone else wants to chime in -- but I want to
ensure that we are not going outside the scope of what a driver should
do. Note that any device is capable of flooding the system with
interrupts (e.g. a counter operating on a high enough frequency
overflowing a low ceiling), so I don't think that reason alone will pass
muster. Nevertheless, it is important to define when a driver should
return an error or not.

Take for example, the period range check right above. If a user requests
the device do something it cannot, such as counting down from a period
value that is too high for it to represent internally, then it is
appropriate to return an error: the device cannot perform the request
and as such the request is not valid input for the driver.

However, when we apply the same method to the zero value case -- a user
requests a timeout period of 0 -- the device is capable of performing
that request: the device is capable of waiting 0 nanoseconds and as such
the request is a valid input for the driver because it can be performed
by the device exactly as expected by the user. As long as the behavior
of a request is well-defined, we must assume the user knows what they
are doing, and thus should be permitted to request their device perform
that behavior.

A driver should not speculate on the intent of a user's actions.
Restricting what a user can do with their device is a matter of
configuration policy, and configuration policy belongs appropriately in
userspace. Rather, the scope of a driver should be limited narrowly to
exposure of a device functionality in a standard and predictable way.

William Breathitt Gray

Attachment: signature.asc
Description: PGP signature