[PATCH 0/3] [media] redrat3: cleanup driver

[PATCH 0/3] [media] redrat3: cleanup driver

[Sean Young]

This driver has various minor issues and could be simpler.

Sean Young (3):
  [media] redrat3: limit periods to hardware limits
  [media] redrat3: remove memcpys and fix unaligned memory access
  [media] redrat3: missing endian conversions and warnings

 drivers/media/rc/redrat3.c |  457 +++++++++++++-------------------------------
 1 files changed, 130 insertions(+), 327 deletions(-)

-- 
1.7.2.5

[PATCH 1/3] [media] redrat3: limit periods to hardware limits

[Sean Young]

The redrat hardware cannot handle periods of larger than 32767us,
limit appropriately. Also fix memory leak in redrat3_get_timeout.

Signed-off-by: Sean Young <sean@mess.org>
---
 drivers/media/rc/redrat3.c |   53 ++++++++++++++++++++------------------------
 1 files changed, 24 insertions(+), 29 deletions(-)

diff --git a/drivers/media/rc/redrat3.c b/drivers/media/rc/redrat3.c
index 1b37fe2..842bdcd 100644
--- a/drivers/media/rc/redrat3.c
+++ b/drivers/media/rc/redrat3.c
@@ -209,9 +209,6 @@ struct redrat3_dev {
 	u16 pktlen;
 	u16 pkttype;
 	u16 bytes_read;
-	/* indicate whether we are going to reprocess
-	 * the USB callback with a bigger buffer */
-	int buftoosmall;
 	char *datap;
 
 	u32 carrier;
@@ -396,7 +393,6 @@ static u32 redrat3_us_to_len(u32 microsec)
 
 	/* don't allow zero lengths to go back, breaks lirc */
 	return result ? result : 1;
-
 }
 
 /* timer callback to send reset event */
@@ -515,8 +511,6 @@ static void redrat3_process_ir_data(struct redrat3_dev *rr3)
 
 	rr3_dbg(dev, "calling ir_raw_event_handle\n");
 	ir_raw_event_handle(rr3->rc);
-
-	return;
 }
 
 /* Util fn to send rr3 cmds */
@@ -613,7 +607,7 @@ static inline void redrat3_delete(struct redrat3_dev *rr3,
 
 static u32 redrat3_get_timeout(struct redrat3_dev *rr3)
 {
-	u32 *tmp;
+	__be32 *tmp;
 	u32 timeout = MS_TO_US(150); /* a sane default, if things go haywire */
 	int len, ret, pipe;
 
@@ -628,14 +622,16 @@ static u32 redrat3_get_timeout(struct redrat3_dev *rr3)
 	ret = usb_control_msg(rr3->udev, pipe, RR3_GET_IR_PARAM,
 			      USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN,
 			      RR3_IR_IO_SIG_TIMEOUT, 0, tmp, len, HZ * 5);
-	if (ret != len) {
+	if (ret != len)
 		dev_warn(rr3->dev, "Failed to read timeout from hardware\n");
-		return timeout;
+	else {
+		timeout = redrat3_len_to_us(be32_to_cpup(tmp));
+
+		rr3_dbg(rr3->dev, "Got timeout of %d ms\n", timeout / 1000);
 	}
 
-	timeout = redrat3_len_to_us(be32_to_cpu(*tmp));
+	kfree(tmp);
 
-	rr3_dbg(rr3->dev, "Got timeout of %d ms\n", timeout / 1000);
 	return timeout;
 }
 
@@ -755,7 +751,6 @@ static void redrat3_read_packet_start(struct redrat3_dev *rr3, int len)
 
 static void redrat3_read_packet_continue(struct redrat3_dev *rr3, int len)
 {
-
 	rr3_ftr(rr3->dev, "Entering %s\n", __func__);
 
 	memcpy(rr3->datap, (unsigned char *)rr3->bulk_in_buf, len);
@@ -815,7 +810,7 @@ out:
 }
 
 /* callback function from USB when async USB request has completed */
-static void redrat3_handle_async(struct urb *urb, struct pt_regs *regs)
+static void redrat3_handle_async(struct urb *urb)
 {
 	struct redrat3_dev *rr3;
 	int ret;
@@ -857,7 +852,7 @@ static void redrat3_handle_async(struct urb *urb, struct pt_regs *regs)
 	}
 }
 
-static void redrat3_write_bulk_callback(struct urb *urb, struct pt_regs *regs)
+static void redrat3_write_bulk_callback(struct urb *urb)
 {
 	struct redrat3_dev *rr3;
 	int len;
@@ -901,7 +896,7 @@ static int redrat3_transmit_ir(struct rc_dev *rcdev, unsigned *txbuf,
 	struct redrat3_dev *rr3 = rcdev->priv;
 	struct device *dev = rr3->dev;
 	struct redrat3_signal_header header;
-	int i, j, ret, ret_len, offset;
+	int i, ret, ret_len, offset;
 	int lencheck, cur_sample_len, pipe;
 	char *buffer = NULL, *sigdata = NULL;
 	int *sample_lens = NULL;
@@ -931,8 +926,19 @@ static int redrat3_transmit_ir(struct rc_dev *rcdev, unsigned *txbuf,
 		goto out;
 	}
 
+	sigdata = kzalloc((count + RR3_TX_TRAILER_LEN), GFP_KERNEL);
+	if (!sigdata) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
 	for (i = 0; i < count; i++) {
 		cur_sample_len = redrat3_us_to_len(txbuf[i]);
+		if (cur_sample_len > 0xffff) {
+			dev_warn(dev, "transmit period of %uus truncated to %uus\n",
+					txbuf[i], redrat3_len_to_us(0xffff));
+			cur_sample_len = 0xffff;
+		}
 		for (lencheck = 0; lencheck < curlencheck; lencheck++) {
 			if (sample_lens[lencheck] == cur_sample_len)
 				break;
@@ -950,22 +956,11 @@ static int redrat3_transmit_ir(struct rc_dev *rcdev, unsigned *txbuf,
 				break;
 			}
 		}
-	}
-
-	sigdata = kzalloc((count + RR3_TX_TRAILER_LEN), GFP_KERNEL);
-	if (!sigdata) {
-		ret = -ENOMEM;
-		goto out;
+		sigdata[i] = lencheck;
 	}
 
 	sigdata[count] = RR3_END_OF_SIGNAL;
 	sigdata[count + 1] = RR3_END_OF_SIGNAL;
-	for (i = 0; i < count; i++) {
-		for (j = 0; j < curlencheck; j++) {
-			if (sample_lens[j] == redrat3_us_to_len(txbuf[i]))
-				sigdata[i] = j;
-		}
-	}
 
 	offset = RR3_TX_HEADER_OFFSET;
 	sendbuf_len = RR3_HEADER_LENGTH + (sizeof(u16) * RR3_DRIVER_MAXLENS)
@@ -1175,7 +1170,7 @@ static int redrat3_dev_probe(struct usb_interface *intf,
 	pipe = usb_rcvbulkpipe(udev, ep_in->bEndpointAddress);
 	usb_fill_bulk_urb(rr3->read_urb, udev, pipe,
 			  rr3->bulk_in_buf, ep_in->wMaxPacketSize,
-			  (usb_complete_t)redrat3_handle_async, rr3);
+			  redrat3_handle_async, rr3);
 
 	/* set up bulk-out endpoint*/
 	rr3->write_urb = usb_alloc_urb(0, GFP_KERNEL);
@@ -1195,7 +1190,7 @@ static int redrat3_dev_probe(struct usb_interface *intf,
 	pipe = usb_sndbulkpipe(udev, ep_out->bEndpointAddress);
 	usb_fill_bulk_urb(rr3->write_urb, udev, pipe,
 			  rr3->bulk_out_buf, ep_out->wMaxPacketSize,
-			  (usb_complete_t)redrat3_write_bulk_callback, rr3);
+			  redrat3_write_bulk_callback, rr3);
 
 	rr3->udev = udev;
 
-- 
1.7.2.5

[PATCH 2/3] [media] redrat3: remove memcpys and fix unaligned memory access

[Sean Young]

In stead of doing a memcpy from #defined offset, declare structs which
describe the incoming and outgoing data accurately.

Tested on first generation RedRat.

Signed-off-by: Sean Young <sean@mess.org>
---
 drivers/media/rc/redrat3.c |  351 +++++++++++++-------------------------------
 1 files changed, 103 insertions(+), 248 deletions(-)

diff --git a/drivers/media/rc/redrat3.c b/drivers/media/rc/redrat3.c
index 842bdcd..ec655b8 100644
--- a/drivers/media/rc/redrat3.c
+++ b/drivers/media/rc/redrat3.c
@@ -45,6 +45,7 @@
  *
  */
 
+#include <asm/unaligned.h>
 #include <linux/device.h>
 #include <linux/module.h>
 #include <linux/slab.h>
@@ -129,25 +130,11 @@ static int debug;
 /* USB bulk-in IR data endpoint address */
 #define RR3_BULK_IN_EP_ADDR	0x82
 
-/* Raw Modulated signal data value offsets */
-#define RR3_PAUSE_OFFSET	0
-#define RR3_FREQ_COUNT_OFFSET	4
-#define RR3_NUM_PERIOD_OFFSET	6
-#define RR3_MAX_LENGTHS_OFFSET	8
-#define RR3_NUM_LENGTHS_OFFSET	9
-#define RR3_MAX_SIGS_OFFSET	10
-#define RR3_NUM_SIGS_OFFSET	12
-#define RR3_REPEATS_OFFSET	14
-
 /* Size of the fixed-length portion of the signal */
-#define RR3_HEADER_LENGTH	15
 #define RR3_DRIVER_MAXLENS	128
 #define RR3_MAX_SIG_SIZE	512
-#define RR3_MAX_BUF_SIZE	\
-	((2 * RR3_HEADER_LENGTH) + RR3_DRIVER_MAXLENS + RR3_MAX_SIG_SIZE)
 #define RR3_TIME_UNIT		50
 #define RR3_END_OF_SIGNAL	0x7f
-#define RR3_TX_HEADER_OFFSET	4
 #define RR3_TX_TRAILER_LEN	2
 #define RR3_RX_MIN_TIMEOUT	5
 #define RR3_RX_MAX_TIMEOUT	2000
@@ -159,6 +146,32 @@ static int debug;
 #define USB_RR3USB_PRODUCT_ID	0x0001
 #define USB_RR3IIUSB_PRODUCT_ID	0x0005
 
+struct redrat3_header {
+	__be16 length;
+	__be16 transfer_type;
+} __packed;
+
+/* sending and receiving irdata */
+struct redrat3_irdata {
+	struct redrat3_header header;
+	__be32 pause;
+	__be16 mod_freq_count;
+	__be16 num_periods;
+	__u8 max_lengths;
+	__u8 no_lengths;
+	__be16 max_sig_size;
+	__be16 sig_size;
+	__u8 no_repeats;
+	__be16 lens[RR3_DRIVER_MAXLENS]; /* not aligned */
+	__u8 sigdata[RR3_MAX_SIG_SIZE];
+} __packed;
+
+/* firmware errors */
+struct redrat3_error {
+	struct redrat3_header header;
+	__be16 fw_error;
+} __packed;
+
 /* table of devices that work with this driver */
 static struct usb_device_id redrat3_dev_table[] = {
 	/* Original version of the RedRat3 */
@@ -180,7 +193,7 @@ struct redrat3_dev {
 	/* the receive endpoint */
 	struct usb_endpoint_descriptor *ep_in;
 	/* the buffer to receive data */
-	unsigned char *bulk_in_buf;
+	void *bulk_in_buf;
 	/* urb used to read ir data */
 	struct urb *read_urb;
 
@@ -205,69 +218,15 @@ struct redrat3_dev {
 	bool transmitting;
 
 	/* store for current packet */
-	char pbuf[RR3_MAX_BUF_SIZE];
-	u16 pktlen;
-	u16 pkttype;
+	struct redrat3_irdata irdata;
 	u16 bytes_read;
-	char *datap;
 
 	u32 carrier;
 
-	char name[128];
+	char name[64];
 	char phys[64];
 };
 
-/* All incoming data buffers adhere to a very specific data format */
-struct redrat3_signal_header {
-	u16 length;	/* Length of data being transferred */
-	u16 transfer_type; /* Type of data transferred */
-	u32 pause;	/* Pause between main and repeat signals */
-	u16 mod_freq_count; /* Value of timer on mod. freq. measurement */
-	u16 no_periods;	/* No. of periods over which mod. freq. is measured */
-	u8 max_lengths;	/* Max no. of lengths (i.e. size of array) */
-	u8 no_lengths;	/* Actual no. of elements in lengths array */
-	u16 max_sig_size; /* Max no. of values in signal data array */
-	u16 sig_size;	/* Acuto no. of values in signal data array */
-	u8 no_repeats;	/* No. of repeats of repeat signal section */
-	/* Here forward is the lengths and signal data */
-};
-
-static void redrat3_dump_signal_header(struct redrat3_signal_header *header)
-{
-	pr_info("%s:\n", __func__);
-	pr_info(" * length: %u, transfer_type: 0x%02x\n",
-		header->length, header->transfer_type);
-	pr_info(" * pause: %u, freq_count: %u, no_periods: %u\n",
-		header->pause, header->mod_freq_count, header->no_periods);
-	pr_info(" * lengths: %u (max: %u)\n",
-		header->no_lengths, header->max_lengths);
-	pr_info(" * sig_size: %u (max: %u)\n",
-		header->sig_size, header->max_sig_size);
-	pr_info(" * repeats: %u\n", header->no_repeats);
-}
-
-static void redrat3_dump_signal_data(char *buffer, u16 len)
-{
-	int offset, i;
-	char *data_vals;
-
-	pr_info("%s:", __func__);
-
-	offset = RR3_TX_HEADER_OFFSET + RR3_HEADER_LENGTH
-		 + (RR3_DRIVER_MAXLENS * sizeof(u16));
-
-	/* read RR3_DRIVER_MAXLENS from ctrl msg */
-	data_vals = buffer + offset;
-
-	for (i = 0; i < len; i++) {
-		if (i % 10 == 0)
-			pr_cont("\n * ");
-		pr_cont("%02x ", *data_vals++);
-	}
-
-	pr_cont("\n");
-}
-
 /*
  * redrat3_issue_async
  *
@@ -349,13 +308,14 @@ static void redrat3_dump_fw_error(struct redrat3_dev *rr3, int code)
 	}
 }
 
-static u32 redrat3_val_to_mod_freq(struct redrat3_signal_header *ph)
+static u32 redrat3_val_to_mod_freq(struct redrat3_irdata *irdata)
 {
 	u32 mod_freq = 0;
+	u16 mod_freq_count = be16_to_cpu(irdata->mod_freq_count);
 
-	if (ph->mod_freq_count != 0)
-		mod_freq = (RR3_CLK * ph->no_periods) /
-				(ph->mod_freq_count * RR3_CLK_PER_COUNT);
+	if (mod_freq_count != 0)
+		mod_freq = (RR3_CLK * be16_to_cpu(irdata->num_periods)) /
+			(mod_freq_count * RR3_CLK_PER_COUNT);
 
 	return mod_freq;
 }
@@ -407,16 +367,11 @@ static void redrat3_rx_timeout(unsigned long data)
 static void redrat3_process_ir_data(struct redrat3_dev *rr3)
 {
 	DEFINE_IR_RAW_EVENT(rawir);
-	struct redrat3_signal_header header;
 	struct device *dev;
 	int i, trailer = 0;
+	unsigned sig_size, single_len, offset, val;
 	unsigned long delay;
-	u32 mod_freq, single_len;
-	u16 *len_vals;
-	u8 *data_vals;
-	u32 tmp32;
-	u16 tmp16;
-	char *sig_data;
+	u32 mod_freq;
 
 	if (!rr3) {
 		pr_err("%s called with no context!\n", __func__);
@@ -426,57 +381,20 @@ static void redrat3_process_ir_data(struct redrat3_dev *rr3)
 	rr3_ftr(rr3->dev, "Entered %s\n", __func__);
 
 	dev = rr3->dev;
-	sig_data = rr3->pbuf;
-
-	header.length = rr3->pktlen;
-	header.transfer_type = rr3->pkttype;
-
-	/* Sanity check */
-	if (!(header.length >= RR3_HEADER_LENGTH))
-		dev_warn(dev, "read returned less than rr3 header len\n");
 
 	/* Make sure we reset the IR kfifo after a bit of inactivity */
 	delay = usecs_to_jiffies(rr3->hw_timeout);
 	mod_timer(&rr3->rx_timeout, jiffies + delay);
 
-	memcpy(&tmp32, sig_data + RR3_PAUSE_OFFSET, sizeof(tmp32));
-	header.pause = be32_to_cpu(tmp32);
-
-	memcpy(&tmp16, sig_data + RR3_FREQ_COUNT_OFFSET, sizeof(tmp16));
-	header.mod_freq_count = be16_to_cpu(tmp16);
-
-	memcpy(&tmp16, sig_data + RR3_NUM_PERIOD_OFFSET, sizeof(tmp16));
-	header.no_periods = be16_to_cpu(tmp16);
-
-	header.max_lengths = sig_data[RR3_MAX_LENGTHS_OFFSET];
-	header.no_lengths = sig_data[RR3_NUM_LENGTHS_OFFSET];
-
-	memcpy(&tmp16, sig_data + RR3_MAX_SIGS_OFFSET, sizeof(tmp16));
-	header.max_sig_size = be16_to_cpu(tmp16);
-
-	memcpy(&tmp16, sig_data + RR3_NUM_SIGS_OFFSET, sizeof(tmp16));
-	header.sig_size = be16_to_cpu(tmp16);
-
-	header.no_repeats= sig_data[RR3_REPEATS_OFFSET];
-
-	if (debug) {
-		redrat3_dump_signal_header(&header);
-		redrat3_dump_signal_data(sig_data, header.sig_size);
-	}
-
-	mod_freq = redrat3_val_to_mod_freq(&header);
+	mod_freq = redrat3_val_to_mod_freq(&rr3->irdata);
 	rr3_dbg(dev, "Got mod_freq of %u\n", mod_freq);
 
-	/* Here we pull out the 'length' values from the signal */
-	len_vals = (u16 *)(sig_data + RR3_HEADER_LENGTH);
-
-	data_vals = sig_data + RR3_HEADER_LENGTH +
-		    (header.max_lengths * sizeof(u16));
-
 	/* process each rr3 encoded byte into an int */
-	for (i = 0; i < header.sig_size; i++) {
-		u16 val = len_vals[data_vals[i]];
-		single_len = redrat3_len_to_us((u32)be16_to_cpu(val));
+	sig_size = be16_to_cpu(rr3->irdata.sig_size);
+	for (i = 0; i < sig_size; i++) {
+		offset = rr3->irdata.sigdata[i];
+		val = get_unaligned_be16(&rr3->irdata.lens[offset]);
+		single_len = redrat3_len_to_us(val);
 
 		/* we should always get pulse/space/pulse/space samples */
 		if (i % 2)
@@ -534,7 +452,7 @@ static u8 redrat3_send_cmd(int cmd, struct redrat3_dev *rr3)
 			__func__, res, *data);
 		res = -EIO;
 	} else
-		res = (u8)data[0];
+		res = data[0];
 
 	kfree(data);
 
@@ -704,79 +622,72 @@ static void redrat3_get_firmware_rev(struct redrat3_dev *rr3)
 
 static void redrat3_read_packet_start(struct redrat3_dev *rr3, int len)
 {
-	u16 tx_error;
-	u16 hdrlen;
+	struct redrat3_header *header = rr3->bulk_in_buf;
+	unsigned pktlen, pkttype;
 
 	rr3_ftr(rr3->dev, "Entering %s\n", __func__);
 
 	/* grab the Length and type of transfer */
-	memcpy(&(rr3->pktlen), (unsigned char *) rr3->bulk_in_buf,
-	       sizeof(rr3->pktlen));
-	memcpy(&(rr3->pkttype), ((unsigned char *) rr3->bulk_in_buf +
-		sizeof(rr3->pktlen)),
-	       sizeof(rr3->pkttype));
+	pktlen = be16_to_cpu(header->length);
+	pkttype = be16_to_cpu(header->transfer_type);
 
-	/*data needs conversion to know what its real values are*/
-	rr3->pktlen = be16_to_cpu(rr3->pktlen);
-	rr3->pkttype = be16_to_cpu(rr3->pkttype);
+	if (pktlen > sizeof(rr3->irdata)) {
+		dev_warn(rr3->dev, "packet length %u too large\n", pktlen);
+		return;
+	}
 
-	switch (rr3->pkttype) {
+	switch (pkttype) {
 	case RR3_ERROR:
-		memcpy(&tx_error, ((unsigned char *)rr3->bulk_in_buf
-			+ (sizeof(rr3->pktlen) + sizeof(rr3->pkttype))),
-		       sizeof(tx_error));
-		tx_error = be16_to_cpu(tx_error);
-		redrat3_dump_fw_error(rr3, tx_error);
+		if (len >= sizeof(struct redrat3_error)) {
+			struct redrat3_error *error = rr3->bulk_in_buf;
+			unsigned fw_error = be16_to_cpu(error->fw_error);
+			redrat3_dump_fw_error(rr3, fw_error);
+		}
 		break;
 
 	case RR3_MOD_SIGNAL_IN:
-		hdrlen = sizeof(rr3->pktlen) + sizeof(rr3->pkttype);
+		memcpy(&rr3->irdata, rr3->bulk_in_buf, len);
 		rr3->bytes_read = len;
-		rr3->bytes_read -= hdrlen;
-		rr3->datap = &(rr3->pbuf[0]);
-
-		memcpy(rr3->datap, ((unsigned char *)rr3->bulk_in_buf + hdrlen),
-		       rr3->bytes_read);
-		rr3->datap += rr3->bytes_read;
 		rr3_dbg(rr3->dev, "bytes_read %d, pktlen %d\n",
-			rr3->bytes_read, rr3->pktlen);
+			rr3->bytes_read, pktlen);
 		break;
 
 	default:
-		rr3_dbg(rr3->dev, "ignoring packet with type 0x%02x, "
-			"len of %d, 0x%02x\n", rr3->pkttype, len, rr3->pktlen);
+		rr3_dbg(rr3->dev, "ignoring packet with type 0x%02x, len of %d, 0x%02x\n",
+						pkttype, len, pktlen);
 		break;
 	}
 }
 
 static void redrat3_read_packet_continue(struct redrat3_dev *rr3, int len)
 {
+	void *irdata = &rr3->irdata;
+
 	rr3_ftr(rr3->dev, "Entering %s\n", __func__);
 
-	memcpy(rr3->datap, (unsigned char *)rr3->bulk_in_buf, len);
-	rr3->datap += len;
+	if (len + rr3->bytes_read > sizeof(rr3->irdata)) {
+		dev_warn(rr3->dev, "too much data for packet\n");
+		rr3->bytes_read = 0;
+		return;
+	}
+
+	memcpy(irdata + rr3->bytes_read, rr3->bulk_in_buf, len);
 
 	rr3->bytes_read += len;
-	rr3_dbg(rr3->dev, "bytes_read %d, pktlen %d\n",
-		rr3->bytes_read, rr3->pktlen);
+	rr3_dbg(rr3->dev, "bytes_read %d, pktlen %d\n", rr3->bytes_read,
+				 be16_to_cpu(rr3->irdata.header.length));
 }
 
 /* gather IR data from incoming urb, process it when we have enough */
 static int redrat3_get_ir_data(struct redrat3_dev *rr3, int len)
 {
 	struct device *dev = rr3->dev;
+	unsigned pkttype;
 	int ret = 0;
 
 	rr3_ftr(dev, "Entering %s\n", __func__);
 
-	if (rr3->pktlen > RR3_MAX_BUF_SIZE) {
-		dev_err(rr3->dev, "error: packet larger than buffer\n");
-		ret = -EINVAL;
-		goto out;
-	}
-
-	if ((rr3->bytes_read == 0) &&
-	    (len >= (sizeof(rr3->pkttype) + sizeof(rr3->pktlen)))) {
+	if (rr3->bytes_read == 0 && len >= sizeof(struct redrat3_header)) {
 		redrat3_read_packet_start(rr3, len);
 	} else if (rr3->bytes_read != 0) {
 		redrat3_read_packet_continue(rr3, len);
@@ -786,26 +697,20 @@ static int redrat3_get_ir_data(struct redrat3_dev *rr3, int len)
 		goto out;
 	}
 
-	if (rr3->bytes_read > rr3->pktlen) {
-		dev_err(dev, "bytes_read (%d) greater than pktlen (%d)\n",
-			rr3->bytes_read, rr3->pktlen);
-		ret = -EINVAL;
-		goto out;
-	} else if (rr3->bytes_read < rr3->pktlen)
+	if (rr3->bytes_read < be16_to_cpu(rr3->irdata.header.length))
 		/* we're still accumulating data */
 		return 0;
 
 	/* if we get here, we've got IR data to decode */
-	if (rr3->pkttype == RR3_MOD_SIGNAL_IN)
+	pkttype = be16_to_cpu(rr3->irdata.header.transfer_type);
+	if (pkttype == RR3_MOD_SIGNAL_IN)
 		redrat3_process_ir_data(rr3);
 	else
-		rr3_dbg(dev, "discarding non-signal data packet "
-			"(type 0x%02x)\n", rr3->pkttype);
+		rr3_dbg(dev, "discarding non-signal data packet (type 0x%02x)\n",
+								pkttype);
 
 out:
 	rr3->bytes_read = 0;
-	rr3->pktlen = 0;
-	rr3->pkttype = 0;
 	return ret;
 }
 
@@ -846,8 +751,6 @@ static void redrat3_handle_async(struct urb *urb)
 	default:
 		dev_warn(rr3->dev, "Error: urb status = %d\n", urb->status);
 		rr3->bytes_read = 0;
-		rr3->pktlen = 0;
-		rr3->pkttype = 0;
 		break;
 	}
 }
@@ -873,7 +776,7 @@ static u16 mod_freq_to_val(unsigned int mod_freq)
 	int mult = 6000000;
 
 	/* Clk used in mod. freq. generation is CLK24/4. */
-	return (u16)(65536 - (mult / mod_freq));
+	return 65536 - (mult / mod_freq);
 }
 
 static int redrat3_set_tx_carrier(struct rc_dev *rcdev, u32 carrier)
@@ -895,16 +798,11 @@ static int redrat3_transmit_ir(struct rc_dev *rcdev, unsigned *txbuf,
 {
 	struct redrat3_dev *rr3 = rcdev->priv;
 	struct device *dev = rr3->dev;
-	struct redrat3_signal_header header;
-	int i, ret, ret_len, offset;
+	struct redrat3_irdata *irdata = NULL;
+	int i, ret, ret_len;
 	int lencheck, cur_sample_len, pipe;
-	char *buffer = NULL, *sigdata = NULL;
 	int *sample_lens = NULL;
-	u32 tmpi;
-	u16 tmps;
-	u8 *datap;
 	u8 curlencheck = 0;
-	u16 *lengths_ptr;
 	int sendbuf_len;
 
 	rr3_ftr(dev, "Entering %s\n", __func__);
@@ -926,8 +824,8 @@ static int redrat3_transmit_ir(struct rc_dev *rcdev, unsigned *txbuf,
 		goto out;
 	}
 
-	sigdata = kzalloc((count + RR3_TX_TRAILER_LEN), GFP_KERNEL);
-	if (!sigdata) {
+	irdata = kzalloc(sizeof(*irdata), GFP_KERNEL);
+	if (!irdata) {
 		ret = -ENOMEM;
 		goto out;
 	}
@@ -950,83 +848,41 @@ static int redrat3_transmit_ir(struct rc_dev *rcdev, unsigned *txbuf,
 				/* now convert the value to a proper
 				 * rr3 value.. */
 				sample_lens[curlencheck] = cur_sample_len;
+				put_unaligned_be16(cur_sample_len,
+						&irdata->lens[curlencheck]);
 				curlencheck++;
 			} else {
 				count = i - 1;
 				break;
 			}
 		}
-		sigdata[i] = lencheck;
+		irdata->sigdata[i] = lencheck;
 	}
 
-	sigdata[count] = RR3_END_OF_SIGNAL;
-	sigdata[count + 1] = RR3_END_OF_SIGNAL;
-
-	offset = RR3_TX_HEADER_OFFSET;
-	sendbuf_len = RR3_HEADER_LENGTH + (sizeof(u16) * RR3_DRIVER_MAXLENS)
-			+ count + RR3_TX_TRAILER_LEN + offset;
-
-	buffer = kzalloc(sendbuf_len, GFP_KERNEL);
-	if (!buffer) {
-		ret = -ENOMEM;
-		goto out;
-	}
+	irdata->sigdata[count] = RR3_END_OF_SIGNAL;
+	irdata->sigdata[count + 1] = RR3_END_OF_SIGNAL;
 
+	sendbuf_len = offsetof(struct redrat3_irdata,
+					sigdata[count + RR3_TX_TRAILER_LEN]);
 	/* fill in our packet header */
-	header.length = sendbuf_len - offset;
-	header.transfer_type = RR3_MOD_SIGNAL_OUT;
-	header.pause = redrat3_len_to_us(100);
-	header.mod_freq_count = mod_freq_to_val(rr3->carrier);
-	header.no_periods = 0; /* n/a to transmit */
-	header.max_lengths = RR3_DRIVER_MAXLENS;
-	header.no_lengths = curlencheck;
-	header.max_sig_size = RR3_MAX_SIG_SIZE;
-	header.sig_size = count + RR3_TX_TRAILER_LEN;
-	/* we currently rely on repeat handling in the IR encoding source */
-	header.no_repeats = 0;
-
-	tmps = cpu_to_be16(header.length);
-	memcpy(buffer, &tmps, 2);
-
-	tmps = cpu_to_be16(header.transfer_type);
-	memcpy(buffer + 2, &tmps, 2);
-
-	tmpi = cpu_to_be32(header.pause);
-	memcpy(buffer + offset, &tmpi, sizeof(tmpi));
-
-	tmps = cpu_to_be16(header.mod_freq_count);
-	memcpy(buffer + offset + RR3_FREQ_COUNT_OFFSET, &tmps, 2);
-
-	buffer[offset + RR3_NUM_LENGTHS_OFFSET] = header.no_lengths;
-
-	tmps = cpu_to_be16(header.sig_size);
-	memcpy(buffer + offset + RR3_NUM_SIGS_OFFSET, &tmps, 2);
-
-	buffer[offset + RR3_REPEATS_OFFSET] = header.no_repeats;
-
-	lengths_ptr = (u16 *)(buffer + offset + RR3_HEADER_LENGTH);
-	for (i = 0; i < curlencheck; ++i)
-		lengths_ptr[i] = cpu_to_be16(sample_lens[i]);
-
-	datap = (u8 *)(buffer + offset + RR3_HEADER_LENGTH +
-			    (sizeof(u16) * RR3_DRIVER_MAXLENS));
-	memcpy(datap, sigdata, (count + RR3_TX_TRAILER_LEN));
-
-	if (debug) {
-		redrat3_dump_signal_header(&header);
-		redrat3_dump_signal_data(buffer, header.sig_size);
-	}
+	irdata->header.length = cpu_to_be16(sendbuf_len -
+						sizeof(struct redrat3_header));
+	irdata->header.transfer_type = cpu_to_be16(RR3_MOD_SIGNAL_OUT);
+	irdata->pause = cpu_to_be32(redrat3_len_to_us(100));
+	irdata->mod_freq_count = cpu_to_be16(mod_freq_to_val(rr3->carrier));
+	irdata->no_lengths = curlencheck;
+	irdata->sig_size = cpu_to_be16(count + RR3_TX_TRAILER_LEN);
 
 	pipe = usb_sndbulkpipe(rr3->udev, rr3->ep_out->bEndpointAddress);
-	tmps = usb_bulk_msg(rr3->udev, pipe, buffer,
+	ret = usb_bulk_msg(rr3->udev, pipe, irdata,
 			    sendbuf_len, &ret_len, 10 * HZ);
-	rr3_dbg(dev, "sent %d bytes, (ret %d)\n", ret_len, tmps);
+	rr3_dbg(dev, "sent %d bytes, (ret %d)\n", ret_len, ret);
 
 	/* now tell the hardware to transmit what we sent it */
 	pipe = usb_rcvctrlpipe(rr3->udev, 0);
 	ret = usb_control_msg(rr3->udev, pipe, RR3_TX_SEND_SIGNAL,
 			      USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN,
-			      0, 0, buffer, 2, HZ * 10);
+			      0, 0, irdata, 2, HZ * 10);
 
 	if (ret < 0)
 		dev_err(dev, "Error: control msg send failed, rc %d\n", ret);
@@ -1035,8 +891,7 @@ static int redrat3_transmit_ir(struct rc_dev *rcdev, unsigned *txbuf,
 
 out:
 	kfree(sample_lens);
-	kfree(buffer);
-	kfree(sigdata);
+	kfree(irdata);
 
 	rr3->transmitting = false;
 	/* rr3 re-enables rc detector because it was enabled before */
-- 
1.7.2.5

Re: [PATCH 2/3] [media] redrat3: remove memcpys and fix unaligned memory access

[David Härdeman]

On Sat, Feb 16, 2013 at 09:25:44PM +0000, Sean Young wrote:
>In stead of doing a memcpy from #defined offset, declare structs which
>describe the incoming and outgoing data accurately.
>
>Tested on first generation RedRat.

Oh, so you have that hardware....that's great.

I greatly appreciate that this patch removes the bizarre
redrat3_dump_signal_* functions....I can't imagine that they ever
worked correctly.

>Signed-off-by: Sean Young <sean@mess.org>
>---
> drivers/media/rc/redrat3.c |  351 +++++++++++++-------------------------------
> 1 files changed, 103 insertions(+), 248 deletions(-)
>
>diff --git a/drivers/media/rc/redrat3.c b/drivers/media/rc/redrat3.c
>index 842bdcd..ec655b8 100644
>--- a/drivers/media/rc/redrat3.c
>+++ b/drivers/media/rc/redrat3.c
>@@ -45,6 +45,7 @@
>  *
>  */
> 
>+#include <asm/unaligned.h>
> #include <linux/device.h>
> #include <linux/module.h>
> #include <linux/slab.h>
>@@ -129,25 +130,11 @@ static int debug;
> /* USB bulk-in IR data endpoint address */
> #define RR3_BULK_IN_EP_ADDR	0x82
> 
>-/* Raw Modulated signal data value offsets */
>-#define RR3_PAUSE_OFFSET	0
>-#define RR3_FREQ_COUNT_OFFSET	4
>-#define RR3_NUM_PERIOD_OFFSET	6
>-#define RR3_MAX_LENGTHS_OFFSET	8
>-#define RR3_NUM_LENGTHS_OFFSET	9
>-#define RR3_MAX_SIGS_OFFSET	10
>-#define RR3_NUM_SIGS_OFFSET	12
>-#define RR3_REPEATS_OFFSET	14
>-
> /* Size of the fixed-length portion of the signal */
>-#define RR3_HEADER_LENGTH	15
> #define RR3_DRIVER_MAXLENS	128
> #define RR3_MAX_SIG_SIZE	512
>-#define RR3_MAX_BUF_SIZE	\
>-	((2 * RR3_HEADER_LENGTH) + RR3_DRIVER_MAXLENS + RR3_MAX_SIG_SIZE)
> #define RR3_TIME_UNIT		50
> #define RR3_END_OF_SIGNAL	0x7f
>-#define RR3_TX_HEADER_OFFSET	4
> #define RR3_TX_TRAILER_LEN	2
> #define RR3_RX_MIN_TIMEOUT	5
> #define RR3_RX_MAX_TIMEOUT	2000
>@@ -159,6 +146,32 @@ static int debug;
> #define USB_RR3USB_PRODUCT_ID	0x0001
> #define USB_RR3IIUSB_PRODUCT_ID	0x0005
> 
>+struct redrat3_header {
>+	__be16 length;
>+	__be16 transfer_type;
>+} __packed;
>+
>+/* sending and receiving irdata */
>+struct redrat3_irdata {
>+	struct redrat3_header header;
>+	__be32 pause;
>+	__be16 mod_freq_count;
>+	__be16 num_periods;
>+	__u8 max_lengths;
>+	__u8 no_lengths;
>+	__be16 max_sig_size;
>+	__be16 sig_size;
>+	__u8 no_repeats;
>+	__be16 lens[RR3_DRIVER_MAXLENS]; /* not aligned */
>+	__u8 sigdata[RR3_MAX_SIG_SIZE];
>+} __packed;
>+
>+/* firmware errors */
>+struct redrat3_error {
>+	struct redrat3_header header;
>+	__be16 fw_error;
>+} __packed;
>+
> /* table of devices that work with this driver */
> static struct usb_device_id redrat3_dev_table[] = {
> 	/* Original version of the RedRat3 */
>@@ -180,7 +193,7 @@ struct redrat3_dev {
> 	/* the receive endpoint */
> 	struct usb_endpoint_descriptor *ep_in;
> 	/* the buffer to receive data */
>-	unsigned char *bulk_in_buf;
>+	void *bulk_in_buf;
> 	/* urb used to read ir data */
> 	struct urb *read_urb;
> 
>@@ -205,69 +218,15 @@ struct redrat3_dev {
> 	bool transmitting;
> 
> 	/* store for current packet */
>-	char pbuf[RR3_MAX_BUF_SIZE];
>-	u16 pktlen;
>-	u16 pkttype;
>+	struct redrat3_irdata irdata;
> 	u16 bytes_read;
>-	char *datap;
> 
> 	u32 carrier;
> 
>-	char name[128];
>+	char name[64];
> 	char phys[64];
> };
> 
>-/* All incoming data buffers adhere to a very specific data format */
>-struct redrat3_signal_header {
>-	u16 length;	/* Length of data being transferred */
>-	u16 transfer_type; /* Type of data transferred */
>-	u32 pause;	/* Pause between main and repeat signals */
>-	u16 mod_freq_count; /* Value of timer on mod. freq. measurement */
>-	u16 no_periods;	/* No. of periods over which mod. freq. is measured */
>-	u8 max_lengths;	/* Max no. of lengths (i.e. size of array) */
>-	u8 no_lengths;	/* Actual no. of elements in lengths array */
>-	u16 max_sig_size; /* Max no. of values in signal data array */
>-	u16 sig_size;	/* Acuto no. of values in signal data array */
>-	u8 no_repeats;	/* No. of repeats of repeat signal section */
>-	/* Here forward is the lengths and signal data */
>-};
>-
>-static void redrat3_dump_signal_header(struct redrat3_signal_header *header)
>-{
>-	pr_info("%s:\n", __func__);
>-	pr_info(" * length: %u, transfer_type: 0x%02x\n",
>-		header->length, header->transfer_type);
>-	pr_info(" * pause: %u, freq_count: %u, no_periods: %u\n",
>-		header->pause, header->mod_freq_count, header->no_periods);
>-	pr_info(" * lengths: %u (max: %u)\n",
>-		header->no_lengths, header->max_lengths);
>-	pr_info(" * sig_size: %u (max: %u)\n",
>-		header->sig_size, header->max_sig_size);
>-	pr_info(" * repeats: %u\n", header->no_repeats);
>-}
>-
>-static void redrat3_dump_signal_data(char *buffer, u16 len)
>-{
>-	int offset, i;
>-	char *data_vals;
>-
>-	pr_info("%s:", __func__);
>-
>-	offset = RR3_TX_HEADER_OFFSET + RR3_HEADER_LENGTH
>-		 + (RR3_DRIVER_MAXLENS * sizeof(u16));
>-
>-	/* read RR3_DRIVER_MAXLENS from ctrl msg */
>-	data_vals = buffer + offset;
>-
>-	for (i = 0; i < len; i++) {
>-		if (i % 10 == 0)
>-			pr_cont("\n * ");
>-		pr_cont("%02x ", *data_vals++);
>-	}
>-
>-	pr_cont("\n");
>-}
>-
> /*
>  * redrat3_issue_async
>  *
>@@ -349,13 +308,14 @@ static void redrat3_dump_fw_error(struct redrat3_dev *rr3, int code)
> 	}
> }
> 
>-static u32 redrat3_val_to_mod_freq(struct redrat3_signal_header *ph)
>+static u32 redrat3_val_to_mod_freq(struct redrat3_irdata *irdata)
> {
> 	u32 mod_freq = 0;
>+	u16 mod_freq_count = be16_to_cpu(irdata->mod_freq_count);
> 
>-	if (ph->mod_freq_count != 0)
>-		mod_freq = (RR3_CLK * ph->no_periods) /
>-				(ph->mod_freq_count * RR3_CLK_PER_COUNT);
>+	if (mod_freq_count != 0)
>+		mod_freq = (RR3_CLK * be16_to_cpu(irdata->num_periods)) /
>+			(mod_freq_count * RR3_CLK_PER_COUNT);
> 
> 	return mod_freq;
> }
>@@ -407,16 +367,11 @@ static void redrat3_rx_timeout(unsigned long data)
> static void redrat3_process_ir_data(struct redrat3_dev *rr3)
> {
> 	DEFINE_IR_RAW_EVENT(rawir);
>-	struct redrat3_signal_header header;
> 	struct device *dev;
> 	int i, trailer = 0;
>+	unsigned sig_size, single_len, offset, val;
> 	unsigned long delay;
>-	u32 mod_freq, single_len;
>-	u16 *len_vals;
>-	u8 *data_vals;
>-	u32 tmp32;
>-	u16 tmp16;
>-	char *sig_data;
>+	u32 mod_freq;
> 
> 	if (!rr3) {
> 		pr_err("%s called with no context!\n", __func__);
>@@ -426,57 +381,20 @@ static void redrat3_process_ir_data(struct redrat3_dev *rr3)
> 	rr3_ftr(rr3->dev, "Entered %s\n", __func__);
> 
> 	dev = rr3->dev;
>-	sig_data = rr3->pbuf;
>-
>-	header.length = rr3->pktlen;
>-	header.transfer_type = rr3->pkttype;
>-
>-	/* Sanity check */
>-	if (!(header.length >= RR3_HEADER_LENGTH))
>-		dev_warn(dev, "read returned less than rr3 header len\n");
> 
> 	/* Make sure we reset the IR kfifo after a bit of inactivity */
> 	delay = usecs_to_jiffies(rr3->hw_timeout);
> 	mod_timer(&rr3->rx_timeout, jiffies + delay);
> 
>-	memcpy(&tmp32, sig_data + RR3_PAUSE_OFFSET, sizeof(tmp32));
>-	header.pause = be32_to_cpu(tmp32);
>-
>-	memcpy(&tmp16, sig_data + RR3_FREQ_COUNT_OFFSET, sizeof(tmp16));
>-	header.mod_freq_count = be16_to_cpu(tmp16);
>-
>-	memcpy(&tmp16, sig_data + RR3_NUM_PERIOD_OFFSET, sizeof(tmp16));
>-	header.no_periods = be16_to_cpu(tmp16);
>-
>-	header.max_lengths = sig_data[RR3_MAX_LENGTHS_OFFSET];
>-	header.no_lengths = sig_data[RR3_NUM_LENGTHS_OFFSET];
>-
>-	memcpy(&tmp16, sig_data + RR3_MAX_SIGS_OFFSET, sizeof(tmp16));
>-	header.max_sig_size = be16_to_cpu(tmp16);
>-
>-	memcpy(&tmp16, sig_data + RR3_NUM_SIGS_OFFSET, sizeof(tmp16));
>-	header.sig_size = be16_to_cpu(tmp16);
>-
>-	header.no_repeats= sig_data[RR3_REPEATS_OFFSET];
>-
>-	if (debug) {
>-		redrat3_dump_signal_header(&header);
>-		redrat3_dump_signal_data(sig_data, header.sig_size);
>-	}
>-
>-	mod_freq = redrat3_val_to_mod_freq(&header);
>+	mod_freq = redrat3_val_to_mod_freq(&rr3->irdata);
> 	rr3_dbg(dev, "Got mod_freq of %u\n", mod_freq);
> 
>-	/* Here we pull out the 'length' values from the signal */
>-	len_vals = (u16 *)(sig_data + RR3_HEADER_LENGTH);
>-
>-	data_vals = sig_data + RR3_HEADER_LENGTH +
>-		    (header.max_lengths * sizeof(u16));
>-
> 	/* process each rr3 encoded byte into an int */
>-	for (i = 0; i < header.sig_size; i++) {
>-		u16 val = len_vals[data_vals[i]];
>-		single_len = redrat3_len_to_us((u32)be16_to_cpu(val));
>+	sig_size = be16_to_cpu(rr3->irdata.sig_size);
>+	for (i = 0; i < sig_size; i++) {
>+		offset = rr3->irdata.sigdata[i];
>+		val = get_unaligned_be16(&rr3->irdata.lens[offset]);
>+		single_len = redrat3_len_to_us(val);
> 
> 		/* we should always get pulse/space/pulse/space samples */
> 		if (i % 2)
>@@ -534,7 +452,7 @@ static u8 redrat3_send_cmd(int cmd, struct redrat3_dev *rr3)
> 			__func__, res, *data);
> 		res = -EIO;
> 	} else
>-		res = (u8)data[0];
>+		res = data[0];
> 
> 	kfree(data);
> 
>@@ -704,79 +622,72 @@ static void redrat3_get_firmware_rev(struct redrat3_dev *rr3)
> 
> static void redrat3_read_packet_start(struct redrat3_dev *rr3, int len)
> {
>-	u16 tx_error;
>-	u16 hdrlen;
>+	struct redrat3_header *header = rr3->bulk_in_buf;
>+	unsigned pktlen, pkttype;
> 
> 	rr3_ftr(rr3->dev, "Entering %s\n", __func__);
> 
> 	/* grab the Length and type of transfer */
>-	memcpy(&(rr3->pktlen), (unsigned char *) rr3->bulk_in_buf,
>-	       sizeof(rr3->pktlen));
>-	memcpy(&(rr3->pkttype), ((unsigned char *) rr3->bulk_in_buf +
>-		sizeof(rr3->pktlen)),
>-	       sizeof(rr3->pkttype));
>+	pktlen = be16_to_cpu(header->length);
>+	pkttype = be16_to_cpu(header->transfer_type);
> 
>-	/*data needs conversion to know what its real values are*/
>-	rr3->pktlen = be16_to_cpu(rr3->pktlen);
>-	rr3->pkttype = be16_to_cpu(rr3->pkttype);
>+	if (pktlen > sizeof(rr3->irdata)) {
>+		dev_warn(rr3->dev, "packet length %u too large\n", pktlen);
>+		return;
>+	}
> 
>-	switch (rr3->pkttype) {
>+	switch (pkttype) {
> 	case RR3_ERROR:
>-		memcpy(&tx_error, ((unsigned char *)rr3->bulk_in_buf
>-			+ (sizeof(rr3->pktlen) + sizeof(rr3->pkttype))),
>-		       sizeof(tx_error));
>-		tx_error = be16_to_cpu(tx_error);
>-		redrat3_dump_fw_error(rr3, tx_error);
>+		if (len >= sizeof(struct redrat3_error)) {
>+			struct redrat3_error *error = rr3->bulk_in_buf;
>+			unsigned fw_error = be16_to_cpu(error->fw_error);
>+			redrat3_dump_fw_error(rr3, fw_error);
>+		}
> 		break;
> 
> 	case RR3_MOD_SIGNAL_IN:
>-		hdrlen = sizeof(rr3->pktlen) + sizeof(rr3->pkttype);
>+		memcpy(&rr3->irdata, rr3->bulk_in_buf, len);
> 		rr3->bytes_read = len;
>-		rr3->bytes_read -= hdrlen;
>-		rr3->datap = &(rr3->pbuf[0]);
>-
>-		memcpy(rr3->datap, ((unsigned char *)rr3->bulk_in_buf + hdrlen),
>-		       rr3->bytes_read);
>-		rr3->datap += rr3->bytes_read;
> 		rr3_dbg(rr3->dev, "bytes_read %d, pktlen %d\n",
>-			rr3->bytes_read, rr3->pktlen);
>+			rr3->bytes_read, pktlen);
> 		break;
> 
> 	default:
>-		rr3_dbg(rr3->dev, "ignoring packet with type 0x%02x, "
>-			"len of %d, 0x%02x\n", rr3->pkttype, len, rr3->pktlen);
>+		rr3_dbg(rr3->dev, "ignoring packet with type 0x%02x, len of %d, 0x%02x\n",
>+						pkttype, len, pktlen);
> 		break;
> 	}
> }
> 
> static void redrat3_read_packet_continue(struct redrat3_dev *rr3, int len)
> {
>+	void *irdata = &rr3->irdata;
>+
> 	rr3_ftr(rr3->dev, "Entering %s\n", __func__);
> 
>-	memcpy(rr3->datap, (unsigned char *)rr3->bulk_in_buf, len);
>-	rr3->datap += len;
>+	if (len + rr3->bytes_read > sizeof(rr3->irdata)) {
>+		dev_warn(rr3->dev, "too much data for packet\n");
>+		rr3->bytes_read = 0;
>+		return;
>+	}
>+
>+	memcpy(irdata + rr3->bytes_read, rr3->bulk_in_buf, len);
> 
> 	rr3->bytes_read += len;
>-	rr3_dbg(rr3->dev, "bytes_read %d, pktlen %d\n",
>-		rr3->bytes_read, rr3->pktlen);
>+	rr3_dbg(rr3->dev, "bytes_read %d, pktlen %d\n", rr3->bytes_read,
>+				 be16_to_cpu(rr3->irdata.header.length));
> }
> 
> /* gather IR data from incoming urb, process it when we have enough */
> static int redrat3_get_ir_data(struct redrat3_dev *rr3, int len)
> {
> 	struct device *dev = rr3->dev;
>+	unsigned pkttype;
> 	int ret = 0;
> 
> 	rr3_ftr(dev, "Entering %s\n", __func__);
> 
>-	if (rr3->pktlen > RR3_MAX_BUF_SIZE) {
>-		dev_err(rr3->dev, "error: packet larger than buffer\n");
>-		ret = -EINVAL;
>-		goto out;
>-	}
>-
>-	if ((rr3->bytes_read == 0) &&
>-	    (len >= (sizeof(rr3->pkttype) + sizeof(rr3->pktlen)))) {
>+	if (rr3->bytes_read == 0 && len >= sizeof(struct redrat3_header)) {
> 		redrat3_read_packet_start(rr3, len);
> 	} else if (rr3->bytes_read != 0) {
> 		redrat3_read_packet_continue(rr3, len);
>@@ -786,26 +697,20 @@ static int redrat3_get_ir_data(struct redrat3_dev *rr3, int len)
> 		goto out;
> 	}
> 
>-	if (rr3->bytes_read > rr3->pktlen) {
>-		dev_err(dev, "bytes_read (%d) greater than pktlen (%d)\n",
>-			rr3->bytes_read, rr3->pktlen);
>-		ret = -EINVAL;
>-		goto out;
>-	} else if (rr3->bytes_read < rr3->pktlen)
>+	if (rr3->bytes_read < be16_to_cpu(rr3->irdata.header.length))
> 		/* we're still accumulating data */
> 		return 0;
> 
> 	/* if we get here, we've got IR data to decode */
>-	if (rr3->pkttype == RR3_MOD_SIGNAL_IN)
>+	pkttype = be16_to_cpu(rr3->irdata.header.transfer_type);
>+	if (pkttype == RR3_MOD_SIGNAL_IN)
> 		redrat3_process_ir_data(rr3);
> 	else
>-		rr3_dbg(dev, "discarding non-signal data packet "
>-			"(type 0x%02x)\n", rr3->pkttype);
>+		rr3_dbg(dev, "discarding non-signal data packet (type 0x%02x)\n",
>+								pkttype);
> 
> out:
> 	rr3->bytes_read = 0;
>-	rr3->pktlen = 0;
>-	rr3->pkttype = 0;
> 	return ret;
> }
> 
>@@ -846,8 +751,6 @@ static void redrat3_handle_async(struct urb *urb)
> 	default:
> 		dev_warn(rr3->dev, "Error: urb status = %d\n", urb->status);
> 		rr3->bytes_read = 0;
>-		rr3->pktlen = 0;
>-		rr3->pkttype = 0;
> 		break;
> 	}
> }
>@@ -873,7 +776,7 @@ static u16 mod_freq_to_val(unsigned int mod_freq)
> 	int mult = 6000000;
> 
> 	/* Clk used in mod. freq. generation is CLK24/4. */
>-	return (u16)(65536 - (mult / mod_freq));
>+	return 65536 - (mult / mod_freq);
> }
> 
> static int redrat3_set_tx_carrier(struct rc_dev *rcdev, u32 carrier)
>@@ -895,16 +798,11 @@ static int redrat3_transmit_ir(struct rc_dev *rcdev, unsigned *txbuf,
> {
> 	struct redrat3_dev *rr3 = rcdev->priv;
> 	struct device *dev = rr3->dev;
>-	struct redrat3_signal_header header;
>-	int i, ret, ret_len, offset;
>+	struct redrat3_irdata *irdata = NULL;
>+	int i, ret, ret_len;
> 	int lencheck, cur_sample_len, pipe;
>-	char *buffer = NULL, *sigdata = NULL;
> 	int *sample_lens = NULL;
>-	u32 tmpi;
>-	u16 tmps;
>-	u8 *datap;
> 	u8 curlencheck = 0;
>-	u16 *lengths_ptr;
> 	int sendbuf_len;
> 
> 	rr3_ftr(dev, "Entering %s\n", __func__);
>@@ -926,8 +824,8 @@ static int redrat3_transmit_ir(struct rc_dev *rcdev, unsigned *txbuf,
> 		goto out;
> 	}
> 
>-	sigdata = kzalloc((count + RR3_TX_TRAILER_LEN), GFP_KERNEL);
>-	if (!sigdata) {
>+	irdata = kzalloc(sizeof(*irdata), GFP_KERNEL);
>+	if (!irdata) {
> 		ret = -ENOMEM;
> 		goto out;
> 	}
>@@ -950,83 +848,41 @@ static int redrat3_transmit_ir(struct rc_dev *rcdev, unsigned *txbuf,
> 				/* now convert the value to a proper
> 				 * rr3 value.. */
> 				sample_lens[curlencheck] = cur_sample_len;
>+				put_unaligned_be16(cur_sample_len,
>+						&irdata->lens[curlencheck]);
> 				curlencheck++;
> 			} else {
> 				count = i - 1;
> 				break;
> 			}
> 		}
>-		sigdata[i] = lencheck;
>+		irdata->sigdata[i] = lencheck;
> 	}
> 
>-	sigdata[count] = RR3_END_OF_SIGNAL;
>-	sigdata[count + 1] = RR3_END_OF_SIGNAL;
>-
>-	offset = RR3_TX_HEADER_OFFSET;
>-	sendbuf_len = RR3_HEADER_LENGTH + (sizeof(u16) * RR3_DRIVER_MAXLENS)
>-			+ count + RR3_TX_TRAILER_LEN + offset;
>-
>-	buffer = kzalloc(sendbuf_len, GFP_KERNEL);
>-	if (!buffer) {
>-		ret = -ENOMEM;
>-		goto out;
>-	}
>+	irdata->sigdata[count] = RR3_END_OF_SIGNAL;
>+	irdata->sigdata[count + 1] = RR3_END_OF_SIGNAL;
> 
>+	sendbuf_len = offsetof(struct redrat3_irdata,
>+					sigdata[count + RR3_TX_TRAILER_LEN]);
> 	/* fill in our packet header */
>-	header.length = sendbuf_len - offset;
>-	header.transfer_type = RR3_MOD_SIGNAL_OUT;
>-	header.pause = redrat3_len_to_us(100);
>-	header.mod_freq_count = mod_freq_to_val(rr3->carrier);
>-	header.no_periods = 0; /* n/a to transmit */
>-	header.max_lengths = RR3_DRIVER_MAXLENS;
>-	header.no_lengths = curlencheck;
>-	header.max_sig_size = RR3_MAX_SIG_SIZE;
>-	header.sig_size = count + RR3_TX_TRAILER_LEN;
>-	/* we currently rely on repeat handling in the IR encoding source */
>-	header.no_repeats = 0;
>-
>-	tmps = cpu_to_be16(header.length);
>-	memcpy(buffer, &tmps, 2);
>-
>-	tmps = cpu_to_be16(header.transfer_type);
>-	memcpy(buffer + 2, &tmps, 2);
>-
>-	tmpi = cpu_to_be32(header.pause);
>-	memcpy(buffer + offset, &tmpi, sizeof(tmpi));
>-
>-	tmps = cpu_to_be16(header.mod_freq_count);
>-	memcpy(buffer + offset + RR3_FREQ_COUNT_OFFSET, &tmps, 2);
>-
>-	buffer[offset + RR3_NUM_LENGTHS_OFFSET] = header.no_lengths;
>-
>-	tmps = cpu_to_be16(header.sig_size);
>-	memcpy(buffer + offset + RR3_NUM_SIGS_OFFSET, &tmps, 2);
>-
>-	buffer[offset + RR3_REPEATS_OFFSET] = header.no_repeats;
>-
>-	lengths_ptr = (u16 *)(buffer + offset + RR3_HEADER_LENGTH);
>-	for (i = 0; i < curlencheck; ++i)
>-		lengths_ptr[i] = cpu_to_be16(sample_lens[i]);
>-
>-	datap = (u8 *)(buffer + offset + RR3_HEADER_LENGTH +
>-			    (sizeof(u16) * RR3_DRIVER_MAXLENS));
>-	memcpy(datap, sigdata, (count + RR3_TX_TRAILER_LEN));
>-
>-	if (debug) {
>-		redrat3_dump_signal_header(&header);
>-		redrat3_dump_signal_data(buffer, header.sig_size);
>-	}
>+	irdata->header.length = cpu_to_be16(sendbuf_len -
>+						sizeof(struct redrat3_header));
>+	irdata->header.transfer_type = cpu_to_be16(RR3_MOD_SIGNAL_OUT);
>+	irdata->pause = cpu_to_be32(redrat3_len_to_us(100));
>+	irdata->mod_freq_count = cpu_to_be16(mod_freq_to_val(rr3->carrier));
>+	irdata->no_lengths = curlencheck;
>+	irdata->sig_size = cpu_to_be16(count + RR3_TX_TRAILER_LEN);
> 
> 	pipe = usb_sndbulkpipe(rr3->udev, rr3->ep_out->bEndpointAddress);
>-	tmps = usb_bulk_msg(rr3->udev, pipe, buffer,
>+	ret = usb_bulk_msg(rr3->udev, pipe, irdata,
> 			    sendbuf_len, &ret_len, 10 * HZ);
>-	rr3_dbg(dev, "sent %d bytes, (ret %d)\n", ret_len, tmps);
>+	rr3_dbg(dev, "sent %d bytes, (ret %d)\n", ret_len, ret);
> 
> 	/* now tell the hardware to transmit what we sent it */
> 	pipe = usb_rcvctrlpipe(rr3->udev, 0);
> 	ret = usb_control_msg(rr3->udev, pipe, RR3_TX_SEND_SIGNAL,
> 			      USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN,
>-			      0, 0, buffer, 2, HZ * 10);
>+			      0, 0, irdata, 2, HZ * 10);
> 
> 	if (ret < 0)
> 		dev_err(dev, "Error: control msg send failed, rc %d\n", ret);
>@@ -1035,8 +891,7 @@ static int redrat3_transmit_ir(struct rc_dev *rcdev, unsigned *txbuf,
> 
> out:
> 	kfree(sample_lens);
>-	kfree(buffer);
>-	kfree(sigdata);
>+	kfree(irdata);
> 
> 	rr3->transmitting = false;
> 	/* rr3 re-enables rc detector because it was enabled before */
>-- 
>1.7.2.5
>

-- 
David Härdeman

[PATCH 3/3] [media] redrat3: missing endian conversions and warnings

[Sean Young]

Spotted by sparse.

Signed-off-by: Sean Young <sean@mess.org>
---
 drivers/media/rc/redrat3.c |   71 +++++++------------------------------------
 1 files changed, 12 insertions(+), 59 deletions(-)

diff --git a/drivers/media/rc/redrat3.c b/drivers/media/rc/redrat3.c
index ec655b8..12167a6 100644
--- a/drivers/media/rc/redrat3.c
+++ b/drivers/media/rc/redrat3.c
@@ -54,7 +54,6 @@
 #include <media/rc-core.h>
 
 /* Driver Information */
-#define DRIVER_VERSION "0.70"
 #define DRIVER_AUTHOR "Jarod Wilson <jarod@redhat.com>"
 #define DRIVER_AUTHOR2 "The Dweller, Stephen Cox"
 #define DRIVER_DESC "RedRat3 USB IR Transceiver Driver"
@@ -199,14 +198,9 @@ struct redrat3_dev {
 
 	/* the send endpoint */
 	struct usb_endpoint_descriptor *ep_out;
-	/* the buffer to send data */
-	unsigned char *bulk_out_buf;
-	/* the urb used to send data */
-	struct urb *write_urb;
 
 	/* usb dma */
 	dma_addr_t dma_in;
-	dma_addr_t dma_out;
 
 	/* rx signal timeout timer */
 	struct timer_list rx_timeout;
@@ -239,7 +233,6 @@ static void redrat3_issue_async(struct redrat3_dev *rr3)
 
 	rr3_ftr(rr3->dev, "Entering %s\n", __func__);
 
-	memset(rr3->bulk_in_buf, 0, rr3->ep_in->wMaxPacketSize);
 	res = usb_submit_urb(rr3->read_urb, GFP_ATOMIC);
 	if (res)
 		rr3_dbg(rr3->dev, "%s: receive request FAILED! "
@@ -368,7 +361,7 @@ static void redrat3_process_ir_data(struct redrat3_dev *rr3)
 {
 	DEFINE_IR_RAW_EVENT(rawir);
 	struct device *dev;
-	int i, trailer = 0;
+	unsigned i, trailer = 0;
 	unsigned sig_size, single_len, offset, val;
 	unsigned long delay;
 	u32 mod_freq;
@@ -510,15 +503,11 @@ static inline void redrat3_delete(struct redrat3_dev *rr3,
 {
 	rr3_ftr(rr3->dev, "%s cleaning up\n", __func__);
 	usb_kill_urb(rr3->read_urb);
-	usb_kill_urb(rr3->write_urb);
 
 	usb_free_urb(rr3->read_urb);
-	usb_free_urb(rr3->write_urb);
 
-	usb_free_coherent(udev, rr3->ep_in->wMaxPacketSize,
+	usb_free_coherent(udev, le16_to_cpu(rr3->ep_in->wMaxPacketSize),
 			  rr3->bulk_in_buf, rr3->dma_in);
-	usb_free_coherent(udev, rr3->ep_out->wMaxPacketSize,
-			  rr3->bulk_out_buf, rr3->dma_out);
 
 	kfree(rr3);
 }
@@ -566,7 +555,7 @@ static void redrat3_reset(struct redrat3_dev *rr3)
 	rxpipe = usb_rcvctrlpipe(udev, 0);
 	txpipe = usb_sndctrlpipe(udev, 0);
 
-	val = kzalloc(len, GFP_KERNEL);
+	val = kmalloc(len, GFP_KERNEL);
 	if (!val) {
 		dev_err(dev, "Memory allocation failure\n");
 		return;
@@ -620,7 +609,7 @@ static void redrat3_get_firmware_rev(struct redrat3_dev *rr3)
 	rr3_ftr(rr3->dev, "Exiting %s\n", __func__);
 }
 
-static void redrat3_read_packet_start(struct redrat3_dev *rr3, int len)
+static void redrat3_read_packet_start(struct redrat3_dev *rr3, unsigned len)
 {
 	struct redrat3_header *header = rr3->bulk_in_buf;
 	unsigned pktlen, pkttype;
@@ -659,7 +648,7 @@ static void redrat3_read_packet_start(struct redrat3_dev *rr3, int len)
 	}
 }
 
-static void redrat3_read_packet_continue(struct redrat3_dev *rr3, int len)
+static void redrat3_read_packet_continue(struct redrat3_dev *rr3, unsigned len)
 {
 	void *irdata = &rr3->irdata;
 
@@ -679,7 +668,7 @@ static void redrat3_read_packet_continue(struct redrat3_dev *rr3, int len)
 }
 
 /* gather IR data from incoming urb, process it when we have enough */
-static int redrat3_get_ir_data(struct redrat3_dev *rr3, int len)
+static int redrat3_get_ir_data(struct redrat3_dev *rr3, unsigned len)
 {
 	struct device *dev = rr3->dev;
 	unsigned pkttype;
@@ -755,22 +744,6 @@ static void redrat3_handle_async(struct urb *urb)
 	}
 }
 
-static void redrat3_write_bulk_callback(struct urb *urb)
-{
-	struct redrat3_dev *rr3;
-	int len;
-
-	if (!urb)
-		return;
-
-	rr3 = urb->context;
-	if (rr3) {
-		len = urb->actual_length;
-		rr3_ftr(rr3->dev, "%s: called (status=%d len=%d)\n",
-			__func__, urb->status, len);
-	}
-}
-
 static u16 mod_freq_to_val(unsigned int mod_freq)
 {
 	int mult = 6000000;
@@ -799,11 +772,11 @@ static int redrat3_transmit_ir(struct rc_dev *rcdev, unsigned *txbuf,
 	struct redrat3_dev *rr3 = rcdev->priv;
 	struct device *dev = rr3->dev;
 	struct redrat3_irdata *irdata = NULL;
-	int i, ret, ret_len;
+	int ret, ret_len;
 	int lencheck, cur_sample_len, pipe;
 	int *sample_lens = NULL;
 	u8 curlencheck = 0;
-	int sendbuf_len;
+	unsigned i, sendbuf_len;
 
 	rr3_ftr(dev, "Entering %s\n", __func__);
 
@@ -1015,38 +988,18 @@ static int redrat3_dev_probe(struct usb_interface *intf,
 	}
 
 	rr3->ep_in = ep_in;
-	rr3->bulk_in_buf = usb_alloc_coherent(udev, ep_in->wMaxPacketSize,
-					      GFP_ATOMIC, &rr3->dma_in);
+	rr3->bulk_in_buf = usb_alloc_coherent(udev,
+		le16_to_cpu(ep_in->wMaxPacketSize), GFP_ATOMIC, &rr3->dma_in);
 	if (!rr3->bulk_in_buf) {
 		dev_err(dev, "Read buffer allocation failure\n");
 		goto error;
 	}
 
 	pipe = usb_rcvbulkpipe(udev, ep_in->bEndpointAddress);
-	usb_fill_bulk_urb(rr3->read_urb, udev, pipe,
-			  rr3->bulk_in_buf, ep_in->wMaxPacketSize,
-			  redrat3_handle_async, rr3);
-
-	/* set up bulk-out endpoint*/
-	rr3->write_urb = usb_alloc_urb(0, GFP_KERNEL);
-	if (!rr3->write_urb) {
-		dev_err(dev, "Write urb allocation failure\n");
-		goto error;
-	}
+	usb_fill_bulk_urb(rr3->read_urb, udev, pipe, rr3->bulk_in_buf,
+		le16_to_cpu(ep_in->wMaxPacketSize), redrat3_handle_async, rr3);
 
 	rr3->ep_out = ep_out;
-	rr3->bulk_out_buf = usb_alloc_coherent(udev, ep_out->wMaxPacketSize,
-					       GFP_ATOMIC, &rr3->dma_out);
-	if (!rr3->bulk_out_buf) {
-		dev_err(dev, "Write buffer allocation failure\n");
-		goto error;
-	}
-
-	pipe = usb_sndbulkpipe(udev, ep_out->bEndpointAddress);
-	usb_fill_bulk_urb(rr3->write_urb, udev, pipe,
-			  rr3->bulk_out_buf, ep_out->wMaxPacketSize,
-			  redrat3_write_bulk_callback, rr3);
-
 	rr3->udev = udev;
 
 	redrat3_reset(rr3);
-- 
1.7.2.5