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

[Johan Hovold <johan@kernel.org>]

On Fri, Jul 02, 2021 at 12:42:18PM +0200, Johan Hovold wrote:
> On Fri, Jun 18, 2021 at 11:18:46AM +0100, Sean Young wrote:
> > This device uses an ftdi usb serial port, so this driver has a tiny
> > amount of usb ftdi code. It would be preferable to connect this driver via
> > serdev or line-discipline, but unfortunately neither support
> > hotplugging yet.
> > 
> > See http://www.usbuirt.com/
> > 
> > Signed-off-by: Sean Young <sean@mess.org>
> > ---

> > +struct uirt {
> > +	struct device *dev;
> > +	struct usb_device *usbdev;
> > +
> > +	struct rc_dev *rc;
> > +	struct urb *urb_in, *urb_out;
> > +
> > +	u8 *in;
> > +	u8 *out;
> > +	struct completion cmd_done;
> > +	u8 freq;
> > +	u8 high;
> > +	bool wideband;
> > +	u32 last_duration;
> > +
> > +	enum cmd_state cmd_state;
> > +	enum rx_state rx_state;
> > +
> > +	void *tx_buf;
> > +	u32 tx_len;
> > +
> > +	char phys[64];
> > +};

> > +static void uirt_response(struct uirt *uirt, u32 len)
> > +{
> > +	int offset = 2;
> > +	int i;
> > +
> > +	dev_dbg(uirt->dev, "state:%d data: %*phN\n", uirt->cmd_state, len, uirt->in);
> > +
> > +	// Do we have more IR to transmit and is Clear-To-Send set
> > +	if (uirt->cmd_state == CMD_STATE_STREAMING_TX && len >= 2 &&
> > +	    uirt->tx_len && uirt->in[0] & FTDI_RS0_CTS) {
> 
> Do you really need to handle this manually when you have hardware
> assisted flow control enabled?
> 
> > +		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);

Also, this looks entirely broken since you don't have any
synchronisation with uirt_command() below which may try to submit the
same URB in parallel.

> > +static int uirt_command(struct uirt *uirt, const u8 *cmd, u32 cmd_len,
> > +			enum cmd_state state)
> > +{
> > +	int err;
> > +
> > +	init_completion(&uirt->cmd_done);
> > +
> > +	uirt->cmd_state = state;
> > +
> > +	memcpy(uirt->out, cmd, cmd_len);
> > +	uirt->urb_out->transfer_buffer_length = cmd_len;
> > +
> > +	err = usb_submit_urb(uirt->urb_out, GFP_KERNEL);
> > +	if (err != 0) {
> > +		uirt->cmd_state = CMD_STATE_IRDATA;
> > +		return err;
> > +	}
> > +
> > +	if (!wait_for_completion_timeout(&uirt->cmd_done,
> > +					 msecs_to_jiffies(USB_CTRL_SET_TIMEOUT))) {
> > +		usb_kill_urb(uirt->urb_out);
> > +		uirt->cmd_state = CMD_STATE_IRDATA;
> > +		return -ETIMEDOUT;
> > +	}
> > +
> > +	return 0;
> > +}
> 
> > +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 over command (just the previous byte)
> > +	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);
> 
> This look fragile; are you sure nothing can access uirt_tx->buf (out)
> after the command returns here? 
> 
> > +	if (err != 0)
> > +		return err;
> > +
> > +	return count;
> > +}

Johan