[PATCH 1/3] iio: core: wrap IIO device into a iio_dev_priv object

[PATCH 1/3] iio: core: wrap IIO device into a iio_dev_priv object

[Alexandru Ardelean]

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'.

This is difficult, as a lot of users copy drivers, and 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_priv' which wraps a public
'struct iio_dev' object.

In the next patches, 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_priv' object).

We also need to hide the implementations for 'iio_priv()' &
'iio_priv_to_dev()', as the pointer arithmetic will not match once things
are moved.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---

Just as a note here, I've been running this patchset without a problem
for 2 weeks now in a work branch.
But it's only been a setup, so no idea if some other thing may cause
bigger issues.

This small patchset is meant to kickstart this, for GSoC people or for
people wanting to start contributing to IIO.

 drivers/iio/iio_core.h          | 11 +++++++++++
 drivers/iio/industrialio-core.c | 32 +++++++++++++++++++++++++++-----
 include/linux/iio/iio.h         | 12 ++----------
 3 files changed, 40 insertions(+), 15 deletions(-)

diff --git a/drivers/iio/iio_core.h b/drivers/iio/iio_core.h
index fd9a5f1d5e51..84f3b4590c05 100644
--- a/drivers/iio/iio_core.h
+++ b/drivers/iio/iio_core.h
@@ -17,6 +17,17 @@ struct iio_dev;
 
 extern struct device_type iio_device_type;
 
+/**
+ * struct iio_dev_priv - industrial I/O device private information
+ * @indio_dev:			public IIO device object
+ */
+struct iio_dev_priv {
+	struct iio_dev			indio_dev;
+};
+
+#define to_iio_dev_priv(indio_dev)	\
+	container_of(indio_dev, struct iio_dev_priv, indio_dev)
+
 int __iio_add_chan_devattr(const char *postfix,
 			   struct iio_chan_spec const *chan,
 			   ssize_t (*func)(struct device *dev,
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 462d3e810013..f0888dd84d3d 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -164,6 +164,23 @@ static const char * const iio_chan_info_postfix[] = {
 	[IIO_CHAN_INFO_THERMOCOUPLE_TYPE] = "thermocouple_type",
 };
 
+
+void *iio_priv(const struct iio_dev *indio_dev)
+{
+	struct iio_dev_priv *iio_dev_priv = to_iio_dev_priv(indio_dev);
+	return (char *)iio_dev_priv + ALIGN(sizeof(struct iio_dev_priv), IIO_ALIGN);
+}
+EXPORT_SYMBOL_GPL(iio_priv);
+
+struct iio_dev *iio_priv_to_dev(void *priv)
+{
+	struct iio_dev_priv *iio_dev_priv =
+		(struct iio_dev_priv *)((char *)priv -
+				  ALIGN(sizeof(struct iio_dev_priv), IIO_ALIGN));
+	return &iio_dev_priv->indio_dev;
+}
+EXPORT_SYMBOL_GPL(iio_priv_to_dev);
+
 /**
  * iio_find_channel_from_si() - get channel from its scan index
  * @indio_dev:		device
@@ -1476,6 +1493,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_priv *iio_dev_priv = to_iio_dev_priv(indio_dev);
+
 	if (indio_dev->modes & INDIO_ALL_TRIGGERED_MODES)
 		iio_device_unregister_trigger_consumer(indio_dev);
 	iio_device_unregister_eventset(indio_dev);
@@ -1484,7 +1503,7 @@ static void iio_dev_release(struct device *device)
 	iio_buffer_put(indio_dev->buffer);
 
 	ida_simple_remove(&iio_ida, indio_dev->id);
-	kfree(indio_dev);
+	kfree(iio_dev_priv);
 }
 
 struct device_type iio_device_type = {
@@ -1498,10 +1517,11 @@ struct device_type iio_device_type = {
  **/
 struct iio_dev *iio_device_alloc(int sizeof_priv)
 {
+	struct iio_dev_priv *iio_dev_priv;
 	struct iio_dev *dev;
 	size_t alloc_size;
 
-	alloc_size = sizeof(struct iio_dev);
+	alloc_size = sizeof(struct iio_dev_priv);
 	if (sizeof_priv) {
 		alloc_size = ALIGN(alloc_size, IIO_ALIGN);
 		alloc_size += sizeof_priv;
@@ -1509,10 +1529,12 @@ struct iio_dev *iio_device_alloc(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_priv = kzalloc(alloc_size, GFP_KERNEL);
+	if (!iio_dev_priv)
 		return NULL;
 
+	dev = &iio_dev_priv->indio_dev;
+
 	dev->dev.groups = dev->groups;
 	dev->dev.type = &iio_device_type;
 	dev->dev.bus = &iio_bus_type;
@@ -1526,7 +1548,7 @@ struct iio_dev *iio_device_alloc(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_priv);
 		return NULL;
 	}
 	dev_set_name(&dev->dev, "iio:device%d", dev->id);
diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index 5f9f439a4f01..38c4ea505394 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -678,16 +678,8 @@ static inline void *iio_device_get_drvdata(struct iio_dev *indio_dev)
 #define IIO_ALIGN L1_CACHE_BYTES
 struct iio_dev *iio_device_alloc(int sizeof_priv);
 
-static inline void *iio_priv(const struct iio_dev *indio_dev)
-{
-	return (char *)indio_dev + ALIGN(sizeof(struct iio_dev), IIO_ALIGN);
-}
-
-static inline struct iio_dev *iio_priv_to_dev(void *priv)
-{
-	return (struct iio_dev *)((char *)priv -
-				  ALIGN(sizeof(struct iio_dev), IIO_ALIGN));
-}
+void *iio_priv(const struct iio_dev *indio_dev);
+struct iio_dev *iio_priv_to_dev(void *priv);
 
 void iio_device_free(struct iio_dev *indio_dev);
 struct iio_dev *devm_iio_device_alloc(struct device *dev, int sizeof_priv);
-- 
2.17.1

[PATCH 2/3] iio: core: simplify alloc alignment code

[Alexandru Ardelean]

There was a recent discussion about this code:
  https://lore.kernel.org/linux-iio/20200322165317.0b1f0674@archlinux/

This looks like a good time to rework this, since any issues about it
should pop-up under testing, because the iio_dev is having a bit of an
overhaul and stuff being moved to iio_dev_priv.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/industrialio-core.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index f0888dd84d3d..b924197b5984 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -1521,13 +1521,9 @@ struct iio_dev *iio_device_alloc(int sizeof_priv)
 	struct iio_dev *dev;
 	size_t alloc_size;
 
-	alloc_size = sizeof(struct iio_dev_priv);
-	if (sizeof_priv) {
-		alloc_size = ALIGN(alloc_size, IIO_ALIGN);
-		alloc_size += sizeof_priv;
-	}
-	/* ensure 32-byte alignment of whole construct ? */
-	alloc_size += IIO_ALIGN - 1;
+	alloc_size = ALIGN(sizeof(struct iio_dev_priv), IIO_ALIGN);
+	if (sizeof_priv)
+		alloc_size += ALIGN(sizeof_priv, IIO_ALIGN);
 
 	iio_dev_priv = kzalloc(alloc_size, GFP_KERNEL);
 	if (!iio_dev_priv)
-- 
2.17.1

[PATCH 3/3] iio: core: move debugfs data on the private iio dev info

[Alexandru Ardelean]

This change moves all iio_dev debugfs fields to the iio_dev_priv object.
It's not the biggest advantage yet (to the whole thing of abstractization)
but it's a start.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/iio_core.h          | 10 +++++++++
 drivers/iio/industrialio-core.c | 40 ++++++++++++++++++++++-----------
 include/linux/iio/iio.h         | 13 +----------
 3 files changed, 38 insertions(+), 25 deletions(-)

diff --git a/drivers/iio/iio_core.h b/drivers/iio/iio_core.h
index 84f3b4590c05..bc9f580d2bdd 100644
--- a/drivers/iio/iio_core.h
+++ b/drivers/iio/iio_core.h
@@ -20,9 +20,19 @@ extern struct device_type iio_device_type;
 /**
  * struct iio_dev_priv - industrial I/O device private information
  * @indio_dev:			public IIO device object
+ * @debugfs_dentry:		device specific debugfs dentry
+ * @cached_reg_addr:		cached register address for debugfs reads
+ * @read_buf:			read buffer to be used for the initial reg read
+ * @read_buf_len:		data length in @read_buf
  */
 struct iio_dev_priv {
 	struct iio_dev			indio_dev;
+#if defined(CONFIG_DEBUG_FS)
+	struct dentry			*debugfs_dentry;
+	unsigned			cached_reg_addr;
+	char				read_buf[20];
+	unsigned int			read_buf_len;
+#endif
 };
 
 #define to_iio_dev_priv(indio_dev)	\
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index b924197b5984..091ae79de751 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -181,6 +181,13 @@ struct iio_dev *iio_priv_to_dev(void *priv)
 }
 EXPORT_SYMBOL_GPL(iio_priv_to_dev);
 
+struct dentry *iio_get_debugfs_dentry(struct iio_dev *indio_dev)
+{
+	struct iio_dev_priv *iio_dev_priv = to_iio_dev_priv(indio_dev);
+	return iio_dev_priv->debugfs_dentry;
+}
+EXPORT_SYMBOL_GPL(iio_get_debugfs_dentry);
+
 /**
  * iio_find_channel_from_si() - get channel from its scan index
  * @indio_dev:		device
@@ -324,35 +331,37 @@ static ssize_t iio_debugfs_read_reg(struct file *file, char __user *userbuf,
 			      size_t count, loff_t *ppos)
 {
 	struct iio_dev *indio_dev = file->private_data;
+	struct iio_dev_priv *iio_dev_priv = to_iio_dev_priv(indio_dev);
 	unsigned val = 0;
 	int ret;
 
 	if (*ppos > 0)
 		return simple_read_from_buffer(userbuf, count, ppos,
-					       indio_dev->read_buf,
-					       indio_dev->read_buf_len);
+					       iio_dev_priv->read_buf,
+					       iio_dev_priv->read_buf_len);
 
 	ret = indio_dev->info->debugfs_reg_access(indio_dev,
-						  indio_dev->cached_reg_addr,
+						  iio_dev_priv->cached_reg_addr,
 						  0, &val);
 	if (ret) {
 		dev_err(indio_dev->dev.parent, "%s: read failed\n", __func__);
 		return ret;
 	}
 
-	indio_dev->read_buf_len = snprintf(indio_dev->read_buf,
-					   sizeof(indio_dev->read_buf),
-					   "0x%X\n", val);
+	iio_dev_priv->read_buf_len = snprintf(iio_dev_priv->read_buf,
+					      sizeof(iio_dev_priv->read_buf),
+					      "0x%X\n", val);
 
 	return simple_read_from_buffer(userbuf, count, ppos,
-				       indio_dev->read_buf,
-				       indio_dev->read_buf_len);
+				       iio_dev_priv->read_buf,
+				       iio_dev_priv->read_buf_len);
 }
 
 static ssize_t iio_debugfs_write_reg(struct file *file,
 		     const char __user *userbuf, size_t count, loff_t *ppos)
 {
 	struct iio_dev *indio_dev = file->private_data;
+	struct iio_dev_priv *iio_dev_priv = to_iio_dev_priv(indio_dev);
 	unsigned reg, val;
 	char buf[80];
 	int ret;
@@ -367,10 +376,10 @@ static ssize_t iio_debugfs_write_reg(struct file *file,
 
 	switch (ret) {
 	case 1:
-		indio_dev->cached_reg_addr = reg;
+		iio_dev_priv->cached_reg_addr = reg;
 		break;
 	case 2:
-		indio_dev->cached_reg_addr = reg;
+		iio_dev_priv->cached_reg_addr = reg;
 		ret = indio_dev->info->debugfs_reg_access(indio_dev, reg,
 							  val, NULL);
 		if (ret) {
@@ -394,23 +403,28 @@ static const struct file_operations iio_debugfs_reg_fops = {
 
 static void iio_device_unregister_debugfs(struct iio_dev *indio_dev)
 {
-	debugfs_remove_recursive(indio_dev->debugfs_dentry);
+	struct iio_dev_priv *iio_dev_priv = to_iio_dev_priv(indio_dev);
+	debugfs_remove_recursive(iio_dev_priv->debugfs_dentry);
 }
 
 static void iio_device_register_debugfs(struct iio_dev *indio_dev)
 {
+	struct iio_dev_priv *iio_dev_priv;
+
 	if (indio_dev->info->debugfs_reg_access == NULL)
 		return;
 
 	if (!iio_debugfs_dentry)
 		return;
 
-	indio_dev->debugfs_dentry =
+	iio_dev_priv = to_iio_dev_priv(indio_dev);
+
+	iio_dev_priv->debugfs_dentry =
 		debugfs_create_dir(dev_name(&indio_dev->dev),
 				   iio_debugfs_dentry);
 
 	debugfs_create_file("direct_reg_access", 0644,
-			    indio_dev->debugfs_dentry, indio_dev,
+			    iio_dev_priv->debugfs_dentry, indio_dev,
 			    &iio_debugfs_reg_fops);
 }
 #else
diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index 38c4ea505394..6155e7aec60c 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -520,8 +520,6 @@ struct iio_buffer_setup_ops {
  * @groups:		[INTERN] attribute groups
  * @groupcounter:	[INTERN] index of next attribute group
  * @flags:		[INTERN] file ops related flags including busy flag.
- * @debugfs_dentry:	[INTERN] device specific debugfs dentry.
- * @cached_reg_addr:	[INTERN] cached register address for debugfs reads.
  */
 struct iio_dev {
 	int				id;
@@ -565,12 +563,6 @@ struct iio_dev {
 	int				groupcounter;
 
 	unsigned long			flags;
-#if defined(CONFIG_DEBUG_FS)
-	struct dentry			*debugfs_dentry;
-	unsigned			cached_reg_addr;
-	char				read_buf[20];
-	unsigned int			read_buf_len;
-#endif
 };
 
 const struct iio_chan_spec
@@ -701,10 +693,7 @@ static inline bool iio_buffer_enabled(struct iio_dev *indio_dev)
  * @indio_dev:		IIO device structure for device
  **/
 #if defined(CONFIG_DEBUG_FS)
-static inline struct dentry *iio_get_debugfs_dentry(struct iio_dev *indio_dev)
-{
-	return indio_dev->debugfs_dentry;
-}
+struct dentry *iio_get_debugfs_dentry(struct iio_dev *indio_dev);
 #else
 static inline struct dentry *iio_get_debugfs_dentry(struct iio_dev *indio_dev)
 {
-- 
2.17.1

Re: [PATCH 1/3] iio: core: wrap IIO device into a iio_dev_priv object

[Jonathan Cameron]

On Fri, 8 May 2020 17:13:04 +0300
Alexandru Ardelean <alexandru.ardelean@analog.com> 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'.

This has been on the todo list for many years so great you are having a go.
 
> 
> This is difficult, as a lot of users copy drivers, and 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_priv' which wraps a public
> 'struct iio_dev' object.
> 
> In the next patches, 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_priv' object).
> 
> We also need to hide the implementations for 'iio_priv()' &
> 'iio_priv_to_dev()', as the pointer arithmetic will not match once things
> are moved.

This last bit has the disadvantage of potentially putting a function
call in some fast paths where there wasn't one before.

We may not need to 'forcefully' hide the internal parts.  May be
enough to just make their use really obvious.  If you have to cast
to an iio_dev_priv then you are doing something very wrong.
The old stick __ in front of it may make that even more obvious.

Naming is a bit confusing though.  iio_dev_priv sounds like private
to the device... iio_dev_opaque maybe?


> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> ---
> 
> Just as a note here, I've been running this patchset without a problem
> for 2 weeks now in a work branch.
> But it's only been a setup, so no idea if some other thing may cause
> bigger issues.
> 
> This small patchset is meant to kickstart this, for GSoC people or for
> people wanting to start contributing to IIO.
> 
>  drivers/iio/iio_core.h          | 11 +++++++++++
>  drivers/iio/industrialio-core.c | 32 +++++++++++++++++++++++++++-----
>  include/linux/iio/iio.h         | 12 ++----------
>  3 files changed, 40 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/iio/iio_core.h b/drivers/iio/iio_core.h
> index fd9a5f1d5e51..84f3b4590c05 100644
> --- a/drivers/iio/iio_core.h
> +++ b/drivers/iio/iio_core.h
> @@ -17,6 +17,17 @@ struct iio_dev;
>  
>  extern struct device_type iio_device_type;
>  
> +/**
> + * struct iio_dev_priv - industrial I/O device private information
> + * @indio_dev:			public IIO device object
> + */
> +struct iio_dev_priv {
> +	struct iio_dev			indio_dev;
> +};
> +
> +#define to_iio_dev_priv(indio_dev)	\
> +	container_of(indio_dev, struct iio_dev_priv, indio_dev)
> +
>  int __iio_add_chan_devattr(const char *postfix,
>  			   struct iio_chan_spec const *chan,
>  			   ssize_t (*func)(struct device *dev,
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index 462d3e810013..f0888dd84d3d 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -164,6 +164,23 @@ static const char * const iio_chan_info_postfix[] = {
>  	[IIO_CHAN_INFO_THERMOCOUPLE_TYPE] = "thermocouple_type",
>  };
>  
> +
> +void *iio_priv(const struct iio_dev *indio_dev)
> +{
> +	struct iio_dev_priv *iio_dev_priv = to_iio_dev_priv(indio_dev);
> +	return (char *)iio_dev_priv + ALIGN(sizeof(struct iio_dev_priv), IIO_ALIGN);
> +}
> +EXPORT_SYMBOL_GPL(iio_priv);
> +
> +struct iio_dev *iio_priv_to_dev(void *priv)
> +{
> +	struct iio_dev_priv *iio_dev_priv =
> +		(struct iio_dev_priv *)((char *)priv -
> +				  ALIGN(sizeof(struct iio_dev_priv), IIO_ALIGN));
> +	return &iio_dev_priv->indio_dev;
> +}
> +EXPORT_SYMBOL_GPL(iio_priv_to_dev);
> +
>  /**
>   * iio_find_channel_from_si() - get channel from its scan index
>   * @indio_dev:		device
> @@ -1476,6 +1493,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_priv *iio_dev_priv = to_iio_dev_priv(indio_dev);
> +
>  	if (indio_dev->modes & INDIO_ALL_TRIGGERED_MODES)
>  		iio_device_unregister_trigger_consumer(indio_dev);
>  	iio_device_unregister_eventset(indio_dev);
> @@ -1484,7 +1503,7 @@ static void iio_dev_release(struct device *device)
>  	iio_buffer_put(indio_dev->buffer);
>  
>  	ida_simple_remove(&iio_ida, indio_dev->id);
> -	kfree(indio_dev);
> +	kfree(iio_dev_priv);
>  }
>  
>  struct device_type iio_device_type = {
> @@ -1498,10 +1517,11 @@ struct device_type iio_device_type = {
>   **/
>  struct iio_dev *iio_device_alloc(int sizeof_priv)
>  {
> +	struct iio_dev_priv *iio_dev_priv;
>  	struct iio_dev *dev;
>  	size_t alloc_size;
>  
> -	alloc_size = sizeof(struct iio_dev);
> +	alloc_size = sizeof(struct iio_dev_priv);
>  	if (sizeof_priv) {
>  		alloc_size = ALIGN(alloc_size, IIO_ALIGN);
>  		alloc_size += sizeof_priv;
> @@ -1509,10 +1529,12 @@ struct iio_dev *iio_device_alloc(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_priv = kzalloc(alloc_size, GFP_KERNEL);
> +	if (!iio_dev_priv)
>  		return NULL;
>  
> +	dev = &iio_dev_priv->indio_dev;
> +
>  	dev->dev.groups = dev->groups;
>  	dev->dev.type = &iio_device_type;
>  	dev->dev.bus = &iio_bus_type;
> @@ -1526,7 +1548,7 @@ struct iio_dev *iio_device_alloc(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_priv);
>  		return NULL;
>  	}
>  	dev_set_name(&dev->dev, "iio:device%d", dev->id);
> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> index 5f9f439a4f01..38c4ea505394 100644
> --- a/include/linux/iio/iio.h
> +++ b/include/linux/iio/iio.h
> @@ -678,16 +678,8 @@ static inline void *iio_device_get_drvdata(struct iio_dev *indio_dev)
>  #define IIO_ALIGN L1_CACHE_BYTES
>  struct iio_dev *iio_device_alloc(int sizeof_priv);
>  
> -static inline void *iio_priv(const struct iio_dev *indio_dev)
> -{
> -	return (char *)indio_dev + ALIGN(sizeof(struct iio_dev), IIO_ALIGN);
> -}
> -
> -static inline struct iio_dev *iio_priv_to_dev(void *priv)
> -{
> -	return (struct iio_dev *)((char *)priv -
> -				  ALIGN(sizeof(struct iio_dev), IIO_ALIGN));
> -}
> +void *iio_priv(const struct iio_dev *indio_dev);
> +struct iio_dev *iio_priv_to_dev(void *priv);
>  
>  void iio_device_free(struct iio_dev *indio_dev);
>  struct iio_dev *devm_iio_device_alloc(struct device *dev, int sizeof_priv);

Re: [PATCH 1/3] iio: core: wrap IIO device into a iio_dev_priv object

[Jonathan Cameron]

On Fri, 8 May 2020 16:40:15 +0100
Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:

> On Fri, 8 May 2020 17:13:04 +0300
> Alexandru Ardelean <alexandru.ardelean@analog.com> 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'.  
> 
> This has been on the todo list for many years so great you are having a go.
>  
> > 
> > This is difficult, as a lot of users copy drivers, and 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_priv' which wraps a public
> > 'struct iio_dev' object.
> > 
> > In the next patches, 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_priv' object).
> > 
> > We also need to hide the implementations for 'iio_priv()' &
> > 'iio_priv_to_dev()', as the pointer arithmetic will not match once things
> > are moved.  
> 
> This last bit has the disadvantage of potentially putting a function
> call in some fast paths where there wasn't one before.
> 
> We may not need to 'forcefully' hide the internal parts.  May be
> enough to just make their use really obvious.  If you have to cast
> to an iio_dev_priv then you are doing something very wrong.

Note, definitely keep the to_* macro only in the private core header.

> The old stick __ in front of it may make that even more obvious.
> 
> Naming is a bit confusing though.  iio_dev_priv sounds like private
> to the device... iio_dev_opaque maybe?
> 
> 
> > 
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> > ---
> > 
> > Just as a note here, I've been running this patchset without a problem
> > for 2 weeks now in a work branch.
> > But it's only been a setup, so no idea if some other thing may cause
> > bigger issues.
> > 
> > This small patchset is meant to kickstart this, for GSoC people or for
> > people wanting to start contributing to IIO.
> > 
> >  drivers/iio/iio_core.h          | 11 +++++++++++
> >  drivers/iio/industrialio-core.c | 32 +++++++++++++++++++++++++++-----
> >  include/linux/iio/iio.h         | 12 ++----------
> >  3 files changed, 40 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/iio/iio_core.h b/drivers/iio/iio_core.h
> > index fd9a5f1d5e51..84f3b4590c05 100644
> > --- a/drivers/iio/iio_core.h
> > +++ b/drivers/iio/iio_core.h
> > @@ -17,6 +17,17 @@ struct iio_dev;
> >  
> >  extern struct device_type iio_device_type;
> >  
> > +/**
> > + * struct iio_dev_priv - industrial I/O device private information
> > + * @indio_dev:			public IIO device object
> > + */
> > +struct iio_dev_priv {
> > +	struct iio_dev			indio_dev;
> > +};
> > +
> > +#define to_iio_dev_priv(indio_dev)	\
> > +	container_of(indio_dev, struct iio_dev_priv, indio_dev)
> > +
> >  int __iio_add_chan_devattr(const char *postfix,
> >  			   struct iio_chan_spec const *chan,
> >  			   ssize_t (*func)(struct device *dev,
> > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> > index 462d3e810013..f0888dd84d3d 100644
> > --- a/drivers/iio/industrialio-core.c
> > +++ b/drivers/iio/industrialio-core.c
> > @@ -164,6 +164,23 @@ static const char * const iio_chan_info_postfix[] = {
> >  	[IIO_CHAN_INFO_THERMOCOUPLE_TYPE] = "thermocouple_type",
> >  };
> >  
> > +
> > +void *iio_priv(const struct iio_dev *indio_dev)
> > +{
> > +	struct iio_dev_priv *iio_dev_priv = to_iio_dev_priv(indio_dev);
> > +	return (char *)iio_dev_priv + ALIGN(sizeof(struct iio_dev_priv), IIO_ALIGN);
> > +}
> > +EXPORT_SYMBOL_GPL(iio_priv);
> > +
> > +struct iio_dev *iio_priv_to_dev(void *priv)
> > +{
> > +	struct iio_dev_priv *iio_dev_priv =
> > +		(struct iio_dev_priv *)((char *)priv -
> > +				  ALIGN(sizeof(struct iio_dev_priv), IIO_ALIGN));
> > +	return &iio_dev_priv->indio_dev;
> > +}
> > +EXPORT_SYMBOL_GPL(iio_priv_to_dev);
> > +
> >  /**
> >   * iio_find_channel_from_si() - get channel from its scan index
> >   * @indio_dev:		device
> > @@ -1476,6 +1493,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_priv *iio_dev_priv = to_iio_dev_priv(indio_dev);
> > +
> >  	if (indio_dev->modes & INDIO_ALL_TRIGGERED_MODES)
> >  		iio_device_unregister_trigger_consumer(indio_dev);
> >  	iio_device_unregister_eventset(indio_dev);
> > @@ -1484,7 +1503,7 @@ static void iio_dev_release(struct device *device)
> >  	iio_buffer_put(indio_dev->buffer);
> >  
> >  	ida_simple_remove(&iio_ida, indio_dev->id);
> > -	kfree(indio_dev);
> > +	kfree(iio_dev_priv);
> >  }
> >  
> >  struct device_type iio_device_type = {
> > @@ -1498,10 +1517,11 @@ struct device_type iio_device_type = {
> >   **/
> >  struct iio_dev *iio_device_alloc(int sizeof_priv)
> >  {
> > +	struct iio_dev_priv *iio_dev_priv;
> >  	struct iio_dev *dev;
> >  	size_t alloc_size;
> >  
> > -	alloc_size = sizeof(struct iio_dev);
> > +	alloc_size = sizeof(struct iio_dev_priv);
> >  	if (sizeof_priv) {
> >  		alloc_size = ALIGN(alloc_size, IIO_ALIGN);
> >  		alloc_size += sizeof_priv;
> > @@ -1509,10 +1529,12 @@ struct iio_dev *iio_device_alloc(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_priv = kzalloc(alloc_size, GFP_KERNEL);
> > +	if (!iio_dev_priv)
> >  		return NULL;
> >  
> > +	dev = &iio_dev_priv->indio_dev;
> > +
> >  	dev->dev.groups = dev->groups;
> >  	dev->dev.type = &iio_device_type;
> >  	dev->dev.bus = &iio_bus_type;
> > @@ -1526,7 +1548,7 @@ struct iio_dev *iio_device_alloc(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_priv);
> >  		return NULL;
> >  	}
> >  	dev_set_name(&dev->dev, "iio:device%d", dev->id);
> > diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> > index 5f9f439a4f01..38c4ea505394 100644
> > --- a/include/linux/iio/iio.h
> > +++ b/include/linux/iio/iio.h
> > @@ -678,16 +678,8 @@ static inline void *iio_device_get_drvdata(struct iio_dev *indio_dev)
> >  #define IIO_ALIGN L1_CACHE_BYTES
> >  struct iio_dev *iio_device_alloc(int sizeof_priv);
> >  
> > -static inline void *iio_priv(const struct iio_dev *indio_dev)
> > -{
> > -	return (char *)indio_dev + ALIGN(sizeof(struct iio_dev), IIO_ALIGN);
> > -}
> > -
> > -static inline struct iio_dev *iio_priv_to_dev(void *priv)
> > -{
> > -	return (struct iio_dev *)((char *)priv -
> > -				  ALIGN(sizeof(struct iio_dev), IIO_ALIGN));
> > -}
> > +void *iio_priv(const struct iio_dev *indio_dev);
> > +struct iio_dev *iio_priv_to_dev(void *priv);
> >  
> >  void iio_device_free(struct iio_dev *indio_dev);
> >  struct iio_dev *devm_iio_device_alloc(struct device *dev, int sizeof_priv);  
> 
> 

Re: [PATCH 1/3] iio: core: wrap IIO device into a iio_dev_priv object

[Ardelean, Alexandru]

On Fri, 2020-05-08 at 16:44 +0100, Jonathan Cameron wrote:
> [External]
> 
> On Fri, 8 May 2020 16:40:15 +0100
> Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
> 
> > On Fri, 8 May 2020 17:13:04 +0300
> > Alexandru Ardelean <alexandru.ardelean@analog.com> 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'.  
> > 
> > This has been on the todo list for many years so great you are having a go.
> >  
> > > This is difficult, as a lot of users copy drivers, and 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_priv' which wraps a public
> > > 'struct iio_dev' object.
> > > 
> > > In the next patches, 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_priv' object).
> > > 
> > > We also need to hide the implementations for 'iio_priv()' &
> > > 'iio_priv_to_dev()', as the pointer arithmetic will not match once things
> > > are moved.  
> > 
> > This last bit has the disadvantage of potentially putting a function
> > call in some fast paths where there wasn't one before.

Hmm.
I think we can change this to have a void *ptr in the public iio.h header and
that points to the private data in the private struct, and we compute it once,
and just return it.

so:

struct iio_dev_opaque;

struct iio_dev {
    struct iio_dev_opaque *opaque;  // compute this during iio_device_alloc()
    void *priv;                     // compute this during iio_device_alloc()
};

static inline void *iio_priv(const struct iio_dev *indio_dev)
{
        return indio_dev->priv;
}

[ or even just convert it to a macro ; no preference ]

> > 
> > We may not need to 'forcefully' hide the internal parts.  May be
> > enough to just make their use really obvious.  If you have to cast
> > to an iio_dev_priv then you are doing something very wrong.
> 
> Note, definitely keep the to_* macro only in the private core header.
> 
> > The old stick __ in front of it may make that even more obvious.
> > 
> > Naming is a bit confusing though.  iio_dev_priv sounds like private
> > to the device... iio_dev_opaque maybe?

regarding the 'opaque' part, that would be a 'struct iio_dev_opaque {' that
lives somewhere like

struct iio_dev_opaque {
    struct iio_dev   indio_dev;  // public IIO device part
    ... // opaque IIO device parts
    ... // iio_priv data
}

The stick __ in front, may work just fine as well.

Personally, I would prefer to forcefully hide stuff, because then the compiler
could also be useful in telling people not to use some private fields.
People [juniors usually] listen to compilers more than they listen to review-
comments. 
We could move the 'struct iio_dev_opaque' in 'include/linux/iio/iio-opaque.h',
so that if they need to access some fields for some advanced/hacky debugging, it
should work as well.

Still, if it's more preferred to prefix them, I can do that as well.
But we should prefix them as we manage to somehow make the more opaque, or
remove the fields from places they should be used.
Otherwise it is pointless, as people would copy 'indio_dev->__mlock' anyway.


> > 
> > 
> > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> > > ---
> > > 
> > > Just as a note here, I've been running this patchset without a problem
> > > for 2 weeks now in a work branch.
> > > But it's only been a setup, so no idea if some other thing may cause
> > > bigger issues.
> > > 
> > > This small patchset is meant to kickstart this, for GSoC people or for
> > > people wanting to start contributing to IIO.
> > > 
> > >  drivers/iio/iio_core.h          | 11 +++++++++++
> > >  drivers/iio/industrialio-core.c | 32 +++++++++++++++++++++++++++-----
> > >  include/linux/iio/iio.h         | 12 ++----------
> > >  3 files changed, 40 insertions(+), 15 deletions(-)
> > > 
> > > diff --git a/drivers/iio/iio_core.h b/drivers/iio/iio_core.h
> > > index fd9a5f1d5e51..84f3b4590c05 100644
> > > --- a/drivers/iio/iio_core.h
> > > +++ b/drivers/iio/iio_core.h
> > > @@ -17,6 +17,17 @@ struct iio_dev;
> > >  
> > >  extern struct device_type iio_device_type;
> > >  
> > > +/**
> > > + * struct iio_dev_priv - industrial I/O device private information
> > > + * @indio_dev:			public IIO device object
> > > + */
> > > +struct iio_dev_priv {
> > > +	struct iio_dev			indio_dev;
> > > +};
> > > +
> > > +#define to_iio_dev_priv(indio_dev)	\
> > > +	container_of(indio_dev, struct iio_dev_priv, indio_dev)
> > > +
> > >  int __iio_add_chan_devattr(const char *postfix,
> > >  			   struct iio_chan_spec const *chan,
> > >  			   ssize_t (*func)(struct device *dev,
> > > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-
> > > core.c
> > > index 462d3e810013..f0888dd84d3d 100644
> > > --- a/drivers/iio/industrialio-core.c
> > > +++ b/drivers/iio/industrialio-core.c
> > > @@ -164,6 +164,23 @@ static const char * const iio_chan_info_postfix[] = {
> > >  	[IIO_CHAN_INFO_THERMOCOUPLE_TYPE] = "thermocouple_type",
> > >  };
> > >  
> > > +
> > > +void *iio_priv(const struct iio_dev *indio_dev)
> > > +{
> > > +	struct iio_dev_priv *iio_dev_priv = to_iio_dev_priv(indio_dev);
> > > +	return (char *)iio_dev_priv + ALIGN(sizeof(struct iio_dev_priv),
> > > IIO_ALIGN);
> > > +}
> > > +EXPORT_SYMBOL_GPL(iio_priv);
> > > +
> > > +struct iio_dev *iio_priv_to_dev(void *priv)
> > > +{
> > > +	struct iio_dev_priv *iio_dev_priv =
> > > +		(struct iio_dev_priv *)((char *)priv -
> > > +				  ALIGN(sizeof(struct iio_dev_priv),
> > > IIO_ALIGN));
> > > +	return &iio_dev_priv->indio_dev;
> > > +}
> > > +EXPORT_SYMBOL_GPL(iio_priv_to_dev);
> > > +
> > >  /**
> > >   * iio_find_channel_from_si() - get channel from its scan index
> > >   * @indio_dev:		device
> > > @@ -1476,6 +1493,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_priv *iio_dev_priv = to_iio_dev_priv(indio_dev);
> > > +
> > >  	if (indio_dev->modes & INDIO_ALL_TRIGGERED_MODES)
> > >  		iio_device_unregister_trigger_consumer(indio_dev);
> > >  	iio_device_unregister_eventset(indio_dev);
> > > @@ -1484,7 +1503,7 @@ static void iio_dev_release(struct device *device)
> > >  	iio_buffer_put(indio_dev->buffer);
> > >  
> > >  	ida_simple_remove(&iio_ida, indio_dev->id);
> > > -	kfree(indio_dev);
> > > +	kfree(iio_dev_priv);
> > >  }
> > >  
> > >  struct device_type iio_device_type = {
> > > @@ -1498,10 +1517,11 @@ struct device_type iio_device_type = {
> > >   **/
> > >  struct iio_dev *iio_device_alloc(int sizeof_priv)
> > >  {
> > > +	struct iio_dev_priv *iio_dev_priv;
> > >  	struct iio_dev *dev;
> > >  	size_t alloc_size;
> > >  
> > > -	alloc_size = sizeof(struct iio_dev);
> > > +	alloc_size = sizeof(struct iio_dev_priv);
> > >  	if (sizeof_priv) {
> > >  		alloc_size = ALIGN(alloc_size, IIO_ALIGN);
> > >  		alloc_size += sizeof_priv;
> > > @@ -1509,10 +1529,12 @@ struct iio_dev *iio_device_alloc(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_priv = kzalloc(alloc_size, GFP_KERNEL);
> > > +	if (!iio_dev_priv)
> > >  		return NULL;
> > >  
> > > +	dev = &iio_dev_priv->indio_dev;
> > > +
> > >  	dev->dev.groups = dev->groups;
> > >  	dev->dev.type = &iio_device_type;
> > >  	dev->dev.bus = &iio_bus_type;
> > > @@ -1526,7 +1548,7 @@ struct iio_dev *iio_device_alloc(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_priv);
> > >  		return NULL;
> > >  	}
> > >  	dev_set_name(&dev->dev, "iio:device%d", dev->id);
> > > diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> > > index 5f9f439a4f01..38c4ea505394 100644
> > > --- a/include/linux/iio/iio.h
> > > +++ b/include/linux/iio/iio.h
> > > @@ -678,16 +678,8 @@ static inline void *iio_device_get_drvdata(struct
> > > iio_dev *indio_dev)
> > >  #define IIO_ALIGN L1_CACHE_BYTES
> > >  struct iio_dev *iio_device_alloc(int sizeof_priv);
> > >  
> > > -static inline void *iio_priv(const struct iio_dev *indio_dev)
> > > -{
> > > -	return (char *)indio_dev + ALIGN(sizeof(struct iio_dev), IIO_ALIGN);
> > > -}
> > > -
> > > -static inline struct iio_dev *iio_priv_to_dev(void *priv)
> > > -{
> > > -	return (struct iio_dev *)((char *)priv -
> > > -				  ALIGN(sizeof(struct iio_dev), IIO_ALIGN));
> > > -}
> > > +void *iio_priv(const struct iio_dev *indio_dev);
> > > +struct iio_dev *iio_priv_to_dev(void *priv);
> > >  
> > >  void iio_device_free(struct iio_dev *indio_dev);
> > >  struct iio_dev *devm_iio_device_alloc(struct device *dev, int
> > > sizeof_priv);  
> 
> 

Re: [PATCH 1/3] iio: core: wrap IIO device into a iio_dev_priv object

[Jonathan Cameron]

On Mon, 11 May 2020 09:16:32 +0000
"Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote:

> On Fri, 2020-05-08 at 16:44 +0100, Jonathan Cameron wrote:
> > [External]
> > 
> > On Fri, 8 May 2020 16:40:15 +0100
> > Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
> >   
> > > On Fri, 8 May 2020 17:13:04 +0300
> > > Alexandru Ardelean <alexandru.ardelean@analog.com> 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'.    
> > > 
> > > This has been on the todo list for many years so great you are having a go.
> > >    
> > > > This is difficult, as a lot of users copy drivers, and 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_priv' which wraps a public
> > > > 'struct iio_dev' object.
> > > > 
> > > > In the next patches, 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_priv' object).
> > > > 
> > > > We also need to hide the implementations for 'iio_priv()' &
> > > > 'iio_priv_to_dev()', as the pointer arithmetic will not match once things
> > > > are moved.    
> > > 
> > > This last bit has the disadvantage of potentially putting a function
> > > call in some fast paths where there wasn't one before.  
> 
> Hmm.
> I think we can change this to have a void *ptr in the public iio.h header and
> that points to the private data in the private struct, and we compute it once,
> and just return it.
> 
> so:
> 
> struct iio_dev_opaque;
> 
> struct iio_dev {
>     struct iio_dev_opaque *opaque;  // compute this during iio_device_alloc()
>     void *priv;                     // compute this during iio_device_alloc()
> };
> 
> static inline void *iio_priv(const struct iio_dev *indio_dev)
> {
>         return indio_dev->priv;
> }
> 
> [ or even just convert it to a macro ; no preference ]

That works for me and avoids any of the messy stuff below.

Make sure to add a comment that the region that points to is guaranteed
to maintain cacheline alignment as thats hidden now.

> 
> > > 
> > > We may not need to 'forcefully' hide the internal parts.  May be
> > > enough to just make their use really obvious.  If you have to cast
> > > to an iio_dev_priv then you are doing something very wrong.  
> > 
> > Note, definitely keep the to_* macro only in the private core header.
> >   
> > > The old stick __ in front of it may make that even more obvious.
> > > 
> > > Naming is a bit confusing though.  iio_dev_priv sounds like private
> > > to the device... iio_dev_opaque maybe?  
> 
> regarding the 'opaque' part, that would be a 'struct iio_dev_opaque {' that
> lives somewhere like
> 
> struct iio_dev_opaque {
>     struct iio_dev   indio_dev;  // public IIO device part
>     ... // opaque IIO device parts
>     ... // iio_priv data
> }
> 
> The stick __ in front, may work just fine as well.
> 
> Personally, I would prefer to forcefully hide stuff, because then the compiler
> could also be useful in telling people not to use some private fields.
> People [juniors usually] listen to compilers more than they listen to review-
> comments. 
> We could move the 'struct iio_dev_opaque' in 'include/linux/iio/iio-opaque.h',
> so that if they need to access some fields for some advanced/hacky debugging, it
> should work as well.
> 
> Still, if it's more preferred to prefix them, I can do that as well.
> But we should prefix them as we manage to somehow make the more opaque, or
> remove the fields from places they should be used.
> Otherwise it is pointless, as people would copy 'indio_dev->__mlock' anyway.
> 
> 
> > > 
> > >   
> > > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> > > > ---
> > > > 
> > > > Just as a note here, I've been running this patchset without a problem
> > > > for 2 weeks now in a work branch.
> > > > But it's only been a setup, so no idea if some other thing may cause
> > > > bigger issues.
> > > > 
> > > > This small patchset is meant to kickstart this, for GSoC people or for
> > > > people wanting to start contributing to IIO.
> > > > 
> > > >  drivers/iio/iio_core.h          | 11 +++++++++++
> > > >  drivers/iio/industrialio-core.c | 32 +++++++++++++++++++++++++++-----
> > > >  include/linux/iio/iio.h         | 12 ++----------
> > > >  3 files changed, 40 insertions(+), 15 deletions(-)
> > > > 
> > > > diff --git a/drivers/iio/iio_core.h b/drivers/iio/iio_core.h
> > > > index fd9a5f1d5e51..84f3b4590c05 100644
> > > > --- a/drivers/iio/iio_core.h
> > > > +++ b/drivers/iio/iio_core.h
> > > > @@ -17,6 +17,17 @@ struct iio_dev;
> > > >  
> > > >  extern struct device_type iio_device_type;
> > > >  
> > > > +/**
> > > > + * struct iio_dev_priv - industrial I/O device private information
> > > > + * @indio_dev:			public IIO device object
> > > > + */
> > > > +struct iio_dev_priv {
> > > > +	struct iio_dev			indio_dev;
> > > > +};
> > > > +
> > > > +#define to_iio_dev_priv(indio_dev)	\
> > > > +	container_of(indio_dev, struct iio_dev_priv, indio_dev)
> > > > +
> > > >  int __iio_add_chan_devattr(const char *postfix,
> > > >  			   struct iio_chan_spec const *chan,
> > > >  			   ssize_t (*func)(struct device *dev,
> > > > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-
> > > > core.c
> > > > index 462d3e810013..f0888dd84d3d 100644
> > > > --- a/drivers/iio/industrialio-core.c
> > > > +++ b/drivers/iio/industrialio-core.c
> > > > @@ -164,6 +164,23 @@ static const char * const iio_chan_info_postfix[] = {
> > > >  	[IIO_CHAN_INFO_THERMOCOUPLE_TYPE] = "thermocouple_type",
> > > >  };
> > > >  
> > > > +
> > > > +void *iio_priv(const struct iio_dev *indio_dev)
> > > > +{
> > > > +	struct iio_dev_priv *iio_dev_priv = to_iio_dev_priv(indio_dev);
> > > > +	return (char *)iio_dev_priv + ALIGN(sizeof(struct iio_dev_priv),
> > > > IIO_ALIGN);
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(iio_priv);
> > > > +
> > > > +struct iio_dev *iio_priv_to_dev(void *priv)
> > > > +{
> > > > +	struct iio_dev_priv *iio_dev_priv =
> > > > +		(struct iio_dev_priv *)((char *)priv -
> > > > +				  ALIGN(sizeof(struct iio_dev_priv),
> > > > IIO_ALIGN));
> > > > +	return &iio_dev_priv->indio_dev;
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(iio_priv_to_dev);
> > > > +
> > > >  /**
> > > >   * iio_find_channel_from_si() - get channel from its scan index
> > > >   * @indio_dev:		device
> > > > @@ -1476,6 +1493,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_priv *iio_dev_priv = to_iio_dev_priv(indio_dev);
> > > > +
> > > >  	if (indio_dev->modes & INDIO_ALL_TRIGGERED_MODES)
> > > >  		iio_device_unregister_trigger_consumer(indio_dev);
> > > >  	iio_device_unregister_eventset(indio_dev);
> > > > @@ -1484,7 +1503,7 @@ static void iio_dev_release(struct device *device)
> > > >  	iio_buffer_put(indio_dev->buffer);
> > > >  
> > > >  	ida_simple_remove(&iio_ida, indio_dev->id);
> > > > -	kfree(indio_dev);
> > > > +	kfree(iio_dev_priv);
> > > >  }
> > > >  
> > > >  struct device_type iio_device_type = {
> > > > @@ -1498,10 +1517,11 @@ struct device_type iio_device_type = {
> > > >   **/
> > > >  struct iio_dev *iio_device_alloc(int sizeof_priv)
> > > >  {
> > > > +	struct iio_dev_priv *iio_dev_priv;
> > > >  	struct iio_dev *dev;
> > > >  	size_t alloc_size;
> > > >  
> > > > -	alloc_size = sizeof(struct iio_dev);
> > > > +	alloc_size = sizeof(struct iio_dev_priv);
> > > >  	if (sizeof_priv) {
> > > >  		alloc_size = ALIGN(alloc_size, IIO_ALIGN);
> > > >  		alloc_size += sizeof_priv;
> > > > @@ -1509,10 +1529,12 @@ struct iio_dev *iio_device_alloc(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_priv = kzalloc(alloc_size, GFP_KERNEL);
> > > > +	if (!iio_dev_priv)
> > > >  		return NULL;
> > > >  
> > > > +	dev = &iio_dev_priv->indio_dev;
> > > > +
> > > >  	dev->dev.groups = dev->groups;
> > > >  	dev->dev.type = &iio_device_type;
> > > >  	dev->dev.bus = &iio_bus_type;
> > > > @@ -1526,7 +1548,7 @@ struct iio_dev *iio_device_alloc(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_priv);
> > > >  		return NULL;
> > > >  	}
> > > >  	dev_set_name(&dev->dev, "iio:device%d", dev->id);
> > > > diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> > > > index 5f9f439a4f01..38c4ea505394 100644
> > > > --- a/include/linux/iio/iio.h
> > > > +++ b/include/linux/iio/iio.h
> > > > @@ -678,16 +678,8 @@ static inline void *iio_device_get_drvdata(struct
> > > > iio_dev *indio_dev)
> > > >  #define IIO_ALIGN L1_CACHE_BYTES
> > > >  struct iio_dev *iio_device_alloc(int sizeof_priv);
> > > >  
> > > > -static inline void *iio_priv(const struct iio_dev *indio_dev)
> > > > -{
> > > > -	return (char *)indio_dev + ALIGN(sizeof(struct iio_dev), IIO_ALIGN);
> > > > -}
> > > > -
> > > > -static inline struct iio_dev *iio_priv_to_dev(void *priv)
> > > > -{
> > > > -	return (struct iio_dev *)((char *)priv -
> > > > -				  ALIGN(sizeof(struct iio_dev), IIO_ALIGN));
> > > > -}
> > > > +void *iio_priv(const struct iio_dev *indio_dev);
> > > > +struct iio_dev *iio_priv_to_dev(void *priv);
> > > >  
> > > >  void iio_device_free(struct iio_dev *indio_dev);
> > > >  struct iio_dev *devm_iio_device_alloc(struct device *dev, int
> > > > sizeof_priv);    
> > 
> >   

Re: [PATCH 1/3] iio: core: wrap IIO device into a iio_dev_priv object

[Ardelean, Alexandru]

On Mon, 2020-05-11 at 18:42 +0100, Jonathan Cameron wrote:
> [External]
> 
> On Mon, 11 May 2020 09:16:32 +0000
> "Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote:
> 
> > On Fri, 2020-05-08 at 16:44 +0100, Jonathan Cameron wrote:
> > > [External]
> > > 
> > > On Fri, 8 May 2020 16:40:15 +0100
> > > Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
> > >   
> > > > On Fri, 8 May 2020 17:13:04 +0300
> > > > Alexandru Ardelean <alexandru.ardelean@analog.com> 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'.    
> > > > 
> > > > This has been on the todo list for many years so great you are having a
> > > > go.
> > > >    
> > > > > This is difficult, as a lot of users copy drivers, and 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_priv' which wraps a
> > > > > public
> > > > > 'struct iio_dev' object.
> > > > > 
> > > > > In the next patches, 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_priv' object).
> > > > > 
> > > > > We also need to hide the implementations for 'iio_priv()' &
> > > > > 'iio_priv_to_dev()', as the pointer arithmetic will not match once
> > > > > things
> > > > > are moved.    
> > > > 
> > > > This last bit has the disadvantage of potentially putting a function
> > > > call in some fast paths where there wasn't one before.  
> > 
> > Hmm.
> > I think we can change this to have a void *ptr in the public iio.h header
> > and
> > that points to the private data in the private struct, and we compute it
> > once,
> > and just return it.
> > 
> > so:
> > 
> > struct iio_dev_opaque;
> > 
> > struct iio_dev {
> >     struct iio_dev_opaque *opaque;  // compute this during
> > iio_device_alloc()
> >     void *priv;                     // compute this during
> > iio_device_alloc()
> > };
> > 
> > static inline void *iio_priv(const struct iio_dev *indio_dev)
> > {
> >         return indio_dev->priv;
> > }
> > 
> > [ or even just convert it to a macro ; no preference ]
> 
> That works for me and avoids any of the messy stuff below.
> 
> Make sure to add a comment that the region that points to is guaranteed
> to maintain cacheline alignment as thats hidden now.

1 note: I would still need to hide the iio_priv_to_dev() implementation.
It's a bit hard not to; or I can't come up with a good idea now, like for
iio_priv().
Thing is, not many drivers use it, and it looks a bit funky that they use it.
It looks like each driver should just keep (or use) a reference to the iio_dev
object, since it should be in the driver somewhere.

Maybe this series needs to be extended to clean-up those uses
of iio_priv_to_dev().

And a question; since I am not clear.
On the 'to_iio_dev_opaque()' : is it preferred to access it via 'indio_dev-
>opaque' or via pointer arithmentic?
i.e. the classic 
#define to_iio_dev_opaque(indio_dev)            \
        container_of(indio_dev, struct iio_dev_opaque, indio_dev)

either is fine from my side; i'll just do the macro, and then the implementation
can be changed as i get the answer;

> 
> > > > We may not need to 'forcefully' hide the internal parts.  May be
> > > > enough to just make their use really obvious.  If you have to cast
> > > > to an iio_dev_priv then you are doing something very wrong.  
> > > 
> > > Note, definitely keep the to_* macro only in the private core header.
> > >   
> > > > The old stick __ in front of it may make that even more obvious.
> > > > 
> > > > Naming is a bit confusing though.  iio_dev_priv sounds like private
> > > > to the device... iio_dev_opaque maybe?  
> > 
> > regarding the 'opaque' part, that would be a 'struct iio_dev_opaque {' that
> > lives somewhere like
> > 
> > struct iio_dev_opaque {
> >     struct iio_dev   indio_dev;  // public IIO device part
> >     ... // opaque IIO device parts
> >     ... // iio_priv data
> > }
> > 
> > The stick __ in front, may work just fine as well.
> > 
> > Personally, I would prefer to forcefully hide stuff, because then the
> > compiler
> > could also be useful in telling people not to use some private fields.
> > People [juniors usually] listen to compilers more than they listen to
> > review-
> > comments. 
> > We could move the 'struct iio_dev_opaque' in 'include/linux/iio/iio-
> > opaque.h',
> > so that if they need to access some fields for some advanced/hacky
> > debugging, it
> > should work as well.
> > 
> > Still, if it's more preferred to prefix them, I can do that as well.
> > But we should prefix them as we manage to somehow make the more opaque, or
> > remove the fields from places they should be used.
> > Otherwise it is pointless, as people would copy 'indio_dev->__mlock' anyway.
> > 
> > 
> > > >   
> > > > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> > > > > ---
> > > > > 
> > > > > Just as a note here, I've been running this patchset without a problem
> > > > > for 2 weeks now in a work branch.
> > > > > But it's only been a setup, so no idea if some other thing may cause
> > > > > bigger issues.
> > > > > 
> > > > > This small patchset is meant to kickstart this, for GSoC people or for
> > > > > people wanting to start contributing to IIO.
> > > > > 
> > > > >  drivers/iio/iio_core.h          | 11 +++++++++++
> > > > >  drivers/iio/industrialio-core.c | 32 +++++++++++++++++++++++++++-----
> > > > >  include/linux/iio/iio.h         | 12 ++----------
> > > > >  3 files changed, 40 insertions(+), 15 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/iio/iio_core.h b/drivers/iio/iio_core.h
> > > > > index fd9a5f1d5e51..84f3b4590c05 100644
> > > > > --- a/drivers/iio/iio_core.h
> > > > > +++ b/drivers/iio/iio_core.h
> > > > > @@ -17,6 +17,17 @@ struct iio_dev;
> > > > >  
> > > > >  extern struct device_type iio_device_type;
> > > > >  
> > > > > +/**
> > > > > + * struct iio_dev_priv - industrial I/O device private information
> > > > > + * @indio_dev:			public IIO device object
> > > > > + */
> > > > > +struct iio_dev_priv {
> > > > > +	struct iio_dev			indio_dev;
> > > > > +};
> > > > > +
> > > > > +#define to_iio_dev_priv(indio_dev)	\
> > > > > +	container_of(indio_dev, struct iio_dev_priv, indio_dev)
> > > > > +
> > > > >  int __iio_add_chan_devattr(const char *postfix,
> > > > >  			   struct iio_chan_spec const *chan,
> > > > >  			   ssize_t (*func)(struct device *dev,
> > > > > diff --git a/drivers/iio/industrialio-core.c
> > > > > b/drivers/iio/industrialio-
> > > > > core.c
> > > > > index 462d3e810013..f0888dd84d3d 100644
> > > > > --- a/drivers/iio/industrialio-core.c
> > > > > +++ b/drivers/iio/industrialio-core.c
> > > > > @@ -164,6 +164,23 @@ static const char * const iio_chan_info_postfix[]
> > > > > = {
> > > > >  	[IIO_CHAN_INFO_THERMOCOUPLE_TYPE] = "thermocouple_type",
> > > > >  };
> > > > >  
> > > > > +
> > > > > +void *iio_priv(const struct iio_dev *indio_dev)
> > > > > +{
> > > > > +	struct iio_dev_priv *iio_dev_priv = to_iio_dev_priv(indio_dev);
> > > > > +	return (char *)iio_dev_priv + ALIGN(sizeof(struct iio_dev_priv),
> > > > > IIO_ALIGN);
> > > > > +}
> > > > > +EXPORT_SYMBOL_GPL(iio_priv);
> > > > > +
> > > > > +struct iio_dev *iio_priv_to_dev(void *priv)
> > > > > +{
> > > > > +	struct iio_dev_priv *iio_dev_priv =
> > > > > +		(struct iio_dev_priv *)((char *)priv -
> > > > > +				  ALIGN(sizeof(struct iio_dev_priv),
> > > > > IIO_ALIGN));
> > > > > +	return &iio_dev_priv->indio_dev;
> > > > > +}
> > > > > +EXPORT_SYMBOL_GPL(iio_priv_to_dev);
> > > > > +
> > > > >  /**
> > > > >   * iio_find_channel_from_si() - get channel from its scan index
> > > > >   * @indio_dev:		device
> > > > > @@ -1476,6 +1493,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_priv *iio_dev_priv = to_iio_dev_priv(indio_dev);
> > > > > +
> > > > >  	if (indio_dev->modes & INDIO_ALL_TRIGGERED_MODES)
> > > > >  		iio_device_unregister_trigger_consumer(indio_dev);
> > > > >  	iio_device_unregister_eventset(indio_dev);
> > > > > @@ -1484,7 +1503,7 @@ static void iio_dev_release(struct device
> > > > > *device)
> > > > >  	iio_buffer_put(indio_dev->buffer);
> > > > >  
> > > > >  	ida_simple_remove(&iio_ida, indio_dev->id);
> > > > > -	kfree(indio_dev);
> > > > > +	kfree(iio_dev_priv);
> > > > >  }
> > > > >  
> > > > >  struct device_type iio_device_type = {
> > > > > @@ -1498,10 +1517,11 @@ struct device_type iio_device_type = {
> > > > >   **/
> > > > >  struct iio_dev *iio_device_alloc(int sizeof_priv)
> > > > >  {
> > > > > +	struct iio_dev_priv *iio_dev_priv;
> > > > >  	struct iio_dev *dev;
> > > > >  	size_t alloc_size;
> > > > >  
> > > > > -	alloc_size = sizeof(struct iio_dev);
> > > > > +	alloc_size = sizeof(struct iio_dev_priv);
> > > > >  	if (sizeof_priv) {
> > > > >  		alloc_size = ALIGN(alloc_size, IIO_ALIGN);
> > > > >  		alloc_size += sizeof_priv;
> > > > > @@ -1509,10 +1529,12 @@ struct iio_dev *iio_device_alloc(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_priv = kzalloc(alloc_size, GFP_KERNEL);
> > > > > +	if (!iio_dev_priv)
> > > > >  		return NULL;
> > > > >  
> > > > > +	dev = &iio_dev_priv->indio_dev;
> > > > > +
> > > > >  	dev->dev.groups = dev->groups;
> > > > >  	dev->dev.type = &iio_device_type;
> > > > >  	dev->dev.bus = &iio_bus_type;
> > > > > @@ -1526,7 +1548,7 @@ struct iio_dev *iio_device_alloc(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_priv);
> > > > >  		return NULL;
> > > > >  	}
> > > > >  	dev_set_name(&dev->dev, "iio:device%d", dev->id);
> > > > > diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> > > > > index 5f9f439a4f01..38c4ea505394 100644
> > > > > --- a/include/linux/iio/iio.h
> > > > > +++ b/include/linux/iio/iio.h
> > > > > @@ -678,16 +678,8 @@ static inline void *iio_device_get_drvdata(struct
> > > > > iio_dev *indio_dev)
> > > > >  #define IIO_ALIGN L1_CACHE_BYTES
> > > > >  struct iio_dev *iio_device_alloc(int sizeof_priv);
> > > > >  
> > > > > -static inline void *iio_priv(const struct iio_dev *indio_dev)
> > > > > -{
> > > > > -	return (char *)indio_dev + ALIGN(sizeof(struct iio_dev),
> > > > > IIO_ALIGN);
> > > > > -}
> > > > > -
> > > > > -static inline struct iio_dev *iio_priv_to_dev(void *priv)
> > > > > -{
> > > > > -	return (struct iio_dev *)((char *)priv -
> > > > > -				  ALIGN(sizeof(struct iio_dev),
> > > > > IIO_ALIGN));
> > > > > -}
> > > > > +void *iio_priv(const struct iio_dev *indio_dev);
> > > > > +struct iio_dev *iio_priv_to_dev(void *priv);
> > > > >  
> > > > >  void iio_device_free(struct iio_dev *indio_dev);
> > > > >  struct iio_dev *devm_iio_device_alloc(struct device *dev, int
> > > > > sizeof_priv);    
> > > 
> > >   
> 
> 

Re: [PATCH 1/3] iio: core: wrap IIO device into a iio_dev_priv object

[Jonathan Cameron]

On Tue, 12 May 2020 11:26:08 +0000
"Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote:

> On Mon, 2020-05-11 at 18:42 +0100, Jonathan Cameron wrote:
> > [External]
> > 
> > On Mon, 11 May 2020 09:16:32 +0000
> > "Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote:
> >   
> > > On Fri, 2020-05-08 at 16:44 +0100, Jonathan Cameron wrote:  
> > > > [External]
> > > > 
> > > > On Fri, 8 May 2020 16:40:15 +0100
> > > > Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
> > > >     
> > > > > On Fri, 8 May 2020 17:13:04 +0300
> > > > > Alexandru Ardelean <alexandru.ardelean@analog.com> 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'.      
> > > > > 
> > > > > This has been on the todo list for many years so great you are having a
> > > > > go.
> > > > >      
> > > > > > This is difficult, as a lot of users copy drivers, and 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_priv' which wraps a
> > > > > > public
> > > > > > 'struct iio_dev' object.
> > > > > > 
> > > > > > In the next patches, 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_priv' object).
> > > > > > 
> > > > > > We also need to hide the implementations for 'iio_priv()' &
> > > > > > 'iio_priv_to_dev()', as the pointer arithmetic will not match once
> > > > > > things
> > > > > > are moved.      
> > > > > 
> > > > > This last bit has the disadvantage of potentially putting a function
> > > > > call in some fast paths where there wasn't one before.    
> > > 
> > > Hmm.
> > > I think we can change this to have a void *ptr in the public iio.h header
> > > and
> > > that points to the private data in the private struct, and we compute it
> > > once,
> > > and just return it.
> > > 
> > > so:
> > > 
> > > struct iio_dev_opaque;
> > > 
> > > struct iio_dev {
> > >     struct iio_dev_opaque *opaque;  // compute this during
> > > iio_device_alloc()
> > >     void *priv;                     // compute this during
> > > iio_device_alloc()
> > > };
> > > 
> > > static inline void *iio_priv(const struct iio_dev *indio_dev)
> > > {
> > >         return indio_dev->priv;
> > > }
> > > 
> > > [ or even just convert it to a macro ; no preference ]  
> > 
> > That works for me and avoids any of the messy stuff below.
> > 
> > Make sure to add a comment that the region that points to is guaranteed
> > to maintain cacheline alignment as thats hidden now.  
> 
> 1 note: I would still need to hide the iio_priv_to_dev() implementation.
> It's a bit hard not to; or I can't come up with a good idea now, like for
> iio_priv().
> Thing is, not many drivers use it, and it looks a bit funky that they use it.
> It looks like each driver should just keep (or use) a reference to the iio_dev
> object, since it should be in the driver somewhere.
> 
> Maybe this series needs to be extended to clean-up those uses
> of iio_priv_to_dev().

I never liked that interface.  IIRC we removed it once before but
it snuck back in because one of those cases was really fiddly with out it
(I can't remember which though :(

> 
> And a question; since I am not clear.
> On the 'to_iio_dev_opaque()' : is it preferred to access it via 'indio_dev-
> >opaque' or via pointer arithmentic?  
> i.e. the classic 
> #define to_iio_dev_opaque(indio_dev)            \
>         container_of(indio_dev, struct iio_dev_opaque, indio_dev)

container of I think. Though I'm not quite sure what you mean by
pointer arithmetic as an alternative..

> 
> either is fine from my side; i'll just do the macro, and then the implementation
> can be changed as i get the answer;
> 
> >   
> > > > > We may not need to 'forcefully' hide the internal parts.  May be
> > > > > enough to just make their use really obvious.  If you have to cast
> > > > > to an iio_dev_priv then you are doing something very wrong.    
> > > > 
> > > > Note, definitely keep the to_* macro only in the private core header.
> > > >     
> > > > > The old stick __ in front of it may make that even more obvious.
> > > > > 
> > > > > Naming is a bit confusing though.  iio_dev_priv sounds like private
> > > > > to the device... iio_dev_opaque maybe?    
> > > 
> > > regarding the 'opaque' part, that would be a 'struct iio_dev_opaque {' that
> > > lives somewhere like
> > > 
> > > struct iio_dev_opaque {
> > >     struct iio_dev   indio_dev;  // public IIO device part
> > >     ... // opaque IIO device parts
> > >     ... // iio_priv data
> > > }
> > > 
> > > The stick __ in front, may work just fine as well.
> > > 
> > > Personally, I would prefer to forcefully hide stuff, because then the
> > > compiler
> > > could also be useful in telling people not to use some private fields.
> > > People [juniors usually] listen to compilers more than they listen to
> > > review-
> > > comments. 
> > > We could move the 'struct iio_dev_opaque' in 'include/linux/iio/iio-
> > > opaque.h',
> > > so that if they need to access some fields for some advanced/hacky
> > > debugging, it
> > > should work as well.
> > > 
> > > Still, if it's more preferred to prefix them, I can do that as well.
> > > But we should prefix them as we manage to somehow make the more opaque, or
> > > remove the fields from places they should be used.
> > > Otherwise it is pointless, as people would copy 'indio_dev->__mlock' anyway.
> > > 
> > >   
> > > > >     
> > > > > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> > > > > > ---
> > > > > > 
> > > > > > Just as a note here, I've been running this patchset without a problem
> > > > > > for 2 weeks now in a work branch.
> > > > > > But it's only been a setup, so no idea if some other thing may cause
> > > > > > bigger issues.
> > > > > > 
> > > > > > This small patchset is meant to kickstart this, for GSoC people or for
> > > > > > people wanting to start contributing to IIO.
> > > > > > 
> > > > > >  drivers/iio/iio_core.h          | 11 +++++++++++
> > > > > >  drivers/iio/industrialio-core.c | 32 +++++++++++++++++++++++++++-----
> > > > > >  include/linux/iio/iio.h         | 12 ++----------
> > > > > >  3 files changed, 40 insertions(+), 15 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/iio/iio_core.h b/drivers/iio/iio_core.h
> > > > > > index fd9a5f1d5e51..84f3b4590c05 100644
> > > > > > --- a/drivers/iio/iio_core.h
> > > > > > +++ b/drivers/iio/iio_core.h
> > > > > > @@ -17,6 +17,17 @@ struct iio_dev;
> > > > > >  
> > > > > >  extern struct device_type iio_device_type;
> > > > > >  
> > > > > > +/**
> > > > > > + * struct iio_dev_priv - industrial I/O device private information
> > > > > > + * @indio_dev:			public IIO device object
> > > > > > + */
> > > > > > +struct iio_dev_priv {
> > > > > > +	struct iio_dev			indio_dev;
> > > > > > +};
> > > > > > +
> > > > > > +#define to_iio_dev_priv(indio_dev)	\
> > > > > > +	container_of(indio_dev, struct iio_dev_priv, indio_dev)
> > > > > > +
> > > > > >  int __iio_add_chan_devattr(const char *postfix,
> > > > > >  			   struct iio_chan_spec const *chan,
> > > > > >  			   ssize_t (*func)(struct device *dev,
> > > > > > diff --git a/drivers/iio/industrialio-core.c
> > > > > > b/drivers/iio/industrialio-
> > > > > > core.c
> > > > > > index 462d3e810013..f0888dd84d3d 100644
> > > > > > --- a/drivers/iio/industrialio-core.c
> > > > > > +++ b/drivers/iio/industrialio-core.c
> > > > > > @@ -164,6 +164,23 @@ static const char * const iio_chan_info_postfix[]
> > > > > > = {
> > > > > >  	[IIO_CHAN_INFO_THERMOCOUPLE_TYPE] = "thermocouple_type",
> > > > > >  };
> > > > > >  
> > > > > > +
> > > > > > +void *iio_priv(const struct iio_dev *indio_dev)
> > > > > > +{
> > > > > > +	struct iio_dev_priv *iio_dev_priv = to_iio_dev_priv(indio_dev);
> > > > > > +	return (char *)iio_dev_priv + ALIGN(sizeof(struct iio_dev_priv),
> > > > > > IIO_ALIGN);
> > > > > > +}
> > > > > > +EXPORT_SYMBOL_GPL(iio_priv);
> > > > > > +
> > > > > > +struct iio_dev *iio_priv_to_dev(void *priv)
> > > > > > +{
> > > > > > +	struct iio_dev_priv *iio_dev_priv =
> > > > > > +		(struct iio_dev_priv *)((char *)priv -
> > > > > > +				  ALIGN(sizeof(struct iio_dev_priv),
> > > > > > IIO_ALIGN));
> > > > > > +	return &iio_dev_priv->indio_dev;
> > > > > > +}
> > > > > > +EXPORT_SYMBOL_GPL(iio_priv_to_dev);
> > > > > > +
> > > > > >  /**
> > > > > >   * iio_find_channel_from_si() - get channel from its scan index
> > > > > >   * @indio_dev:		device
> > > > > > @@ -1476,6 +1493,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_priv *iio_dev_priv = to_iio_dev_priv(indio_dev);
> > > > > > +
> > > > > >  	if (indio_dev->modes & INDIO_ALL_TRIGGERED_MODES)
> > > > > >  		iio_device_unregister_trigger_consumer(indio_dev);
> > > > > >  	iio_device_unregister_eventset(indio_dev);
> > > > > > @@ -1484,7 +1503,7 @@ static void iio_dev_release(struct device
> > > > > > *device)
> > > > > >  	iio_buffer_put(indio_dev->buffer);
> > > > > >  
> > > > > >  	ida_simple_remove(&iio_ida, indio_dev->id);
> > > > > > -	kfree(indio_dev);
> > > > > > +	kfree(iio_dev_priv);
> > > > > >  }
> > > > > >  
> > > > > >  struct device_type iio_device_type = {
> > > > > > @@ -1498,10 +1517,11 @@ struct device_type iio_device_type = {
> > > > > >   **/
> > > > > >  struct iio_dev *iio_device_alloc(int sizeof_priv)
> > > > > >  {
> > > > > > +	struct iio_dev_priv *iio_dev_priv;
> > > > > >  	struct iio_dev *dev;
> > > > > >  	size_t alloc_size;
> > > > > >  
> > > > > > -	alloc_size = sizeof(struct iio_dev);
> > > > > > +	alloc_size = sizeof(struct iio_dev_priv);
> > > > > >  	if (sizeof_priv) {
> > > > > >  		alloc_size = ALIGN(alloc_size, IIO_ALIGN);
> > > > > >  		alloc_size += sizeof_priv;
> > > > > > @@ -1509,10 +1529,12 @@ struct iio_dev *iio_device_alloc(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_priv = kzalloc(alloc_size, GFP_KERNEL);
> > > > > > +	if (!iio_dev_priv)
> > > > > >  		return NULL;
> > > > > >  
> > > > > > +	dev = &iio_dev_priv->indio_dev;
> > > > > > +
> > > > > >  	dev->dev.groups = dev->groups;
> > > > > >  	dev->dev.type = &iio_device_type;
> > > > > >  	dev->dev.bus = &iio_bus_type;
> > > > > > @@ -1526,7 +1548,7 @@ struct iio_dev *iio_device_alloc(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_priv);
> > > > > >  		return NULL;
> > > > > >  	}
> > > > > >  	dev_set_name(&dev->dev, "iio:device%d", dev->id);
> > > > > > diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> > > > > > index 5f9f439a4f01..38c4ea505394 100644
> > > > > > --- a/include/linux/iio/iio.h
> > > > > > +++ b/include/linux/iio/iio.h
> > > > > > @@ -678,16 +678,8 @@ static inline void *iio_device_get_drvdata(struct
> > > > > > iio_dev *indio_dev)
> > > > > >  #define IIO_ALIGN L1_CACHE_BYTES
> > > > > >  struct iio_dev *iio_device_alloc(int sizeof_priv);
> > > > > >  
> > > > > > -static inline void *iio_priv(const struct iio_dev *indio_dev)
> > > > > > -{
> > > > > > -	return (char *)indio_dev + ALIGN(sizeof(struct iio_dev),
> > > > > > IIO_ALIGN);
> > > > > > -}
> > > > > > -
> > > > > > -static inline struct iio_dev *iio_priv_to_dev(void *priv)
> > > > > > -{
> > > > > > -	return (struct iio_dev *)((char *)priv -
> > > > > > -				  ALIGN(sizeof(struct iio_dev),
> > > > > > IIO_ALIGN));
> > > > > > -}
> > > > > > +void *iio_priv(const struct iio_dev *indio_dev);
> > > > > > +struct iio_dev *iio_priv_to_dev(void *priv);
> > > > > >  
> > > > > >  void iio_device_free(struct iio_dev *indio_dev);
> > > > > >  struct iio_dev *devm_iio_device_alloc(struct device *dev, int
> > > > > > sizeof_priv);      
> > > > 
> > > >     
> > 
> >   

Re: [PATCH 1/3] iio: core: wrap IIO device into a iio_dev_priv object

[Ardelean, Alexandru]

On Sat, 2020-05-16 at 17:29 +0100, Jonathan Cameron wrote:
> [External]
> 
> On Tue, 12 May 2020 11:26:08 +0000
> "Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote:
> 
> > On Mon, 2020-05-11 at 18:42 +0100, Jonathan Cameron wrote:
> > > [External]
> > > 
> > > On Mon, 11 May 2020 09:16:32 +0000
> > > "Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote:
> > >   
> > > > On Fri, 2020-05-08 at 16:44 +0100, Jonathan Cameron wrote:  
> > > > > [External]
> > > > > 
> > > > > On Fri, 8 May 2020 16:40:15 +0100
> > > > > Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
> > > > >     
> > > > > > On Fri, 8 May 2020 17:13:04 +0300
> > > > > > Alexandru Ardelean <alexandru.ardelean@analog.com> 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'.      
> > > > > > 
> > > > > > This has been on the todo list for many years so great you are
> > > > > > having a
> > > > > > go.
> > > > > >      
> > > > > > > This is difficult, as a lot of users copy drivers, and 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_priv' which wraps a
> > > > > > > public
> > > > > > > 'struct iio_dev' object.
> > > > > > > 
> > > > > > > In the next patches, 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_priv' object).
> > > > > > > 
> > > > > > > We also need to hide the implementations for 'iio_priv()' &
> > > > > > > 'iio_priv_to_dev()', as the pointer arithmetic will not match once
> > > > > > > things
> > > > > > > are moved.      
> > > > > > 
> > > > > > This last bit has the disadvantage of potentially putting a function
> > > > > > call in some fast paths where there wasn't one before.    
> > > > 
> > > > Hmm.
> > > > I think we can change this to have a void *ptr in the public iio.h
> > > > header
> > > > and
> > > > that points to the private data in the private struct, and we compute it
> > > > once,
> > > > and just return it.
> > > > 
> > > > so:
> > > > 
> > > > struct iio_dev_opaque;
> > > > 
> > > > struct iio_dev {
> > > >     struct iio_dev_opaque *opaque;  // compute this during
> > > > iio_device_alloc()
> > > >     void *priv;                     // compute this during
> > > > iio_device_alloc()
> > > > };
> > > > 
> > > > static inline void *iio_priv(const struct iio_dev *indio_dev)
> > > > {
> > > >         return indio_dev->priv;
> > > > }
> > > > 
> > > > [ or even just convert it to a macro ; no preference ]  
> > > 
> > > That works for me and avoids any of the messy stuff below.
> > > 
> > > Make sure to add a comment that the region that points to is guaranteed
> > > to maintain cacheline alignment as thats hidden now.  
> > 
> > 1 note: I would still need to hide the iio_priv_to_dev() implementation.
> > It's a bit hard not to; or I can't come up with a good idea now, like for
> > iio_priv().
> > Thing is, not many drivers use it, and it looks a bit funky that they use
> > it.
> > It looks like each driver should just keep (or use) a reference to the
> > iio_dev
> > object, since it should be in the driver somewhere.
> > 
> > Maybe this series needs to be extended to clean-up those uses
> > of iio_priv_to_dev().
> 
> I never liked that interface.  IIRC we removed it once before but
> it snuck back in because one of those cases was really fiddly with out it
> (I can't remember which though :(
> 
> > And a question; since I am not clear.
> > On the 'to_iio_dev_opaque()' : is it preferred to access it via 'indio_dev-
> > > opaque' or via pointer arithmentic?  
> > i.e. the classic 
> > #define to_iio_dev_opaque(indio_dev)            \
> >         container_of(indio_dev, struct iio_dev_opaque, indio_dev)
> 
> container of I think. Though I'm not quite sure what you mean by
> pointer arithmetic as an alternative..

I was referring to container_of as pointer-arithmetic.

> 
> > either is fine from my side; i'll just do the macro, and then the
> > implementation
> > can be changed as i get the answer;
> > 
> > >   
> > > > > > We may not need to 'forcefully' hide the internal parts.  May be
> > > > > > enough to just make their use really obvious.  If you have to cast
> > > > > > to an iio_dev_priv then you are doing something very wrong.    
> > > > > 
> > > > > Note, definitely keep the to_* macro only in the private core header.
> > > > >     
> > > > > > The old stick __ in front of it may make that even more obvious.
> > > > > > 
> > > > > > Naming is a bit confusing though.  iio_dev_priv sounds like private
> > > > > > to the device... iio_dev_opaque maybe?    
> > > > 
> > > > regarding the 'opaque' part, that would be a 'struct iio_dev_opaque {'
> > > > that
> > > > lives somewhere like
> > > > 
> > > > struct iio_dev_opaque {
> > > >     struct iio_dev   indio_dev;  // public IIO device part
> > > >     ... // opaque IIO device parts
> > > >     ... // iio_priv data
> > > > }
> > > > 
> > > > The stick __ in front, may work just fine as well.
> > > > 
> > > > Personally, I would prefer to forcefully hide stuff, because then the
> > > > compiler
> > > > could also be useful in telling people not to use some private fields.
> > > > People [juniors usually] listen to compilers more than they listen to
> > > > review-
> > > > comments. 
> > > > We could move the 'struct iio_dev_opaque' in 'include/linux/iio/iio-
> > > > opaque.h',
> > > > so that if they need to access some fields for some advanced/hacky
> > > > debugging, it
> > > > should work as well.
> > > > 
> > > > Still, if it's more preferred to prefix them, I can do that as well.
> > > > But we should prefix them as we manage to somehow make the more opaque,
> > > > or
> > > > remove the fields from places they should be used.
> > > > Otherwise it is pointless, as people would copy 'indio_dev->__mlock'
> > > > anyway.
> > > > 
> > > >   
> > > > > >     
> > > > > > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> > > > > > > ---
> > > > > > > 
> > > > > > > Just as a note here, I've been running this patchset without a
> > > > > > > problem
> > > > > > > for 2 weeks now in a work branch.
> > > > > > > But it's only been a setup, so no idea if some other thing may
> > > > > > > cause
> > > > > > > bigger issues.
> > > > > > > 
> > > > > > > This small patchset is meant to kickstart this, for GSoC people or
> > > > > > > for
> > > > > > > people wanting to start contributing to IIO.
> > > > > > > 
> > > > > > >  drivers/iio/iio_core.h          | 11 +++++++++++
> > > > > > >  drivers/iio/industrialio-core.c | 32 +++++++++++++++++++++++++++-
> > > > > > > ----
> > > > > > >  include/linux/iio/iio.h         | 12 ++----------
> > > > > > >  3 files changed, 40 insertions(+), 15 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/iio/iio_core.h b/drivers/iio/iio_core.h
> > > > > > > index fd9a5f1d5e51..84f3b4590c05 100644
> > > > > > > --- a/drivers/iio/iio_core.h
> > > > > > > +++ b/drivers/iio/iio_core.h
> > > > > > > @@ -17,6 +17,17 @@ struct iio_dev;
> > > > > > >  
> > > > > > >  extern struct device_type iio_device_type;
> > > > > > >  
> > > > > > > +/**
> > > > > > > + * struct iio_dev_priv - industrial I/O device private
> > > > > > > information
> > > > > > > + * @indio_dev:			public IIO device object
> > > > > > > + */
> > > > > > > +struct iio_dev_priv {
> > > > > > > +	struct iio_dev			indio_dev;
> > > > > > > +};
> > > > > > > +
> > > > > > > +#define to_iio_dev_priv(indio_dev)	\
> > > > > > > +	container_of(indio_dev, struct iio_dev_priv, indio_dev)
> > > > > > > +
> > > > > > >  int __iio_add_chan_devattr(const char *postfix,
> > > > > > >  			   struct iio_chan_spec const *chan,
> > > > > > >  			   ssize_t (*func)(struct device *dev,
> > > > > > > diff --git a/drivers/iio/industrialio-core.c
> > > > > > > b/drivers/iio/industrialio-
> > > > > > > core.c
> > > > > > > index 462d3e810013..f0888dd84d3d 100644
> > > > > > > --- a/drivers/iio/industrialio-core.c
> > > > > > > +++ b/drivers/iio/industrialio-core.c
> > > > > > > @@ -164,6 +164,23 @@ static const char * const
> > > > > > > iio_chan_info_postfix[]
> > > > > > > = {
> > > > > > >  	[IIO_CHAN_INFO_THERMOCOUPLE_TYPE] = "thermocouple_type",
> > > > > > >  };
> > > > > > >  
> > > > > > > +
> > > > > > > +void *iio_priv(const struct iio_dev *indio_dev)
> > > > > > > +{
> > > > > > > +	struct iio_dev_priv *iio_dev_priv = to_iio_dev_priv(indio_dev);
> > > > > > > +	return (char *)iio_dev_priv + ALIGN(sizeof(struct iio_dev_priv),
> > > > > > > IIO_ALIGN);
> > > > > > > +}
> > > > > > > +EXPORT_SYMBOL_GPL(iio_priv);
> > > > > > > +
> > > > > > > +struct iio_dev *iio_priv_to_dev(void *priv)
> > > > > > > +{
> > > > > > > +	struct iio_dev_priv *iio_dev_priv =
> > > > > > > +		(struct iio_dev_priv *)((char *)priv -
> > > > > > > +				  ALIGN(sizeof(struct iio_dev_priv),
> > > > > > > IIO_ALIGN));
> > > > > > > +	return &iio_dev_priv->indio_dev;
> > > > > > > +}
> > > > > > > +EXPORT_SYMBOL_GPL(iio_priv_to_dev);
> > > > > > > +
> > > > > > >  /**
> > > > > > >   * iio_find_channel_from_si() - get channel from its scan index
> > > > > > >   * @indio_dev:		device
> > > > > > > @@ -1476,6 +1493,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_priv *iio_dev_priv = to_iio_dev_priv(indio_dev);
> > > > > > > +
> > > > > > >  	if (indio_dev->modes & INDIO_ALL_TRIGGERED_MODES)
> > > > > > >  		iio_device_unregister_trigger_consumer(indio_dev);
> > > > > > >  	iio_device_unregister_eventset(indio_dev);
> > > > > > > @@ -1484,7 +1503,7 @@ static void iio_dev_release(struct device
> > > > > > > *device)
> > > > > > >  	iio_buffer_put(indio_dev->buffer);
> > > > > > >  
> > > > > > >  	ida_simple_remove(&iio_ida, indio_dev->id);
> > > > > > > -	kfree(indio_dev);
> > > > > > > +	kfree(iio_dev_priv);
> > > > > > >  }
> > > > > > >  
> > > > > > >  struct device_type iio_device_type = {
> > > > > > > @@ -1498,10 +1517,11 @@ struct device_type iio_device_type = {
> > > > > > >   **/
> > > > > > >  struct iio_dev *iio_device_alloc(int sizeof_priv)
> > > > > > >  {
> > > > > > > +	struct iio_dev_priv *iio_dev_priv;
> > > > > > >  	struct iio_dev *dev;
> > > > > > >  	size_t alloc_size;
> > > > > > >  
> > > > > > > -	alloc_size = sizeof(struct iio_dev);
> > > > > > > +	alloc_size = sizeof(struct iio_dev_priv);
> > > > > > >  	if (sizeof_priv) {
> > > > > > >  		alloc_size = ALIGN(alloc_size, IIO_ALIGN);
> > > > > > >  		alloc_size += sizeof_priv;
> > > > > > > @@ -1509,10 +1529,12 @@ struct iio_dev *iio_device_alloc(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_priv = kzalloc(alloc_size, GFP_KERNEL);
> > > > > > > +	if (!iio_dev_priv)
> > > > > > >  		return NULL;
> > > > > > >  
> > > > > > > +	dev = &iio_dev_priv->indio_dev;
> > > > > > > +
> > > > > > >  	dev->dev.groups = dev->groups;
> > > > > > >  	dev->dev.type = &iio_device_type;
> > > > > > >  	dev->dev.bus = &iio_bus_type;
> > > > > > > @@ -1526,7 +1548,7 @@ struct iio_dev *iio_device_alloc(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_priv);
> > > > > > >  		return NULL;
> > > > > > >  	}
> > > > > > >  	dev_set_name(&dev->dev, "iio:device%d", dev->id);
> > > > > > > diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> > > > > > > index 5f9f439a4f01..38c4ea505394 100644
> > > > > > > --- a/include/linux/iio/iio.h
> > > > > > > +++ b/include/linux/iio/iio.h
> > > > > > > @@ -678,16 +678,8 @@ static inline void
> > > > > > > *iio_device_get_drvdata(struct
> > > > > > > iio_dev *indio_dev)
> > > > > > >  #define IIO_ALIGN L1_CACHE_BYTES
> > > > > > >  struct iio_dev *iio_device_alloc(int sizeof_priv);
> > > > > > >  
> > > > > > > -static inline void *iio_priv(const struct iio_dev *indio_dev)
> > > > > > > -{
> > > > > > > -	return (char *)indio_dev + ALIGN(sizeof(struct iio_dev),
> > > > > > > IIO_ALIGN);
> > > > > > > -}
> > > > > > > -
> > > > > > > -static inline struct iio_dev *iio_priv_to_dev(void *priv)
> > > > > > > -{
> > > > > > > -	return (struct iio_dev *)((char *)priv -
> > > > > > > -				  ALIGN(sizeof(struct iio_dev),
> > > > > > > IIO_ALIGN));
> > > > > > > -}
> > > > > > > +void *iio_priv(const struct iio_dev *indio_dev);
> > > > > > > +struct iio_dev *iio_priv_to_dev(void *priv);
> > > > > > >  
> > > > > > >  void iio_device_free(struct iio_dev *indio_dev);
> > > > > > >  struct iio_dev *devm_iio_device_alloc(struct device *dev, int
> > > > > > > sizeof_priv);      
> > > > > 
> > > > >     
> > > 
> > >   

Re: [PATCH 1/3] iio: core: wrap IIO device into a iio_dev_priv object

[Jonathan Cameron]

On Mon, 18 May 2020 08:07:21 +0000
"Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote:

> On Sat, 2020-05-16 at 17:29 +0100, Jonathan Cameron wrote:
> > [External]
> > 
> > On Tue, 12 May 2020 11:26:08 +0000
> > "Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote:
> >   
> > > On Mon, 2020-05-11 at 18:42 +0100, Jonathan Cameron wrote:  
> > > > [External]
> > > > 
> > > > On Mon, 11 May 2020 09:16:32 +0000
> > > > "Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote:
> > > >     
> > > > > On Fri, 2020-05-08 at 16:44 +0100, Jonathan Cameron wrote:    
> > > > > > [External]
> > > > > > 
> > > > > > On Fri, 8 May 2020 16:40:15 +0100
> > > > > > Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
> > > > > >       
> > > > > > > On Fri, 8 May 2020 17:13:04 +0300
> > > > > > > Alexandru Ardelean <alexandru.ardelean@analog.com> 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'.        
> > > > > > > 
> > > > > > > This has been on the todo list for many years so great you are
> > > > > > > having a
> > > > > > > go.
> > > > > > >        
> > > > > > > > This is difficult, as a lot of users copy drivers, and 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_priv' which wraps a
> > > > > > > > public
> > > > > > > > 'struct iio_dev' object.
> > > > > > > > 
> > > > > > > > In the next patches, 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_priv' object).
> > > > > > > > 
> > > > > > > > We also need to hide the implementations for 'iio_priv()' &
> > > > > > > > 'iio_priv_to_dev()', as the pointer arithmetic will not match once
> > > > > > > > things
> > > > > > > > are moved.        
> > > > > > > 
> > > > > > > This last bit has the disadvantage of potentially putting a function
> > > > > > > call in some fast paths where there wasn't one before.      
> > > > > 
> > > > > Hmm.
> > > > > I think we can change this to have a void *ptr in the public iio.h
> > > > > header
> > > > > and
> > > > > that points to the private data in the private struct, and we compute it
> > > > > once,
> > > > > and just return it.
> > > > > 
> > > > > so:
> > > > > 
> > > > > struct iio_dev_opaque;
> > > > > 
> > > > > struct iio_dev {
> > > > >     struct iio_dev_opaque *opaque;  // compute this during
> > > > > iio_device_alloc()
> > > > >     void *priv;                     // compute this during
> > > > > iio_device_alloc()
> > > > > };
> > > > > 
> > > > > static inline void *iio_priv(const struct iio_dev *indio_dev)
> > > > > {
> > > > >         return indio_dev->priv;
> > > > > }
> > > > > 
> > > > > [ or even just convert it to a macro ; no preference ]    
> > > > 
> > > > That works for me and avoids any of the messy stuff below.
> > > > 
> > > > Make sure to add a comment that the region that points to is guaranteed
> > > > to maintain cacheline alignment as thats hidden now.    
> > > 
> > > 1 note: I would still need to hide the iio_priv_to_dev() implementation.
> > > It's a bit hard not to; or I can't come up with a good idea now, like for
> > > iio_priv().
> > > Thing is, not many drivers use it, and it looks a bit funky that they use
> > > it.
> > > It looks like each driver should just keep (or use) a reference to the
> > > iio_dev
> > > object, since it should be in the driver somewhere.
> > > 
> > > Maybe this series needs to be extended to clean-up those uses
> > > of iio_priv_to_dev().  
> > 
> > I never liked that interface.  IIRC we removed it once before but
> > it snuck back in because one of those cases was really fiddly with out it
> > (I can't remember which though :(
> >   
> > > And a question; since I am not clear.
> > > On the 'to_iio_dev_opaque()' : is it preferred to access it via 'indio_dev-  
> > > > opaque' or via pointer arithmentic?    
> > > i.e. the classic 
> > > #define to_iio_dev_opaque(indio_dev)            \
> > >         container_of(indio_dev, struct iio_dev_opaque, indio_dev)  
> > 
> > container of I think. Though I'm not quite sure what you mean by
> > pointer arithmetic as an alternative..  
> 
> I was referring to container_of as pointer-arithmetic.

Ah. Got you. Given we aren't moving things into opaque until only
the core is accessing them, I think we should use the container_of
approach for this one.

> 
> >   
> > > either is fine from my side; i'll just do the macro, and then the
> > > implementation
> > > can be changed as i get the answer;
> > >   
> > > >     
> > > > > > > We may not need to 'forcefully' hide the internal parts.  May be
> > > > > > > enough to just make their use really obvious.  If you have to cast
> > > > > > > to an iio_dev_priv then you are doing something very wrong.      
> > > > > > 
> > > > > > Note, definitely keep the to_* macro only in the private core header.
> > > > > >       
> > > > > > > The old stick __ in front of it may make that even more obvious.
> > > > > > > 
> > > > > > > Naming is a bit confusing though.  iio_dev_priv sounds like private
> > > > > > > to the device... iio_dev_opaque maybe?      
> > > > > 
> > > > > regarding the 'opaque' part, that would be a 'struct iio_dev_opaque {'
> > > > > that
> > > > > lives somewhere like
> > > > > 
> > > > > struct iio_dev_opaque {
> > > > >     struct iio_dev   indio_dev;  // public IIO device part
> > > > >     ... // opaque IIO device parts
> > > > >     ... // iio_priv data
> > > > > }
> > > > > 
> > > > > The stick __ in front, may work just fine as well.
> > > > > 
> > > > > Personally, I would prefer to forcefully hide stuff, because then the
> > > > > compiler
> > > > > could also be useful in telling people not to use some private fields.
> > > > > People [juniors usually] listen to compilers more than they listen to
> > > > > review-
> > > > > comments. 
> > > > > We could move the 'struct iio_dev_opaque' in 'include/linux/iio/iio-
> > > > > opaque.h',
> > > > > so that if they need to access some fields for some advanced/hacky
> > > > > debugging, it
> > > > > should work as well.
> > > > > 
> > > > > Still, if it's more preferred to prefix them, I can do that as well.
> > > > > But we should prefix them as we manage to somehow make the more opaque,
> > > > > or
> > > > > remove the fields from places they should be used.
> > > > > Otherwise it is pointless, as people would copy 'indio_dev->__mlock'
> > > > > anyway.
> > > > > 
> > > > >     
> > > > > > >       
> > > > > > > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> > > > > > > > ---
> > > > > > > > 
> > > > > > > > Just as a note here, I've been running this patchset without a
> > > > > > > > problem
> > > > > > > > for 2 weeks now in a work branch.
> > > > > > > > But it's only been a setup, so no idea if some other thing may
> > > > > > > > cause
> > > > > > > > bigger issues.
> > > > > > > > 
> > > > > > > > This small patchset is meant to kickstart this, for GSoC people or
> > > > > > > > for
> > > > > > > > people wanting to start contributing to IIO.
> > > > > > > > 
> > > > > > > >  drivers/iio/iio_core.h          | 11 +++++++++++
> > > > > > > >  drivers/iio/industrialio-core.c | 32 +++++++++++++++++++++++++++-
> > > > > > > > ----
> > > > > > > >  include/linux/iio/iio.h         | 12 ++----------
> > > > > > > >  3 files changed, 40 insertions(+), 15 deletions(-)
> > > > > > > > 
> > > > > > > > diff --git a/drivers/iio/iio_core.h b/drivers/iio/iio_core.h
> > > > > > > > index fd9a5f1d5e51..84f3b4590c05 100644
> > > > > > > > --- a/drivers/iio/iio_core.h
> > > > > > > > +++ b/drivers/iio/iio_core.h
> > > > > > > > @@ -17,6 +17,17 @@ struct iio_dev;
> > > > > > > >  
> > > > > > > >  extern struct device_type iio_device_type;
> > > > > > > >  
> > > > > > > > +/**
> > > > > > > > + * struct iio_dev_priv - industrial I/O device private
> > > > > > > > information
> > > > > > > > + * @indio_dev:			public IIO device object
> > > > > > > > + */
> > > > > > > > +struct iio_dev_priv {
> > > > > > > > +	struct iio_dev			indio_dev;
> > > > > > > > +};
> > > > > > > > +
> > > > > > > > +#define to_iio_dev_priv(indio_dev)	\
> > > > > > > > +	container_of(indio_dev, struct iio_dev_priv, indio_dev)
> > > > > > > > +
> > > > > > > >  int __iio_add_chan_devattr(const char *postfix,
> > > > > > > >  			   struct iio_chan_spec const *chan,
> > > > > > > >  			   ssize_t (*func)(struct device *dev,
> > > > > > > > diff --git a/drivers/iio/industrialio-core.c
> > > > > > > > b/drivers/iio/industrialio-
> > > > > > > > core.c
> > > > > > > > index 462d3e810013..f0888dd84d3d 100644
> > > > > > > > --- a/drivers/iio/industrialio-core.c
> > > > > > > > +++ b/drivers/iio/industrialio-core.c
> > > > > > > > @@ -164,6 +164,23 @@ static const char * const
> > > > > > > > iio_chan_info_postfix[]
> > > > > > > > = {
> > > > > > > >  	[IIO_CHAN_INFO_THERMOCOUPLE_TYPE] = "thermocouple_type",
> > > > > > > >  };
> > > > > > > >  
> > > > > > > > +
> > > > > > > > +void *iio_priv(const struct iio_dev *indio_dev)
> > > > > > > > +{
> > > > > > > > +	struct iio_dev_priv *iio_dev_priv = to_iio_dev_priv(indio_dev);
> > > > > > > > +	return (char *)iio_dev_priv + ALIGN(sizeof(struct iio_dev_priv),
> > > > > > > > IIO_ALIGN);
> > > > > > > > +}
> > > > > > > > +EXPORT_SYMBOL_GPL(iio_priv);
> > > > > > > > +
> > > > > > > > +struct iio_dev *iio_priv_to_dev(void *priv)
> > > > > > > > +{
> > > > > > > > +	struct iio_dev_priv *iio_dev_priv =
> > > > > > > > +		(struct iio_dev_priv *)((char *)priv -
> > > > > > > > +				  ALIGN(sizeof(struct iio_dev_priv),
> > > > > > > > IIO_ALIGN));
> > > > > > > > +	return &iio_dev_priv->indio_dev;
> > > > > > > > +}
> > > > > > > > +EXPORT_SYMBOL_GPL(iio_priv_to_dev);
> > > > > > > > +
> > > > > > > >  /**
> > > > > > > >   * iio_find_channel_from_si() - get channel from its scan index
> > > > > > > >   * @indio_dev:		device
> > > > > > > > @@ -1476,6 +1493,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_priv *iio_dev_priv = to_iio_dev_priv(indio_dev);
> > > > > > > > +
> > > > > > > >  	if (indio_dev->modes & INDIO_ALL_TRIGGERED_MODES)
> > > > > > > >  		iio_device_unregister_trigger_consumer(indio_dev);
> > > > > > > >  	iio_device_unregister_eventset(indio_dev);
> > > > > > > > @@ -1484,7 +1503,7 @@ static void iio_dev_release(struct device
> > > > > > > > *device)
> > > > > > > >  	iio_buffer_put(indio_dev->buffer);
> > > > > > > >  
> > > > > > > >  	ida_simple_remove(&iio_ida, indio_dev->id);
> > > > > > > > -	kfree(indio_dev);
> > > > > > > > +	kfree(iio_dev_priv);
> > > > > > > >  }
> > > > > > > >  
> > > > > > > >  struct device_type iio_device_type = {
> > > > > > > > @@ -1498,10 +1517,11 @@ struct device_type iio_device_type = {
> > > > > > > >   **/
> > > > > > > >  struct iio_dev *iio_device_alloc(int sizeof_priv)
> > > > > > > >  {
> > > > > > > > +	struct iio_dev_priv *iio_dev_priv;
> > > > > > > >  	struct iio_dev *dev;
> > > > > > > >  	size_t alloc_size;
> > > > > > > >  
> > > > > > > > -	alloc_size = sizeof(struct iio_dev);
> > > > > > > > +	alloc_size = sizeof(struct iio_dev_priv);
> > > > > > > >  	if (sizeof_priv) {
> > > > > > > >  		alloc_size = ALIGN(alloc_size, IIO_ALIGN);
> > > > > > > >  		alloc_size += sizeof_priv;
> > > > > > > > @@ -1509,10 +1529,12 @@ struct iio_dev *iio_device_alloc(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_priv = kzalloc(alloc_size, GFP_KERNEL);
> > > > > > > > +	if (!iio_dev_priv)
> > > > > > > >  		return NULL;
> > > > > > > >  
> > > > > > > > +	dev = &iio_dev_priv->indio_dev;
> > > > > > > > +
> > > > > > > >  	dev->dev.groups = dev->groups;
> > > > > > > >  	dev->dev.type = &iio_device_type;
> > > > > > > >  	dev->dev.bus = &iio_bus_type;
> > > > > > > > @@ -1526,7 +1548,7 @@ struct iio_dev *iio_device_alloc(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_priv);
> > > > > > > >  		return NULL;
> > > > > > > >  	}
> > > > > > > >  	dev_set_name(&dev->dev, "iio:device%d", dev->id);
> > > > > > > > diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> > > > > > > > index 5f9f439a4f01..38c4ea505394 100644
> > > > > > > > --- a/include/linux/iio/iio.h
> > > > > > > > +++ b/include/linux/iio/iio.h
> > > > > > > > @@ -678,16 +678,8 @@ static inline void
> > > > > > > > *iio_device_get_drvdata(struct
> > > > > > > > iio_dev *indio_dev)
> > > > > > > >  #define IIO_ALIGN L1_CACHE_BYTES
> > > > > > > >  struct iio_dev *iio_device_alloc(int sizeof_priv);
> > > > > > > >  
> > > > > > > > -static inline void *iio_priv(const struct iio_dev *indio_dev)
> > > > > > > > -{
> > > > > > > > -	return (char *)indio_dev + ALIGN(sizeof(struct iio_dev),
> > > > > > > > IIO_ALIGN);
> > > > > > > > -}
> > > > > > > > -
> > > > > > > > -static inline struct iio_dev *iio_priv_to_dev(void *priv)
> > > > > > > > -{
> > > > > > > > -	return (struct iio_dev *)((char *)priv -
> > > > > > > > -				  ALIGN(sizeof(struct iio_dev),
> > > > > > > > IIO_ALIGN));
> > > > > > > > -}
> > > > > > > > +void *iio_priv(const struct iio_dev *indio_dev);
> > > > > > > > +struct iio_dev *iio_priv_to_dev(void *priv);
> > > > > > > >  
> > > > > > > >  void iio_device_free(struct iio_dev *indio_dev);
> > > > > > > >  struct iio_dev *devm_iio_device_alloc(struct device *dev, int
> > > > > > > > sizeof_priv);        
> > > > > > 
> > > > > >       
> > > > 
> > > >