diff mbox series

[v4,4/4] spi: Use struct_size() helper

Message ID 20230714091748.89681-5-andriy.shevchenko@linux.intel.com
State Accepted
Commit 75e308ffc4f0d36b895f1110ece8b77d4116fdb1
Headers show
Series spi: Header and core clean up and refactoring | expand

Commit Message

Andy Shevchenko July 14, 2023, 9:17 a.m. UTC
The Documentation/process/deprecated.rst suggests to use flexible array
members to provide a way to declare having a dynamically sized set of
trailing elements in a structure.This makes code robust agains bunch of
the issues described in the documentation, main of which is about the
correctness of the sizeof() calculation for this data structure.

Due to above, prefer struct_size() over open-coded versions.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 include/linux/spi/spi.h | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

Comments

Marc Kleine-Budde Oct. 9, 2023, 9:15 a.m. UTC | #1
Hello,

this change (75e308ffc4f0 ("spi: Use struct_size() helper")) reached
mainline with v6.6-rc1 and causes the following warning in my mcp251xfd
CAN driver.

On 14.07.2023 12:17:48, Andy Shevchenko wrote:
> The Documentation/process/deprecated.rst suggests to use flexible array
> members to provide a way to declare having a dynamically sized set of
> trailing elements in a structure.This makes code robust agains bunch of
> the issues described in the documentation, main of which is about the
> correctness of the sizeof() calculation for this data structure.
>
> Due to above, prefer struct_size() over open-coded versions.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  include/linux/spi/spi.h | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
> index 04daf61dfd3f..7f8b478fdeb3 100644
> --- a/include/linux/spi/spi.h
> +++ b/include/linux/spi/spi.h
> @@ -13,6 +13,7 @@
>  #include <linux/gpio/consumer.h>
>  #include <linux/kthread.h>
>  #include <linux/mod_devicetable.h>
> +#include <linux/overflow.h>
>  #include <linux/scatterlist.h>
>  #include <linux/slab.h>
>  #include <linux/u64_stats_sync.h>
> @@ -1085,6 +1086,8 @@ struct spi_transfer {
>   * @state: for use by whichever driver currently owns the message
>   * @resources: for resource management when the SPI message is processed
>   * @prepared: spi_prepare_message was called for the this message
> + * @t: for use with spi_message_alloc() when message and transfers have
> + *	been allocated together
>   *
>   * A @spi_message is used to execute an atomic sequence of data transfers,
>   * each represented by a struct spi_transfer.  The sequence is "atomic"
> @@ -1139,6 +1142,9 @@ struct spi_message {
>
>  	/* List of spi_res resources when the SPI message is processed */
>  	struct list_head        resources;
> +
> +	/* For embedding transfers into the memory of the message */
> +	struct spi_transfer	t[];

|   CHECK   drivers/net/can/spi/mcp251xfd/mcp251xfd-chip-fifo.c
| drivers/net/can/spi/mcp251xfd/mcp251xfd-chip-fifo.c: note: in included file:
| drivers/net/can/spi/mcp251xfd/mcp251xfd.h:632:38: warning: array of flexible structures
| drivers/net/can/spi/mcp251xfd/mcp251xfd.h:547:36: warning: array of flexible structures

Line 632 is an array of struct mcp251xfd_tef_ring in the struct mcp251xfd_priv:

| struct mcp251xfd_priv {
[...]
| 	struct mcp251xfd_tef_ring tef[MCP251XFD_FIFO_TEF_NUM];
[...]
| }

...and struct mcp251xfd_tef_ring contains a struct spi_transfer:

| struct mcp251xfd_tef_ring {
| 	unsigned int head;
| 	unsigned int tail;
| 
| 	/* u8 obj_num equals tx_ring->obj_num */
| 	/* u8 obj_size equals sizeof(struct mcp251xfd_hw_tef_obj) */
| 
| 	union mcp251xfd_write_reg_buf irq_enable_buf;
| 	struct spi_transfer irq_enable_xfer;
| 	struct spi_message irq_enable_msg;
| 
| 	union mcp251xfd_write_reg_buf uinc_buf;
| 	union mcp251xfd_write_reg_buf uinc_irq_disable_buf;
| 	struct spi_transfer uinc_xfer[MCP251XFD_TX_OBJ_NUM_MAX];
| };

The warning in line 547 is similar, but for the TX ring.

regards,
Marc
diff mbox series

Patch

diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index 04daf61dfd3f..7f8b478fdeb3 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -13,6 +13,7 @@ 
 #include <linux/gpio/consumer.h>
 #include <linux/kthread.h>
 #include <linux/mod_devicetable.h>
+#include <linux/overflow.h>
 #include <linux/scatterlist.h>
 #include <linux/slab.h>
 #include <linux/u64_stats_sync.h>
@@ -1085,6 +1086,8 @@  struct spi_transfer {
  * @state: for use by whichever driver currently owns the message
  * @resources: for resource management when the SPI message is processed
  * @prepared: spi_prepare_message was called for the this message
+ * @t: for use with spi_message_alloc() when message and transfers have
+ *	been allocated together
  *
  * A @spi_message is used to execute an atomic sequence of data transfers,
  * each represented by a struct spi_transfer.  The sequence is "atomic"
@@ -1139,6 +1142,9 @@  struct spi_message {
 
 	/* List of spi_res resources when the SPI message is processed */
 	struct list_head        resources;
+
+	/* For embedding transfers into the memory of the message */
+	struct spi_transfer	t[];
 };
 
 static inline void spi_message_init_no_memset(struct spi_message *m)
@@ -1199,16 +1205,13 @@  static inline struct spi_message *spi_message_alloc(unsigned ntrans, gfp_t flags
 {
 	struct spi_message *m;
 
-	m = kzalloc(sizeof(struct spi_message)
-			+ ntrans * sizeof(struct spi_transfer),
-			flags);
+	m = kzalloc(struct_size(m, t, ntrans), flags);
 	if (m) {
 		unsigned i;
-		struct spi_transfer *t = (struct spi_transfer *)(m + 1);
 
 		spi_message_init_no_memset(m);
-		for (i = 0; i < ntrans; i++, t++)
-			spi_message_add_tail(t, m);
+		for (i = 0; i < ntrans; i++)
+			spi_message_add_tail(&m->t[i], m);
 	}
 	return m;
 }