Re: [PATCH 6/8] Char: rtc, use wait_event_interruptible

[Andrew Morton]

On Mon, 28 May 2007 15:29:50 +0200 (CEST)
Jiri Slaby <jirislaby@xxxxxxxxx> wrote:

> rtc, use wait_event_interruptible
> 
> Signed-off-by: Jiri Slaby <jirislaby@xxxxxxxxx>
> 
> ---
> commit 62bd843054a7b2b2a3e3003a6b57b6359f199837
> tree 7c6912d4ed42581ca46d384ab2467bfb9d9d9dbe
> parent 2b7813d0f34703e0a4b18593a3186bdc7e719c06
> author Jiri Slaby <jirislaby@xxxxxxxxx> Sat, 26 May 2007 21:58:54 +0200
> committer Jiri Slaby <jirislaby@xxxxxxxxx> Sat, 26 May 2007 21:58:54 +0200
> 
>  drivers/char/rtc.c |   40 +++++++++++++++-------------------------
>  1 files changed, 15 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/char/rtc.c b/drivers/char/rtc.c
> index 20380a2..787b520 100644
> --- a/drivers/char/rtc.c
> +++ b/drivers/char/rtc.c
> @@ -353,33 +353,26 @@ static ssize_t rtc_read(struct file *file, char __user *buf,
>  	if (count != sizeof(unsigned int) && count !=  sizeof(unsigned long))
>  		return -EINVAL;
>  
> -	add_wait_queue(&rtc_wait, &wait);
> -
> -	do {
> -		/* First make it right. Then make it fast. Putting this whole
> -		 * block within the parentheses of a while would be too
> -		 * confusing. And no, xchg() is not the answer. */
> -
> -		__set_current_state(TASK_INTERRUPTIBLE);
> -		
> -		spin_lock_irq (&rtc_lock);
> -		data = rtc_irq_data;
> -		rtc_irq_data = 0;
> -		spin_unlock_irq (&rtc_lock);
> -
> -		if (data != 0)
> -			break;
> +	spin_lock_irq (&rtc_lock);
> +	data = rtc_irq_data;
> +	rtc_irq_data = 0;
> +	spin_unlock_irq (&rtc_lock);
>  
> +	if (data == 0) {
>  		if (file->f_flags & O_NONBLOCK) {
>  			retval = -EAGAIN;
>  			goto out;
>  		}
> -		if (signal_pending(current)) {
> -			retval = -ERESTARTSYS;
> +		retval = wait_event_interruptible(rtc_wait, ({
> +			spin_lock_irq (&rtc_lock);
> +			data = rtc_irq_data;
> +			rtc_irq_data = 0;
> +			spin_unlock_irq (&rtc_lock);
> +			data;
> +		}));
> +		if (retval)
>  			goto out;
> -		}
> -		schedule();
> -	} while (1);
> +	}
>  
>  	if (count == sizeof(unsigned int))
>  		retval = put_user(data, (unsigned int __user *)buf) ?: sizeof(int);
> @@ -387,10 +380,7 @@ static ssize_t rtc_read(struct file *file, char __user *buf,
>  		retval = put_user(data, (unsigned long __user *)buf) ?: sizeof(long);
>  	if (!retval)
>  		retval = count;
> - out:
> -	__set_current_state(TASK_RUNNING);
> -	remove_wait_queue(&rtc_wait, &wait);
> -
> +out:
>  	return retval;
>  #endif

erm, I'm not sure that really improved the code.

Note that wait_event_interruptible() evaluates the condition twice, so
that's now three copies we have in there.  A little helper function would
make this all much better.

Also, feel free to sneak in a s/spin_lock_irq (/spin_lock_irq(/ cleanup
when making changes like this.

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/