[PATCH 1/2] mux: Turn 'mux' into a flexible array in 'struct mux_chip'

[PATCH 1/2] mux: Turn 'mux' into a flexible array in 'struct mux_chip'

[Christophe JAILLET]

The 'mux' array stored in 'struct mux_chip' can be changed into a flexible
array.

This saves:
   - a pointer in the structure
   - an indirection when accessing the array
   - some pointer arithmetic when computing and storing the address in
     'mux'

It is also now possible to use __counted_by() and struct_size() for
additional safety.

The address for the 'priv' memory is computed with mux_chip_priv(). It
should work as good with a flexible array.

Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
The struct_size() goodies only work if sizeof_priv is 0. Adding an
additional size_add() would make it safe in all cases but would make code
less readable (IMHO).
---
 drivers/mux/core.c         | 4 +---
 include/linux/mux/driver.h | 5 +++--
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/mux/core.c b/drivers/mux/core.c
index 775816112932..80b2607b083b 100644
--- a/drivers/mux/core.c
+++ b/drivers/mux/core.c
@@ -98,13 +98,11 @@ struct mux_chip *mux_chip_alloc(struct device *dev,
 	if (WARN_ON(!dev || !controllers))
 		return ERR_PTR(-EINVAL);
 
-	mux_chip = kzalloc(sizeof(*mux_chip) +
-			   controllers * sizeof(*mux_chip->mux) +
+	mux_chip = kzalloc(struct_size(mux_chip, mux, controllers) +
 			   sizeof_priv, GFP_KERNEL);
 	if (!mux_chip)
 		return ERR_PTR(-ENOMEM);
 
-	mux_chip->mux = (struct mux_control *)(mux_chip + 1);
 	mux_chip->dev.class = &mux_class;
 	mux_chip->dev.type = &mux_type;
 	mux_chip->dev.parent = dev;
diff --git a/include/linux/mux/driver.h b/include/linux/mux/driver.h
index 18824064f8c0..c29e9b7fb17b 100644
--- a/include/linux/mux/driver.h
+++ b/include/linux/mux/driver.h
@@ -56,18 +56,19 @@ struct mux_control {
 /**
  * struct mux_chip -	Represents a chip holding mux controllers.
  * @controllers:	Number of mux controllers handled by the chip.
- * @mux:		Array of mux controllers that are handled.
  * @dev:		Device structure.
  * @id:			Used to identify the device internally.
  * @ops:		Mux controller operations.
+ * @mux:		Array of mux controllers that are handled.
  */
 struct mux_chip {
 	unsigned int controllers;
-	struct mux_control *mux;
 	struct device dev;
 	int id;
 
 	const struct mux_control_ops *ops;
+
+	struct mux_control mux[] __counted_by(controllers);
 };
 
 #define to_mux_chip(x) container_of((x), struct mux_chip, dev)
-- 
2.34.1

[PATCH 2/2] mux: Slightly reorder 'struct mux_chip'

[Christophe JAILLET]

Based on pahole, 2 holes can be combined in the 'struct mux_chip'. This
saves 8 bytes in the structure on my x86_64.

Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
 include/linux/mux/driver.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/mux/driver.h b/include/linux/mux/driver.h
index c29e9b7fb17b..09d519af521b 100644
--- a/include/linux/mux/driver.h
+++ b/include/linux/mux/driver.h
@@ -56,15 +56,15 @@ struct mux_control {
 /**
  * struct mux_chip -	Represents a chip holding mux controllers.
  * @controllers:	Number of mux controllers handled by the chip.
- * @dev:		Device structure.
  * @id:			Used to identify the device internally.
+ * @dev:		Device structure.
  * @ops:		Mux controller operations.
  * @mux:		Array of mux controllers that are handled.
  */
 struct mux_chip {
 	unsigned int controllers;
-	struct device dev;
 	int id;
+	struct device dev;
 
 	const struct mux_control_ops *ops;
 
-- 
2.34.1

Re: [PATCH 1/2] mux: Turn 'mux' into a flexible array in 'struct mux_chip'

[Gustavo A. R. Silva]

On 11/26/23 02:08, Christophe JAILLET wrote:
> The 'mux' array stored in 'struct mux_chip' can be changed into a flexible
> array.
> 
> This saves:
>     - a pointer in the structure
>     - an indirection when accessing the array
>     - some pointer arithmetic when computing and storing the address in
>       'mux'
> 
> It is also now possible to use __counted_by() and struct_size() for
> additional safety.
> 
> The address for the 'priv' memory is computed with mux_chip_priv(). It
> should work as good with a flexible array.
> 
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> The struct_size() goodies only work if sizeof_priv is 0. Adding an
> additional size_add() would make it safe in all cases but would make code
> less readable (IMHO).

Just save struct_size() in another variable, and use size_add() like this:

size_t size = struct_size(..);

.. kzalloc(size_add(size, sizeof_priv), ...);

--
Gustavo

> ---
>   drivers/mux/core.c         | 4 +---
>   include/linux/mux/driver.h | 5 +++--
>   2 files changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/mux/core.c b/drivers/mux/core.c
> index 775816112932..80b2607b083b 100644
> --- a/drivers/mux/core.c
> +++ b/drivers/mux/core.c
> @@ -98,13 +98,11 @@ struct mux_chip *mux_chip_alloc(struct device *dev,
>   	if (WARN_ON(!dev || !controllers))
>   		return ERR_PTR(-EINVAL);
>   
> -	mux_chip = kzalloc(sizeof(*mux_chip) +
> -			   controllers * sizeof(*mux_chip->mux) +
> +	mux_chip = kzalloc(struct_size(mux_chip, mux, controllers) +
>   			   sizeof_priv, GFP_KERNEL);
>   	if (!mux_chip)
>   		return ERR_PTR(-ENOMEM);
>   
> -	mux_chip->mux = (struct mux_control *)(mux_chip + 1);
>   	mux_chip->dev.class = &mux_class;
>   	mux_chip->dev.type = &mux_type;
>   	mux_chip->dev.parent = dev;
> diff --git a/include/linux/mux/driver.h b/include/linux/mux/driver.h
> index 18824064f8c0..c29e9b7fb17b 100644
> --- a/include/linux/mux/driver.h
> +++ b/include/linux/mux/driver.h
> @@ -56,18 +56,19 @@ struct mux_control {
>   /**
>    * struct mux_chip -	Represents a chip holding mux controllers.
>    * @controllers:	Number of mux controllers handled by the chip.
> - * @mux:		Array of mux controllers that are handled.
>    * @dev:		Device structure.
>    * @id:			Used to identify the device internally.
>    * @ops:		Mux controller operations.
> + * @mux:		Array of mux controllers that are handled.
>    */
>   struct mux_chip {
>   	unsigned int controllers;
> -	struct mux_control *mux;
>   	struct device dev;
>   	int id;
>   
>   	const struct mux_control_ops *ops;
> +
> +	struct mux_control mux[] __counted_by(controllers);
>   };
>   
>   #define to_mux_chip(x) container_of((x), struct mux_chip, dev)

Re: [PATCH 1/2] mux: Turn 'mux' into a flexible array in 'struct mux_chip'

[Dan Carpenter]

On Sun, Nov 26, 2023 at 09:08:11AM +0100, Christophe JAILLET wrote:
> The 'mux' array stored in 'struct mux_chip' can be changed into a flexible
> array.
> 
> This saves:
>    - a pointer in the structure
>    - an indirection when accessing the array
>    - some pointer arithmetic when computing and storing the address in
>      'mux'
> 
> It is also now possible to use __counted_by() and struct_size() for
> additional safety.
> 
> The address for the 'priv' memory is computed with mux_chip_priv(). It
> should work as good with a flexible array.
> 
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> The struct_size() goodies only work if sizeof_priv is 0. Adding an
> additional size_add() would make it safe in all cases but would make code
> less readable (IMHO).

Once people start using size_add() then we'll get used to reading it.

The controllers value comes from device tree.  For example, in
mux_mmio_probe().

	ret = of_property_count_u32_elems(np, "mux-reg-masks");

I should make Smatch parse device trees.  So that it will know the
correct range for ret in that assignment.  Eventually, I will.

KTODO: make Smatch understand device tree values

regards,
dan carpenter