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

[David Lechner <david@lechnology.com>]

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@lechnology.com>
>>>
>>> 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.