Re: [PATCH v5 3/5] counter: Add character device interface

[William Breathitt Gray]

On Tue, Oct 20, 2020 at 10:53:32AM -0500, David Lechner wrote:
> >>> +
> >>> +static int counter_chrdev_release(struct inode *inode, struct file *filp)
> >>> +{
> >>> +	struct counter_device *const counter = filp->private_data;
> >>> +	unsigned long flags;
> >>> +
> >>> +	put_device(&counter->dev);
> >>
> >> put_device() should be at the end of the function in case it is the last
> >> reference.
> > 
> > put_device() shouldn't affect the counter_device events members, so I
> > don't think there's a difference in this case if it's called at the
> > beginning or end of the counter_chrdev_release function.
> > 
> 
> It isn't possible the some memory allocated with devm_kalloc() could be
> be referenced after calling put_device() now or in the future?

You're right, if put_device() is called before then we could end up in a
garbage state where the device memory is released but events list has
not yet been freed. I'll move put_device() to after the events list is
freed then.

> >>> diff --git a/drivers/counter/counter-sysfs.c b/drivers/counter/counter-sysfs.c
> >>> index e66ed99dd5ea..cefef61f170d 100644
> >>> --- a/drivers/counter/counter-sysfs.c
> >>> +++ b/drivers/counter/counter-sysfs.c
> >>
> >>
> >> Not sure why sysfs changes are in the chrdev patch. Are these
> >> changes related somehow?
> > 
> > Sorry, I forgot to explain this in the cover letter. The changes here
> > are only useful for the character device interface. These changes
> > introduce the extensionZ_name and extensionZ_width sysfs attributes.
> > 
> > In the character device interface, extensions are selected by their id
> > number, and the value returned depends on the type of data. The new
> > sysfs attributes introduced here allow users to match the id of an
> > extension with its name, as well as the bit width of the value returned
> > so that the user knows whether to use the value_u8 or value_u64 union
> > member in struct counter_event.
> > 
> 
> Are we sure that all value types will always be CPU-endian unsigned
> integers? Or should we make an enum to describe the data type instead
> of just the width?

It should be safe to assume that the character device interface will
only ever return CPU-endian unsigned integers. The device driver should
handle the conversion of any strange endianness from the device before
the character device interface, while userspace is the one responsible
for interpreting the meaning of count in the context of the application.

Let's create a scenario for the sake of example. Suppose we want to use
a counter device to track the vertical position of a component moved by
a linear actuator. The operator considers some vertical position as the
horizon, where anything above would be a positive position and anything
below a negative position. The counter device stores its count in
big-endian format; but the system CPU expects little-endian.

The flow of data for this scenario would look like the following (where
BE = big-endian, LE = little-endian):

+----------+         +---------------+          +--------+
| Raw data | - BE -> | Device driver | -> LE -> | chrdev | - u64 ->
+----------+         +---------------+          +--------+

At this point, the userspace application would read the unsigned integer
from the character device and determine how to interpret the position --
whether the count be converted to a signed value to represent a negative
physical position.

Whether or not a position should be considered negative is dependent on
the user application and context. Because the character device does not
know the context of the user application, it should only provide
unsigned integers in order to ensure a standard interface for counter
devices; userspace will be responsible for interpreting those counts to
something meaningful for the context of their applications.

William Breathitt Gray

Attachment: signature.asc
Description: PGP signature