From: Jonathan Cameron <jic23@kernel.org> To: David Lechner <david@lechnology.com> Cc: William Breathitt Gray <vilhelm.gray@gmail.com>, kamel.bouhara@bootlin.com, gwendal@chromium.org, alexandre.belloni@bootlin.com, linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org, linux-stm32@st-md-mailman.stormreply.com, linux-arm-kernel@lists.infradead.org, syednwaris@gmail.com, patrick.havelange@essensium.com, fabrice.gasnier@st.com, mcoquelin.stm32@gmail.com, alexandre.torgue@st.com, David.Laight@ACULAB.COM Subject: Re: [PATCH v4 1/5] counter: Internalize sysfs interface code Date: Sun, 9 Aug 2020 14:42:21 +0100 [thread overview] Message-ID: <20200809144221.6947ea6e@archlinux> (raw) In-Reply-To: <4061c9e4-775e-b7a6-14fa-446de4fae537@lechnology.com> On Mon, 3 Aug 2020 15:00:49 -0500 David Lechner <david@lechnology.com> 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
WARNING: multiple messages have this Message-ID (diff)
From: Jonathan Cameron <jic23@kernel.org> To: David Lechner <david@lechnology.com> Cc: kamel.bouhara@bootlin.com, gwendal@chromium.org, alexandre.torgue@st.com, alexandre.belloni@bootlin.com, linux-iio@vger.kernel.org, patrick.havelange@essensium.com, William Breathitt Gray <vilhelm.gray@gmail.com>, linux-kernel@vger.kernel.org, David.Laight@ACULAB.COM, mcoquelin.stm32@gmail.com, fabrice.gasnier@st.com, syednwaris@gmail.com, linux-stm32@st-md-mailman.stormreply.com, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH v4 1/5] counter: Internalize sysfs interface code Date: Sun, 9 Aug 2020 14:42:21 +0100 [thread overview] Message-ID: <20200809144221.6947ea6e@archlinux> (raw) In-Reply-To: <4061c9e4-775e-b7a6-14fa-446de4fae537@lechnology.com> On Mon, 3 Aug 2020 15:00:49 -0500 David Lechner <david@lechnology.com> 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2020-08-09 13:42 UTC|newest] Thread overview: 43+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-07-21 19:35 [PATCH v4 0/5] Introduce the Counter character device interface William Breathitt Gray 2020-07-21 19:35 ` William Breathitt Gray 2020-07-21 19:35 ` [PATCH v4 1/5] counter: Internalize sysfs interface code William Breathitt Gray 2020-07-28 22:45 ` David Lechner 2020-08-02 21:04 ` William Breathitt Gray 2020-08-03 20:00 ` David Lechner 2020-08-03 20:00 ` David Lechner 2020-08-09 13:42 ` Jonathan Cameron [this message] 2020-08-09 13:42 ` Jonathan Cameron 2020-08-09 19:15 ` William Breathitt Gray 2020-08-09 19:15 ` William Breathitt Gray 2020-08-10 22:48 ` David Lechner 2020-08-10 22:48 ` David Lechner 2020-08-15 16:33 ` William Breathitt Gray 2020-08-15 16:33 ` William Breathitt Gray 2020-07-21 19:35 ` [PATCH v4 2/5] docs: counter: Update to reflect sysfs internalization William Breathitt Gray 2020-07-21 19:35 ` William Breathitt Gray 2020-07-21 19:35 ` [PATCH v4 3/5] counter: Add character device interface William Breathitt Gray 2020-07-21 19:35 ` William Breathitt Gray 2020-07-29 0:20 ` David Lechner 2020-07-29 0:20 ` David Lechner 2020-07-30 22:49 ` David Lechner 2020-07-30 22:49 ` David Lechner 2020-07-31 8:13 ` Alexandre Belloni 2020-07-31 8:13 ` Alexandre Belloni 2020-08-02 21:11 ` William Breathitt Gray 2020-08-02 21:11 ` William Breathitt Gray 2020-08-09 14:51 ` William Breathitt Gray 2020-08-09 14:51 ` William Breathitt Gray 2020-08-10 23:02 ` David Lechner 2020-08-10 23:02 ` David Lechner 2020-08-15 17:23 ` William Breathitt Gray 2020-08-15 17:23 ` William Breathitt Gray 2020-07-21 19:35 ` [PATCH v4 4/5] docs: counter: Document " William Breathitt Gray 2020-07-21 19:35 ` William Breathitt Gray 2020-07-21 19:35 ` [PATCH v4 5/5] counter: 104-quad-8: Add IRQ support for the ACCES 104-QUAD-8 William Breathitt Gray 2020-07-21 19:35 ` William Breathitt Gray 2020-07-30 17:07 ` Syed Nayyar Waris 2020-07-30 17:07 ` Syed Nayyar Waris 2020-08-09 13:48 ` [PATCH v4 0/5] Introduce the Counter character device interface Jonathan Cameron 2020-08-09 13:48 ` Jonathan Cameron 2020-08-09 17:51 ` William Breathitt Gray 2020-08-09 17:51 ` William Breathitt Gray
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20200809144221.6947ea6e@archlinux \ --to=jic23@kernel.org \ --cc=David.Laight@ACULAB.COM \ --cc=alexandre.belloni@bootlin.com \ --cc=alexandre.torgue@st.com \ --cc=david@lechnology.com \ --cc=fabrice.gasnier@st.com \ --cc=gwendal@chromium.org \ --cc=kamel.bouhara@bootlin.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-iio@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-stm32@st-md-mailman.stormreply.com \ --cc=mcoquelin.stm32@gmail.com \ --cc=patrick.havelange@essensium.com \ --cc=syednwaris@gmail.com \ --cc=vilhelm.gray@gmail.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.