[PATCH] [media] lirc: introduce LIRC_SET_TRANSMITTER_WAIT ioctl

[PATCH] [media] lirc: introduce LIRC_SET_TRANSMITTER_WAIT ioctl

[Sean Young]

lirc transmit waits for the IR to complete, since existing versions
of lircd (prior to 0.9.4) rely on this. Allows this to be configurable
if this is not desirable.

Signed-off-by: Sean Young <sean@mess.org>
---
 Documentation/media/uapi/rc/lirc-func.rst          |  1 +
 .../media/uapi/rc/lirc-set-transmitter-wait.rst    | 46 ++++++++++++++++++++++
 Documentation/media/uapi/rc/lirc-write.rst         |  6 ++-
 drivers/media/rc/ir-lirc-codec.c                   | 29 +++++++++-----
 drivers/media/rc/rc-core-priv.h                    |  2 +-
 include/uapi/linux/lirc.h                          |  5 +++
 6 files changed, 76 insertions(+), 13 deletions(-)
 create mode 100644 Documentation/media/uapi/rc/lirc-set-transmitter-wait.rst

diff --git a/Documentation/media/uapi/rc/lirc-func.rst b/Documentation/media/uapi/rc/lirc-func.rst
index 9b5a772..be02ca2 100644
--- a/Documentation/media/uapi/rc/lirc-func.rst
+++ b/Documentation/media/uapi/rc/lirc-func.rst
@@ -26,3 +26,4 @@ LIRC Function Reference
     lirc-set-rec-timeout-reports
     lirc-set-measure-carrier-mode
     lirc-set-wideband-receiver
+    lirc-set-transmitter-wait
diff --git a/Documentation/media/uapi/rc/lirc-set-transmitter-wait.rst b/Documentation/media/uapi/rc/lirc-set-transmitter-wait.rst
new file mode 100644
index 0000000..37835ad
--- /dev/null
+++ b/Documentation/media/uapi/rc/lirc-set-transmitter-wait.rst
@@ -0,0 +1,46 @@
+.. -*- coding: utf-8; mode: rst -*-
+
+.. _lirc_set_transmitter_mask:
+
+*******************************
+ioctl LIRC_SET_TRANSMITTER_WAIT
+*******************************
+
+Name
+====
+
+LIRC_SET_TRANSMITTER_WAIT - Wait for IR to transmit
+
+Synopsis
+========
+
+.. c:function:: int ioctl( int fd, LIRC_SET_TRANSMITTER_WAIT, __u32 *enable )
+    :name: LIRC_SET_TRANSMITTER_WAIT
+
+Arguments
+=========
+
+``fd``
+    File descriptor returned by open().
+
+``enable``
+    enable = 1 means wait for IR to transmit before write() returns,
+    enable = 0 means return as soon as the driver has sent the commmand
+    to the hardware.
+
+
+Description
+===========
+
+Early lirc drivers would only return from write() when the IR had been
+transmitted and the lirc daemon relies on this for calculating when to
+send the next IR signal. Some drivers (e.g. usb drivers) can return
+earlier than that.
+
+
+Return Value
+============
+
+On success 0 is returned, on error -1 and the ``errno`` variable is set
+appropriately. The generic error codes are described at the
+:ref:`Generic Error Codes <gen-errors>` chapter.
diff --git a/Documentation/media/uapi/rc/lirc-write.rst b/Documentation/media/uapi/rc/lirc-write.rst
index 3b035c6..32c2152 100644
--- a/Documentation/media/uapi/rc/lirc-write.rst
+++ b/Documentation/media/uapi/rc/lirc-write.rst
@@ -46,8 +46,10 @@ The data written to the chardev is a pulse/space sequence of integer
 values. Pulses and spaces are only marked implicitly by their position.
 The data must start and end with a pulse, therefore, the data must
 always include an uneven number of samples. The write function must
-block until the data has been transmitted by the hardware. If more data
-is provided than the hardware can send, the driver returns ``EINVAL``.
+block until the data has been transmitted by the hardware, unless
+:ref:`LIRC_SET_TRANSMITTER_WAIT <LIRC_SET_TRANSMITTER_WAIT>` has been
+disabled. If more data is provided than the hardware can send, the driver
+returns ``EINVAL``.
 
 
 Return Value
diff --git a/drivers/media/rc/ir-lirc-codec.c b/drivers/media/rc/ir-lirc-codec.c
index c327730..110e501 100644
--- a/drivers/media/rc/ir-lirc-codec.c
+++ b/drivers/media/rc/ir-lirc-codec.c
@@ -161,17 +161,19 @@ static ssize_t ir_lirc_transmit_ir(struct file *file, const char __user *buf,
 
 	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));
+		}
 	}
-
 out:
 	kfree(txbuf);
 	return ret;
@@ -234,6 +236,13 @@ static long ir_lirc_ioctl(struct file *filep, unsigned int cmd,
 
 		return dev->s_tx_duty_cycle(dev, val);
 
+	case LIRC_SET_TRANSMITTER_WAIT:
+		if (!dev->tx_ir)
+			return -ENOTTY;
+
+		lirc->tx_no_wait = !val;
+		break;
+
 	/* RX settings */
 	case LIRC_SET_REC_CARRIER:
 		if (!dev->s_rx_carrier_range)
diff --git a/drivers/media/rc/rc-core-priv.h b/drivers/media/rc/rc-core-priv.h
index 585d5e5..0c0d2f2 100644
--- 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;
 	struct xmp_dec {
 		int state;
diff --git a/include/uapi/linux/lirc.h b/include/uapi/linux/lirc.h
index 991ab45..3874f21 100644
--- a/include/uapi/linux/lirc.h
+++ b/include/uapi/linux/lirc.h
@@ -130,4 +130,9 @@
 
 #define LIRC_SET_WIDEBAND_RECEIVER     _IOW('i', 0x00000023, __u32)
 
+/*
+ * Should the lirc driver wait until the IR has been transmitted.
+ */
+#define LIRC_SET_TRANSMITTER_WAIT      _IOW('i', 0x00000024, __u32)
+
 #endif
-- 
2.7.4

Re: [PATCH] [media] lirc: introduce LIRC_SET_TRANSMITTER_WAIT ioctl

[Andi Shyti]

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

Re: [PATCH] [media] lirc: introduce LIRC_SET_TRANSMITTER_WAIT ioctl

[Sean Young]

Hi Andi,

On Fri, Oct 28, 2016 at 04:38:39PM +0900, Andi Shyti wrote:
> 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.

What problem are you trying to solve?

I wrote this patch as a response to this patch:

https://lkml.org/lkml/2016/9/1/653

In the spi case, the driver already waits for the IR to complete so the 
wait in ir_lirc_transmit_ir() is unnecessary. However it does not end up
waiting. There are other drivers like yours that wait for the IR to 
complete (ene_ir, ite-cir). Since towait in ir_lirc_transmit_ir is the 
delta between before and after the driver transmits, it will be 0 and 
will never goto into schedule_timeout(), barring some very minor rounding 
differences.

> 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);

I don't understand what problem this would introduce.

You can't write more than 512 pulse/spaces and each write cannot
have more than 500ms in IR (so adding up the pulses and spaces). The driver
should only send once the previous send completed.

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

That is true. Do you want the ir-spi driver to be able to send without
waiting?

> > --- 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?

This was just done to avoid having to initialise to true (non-zero).


Thanks,
Sean

Re: [PATCH] [media] lirc: introduce LIRC_SET_TRANSMITTER_WAIT ioctl

[Andi Shyti]

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.
> 
> What problem are you trying to solve?
> 
> I wrote this patch as a response to this patch:
> 
> https://lkml.org/lkml/2016/9/1/653

Actually no big problem here but what I wrote below.

If the user sets LIRC_SET_TRANSMITTER_WAIT to 0 and the driver
waits anyway, this patch wouldn't be of any use and it would
do exactly what it whas doing before.

> In the spi case, the driver already waits for the IR to complete so the 
> wait in ir_lirc_transmit_ir() is unnecessary. However it does not end up
> waiting. There are other drivers like yours that wait for the IR to 
> complete (ene_ir, ite-cir). Since towait in ir_lirc_transmit_ir is the 
> delta between before and after the driver transmits, it will be 0 and 
> will never goto into schedule_timeout(), barring some very minor rounding 
> differences.
> 
> > 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);
> 
> I don't understand what problem this would introduce.
> 
> You can't write more than 512 pulse/spaces and each write cannot
> have more than 500ms in IR (so adding up the pulses and spaces). The driver
> should only send once the previous send completed.

OK.

> > > +	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;
> 
> That is true. Do you want the ir-spi driver to be able to send without
> waiting?

I think there should be some meccanism to keep it coherent with
the ABI, mine was a suggestion.

> > > --- 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?
> 
> This was just done to avoid having to initialise to true (non-zero).

OK, this was just a nitpick anyway :)

Thanks,
Andi