Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Sat, 30 Oct 2021 17:35:46 +0900
From:   William Breathitt Gray <vilhelm.gray@gmail.com>
To:     David Lechner <david@lechnology.com>
Cc:     linux-iio@vger.kernel.org, Robert Nelson <robertcnelson@gmail.com>,
        linux-kernel@vger.kernel.org, jic23@kernel.org
Subject: Re: [PATCH 3/8] counter/ti-eqep: add support for unit timer
Message-ID: <YX0D4j5B++G3QTj1@shinobu>
References: <20211017013343.3385923-1-david@lechnology.com>
 <20211017013343.3385923-4-david@lechnology.com>
 <YXZvQSU6bRRaWD89@shinobu>
 <253916e2-a808-8786-ac72-60a1a62b1531@lechnology.com>
 <YXpVyjnrrmRbpHJU@shinobu>
 <f5e40a22-3c7f-4d4d-d160-fe5b5a7dd72e@lechnology.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="iAECRAllJz4Bp4cu"
Content-Disposition: inline
In-Reply-To: <f5e40a22-3c7f-4d4d-d160-fe5b5a7dd72e@lechnology.com>
Precedence: bulk


--iAECRAllJz4Bp4cu
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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 timeo=
ut
> >>>> 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 *c=
ounter,
> >>>> +					   u64 value)
> >>>> +{
> >>>> +	struct ti_eqep_cnt *priv =3D counter->priv;
> >>>> +	u32 quprd;
> >>>> +
> >>>> +	/* convert nanoseconds to timer ticks */
> >>>> +	quprd =3D value =3D mul_u64_u32_div(value, priv->sysclkout_rate, N=
SEC_PER_SEC);
> >>>> +	if (quprd !=3D value)
> >>>> +		return -ERANGE;
> >>>> +
> >>>> +	/* protect against infinite unit timeout interrupts */
> >>>> +	if (quprd =3D=3D 0)
> >>>> +		return -EINVAL;
> >>>
> >>> I doubt there's any practical reason for a user to set the timer peri=
od
> >>> 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 t=
hat
> >>> 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?
> >=20
> > 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.
> >=20
> > 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.
> >=20
> > 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.
> >=20
> > 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.
> >=20
> > William Breathitt Gray
> >=20
>=20
> 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

--iAECRAllJz4Bp4cu
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEk5I4PDJ2w1cDf/bghvpINdm7VJIFAmF9A9QACgkQhvpINdm7
VJIPyw/+KKWpbFVnxxqNc75d7n80BlKyJDwICE1wa0pCJKB3FiD3L7yFTrX+pjtl
gwHNdRZ5v1FwUoPxTAiqVzDuPDHZQq44wzCspznvL+jcsOFZPqqos8+qQ5xugEIp
pbf70AGh0QIru6IAq1RH9NxkR8b0pryiJPTNdIPz/4YTLI7c7q8IV6GqN9GjaVtg
fJvKemuP8nS5adaTCMzxE60Uu9CHMFjhKQDfs0WMua7XCvZBhH/uus8ghwtmnlHF
ISFujrnMn6dFInDfL4ZMv0L82l6P6tBh3tUpB14u6Tc3e3b3u/FjVx7VU7pmja5J
KczI/5Lo0gboX+VfJYkEMmPI3rhZwelNwCprdcFgvz3viHQT9KZgf/jEZdU0PmUg
uatEmBzV7GWBnXfSdZNRDQcV5ifPBGNLOW7KspIW8vCeIPktFCd/927qYQkkE6Lo
xojpvS1MVu69qNg57zSwVSxtkwYnrH/3XcjN9qHz26ZqgZSmlntxTnhFmyR0rFhg
GNj8u6iBlDxbHffbsGnOJBWKLMSRwY53dY079KXCITrGm3MakSOoJO9oJycVihvC
ZrV2dqnOc8jd86XmbcwRsSQSfwTxmTm6gY6XqF77tfoR0DFT6qAA/s+mlFALTwHU
sQPwZRHC+FpZfb+pT46Ov+pEkY6AzBaXoMXxrqtL3HbNACOrpfg=
=KZ1I
-----END PGP SIGNATURE-----

--iAECRAllJz4Bp4cu--