[PATCH v4 0/4] spi: Header and core clean up and refactoring

[PATCH v4 0/4] spi: Header and core clean up and refactoring

[Andy Shevchenko]

Various cleanups and refactorings of the SPI header and core parts
united in a single series. It also touches drivers under SPI subsystem
folder on the pure renaming purposes of some constants.

No functional change intended (with some subtle shortcuts which are
explained in the respective commit messages).

Changelog v4:
- dropped applied patches
- added tag to patch 1 (Sebastian)

v3: 20230711171756.86736-1-andriy.shevchenko@linux.intel.com

Changelog v3:
- dropped controversial used to be patches 3,4,8,10 (Mark)
- amended many commit messages (Mark)
- added tag to patch 1 (AngeloGioacchino)
- split used to be patch 2 to patches 2 & 3 for better review (Mark)
- rewritten used to be patch 5 to patches 4 & 5 (Mark, Sebastian)
- added new patch 7
- fixed typos and added tag to patch 12 (Serge)

v2: 20230710154932.68377-1-andriy.shevchenko@linux.intel.com

Changelog v2:
- added new patches 3,4,5,10,13,14
- massaged comment and kernel doc in patch 9
- split used to be patch 4 to patches 11,12
- covered a few things in SPI core in patch 15
- amended commit message for above (Mark)
- reshuffled patches in the series for better logical grouping

Andy Shevchenko (4):
  spi: Remove code duplication in spi_add_device*()
  spi: Kill spi_add_device_locked()
  spi: Use BITS_TO_BYTES()
  spi: Use struct_size() helper

 drivers/spi/spi.c       | 51 +++++++++++++----------------------------
 include/linux/spi/spi.h | 15 +++++++-----
 2 files changed, 25 insertions(+), 41 deletions(-)

-- 
2.40.0.1.gaa8946217a0b

[PATCH v4 1/4] spi: Remove code duplication in spi_add_device*()

[Andy Shevchenko]

The commit 0c79378c0199 ("spi: add ancillary device support")
added a dozen of duplicating lines of code. We may move them
to the __spi_add_device(). Note, that the code may be called
under the mutex.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 drivers/spi/spi.c | 32 ++++++++++----------------------
 1 file changed, 10 insertions(+), 22 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index ae2693ba1744..8e70f4183e62 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -631,6 +631,16 @@ static int __spi_add_device(struct spi_device *spi)
 	struct device *dev = ctlr->dev.parent;
 	int status;
 
+	/* Chipselects are numbered 0..max; validate. */
+	if (spi_get_chipselect(spi, 0) >= ctlr->num_chipselect) {
+		dev_err(dev, "cs%d >= max %d\n", spi_get_chipselect(spi, 0),
+			ctlr->num_chipselect);
+		return -EINVAL;
+	}
+
+	/* Set the bus ID string */
+	spi_dev_set_name(spi);
+
 	/*
 	 * We need to make sure there's no other device with this
 	 * chipselect **BEFORE** we call setup(), else we'll trash
@@ -689,19 +699,8 @@ static int __spi_add_device(struct spi_device *spi)
 int spi_add_device(struct spi_device *spi)
 {
 	struct spi_controller *ctlr = spi->controller;
-	struct device *dev = ctlr->dev.parent;
 	int status;
 
-	/* Chipselects are numbered 0..max; validate. */
-	if (spi_get_chipselect(spi, 0) >= ctlr->num_chipselect) {
-		dev_err(dev, "cs%d >= max %d\n", spi_get_chipselect(spi, 0),
-			ctlr->num_chipselect);
-		return -EINVAL;
-	}
-
-	/* Set the bus ID string */
-	spi_dev_set_name(spi);
-
 	mutex_lock(&ctlr->add_lock);
 	status = __spi_add_device(spi);
 	mutex_unlock(&ctlr->add_lock);
@@ -712,17 +711,6 @@ EXPORT_SYMBOL_GPL(spi_add_device);
 static int spi_add_device_locked(struct spi_device *spi)
 {
 	struct spi_controller *ctlr = spi->controller;
-	struct device *dev = ctlr->dev.parent;
-
-	/* Chipselects are numbered 0..max; validate. */
-	if (spi_get_chipselect(spi, 0) >= ctlr->num_chipselect) {
-		dev_err(dev, "cs%d >= max %d\n", spi_get_chipselect(spi, 0),
-			ctlr->num_chipselect);
-		return -EINVAL;
-	}
-
-	/* Set the bus ID string */
-	spi_dev_set_name(spi);
 
 	WARN_ON(!mutex_is_locked(&ctlr->add_lock));
 	return __spi_add_device(spi);
-- 
2.40.0.1.gaa8946217a0b

[PATCH v4 2/4] spi: Kill spi_add_device_locked()

[Andy Shevchenko]

Now, spi_add_device_locked() has just a line on top of __spi_add_device().
Besides that, it has a single caller. So, just kill it and embed its parts
into the caller.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/spi/spi.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 8e70f4183e62..05f702339182 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -708,14 +708,6 @@ int spi_add_device(struct spi_device *spi)
 }
 EXPORT_SYMBOL_GPL(spi_add_device);
 
-static int spi_add_device_locked(struct spi_device *spi)
-{
-	struct spi_controller *ctlr = spi->controller;
-
-	WARN_ON(!mutex_is_locked(&ctlr->add_lock));
-	return __spi_add_device(spi);
-}
-
 /**
  * spi_new_device - instantiate one new SPI device
  * @ctlr: Controller to which device is connected
@@ -2417,11 +2409,12 @@ static void of_register_spi_devices(struct spi_controller *ctlr) { }
 struct spi_device *spi_new_ancillary_device(struct spi_device *spi,
 					     u8 chip_select)
 {
+	struct spi_controller *ctlr = spi->controller;
 	struct spi_device *ancillary;
 	int rc = 0;
 
 	/* Alloc an spi_device */
-	ancillary = spi_alloc_device(spi->controller);
+	ancillary = spi_alloc_device(ctlr);
 	if (!ancillary) {
 		rc = -ENOMEM;
 		goto err_out;
@@ -2436,8 +2429,10 @@ struct spi_device *spi_new_ancillary_device(struct spi_device *spi,
 	ancillary->max_speed_hz = spi->max_speed_hz;
 	ancillary->mode = spi->mode;
 
+	WARN_ON(!mutex_is_locked(&ctlr->add_lock));
+
 	/* Register the new device */
-	rc = spi_add_device_locked(ancillary);
+	rc = __spi_add_device(ancillary);
 	if (rc) {
 		dev_err(&spi->dev, "failed to register ancillary device\n");
 		goto err_out;
-- 
2.40.0.1.gaa8946217a0b

[PATCH v4 3/4] spi: Use BITS_TO_BYTES()

[Andy Shevchenko]

BITS_TO_BYTES() is the existing macro which takes care about full
bytes that may fully hold the given amount of bits. Use it.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/spi/spi.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 05f702339182..8d6304cb061e 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -3877,11 +3877,9 @@ static int __spi_validate(struct spi_device *spi, struct spi_message *message)
 	 */
 	if ((spi->mode & SPI_CS_WORD) && (!(ctlr->mode_bits & SPI_CS_WORD) ||
 					  spi_get_csgpiod(spi, 0))) {
-		size_t maxsize;
+		size_t maxsize = BITS_TO_BYTES(spi->bits_per_word);
 		int ret;
 
-		maxsize = (spi->bits_per_word + 7) / 8;
-
 		/* spi_split_transfers_maxsize() requires message->spi */
 		message->spi = spi;
 
-- 
2.40.0.1.gaa8946217a0b

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

[Andy Shevchenko]

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[];
 };
 
 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;
 }
-- 
2.40.0.1.gaa8946217a0b

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

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 3492 bytes --]

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

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v4 0/4] spi: Header and core clean up and refactoring

[Mark Brown]

On Fri, 14 Jul 2023 12:17:44 +0300, Andy Shevchenko wrote:
> Various cleanups and refactorings of the SPI header and core parts
> united in a single series. It also touches drivers under SPI subsystem
> folder on the pure renaming purposes of some constants.
> 
> No functional change intended (with some subtle shortcuts which are
> explained in the respective commit messages).
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next

Thanks!

[1/4] spi: Remove code duplication in spi_add_device*()
      commit: 36124dea164cf684869e856b2ada23e8adab5f03
[2/4] spi: Kill spi_add_device_locked()
      commit: 7b5c6a545b3491fb785c75cee60e6b0c35a4de1b
[3/4] spi: Use BITS_TO_BYTES()
      commit: 169f5312dc46deb986e368b6828bedbedd297f6e
[4/4] spi: Use struct_size() helper
      commit: 75e308ffc4f0d36b895f1110ece8b77d4116fdb1

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark