Re: [PATCH v2 1/3] media: rc: add support for Infrared Toy and IR Droid devices

[Sean Young <sean@mess.org>]

Hi Oliver,

On Wed, May 27, 2020 at 01:16:41PM +0200, Oliver Neukum wrote:
> Am Mittwoch, den 27.05.2020, 10:41 +0100 schrieb Sean Young:
> 
> Hi,
> 
> thank you for the driver. Much cleaner than routing this through
> CDC_ACM.

Yes, it certainly is. It took some time for that insight to sink in (for me).

> I am afraid there are a few issues though. Nothing major.
> I've added remarks directly to the code. Would you care to
> fix them up?

Of course, thank you for reviewing it. The driver is much better for it.

I'll send out a v3 shortly, response and a question below.

Thanks,

Sean

> 
> 	Regards
> 		Oliver
> 
> > +static const u8 COMMAND_VERSION[] = { 'v' };
> > +// End transmit and repeat reset command so we exit sump mode
> > +static const u8 COMMAND_RESET[] = { 0xff, 0xff, 0, 0, 0, 0, 0 };
> > +static const u8 COMMAND_SMODE_ENTER[] = { 's' };
> > +static const u8 COMMAND_TXSTART[] = { 0x26, 0x24, 0x25, 0x03 };
> 
> Using these directly as buffers is based on the assuption that the
> kernel code is accessable by DMA. On some architectures that is
> false. You need to use a bounce buffer.

This is true, however these arrays are not directly accessed, they're
memcpy'ed into irtoy->out.

> 
> > +
> > +#define REPLY_XMITCOUNT 't'
> > +#define REPLY_XMITSUCCESS 'C'
> > +#define REPLY_VERSION 'V'
> > +#define REPLY_SAMPLEMODEPROTO 'S'
> > +
> > +#define TIMEOUT 500
> > +
> > +#define LEN_XMITRES 3
> > +#define LEN_VERSION 4
> > +#define LEN_SAMPLEMODEPROTO 3
> > +
> > +#define MIN_FW_VERSION 20
> > +#define UNIT_NS 21333
> > +#define MAX_TIMEOUT_NS (UNIT_NS * U16_MAX)
> > +
> > +#define MAX_PACKET 64
> > +
> > +enum state {
> > +	STATE_IRDATA,
> > +	STATE_RESET,
> > +	STATE_COMMAND,
> > +	STATE_TX,
> > +};
> > +
> > +struct irtoy {
> > +	struct device *dev;
> > +	struct usb_device *usbdev;
> > +
> > +	struct rc_dev *rc;
> > +	struct urb *urb_in, *urb_out;
> > +
> > +	u8 in[MAX_PACKET];
> > +	u8 out[MAX_PACKET];
> 
> This violates the DMA coherency rules. The buffers must be
> allocated separately with kmalloc().

Right, I'll fix this and send out a v3. There are other usb drivers in
drivers/media/rc/.. that break this rule too.

> > +	case STATE_IRDATA: {
> > +		struct ir_raw_event rawir = { .pulse = irtoy->pulse };
> > +		__be16 *in = (__be16 *)irtoy->in;
> > +		int i;
> > +
> > +		for (i = 0; i < len / sizeof(__be16); i++) {
> > +			u32 v = be16_to_cpup(in + i);
> 
> Is this 16 or 32 bit?

It's 16 bit but I would like it up-cast so that v * UNIT_NS is a 32 bit
multiply. This could do with a comment. Also could be be16_to_cpu(in[i]).

> 
> > +
> > +			if (v == 0xffff) {
> > +				rawir.pulse = false;
> > +			} else {
> > +				rawir.duration = v * UNIT_NS;
> > +				ir_raw_event_store_with_timeout(irtoy->rc,
> > +								&rawir);
> > +			}
> > +
> > +			rawir.pulse = !rawir.pulse;
> > +		}
> > +
> > +		irtoy->pulse = rawir.pulse;
> > +
> > +		ir_raw_event_handle(irtoy->rc);
> > +		break;
> > +	}
> > +	case STATE_TX:
> > +		if (irtoy->tx_len == 0) {
> > +			if (len == LEN_XMITRES &&
> > +			    irtoy->in[0] == REPLY_XMITCOUNT) {
> 
> Endianness?

Single byte.

> > +				__be16 *emitted = (__be16 *)(irtoy->in + 1);
> > +
> > +				irtoy->emitted = be16_to_cpup(emitted);
> 
> Reason you are using cpup versions?

emitted is not aligned. So, I'm wrong and this should be get_unaligned_be16().

> 
> > +			} else if (len == 1 &&
> > +				   irtoy->in[0] == REPLY_XMITSUCCESS) {
> > +				complete(&irtoy->rx_done);
> > +				irtoy->state = STATE_IRDATA;
> 
> Race condition. Whoever you wake up with that complete could read
> the old state.

irtoy->state is only read in this function, the urb callback handler. However,
as you noticed, this is looks wrong so it should be re-ordered.

> > +			}
> > +		} else {
> > +			// send next part of tx buffer
> > +			uint max_send = irtoy->in[0];
> > +			uint buf_len = min(max_send, irtoy->tx_len);
> > +			int err;
> > +
> > +			dev_dbg(irtoy->dev, "ready to receive: 0x%02x\n",
> > +				max_send);
> > +
> > +			memcpy(irtoy->out, irtoy->tx_buf, buf_len);
> > +			irtoy->urb_out->transfer_buffer_length = buf_len;
> > +			err = usb_submit_urb(irtoy->urb_out, GFP_ATOMIC);
> > +			if (err != 0) {
> > +				dev_err(irtoy->dev, "fail to submit tx buf urb: %d\n",
> > +					err);
> > +				complete(&irtoy->rx_done);
> > +				irtoy->state = STATE_IRDATA;
> 
> Same race condition as above.
> 
> > +			}
> > +
> > +			irtoy->tx_buf += buf_len;
> > +			irtoy->tx_len -= buf_len;
> > +			break;
> > +		}
> > +		break;
> > +	case STATE_RESET:
> > +		dev_err(irtoy->dev, "unexpected response to reset: %*phN\n",
> > +			len, irtoy->in);
> > +	}
> > +}
> > +
> > +static void irtoy_out_callback(struct urb *urb)
> > +{
> > +	struct irtoy *irtoy = urb->context;
> > +
> > +	switch (urb->status) {
> > +	case 0:
> > +		if (irtoy->state == STATE_RESET)
> > +			complete(&irtoy->rx_done);
> > +		break;
> > +
> > +	case -ECONNRESET:
> > +	case -ENOENT:
> > +	case -ESHUTDOWN:
> > +	case -EPROTO:
> > +		usb_unlink_urb(urb);
> 
> Redundant.

Removed, thanks.

> > +		return;
> > +
> > +	default:
> > +		dev_warn(irtoy->dev, "out urb status: %d\n", urb->status);
> > +	}
> > +}
> > +
> > +static void irtoy_in_callback(struct urb *urb)
> > +{
> > +	struct irtoy *irtoy = urb->context;
> > +	int ret;
> > +
> > +	switch (urb->status) {
> > +	case 0:
> > +		irtoy_response(irtoy, urb->actual_length);
> > +		break;
> > +
> > +	case -ECONNRESET:
> > +	case -ENOENT:
> > +	case -ESHUTDOWN:
> > +	case -EPROTO:
> > +		usb_unlink_urb(urb);
> 
> Redundant.

Removed, thanks.

> > +		return;
> > +
> > +	default:
> > +		dev_warn(irtoy->dev, "in urb status: %d\n", urb->status);
> > +	}
> > +
> > +	ret = usb_submit_urb(urb, GFP_ATOMIC);
> > +	if (ret && ret != -ENODEV)
> > +		dev_warn(irtoy->dev, "failed to resubmit urb: %d\n", ret);
> > +}
> > +
> > +static int irtoy_command(struct irtoy *irtoy, const u8 *cmd, int cmd_len)
> > +{
> > +	int err;
> > +
> > +	init_completion(&irtoy->rx_done);
> > +
> > +	memcpy(irtoy->out, cmd, cmd_len);
> > +	irtoy->urb_out->transfer_buffer_length = cmd_len;
> > +
> > +	err = usb_submit_urb(irtoy->urb_out, GFP_KERNEL);
> > +	if (err != 0)
> > +		return err;
> > +
> > +	if (!wait_for_completion_timeout(&irtoy->rx_done,
> > +					 msecs_to_jiffies(TIMEOUT)))
> > +		return -ETIMEDOUT;
> 
> Wrong error handling. The URB is still active. You cannot free the
> buffer. The caller has no idea when that is safe to do so. You must
> kill the URB in the timeout case.

Good point, fixed.

> > +
> > +	return 0;
> > +}
> > +
> > +/*
> > + * When sending IR, it is imperative that we send the IR data as quickly
> > + * as possible to the device, so it does not run out of IR data and
> > + * introduce gaps. So, we feed the data from the urb callback handler
> > + */
> > +static int irtoy_tx(struct rc_dev *rc, uint *txbuf, uint count)
> > +{
> > +	struct irtoy *irtoy = rc->priv;
> > +	unsigned int i, size;
> > +	__be16 *buf;
> > +	int err;
> > +
> > +	size = sizeof(u16) * (count + 1);
> > +	buf = kmalloc(size, GFP_KERNEL);
> 
> This is incompatible with the comment. If you are potentially in
> interrupt, you must use GFP_ATOMIC. Please clarify.

This function is called in process context, from a write() on a lirc chardev.
However, this buffer is stored in irtoy->tx_buf. It is then sent, in packets,
to the device (memcpy'ed to irtoy->out). This handled in irtoy_response() in
a response to the device sending a "you may send X bytes irdata now" message. 

I'll update the comment.

> > +static int irtoy_suspend(struct usb_interface *intf, pm_message_t message)
> > +{
> > +	struct irtoy *irtoy = usb_get_intfdata(intf);
> > +
> > +	usb_kill_urb(irtoy->urb_in);
> > +	usb_kill_urb(irtoy->urb_out);
> 
> That is brutal. It could fail commands. Do you really want to
> do that?

Commands can only be sent during 1) device probe and 2) ir transmit. During
ir transmit we are non-interruptable process context, so we should not end up
here unless I'm mistaken.

When we're not issuing commands we're waiting for ir receive; should that
urb be killed for the duration of suspend?

> 
> > +
> > +	return 0;
> > +}
> > +
> > +static int irtoy_resume(struct usb_interface *intf)
> > +{
> > +	struct irtoy *irtoy = usb_get_intfdata(intf);
> > +	int err;
> > +
> > +	err = usb_submit_urb(irtoy->urb_in, GFP_KERNEL);
> 
> That should technically be GFP_NOIO

Fixed.

> 
> > +	if (err)
> > +		dev_warn(&intf->dev, "failed to submit urb: %d\n", err);
> > +
> > +	return err;
> > +}