| Message ID | 20211215151344.163036-9-miquel.raynal@bootlin.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Light core IIO enhancements | expand |
On Wed, Dec 15, 2021 at 10:03 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > This entry should, under no situation, be modified by device > drivers. Now that we have limited its read access to device drivers > really needing it, and handled modifications through another dedicated > helper within the core, we can easily move this variable to the opaque > structure as well as the write helper in order to prevent device driver > from playing with it. > Ah, the set-helper becomes inline. The helpers make more sense now. [ regardless of the naming of the helpers] Reviewed-by: Alexandru Ardelean <ardeleanalex@gmail.com> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > --- > drivers/iio/industrialio-core.c | 15 +++------------ > include/linux/iio/iio-opaque.h | 16 ++++++++++++++++ > include/linux/iio/iio.h | 5 ----- > 3 files changed, 19 insertions(+), 17 deletions(-) > > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c > index 5c7e0c9e1f59..9836a57ff151 100644 > --- a/drivers/iio/industrialio-core.c > +++ b/drivers/iio/industrialio-core.c > @@ -2064,21 +2064,12 @@ EXPORT_SYMBOL_GPL(iio_device_release_direct_mode); > **/ > int iio_get_internal_mode(struct iio_dev *indio_dev) > { > - return indio_dev->currentmode; > + struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev); > + > + return iio_dev_opaque->currentmode; > } > EXPORT_SYMBOL_GPL(iio_get_internal_mode); > > -/** > - * iio_set_internal_mode() - helper function providing write-only access to the > - * internal @currentmode variable > - * @indio_dev: IIO device structure for device > - **/ > -void iio_set_internal_mode(struct iio_dev *indio_dev, int mode) > -{ > - indio_dev->currentmode = mode; > -} > -EXPORT_SYMBOL_GPL(iio_set_internal_mode); > - > subsys_initcall(iio_init); > module_exit(iio_exit); > > diff --git a/include/linux/iio/iio-opaque.h b/include/linux/iio/iio-opaque.h > index 2be12b7b5dc5..72f35270a387 100644 > --- a/include/linux/iio/iio-opaque.h > +++ b/include/linux/iio/iio-opaque.h > @@ -7,6 +7,9 @@ > * struct iio_dev_opaque - industrial I/O device opaque information > * @indio_dev: public industrial I/O device information > * @id: used to identify device internally > + * @currentmode: operating mode currently in use, may be eventually > + * checked by device drivers but should be considered > + * read-only as this is a core internal bit > * @driver_module: used to make it harder to undercut users > * @info_exist_lock: lock to prevent use during removal > * @trig_readonly: mark the current trigger immutable > @@ -36,6 +39,7 @@ > */ > struct iio_dev_opaque { > struct iio_dev indio_dev; > + int currentmode; > int id; > struct module *driver_module; > struct mutex info_exist_lock; > @@ -71,4 +75,16 @@ struct iio_dev_opaque { > #define to_iio_dev_opaque(_indio_dev) \ > container_of((_indio_dev), struct iio_dev_opaque, indio_dev) > > +/** > + * iio_set_internal_mode() - helper function providing write-only access to the > + * internal @currentmode variable > + * @indio_dev: IIO device structure for device > + **/ > +static inline void iio_set_internal_mode(struct iio_dev *indio_dev, int mode) > +{ > + struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev); > + > + iio_dev_opaque->currentmode = mode; > +} > + > #endif > diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h > index 27551d251904..d04ab89fa0c2 100644 > --- a/include/linux/iio/iio.h > +++ b/include/linux/iio/iio.h > @@ -492,9 +492,6 @@ struct iio_buffer_setup_ops { > * registering the IIO device and can be filed up by the > * IIO core depending on the features advertised by the > * driver during other steps of the registration > - * @currentmode: [INTERN] operating mode currently in use, may be > - * eventually checked by device drivers but should be > - * considered read-only as this is a core internal bit > * @dev: [DRIVER] device structure, should be assigned a parent > * and owner > * @buffer: [DRIVER] any buffer present > @@ -521,7 +518,6 @@ struct iio_buffer_setup_ops { > */ > struct iio_dev { > int modes; > - int currentmode; > struct device dev; > > struct iio_buffer *buffer; > @@ -679,7 +675,6 @@ struct iio_trigger *devm_iio_trigger_alloc(struct device *parent, > const char *fmt, ...); > > int iio_get_internal_mode(struct iio_dev *indio_dev); > -void iio_set_internal_mode(struct iio_dev *indio_dev, int mode); > > /** > * iio_buffer_enabled() - helper function to test if the buffer is enabled > -- > 2.27.0 >
Hi Alexandru, ardeleanalex@gmail.com wrote on Thu, 16 Dec 2021 09:48:36 +0200: > On Wed, Dec 15, 2021 at 10:03 PM Miquel Raynal > <miquel.raynal@bootlin.com> wrote: > > > > This entry should, under no situation, be modified by device > > drivers. Now that we have limited its read access to device drivers > > really needing it, and handled modifications through another dedicated > > helper within the core, we can easily move this variable to the opaque > > structure as well as the write helper in order to prevent device driver > > from playing with it. > > > > Ah, the set-helper becomes inline. > > The helpers make more sense now. > > [ regardless of the naming of the helpers] > Reviewed-by: Alexandru Ardelean <ardeleanalex@gmail.com> Great! Thanks for the entire review and feedback! > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > --- > > drivers/iio/industrialio-core.c | 15 +++------------ > > include/linux/iio/iio-opaque.h | 16 ++++++++++++++++ > > include/linux/iio/iio.h | 5 ----- > > 3 files changed, 19 insertions(+), 17 deletions(-) > > > > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c > > index 5c7e0c9e1f59..9836a57ff151 100644 > > --- a/drivers/iio/industrialio-core.c > > +++ b/drivers/iio/industrialio-core.c > > @@ -2064,21 +2064,12 @@ EXPORT_SYMBOL_GPL(iio_device_release_direct_mode); > > **/ > > int iio_get_internal_mode(struct iio_dev *indio_dev) > > { > > - return indio_dev->currentmode; > > + struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev); > > + > > + return iio_dev_opaque->currentmode; > > } > > EXPORT_SYMBOL_GPL(iio_get_internal_mode); > > > > -/** > > - * iio_set_internal_mode() - helper function providing write-only access to the > > - * internal @currentmode variable > > - * @indio_dev: IIO device structure for device > > - **/ > > -void iio_set_internal_mode(struct iio_dev *indio_dev, int mode) > > -{ > > - indio_dev->currentmode = mode; > > -} > > -EXPORT_SYMBOL_GPL(iio_set_internal_mode); > > - > > subsys_initcall(iio_init); > > module_exit(iio_exit); > > > > diff --git a/include/linux/iio/iio-opaque.h b/include/linux/iio/iio-opaque.h > > index 2be12b7b5dc5..72f35270a387 100644 > > --- a/include/linux/iio/iio-opaque.h > > +++ b/include/linux/iio/iio-opaque.h > > @@ -7,6 +7,9 @@ > > * struct iio_dev_opaque - industrial I/O device opaque information > > * @indio_dev: public industrial I/O device information > > * @id: used to identify device internally > > + * @currentmode: operating mode currently in use, may be eventually > > + * checked by device drivers but should be considered > > + * read-only as this is a core internal bit > > * @driver_module: used to make it harder to undercut users > > * @info_exist_lock: lock to prevent use during removal > > * @trig_readonly: mark the current trigger immutable > > @@ -36,6 +39,7 @@ > > */ > > struct iio_dev_opaque { > > struct iio_dev indio_dev; > > + int currentmode; > > int id; > > struct module *driver_module; > > struct mutex info_exist_lock; > > @@ -71,4 +75,16 @@ struct iio_dev_opaque { > > #define to_iio_dev_opaque(_indio_dev) \ > > container_of((_indio_dev), struct iio_dev_opaque, indio_dev) > > > > +/** > > + * iio_set_internal_mode() - helper function providing write-only access to the > > + * internal @currentmode variable > > + * @indio_dev: IIO device structure for device > > + **/ > > +static inline void iio_set_internal_mode(struct iio_dev *indio_dev, int mode) > > +{ > > + struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev); > > + > > + iio_dev_opaque->currentmode = mode; > > +} > > + > > #endif > > diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h > > index 27551d251904..d04ab89fa0c2 100644 > > --- a/include/linux/iio/iio.h > > +++ b/include/linux/iio/iio.h > > @@ -492,9 +492,6 @@ struct iio_buffer_setup_ops { > > * registering the IIO device and can be filed up by the > > * IIO core depending on the features advertised by the > > * driver during other steps of the registration > > - * @currentmode: [INTERN] operating mode currently in use, may be > > - * eventually checked by device drivers but should be > > - * considered read-only as this is a core internal bit > > * @dev: [DRIVER] device structure, should be assigned a parent > > * and owner > > * @buffer: [DRIVER] any buffer present > > @@ -521,7 +518,6 @@ struct iio_buffer_setup_ops { > > */ > > struct iio_dev { > > int modes; > > - int currentmode; > > struct device dev; > > > > struct iio_buffer *buffer; > > @@ -679,7 +675,6 @@ struct iio_trigger *devm_iio_trigger_alloc(struct device *parent, > > const char *fmt, ...); > > > > int iio_get_internal_mode(struct iio_dev *indio_dev); > > -void iio_set_internal_mode(struct iio_dev *indio_dev, int mode); > > > > /** > > * iio_buffer_enabled() - helper function to test if the buffer is enabled > > -- > > 2.27.0 > > Thanks, Miquèl
On Thu, 16 Dec 2021 09:38:48 +0100 Miquel Raynal <miquel.raynal@bootlin.com> wrote: > Hi Alexandru, > > ardeleanalex@gmail.com wrote on Thu, 16 Dec 2021 09:48:36 +0200: > > > On Wed, Dec 15, 2021 at 10:03 PM Miquel Raynal > > <miquel.raynal@bootlin.com> wrote: > > > > > > This entry should, under no situation, be modified by device > > > drivers. Now that we have limited its read access to device drivers > > > really needing it, and handled modifications through another dedicated > > > helper within the core, we can easily move this variable to the opaque > > > structure as well as the write helper in order to prevent device driver > > > from playing with it. > > > > > > > Ah, the set-helper becomes inline. > > > > The helpers make more sense now. Ah. I didn't read ahead either. I'm still unconvinced that it serves any useful purpose though. I'm entirely in favour of moving the variable but direct access is fine form the core code. We have similar cases for iio_dev_opaque->id for example where we provide a read only accessor for drivers and directly use it in the core. > > > > [ regardless of the naming of the helpers] > > Reviewed-by: Alexandru Ardelean <ardeleanalex@gmail.com> > > Great! Thanks for the entire review and feedback! > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > > --- > > > drivers/iio/industrialio-core.c | 15 +++------------ > > > include/linux/iio/iio-opaque.h | 16 ++++++++++++++++ > > > include/linux/iio/iio.h | 5 ----- > > > 3 files changed, 19 insertions(+), 17 deletions(-) > > > > > > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c > > > index 5c7e0c9e1f59..9836a57ff151 100644 > > > --- a/drivers/iio/industrialio-core.c > > > +++ b/drivers/iio/industrialio-core.c > > > @@ -2064,21 +2064,12 @@ EXPORT_SYMBOL_GPL(iio_device_release_direct_mode); > > > **/ > > > int iio_get_internal_mode(struct iio_dev *indio_dev) > > > { > > > - return indio_dev->currentmode; > > > + struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev); > > > + > > > + return iio_dev_opaque->currentmode; > > > } > > > EXPORT_SYMBOL_GPL(iio_get_internal_mode); > > > > > > -/** > > > - * iio_set_internal_mode() - helper function providing write-only access to the > > > - * internal @currentmode variable > > > - * @indio_dev: IIO device structure for device > > > - **/ > > > -void iio_set_internal_mode(struct iio_dev *indio_dev, int mode) > > > -{ > > > - indio_dev->currentmode = mode; > > > -} > > > -EXPORT_SYMBOL_GPL(iio_set_internal_mode); > > > - > > > subsys_initcall(iio_init); > > > module_exit(iio_exit); > > > > > > diff --git a/include/linux/iio/iio-opaque.h b/include/linux/iio/iio-opaque.h > > > index 2be12b7b5dc5..72f35270a387 100644 > > > --- a/include/linux/iio/iio-opaque.h > > > +++ b/include/linux/iio/iio-opaque.h > > > @@ -7,6 +7,9 @@ > > > * struct iio_dev_opaque - industrial I/O device opaque information > > > * @indio_dev: public industrial I/O device information > > > * @id: used to identify device internally > > > + * @currentmode: operating mode currently in use, may be eventually > > > + * checked by device drivers but should be considered > > > + * read-only as this is a core internal bit > > > * @driver_module: used to make it harder to undercut users > > > * @info_exist_lock: lock to prevent use during removal > > > * @trig_readonly: mark the current trigger immutable > > > @@ -36,6 +39,7 @@ > > > */ > > > struct iio_dev_opaque { > > > struct iio_dev indio_dev; > > > + int currentmode; > > > int id; > > > struct module *driver_module; > > > struct mutex info_exist_lock; > > > @@ -71,4 +75,16 @@ struct iio_dev_opaque { > > > #define to_iio_dev_opaque(_indio_dev) \ > > > container_of((_indio_dev), struct iio_dev_opaque, indio_dev) > > > > > > +/** > > > + * iio_set_internal_mode() - helper function providing write-only access to the > > > + * internal @currentmode variable > > > + * @indio_dev: IIO device structure for device > > > + **/ > > > +static inline void iio_set_internal_mode(struct iio_dev *indio_dev, int mode) > > > +{ > > > + struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev); > > > + > > > + iio_dev_opaque->currentmode = mode; > > > +} > > > + > > > #endif > > > diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h > > > index 27551d251904..d04ab89fa0c2 100644 > > > --- a/include/linux/iio/iio.h > > > +++ b/include/linux/iio/iio.h > > > @@ -492,9 +492,6 @@ struct iio_buffer_setup_ops { > > > * registering the IIO device and can be filed up by the > > > * IIO core depending on the features advertised by the > > > * driver during other steps of the registration > > > - * @currentmode: [INTERN] operating mode currently in use, may be > > > - * eventually checked by device drivers but should be > > > - * considered read-only as this is a core internal bit > > > * @dev: [DRIVER] device structure, should be assigned a parent > > > * and owner > > > * @buffer: [DRIVER] any buffer present > > > @@ -521,7 +518,6 @@ struct iio_buffer_setup_ops { > > > */ > > > struct iio_dev { > > > int modes; > > > - int currentmode; > > > struct device dev; > > > > > > struct iio_buffer *buffer; > > > @@ -679,7 +675,6 @@ struct iio_trigger *devm_iio_trigger_alloc(struct device *parent, > > > const char *fmt, ...); > > > > > > int iio_get_internal_mode(struct iio_dev *indio_dev); > > > -void iio_set_internal_mode(struct iio_dev *indio_dev, int mode); > > > > > > /** > > > * iio_buffer_enabled() - helper function to test if the buffer is enabled > > > -- > > > 2.27.0 > > > > > Thanks, > Miquèl
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c index 5c7e0c9e1f59..9836a57ff151 100644 --- a/drivers/iio/industrialio-core.c +++ b/drivers/iio/industrialio-core.c @@ -2064,21 +2064,12 @@ EXPORT_SYMBOL_GPL(iio_device_release_direct_mode); **/ int iio_get_internal_mode(struct iio_dev *indio_dev) { - return indio_dev->currentmode; + struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev); + + return iio_dev_opaque->currentmode; } EXPORT_SYMBOL_GPL(iio_get_internal_mode); -/** - * iio_set_internal_mode() - helper function providing write-only access to the - * internal @currentmode variable - * @indio_dev: IIO device structure for device - **/ -void iio_set_internal_mode(struct iio_dev *indio_dev, int mode) -{ - indio_dev->currentmode = mode; -} -EXPORT_SYMBOL_GPL(iio_set_internal_mode); - subsys_initcall(iio_init); module_exit(iio_exit); diff --git a/include/linux/iio/iio-opaque.h b/include/linux/iio/iio-opaque.h index 2be12b7b5dc5..72f35270a387 100644 --- a/include/linux/iio/iio-opaque.h +++ b/include/linux/iio/iio-opaque.h @@ -7,6 +7,9 @@ * struct iio_dev_opaque - industrial I/O device opaque information * @indio_dev: public industrial I/O device information * @id: used to identify device internally + * @currentmode: operating mode currently in use, may be eventually + * checked by device drivers but should be considered + * read-only as this is a core internal bit * @driver_module: used to make it harder to undercut users * @info_exist_lock: lock to prevent use during removal * @trig_readonly: mark the current trigger immutable @@ -36,6 +39,7 @@ */ struct iio_dev_opaque { struct iio_dev indio_dev; + int currentmode; int id; struct module *driver_module; struct mutex info_exist_lock; @@ -71,4 +75,16 @@ struct iio_dev_opaque { #define to_iio_dev_opaque(_indio_dev) \ container_of((_indio_dev), struct iio_dev_opaque, indio_dev) +/** + * iio_set_internal_mode() - helper function providing write-only access to the + * internal @currentmode variable + * @indio_dev: IIO device structure for device + **/ +static inline void iio_set_internal_mode(struct iio_dev *indio_dev, int mode) +{ + struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev); + + iio_dev_opaque->currentmode = mode; +} + #endif diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h index 27551d251904..d04ab89fa0c2 100644 --- a/include/linux/iio/iio.h +++ b/include/linux/iio/iio.h @@ -492,9 +492,6 @@ struct iio_buffer_setup_ops { * registering the IIO device and can be filed up by the * IIO core depending on the features advertised by the * driver during other steps of the registration - * @currentmode: [INTERN] operating mode currently in use, may be - * eventually checked by device drivers but should be - * considered read-only as this is a core internal bit * @dev: [DRIVER] device structure, should be assigned a parent * and owner * @buffer: [DRIVER] any buffer present @@ -521,7 +518,6 @@ struct iio_buffer_setup_ops { */ struct iio_dev { int modes; - int currentmode; struct device dev; struct iio_buffer *buffer; @@ -679,7 +675,6 @@ struct iio_trigger *devm_iio_trigger_alloc(struct device *parent, const char *fmt, ...); int iio_get_internal_mode(struct iio_dev *indio_dev); -void iio_set_internal_mode(struct iio_dev *indio_dev, int mode); /** * iio_buffer_enabled() - helper function to test if the buffer is enabled
This entry should, under no situation, be modified by device drivers. Now that we have limited its read access to device drivers really needing it, and handled modifications through another dedicated helper within the core, we can easily move this variable to the opaque structure as well as the write helper in order to prevent device driver from playing with it. Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> --- drivers/iio/industrialio-core.c | 15 +++------------ include/linux/iio/iio-opaque.h | 16 ++++++++++++++++ include/linux/iio/iio.h | 5 ----- 3 files changed, 19 insertions(+), 17 deletions(-)