[PATCH v2 net-next 0/2] Adapt the sja1105 DSA driver to the SPI controller's transfer limits

[PATCH v2 net-next 0/2] Adapt the sja1105 DSA driver to the SPI controller's transfer limits

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

This series changes the SPI transfer procedure in sja1105 to take into
consideration the buffer size limitations that the SPI controller driver
might have.

Changes in v2:
Remove the driver's use of cs_change and send multiple, smaller SPI
messages instead of a single large one.

Vladimir Oltean (2):
  net: dsa: sja1105: send multiple spi_messages instead of using
    cs_change
  net: dsa: sja1105: adapt to a SPI controller with a limited max
    transfer size

 drivers/net/dsa/sja1105/sja1105_spi.c | 80 ++++++++++++---------------
 1 file changed, 34 insertions(+), 46 deletions(-)

-- 
2.25.1

[PATCH v2 net-next 1/2] net: dsa: sja1105: send multiple spi_messages instead of using cs_change

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

The sja1105 driver has been described by Mark Brown as "not using the
[ SPI ] API at all idiomatically" due to the use of cs_change:
https://patchwork.kernel.org/project/netdevbpf/patch/20210520135031.2969183-1-olteanv@gmail.com/

According to include/linux/spi/spi.h, the chip select is supposed to be
asserted for the entire length of a SPI message, as long as cs_change is
false for all member transfers. The cs_change flag changes the following:

(i) When a non-final SPI transfer has cs_change = true, the chip select
    should temporarily deassert and then reassert starting with the next
    transfer.
(ii) When a final SPI transfer has cs_change = true, the chip select
     should remain asserted until the following SPI message.

The sja1105 driver only uses cs_change for its first property, to form a
single SPI message whose layout can be seen below:

                                             this is an entire, single spi_message
           _______________________________________________________________________________________________
          /                                                                                               \
          +-------------+---------------+-------------+---------------+ ... +-------------+---------------+
          | hdr_xfer[0] | chunk_xfer[0] | hdr_xfer[1] | chunk_xfer[1] |     | hdr_xfer[n] | chunk_xfer[n] |
          +-------------+---------------+-------------+---------------+ ... +-------------+---------------+
cs_change      false          true           false           true                false          false

           ____________________________  _____________________________       _____________________________
CS line __/                            \/                             \ ... /                             \__

The fact of the matter is that spi_max_message_size() has an ambiguous
meaning if any non-final transfer has cs_change = true.

If the SPI master has a limitation in that it cannot keep the chip
select asserted for more than, say, 200 bytes (like the spi-sc18is602),
the normal thing for it to do is to implement .max_transfer_size and
.max_message_size, and limit both to 200: in the "worst case" where
cs_change is always false, then the controller can, indeed, not send
messages larger than 200 bytes.

But the fact that the SPI controller's max_message_size does not
necessarily mean that we cannot send messages larger than that.
Notably, if the SPI master special-cases the transfers with cs_change
and treats every chip select toggling as an entirely new transaction,
then a SPI message can easily exceed that limit. So there is a
temptation to ignore the controller's reported max_message_size when
using cs_change = true in non-final transfers.

But that can lead to false conclusions. As Mark points out, the SPI
controller might have a different kind of limitation with the max
message size, that has nothing at all to do with how long it can keep
the chip select asserted.
For example, that might be the case if the device is able to offload the
chip select changes to the hardware as part of the data stream, and it
packs the entire stream of commands+data (corresponding to a SPI
message) into a single DMA transfer that is itself limited in size.

So the only thing we can do is avoid ambiguity by not using cs_change at
all. Instead of sending a single spi_message, we now send multiple SPI
messages as follows:

                  spi_message 0                 spi_message 1                       spi_message n
           ____________________________   ___________________________        _____________________________
          /                            \ /                           \      /                             \
          +-------------+---------------+-------------+---------------+ ... +-------------+---------------+
          | hdr_xfer[0] | chunk_xfer[0] | hdr_xfer[1] | chunk_xfer[1] |     | hdr_xfer[n] | chunk_xfer[n] |
          +-------------+---------------+-------------+---------------+ ... +-------------+---------------+
cs_change      false          true           false           true                false          false

           ____________________________  _____________________________       _____________________________
CS line __/                            \/                             \ ... /                             \__

which is clearer because the max_message_size limit is now easier to
enforce. What is transmitted on the wire stays, of course, the same.

Additionally, because we send no more than 2 transfers at a time, we now
avoid dynamic memory allocation too, which might be seen as an
improvement by some.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/sja1105/sja1105_spi.c | 52 +++++++--------------------
 1 file changed, 12 insertions(+), 40 deletions(-)

diff --git a/drivers/net/dsa/sja1105/sja1105_spi.c b/drivers/net/dsa/sja1105/sja1105_spi.c
index f7a1514f81e8..8746e3f158a0 100644
--- a/drivers/net/dsa/sja1105/sja1105_spi.c
+++ b/drivers/net/dsa/sja1105/sja1105_spi.c
@@ -29,13 +29,6 @@ sja1105_spi_message_pack(void *buf, const struct sja1105_spi_message *msg)
 	sja1105_pack(buf, &msg->address,    24,  4, size);
 }
 
-#define sja1105_hdr_xfer(xfers, chunk) \
-	((xfers) + 2 * (chunk))
-#define sja1105_chunk_xfer(xfers, chunk) \
-	((xfers) + 2 * (chunk) + 1)
-#define sja1105_hdr_buf(hdr_bufs, chunk) \
-	((hdr_bufs) + (chunk) * SJA1105_SIZE_SPI_MSG_HEADER)
-
 /* If @rw is:
  * - SPI_WRITE: creates and sends an SPI write message at absolute
  *		address reg_addr, taking @len bytes from *buf
@@ -46,41 +39,25 @@ static int sja1105_xfer(const struct sja1105_private *priv,
 			sja1105_spi_rw_mode_t rw, u64 reg_addr, u8 *buf,
 			size_t len, struct ptp_system_timestamp *ptp_sts)
 {
+	u8 hdr_buf[SJA1105_SIZE_SPI_MSG_HEADER] = {0};
 	struct sja1105_chunk chunk = {
 		.len = min_t(size_t, len, SJA1105_SIZE_SPI_MSG_MAXLEN),
 		.reg_addr = reg_addr,
 		.buf = buf,
 	};
 	struct spi_device *spi = priv->spidev;
-	struct spi_transfer *xfers;
+	struct spi_transfer xfers[2] = {0};
+	struct spi_transfer *chunk_xfer;
+	struct spi_transfer *hdr_xfer;
 	int num_chunks;
 	int rc, i = 0;
-	u8 *hdr_bufs;
 
 	num_chunks = DIV_ROUND_UP(len, SJA1105_SIZE_SPI_MSG_MAXLEN);
 
-	/* One transfer for each message header, one for each message
-	 * payload (chunk).
-	 */
-	xfers = kcalloc(2 * num_chunks, sizeof(struct spi_transfer),
-			GFP_KERNEL);
-	if (!xfers)
-		return -ENOMEM;
-
-	/* Packed buffers for the num_chunks SPI message headers,
-	 * stored as a contiguous array
-	 */
-	hdr_bufs = kcalloc(num_chunks, SJA1105_SIZE_SPI_MSG_HEADER,
-			   GFP_KERNEL);
-	if (!hdr_bufs) {
-		kfree(xfers);
-		return -ENOMEM;
-	}
+	hdr_xfer = &xfers[0];
+	chunk_xfer = &xfers[1];
 
 	for (i = 0; i < num_chunks; i++) {
-		struct spi_transfer *chunk_xfer = sja1105_chunk_xfer(xfers, i);
-		struct spi_transfer *hdr_xfer = sja1105_hdr_xfer(xfers, i);
-		u8 *hdr_buf = sja1105_hdr_buf(hdr_bufs, i);
 		struct spi_transfer *ptp_sts_xfer;
 		struct sja1105_spi_message msg;
 
@@ -129,19 +106,14 @@ static int sja1105_xfer(const struct sja1105_private *priv,
 		chunk.len = min_t(size_t, (ptrdiff_t)(buf + len - chunk.buf),
 				  SJA1105_SIZE_SPI_MSG_MAXLEN);
 
-		/* De-assert the chip select after each chunk. */
-		if (chunk.len)
-			chunk_xfer->cs_change = 1;
+		rc = spi_sync_transfer(spi, xfers, 2);
+		if (rc < 0) {
+			dev_err(&spi->dev, "SPI transfer failed: %d\n", rc);
+			return rc;
+		}
 	}
 
-	rc = spi_sync_transfer(spi, xfers, 2 * num_chunks);
-	if (rc < 0)
-		dev_err(&spi->dev, "SPI transfer failed: %d\n", rc);
-
-	kfree(hdr_bufs);
-	kfree(xfers);
-
-	return rc;
+	return 0;
 }
 
 int sja1105_xfer_buf(const struct sja1105_private *priv,
-- 
2.25.1

[PATCH v2 net-next 2/2] net: dsa: sja1105: adapt to a SPI controller with a limited max transfer size

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

The static config of the sja1105 switch is a long stream of bytes which
is programmed to the hardware in chunks (portions with the chip select
continuously asserted) of max 256 bytes each.

Only that certain SPI controllers, such as the spi-sc18is602 I2C-to-SPI
bridge, cannot keep the chip select asserted for that long.
The spi_max_transfer_size() and spi_max_message_size() functions are how
the controller can impose its hardware limitations upon the SPI
peripheral driver.

The sja1105 sends its static config to the SPI master in chunks, and
each chunk is a spi_message composed of 2 spi_transfers: the buffer with
the data and a preceding buffer with the SPI access header. Both buffers
must be smaller than the transfer limit, and their sum must be smaller
than the message limit.

Regression-tested on a switch connected to a controller with no
limitations (spi-fsl-dspi) as well as with one with caps for both
max_transfer_size and max_message_size (spi-sc18is602).

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/sja1105/sja1105_spi.c | 30 ++++++++++++++++++++-------
 1 file changed, 23 insertions(+), 7 deletions(-)

diff --git a/drivers/net/dsa/sja1105/sja1105_spi.c b/drivers/net/dsa/sja1105/sja1105_spi.c
index 8746e3f158a0..7bcf2e419037 100644
--- a/drivers/net/dsa/sja1105/sja1105_spi.c
+++ b/drivers/net/dsa/sja1105/sja1105_spi.c
@@ -40,19 +40,35 @@ static int sja1105_xfer(const struct sja1105_private *priv,
 			size_t len, struct ptp_system_timestamp *ptp_sts)
 {
 	u8 hdr_buf[SJA1105_SIZE_SPI_MSG_HEADER] = {0};
-	struct sja1105_chunk chunk = {
-		.len = min_t(size_t, len, SJA1105_SIZE_SPI_MSG_MAXLEN),
-		.reg_addr = reg_addr,
-		.buf = buf,
-	};
 	struct spi_device *spi = priv->spidev;
 	struct spi_transfer xfers[2] = {0};
 	struct spi_transfer *chunk_xfer;
 	struct spi_transfer *hdr_xfer;
+	struct sja1105_chunk chunk;
+	ssize_t xfer_len;
 	int num_chunks;
 	int rc, i = 0;
 
-	num_chunks = DIV_ROUND_UP(len, SJA1105_SIZE_SPI_MSG_MAXLEN);
+	/* One spi_message is composed of two spi_transfers: a small one for
+	 * the message header and another one for the current chunk of the
+	 * packed buffer.
+	 * Check that the restrictions imposed by the SPI controller are
+	 * respected: the chunk buffer is smaller than the max transfer size,
+	 * and the total length of the chunk plus its message header is smaller
+	 * than the max message size.
+	 */
+	xfer_len = min_t(ssize_t, SJA1105_SIZE_SPI_MSG_MAXLEN,
+			 spi_max_transfer_size(spi));
+	xfer_len = min_t(ssize_t, SJA1105_SIZE_SPI_MSG_MAXLEN,
+			 spi_max_message_size(spi) - SJA1105_SIZE_SPI_MSG_HEADER);
+	if (xfer_len < 0)
+		return -ERANGE;
+
+	num_chunks = DIV_ROUND_UP(len, xfer_len);
+
+	chunk.reg_addr = reg_addr;
+	chunk.buf = buf;
+	chunk.len = min_t(size_t, len, xfer_len);
 
 	hdr_xfer = &xfers[0];
 	chunk_xfer = &xfers[1];
@@ -104,7 +120,7 @@ static int sja1105_xfer(const struct sja1105_private *priv,
 		chunk.buf += chunk.len;
 		chunk.reg_addr += chunk.len / 4;
 		chunk.len = min_t(size_t, (ptrdiff_t)(buf + len - chunk.buf),
-				  SJA1105_SIZE_SPI_MSG_MAXLEN);
+				  xfer_len);
 
 		rc = spi_sync_transfer(spi, xfers, 2);
 		if (rc < 0) {
-- 
2.25.1

Re: [PATCH v2 net-next 2/2] net: dsa: sja1105: adapt to a SPI controller with a limited max transfer size

[Vladimir Oltean]

On Thu, May 20, 2021 at 11:02:23PM +0300, Vladimir Oltean wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> 
> The static config of the sja1105 switch is a long stream of bytes which
> is programmed to the hardware in chunks (portions with the chip select
> continuously asserted) of max 256 bytes each.
> 
> Only that certain SPI controllers, such as the spi-sc18is602 I2C-to-SPI
> bridge, cannot keep the chip select asserted for that long.
> The spi_max_transfer_size() and spi_max_message_size() functions are how
> the controller can impose its hardware limitations upon the SPI
> peripheral driver.
> 
> The sja1105 sends its static config to the SPI master in chunks, and
> each chunk is a spi_message composed of 2 spi_transfers: the buffer with
> the data and a preceding buffer with the SPI access header. Both buffers
> must be smaller than the transfer limit, and their sum must be smaller
> than the message limit.
> 
> Regression-tested on a switch connected to a controller with no
> limitations (spi-fsl-dspi) as well as with one with caps for both
> max_transfer_size and max_message_size (spi-sc18is602).
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
>  drivers/net/dsa/sja1105/sja1105_spi.c | 30 ++++++++++++++++++++-------
>  1 file changed, 23 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/dsa/sja1105/sja1105_spi.c b/drivers/net/dsa/sja1105/sja1105_spi.c
> index 8746e3f158a0..7bcf2e419037 100644
> --- a/drivers/net/dsa/sja1105/sja1105_spi.c
> +++ b/drivers/net/dsa/sja1105/sja1105_spi.c
> @@ -40,19 +40,35 @@ static int sja1105_xfer(const struct sja1105_private *priv,
>  			size_t len, struct ptp_system_timestamp *ptp_sts)
>  {
>  	u8 hdr_buf[SJA1105_SIZE_SPI_MSG_HEADER] = {0};
> -	struct sja1105_chunk chunk = {
> -		.len = min_t(size_t, len, SJA1105_SIZE_SPI_MSG_MAXLEN),
> -		.reg_addr = reg_addr,
> -		.buf = buf,
> -	};
>  	struct spi_device *spi = priv->spidev;
>  	struct spi_transfer xfers[2] = {0};
>  	struct spi_transfer *chunk_xfer;
>  	struct spi_transfer *hdr_xfer;
> +	struct sja1105_chunk chunk;
> +	ssize_t xfer_len;
>  	int num_chunks;
>  	int rc, i = 0;
>  
> -	num_chunks = DIV_ROUND_UP(len, SJA1105_SIZE_SPI_MSG_MAXLEN);
> +	/* One spi_message is composed of two spi_transfers: a small one for
> +	 * the message header and another one for the current chunk of the
> +	 * packed buffer.
> +	 * Check that the restrictions imposed by the SPI controller are
> +	 * respected: the chunk buffer is smaller than the max transfer size,
> +	 * and the total length of the chunk plus its message header is smaller
> +	 * than the max message size.
> +	 */
> +	xfer_len = min_t(ssize_t, SJA1105_SIZE_SPI_MSG_MAXLEN,
> +			 spi_max_transfer_size(spi));
> +	xfer_len = min_t(ssize_t, SJA1105_SIZE_SPI_MSG_MAXLEN,
> +			 spi_max_message_size(spi) - SJA1105_SIZE_SPI_MSG_HEADER);
> +	if (xfer_len < 0)
> +		return -ERANGE;

I've introduced a bug here when spi_max_message_size returns SIZE_MAX
which is of the unsigned size_t type. Converted to ssize_t it's negative,
so it triggers the negative check...

Please wait until I send a v3 with this fixed. Thanks.

> +
> +	num_chunks = DIV_ROUND_UP(len, xfer_len);
> +
> +	chunk.reg_addr = reg_addr;
> +	chunk.buf = buf;
> +	chunk.len = min_t(size_t, len, xfer_len);
>  
>  	hdr_xfer = &xfers[0];
>  	chunk_xfer = &xfers[1];
> @@ -104,7 +120,7 @@ static int sja1105_xfer(const struct sja1105_private *priv,
>  		chunk.buf += chunk.len;
>  		chunk.reg_addr += chunk.len / 4;
>  		chunk.len = min_t(size_t, (ptrdiff_t)(buf + len - chunk.buf),
> -				  SJA1105_SIZE_SPI_MSG_MAXLEN);
> +				  xfer_len);
>  
>  		rc = spi_sync_transfer(spi, xfers, 2);
>  		if (rc < 0) {
> -- 
> 2.25.1
>