Re: uio drivers with IRQF_NO_THREAD on preempt-rt kernel

[Matthias Fuchs <mfuchs@ma-fu.de>]

Hi Sebastian,

On 29.05.2018 18:51, Sebastian Andrzej Siewior wrote:
> On 2018-05-28 22:26:55 [+0200], Matthias Fuchs wrote:
>> Hi,
>>
>> I updated my modified uio.c code using simple wake queues. See below.
>> Blocking read on the uio device is fine. But select() with timeout
>> behaves a little strange. I am still digging to find out what happens,
>> but it seems that even I should never run into a timeout in my test application,
>> the event_count of two consecutive select()/read() pairs is not advanced by one.
>>
>> So is my implementation correct? Does using the normal waitqueue in this
>> manner satisfy uio_poll(). So in my case irq_flags has IRQF_NO_THREAD always set. This means
>> idev->wait never gets a wake_up_interruptible().
>>
>> diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
>> index bcc1fc0..779dcaf 100644
>> --- a/drivers/uio/uio.c
>> +++ b/drivers/uio/uio.c
>> @@ -25,6 +25,7 @@
>>   #include <linux/kobject.h>
>>   #include <linux/cdev.h>
>>   #include <linux/uio_driver.h>
>> +#include <linux/swait.h>
>>   
>>   #define UIO_MAX_DEVICES		(1U << MINORBITS)
>>   
>> @@ -394,8 +395,12 @@ void uio_event_notify(struct uio_info *info)
>>   	struct uio_device *idev = info->uio_dev;
>>   
>>   	atomic_inc(&idev->event);
>> -	wake_up_interruptible(&idev->wait);
>> -	kill_fasync(&idev->async_queue, SIGIO, POLL_IN);
>> +	if (idev->info->irq_flags & IRQF_NO_THREAD) {
>> +		swake_up_locked(&idev->swait);
> 
> you want swake_up().
> 
>> +	} else {
>> +		wake_up_interruptible(&idev->wait);
>> +		kill_fasync(&idev->async_queue, SIGIO, POLL_IN);
> 
> also you need do this if you have someone is in poll(). You could the
> upper part in the primary handler this in the threaded handler.

uio.c only has a single handler that calls uio_event_notify(). That 
handler is typically threaded and in my case not (uio driver passes 
IRQF_NO_THREAD). So what threaded handler do you mean? Do you mean from 
uio_read()? Does this work?

My itention is to use select() (aka poll) on the uio driver from my RT 
application and have no irq thread(). I do not want to implement a 
separate timeout mechanism on read().

I've seen this requirement in other places before.

Matthias

> 
>> +	}
>>   }
>>   EXPORT_SYMBOL_GPL(uio_event_notify);
>>   
>> @@ -508,6 +513,7 @@ static ssize_t uio_read(struct file *filep, char __user *buf,
>>   	struct uio_listener *listener = filep->private_data;
>>   	struct uio_device *idev = listener->dev;
>>   	DECLARE_WAITQUEUE(wait, current);
>> +	DECLARE_SWAITQUEUE(swait);
>>   	ssize_t retval;
>>   	s32 event_count;
>>   
>> @@ -520,11 +526,10 @@ static ssize_t uio_read(struct file *filep, char __user *buf,
>>   	add_wait_queue(&idev->wait, &wait);
>>   
>>   	do {
>> -		set_current_state(TASK_INTERRUPTIBLE);
>> +		prepare_to_swait(&idev->swait, &swait, TASK_INTERRUPTIBLE);
>>   
>>   		event_count = atomic_read(&idev->event);
>>   		if (event_count != listener->event_count) {
>> -			__set_current_state(TASK_RUNNING);
>>   			if (copy_to_user(buf, &event_count, count))
>>   				retval = -EFAULT;
>>   			else {
>> @@ -546,7 +551,7 @@ static ssize_t uio_read(struct file *filep, char __user *buf,
>>   		schedule();
>>   	} while (1);
>>   
>> -	__set_current_state(TASK_RUNNING);
>> +	finish_swait(&idev->swait, &swait);
>>   	remove_wait_queue(&idev->wait, &wait);
> 
> and ->wait isn't used in ->read() anymore, right? Just in ->poll(). If
> so it could go.
> 
>>   	return retval;
>> @@ -814,6 +819,7 @@ int __uio_register_device(struct module *owner,
>>   	idev->owner = owner;
>>   	idev->info = info;
>>   	init_waitqueue_head(&idev->wait);
>> +	init_swait_queue_head(&idev->swait);
>>   	atomic_set(&idev->event, 0);
>>   
>>   	ret = uio_get_minor(idev);
>>
>>
>> Cheers,
>> Matthias
> 
> Sebastian
> 
Matthias