[PATCH] Add uio_sync_event interface to support mulit-instances case in UIO framework

[PATCH] Add uio_sync_event interface to support mulit-instances case in UIO framework

[Xiaolong Ye]

Hi, guys,

Recently, we have met fake interrupt issue when using UIO as our vpu driver in mulit-instances case,
the issue can be described as below:

In multi-instances case, we use vpu_lock(semaphore) to implement the mutually exclusive access to the
device, each instace open uio device once and will be associated with a fd and its own uio_listener,
we use poll to wait for the hardware interrupt. So let's assume that there are two instances, A and B,
their uio_listener->event_count and idev->event are all 0 at the beginning, then A get the vpu_lock and start
to work, it will block at poll()(because listener->event_count == idev->count) until H/W interrupt happens
(atomic_inc(&idev->event)), after it releases vpu_lock, B will get the lock and start to work, however,
its poll will return immediately because B's uio_listener->event_count is 0 while idev->event has been 1,
that's how the fake interrupt happens.

Our proposal:
Add new uio_sync_event(struct uio_listener *listener) function in uio,c, so that we can call it directly
when we want to sync the event count.


Xiaolong Ye (1):
  uio: add uio_event_sync interface

 drivers/uio/uio.c          |   12 ++++++++++++
 include/linux/uio_driver.h |    1 +
 2 files changed, 13 insertions(+)

-- 
1.7.9.5

[PATCH] uio: add uio_event_sync interface

[Xiaolong Ye]

This interface is called to sync listener->event_count and device
event count in multi-instances case.

Change-Id: Ibb1e4888ce55b4993394b61e4bcd6dce8b8291f0
Signed-off-by: Xiaolong Ye <yexl@marvell.com>
---
 drivers/uio/uio.c          |   12 ++++++++++++
 include/linux/uio_driver.h |    1 +
 2 files changed, 13 insertions(+)

diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
index ba47563..95d559f 100644
--- a/drivers/uio/uio.c
+++ b/drivers/uio/uio.c
@@ -408,6 +408,18 @@ void uio_event_notify(struct uio_info *info)
 EXPORT_SYMBOL_GPL(uio_event_notify);
 
 /**
+ * uio_event_sync - sync listener's event count with UIO device
+ * @listener: uio_listener structure
+ */
+void uio_event_sync(struct uio_listener *listener)
+{
+	struct uio_device *idev = listener->dev;
+
+	listener->event_count = atomic_read(&idev->event);
+}
+EXPORT_SYMBOL_GPL(uio_event_sync);
+
+/**
  * uio_interrupt - hardware interrupt handler
  * @irq: IRQ number, can be UIO_IRQ_CYCLIC for cyclic timer
  * @dev_id: Pointer to the devices uio_device structure
diff --git a/include/linux/uio_driver.h b/include/linux/uio_driver.h
index 1ad4724..d10e634 100644
--- a/include/linux/uio_driver.h
+++ b/include/linux/uio_driver.h
@@ -108,6 +108,7 @@ extern int __must_check
 
 extern void uio_unregister_device(struct uio_info *info);
 extern void uio_event_notify(struct uio_info *info);
+extern void uio_event_sync(struct uio_listener *listener);
 
 /* defines for uio_info->irq */
 #define UIO_IRQ_CUSTOM	-1
-- 
1.7.9.5

Re: [PATCH] uio: add uio_event_sync interface

[Greg KH]

On Wed, Aug 14, 2013 at 08:30:23PM +0800, Xiaolong Ye wrote:
> This interface is called to sync listener->event_count and device
> event count in multi-instances case.
> 
> Change-Id: Ibb1e4888ce55b4993394b61e4bcd6dce8b8291f0

What is this for?  (hint, it should never be in a kernel patch...)

> Signed-off-by: Xiaolong Ye <yexl@marvell.com>
> ---
>  drivers/uio/uio.c          |   12 ++++++++++++
>  include/linux/uio_driver.h |    1 +
>  2 files changed, 13 insertions(+)
> 
> diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
> index ba47563..95d559f 100644
> --- a/drivers/uio/uio.c
> +++ b/drivers/uio/uio.c
> @@ -408,6 +408,18 @@ void uio_event_notify(struct uio_info *info)
>  EXPORT_SYMBOL_GPL(uio_event_notify);
>  
>  /**
> + * uio_event_sync - sync listener's event count with UIO device
> + * @listener: uio_listener structure
> + */
> +void uio_event_sync(struct uio_listener *listener)
> +{
> +	struct uio_device *idev = listener->dev;
> +
> +	listener->event_count = atomic_read(&idev->event);

What prevents that count from changing right after you call this
function?

> +}
> +EXPORT_SYMBOL_GPL(uio_event_sync);

You are exporting a symbol, yet never using it, which means I can't
accept this patch, sorry.

greg k-h

Re: [PATCH] Add uio_sync_event interface to support mulit-instances case in UIO framework

[Greg KH]

On Wed, Aug 14, 2013 at 08:30:22PM +0800, Xiaolong Ye wrote:
> Hi, guys,
> 
> Recently, we have met fake interrupt issue when using UIO as our vpu
> driver in mulit-instances case, the issue can be described as below:

Do you have a pointer to this driver?

> In multi-instances case, we use vpu_lock(semaphore) to implement the mutually exclusive access to the
> device, each instace open uio device once and will be associated with a fd and its own uio_listener,
> we use poll to wait for the hardware interrupt. So let's assume that there are two instances, A and B,
> their uio_listener->event_count and idev->event are all 0 at the beginning, then A get the vpu_lock and start
> to work, it will block at poll()(because listener->event_count == idev->count) until H/W interrupt happens
> (atomic_inc(&idev->event)), after it releases vpu_lock, B will get the lock and start to work, however,
> its poll will return immediately because B's uio_listener->event_count is 0 while idev->event has been 1,
> that's how the fake interrupt happens.

Why not have just one instance talking to the hardware and then do the
locking in userspace for who ever needs to access the device?

thanks,

greg k-h