[PATCH] spi: spidev: only use up TX/RX bounce buffer space when needed

[PATCH] spi: spidev: only use up TX/RX bounce buffer space when needed

[Ian Abbott]

Devices have separate, pre-allocated TX and RX bounce buffers of fixed
size.  Currently, each transfer uses up space in both buffers even if
the user-supplied no TX data or no RX space.  Change it to only use up
space in the TX and RX bounce buffers as required.

Since dummy transfers with no user-supplied TX data and no user-supplied
RX space will no longer use up space in the bounce buffers, limit the
overall SPI message length to INT_MAX instead of the buffer size.

Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
---
 drivers/spi/spidev.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/drivers/spi/spidev.c b/drivers/spi/spidev.c
index d1ccbfe..75de351 100644
--- a/drivers/spi/spidev.c
+++ b/drivers/spi/spidev.c
@@ -227,7 +227,7 @@ static int spidev_message(struct spidev_data *spidev,
 	struct spi_transfer	*k_xfers;
 	struct spi_transfer	*k_tmp;
 	struct spi_ioc_transfer *u_tmp;
-	unsigned		n, total;
+	unsigned		n, total, tx_total, rx_total;
 	u8			*tx_buf, *rx_buf;
 	int			status = -EFAULT;
 
@@ -243,33 +243,45 @@ static int spidev_message(struct spidev_data *spidev,
 	tx_buf = spidev->tx_buffer;
 	rx_buf = spidev->rx_buffer;
 	total = 0;
+	tx_total = 0;
+	rx_total = 0;
 	for (n = n_xfers, k_tmp = k_xfers, u_tmp = u_xfers;
 			n;
 			n--, k_tmp++, u_tmp++) {
 		k_tmp->len = u_tmp->len;
 
 		total += k_tmp->len;
-		if (total > bufsiz) {
+		if (total > INT_MAX) {
 			status = -EMSGSIZE;
 			goto done;
 		}
 
 		if (u_tmp->rx_buf) {
+			rx_total += k_tmp->len;
+			if (rx_total > bufsiz) {
+				status = -EMSGSIZE;
+				goto done;
+			}
 			k_tmp->rx_buf = rx_buf;
 			if (!access_ok(VERIFY_WRITE, (u8 __user *)
 						(uintptr_t) u_tmp->rx_buf,
 						u_tmp->len))
 				goto done;
+			rx_buf += k_tmp->len;
 		}
 		if (u_tmp->tx_buf) {
+			tx_total += k_tmp->len;
+			if (tx_total > bufsiz) {
+				status = -EMSGSIZE;
+				goto done;
+			}
 			k_tmp->tx_buf = tx_buf;
 			if (copy_from_user(tx_buf, (const u8 __user *)
 						(uintptr_t) u_tmp->tx_buf,
 					u_tmp->len))
 				goto done;
+			tx_buf += k_tmp->len;
 		}
-		tx_buf += k_tmp->len;
-		rx_buf += k_tmp->len;
 
 		k_tmp->cs_change = !!u_tmp->cs_change;
 		k_tmp->tx_nbits = u_tmp->tx_nbits;
@@ -307,8 +319,8 @@ static int spidev_message(struct spidev_data *spidev,
 				status = -EFAULT;
 				goto done;
 			}
+			rx_buf += u_tmp->len;
 		}
-		rx_buf += u_tmp->len;
 	}
 	status = total;
 
-- 
2.1.4

Re: [PATCH] spi: spidev: only use up TX/RX bounce buffer space when needed

[Mark Brown]

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

On Thu, Feb 12, 2015 at 12:13:08PM +0000, Ian Abbott wrote:
> Devices have separate, pre-allocated TX and RX bounce buffers of fixed
> size.  Currently, each transfer uses up space in both buffers even if
> the user-supplied no TX data or no RX space.  Change it to only use up
> space in the TX and RX bounce buffers as required.

This is a bit hard to parse.  I think you're talking about buffers in
spidev here but it's unclear and you've not described in what way you're
changing the code and we do currently only seem to copy data when the
user has asked for it.

> Since dummy transfers with no user-supplied TX data and no user-supplied
> RX space will no longer use up space in the bounce buffers, limit the
> overall SPI message length to INT_MAX instead of the buffer size.

That doesn't seem to follow at all.  No mention has been made of
eliminating the buffers entirely or otherwise ensuring that we can
handle transfers of any length - not using the buffer for transmit isn't
going to make the receive buffer any bigger and indeed...

>  		if (u_tmp->rx_buf) {
> +			rx_total += k_tmp->len;
> +			if (rx_total > bufsiz) {
> +				status = -EMSGSIZE;
> +				goto done;
> +			}

...we do still seem to be limited to the buffer size here as one would
expect?

Basically I'm not entirely clear what this change is supposed to be
doing or what the expected benefit is.  I *think* the goal is to allow
multi-transfer messages with a mix of unidirectional transfers to be
larger but the changelog needs to be clearer and it's not 100% clear why
we'd bother to check for INT_MAX.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH] spi: spidev: only use up TX/RX bounce buffer space when needed

[Ian Abbott]

On 14/02/15 04:49, Mark Brown wrote:
> On Thu, Feb 12, 2015 at 12:13:08PM +0000, Ian Abbott wrote:
>> Devices have separate, pre-allocated TX and RX bounce buffers of fixed
>> size.  Currently, each transfer uses up space in both buffers even if
>> the user-supplied no TX data or no RX space.  Change it to only use up
>> space in the TX and RX bounce buffers as required.
>
> This is a bit hard to parse.  I think you're talking about buffers in
> spidev here but it's unclear and you've not described in what way you're
> changing the code and we do currently only seem to copy data when the
> user has asked for it.

Yes, I was talking about spidev. I did tag it in the subject line of the 
commit message, though I'm sorry if the rest of it is difficult to parse.

Yes, we copy data when the user asked for it. The patch is trying to 
avoid wasting space in the TX and/or RX buffers when the user _didn't_ 
ask for it.

>> Since dummy transfers with no user-supplied TX data and no user-supplied
>> RX space will no longer use up space in the bounce buffers, limit the
>> overall SPI message length to INT_MAX instead of the buffer size.
>
> That doesn't seem to follow at all.  No mention has been made of
> eliminating the buffers entirely or otherwise ensuring that we can
> handle transfers of any length - not using the buffer for transmit isn't
> going to make the receive buffer any bigger and indeed...
>
>>   		if (u_tmp->rx_buf) {
>> +			rx_total += k_tmp->len;
>> +			if (rx_total > bufsiz) {
>> +				status = -EMSGSIZE;
>> +				goto done;
>> +			}
>
> ...we do still seem to be limited to the buffer size here as one would
> expect?

Yes, the patch limits the total user-specified TX data and the total 
user-specified RX data to the pre-allocated buffer size individually 
rather than limiting the total sum of user RX and TX data.

> Basically I'm not entirely clear what this change is supposed to be
> doing or what the expected benefit is.  I *think* the goal is to allow
> multi-transfer messages with a mix of unidirectional transfers to be
> larger but the changelog needs to be clearer and it's not 100% clear why
> we'd bother to check for INT_MAX.

My intention was to use the pre-allocated buffers more efficiently in 
the case of multiple, half-duplex transfers in different directions, not 
to allow transfers of any length.

The check against INT_MAX is there because a struct spi_ioc_transfer 
might have rx_buf==NULL, tx_buf==NULL and len!=0, in which case it would 
no longer use up space in either of the pre-allocated buffers so neither 
rx_total nor tx_total would increase.  Checking the sum of the len 
fields against INT_MAX prevents arithmetic overflow in the return value 
of the function.

-- 
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti@mev.co.uk> )=-
-=(                          Web: http://www.mev.co.uk/  )=-

Re: [PATCH] spi: spidev: only use up TX/RX bounce buffer space when needed

[Ian Abbott]

On 14/02/15 04:49, Mark Brown wrote:
> On Thu, Feb 12, 2015 at 12:13:08PM +0000, Ian Abbott wrote:
>> Devices have separate, pre-allocated TX and RX bounce buffers of fixed
>> size.  Currently, each transfer uses up space in both buffers even if
>> the user-supplied no TX data or no RX space.  Change it to only use up
>> space in the TX and RX bounce buffers as required.
>
> This is a bit hard to parse.  [...]

I just noticed I typed "user-supplied" in the first paragraph when I 
meant to type "user supplied".

-- 
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti@mev.co.uk> )=-
-=(                          Web: http://www.mev.co.uk/  )=-

Re: [PATCH] spi: spidev: only use up TX/RX bounce buffer space when needed

[Mark Brown]

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

On Sun, Feb 15, 2015 at 10:30:22AM +0000, Ian Abbott wrote:
> On 14/02/15 04:49, Mark Brown wrote:

> >This is a bit hard to parse.  I think you're talking about buffers in
> >spidev here but it's unclear and you've not described in what way you're
> >changing the code and we do currently only seem to copy data when the
> >user has asked for it.

> Yes, I was talking about spidev. I did tag it in the subject line of the
> commit message, though I'm sorry if the rest of it is difficult to parse.

Right, but it's not clear if you mean that this is something to do with
the device drivers for SPI controllers or spidev itself.

> Yes, the patch limits the total user-specified TX data and the total
> user-specified RX data to the pre-allocated buffer size individually rather
> than limiting the total sum of user RX and TX data.

Your commit message needs to say this rather than requiring the user to
reverse engineer it from the code - a key part of reviewing a code
change is making sure that it does what the commit message says that it
does to make sure that it is having the indended effect.

> The check against INT_MAX is there because a struct spi_ioc_transfer might
> have rx_buf==NULL, tx_buf==NULL and len!=0, in which case it would no longer
> use up space in either of the pre-allocated buffers so neither rx_total nor
> tx_total would increase.  Checking the sum of the len fields against INT_MAX
> prevents arithmetic overflow in the return value of the function.

If that's what the code is supposed to do then someone reading the code
needs to be able to tell that without too much effort, I'd not expect
that to be possible as things are.  Maintainability is very important.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH] spi: spidev: only use up TX/RX bounce buffer space when needed

[Ian Abbott]

On 16/02/15 04:13, Mark Brown wrote:
> On Sun, Feb 15, 2015 at 10:30:22AM +0000, Ian Abbott wrote:
>> On 14/02/15 04:49, Mark Brown wrote:
>
>>> This is a bit hard to parse.  I think you're talking about buffers in
>>> spidev here but it's unclear and you've not described in what way you're
>>> changing the code and we do currently only seem to copy data when the
>>> user has asked for it.
>
>> Yes, I was talking about spidev. I did tag it in the subject line of the
>> commit message, though I'm sorry if the rest of it is difficult to parse.
>
> Right, but it's not clear if you mean that this is something to do with
> the device drivers for SPI controllers or spidev itself.

Okay, how about if I used the term "spidev device" to distinguish it 
from the lower-level SPI device?

>> Yes, the patch limits the total user-specified TX data and the total
>> user-specified RX data to the pre-allocated buffer size individually rather
>> than limiting the total sum of user RX and TX data.
>
> Your commit message needs to say this rather than requiring the user to
> reverse engineer it from the code - a key part of reviewing a code
> change is making sure that it does what the commit message says that it
> does to make sure that it is having the indended effect.

I thought it said that (somewhat clumsily) in the first paragraph.

>> The check against INT_MAX is there because a struct spi_ioc_transfer might
>> have rx_buf==NULL, tx_buf==NULL and len!=0, in which case it would no longer
>> use up space in either of the pre-allocated buffers so neither rx_total nor
>> tx_total would increase.  Checking the sum of the len fields against INT_MAX
>> prevents arithmetic overflow in the return value of the function.
>
> If that's what the code is supposed to do then someone reading the code
> needs to be able to tell that without too much effort, I'd not expect
> that to be possible as things are.  Maintainability is very important.

There was a whole paragraph about that in the commit message, but maybe 
it was too concise.

I'll have another attempt at the commit message to make it less concise 
and hopefully easier to follow!

-- 
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti@mev.co.uk> )=-
-=(                          Web: http://www.mev.co.uk/  )=-

Re: [PATCH] spi: spidev: only use up TX/RX bounce buffer space when needed

[Mark Brown]

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

On Mon, Feb 16, 2015 at 10:18:01AM +0000, Ian Abbott wrote:
> On 16/02/15 04:13, Mark Brown wrote:

> >Right, but it's not clear if you mean that this is something to do with
> >the device drivers for SPI controllers or spidev itself.

> Okay, how about if I used the term "spidev device" to distinguish it from
> the lower-level SPI device?

Or just spidev.

> >>Yes, the patch limits the total user-specified TX data and the total
> >>user-specified RX data to the pre-allocated buffer size individually rather
> >>than limiting the total sum of user RX and TX data.

> >Your commit message needs to say this rather than requiring the user to
> >reverse engineer it from the code - a key part of reviewing a code
> >change is making sure that it does what the commit message says that it
> >does to make sure that it is having the indended effect.

> I thought it said that (somewhat clumsily) in the first paragraph.

Not really, it repeats the what that can be seen from the code but
doesn't explain what the goal of the change is supposed to be.  This
means it's not really possible to tell if that goal is being achieved.

> >>The check against INT_MAX is there because a struct spi_ioc_transfer might
> >>have rx_buf==NULL, tx_buf==NULL and len!=0, in which case it would no longer
> >>use up space in either of the pre-allocated buffers so neither rx_total nor
> >>tx_total would increase.  Checking the sum of the len fields against INT_MAX
> >>prevents arithmetic overflow in the return value of the function.

> >If that's what the code is supposed to do then someone reading the code
> >needs to be able to tell that without too much effort, I'd not expect
> >that to be possible as things are.  Maintainability is very important.

> There was a whole paragraph about that in the commit message, but maybe it
> was too concise.

The commit message is not the code.  The code itself needs to be clear,
and even based on what's in the commit message it's not terribly obvious
(and with the above the return value that will be overflowed doesn't
jump out).

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH] spi: spidev: only use up TX/RX bounce buffer space when needed

[Ian Abbott]

On 16/02/15 13:23, Mark Brown wrote:
> On Mon, Feb 16, 2015 at 10:18:01AM +0000, Ian Abbott wrote:
>> On 16/02/15 04:13, Mark Brown wrote:
 >>> On Sun, Feb 15, 2015 at 10:30:22AM +0000, Ian Abbott wrote:
>>>> The check against INT_MAX is there because a struct spi_ioc_transfer might
>>>> have rx_buf==NULL, tx_buf==NULL and len!=0, in which case it would no longer
>>>> use up space in either of the pre-allocated buffers so neither rx_total nor
>>>> tx_total would increase.  Checking the sum of the len fields against INT_MAX
>>>> prevents arithmetic overflow in the return value of the function.
>
>>> If that's what the code is supposed to do then someone reading the code
>>> needs to be able to tell that without too much effort, I'd not expect
>>> that to be possible as things are.  Maintainability is very important.
>
>> There was a whole paragraph about that in the commit message, but maybe it
>> was too concise.
>
> The commit message is not the code.  The code itself needs to be clear,
> and even based on what's in the commit message it's not terribly obvious
> (and with the above the return value that will be overflowed doesn't
> jump out).

Good point.  I'll add some comments to the code.

-- 
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti@mev.co.uk> )=-
-=(                          Web: http://www.mev.co.uk/  )=-