diff mbox

[media] lirc: introduce LIRC_SET_TRANSMITTER_WAIT ioctl

Message ID 1477578953-5309-1-git-send-email-sean@mess.org (mailing list archive)
State New, archived
Headers show

Commit Message

Sean Young Oct. 27, 2016, 2:35 p.m. UTC
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

Comments

Andi Shyti Oct. 28, 2016, 7:38 a.m. UTC | #1
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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sean Young Oct. 28, 2016, 9:05 a.m. UTC | #2
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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andi Shyti Oct. 31, 2016, 6:13 a.m. UTC | #3
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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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