[lkml]   [2015]   [Feb]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [PATCH] spi: spidev: only use up TX/RX bounce buffer space when needed
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: <> )=-
-=( Web: )=-

 \ /
  Last update: 2015-02-15 11:41    [W:0.067 / U:5.840 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site