On Fri, Jul 02, 2021 at 02:20:22PM +0100, Sean Young wrote: > On Fri, Jul 02, 2021 at 02:23:48PM +0200, Johan Hovold wrote: > > 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@xxxxxxxx> > > > > --- > > > > > > +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. > > uirt_command() only gets called via lirc chardev ioctl/write ops; the lirc > chardev code does locking for the drivers already. So, if someone does a > write to /dev/lirc0 (which means transmit) the mutex is taken, no other > writes/ioctls are allowed on /dev/lirc0; the uirt_tx() calls uirt_command() > which waits for completion. During this period the code above can be > executed, but not after the transmit succeeds or fails (when the lircdev > chardev mutex is released, see: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/media/rc/lirc_dev.c#n337 > > Having said all that this is not evident from code at all. A comment could > really help. My point is that uirt_command() may race with the bulk-in completion handler and uirt_response() which can end up submitting the out urb in parallel with uirt_command() and corrupting the tx state. There only needs to be one ioctl()/write() for this to happen. Johan
