Message-ID: <lsq.1496331796.971520182@decadent.org.uk>
Date:   Thu, 01 Jun 2017 16:43:16 +0100
From:   Ben Hutchings <ben@...adent.org.uk>
To:     linux-kernel@...r.kernel.org, stable@...r.kernel.org
CC:     akpm@...ux-foundation.org,
        "Maciej S. Szmigiero" <mail@...iej.szmigiero.name>,
        "Greg Kroah-Hartman" <gregkh@...uxfoundation.org>,
        "Evgeniy Polyakov" <zbr@...emap.net>
Subject: [PATCH 3.16 072/212] w1: ds2490: USB transfer buffers need to be
 DMAable

3.16.44-rc1 review patch.  If anyone has any objections, please let me know.

------------------

From: "Maciej S. Szmigiero" <mail@...iej.szmigiero.name>

commit 61cd1b4cd1e8f7f7642ab64529d9bd52e8374641 upstream.

ds2490 driver was doing USB transfers from / to buffers on a stack.
This is not permitted and made the driver non-working with vmapped stacks.

Since all these transfers are done under the same bus_mutex lock we can
simply use shared buffers in a device private structure for two most common
of them.

While we are at it, let's also fix a comparison between int and size_t in
ds9490r_search() which made the driver spin in this function if state
register get requests were failing.

Signed-off-by: Maciej S. Szmigiero <mail@...iej.szmigiero.name>
Acked-by: Evgeniy Polyakov <zbr@...emap.net>
Signed-off-by: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
[bwh: Backported to 3.16: old code was using printk() directly]
Signed-off-by: Ben Hutchings <ben@...adent.org.uk>
---
 drivers/w1/masters/ds2490.c | 142 ++++++++++++++++++++++++++------------------
 1 file changed, 84 insertions(+), 58 deletions(-)

--- a/drivers/w1/masters/ds2490.c
+++ b/drivers/w1/masters/ds2490.c
@@ -153,6 +153,9 @@ struct ds_device
 	 */
 	u16			spu_bit;
 
+	u8			st_buf[ST_SIZE];
+	u8			byte_buf;
+
 	struct w1_bus_master	master;
 };
 
@@ -174,7 +177,6 @@ struct ds_status
 	u8			data_in_buffer_status;
 	u8			reserved1;
 	u8			reserved2;
-
 };
 
 static struct usb_device_id ds_id_table [] = {
@@ -244,27 +246,6 @@ static int ds_send_control(struct ds_dev
 	return err;
 }
 
-static int ds_recv_status_nodump(struct ds_device *dev, struct ds_status *st,
-				 unsigned char *buf, int size)
-{
-	int count, err;
-
-	memset(st, 0, sizeof(*st));
-
-	count = 0;
-	err = usb_interrupt_msg(dev->udev, usb_rcvintpipe(dev->udev,
-		dev->ep[EP_STATUS]), buf, size, &count, 1000);
-	if (err < 0) {
-		printk(KERN_ERR "Failed to read 1-wire data from 0x%x: err=%d.\n", dev->ep[EP_STATUS], err);
-		return err;
-	}
-
-	if (count >= sizeof(*st))
-		memcpy(st, buf, sizeof(*st));
-
-	return count;
-}
-
 static inline void ds_print_msg(unsigned char *buf, unsigned char *str, int off)
 {
 	printk(KERN_INFO "%45s: %8x\n", str, buf[off]);
@@ -323,6 +304,35 @@ static void ds_dump_status(struct ds_dev
 	}
 }
 
+static int ds_recv_status(struct ds_device *dev, struct ds_status *st,
+			  bool dump)
+{
+	int count, err;
+
+	if (st)
+		memset(st, 0, sizeof(*st));
+
+	count = 0;
+	err = usb_interrupt_msg(dev->udev,
+				usb_rcvintpipe(dev->udev,
+					       dev->ep[EP_STATUS]),
+				dev->st_buf, sizeof(dev->st_buf),
+				&count, 1000);
+	if (err < 0) {
+		pr_err("Failed to read 1-wire data from 0x%x: err=%d.\n",
+		       dev->ep[EP_STATUS], err);
+		return err;
+	}
+
+	if (dump)
+		ds_dump_status(dev, dev->st_buf, count);
+
+	if (st && count >= sizeof(*st))
+		memcpy(st, dev->st_buf, sizeof(*st));
+
+	return count;
+}
+
 static void ds_reset_device(struct ds_device *dev)
 {
 	ds_send_control_cmd(dev, CTL_RESET_DEVICE, 0);
@@ -345,7 +355,6 @@ static void ds_reset_device(struct ds_de
 static int ds_recv_data(struct ds_device *dev, unsigned char *buf, int size)
 {
 	int count, err;
-	struct ds_status st;
 
 	/* Careful on size.  If size is less than what is available in
 	 * the input buffer, the device fails the bulk transfer and
@@ -360,14 +369,9 @@ static int ds_recv_data(struct ds_device
 	err = usb_bulk_msg(dev->udev, usb_rcvbulkpipe(dev->udev, dev->ep[EP_DATA_IN]),
 				buf, size, &count, 1000);
 	if (err < 0) {
-		u8 buf[ST_SIZE];
-		int count;
-
 		printk(KERN_INFO "Clearing ep0x%x.\n", dev->ep[EP_DATA_IN]);
 		usb_clear_halt(dev->udev, usb_rcvbulkpipe(dev->udev, dev->ep[EP_DATA_IN]));
-
-		count = ds_recv_status_nodump(dev, &st, buf, sizeof(buf));
-		ds_dump_status(dev, buf, count);
+		ds_recv_status(dev, NULL, true);
 		return err;
 	}
 
@@ -405,7 +409,6 @@ int ds_stop_pulse(struct ds_device *dev,
 {
 	struct ds_status st;
 	int count = 0, err = 0;
-	u8 buf[ST_SIZE];
 
 	do {
 		err = ds_send_control(dev, CTL_HALT_EXE_IDLE, 0);
@@ -414,7 +417,7 @@ int ds_stop_pulse(struct ds_device *dev,
 		err = ds_send_control(dev, CTL_RESUME_EXE, 0);
 		if (err)
 			break;
-		err = ds_recv_status_nodump(dev, &st, buf, sizeof(buf));
+		err = ds_recv_status(dev, &st, false);
 		if (err)
 			break;
 
@@ -457,18 +460,17 @@ int ds_detect(struct ds_device *dev, str
 
 static int ds_wait_status(struct ds_device *dev, struct ds_status *st)
 {
-	u8 buf[ST_SIZE];
 	int err, count = 0;
 
 	do {
 		st->status = 0;
-		err = ds_recv_status_nodump(dev, st, buf, sizeof(buf));
+		err = ds_recv_status(dev, st, false);
 #if 0
 		if (err >= 0) {
 			int i;
 			printk("0x%x: count=%d, status: ", dev->ep[EP_STATUS], err);
 			for (i=0; i<err; ++i)
-				printk("%02x ", buf[i]);
+				printk("%02x ", dev->st_buf[i]);
 			printk("\n");
 		}
 #endif
@@ -486,7 +488,7 @@ static int ds_wait_status(struct ds_devi
 	 * can do something with it).
 	 */
 	if (err > 16 || count >= 100 || err < 0)
-		ds_dump_status(dev, buf, err);
+		ds_dump_status(dev, dev->st_buf, err);
 
 	/* Extended data isn't an error.  Well, a short is, but the dump
 	 * would have already told the user that and we can't do anything
@@ -609,7 +611,6 @@ static int ds_write_byte(struct ds_devic
 {
 	int err;
 	struct ds_status st;
-	u8 rbyte;
 
 	err = ds_send_control(dev, COMM_BYTE_IO | COMM_IM | dev->spu_bit, byte);
 	if (err)
@@ -622,11 +623,11 @@ static int ds_write_byte(struct ds_devic
 	if (err)
 		return err;
 
-	err = ds_recv_data(dev, &rbyte, sizeof(rbyte));
+	err = ds_recv_data(dev, &dev->byte_buf, 1);
 	if (err < 0)
 		return err;
 
-	return !(byte == rbyte);
+	return !(byte == dev->byte_buf);
 }
 
 static int ds_read_byte(struct ds_device *dev, u8 *byte)
@@ -713,7 +714,6 @@ static void ds9490r_search(void *data, s
 	int err;
 	u16 value, index;
 	struct ds_status st;
-	u8 st_buf[ST_SIZE];
 	int search_limit;
 	int found = 0;
 	int i;
@@ -725,7 +725,12 @@ static void ds9490r_search(void *data, s
 	/* FIFO 128 bytes, bulk packet size 64, read a multiple of the
 	 * packet size.
 	 */
-	u64 buf[2*64/8];
+	const size_t bufsize = 2 * 64;
+	u64 *buf;
+
+	buf = kmalloc(bufsize, GFP_KERNEL);
+	if (!buf)
+		return;
 
 	mutex_lock(&master->bus_mutex);
 
@@ -746,10 +751,9 @@ static void ds9490r_search(void *data, s
 	do {
 		schedule_timeout(jtime);
 
-		if (ds_recv_status_nodump(dev, &st, st_buf, sizeof(st_buf)) <
-			sizeof(st)) {
+		err = ds_recv_status(dev, &st, false);
+		if (err < 0 || err < sizeof(st))
 			break;
-		}
 
 		if (st.data_in_buffer_status) {
 			/* Bulk in can receive partial ids, but when it does
@@ -759,7 +763,7 @@ static void ds9490r_search(void *data, s
 			 * bulk without first checking if status says there
 			 * is data to read.
 			 */
-			err = ds_recv_data(dev, (u8 *)buf, sizeof(buf));
+			err = ds_recv_data(dev, (u8 *)buf, bufsize);
 			if (err < 0)
 				break;
 			for (i = 0; i < err/8; ++i) {
@@ -795,9 +799,14 @@ static void ds9490r_search(void *data, s
 	}
 search_out:
 	mutex_unlock(&master->bus_mutex);
+	kfree(buf);
 }
 
 #if 0
+/*
+ * FIXME: if this disabled code is ever used in the future all ds_send_data()
+ * calls must be changed to use a DMAable buffer.
+ */
 static int ds_match_access(struct ds_device *dev, u64 init)
 {
 	int err;
@@ -846,13 +855,12 @@ static int ds_set_path(struct ds_device
 
 static u8 ds9490r_touch_bit(void *data, u8 bit)
 {
-	u8 ret;
 	struct ds_device *dev = data;
 
-	if (ds_touch_bit(dev, bit, &ret))
+	if (ds_touch_bit(dev, bit, &dev->byte_buf))
 		return 0;
 
-	return ret;
+	return dev->byte_buf;
 }
 
 #if 0
@@ -867,13 +875,12 @@ static u8 ds9490r_read_bit(void *data)
 {
 	struct ds_device *dev = data;
 	int err;
-	u8 bit = 0;
 
-	err = ds_touch_bit(dev, 1, &bit);
+	err = ds_touch_bit(dev, 1, &dev->byte_buf);
 	if (err)
 		return 0;
 
-	return bit & 1;
+	return dev->byte_buf & 1;
 }
 #endif
 
@@ -888,32 +895,52 @@ static u8 ds9490r_read_byte(void *data)
 {
 	struct ds_device *dev = data;
 	int err;
-	u8 byte = 0;
 
-	err = ds_read_byte(dev, &byte);
+	err = ds_read_byte(dev, &dev->byte_buf);
 	if (err)
 		return 0;
 
-	return byte;
+	return dev->byte_buf;
 }
 
 static void ds9490r_write_block(void *data, const u8 *buf, int len)
 {
 	struct ds_device *dev = data;
+	u8 *tbuf;
+
+	if (len <= 0)
+		return;
+
+	tbuf = kmalloc(len, GFP_KERNEL);
+	if (!tbuf)
+		return;
+
+	memcpy(tbuf, buf, len);
+	ds_write_block(dev, tbuf, len);
 
-	ds_write_block(dev, (u8 *)buf, len);
+	kfree(tbuf);
 }
 
 static u8 ds9490r_read_block(void *data, u8 *buf, int len)
 {
 	struct ds_device *dev = data;
 	int err;
+	u8 *tbuf;
 
-	err = ds_read_block(dev, buf, len);
-	if (err < 0)
+	if (len <= 0)
+		return 0;
+
+	tbuf = kmalloc(len, GFP_KERNEL);
+	if (!tbuf)
 		return 0;
 
-	return len;
+	err = ds_read_block(dev, tbuf, len);
+	if (err >= 0)
+		memcpy(buf, tbuf, len);
+
+	kfree(tbuf);
+
+	return err >= 0 ? len : 0;
 }
 
 static u8 ds9490r_reset(void *data)

Powered by blists - more mailing lists