Re: [PATCH v4 1/5] counter: Internalize sysfs interface code

[Jonathan Cameron]

On Mon, 3 Aug 2020 15:00:49 -0500
David Lechner <david@xxxxxxxxxxxxxx> wrote:

> On 8/2/20 4:04 PM, William Breathitt Gray wrote:
> > On Tue, Jul 28, 2020 at 05:45:53PM -0500, David Lechner wrote:  
> >> On 7/21/20 2:35 PM, William Breathitt Gray wrote:  
> >>> This is a reimplementation of the Generic Counter driver interface.  
> 
> ...
> 
> >>> -F:	include/linux/counter_enum.h
> >>> +F:	include/uapi/linux/counter.h  
> >>
> >> Seems odd to be introducing a uapi header here since this patch doesn't
> >> make any changes to userspace.  
> > 
> > These defines are needed by userspace for the character device
> > interface, but I see your point that at this point in the patchset they
> > don't need to be exposed yet.
> > 
> > I could create temporary include/linux/counter_types.h to house these
> > defines, and then later move them to include/uapi/linux/counter.h in the
> > character device interface introduction patch. Do you think I should do
> > so?  
> 
> Since this patch is independent of the chardev changes and probably ready
> to merge after one more round of review, I would say it probably makes
> sense to just leave them in counter.h for now and move them to uapi when
> the chardev interface is finalized. This way, we can just merge this patch
> as soon as it is ready.
> 
Agreed.

...

> >>>    /**
> >>>     * struct counter_device - Counter data structure
> >>> - * @name:		name of the device as it appears in the datasheet
> >>> + * @name:		name of the device
> >>>     * @parent:		optional parent device providing the counters
> >>> - * @device_state:	internal device state container
> >>> - * @ops:		callbacks from driver
> >>> + * @signal_read:	optional read callback for Signals. The read value of
> >>> + *			the respective Signal should be passed back via the
> >>> + *			value parameter.
> >>> + * @count_read:		optional read callback for Counts. The read value of the
> >>> + *			respective Count should be passed back via the value
> >>> + *			parameter.
> >>> + * @count_write:	optional write callback for Counts. The write value for
> >>> + *			the respective Count is passed in via the value
> >>> + *			parameter.
> >>> + * @function_read:	optional read callback the Count function modes. The
> >>> + *			read function mode of the respective Count should be
> >>> + *			passed back via the function parameter.
> >>> + * @function_write:	option write callback for Count function modes. The
> >>> + *			function mode to write for the respective Count is
> >>> + *			passed in via the function parameter.
> >>> + * @action_read:	optional read callback the Synapse action modes. The
> >>> + *			read action mode of the respective Synapse should be
> >>> + *			passed back via the action parameter.
> >>> + * @action_write:	option write callback for Synapse action modes. The
> >>> + *			action mode to write for the respective Synapse is
> >>> + *			passed in via the action parameter.
> >>>     * @signals:		array of Signals  
> >>
> >> Why not keep the ops struct?  
> > 
> > Defining static ops structures in the drivers seemed to have no
> > advantage when those callbacks are always used via the counter_device
> > structure. I decided it'd be simpler to just set them directly in the
> > counter_device structure then.
> > 
> > I could reorganize them into an ops structure again if there's enough
> > interest.  
> 
> I've been working on really constrained systems lately where every byte
> counts, so this stuck out to me since there would be a copy of all
> functions for each counter instance. But probably not that big of a deal
> in the Linux kernel. :-)
> 
In addition to that..

There are other advantages to keeping an ops structure including
easy function order randomization (for security), plus
the fact that we want to make any function pointers build time assignments
if we possibly can.  Makes them harder to attack.

So in more recent kernel code we try to use ops structures wherever possible.

Jonathan