Message-ID: <1056c115-797c-d121-4623-3b1004032992@redhat.com>
Date:   Thu, 9 Feb 2017 07:44:48 -0800
From:   Laura Abbott <labbott@...hat.com>
To:     "Maciej S. Szmigiero" <mail@...iej.szmigiero.name>,
        Evgeniy Polyakov <zbr@...emap.net>
Cc:     linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] w1: ds2490: USB transfer buffers need to be DMAable

On 01/18/2017 12:31 PM, Maciej S. Szmigiero wrote:
> 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>
> Cc: stable@...r.kernel.org

FWIW, Fedora got a bug report and the reporter there said the patch
worked

https://bugzilla.redhat.com/show_bug.cgi?id=1415397

> ---
>  drivers/w1/masters/ds2490.c | 142 ++++++++++++++++++++++++++------------------
>  1 file changed, 84 insertions(+), 58 deletions(-)
> 
> diff --git a/drivers/w1/masters/ds2490.c b/drivers/w1/masters/ds2490.c
> index 049a884a756f..59d74d1b47a8 100644
> --- 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,28 +246,6 @@ static int ds_send_control(struct ds_device *dev, u16 value, u16 index)
>  	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) {
> -		pr_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)
>  {
>  	pr_info("%45s: %8x\n", str, buf[off]);
> @@ -324,6 +304,35 @@ static void ds_dump_status(struct ds_device *dev, unsigned char *buf, int count)
>  	}
>  }
>  
> +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);
> @@ -344,7 +353,6 @@ static void ds_reset_device(struct ds_device *dev)
>  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
> @@ -359,14 +367,9 @@ static int ds_recv_data(struct ds_device *dev, unsigned char *buf, int size)
>  	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;
> -
>  		pr_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;
>  	}
>  
> @@ -404,7 +407,6 @@ int ds_stop_pulse(struct ds_device *dev, int limit)
>  {
>  	struct ds_status st;
>  	int count = 0, err = 0;
> -	u8 buf[ST_SIZE];
>  
>  	do {
>  		err = ds_send_control(dev, CTL_HALT_EXE_IDLE, 0);
> @@ -413,7 +415,7 @@ int ds_stop_pulse(struct ds_device *dev, int limit)
>  		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;
>  
> @@ -456,18 +458,17 @@ int ds_detect(struct ds_device *dev, struct ds_status *st)
>  
>  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
> @@ -485,7 +486,7 @@ static int ds_wait_status(struct ds_device *dev, struct ds_status *st)
>  	 * 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
> @@ -608,7 +609,6 @@ static int ds_write_byte(struct ds_device *dev, u8 byte)
>  {
>  	int err;
>  	struct ds_status st;
> -	u8 rbyte;
>  
>  	err = ds_send_control(dev, COMM_BYTE_IO | COMM_IM | dev->spu_bit, byte);
>  	if (err)
> @@ -621,11 +621,11 @@ static int ds_write_byte(struct ds_device *dev, u8 byte)
>  	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)
> @@ -712,7 +712,6 @@ static void ds9490r_search(void *data, struct w1_master *master,
>  	int err;
>  	u16 value, index;
>  	struct ds_status st;
> -	u8 st_buf[ST_SIZE];
>  	int search_limit;
>  	int found = 0;
>  	int i;
> @@ -724,7 +723,12 @@ static void ds9490r_search(void *data, struct w1_master *master,
>  	/* 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);
>  
> @@ -745,10 +749,9 @@ static void ds9490r_search(void *data, struct w1_master *master,
>  	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
> @@ -758,7 +761,7 @@ static void ds9490r_search(void *data, struct w1_master *master,
>  			 * 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) {
> @@ -794,9 +797,14 @@ static void ds9490r_search(void *data, struct w1_master *master,
>  	}
>  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;
> @@ -845,13 +853,12 @@ static int ds_set_path(struct ds_device *dev, u64 init)
>  
>  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
> @@ -866,13 +873,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
>  
> @@ -887,32 +893,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;
>  
> -	ds_write_block(dev, (u8 *)buf, len);
> +	memcpy(tbuf, buf, len);
> +	ds_write_block(dev, tbuf, 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