Hi Sean,
> ret *= sizeof(unsigned int);
>
> - /*
> - * The lircd gap calculation expects the write function to
> - * wait for the actual IR signal to be transmitted before
> - * returning.
> - */
> - towait = ktime_us_delta(ktime_add_us(start, duration), ktime_get());
> - if (towait > 0) {
> - set_current_state(TASK_INTERRUPTIBLE);
> - schedule_timeout(usecs_to_jiffies(towait));
> + if (!lirc->tx_no_wait) {
> + /*
> + * The lircd gap calculation expects the write function to
> + * wait for the actual IR signal to be transmitted before
> + * returning.
> + */
> + towait = ktime_us_delta(ktime_add_us(start, duration),
> + ktime_get());
> + if (towait > 0) {
> + set_current_state(TASK_INTERRUPTIBLE);
> + schedule_timeout(usecs_to_jiffies(towait));
> + }
> }
> -
this doesn't fix my problem, though.
This approach gives the userspace the possibility to choose to
either sync or not. In my case the sync happens, but in a
different level and it's not up to the userspace to make the
decision.
Besides, I see here a security issue: what happens if userspace
does something like
fd = open("/dev/lirc0", O_RDWR);
ioctl(fd, LIRC_SET_TRANSMITTER_WAIT, 0);
while(1)
write(fd, buffer, ENORMOUS_BUFFER_SIZE);
>
> + case LIRC_SET_TRANSMITTER_WAIT:
> + if (!dev->tx_ir)
> + return -ENOTTY;
> +
> + lirc->tx_no_wait = !val;
> + break;
> +
Here I see an innocuous bug. Depending on the hardware (for
example ir-spi) it might happen that the device waits in any
case (in ir-spi the sync is done by the spi). This means that if
userspace sets 'tx_no_wait = true', the device/driver doesn't
care and waits anyway, doing the opposite from what is described
in the ABI.
Here we could call a dev->tx_set_transmitter_wait(...) function
that sets the value or returns error in case the wait is not
feasable, something like:
case LIRC_SET_TRANSMITTER_WAIT:
if (!dev->tx_ir)
return -ENOTTY;
if (dev->tx_set_transmitter_wait)
return dev->tx_set_transmitter_wait(lirc, val);
lirc->tx_no_wait = !val;
break;
> --- a/drivers/media/rc/rc-core-priv.h
> +++ b/drivers/media/rc/rc-core-priv.h
> @@ -112,7 +112,7 @@ struct ir_raw_event_ctrl {
> u64 gap_duration;
> bool gap;
> bool send_timeout_reports;
> -
> + bool tx_no_wait;
> } lirc;
this to me looks confusing, it has a negative meaning in kernel
space and a positive meaning in userspace. Can't we call it
lirc->tx_wait instead of lirc->tx_no_wait, so that we keep the
same meaning and we don't need to negate val?
Thanks,
Andi
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
