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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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)
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 <[email protected]>
> ---
> 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