Re: [PATCH 2/8] ir-rx51: Handle signals properly

[Timo Kokkonen <timo.t.kokkonen@iki.fi>]

On 08/24/12 13:03, Sean Young wrote:
> On Wed, Aug 22, 2012 at 10:50:35PM +0300, Timo Kokkonen wrote:
>> 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..a7b787a 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_interruptible(&lirc_rx51->wqueue);
> 
> Unless I'm mistaken, wait_up_interruptable() won't wake up any 
> non-interruptable sleepers. Should this not be wake_up()?
> 

Thanks for pointing out that. I guess I never noticed that because I'm
using a timeout with the wait, so it will wake up anyway. I'll fix it.

-Timo

>> +}
>> +
>>  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,
> 
> Note non-interruptable sleeper.
> 
>> +			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
>>
>> --
>> 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
> --
> 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
>