Re: [PATCH v3 2/7] iio: core: wrap IIO device into an iio_dev_opaque object
On Sat, 2020-06-27 at 17:40 +0100, Jonathan Cameron wrote:
> On Sun, 21 Jun 2020 15:33:40 +0300
> Alexandru Ardelean wrote:
>
> > There are plenty of bad designs we want to discourage or not have to
> > review
> > manually usually about accessing private (marked as [INTERN]) fields of
> > 'struct iio_dev'.
> >
> > Sometimes users copy drivers that are not always the best examples.
> >
> > A better idea is to hide those fields into the framework.
> > For 'struct iio_dev' this is a 'struct iio_dev_opaque' which wraps a
> > public
> > 'struct iio_dev' object.
> >
> > In the next series, some fields will be moved to this new struct, each
> > with
> > it's own rework.
> >
> > This rework will not be complete-able for a while, as many fields need
> > some
> > drivers to be reworked in order to finalize them (e.g. 'indio_dev-
> > >mlock').
> >
> > But some fields can already be moved, and in time, all of them may get
> > there (in the 'struct iio_dev_opaque' object).
> >
> > Since a lot of drivers also call 'iio_priv()', in order to preserve
> > fast-paths (where this matters), the public iio_dev object will have a
> > 'priv' field that will have the pointer to the private information
> > already
> > computed. The reference returned by this field should be guaranteed to
> > be
> > cacheline aligned.
> >
> > As for the 'iio_priv_to_dev()' helper, this needs to be hidden away.
> > There
> > aren't many users of this helper, and arguably drivers shouldn't need
> > to
> > use it in any fast-paths, as they can maintain a reference to the IIO
> > device.
> Dropped this bit as previous patch deleted iio_priv_to_dev :)
Right.
My bad.
> > The opaque parts will be moved into the 'include/linux/iio/iio-
> > opaque.h'
> > header. Should the hidden information be required for some debugging or
> > some special needs, it can be made available via this header.
> > Otherwise, only the IIO core files should include this file.
> >
> > Signed-off-by: Alexandru Ardelean
>
> I've applied this as it stands, but I wonder if we should combine
> this with the existing iio-core.h header.
>
> Can do that later if it makes sense.
Initially, I had thought about putting all of the iio-opaque.h code in iio-
core.h, and limit the include range in drivers/iio, but then thought maybe
leave it in 'include/linux/iio/iio-opaque.h'. That way, it's probably a bit
easier for some debugging.
I am still not sure which direction is best.
But, we can move iio-core.h into iio-opaque.h (or the other way around).
>
> Jonathan
>
>
>
> > ---
> > drivers/iio/industrialio-core.c | 19 +--
> > include/linux/iio/iio-opaque.h | 17 +
> > include/linux/iio/iio.h | 6 +-
> > 3 files changed, 35 insertions(+), 7 deletions(-)
> > create mode 100644 include/linux/iio/iio-opaque.h
> >
> > diff --git a/drivers/iio/industrialio-core.c
> > b/drivers/iio/industrialio-core.c
> > index 75661661aaba..33e2953cf021 100644
> > --- a/drivers/iio/industrialio-core.c
> > +++ b/drivers/iio/industrialio-core.c
> > @@ -25,6 +25,7 @@
> > #include
> > #include
> > #include
> > +#include
> > #include "iio_core.h"
> > #include "iio_core_trigger.h"
> > #include
> > @@ -1473,6 +1474,8 @@ static void iio_device_unregister_sysfs(struct
> > iio_dev *indio_dev)
> > static void iio_dev_release(struct device *device)
> > {
> > struct iio_dev *indio_dev = dev_to_iio_dev(device);
> > + struct iio_dev_opaque *iio_dev_opaque =
> > to_iio_dev_opaque(indio_dev);
> > +
> > if (indio_dev->modes & INDIO_ALL_TRIGGERED_MODES)
> > iio_device_unregister_trigger_consumer(indio_dev);
> > iio_device_unregister_eventset(indio_dev);
> > @@ -1481,7 +1484,7 @@ static void iio_dev_release(struct device
> > *device)
> > iio_buffer_put(indio_dev->buffer);
> >
> > ida_simple_remove(_ida, indio_dev->id);
> > - kfree(indio_dev);
> > + kfree(iio_dev_opaque);
> > }
> >
> > struct device_type iio_device_type = {
> > @@ -1495,10 +1498,11 @@ struct device_type iio_device_type = {
> > **/
> > struct iio_dev *iio_device_alloc(struct device *parent, int
> > sizeof_priv)
> > {
> > + struct iio_dev_opaque *iio_dev_opaque;
> > struct iio_dev *dev;
> > size_t alloc_size;
> >
> > - alloc_size = sizeof(struct iio_dev);
> > + alloc_size = sizeof(struct iio_dev_opaque);
> > if (sizeof_priv) {
> > alloc_size = ALIGN(alloc_size, IIO_ALIGN);
> > alloc_size += sizeof_priv;
> > @@ -1506,11 +1510,14 @@ struct iio_dev *iio_device_alloc(struct device
> > *parent, int sizeof_priv)
> > /* ensure 32-byte alignment of whole construct ? */
> > alloc_size += IIO_ALIGN - 1;
> >
> > - dev = kzalloc(alloc_size, GFP_KERNEL);
> > - if (!dev)
> > + iio_dev_opaque = kzalloc(alloc_size, GFP_KERNEL);
> > + if (!iio_dev_opaque)
> > return NULL;
> >
> > - dev->dev.parent = parent;
> > + dev = _dev_opaque->indio_dev;
> > + dev->priv = (char
Re: [PATCH v3 2/7] iio: core: wrap IIO device into an iio_dev_opaque object
On Sun, 21 Jun 2020 15:33:40 +0300
Alexandru Ardelean wrote:
> There are plenty of bad designs we want to discourage or not have to review
> manually usually about accessing private (marked as [INTERN]) fields of
> 'struct iio_dev'.
>
> Sometimes users copy drivers that are not always the best examples.
>
> A better idea is to hide those fields into the framework.
> For 'struct iio_dev' this is a 'struct iio_dev_opaque' which wraps a public
> 'struct iio_dev' object.
>
> In the next series, some fields will be moved to this new struct, each with
> it's own rework.
>
> This rework will not be complete-able for a while, as many fields need some
> drivers to be reworked in order to finalize them (e.g. 'indio_dev->mlock').
>
> But some fields can already be moved, and in time, all of them may get
> there (in the 'struct iio_dev_opaque' object).
>
> Since a lot of drivers also call 'iio_priv()', in order to preserve
> fast-paths (where this matters), the public iio_dev object will have a
> 'priv' field that will have the pointer to the private information already
> computed. The reference returned by this field should be guaranteed to be
> cacheline aligned.
>
> As for the 'iio_priv_to_dev()' helper, this needs to be hidden away. There
> aren't many users of this helper, and arguably drivers shouldn't need to
> use it in any fast-paths, as they can maintain a reference to the IIO
> device.
Dropped this bit as previous patch deleted iio_priv_to_dev :)
>
> The opaque parts will be moved into the 'include/linux/iio/iio-opaque.h'
> header. Should the hidden information be required for some debugging or
> some special needs, it can be made available via this header.
> Otherwise, only the IIO core files should include this file.
>
> Signed-off-by: Alexandru Ardelean
I've applied this as it stands, but I wonder if we should combine
this with the existing iio-core.h header.
Can do that later if it makes sense.
Jonathan
> ---
> drivers/iio/industrialio-core.c | 19 +--
> include/linux/iio/iio-opaque.h | 17 +
> include/linux/iio/iio.h | 6 +-
> 3 files changed, 35 insertions(+), 7 deletions(-)
> create mode 100644 include/linux/iio/iio-opaque.h
>
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index 75661661aaba..33e2953cf021 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -25,6 +25,7 @@
> #include
> #include
> #include
> +#include
> #include "iio_core.h"
> #include "iio_core_trigger.h"
> #include
> @@ -1473,6 +1474,8 @@ static void iio_device_unregister_sysfs(struct iio_dev
> *indio_dev)
> static void iio_dev_release(struct device *device)
> {
> struct iio_dev *indio_dev = dev_to_iio_dev(device);
> + struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
> +
> if (indio_dev->modes & INDIO_ALL_TRIGGERED_MODES)
> iio_device_unregister_trigger_consumer(indio_dev);
> iio_device_unregister_eventset(indio_dev);
> @@ -1481,7 +1484,7 @@ static void iio_dev_release(struct device *device)
> iio_buffer_put(indio_dev->buffer);
>
> ida_simple_remove(_ida, indio_dev->id);
> - kfree(indio_dev);
> + kfree(iio_dev_opaque);
> }
>
> struct device_type iio_device_type = {
> @@ -1495,10 +1498,11 @@ struct device_type iio_device_type = {
> **/
> struct iio_dev *iio_device_alloc(struct device *parent, int sizeof_priv)
> {
> + struct iio_dev_opaque *iio_dev_opaque;
> struct iio_dev *dev;
> size_t alloc_size;
>
> - alloc_size = sizeof(struct iio_dev);
> + alloc_size = sizeof(struct iio_dev_opaque);
> if (sizeof_priv) {
> alloc_size = ALIGN(alloc_size, IIO_ALIGN);
> alloc_size += sizeof_priv;
> @@ -1506,11 +1510,14 @@ struct iio_dev *iio_device_alloc(struct device
> *parent, int sizeof_priv)
> /* ensure 32-byte alignment of whole construct ? */
> alloc_size += IIO_ALIGN - 1;
>
> - dev = kzalloc(alloc_size, GFP_KERNEL);
> - if (!dev)
> + iio_dev_opaque = kzalloc(alloc_size, GFP_KERNEL);
> + if (!iio_dev_opaque)
> return NULL;
>
> - dev->dev.parent = parent;
> + dev = _dev_opaque->indio_dev;
> + dev->priv = (char *)iio_dev_opaque +
> + ALIGN(sizeof(struct iio_dev_opaque), IIO_ALIGN);
> +
> dev->dev.groups = dev->groups;
> dev->dev.type = _device_type;
> dev->dev.bus = _bus_type;
> @@ -1524,7 +1531,7 @@ struct iio_dev *iio_device_alloc(struct device *parent,
> int sizeof_priv)
> if (dev->id < 0) {
> /* cannot use a dev_err as the name isn't available */
> pr_err("failed to get device id\n");
> - kfree(dev);
> + kfree(iio_dev_opaque);
> return NULL;
> }
> dev_set_name(>dev, "iio:device%d", dev->id);
> diff --git a/include/linux/iio/iio-opaque.h b/include/linux/iio/iio-opaque.h
>
[PATCH v3 2/7] iio: core: wrap IIO device into an iio_dev_opaque object
There are plenty of bad designs we want to discourage or not have to review
manually usually about accessing private (marked as [INTERN]) fields of
'struct iio_dev'.
Sometimes users copy drivers that are not always the best examples.
A better idea is to hide those fields into the framework.
For 'struct iio_dev' this is a 'struct iio_dev_opaque' which wraps a public
'struct iio_dev' object.
In the next series, some fields will be moved to this new struct, each with
it's own rework.
This rework will not be complete-able for a while, as many fields need some
drivers to be reworked in order to finalize them (e.g. 'indio_dev->mlock').
But some fields can already be moved, and in time, all of them may get
there (in the 'struct iio_dev_opaque' object).
Since a lot of drivers also call 'iio_priv()', in order to preserve
fast-paths (where this matters), the public iio_dev object will have a
'priv' field that will have the pointer to the private information already
computed. The reference returned by this field should be guaranteed to be
cacheline aligned.
As for the 'iio_priv_to_dev()' helper, this needs to be hidden away. There
aren't many users of this helper, and arguably drivers shouldn't need to
use it in any fast-paths, as they can maintain a reference to the IIO
device.
The opaque parts will be moved into the 'include/linux/iio/iio-opaque.h'
header. Should the hidden information be required for some debugging or
some special needs, it can be made available via this header.
Otherwise, only the IIO core files should include this file.
Signed-off-by: Alexandru Ardelean
---
drivers/iio/industrialio-core.c | 19 +--
include/linux/iio/iio-opaque.h | 17 +
include/linux/iio/iio.h | 6 +-
3 files changed, 35 insertions(+), 7 deletions(-)
create mode 100644 include/linux/iio/iio-opaque.h
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 75661661aaba..33e2953cf021 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -25,6 +25,7 @@
#include
#include
#include
+#include
#include "iio_core.h"
#include "iio_core_trigger.h"
#include
@@ -1473,6 +1474,8 @@ static void iio_device_unregister_sysfs(struct iio_dev
*indio_dev)
static void iio_dev_release(struct device *device)
{
struct iio_dev *indio_dev = dev_to_iio_dev(device);
+ struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
+
if (indio_dev->modes & INDIO_ALL_TRIGGERED_MODES)
iio_device_unregister_trigger_consumer(indio_dev);
iio_device_unregister_eventset(indio_dev);
@@ -1481,7 +1484,7 @@ static void iio_dev_release(struct device *device)
iio_buffer_put(indio_dev->buffer);
ida_simple_remove(_ida, indio_dev->id);
- kfree(indio_dev);
+ kfree(iio_dev_opaque);
}
struct device_type iio_device_type = {
@@ -1495,10 +1498,11 @@ struct device_type iio_device_type = {
**/
struct iio_dev *iio_device_alloc(struct device *parent, int sizeof_priv)
{
+ struct iio_dev_opaque *iio_dev_opaque;
struct iio_dev *dev;
size_t alloc_size;
- alloc_size = sizeof(struct iio_dev);
+ alloc_size = sizeof(struct iio_dev_opaque);
if (sizeof_priv) {
alloc_size = ALIGN(alloc_size, IIO_ALIGN);
alloc_size += sizeof_priv;
@@ -1506,11 +1510,14 @@ struct iio_dev *iio_device_alloc(struct device *parent,
int sizeof_priv)
/* ensure 32-byte alignment of whole construct ? */
alloc_size += IIO_ALIGN - 1;
- dev = kzalloc(alloc_size, GFP_KERNEL);
- if (!dev)
+ iio_dev_opaque = kzalloc(alloc_size, GFP_KERNEL);
+ if (!iio_dev_opaque)
return NULL;
- dev->dev.parent = parent;
+ dev = _dev_opaque->indio_dev;
+ dev->priv = (char *)iio_dev_opaque +
+ ALIGN(sizeof(struct iio_dev_opaque), IIO_ALIGN);
+
dev->dev.groups = dev->groups;
dev->dev.type = _device_type;
dev->dev.bus = _bus_type;
@@ -1524,7 +1531,7 @@ struct iio_dev *iio_device_alloc(struct device *parent,
int sizeof_priv)
if (dev->id < 0) {
/* cannot use a dev_err as the name isn't available */
pr_err("failed to get device id\n");
- kfree(dev);
+ kfree(iio_dev_opaque);
return NULL;
}
dev_set_name(>dev, "iio:device%d", dev->id);
diff --git a/include/linux/iio/iio-opaque.h b/include/linux/iio/iio-opaque.h
new file mode 100644
index ..1375674f14cd
--- /dev/null
+++ b/include/linux/iio/iio-opaque.h
@@ -0,0 +1,17 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef _INDUSTRIAL_IO_OPAQUE_H_
+#define _INDUSTRIAL_IO_OPAQUE_H_
+
+/**
+ * struct iio_dev_opaque - industrial I/O device opaque information
+ * @indio_dev: public industrial I/O device information
+ */
+struct iio_dev_opaque {
+ struct iio_dev
