Re: [PATCH 2/3] media: rc: new driver for USB-UIRT device

[Oliver Neukum <oneukum@xxxxxxxx>]

Am Dienstag, den 04.05.2021, 18:52 +0100 schrieb Sean Young:
> See http://www.usbuirt.com/
> 

Hi,

nice driver, just a few issues.
I have marked them inline.

	Regards
		Oliver

> +
> +#define WDR_TIMEOUT 5000 /* default urb timeout */

That is the default ctrl timeout. Do you need this?

> +#define WDR_SHORT_TIMEOUT 1000	/* shorter urb timeout */
> +#define UNIT_US 50
> +#define IR_TIMEOUT 12500
> +#define MAX_PACKET 64
> 
> +static int uirt_tx(struct rc_dev *rc, uint *txbuf, uint count)
> +{
> +	struct uirt *uirt = rc->priv;
> +	u8 *out;
> +	u32 i, dest, unit_raw, freq, len;
> +	int err;
> +
> +	// streaming tx does not work for short IR; use non-streaming
> +	// tx for short IR
> +	if (count <= 24)
> +		return uirt_short_tx(rc, txbuf, count);
> +
> +	out = kmalloc(count * 2 + 3, GFP_KERNEL);
> +	if (!out)
> +		return -ENOMEM;
> +
> +	out[0] = 0x25; // Streaming Transmit
> +	out[1] = 0xdb; // checksum

A constant checksum? Now that is a new concept.

> +	out[2] = uirt->freq; // carrier frequency
> +
> +	dest = 3;
> +
> +	freq = uirt->freq & 0x7f;
> +
> +	for (i = 0; i < count; i++) {
> +		// width = (us / freq) * 2.5
> +		unit_raw = DIV_ROUND_CLOSEST(txbuf[i] * 5, freq * 2);
> +
> +		if (unit_raw == 0)
> +			unit_raw = 1;
> +		else if (unit_raw > 127)
> +			out[dest++] = (unit_raw >> 8) | 0x80;
> +
> +		out[dest++] = unit_raw;
> +	}
> +
> +	len = min_t(u32, dest, MAX_PACKET);
> +
> +	uirt->tx_buf = out + len;
> +	uirt->tx_len = dest - len;
> +
> +	err = uirt_command(uirt, out, len, CMD_STATE_STREAMING_TX);
> +	kfree(out);
> +	if (err != 0)
> +		return err;
> +
> +	return count;
> +}
> +
> 
> +static int uirt_probe(struct usb_interface *intf,
> +		      const struct usb_device_id *id)
> +{
> +	struct usb_host_interface *idesc = intf->cur_altsetting;
> +	struct usb_device *usbdev = interface_to_usbdev(intf);
> +	struct usb_endpoint_descriptor *ep_in = NULL;
> +	struct usb_endpoint_descriptor *ep_out = NULL;
> +	struct usb_endpoint_descriptor *ep = NULL;
> +	struct uirt *uirt;
> +	struct rc_dev *rc;
> +	struct urb *urb;
> +	int i, pipe, err = -ENOMEM;
> +
> +	for (i = 0; i < idesc->desc.bNumEndpoints; i++) {
> +		ep = &idesc->endpoint[i].desc;
> +
> +		if (!ep_in && usb_endpoint_is_bulk_in(ep) &&
> +		    usb_endpoint_maxp(ep) == MAX_PACKET)
> +			ep_in = ep;
> +
> +		if (!ep_out && usb_endpoint_is_bulk_out(ep) &&
> +		    usb_endpoint_maxp(ep) == MAX_PACKET)
> +			ep_out = ep;
> +	}
> +
> +	if (!ep_in || !ep_out) {
> +		dev_err(&intf->dev, "required endpoints not found\n");
> +		return -ENODEV;
> +	}
> +
> +	uirt = kzalloc(sizeof(*uirt), GFP_KERNEL);
> +	if (!uirt)
> +		return -ENOMEM;
> +
> +	uirt->in = kmalloc(MAX_PACKET, GFP_KERNEL);
> +	if (!uirt->in)
> +		goto free_uirt;
> +
> +	uirt->out = kmalloc(MAX_PACKET, GFP_KERNEL);
> +	if (!uirt->out)
> +		goto free_uirt;
> +
> +	rc = rc_allocate_device(RC_DRIVER_IR_RAW);
> +	if (!rc)
> +		goto free_uirt;
> +
> +	urb = usb_alloc_urb(0, GFP_KERNEL);
> +	if (!urb)
> +		goto free_rcdev;
> +
> +	pipe = usb_rcvbulkpipe(usbdev, ep_in->bEndpointAddress);
> +	usb_fill_bulk_urb(urb, usbdev, pipe, uirt->in, MAX_PACKET,
> +			  uirt_in_callback, uirt);
> +	uirt->urb_in = urb;
> +
> +	urb = usb_alloc_urb(0, GFP_KERNEL);
> +	if (!urb)
> +		goto free_rcdev;
> +
> +	pipe = usb_sndbulkpipe(usbdev, ep_out->bEndpointAddress);
> +	usb_fill_bulk_urb(urb, usbdev, pipe, uirt->out, MAX_PACKET,
> +			  uirt_out_callback, uirt);
> +
> +	uirt->dev = &intf->dev;
> +	uirt->usbdev = usbdev;
> +	uirt->rc = rc;
> +	uirt->urb_out = urb;
> +	uirt->rx_state = RX_STATE_INTERSPACE_HIGH;
> +
> +	err = usb_submit_urb(uirt->urb_in, GFP_KERNEL);
> +	if (err != 0) {
> +		dev_err(uirt->dev, "failed to submit read urb: %d\n",
> err);
> +		return err;

Massive memory leak. You cannot just return.