[PATCH -next] mux: convert mux_chip->mux to flexible array

* [PATCH -next] mux: convert mux_chip->mux to flexible array
@ 2023-02-23  1:42 Jacob Keller
  2023-02-27 20:28 ` Jesse Brandeburg
  0 siblings, 1 reply; 5+ messages in thread
From: Jacob Keller @ 2023-02-23  1:42 UTC (permalink / raw)
  To: Peter Rosin; +Cc: linux-kernel, Jacob Keller

The mux_chip structure size is over allocated to additionally include both
the array of mux controllers as well as a device specific private area.
The controllers array is then pointed to by assigning mux_chip->mux to the
first block of extra memory, while the private area is extracted via
mux_chip_priv() and points to the area just after the controllers.

The size of the mux_chip allocation uses direct multiplication and addition
rather than the <linux/overflow.h> helpers. In addition, the mux_chip->mux
struct member wastes space by having to store the pointer as part of the
structures.

Convert struct mux_chip to use a flexible array member for the mux
controller array. Use struct_size() and size_add() to compute the size of
the structure while protecting against overflow.

After converting the mux pointer, notice that two 4-byte holes remain in
the structure layout due to the alignment requirements for the dev
sub-structure and the ops pointer.

These can be easily fixed through re-ordering the id field to the 4-byte
hole just after the controllers member.

This changes the layout from:

struct mux_chip {
        unsigned int               controllers;          /*     0     4 */

        /* XXX 4 bytes hole, try to pack */

        struct mux_control *       mux;                  /*     8     8 */
        struct device              dev __attribute__((__aligned__(8))); /*    16  1400 */

        /* XXX last struct has 3 bytes of padding */

        /* --- cacheline 22 boundary (1408 bytes) was 8 bytes ago --- */
        int                        id;                   /*  1416     4 */

        /* XXX 4 bytes hole, try to pack */

        const struct mux_control_ops  * ops;             /*  1424     8 */

        /* size: 1432, cachelines: 23, members: 5 */
        /* sum members: 1424, holes: 2, sum holes: 8 */
        /* paddings: 1, sum paddings: 3 */
        /* forced alignments: 1 */
        /* last cacheline: 24 bytes */
} __attribute__((__aligned__(8)));

To the following:

struct mux_chip {
        unsigned int               controllers;          /*     0     4 */
        int                        id;                   /*     4     4 */
        struct device              dev __attribute__((__aligned__(8))); /*     8  1400 */

        /* XXX last struct has 3 bytes of padding */

        /* --- cacheline 22 boundary (1408 bytes) --- */
        const struct mux_control_ops  * ops;             /*  1408     8 */
        struct mux_control         mux[];                /*  1416     0 */

        /* size: 1416, cachelines: 23, members: 5 */
        /* paddings: 1, sum paddings: 3 */
        /* forced alignments: 1 */
        /* last cacheline: 8 bytes */
} __attribute__((__aligned__(8)));

This both removes risk of overflowing and performing an under-allocation,
as well as saves 16 bytes of otherwise wasted space for every mux_chip.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 drivers/mux/core.c         |  7 +++----
 include/linux/mux/driver.h | 10 +++++-----
 2 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/mux/core.c b/drivers/mux/core.c
index 49bedbe6316c..3e26b9911cc2 100644
--- a/drivers/mux/core.c
+++ b/drivers/mux/core.c
@@ -100,13 +100,12 @@ 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) +
-			   sizeof_priv, GFP_KERNEL);
+	mux_chip = kzalloc(size_add(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..84dc0d3e79d6 100644
--- a/include/linux/mux/driver.h
+++ b/include/linux/mux/driver.h
@@ -56,18 +56,18 @@ 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.
+ * @dev:		Device structure.
  * @ops:		Mux controller operations.
+ * @mux:		Flexible array of mux controllers that are handled.
  */
 struct mux_chip {
 	unsigned int controllers;
-	struct mux_control *mux;
-	struct device dev;
 	int id;
-
+	struct device dev;
 	const struct mux_control_ops *ops;
+
+	struct mux_control mux[];
 };
 
 #define to_mux_chip(x) container_of((x), struct mux_chip, dev)

base-commit: 307e14c039063f0c9bd7a18a7add8f940580dcc9
-- 
I found this while developing a coccinelle patch that helps detect potential
code that could be converted to struct_size() and noticed this weird case
that could be a flexible array and save memory.

2.39.1.405.gd4c25cc71f83


^ permalink raw reply related	[flat|nested] 5+ messages in thread