From 9a12bff7c34635c329079858460e9705d819c500 Mon Sep 17 00:00:00 2001 From: Ian Abbott Date: Mon, 16 Feb 2015 15:00:47 +0000 Subject: spi: spidev: only use up TX/RX bounce buffer space when needed This patch changes the way space is reserved in spidev's pre-allocated TX and RX bounce buffers to avoid wasting space in the buffers for an SPI message consisting of multiple, half-duplex transfers in different directions. Background: spidev data structures have separate, pre-allocated TX and RX bounce buffers (`spidev->tx_buffer` and `spidev->rx_buffer`) of fixed size (`bufsiz`). The `SPI_IOC_MESSAGE(N)` ioctl processing uses a kernel copy of the N `struct spi_ioc_transfer` elements copied from the userspace ioctl arg pointer. In these elements: `.len` is the length of transfer in bytes; `.rx_buf` is either a userspace pointer to a buffer to copy the RX data to or is set to 0 to discard the data; and `.tx_buf` is either a userspace pointer to TX data supplied by the user or is set to 0 to transmit zeros for this transfer. `spidev_message()` uses the array of N `struct spi_ioc_transfer` elements to construct a kernel SPI message consisting of a `struct spi_message` containing a linked list (allocated as an array) of N `struct spi_transfer` elements. This involves iterating through the `struct spi_ioc_transfer` and `struct spi_transfer` elements (variables `u_tmp` and `k_tmp` respectively). Before the first iteration, variables `tx_buf` and `rx_buf` point to the start of the TX and RX bounce buffers `spidev->tx_buffer` and `spidev->rx_buffer` and variable `total` is set to 0. These variables keep track of the next available space in the bounce buffers and the total length of the SPI message. Each iteration checks that there is enough room left in the buffers for the transfer. If `u_tmp->rx_buf` is non-zero, `k_tmp->rx_buf` is set to `rx_buf`, otherwise it remains set to NULL. If `u_tmp->tx_buf` is non-zero, `k_tmp->tx_buf` is set to `tx_buf` and the userspace TX data copied there, otherwise it remains set to NULL. The variables `total`, `rx_buf` and `tx_buf` are advanced by the length of the transfer. The "problem": While iterating through the transfers, the local bounce buffer "free space" pointer variables `tx_buf` and `rx_buf` are always advanced by the length of the transfer. If `u_tmp->rx_buf` is 0 (so `k_tmp->rx_buf` is NULL), then `rx_buf` is advanced unnecessarily and that part of `spidev->rx_buffer` is wasted. Similarly, if `u_tmp->tx_buf` is 0 (so `k_tmp->tx_buf` is NULL), part of `spidev->tx_buffer` is wasted. What this patch does: To avoid wasting space unnecessarily in the RX bounce buffer, only advance `rx_buf` by the transfer length if `u_tmp->rx_buf` is non-zero. Similarly, to avoid wasting space unnecessarily in the TX bounce buffer, only advance `tx_buf` if `u_tmp->tx_buf is non-zero. To avoid pointer subtraction, use new variables `rx_total` and `tx_total` to keep track of the amount of space allocated in each of the bounce buffers. If these exceed the available space, a `-EMSGSIZE` error will be returned. Limit the total length of the transfers (tracked by variable `total`) to `INT_MAX` instead of `bufsiz`, returning an `-EMSGSIZE` error if exceeded. The total length is returned by `spidev_message()` on success and we want that to be non-negative. The message size limits for the `SPI_IOC_MESSAGE(N)` ioctl are now as follows: (a) total length of transfers is <= INTMAX; (b) total length of transfers with non-NULL rx_buf is <= bufsiz; (c) total length of transfers with non-NULL tx_buf is <= bufsiz. Some transfers may have NULL rx_buf and NULL tx_buf. If the transfer is completed successfully by the SPI core, `spidev_message()` iterates through the transfers to copy any RX data from the bounce buffer back to userspace on those transfers where `u_tmp->rx_buf` is non-zero. The variable `rx_buf` is again used to keep track of the corresponding positions in the bounce buffer. Now it is only advanced for those transfers that use the RX bounce buffer. Signed-off-by: Ian Abbott Signed-off-by: Mark Brown diff --git a/drivers/spi/spidev.c b/drivers/spi/spidev.c index 4eb7a98..bb6b3ab 100644 --- a/drivers/spi/spidev.c +++ b/drivers/spi/spidev.c @@ -223,7 +223,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; @@ -239,33 +239,51 @@ 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) { + /* Since the function returns the total length of transfers + * on success, restrict the total to positive int values to + * avoid the return value looking like an error. + */ + if (total > INT_MAX) { status = -EMSGSIZE; goto done; } if (u_tmp->rx_buf) { + /* this transfer needs space in RX bounce buffer */ + 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) { + /* this transfer needs space in TX bounce buffer */ + 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; @@ -303,8 +321,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; -- cgit v0.10.2 From b78ce7ed54093e5e63d4f5d7cbdc3f7d42ebbd5c Mon Sep 17 00:00:00 2001 From: Adrian Remonda Date: Tue, 10 Mar 2015 16:12:30 -0400 Subject: spi: spidev_test: Cleaned hexadecimal dump Signed-off-by: Adrian Remonda Signed-off-by: Mark Brown diff --git a/Documentation/spi/spidev_test.c b/Documentation/spi/spidev_test.c index 3a2f9d5..9cb0918 100644 --- a/Documentation/spi/spidev_test.c +++ b/Documentation/spi/spidev_test.c @@ -15,6 +15,7 @@ #include #include #include +#include #include #include #include @@ -35,6 +36,33 @@ static uint8_t bits = 8; static uint32_t speed = 500000; static uint16_t delay; +static void hex_dump(const void *src, size_t length, size_t line_size, char *prefix) +{ + int i = 0; + const unsigned char *address = src; + const unsigned char *line = address; + unsigned char c; + + printf("%s | ", prefix); + while (length-- > 0) { + printf("%02X ", *address++); + if (!(++i % line_size) || (length == 0 && i % line_size)) { + if (length == 0) { + while (i++ % line_size) + printf("__ "); + } + printf(" | "); /* right close */ + while (line < address) { + c = *line++; + printf("%c", (c < 33 || c == 255) ? 0x2E : c); + } + printf("\n"); + if (length > 0) + printf("%s | ", prefix); + } + } +} + static void transfer(int fd) { int ret; @@ -76,12 +104,7 @@ static void transfer(int fd) if (ret < 1) pabort("can't send spi message"); - for (ret = 0; ret < ARRAY_SIZE(tx); ret++) { - if (!(ret % 6)) - puts(""); - printf("%.2X ", rx[ret]); - } - puts(""); + hex_dump(rx, ARRAY_SIZE(rx), 32, "RX"); } static void print_usage(const char *prog) -- cgit v0.10.2 From 31a5c5a72b90e074bd2377ada2bdfd506801ed72 Mon Sep 17 00:00:00 2001 From: Adrian Remonda Date: Tue, 10 Mar 2015 16:12:31 -0400 Subject: spi: spidev_test: Added verbose output Signed-off-by: Adrian Remonda Signed-off-by: Mark Brown diff --git a/Documentation/spi/spidev_test.c b/Documentation/spi/spidev_test.c index 9cb0918..4b6a380 100644 --- a/Documentation/spi/spidev_test.c +++ b/Documentation/spi/spidev_test.c @@ -35,6 +35,7 @@ static uint32_t mode; static uint8_t bits = 8; static uint32_t speed = 500000; static uint16_t delay; +static int verbose; static void hex_dump(const void *src, size_t length, size_t line_size, char *prefix) { @@ -104,6 +105,8 @@ static void transfer(int fd) if (ret < 1) pabort("can't send spi message"); + if (verbose) + hex_dump(tx, ARRAY_SIZE(tx), 32, "TX"); hex_dump(rx, ARRAY_SIZE(rx), 32, "RX"); } @@ -120,6 +123,7 @@ static void print_usage(const char *prog) " -L --lsb least significant bit first\n" " -C --cs-high chip select active high\n" " -3 --3wire SI/SO signals shared\n" + " -v --verbose Verbose (show tx buffer)\n" " -N --no-cs no chip select\n" " -R --ready slave pulls low to pause\n" " -2 --dual dual transfer\n" @@ -144,12 +148,13 @@ static void parse_opts(int argc, char *argv[]) { "no-cs", 0, 0, 'N' }, { "ready", 0, 0, 'R' }, { "dual", 0, 0, '2' }, + { "verbose", 0, 0, 'v' }, { "quad", 0, 0, '4' }, { NULL, 0, 0, 0 }, }; int c; - c = getopt_long(argc, argv, "D:s:d:b:lHOLC3NR24", lopts, NULL); + c = getopt_long(argc, argv, "D:s:d:b:lHOLC3NR24:v", lopts, NULL); if (c == -1) break; @@ -188,6 +193,9 @@ static void parse_opts(int argc, char *argv[]) case 'N': mode |= SPI_NO_CS; break; + case 'v': + verbose = 1; + break; case 'R': mode |= SPI_READY; break; -- cgit v0.10.2 From 30061915be6e3a2c2a4bc4f22a229d4eb7b7cc65 Mon Sep 17 00:00:00 2001 From: Adrian Remonda Date: Tue, 10 Mar 2015 16:12:32 -0400 Subject: spi: spidev_test: Added input buffer from the terminal Signed-off-by: Adrian Remonda Signed-off-by: Mark Brown diff --git a/Documentation/spi/spidev_test.c b/Documentation/spi/spidev_test.c index 4b6a380..94f574b 100644 --- a/Documentation/spi/spidev_test.c +++ b/Documentation/spi/spidev_test.c @@ -37,6 +37,18 @@ static uint32_t speed = 500000; static uint16_t delay; static int verbose; +uint8_t default_tx[] = { + 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, + 0x40, 0x00, 0x00, 0x00, 0x00, 0x95, + 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, + 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, + 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, + 0xF0, 0x0D, +}; + +uint8_t default_rx[ARRAY_SIZE(default_tx)] = {0, }; +char *input_tx; + static void hex_dump(const void *src, size_t length, size_t line_size, char *prefix) { int i = 0; @@ -64,23 +76,38 @@ static void hex_dump(const void *src, size_t length, size_t line_size, char *pre } } -static void transfer(int fd) +/* + * Unescape - process hexadecimal escape character + * converts shell input "\x23" -> 0x23 + */ +int unespcape(char *_dst, char *_src, size_t len) +{ + int ret = 0; + char *src = _src; + char *dst = _dst; + unsigned int ch; + + while (*src) { + if (*src == '\\' && *(src+1) == 'x') { + sscanf(src + 2, "%2x", &ch); + src += 4; + *dst++ = (unsigned char)ch; + } else { + *dst++ = *src++; + } + ret++; + } + return ret; +} + +static void transfer(int fd, uint8_t const *tx, uint8_t const *rx, size_t len) { int ret; - uint8_t tx[] = { - 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, - 0x40, 0x00, 0x00, 0x00, 0x00, 0x95, - 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, - 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, - 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, - 0xDE, 0xAD, 0xBE, 0xEF, 0xBA, 0xAD, - 0xF0, 0x0D, - }; - uint8_t rx[ARRAY_SIZE(tx)] = {0, }; + struct spi_ioc_transfer tr = { .tx_buf = (unsigned long)tx, .rx_buf = (unsigned long)rx, - .len = ARRAY_SIZE(tx), + .len = len, .delay_usecs = delay, .speed_hz = speed, .bits_per_word = bits, @@ -106,8 +133,8 @@ static void transfer(int fd) pabort("can't send spi message"); if (verbose) - hex_dump(tx, ARRAY_SIZE(tx), 32, "TX"); - hex_dump(rx, ARRAY_SIZE(rx), 32, "RX"); + hex_dump(tx, len, 32, "TX"); + hex_dump(rx, len, 32, "RX"); } static void print_usage(const char *prog) @@ -124,6 +151,7 @@ static void print_usage(const char *prog) " -C --cs-high chip select active high\n" " -3 --3wire SI/SO signals shared\n" " -v --verbose Verbose (show tx buffer)\n" + " -p Send data (e.g. \"1234\\xde\\xad\")\n" " -N --no-cs no chip select\n" " -R --ready slave pulls low to pause\n" " -2 --dual dual transfer\n" @@ -154,7 +182,7 @@ static void parse_opts(int argc, char *argv[]) }; int c; - c = getopt_long(argc, argv, "D:s:d:b:lHOLC3NR24:v", lopts, NULL); + c = getopt_long(argc, argv, "D:s:d:b:lHOLC3NR24p:v", lopts, NULL); if (c == -1) break; @@ -199,6 +227,9 @@ static void parse_opts(int argc, char *argv[]) case 'R': mode |= SPI_READY; break; + case 'p': + input_tx = optarg; + break; case '2': mode |= SPI_TX_DUAL; break; @@ -222,6 +253,9 @@ int main(int argc, char *argv[]) { int ret = 0; int fd; + uint8_t *tx; + uint8_t *rx; + int size; parse_opts(argc, argv); @@ -266,7 +300,17 @@ int main(int argc, char *argv[]) printf("bits per word: %d\n", bits); printf("max speed: %d Hz (%d KHz)\n", speed, speed/1000); - transfer(fd); + if (input_tx) { + size = strlen(input_tx+1); + tx = malloc(size); + rx = malloc(size); + size = unespcape((char *)tx, input_tx, size); + transfer(fd, tx, rx, size); + free(rx); + free(tx); + } else { + transfer(fd, default_tx, default_rx, sizeof(default_tx)); + } close(fd); -- cgit v0.10.2 From f20fbaad7620af2df36a1f9d1c9ecf48ead5b747 Mon Sep 17 00:00:00 2001 From: Ian Abbott Date: Mon, 23 Mar 2015 17:50:27 +0000 Subject: spi: spidev: fix possible arithmetic overflow for multi-transfer message `spidev_message()` sums the lengths of the individual SPI transfers to determine the overall SPI message length. It restricts the total length, returning an error if too long, but it does not check for arithmetic overflow. For example, if the SPI message consisted of two transfers and the first has a length of 10 and the second has a length of (__u32)(-1), the total length would be seen as 9, even though the second transfer is actually very long. If the second transfer specifies a null `rx_buf` and a non-null `tx_buf`, the `copy_from_user()` could overrun the spidev's pre-allocated tx buffer before it reaches an invalid user memory address. Fix it by checking that neither the total nor the individual transfer lengths exceed the maximum allowed value. Thanks to Dan Carpenter for reporting the potential integer overflow. Signed-off-by: Ian Abbott Signed-off-by: Mark Brown diff --git a/drivers/spi/spidev.c b/drivers/spi/spidev.c index bb6b3ab..23ad978 100644 --- a/drivers/spi/spidev.c +++ b/drivers/spi/spidev.c @@ -249,9 +249,10 @@ static int spidev_message(struct spidev_data *spidev, total += k_tmp->len; /* Since the function returns the total length of transfers * on success, restrict the total to positive int values to - * avoid the return value looking like an error. + * avoid the return value looking like an error. Also check + * each transfer length to avoid arithmetic overflow. */ - if (total > INT_MAX) { + if (total > INT_MAX || k_tmp->len > INT_MAX) { status = -EMSGSIZE; goto done; } -- cgit v0.10.2 From 956b200a846e324322f6211034c734c65a38e550 Mon Sep 17 00:00:00 2001 From: Mark Brown Date: Fri, 27 Mar 2015 16:36:04 -0700 Subject: spi: spidev: Warn loudly if instantiated from DT as "spidev" Since spidev is a detail of how Linux controls a device rather than a description of the hardware in the system we should never have a node described as "spidev" in DT, any SPI device could be a spidev so this is just not a useful description. In order to help prevent users from writing such device trees generate a warning if spidev is instantiated as a DT node without an ID in the match table. Signed-off-by: Mark Brown diff --git a/drivers/spi/spidev.c b/drivers/spi/spidev.c index 23ad978..92c909e 100644 --- a/drivers/spi/spidev.c +++ b/drivers/spi/spidev.c @@ -703,6 +703,14 @@ static const struct file_operations spidev_fops = { static struct class *spidev_class; +#ifdef CONFIG_OF +static const struct of_device_id spidev_dt_ids[] = { + { .compatible = "rohm,dh2228fv" }, + {}, +}; +MODULE_DEVICE_TABLE(of, spidev_dt_ids); +#endif + /*-------------------------------------------------------------------------*/ static int spidev_probe(struct spi_device *spi) @@ -711,6 +719,17 @@ static int spidev_probe(struct spi_device *spi) int status; unsigned long minor; + /* + * spidev should never be referenced in DT without a specific + * compatbile string, it is a Linux implementation thing + * rather than a description of the hardware. + */ + if (spi->dev.of_node && !of_match_device(spidev_dt_ids, &spi->dev)) { + dev_err(&spi->dev, "buggy DT: spidev listed directly in DT\n"); + WARN_ON(spi->dev.of_node && + !of_match_device(spidev_dt_ids, &spi->dev)); + } + /* Allocate driver data */ spidev = kzalloc(sizeof(*spidev), GFP_KERNEL); if (!spidev) @@ -777,13 +796,6 @@ static int spidev_remove(struct spi_device *spi) return 0; } -static const struct of_device_id spidev_dt_ids[] = { - { .compatible = "rohm,dh2228fv" }, - {}, -}; - -MODULE_DEVICE_TABLE(of, spidev_dt_ids); - static struct spi_driver spidev_spi_driver = { .driver = { .name = "spidev", -- cgit v0.10.2