[PATCHv3 0/9] Fixes in response to review comments

[PATCHv3 0/9] Fixes in response to review comments

[Timo Kokkonen]

These patches fix most of the issues pointed out in the patch review
by Sean Young and Sakari Ailus.

The most noticeable change after these patch set is that the IR
transmission no longer times out even if the timers are not waking up
the MPU as it should be. Now that Jean Pihet kindly instructed me how
to use the PM QoS API for setting the latency constraints, the driver
will now work without any background load. Someone might consider such
restriction a blocker bug, that is fixed on this patch set.

Changes since v2:

- The 10us PM QoS latency requrement is documented in the code

- A missing quote mark is added into the Kconfig text

Changes since v1:

- Replace wake_up_interruptible with wake_up, as the driver is having
  non-interruptible sleeps

- Instead of just removing the set_max_mpu_wakeup_lat calls, replace
  them with QoS API calls

Timo Kokkonen (9):
  ir-rx51: Adjust dependencies
  ir-rx51: Handle signals properly
  ir-rx51: Trivial fixes
  ir-rx51: Clean up timer initialization code
  ir-rx51: Move platform data checking into probe function
  ir-rx51: Replace module_{init,exit} macros with
    module_platform_driver
  ir-rx51: Convert latency constraints to PM QoS API
  ir-rx51: Remove useless variable from struct lirc_rx51
  ir-rx51: Add missing quote mark in Kconfig text

 arch/arm/mach-omap2/board-rx51-peripherals.c |   2 -
 drivers/media/rc/Kconfig                     |   6 +-
 drivers/media/rc/ir-rx51.c                   | 102 ++++++++++++++-------------
 include/media/ir-rx51.h                      |   2 -
 4 files changed, 57 insertions(+), 55 deletions(-)

-- 
1.7.12

[PATCHv3 1/9] ir-rx51: Adjust dependencies

[Timo Kokkonen]

Although this kind of IR diode circuitry is known to exist only in
N900 hardware, nothing prevents making similar circuitry on any OMAP
based board. The MACH_NOKIA_RX51 dependency is thus not something we
want to be there.

Also, this should depend on LIRC as it is a LIRC driver.

Signed-off-by: Timo Kokkonen <timo.t.kokkonen@iki.fi>
---
 drivers/media/rc/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/rc/Kconfig b/drivers/media/rc/Kconfig
index ffef8b4..093982b 100644
--- a/drivers/media/rc/Kconfig
+++ b/drivers/media/rc/Kconfig
@@ -273,7 +273,7 @@ config IR_IGUANA
 
 config IR_RX51
 	tristate "Nokia N900 IR transmitter diode
-	depends on MACH_NOKIA_RX51 && OMAP_DM_TIMER
+	depends on OMAP_DM_TIMER && LIRC
 	---help---
 	   Say Y or M here if you want to enable support for the IR
 	   transmitter diode built in the Nokia N900 (RX51) device.
-- 
1.7.12

[PATCHv3 2/9] ir-rx51: Handle signals properly

[Timo Kokkonen]

The lirc-dev expects the ir-code to be transmitted when the write call
returns back to the user space. We should not leave TX ongoing no
matter what is the reason we return to the user space. Easiest
solution for that is to simply remove interruptible sleeps.

The first wait_event_interruptible is thus replaced with return -EBUSY
in case there is still ongoing transfer. This should suffice as the
concept of sending multiple codes in parallel does not make sense.

The second wait_event_interruptible call is replaced with
wait_even_timeout with a fixed and safe timeout that should prevent
the process from getting stuck in kernel for too long.

Also, from now on we will force the TX to stop before we return from
write call. If the TX happened to time out for some reason, we should
not leave the HW transmitting anything.

Signed-off-by: Timo Kokkonen <timo.t.kokkonen@iki.fi>
---
 drivers/media/rc/ir-rx51.c | 39 ++++++++++++++++++++++++++++-----------
 1 file changed, 28 insertions(+), 11 deletions(-)

diff --git a/drivers/media/rc/ir-rx51.c b/drivers/media/rc/ir-rx51.c
index 9487dd3..e2db94e 100644
--- a/drivers/media/rc/ir-rx51.c
+++ b/drivers/media/rc/ir-rx51.c
@@ -74,6 +74,19 @@ static void lirc_rx51_off(struct lirc_rx51 *lirc_rx51)
 			      OMAP_TIMER_TRIGGER_NONE);
 }
 
+static void lirc_rx51_stop_tx(struct lirc_rx51 *lirc_rx51)
+{
+	if (lirc_rx51->wbuf_index < 0)
+		return;
+
+	lirc_rx51_off(lirc_rx51);
+	lirc_rx51->wbuf_index = -1;
+	omap_dm_timer_stop(lirc_rx51->pwm_timer);
+	omap_dm_timer_stop(lirc_rx51->pulse_timer);
+	omap_dm_timer_set_int_enable(lirc_rx51->pulse_timer, 0);
+	wake_up(&lirc_rx51->wqueue);
+}
+
 static int init_timing_params(struct lirc_rx51 *lirc_rx51)
 {
 	u32 load, match;
@@ -160,13 +173,7 @@ static irqreturn_t lirc_rx51_interrupt_handler(int irq, void *ptr)
 
 	return IRQ_HANDLED;
 end:
-	/* Stop TX here */
-	lirc_rx51_off(lirc_rx51);
-	lirc_rx51->wbuf_index = -1;
-	omap_dm_timer_stop(lirc_rx51->pwm_timer);
-	omap_dm_timer_stop(lirc_rx51->pulse_timer);
-	omap_dm_timer_set_int_enable(lirc_rx51->pulse_timer, 0);
-	wake_up_interruptible(&lirc_rx51->wqueue);
+	lirc_rx51_stop_tx(lirc_rx51);
 
 	return IRQ_HANDLED;
 }
@@ -246,8 +253,9 @@ static ssize_t lirc_rx51_write(struct file *file, const char *buf,
 	if ((count > WBUF_LEN) || (count % 2 == 0))
 		return -EINVAL;
 
-	/* Wait any pending transfers to finish */
-	wait_event_interruptible(lirc_rx51->wqueue, lirc_rx51->wbuf_index < 0);
+	/* We can have only one transmit at a time */
+	if (lirc_rx51->wbuf_index >= 0)
+		return -EBUSY;
 
 	if (copy_from_user(lirc_rx51->wbuf, buf, n))
 		return -EFAULT;
@@ -273,9 +281,18 @@ static ssize_t lirc_rx51_write(struct file *file, const char *buf,
 
 	/*
 	 * Don't return back to the userspace until the transfer has
-	 * finished
+	 * finished. However, we wish to not spend any more than 500ms
+	 * in kernel. No IR code TX should ever take that long.
+	 */
+	i = wait_event_timeout(lirc_rx51->wqueue, lirc_rx51->wbuf_index < 0,
+			HZ / 2);
+
+	/*
+	 * Ensure transmitting has really stopped, even if the timers
+	 * went mad or something else happened that caused it still
+	 * sending out something.
 	 */
-	wait_event_interruptible(lirc_rx51->wqueue, lirc_rx51->wbuf_index < 0);
+	lirc_rx51_stop_tx(lirc_rx51);
 
 	/* We can sleep again */
 	lirc_rx51->pdata->set_max_mpu_wakeup_lat(lirc_rx51->dev, -1);
-- 
1.7.12

[PATCHv3 3/9] ir-rx51: Trivial fixes

[Timo Kokkonen]

-Fix typo

-Change pwm_timer_num type to match type in platform data

-Remove extra parenthesis

-Replace magic constant with proper bit defintions

-Remove duplicate exit pointer

Signed-off-by: Timo Kokkonen <timo.t.kokkonen@iki.fi>
---
 drivers/media/rc/Kconfig   |  2 +-
 drivers/media/rc/ir-rx51.c | 10 ++++++----
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/media/rc/Kconfig b/drivers/media/rc/Kconfig
index 093982b..4a68014 100644
--- a/drivers/media/rc/Kconfig
+++ b/drivers/media/rc/Kconfig
@@ -278,7 +278,7 @@ config IR_RX51
 	   Say Y or M here if you want to enable support for the IR
 	   transmitter diode built in the Nokia N900 (RX51) device.
 
-	   The driver uses omap DM timers for gereating the carrier
+	   The driver uses omap DM timers for generating the carrier
 	   wave and pulses.
 
 config RC_LOOPBACK
diff --git a/drivers/media/rc/ir-rx51.c b/drivers/media/rc/ir-rx51.c
index e2db94e..125d4c3 100644
--- a/drivers/media/rc/ir-rx51.c
+++ b/drivers/media/rc/ir-rx51.c
@@ -59,7 +59,7 @@ struct lirc_rx51 {
 	int		wbuf[WBUF_LEN];
 	int		wbuf_index;
 	unsigned long	device_is_open;
-	unsigned int	pwm_timer_num;
+	int		pwm_timer_num;
 };
 
 static void lirc_rx51_on(struct lirc_rx51 *lirc_rx51)
@@ -138,11 +138,14 @@ static irqreturn_t lirc_rx51_interrupt_handler(int irq, void *ptr)
 	if (!retval)
 		return IRQ_NONE;
 
-	if ((retval & ~OMAP_TIMER_INT_MATCH))
+	if (retval & ~OMAP_TIMER_INT_MATCH)
 		dev_err_ratelimited(lirc_rx51->dev,
 				": Unexpected interrupt source: %x\n", retval);
 
-	omap_dm_timer_write_status(lirc_rx51->pulse_timer, 7);
+	omap_dm_timer_write_status(lirc_rx51->pulse_timer,
+				OMAP_TIMER_INT_MATCH	|
+				OMAP_TIMER_INT_OVERFLOW	|
+				OMAP_TIMER_INT_CAPTURE);
 	if (lirc_rx51->wbuf_index < 0) {
 		dev_err_ratelimited(lirc_rx51->dev,
 				": BUG wbuf_index has value of %i\n",
@@ -489,7 +492,6 @@ struct platform_driver lirc_rx51_platform_driver = {
 	.remove		= __exit_p(lirc_rx51_remove),
 	.suspend	= lirc_rx51_suspend,
 	.resume		= lirc_rx51_resume,
-	.remove		= __exit_p(lirc_rx51_remove),
 	.driver		= {
 		.name	= DRIVER_NAME,
 		.owner	= THIS_MODULE,
-- 
1.7.12

[PATCHv3 4/9] ir-rx51: Clean up timer initialization code

[Timo Kokkonen]

Remove a redundant macro definition. This is unneeded and becomes more
readable once the actual timer code is refactored a little.

Signed-off-by: Timo Kokkonen <timo.t.kokkonen@iki.fi>
---
 drivers/media/rc/ir-rx51.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/media/rc/ir-rx51.c b/drivers/media/rc/ir-rx51.c
index 125d4c3..f22e5e4 100644
--- a/drivers/media/rc/ir-rx51.c
+++ b/drivers/media/rc/ir-rx51.c
@@ -105,11 +105,9 @@ static int init_timing_params(struct lirc_rx51 *lirc_rx51)
 	return 0;
 }
 
-#define tics_after(a, b) ((long)(b) - (long)(a) < 0)
-
 static int pulse_timer_set_timeout(struct lirc_rx51 *lirc_rx51, int usec)
 {
-	int counter;
+	int counter, counter_now;
 
 	BUG_ON(usec < 0);
 
@@ -122,11 +120,8 @@ static int pulse_timer_set_timeout(struct lirc_rx51 *lirc_rx51, int usec)
 	omap_dm_timer_set_match(lirc_rx51->pulse_timer, 1, counter);
 	omap_dm_timer_set_int_enable(lirc_rx51->pulse_timer,
 				     OMAP_TIMER_INT_MATCH);
-	if (tics_after(omap_dm_timer_read_counter(lirc_rx51->pulse_timer),
-		       counter)) {
-		return 1;
-	}
-	return 0;
+	counter_now = omap_dm_timer_read_counter(lirc_rx51->pulse_timer);
+	return (counter - counter_now) < 0;
 }
 
 static irqreturn_t lirc_rx51_interrupt_handler(int irq, void *ptr)
-- 
1.7.12

[PATCHv3 5/9] ir-rx51: Move platform data checking into probe function

[Timo Kokkonen]

This driver is useless without proper platform data. If data is not
available, we should not register the driver at all. Once this check
is done, the BUG_ON check during device open is no longer needed.

Signed-off-by: Timo Kokkonen <timo.t.kokkonen@iki.fi>
---
 drivers/media/rc/ir-rx51.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/media/rc/ir-rx51.c b/drivers/media/rc/ir-rx51.c
index f22e5e4..16b3c1f 100644
--- a/drivers/media/rc/ir-rx51.c
+++ b/drivers/media/rc/ir-rx51.c
@@ -378,7 +378,6 @@ static long lirc_rx51_ioctl(struct file *filep,
 static int lirc_rx51_open(struct inode *inode, struct file *file)
 {
 	struct lirc_rx51 *lirc_rx51 = lirc_get_pdata(file);
-	BUG_ON(!lirc_rx51);
 
 	file->private_data = lirc_rx51;
 
@@ -458,6 +457,9 @@ static int lirc_rx51_resume(struct platform_device *dev)
 
 static int __devinit lirc_rx51_probe(struct platform_device *dev)
 {
+	if (!dev->dev.platform_data)
+		return -ENODEV;
+
 	lirc_rx51_driver.features = LIRC_RX51_DRIVER_FEATURES;
 	lirc_rx51.pdata = dev->dev.platform_data;
 	lirc_rx51.pwm_timer_num = lirc_rx51.pdata->pwm_timer;
-- 
1.7.12

[PATCHv3 6/9] ir-rx51: Replace module_{init,exit} macros with module_platform_driver

[Timo Kokkonen]

No reason to avoid using the existing helpers.

Signed-off-by: Timo Kokkonen <timo.t.kokkonen@iki.fi>
---
 drivers/media/rc/ir-rx51.c | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/drivers/media/rc/ir-rx51.c b/drivers/media/rc/ir-rx51.c
index 16b3c1f..6e1ffa6 100644
--- a/drivers/media/rc/ir-rx51.c
+++ b/drivers/media/rc/ir-rx51.c
@@ -495,17 +495,7 @@ struct platform_driver lirc_rx51_platform_driver = {
 	},
 };
 
-static int __init lirc_rx51_init(void)
-{
-	return platform_driver_register(&lirc_rx51_platform_driver);
-}
-module_init(lirc_rx51_init);
-
-static void __exit lirc_rx51_exit(void)
-{
-	platform_driver_unregister(&lirc_rx51_platform_driver);
-}
-module_exit(lirc_rx51_exit);
+module_platform_driver(lirc_rx51_platform_driver);
 
 MODULE_DESCRIPTION("LIRC TX driver for Nokia RX51");
 MODULE_AUTHOR("Nokia Corporation");
-- 
1.7.12

[PATCHv3 7/9] ir-rx51: Convert latency constraints to PM QoS API

[Timo Kokkonen]

Convert the driver from the obsolete omap_pm_set_max_mpu_wakeup_lat
API to the new PM QoS API. This allows the callback to be removed from
the platform data structure.

The latency requirements are also adjusted to prevent the MPU from
going into sleep mode. This is needed as the GP timers have no means
to wake up the MPU once it has gone into sleep. The "side effect" is
that from now on the driver actually works even if there is no
background load keeping the MPU awake.

Signed-off-by: Timo Kokkonen <timo.t.kokkonen@iki.fi>
Acked-by: Tony Lindgren <tony@atomide.com>
Acked-by: Jean Pihet <j-pihet@ti.com>
---
 arch/arm/mach-omap2/board-rx51-peripherals.c |  2 --
 drivers/media/rc/ir-rx51.c                   | 17 ++++++++++++-----
 include/media/ir-rx51.h                      |  2 --
 3 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/arch/arm/mach-omap2/board-rx51-peripherals.c b/arch/arm/mach-omap2/board-rx51-peripherals.c
index ca07264..e0750cb 100644
--- a/arch/arm/mach-omap2/board-rx51-peripherals.c
+++ b/arch/arm/mach-omap2/board-rx51-peripherals.c
@@ -34,7 +34,6 @@
 #include <plat/gpmc.h>
 #include <plat/onenand.h>
 #include <plat/gpmc-smc91x.h>
-#include <plat/omap-pm.h>
 
 #include <mach/board-rx51.h>
 
@@ -1227,7 +1226,6 @@ static void __init rx51_init_tsc2005(void)
 
 #if defined(CONFIG_IR_RX51) || defined(CONFIG_IR_RX51_MODULE)
 static struct lirc_rx51_platform_data rx51_lirc_data = {
-	.set_max_mpu_wakeup_lat = omap_pm_set_max_mpu_wakeup_lat,
 	.pwm_timer = 9, /* Use GPT 9 for CIR */
 };
 
diff --git a/drivers/media/rc/ir-rx51.c b/drivers/media/rc/ir-rx51.c
index 6e1ffa6..96ed23d 100644
--- a/drivers/media/rc/ir-rx51.c
+++ b/drivers/media/rc/ir-rx51.c
@@ -25,6 +25,7 @@
 #include <linux/platform_device.h>
 #include <linux/sched.h>
 #include <linux/wait.h>
+#include <linux/pm_qos.h>
 
 #include <plat/dmtimer.h>
 #include <plat/clock.h>
@@ -49,6 +50,7 @@ struct lirc_rx51 {
 	struct omap_dm_timer *pulse_timer;
 	struct device	     *dev;
 	struct lirc_rx51_platform_data *pdata;
+	struct pm_qos_request	pm_qos_request;
 	wait_queue_head_t     wqueue;
 
 	unsigned long	fclk_khz;
@@ -268,10 +270,16 @@ static ssize_t lirc_rx51_write(struct file *file, const char *buf,
 		lirc_rx51->wbuf[count] = -1; /* Insert termination mark */
 
 	/*
-	 * Adjust latency requirements so the device doesn't go in too
-	 * deep sleep states
+	 * If the MPU is going into too deep sleep state while we are
+	 * transmitting the IR code, timers will not be able to wake
+	 * up the MPU. Thus, we need to set a strict enough latency
+	 * requirement in order to ensure the interrupts come though
+	 * properly. The 10us latency requirement is low enough to
+	 * keep MPU from sleeping and thus ensures the interrupts are
+	 * getting through properly.
 	 */
-	lirc_rx51->pdata->set_max_mpu_wakeup_lat(lirc_rx51->dev, 50);
+	pm_qos_add_request(&lirc_rx51->pm_qos_request,
+			PM_QOS_CPU_DMA_LATENCY,	10);
 
 	lirc_rx51_on(lirc_rx51);
 	lirc_rx51->wbuf_index = 1;
@@ -292,8 +300,7 @@ static ssize_t lirc_rx51_write(struct file *file, const char *buf,
 	 */
 	lirc_rx51_stop_tx(lirc_rx51);
 
-	/* We can sleep again */
-	lirc_rx51->pdata->set_max_mpu_wakeup_lat(lirc_rx51->dev, -1);
+	pm_qos_remove_request(&lirc_rx51->pm_qos_request);
 
 	return n;
 }
diff --git a/include/media/ir-rx51.h b/include/media/ir-rx51.h
index 104aa89..57523f2 100644
--- a/include/media/ir-rx51.h
+++ b/include/media/ir-rx51.h
@@ -3,8 +3,6 @@
 
 struct lirc_rx51_platform_data {
 	int pwm_timer;
-
-	int(*set_max_mpu_wakeup_lat)(struct device *dev, long t);
 };
 
 #endif
-- 
1.7.12

[PATCHv3 8/9] ir-rx51: Remove useless variable from struct lirc_rx51

[Timo Kokkonen]

As clearly visible from the patch, this variable has no useful purpose
what so ever. Thus, it can be removed altogether without any side
effects.

Signed-off-by: Timo Kokkonen <timo.t.kokkonen@iki.fi>
---
 drivers/media/rc/ir-rx51.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/media/rc/ir-rx51.c b/drivers/media/rc/ir-rx51.c
index 96ed23d..edb1562 100644
--- a/drivers/media/rc/ir-rx51.c
+++ b/drivers/media/rc/ir-rx51.c
@@ -57,7 +57,6 @@ struct lirc_rx51 {
 	unsigned int	freq;		/* carrier frequency */
 	unsigned int	duty_cycle;	/* carrier duty cycle */
 	unsigned int	irq_num;
-	unsigned int	match;
 	int		wbuf[WBUF_LEN];
 	int		wbuf_index;
 	unsigned long	device_is_open;
@@ -102,8 +101,6 @@ static int init_timing_params(struct lirc_rx51 *lirc_rx51)
 	omap_dm_timer_set_int_enable(lirc_rx51->pulse_timer, 0);
 	omap_dm_timer_start(lirc_rx51->pulse_timer);
 
-	lirc_rx51->match = 0;
-
 	return 0;
 }
 
@@ -113,11 +110,7 @@ static int pulse_timer_set_timeout(struct lirc_rx51 *lirc_rx51, int usec)
 
 	BUG_ON(usec < 0);
 
-	if (lirc_rx51->match == 0)
-		counter = omap_dm_timer_read_counter(lirc_rx51->pulse_timer);
-	else
-		counter = lirc_rx51->match;
-
+	counter = omap_dm_timer_read_counter(lirc_rx51->pulse_timer);
 	counter += (u32)(lirc_rx51->fclk_khz * usec / (1000));
 	omap_dm_timer_set_match(lirc_rx51->pulse_timer, 1, counter);
 	omap_dm_timer_set_int_enable(lirc_rx51->pulse_timer,
-- 
1.7.12

[PATCHv3 9/9] ir-rx51: Add missing quote mark in Kconfig text

[Timo Kokkonen]

This trivial fix cures the following warning message:

drivers/media/rc/Kconfig:275:warning: multi-line strings not supported

Signed-off-by: Timo Kokkonen <timo.t.kokkonen@iki.fi>
---
 drivers/media/rc/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/rc/Kconfig b/drivers/media/rc/Kconfig
index 4a68014..1300655 100644
--- a/drivers/media/rc/Kconfig
+++ b/drivers/media/rc/Kconfig
@@ -272,7 +272,7 @@ config IR_IGUANA
 	   be called iguanair.
 
 config IR_RX51
-	tristate "Nokia N900 IR transmitter diode
+	tristate "Nokia N900 IR transmitter diode"
 	depends on OMAP_DM_TIMER && LIRC
 	---help---
 	   Say Y or M here if you want to enable support for the IR
-- 
1.7.12

Re: [PATCHv3 2/9] ir-rx51: Handle signals properly

[Sakari Ailus]

Moi,

On Thu, Aug 30, 2012 at 08:54:24PM +0300, Timo Kokkonen wrote:
> @@ -273,9 +281,18 @@ static ssize_t lirc_rx51_write(struct file *file, const char *buf,
>  
>  	/*
>  	 * Don't return back to the userspace until the transfer has
> -	 * finished
> +	 * finished. However, we wish to not spend any more than 500ms
> +	 * in kernel. No IR code TX should ever take that long.
> +	 */
> +	i = wait_event_timeout(lirc_rx51->wqueue, lirc_rx51->wbuf_index < 0,
> +			HZ / 2);

Why such an arbitrary timeout? In reality it might not bite the user space
in practice ever, but is it (and if so, why) really required in the first
place?

Cheers,

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCHv3 9/9] ir-rx51: Add missing quote mark in Kconfig text

[Sakari Ailus]

Moi,

On Thu, Aug 30, 2012 at 08:54:31PM +0300, Timo Kokkonen wrote:
> This trivial fix cures the following warning message:
> 
> drivers/media/rc/Kconfig:275:warning: multi-line strings not supported
> 
> Signed-off-by: Timo Kokkonen <timo.t.kokkonen@iki.fi>
> ---
>  drivers/media/rc/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/rc/Kconfig b/drivers/media/rc/Kconfig
> index 4a68014..1300655 100644
> --- a/drivers/media/rc/Kconfig
> +++ b/drivers/media/rc/Kconfig
> @@ -272,7 +272,7 @@ config IR_IGUANA
>  	   be called iguanair.
>  
>  config IR_RX51
> -	tristate "Nokia N900 IR transmitter diode
> +	tristate "Nokia N900 IR transmitter diode"
>  	depends on OMAP_DM_TIMER && LIRC
>  	---help---
>  	   Say Y or M here if you want to enable support for the IR

This should be combined with patch 1.

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCHv3 2/9] ir-rx51: Handle signals properly

[Timo Kokkonen]

Terve,

On 09/01/12 20:14, Sakari Ailus wrote:
> Moi,
> 
> On Thu, Aug 30, 2012 at 08:54:24PM +0300, Timo Kokkonen wrote:
>> @@ -273,9 +281,18 @@ static ssize_t lirc_rx51_write(struct file *file, const char *buf,
>>  
>>  	/*
>>  	 * Don't return back to the userspace until the transfer has
>> -	 * finished
>> +	 * finished. However, we wish to not spend any more than 500ms
>> +	 * in kernel. No IR code TX should ever take that long.
>> +	 */
>> +	i = wait_event_timeout(lirc_rx51->wqueue, lirc_rx51->wbuf_index < 0,
>> +			HZ / 2);
> 
> Why such an arbitrary timeout? In reality it might not bite the user space
> in practice ever, but is it (and if so, why) really required in the first
> place?

Well, I can think of two cases:

1) Something goes wrong. Such before I converted the patch to use the up
to date PM QoS implementation, the transmitting could take very long
time because the interrupts were not waking up the MPU. Now that this is
sorted out only unknown bugs can cause transmitting to hang indefinitely.

2) User is (intentionally?) doing something wrong. For example by
feeding in an IR code that has got very long pulses, he could end up
having the lircd process hung in kernel unkillable for long time. That
could be avoided quite easily by counting the pulse lengths and
rejecting any IR codes that are obviously too long. But since I'd like
to also protect against 1) case, I think this solution works just fine.

In the end, this is just safety measure that this driver behaves well.

-Timo

Re: [PATCHv3 9/9] ir-rx51: Add missing quote mark in Kconfig text

[Timo Kokkonen]

On 09/01/12 20:16, Sakari Ailus wrote:
> Moi,
> 
> On Thu, Aug 30, 2012 at 08:54:31PM +0300, Timo Kokkonen wrote:
>> This trivial fix cures the following warning message:
>>
>> drivers/media/rc/Kconfig:275:warning: multi-line strings not supported
>>
>> Signed-off-by: Timo Kokkonen <timo.t.kokkonen@iki.fi>
>> ---
>>  drivers/media/rc/Kconfig | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/rc/Kconfig b/drivers/media/rc/Kconfig
>> index 4a68014..1300655 100644
>> --- a/drivers/media/rc/Kconfig
>> +++ b/drivers/media/rc/Kconfig
>> @@ -272,7 +272,7 @@ config IR_IGUANA
>>  	   be called iguanair.
>>  
>>  config IR_RX51
>> -	tristate "Nokia N900 IR transmitter diode
>> +	tristate "Nokia N900 IR transmitter diode"
>>  	depends on OMAP_DM_TIMER && LIRC
>>  	---help---
>>  	   Say Y or M here if you want to enable support for the IR
> 
> This should be combined with patch 1.
> 

Actually I'd rather keep the patch 1 as is as it has already a purpose.
Instead, I'd squash this into patch 3 as it already touches the Kconfig
file and it has also the other trivial fixes combined in it.

-Timo

Re: [PATCHv3 2/9] ir-rx51: Handle signals properly

[Sakari Ailus]

Heippa,

Timo Kokkonen wrote:
> Terve,
>
> On 09/01/12 20:14, Sakari Ailus wrote:
>> Moi,
>>
>> On Thu, Aug 30, 2012 at 08:54:24PM +0300, Timo Kokkonen wrote:
>>> @@ -273,9 +281,18 @@ static ssize_t lirc_rx51_write(struct file *file, const char *buf,
>>>
>>>   	/*
>>>   	 * Don't return back to the userspace until the transfer has
>>> -	 * finished
>>> +	 * finished. However, we wish to not spend any more than 500ms
>>> +	 * in kernel. No IR code TX should ever take that long.
>>> +	 */
>>> +	i = wait_event_timeout(lirc_rx51->wqueue, lirc_rx51->wbuf_index < 0,
>>> +			HZ / 2);
>>
>> Why such an arbitrary timeout? In reality it might not bite the user space
>> in practice ever, but is it (and if so, why) really required in the first
>> place?
>
> Well, I can think of two cases:
>
> 1) Something goes wrong. Such before I converted the patch to use the up
> to date PM QoS implementation, the transmitting could take very long
> time because the interrupts were not waking up the MPU. Now that this is
> sorted out only unknown bugs can cause transmitting to hang indefinitely.
>
> 2) User is (intentionally?) doing something wrong. For example by
> feeding in an IR code that has got very long pulses, he could end up
> having the lircd process hung in kernel unkillable for long time. That
> could be avoided quite easily by counting the pulse lengths and
> rejecting any IR codes that are obviously too long. But since I'd like
> to also protect against 1) case, I think this solution works just fine.
>
> In the end, this is just safety measure that this driver behaves well.

In that case I think you should use wait_event_interruptible() instead. 
It's not the driver's job to decide what the user can do with the 
hardware and what not, is it?

Terveisin,

-- 
Sakari Ailus
sakari.ailus@iki.fi

Re: [PATCHv3 2/9] ir-rx51: Handle signals properly

[Timo Kokkonen]

On 09.02 2012 18:06:34, Sakari Ailus wrote:
> Heippa,
> 
> Timo Kokkonen wrote:
> > Terve,
> >
> > On 09/01/12 20:14, Sakari Ailus wrote:
> >> Moi,
> >>
> >> On Thu, Aug 30, 2012 at 08:54:24PM +0300, Timo Kokkonen wrote:
> >>> @@ -273,9 +281,18 @@ static ssize_t lirc_rx51_write(struct file *file, const char *buf,
> >>>
> >>>   	/*
> >>>   	 * Don't return back to the userspace until the transfer has
> >>> -	 * finished
> >>> +	 * finished. However, we wish to not spend any more than 500ms
> >>> +	 * in kernel. No IR code TX should ever take that long.
> >>> +	 */
> >>> +	i = wait_event_timeout(lirc_rx51->wqueue, lirc_rx51->wbuf_index < 0,
> >>> +			HZ / 2);
> >>
> >> Why such an arbitrary timeout? In reality it might not bite the user space
> >> in practice ever, but is it (and if so, why) really required in the first
> >> place?
> >
> > Well, I can think of two cases:
> >
> > 1) Something goes wrong. Such before I converted the patch to use the up
> > to date PM QoS implementation, the transmitting could take very long
> > time because the interrupts were not waking up the MPU. Now that this is
> > sorted out only unknown bugs can cause transmitting to hang indefinitely.
> >
> > 2) User is (intentionally?) doing something wrong. For example by
> > feeding in an IR code that has got very long pulses, he could end up
> > having the lircd process hung in kernel unkillable for long time. That
> > could be avoided quite easily by counting the pulse lengths and
> > rejecting any IR codes that are obviously too long. But since I'd like
> > to also protect against 1) case, I think this solution works just fine.
> >
> > In the end, this is just safety measure that this driver behaves well.
> 
> In that case I think you should use wait_event_interruptible() instead. 

Well, that's what I had there in the first place. With interruptible
wait we are left with problem with signals. I was told by Sean Young
that the lirc API expects the write call to finish only after the IR
code is transmitted.

> It's not the driver's job to decide what the user can do with the 
> hardware and what not, is it?

Yeah, policy should be decided by the user space. However, kernel
should not leave any objvious denial of service holes open
either. Allowing a process to get stuck unkillable within kernel for
long time sounds like one to me.

Anyway, we are trying to cover some rare corner cases here, I'm not
sure how it should work exactly..

-Timo

> 
> Terveisin,
> 
> -- 
> Sakari Ailus
> sakari.ailus@iki.fi
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 2/9] ir-rx51: Handle signals properly

[Sakari Ailus]

On Sun, Sep 02, 2012 at 06:20:27PM +0300, Timo Kokkonen wrote:
> On 09.02 2012 18:06:34, Sakari Ailus wrote:
> > Heippa,
> > 
> > Timo Kokkonen wrote:
> > > Terve,
> > >
> > > On 09/01/12 20:14, Sakari Ailus wrote:
> > >> Moi,
> > >>
> > >> On Thu, Aug 30, 2012 at 08:54:24PM +0300, Timo Kokkonen wrote:
> > >>> @@ -273,9 +281,18 @@ static ssize_t lirc_rx51_write(struct file *file, const char *buf,
> > >>>
> > >>>   	/*
> > >>>   	 * Don't return back to the userspace until the transfer has
> > >>> -	 * finished
> > >>> +	 * finished. However, we wish to not spend any more than 500ms
> > >>> +	 * in kernel. No IR code TX should ever take that long.
> > >>> +	 */
> > >>> +	i = wait_event_timeout(lirc_rx51->wqueue, lirc_rx51->wbuf_index < 0,
> > >>> +			HZ / 2);
> > >>
> > >> Why such an arbitrary timeout? In reality it might not bite the user space
> > >> in practice ever, but is it (and if so, why) really required in the first
> > >> place?
> > >
> > > Well, I can think of two cases:
> > >
> > > 1) Something goes wrong. Such before I converted the patch to use the up
> > > to date PM QoS implementation, the transmitting could take very long
> > > time because the interrupts were not waking up the MPU. Now that this is
> > > sorted out only unknown bugs can cause transmitting to hang indefinitely.
> > >
> > > 2) User is (intentionally?) doing something wrong. For example by
> > > feeding in an IR code that has got very long pulses, he could end up
> > > having the lircd process hung in kernel unkillable for long time. That
> > > could be avoided quite easily by counting the pulse lengths and
> > > rejecting any IR codes that are obviously too long. But since I'd like
> > > to also protect against 1) case, I think this solution works just fine.
> > >
> > > In the end, this is just safety measure that this driver behaves well.
> > 
> > In that case I think you should use wait_event_interruptible() instead. 
> 
> Well, that's what I had there in the first place. With interruptible
> wait we are left with problem with signals. I was told by Sean Young
> that the lirc API expects the write call to finish only after the IR
> code is transmitted.
> 
> > It's not the driver's job to decide what the user can do with the 
> > hardware and what not, is it?
> 
> Yeah, policy should be decided by the user space. However, kernel
> should not leave any objvious denial of service holes open
> either. Allowing a process to get stuck unkillable within kernel for
> long time sounds like one to me.

It's interruptible, so the user space can interrupt that wait if it chooses
so. Besides, if you call this denial of service, then capturing video on
V4L2 is, too, since others can't use the device in the meantime. :-)

> Anyway, we are trying to cover some rare corner cases here, I'm not
> sure how it should work exactly..

If there was a generic maximum timeout for sending a code, wouldn't it make
sense to enforce that in the LIRC framework instead?

Terveisin,

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCHv3 9/9] ir-rx51: Add missing quote mark in Kconfig text

[Sakari Ailus]

On Sun, Sep 02, 2012 at 05:57:25PM +0300, Timo Kokkonen wrote:
> On 09/01/12 20:16, Sakari Ailus wrote:
> > Moi,
> > 
> > On Thu, Aug 30, 2012 at 08:54:31PM +0300, Timo Kokkonen wrote:
> >> This trivial fix cures the following warning message:
> >>
> >> drivers/media/rc/Kconfig:275:warning: multi-line strings not supported
> >>
> >> Signed-off-by: Timo Kokkonen <timo.t.kokkonen@iki.fi>
> >> ---
> >>  drivers/media/rc/Kconfig | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/media/rc/Kconfig b/drivers/media/rc/Kconfig
> >> index 4a68014..1300655 100644
> >> --- a/drivers/media/rc/Kconfig
> >> +++ b/drivers/media/rc/Kconfig
> >> @@ -272,7 +272,7 @@ config IR_IGUANA
> >>  	   be called iguanair.
> >>  
> >>  config IR_RX51
> >> -	tristate "Nokia N900 IR transmitter diode
> >> +	tristate "Nokia N900 IR transmitter diode"
> >>  	depends on OMAP_DM_TIMER && LIRC
> >>  	---help---
> >>  	   Say Y or M here if you want to enable support for the IR
> > 
> > This should be combined with patch 1.
> > 
> 
> Actually I'd rather keep the patch 1 as is as it has already a purpose.
> Instead, I'd squash this into patch 3 as it already touches the Kconfig
> file and it has also the other trivial fixes combined in it.

Sounds good, that's actually a better place for it.

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCHv3 2/9] ir-rx51: Handle signals properly

[Timo Kokkonen]

On 09/02/12 22:41, Sakari Ailus wrote:
> On Sun, Sep 02, 2012 at 06:20:27PM +0300, Timo Kokkonen wrote:
>> On 09.02 2012 18:06:34, Sakari Ailus wrote:
>>> Heippa,
>>>
>>> Timo Kokkonen wrote:
>>>> Terve,
>>>>
>>>> On 09/01/12 20:14, Sakari Ailus wrote:
>>>>> Moi,
>>>>>
>>>>> On Thu, Aug 30, 2012 at 08:54:24PM +0300, Timo Kokkonen wrote:
>>>>>> @@ -273,9 +281,18 @@ static ssize_t lirc_rx51_write(struct file *file, const char *buf,
>>>>>>
>>>>>>   	/*
>>>>>>   	 * Don't return back to the userspace until the transfer has
>>>>>> -	 * finished
>>>>>> +	 * finished. However, we wish to not spend any more than 500ms
>>>>>> +	 * in kernel. No IR code TX should ever take that long.
>>>>>> +	 */
>>>>>> +	i = wait_event_timeout(lirc_rx51->wqueue, lirc_rx51->wbuf_index < 0,
>>>>>> +			HZ / 2);
>>>>>
>>>>> Why such an arbitrary timeout? In reality it might not bite the user space
>>>>> in practice ever, but is it (and if so, why) really required in the first
>>>>> place?
>>>>
>>>> Well, I can think of two cases:
>>>>
>>>> 1) Something goes wrong. Such before I converted the patch to use the up
>>>> to date PM QoS implementation, the transmitting could take very long
>>>> time because the interrupts were not waking up the MPU. Now that this is
>>>> sorted out only unknown bugs can cause transmitting to hang indefinitely.
>>>>
>>>> 2) User is (intentionally?) doing something wrong. For example by
>>>> feeding in an IR code that has got very long pulses, he could end up
>>>> having the lircd process hung in kernel unkillable for long time. That
>>>> could be avoided quite easily by counting the pulse lengths and
>>>> rejecting any IR codes that are obviously too long. But since I'd like
>>>> to also protect against 1) case, I think this solution works just fine.
>>>>
>>>> In the end, this is just safety measure that this driver behaves well.
>>>
>>> In that case I think you should use wait_event_interruptible() instead. 
>>
>> Well, that's what I had there in the first place. With interruptible
>> wait we are left with problem with signals. I was told by Sean Young
>> that the lirc API expects the write call to finish only after the IR
>> code is transmitted.
>>
>>> It's not the driver's job to decide what the user can do with the 
>>> hardware and what not, is it?
>>
>> Yeah, policy should be decided by the user space. However, kernel
>> should not leave any objvious denial of service holes open
>> either. Allowing a process to get stuck unkillable within kernel for
>> long time sounds like one to me.
> 
> It's interruptible, so the user space can interrupt that wait if it chooses
> so. Besides, if you call this denial of service, then capturing video on
> V4L2 is, too, since others can't use the device in the meantime. :-)
> 

Well, of course there is no problem if we use interruptible waits. But I
was told by Sean that the lirc API expects the IR TX to be finished
always when the write call returns. I guess the assumption is to avoid
breaking the transmission in the middle in case the process is signaled.
And that's why we shouldn't use interruptible waits.

However, if we allow simply breaking the transmitting in case the
process is signaled any way during the transmission, then the handling
would be trivial in the driver. That is, if someone for example kills or
stops the lirc daemon process, then the IR code just wouldn't finish ever.

Sean, do you have an opinion how this should or is allowed to work?

>> Anyway, we are trying to cover some rare corner cases here, I'm not
>> sure how it should work exactly..
> 
> If there was a generic maximum timeout for sending a code, wouldn't it make
> sense to enforce that in the LIRC framework instead?
> 

Yes, I agree it makes sense to leave unrestricted. But in that case we
definitely have to use interruptible waits in case user space is doing
something stupid and regrets it later :)

> Terveisin,
> 

Re: [PATCHv3 2/9] ir-rx51: Handle signals properly

[Sean Young]

On Sun, Sep 02, 2012 at 11:08:20PM +0300, Timo Kokkonen wrote:
> On 09/02/12 22:41, Sakari Ailus wrote:
> > On Sun, Sep 02, 2012 at 06:20:27PM +0300, Timo Kokkonen wrote:
> >> On 09.02 2012 18:06:34, Sakari Ailus wrote:
> >>> Heippa,
> >>>
> >>> Timo Kokkonen wrote:
> >>>> Terve,
> >>>>
> >>>> On 09/01/12 20:14, Sakari Ailus wrote:
> >>>>> Moi,
> >>>>>
> >>>>> On Thu, Aug 30, 2012 at 08:54:24PM +0300, Timo Kokkonen wrote:
> >>>>>> @@ -273,9 +281,18 @@ static ssize_t lirc_rx51_write(struct file *file, const char *buf,
> >>>>>>
> >>>>>>   	/*
> >>>>>>   	 * Don't return back to the userspace until the transfer has
> >>>>>> -	 * finished
> >>>>>> +	 * finished. However, we wish to not spend any more than 500ms
> >>>>>> +	 * in kernel. No IR code TX should ever take that long.
> >>>>>> +	 */
> >>>>>> +	i = wait_event_timeout(lirc_rx51->wqueue, lirc_rx51->wbuf_index < 0,
> >>>>>> +			HZ / 2);
> >>>>>
> >>>>> Why such an arbitrary timeout? In reality it might not bite the user space
> >>>>> in practice ever, but is it (and if so, why) really required in the first
> >>>>> place?
> >>>>
> >>>> Well, I can think of two cases:
> >>>>
> >>>> 1) Something goes wrong. Such before I converted the patch to use the up
> >>>> to date PM QoS implementation, the transmitting could take very long
> >>>> time because the interrupts were not waking up the MPU. Now that this is
> >>>> sorted out only unknown bugs can cause transmitting to hang indefinitely.
> >>>>
> >>>> 2) User is (intentionally?) doing something wrong. For example by
> >>>> feeding in an IR code that has got very long pulses, he could end up
> >>>> having the lircd process hung in kernel unkillable for long time. That
> >>>> could be avoided quite easily by counting the pulse lengths and
> >>>> rejecting any IR codes that are obviously too long. But since I'd like
> >>>> to also protect against 1) case, I think this solution works just fine.
> >>>>
> >>>> In the end, this is just safety measure that this driver behaves well.
> >>>
> >>> In that case I think you should use wait_event_interruptible() instead. 
> >>
> >> Well, that's what I had there in the first place. With interruptible
> >> wait we are left with problem with signals. I was told by Sean Young
> >> that the lirc API expects the write call to finish only after the IR
> >> code is transmitted.
> >>
> >>> It's not the driver's job to decide what the user can do with the 
> >>> hardware and what not, is it?
> >>
> >> Yeah, policy should be decided by the user space. However, kernel
> >> should not leave any objvious denial of service holes open
> >> either. Allowing a process to get stuck unkillable within kernel for
> >> long time sounds like one to me.

It's not elegant, but this can't be used as a denial of service attack.
The driver waits for a maximum of a half a second after which signals
are serviced as normal.

> > It's interruptible, so the user space can interrupt that wait if it chooses
> > so. Besides, if you call this denial of service, then capturing video on
> > V4L2 is, too, since others can't use the device in the meantime. :-)
> > 
> 
> Well, of course there is no problem if we use interruptible waits. But I
> was told by Sean that the lirc API expects the IR TX to be finished
> always when the write call returns.

This is part of the ABI. The lircd deamon might want to do gap calculation
if there are large spaces in the IR code being sent. Maybe others can
enlighten us why such an ABI was choosen.

> I guess the assumption is to avoid
> breaking the transmission in the middle in case the process is signaled.
> And that's why we shouldn't use interruptible waits.
>
> However, if we allow simply breaking the transmitting in case the
> process is signaled any way during the transmission, then the handling
> would be trivial in the driver. That is, if someone for example kills or
> stops the lirc daemon process, then the IR code just wouldn't finish ever.
> 
> Sean, do you have an opinion how this should or is allowed to work?

You want to know when the hardware is done sending the IR. If you return
EINTR to user space, how would user space know how much IR has been sent, 
if any?

This ABI is not particularily elegant so there are proposals for a better
interface which would obsolete the lirc interface. David Hardeman has
worked on this:

http://patchwork.linuxtv.org/patch/11411/

> >> Anyway, we are trying to cover some rare corner cases here, I'm not
> >> sure how it should work exactly..
> > 
> > If there was a generic maximum timeout for sending a code, wouldn't it make
> > sense to enforce that in the LIRC framework instead?
> > 
> 
> Yes, I agree it makes sense to leave unrestricted. But in that case we
> definitely have to use interruptible waits in case user space is doing
> something stupid and regrets it later :)

Only for 500ms. 


Sean

Re: [PATCHv3 2/9] ir-rx51: Handle signals properly

[David Härdeman]

Hej,

On Mon, Sep 03, 2012 at 01:36:53PM +0100, Sean Young wrote:
>On Sun, Sep 02, 2012 at 11:08:20PM +0300, Timo Kokkonen wrote:
>> I guess the assumption is to avoid
>> breaking the transmission in the middle in case the process is signaled.
>> And that's why we shouldn't use interruptible waits.
>>
>> However, if we allow simply breaking the transmitting in case the
>> process is signaled any way during the transmission, then the handling
>> would be trivial in the driver. That is, if someone for example kills or
>> stops the lirc daemon process, then the IR code just wouldn't finish ever.
>> 
>> Sean, do you have an opinion how this should or is allowed to work?
>
>You want to know when the hardware is done sending the IR. If you return
>EINTR to user space, how would user space know how much IR has been sent, 
>if any?
>
>This ABI is not particularily elegant so there are proposals for a better
>interface which would obsolete the lirc interface. David Hardeman has
>worked on this:
>
>http://patchwork.linuxtv.org/patch/11411/
>

Yes, the first step is an asynchronous interface using a kfifo which is
managed/fed using functionality in rc-core and drained by the drivers.

The size of the kfifo() itself is the only limiting factor right now,
but I do think we should eventually add some restrictions on the combined
duration of the pulse/space timings that are in the queue at any given
point.

Say, for example, that any given pulse/space value is not allowed to be
above 500ms and the total duration of the queue is not allowed to be
above 1000ms. In case user-space wants (for whatever reason)...to write
a 4000ms space, it would have to do so in 8 messages of 500ms each.

Each message write() provides the opportunity for a interruptible wait
(in the regular case) or returning EAGAIN (in the O_NONBLOCK case) -
assuming that the kfifo already holds pulse/space timing totalling
1000ms and/or is full.

EINTR should only be returned if nothing has been written to the kfifo
at all.

That way we would avoid policy in kernel while still making it possible
to kill a misbehaving user-space process by forcing it to drip feed long
TX sequences.

-- 
David Härdeman

Re: [PATCHv3 2/9] ir-rx51: Handle signals properly

[Timo Kokkonen]

On 09/03/12 15:36, Sean Young wrote:
> On Sun, Sep 02, 2012 at 11:08:20PM +0300, Timo Kokkonen wrote:
>> On 09/02/12 22:41, Sakari Ailus wrote:
>>> On Sun, Sep 02, 2012 at 06:20:27PM +0300, Timo Kokkonen wrote:
>>>> On 09.02 2012 18:06:34, Sakari Ailus wrote:
>>>>> Heippa,
>>>>>
>>>>> Timo Kokkonen wrote:
>>>>>> Terve,
>>>>>>
>>>>>> On 09/01/12 20:14, Sakari Ailus wrote:
>>>>>>> Moi,
>>>>>>>
>>>>>>> On Thu, Aug 30, 2012 at 08:54:24PM +0300, Timo Kokkonen wrote:
>>>>>>>> @@ -273,9 +281,18 @@ static ssize_t lirc_rx51_write(struct file *file, const char *buf,
>>>>>>>>
>>>>>>>>   	/*
>>>>>>>>   	 * Don't return back to the userspace until the transfer has
>>>>>>>> -	 * finished
>>>>>>>> +	 * finished. However, we wish to not spend any more than 500ms
>>>>>>>> +	 * in kernel. No IR code TX should ever take that long.
>>>>>>>> +	 */
>>>>>>>> +	i = wait_event_timeout(lirc_rx51->wqueue, lirc_rx51->wbuf_index < 0,
>>>>>>>> +			HZ / 2);
>>>>>>>
>>>>>>> Why such an arbitrary timeout? In reality it might not bite the user space
>>>>>>> in practice ever, but is it (and if so, why) really required in the first
>>>>>>> place?
>>>>>>
>>>>>> Well, I can think of two cases:
>>>>>>
>>>>>> 1) Something goes wrong. Such before I converted the patch to use the up
>>>>>> to date PM QoS implementation, the transmitting could take very long
>>>>>> time because the interrupts were not waking up the MPU. Now that this is
>>>>>> sorted out only unknown bugs can cause transmitting to hang indefinitely.
>>>>>>
>>>>>> 2) User is (intentionally?) doing something wrong. For example by
>>>>>> feeding in an IR code that has got very long pulses, he could end up
>>>>>> having the lircd process hung in kernel unkillable for long time. That
>>>>>> could be avoided quite easily by counting the pulse lengths and
>>>>>> rejecting any IR codes that are obviously too long. But since I'd like
>>>>>> to also protect against 1) case, I think this solution works just fine.
>>>>>>
>>>>>> In the end, this is just safety measure that this driver behaves well.
>>>>>
>>>>> In that case I think you should use wait_event_interruptible() instead. 
>>>>
>>>> Well, that's what I had there in the first place. With interruptible
>>>> wait we are left with problem with signals. I was told by Sean Young
>>>> that the lirc API expects the write call to finish only after the IR
>>>> code is transmitted.
>>>>
>>>>> It's not the driver's job to decide what the user can do with the 
>>>>> hardware and what not, is it?
>>>>
>>>> Yeah, policy should be decided by the user space. However, kernel
>>>> should not leave any objvious denial of service holes open
>>>> either. Allowing a process to get stuck unkillable within kernel for
>>>> long time sounds like one to me.
> 
> It's not elegant, but this can't be used as a denial of service attack.
> The driver waits for a maximum of a half a second after which signals
> are serviced as normal.
> 
>>> It's interruptible, so the user space can interrupt that wait if it chooses
>>> so. Besides, if you call this denial of service, then capturing video on
>>> V4L2 is, too, since others can't use the device in the meantime. :-)
>>>
>>
>> Well, of course there is no problem if we use interruptible waits. But I
>> was told by Sean that the lirc API expects the IR TX to be finished
>> always when the write call returns.
> 
> This is part of the ABI. The lircd deamon might want to do gap calculation
> if there are large spaces in the IR code being sent. Maybe others can
> enlighten us why such an ABI was choosen.
> 
>> I guess the assumption is to avoid
>> breaking the transmission in the middle in case the process is signaled.
>> And that's why we shouldn't use interruptible waits.
>>
>> However, if we allow simply breaking the transmitting in case the
>> process is signaled any way during the transmission, then the handling
>> would be trivial in the driver. That is, if someone for example kills or
>> stops the lirc daemon process, then the IR code just wouldn't finish ever.
>>
>> Sean, do you have an opinion how this should or is allowed to work?
> 
> You want to know when the hardware is done sending the IR. If you return
> EINTR to user space, how would user space know how much IR has been sent, 
> if any?
> 
> This ABI is not particularily elegant so there are proposals for a better
> interface which would obsolete the lirc interface. David Hardeman has
> worked on this:
> 
> http://patchwork.linuxtv.org/patch/11411/
> 

It appears that all "modern" lirc drivers are now using the rc-core
functionalities to implement the common stuff. When the rx51 lirc driver
was first written, the core was not in place yet. Therefore it is
implementing the file operations in the driver, which other rc drivers
won't do today.

So, I think it would make sense to modify the rx51 driver to use the rc
core functionality. But if there is an ABI change ongoing, I could wait
until you have that done before I start working on the change?

Considering this patch set, I think it makes sense still to apply these
as they improve the existing code base. I'll just squash the one patch
to the misc fixes, as pointed by Sakari, and then re-send the set.

-Timo

>>>> Anyway, we are trying to cover some rare corner cases here, I'm not
>>>> sure how it should work exactly..
>>>
>>> If there was a generic maximum timeout for sending a code, wouldn't it make
>>> sense to enforce that in the LIRC framework instead?
>>>
>>
>> Yes, I agree it makes sense to leave unrestricted. But in that case we
>> definitely have to use interruptible waits in case user space is doing
>> something stupid and regrets it later :)
> 
> Only for 500ms. 
> 
> 
> Sean
> 

Re: [PATCHv3 2/9] ir-rx51: Handle signals properly

[Sean Young]

On Fri, Sep 14, 2012 at 10:58:53AM +0300, Timo Kokkonen wrote:
> It appears that all "modern" lirc drivers are now using the rc-core
> functionalities to implement the common stuff. When the rx51 lirc driver
> was first written, the core was not in place yet. Therefore it is
> implementing the file operations in the driver, which other rc drivers
> won't do today.
> 
> So, I think it would make sense to modify the rx51 driver to use the rc
> core functionality. But if there is an ABI change ongoing, I could wait
> until you have that done before I start working on the change?

There is no immediate need for porting to rc-core, AFAIK. OTOH I suspect 
that only some of the drivers using rc-core will only need to have their 
tx_ir method modified for a new sending/receiving ABI, so it shouldn't
stop you. If anything it might make the driver smaller.

At the moment I'm only just put initial patches together so I don't know 
when I or anyone else will have this finished. 


Sean