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

[Sean Young <sean@mess.org>]

On Fri, May 14, 2021 at 01:38:11PM +0200, Johan Hovold wrote:
> On Thu, May 06, 2021 at 01:44:54PM +0100, Sean Young wrote:
> > See http://www.usbuirt.com/
> 
> No proper commit message?

No sure what to say here what's not already in the first line of the commit.

Maybe I could mention the fact this uses fdti usb serial chip.

> > Signed-off-by: Sean Young <sean@mess.org>
> 
> > +// Read IR in wideband mode. The byte stream is delivered in packets,
> > +// and the values which come in two bytes may straddle two packets
> > +static void uirt_wideband(struct uirt *uirt, u32 len)
> > +{
> > +	uint i, duration, carrier, pulses;
> > +
> > +	for (i = 2; i < len; i++) {
> > +		switch (uirt->rx_state) {
> > +		case RX_STATE_INTERSPACE_HIGH:
> > +			uirt->rx_state = RX_STATE_INTERSPACE_LOW;
> > +			break;
> > +		case RX_STATE_INTERSPACE_LOW:
> > +			uirt->rx_state = RX_STATE_ON_HIGH;
> > +			break;
> > +		case RX_STATE_ON_HIGH:
> > +			uirt->high = uirt->in[i];
> > +			uirt->rx_state = RX_STATE_ON_LOW;
> > +			break;
> > +		case RX_STATE_ON_LOW:
> > +			// duration is in 400ns units
> > +			duration = (uirt->high << 8) | uirt->in[i];
> > +			uirt->last_duration = duration;
> > +			ir_raw_event_store(uirt->rc, &((struct ir_raw_event) {
> > +				.duration = DIV_ROUND_CLOSEST(duration * 2, 5),
> > +				.pulse = true,
> > +			}));
> > +			uirt->rx_state = RX_STATE_FREQ_HIGH;
> > +			break;
> > +		case RX_STATE_FREQ_HIGH:
> > +			if (uirt->in[i] & 0x80) {
> > +				uirt->high = uirt->in[i] & 0x7f;
> > +				uirt->rx_state = RX_STATE_FREQ_LOW;
> > +				break;
> > +			}
> > +
> > +			uirt->high = 0;
> > +			fallthrough;
> > +		case RX_STATE_FREQ_LOW:
> > +			pulses = (uirt->high << 8) | uirt->in[i];
> > +			if (pulses) {
> > +				dev_dbg(uirt->dev, "carrier duration %u pulses %u",
> > +					uirt->last_duration, pulses);
> > +
> > +				// calculate the Hz of pulses in duration 400ns units
> > +				carrier = DIV_ROUND_CLOSEST_ULL(pulses * 10000000ull,
> > +								uirt->last_duration * 4);
> 
> Looks like uirt->last_duration comes from the external device and you
> don't have any sanity checks to prevent division by zero here.

Yes, you are right. In fact, if this is zero the reported IR duration is
also garbage. I'll fix this.

> 
> > +				ir_raw_event_store(uirt->rc, &((struct ir_raw_event) {
> > +					.carrier = carrier,
> > +					.carrier_report = true,
> > +				}));
> > +			}
> > +			uirt->rx_state = RX_STATE_OFF_HIGH;
> > +			break;
> > +		case RX_STATE_OFF_HIGH:
> > +			if (uirt->in[i] == 0xff) {
> > +				ir_raw_event_store(uirt->rc, &((struct ir_raw_event) {
> > +					.duration = IR_TIMEOUT,
> > +					.timeout = true,
> > +				}));
> > +				uirt->rx_state = RX_STATE_INTERSPACE_HIGH;
> > +			} else {
> > +				uirt->high = uirt->in[i];
> > +				uirt->rx_state = RX_STATE_OFF_LOW;
> > +			}
> > +			break;
> > +		case RX_STATE_OFF_LOW:
> > +			// duration is in 400ns units
> > +			duration = (uirt->high << 8) | uirt->in[i];
> > +			ir_raw_event_store(uirt->rc, &((struct ir_raw_event) {
> > +				.duration = DIV_ROUND_CLOSEST(duration * 2, 5),
> > +				.pulse = false,
> > +			}));
> > +			uirt->rx_state = RX_STATE_ON_HIGH;
> > +			break;
> > +		}
> > +	}
> > +
> > +	ir_raw_event_handle(uirt->rc);
> > +}
> > +
> > +static void uirt_response(struct uirt *uirt, u32 len)
> > +{
> > +	int i;
> > +
> > +	dev_dbg(uirt->dev, "state:%d data: %*phN\n", uirt->cmd_state, len, uirt->in);
> > +
> > +	// Do we have more IR to transmit
> > +	if (uirt->cmd_state == CMD_STATE_STREAMING_TX && len >= 2 &&
> > +	    uirt->tx_len && uirt->in[0] & FTDI_RS0_CTS) {
> > +		u32 len;
> > +		int err;
> > +
> > +		len = min_t(u32, uirt->tx_len, MAX_PACKET);
> > +
> > +		memcpy(uirt->out, uirt->tx_buf, len);
> > +		uirt->urb_out->transfer_buffer_length = len;
> > +
> > +		uirt->tx_len -= len;
> > +		uirt->tx_buf += len;
> > +
> > +		err = usb_submit_urb(uirt->urb_out, GFP_ATOMIC);
> > +		if (err != 0)
> > +			dev_warn(uirt->dev,
> > +				 "failed to submit out urb: %d\n", err);
> > +	}
> > +
> > +	// if we only have two bytes, it just gives us the serial line status
> > +	if (len <= 2)
> > +		return;
> > +
> > +	switch (uirt->cmd_state) {
> > +	case CMD_STATE_GETVERSION:
> > +		if (len == 10) {
> > +			// check checksum
> > +			u8 checksum = 0;
> > +
> > +			for (i = 2; i < len; i++)
> > +				checksum += uirt->in[i];
> > +
> > +			if (checksum != 0) {
> > +				dev_err(uirt->dev, "checksum does not match: %*phN\n",
> > +					len, uirt->in);
> > +				return;
> > +			}
> > +
> > +			dev_info(uirt->dev,
> > +				 "USB-UIRT firmware v%u.%u protocol v%u.%u %02u-%02u-%04u",
> > +				 uirt->in[2], uirt->in[3], uirt->in[4],
> > +				 uirt->in[5], uirt->in[6], uirt->in[7],
> > +				 2000 + uirt->in[8]);
> > +
> > +			complete(&uirt->cmd_done);
> > +			uirt->cmd_state = CMD_STATE_IRDATA;
> > +			return;
> > +		}
> > +		break;
> > +	case CMD_STATE_DOTXRAW:
> > +	case CMD_STATE_STREAMING_TX:
> > +	case CMD_STATE_SETMODERAW:
> > +	case CMD_STATE_SETMODEWIDEBAND:
> > +		if (len == 3) {
> > +			switch (uirt->in[2]) {
> > +			case 0x20:
> > +				// 0x20 transmitting is expected during streaming tx
> > +				if (uirt->cmd_state == CMD_STATE_STREAMING_TX)
> > +					return;
> > +
> > +				if (uirt->cmd_state == CMD_STATE_DOTXRAW)
> > +					complete(&uirt->cmd_done);
> > +				else
> > +					dev_err(uirt->dev, "device transmitting");
> > +				break;
> > +			case 0x21:
> > +				if (uirt->tx_len) {
> > +					dev_err(uirt->dev, "tx completed with %u left to send",
> > +						uirt->tx_len);
> > +				} else {
> > +					if (uirt->cmd_state == CMD_STATE_SETMODERAW)
> > +						uirt->wideband = false;
> > +					if (uirt->cmd_state == CMD_STATE_SETMODEWIDEBAND)
> > +						uirt->wideband = true;
> > +
> > +					complete(&uirt->cmd_done);
> > +				}
> > +				break;
> > +			case 0x80:
> > +				dev_err(uirt->dev, "checksum error");
> > +				break;
> > +			case 0x81:
> > +				dev_err(uirt->dev, "timeout");
> > +				break;
> > +			case 0x82:
> > +				dev_err(uirt->dev, "command error");
> > +				break;
> > +			default:
> > +				dev_err(uirt->dev, "unknown response");
> > +			}
> > +
> > +			uirt->cmd_state = CMD_STATE_IRDATA;
> > +			return;
> > +		}
> > +	default:
> > +		break;
> > +	}
> > +
> > +	if (uirt->wideband)
> > +		uirt_wideband(uirt, len);
> > +	else
> > +		uirt_raw_mode(uirt, len);
> > +}
> 
> Your code assumes that you'll always get one message per transfer, but
> since the device uses a regular FTDI chip with a FIFO this isn't
> guaranteed. 

I guess you're talking about that data can straddle multiple packets
because there is a fifo involved, or there can be more data before/after
a command response.

Let me see what I can do about that.

> > +
> > +static void uirt_out_callback(struct urb *urb)
> > +{
> > +	struct uirt *uirt = urb->context;
> > +
> > +	if (urb->status)
> > +		dev_warn(uirt->dev, "out urb status: %d\n", urb->status);
> 
> Disconnecting a device shouldn't print a warning.

Ok.

> > +}
> > +
> > +static void uirt_in_callback(struct urb *urb)
> > +{
> > +	struct uirt *uirt = urb->context;
> > +	int ret;
> > +
> > +	if (urb->status == 0)
> > +		uirt_response(uirt, urb->actual_length);
> > +	else
> > +		dev_dbg(uirt->dev, "in urb status: %d\n", urb->status);
> > +
> > +	ret = usb_submit_urb(urb, GFP_ATOMIC);
> > +	if (ret && ret != -ENODEV)
> > +		dev_warn(uirt->dev, "failed to resubmit urb: %d\n", ret);
> > +}
> 
> > +static int init_ftdi(struct usb_device *udev)
> > +{
> > +	int err;
> > +
> > +	// set the baud rate
> > +	err = usb_control_msg(udev, usb_sndctrlpipe(udev, 0),
> > +			      FTDI_SIO_SET_BAUDRATE_REQUEST,
> > +			      FTDI_SIO_SET_BAUDRATE_REQUEST_TYPE,
> > +			      0x4009, 0x0001,
> > +			      NULL, 0, USB_CTRL_SET_TIMEOUT);
> > +	if (err)
> > +		return err;
> > +
> > +	// enabling rts/cts flow control
> > +	err = usb_control_msg(udev, usb_sndctrlpipe(udev, 0),
> > +			      FTDI_SIO_SET_FLOW_CTRL_REQUEST,
> > +			      FTDI_SIO_SET_FLOW_CTRL_REQUEST_TYPE,
> > +			      0, FTDI_SIO_RTS_CTS_HS,
> > +			      NULL, 0, USB_CTRL_SET_TIMEOUT);
> > +	if (err)
> > +		return err;
> 
> Does the device UART actually have RTS wired up?

I don't know TBH. However it does have CTS wired up.

> > +
> > +	// Set latency in milliseconds. The USB-UIRT will generate a
> > +	// urb every latency milliseconds (IR or not), so this should be
> > +	// set as high as possible.
> > +	//
> > +	// The USB-UIRT has a IR timeout of 14ms (i.e. it sends the 0xff
> > +	// byte after 14ms of IR silence. So if we set the timeout to a higher
> > +	// value, the 0xff just gets sent in a separate packet, and there
> > +	// is a small delay for the IR to be processed.
> > +	return usb_control_msg(udev, usb_sndctrlpipe(udev, 0),
> > +			      FTDI_SIO_SET_LATENCY_TIMER_REQUEST,
> > +			      FTDI_SIO_SET_LATENCY_TIMER_REQUEST_TYPE,
> > +			      50, 0,
> 
> Magic constant; it's not entirely clear from the code (or comment) what
> 50 is here. The latency go as high as 255 IIRC.

You're right, the comment is not clear. I'll update it.

> > +			      NULL, 0, USB_CTRL_SET_TIMEOUT);
> > +}
> > +
> > +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;
> > +	}
> 
> You should use usb_find_common_endpoints() instead of open coding even
> if you'd still need to check the endpoint size with your implementation.

I was not aware of that function, thanks. This makes the code much shorter.

> > +
> > +	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);
> > +		goto free_rcdev;
> > +	}
> > +
> > +	err = init_ftdi(usbdev);
> > +	if (err) {
> > +		dev_err(uirt->dev, "failed to setup ftdi: %d\n", err);
> > +		goto free_rcdev;
> > +	}
> > +
> > +	err = uirt_setup(uirt);
> > +	if (err)
> > +		goto free_rcdev;
> > +
> > +	usb_make_path(usbdev, uirt->phys, sizeof(uirt->phys));
> > +
> > +	rc->device_name = "USB-UIRT";
> > +	rc->driver_name = KBUILD_MODNAME;
> > +	rc->input_phys = uirt->phys;
> > +	usb_to_input_id(usbdev, &rc->input_id);
> > +	rc->dev.parent = &intf->dev;
> > +	rc->priv = uirt;
> > +	rc->tx_ir = uirt_tx;
> > +	rc->s_tx_carrier = uirt_set_tx_carrier;
> > +	rc->s_learning_mode = uirt_set_rx_wideband;
> > +	rc->allowed_protocols = RC_PROTO_BIT_ALL_IR_DECODER;
> > +	rc->map_name = RC_MAP_RC6_MCE;
> > +	rc->rx_resolution = UNIT_US;
> > +	rc->timeout = IR_TIMEOUT;
> > +
> > +	uirt_set_tx_carrier(rc, 38000);
> > +
> > +	err = rc_register_device(rc);
> > +	if (err)
> > +		goto free_rcdev;
> > +
> > +	usb_set_intfdata(intf, uirt);
> > +
> > +	return 0;
> > +
> > +free_rcdev:
> > +	usb_kill_urb(uirt->urb_in);
> > +	usb_kill_urb(uirt->urb_out);
> 
> Looks lazy to kill the URBs before they've been allocated, better to add
> another label even if it's not a bug.

Ok, will do.

> > +	usb_free_urb(uirt->urb_in);
> > +	usb_free_urb(uirt->urb_out);
> > +	rc_free_device(rc);
> > +free_uirt:
> > +	kfree(uirt->in);
> > +	kfree(uirt->out);
> > +	kfree(uirt);
> > +	return err;
> > +}

Thank you for the review. I'll send out a v4 when its ready.


Sean