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. > > +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
