[PATCH] uio: User IRQ Mode

[PATCH] uio: User IRQ Mode

[Magnus Damm]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 2095 bytes --]

From: Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>

This patch adds a "User IRQ Mode" to UIO. In this mode the user space driver
is responsible for acknowledging and re-enabling the interrupt.
Shared interrupts are not supported by this mode.

Signed-off-by: Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>
Signed-off-by: Magnus Damm <damm@igel.co.jp>
---

 Similar code has been posted some time ago as:
 "[PATCH] uio_pdrv: Unique IRQ Mode"
 "[PATCH 00/03][RFC] Reusable UIO Platform Driver".

 Changes since Uwe's last version:
  - flags should be unsigned long
  - simplify uio_userirq_handler()

 Needs "[PATCH 0/1] UIO: Add a write() function to enable/disable interrupts"

 include/linux/uio_driver.h |   29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

--- 0020/include/linux/uio_driver.h
+++ work/include/linux/uio_driver.h	2008-07-02 19:45:55.000000000 +0900
@@ -17,6 +17,7 @@
 #include <linux/module.h>
 #include <linux/fs.h>
 #include <linux/interrupt.h>
+#include <linux/bitops.h>
 
 struct uio_map;
 
@@ -63,6 +64,7 @@ struct uio_info {
 	long			irq;
 	unsigned long		irq_flags;
 	void			*priv;
+	unsigned long		flags;
 	irqreturn_t (*handler)(int irq, struct uio_info *dev_info);
 	int (*mmap)(struct uio_info *info, struct vm_area_struct *vma);
 	int (*open)(struct uio_info *info, struct inode *inode);
@@ -92,4 +94,31 @@ extern void uio_event_notify(struct uio_
 #define UIO_MEM_LOGICAL	2
 #define UIO_MEM_VIRTUAL 3
 
+/* defines for uio_info->flags */
+#define UIO_FLAGS_IRQDISABLED 0
+
+static inline irqreturn_t uio_userirq_handler(int irq,
+		struct uio_info *dev_info)
+{
+	int ret = -1;
+
+	if (likely(dev_info->irqcontrol))
+		ret = dev_info->irqcontrol(dev_info, 0);
+
+	return IRQ_RETVAL(ret == 0);
+}
+
+static inline int uio_userirq_irqcontrol(struct uio_info *info, s32 irq_on)
+{
+	if (irq_on) {
+		if (test_and_clear_bit(UIO_FLAGS_IRQDISABLED, &info->flags))
+			enable_irq(info->irq);
+	} else {
+		if (!test_and_set_bit(UIO_FLAGS_IRQDISABLED, &info->flags))
+			disable_irq(info->irq);
+	}
+
+	return 0;
+}
+
 #endif /* _LINUX_UIO_DRIVER_H_ */

Re: [PATCH] uio: User IRQ Mode

[Alan Cox]

On Wed, 02 Jul 2008 19:59:51 +0900
Magnus Damm <magnus.damm@gmail.com> wrote:

> From: Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>
> 
> This patch adds a "User IRQ Mode" to UIO. In this mode the user space driver
> is responsible for acknowledging and re-enabling the interrupt.
> Shared interrupts are not supported by this mode.

This doesn't work even for some non shared interrupts.

If I take a level triggered interrupt then the IRQ handler code must
clear the IRQ before the line can be unmasked.

It might work for edge triggered providing you don't get too many edges
before you respond (in which case we will decide its a stuck IRQ and turn
it off for good).

Re: [PATCH] uio: User IRQ Mode

[Uwe Kleine-König]

Alan Cox wrote:
> On Wed, 02 Jul 2008 19:59:51 +0900
> Magnus Damm <magnus.damm@gmail.com> wrote:
> 
> > From: Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>
> >
> > This patch adds a "User IRQ Mode" to UIO. In this mode the user space driver
> > is responsible for acknowledging and re-enabling the interrupt.
> > Shared interrupts are not supported by this mode.
> 
> This doesn't work even for some non shared interrupts.
> 
> If I take a level triggered interrupt then the IRQ handler code must
> clear the IRQ before the line can be unmasked.
Note that the irq is disabled instead of acked.  So this should not be a
problem.  The userspace part then is responsible to ack (first) and
unmask the irq.

This mis-understanding might be a hint to improve the commit log ...

Best regards
Uwe

-- 
Uwe Kleine-König, Software Engineer
Digi International GmbH Branch Breisach, Küferstrasse 8, 79206 Breisach, Germany
Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962

Re: [PATCH] uio: User IRQ Mode

[Alan Cox]

> Note that the irq is disabled instead of acked.  So this should not be a
> problem.  The userspace part then is responsible to ack (first) and
> unmask the irq.

That still doesn't make sense.

If you call disable_irq in the IRQ handler you will deadlock because it
waits until the IRQ handler has completed.

Re: [PATCH] uio: User IRQ Mode

[Magnus Damm]

On Wed, Jul 2, 2008 at 8:31 PM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
>> Note that the irq is disabled instead of acked.  So this should not be a
>> problem.  The userspace part then is responsible to ack (first) and
>> unmask the irq.
>
> That still doesn't make sense.
>
> If you call disable_irq in the IRQ handler you will deadlock because it
> waits until the IRQ handler has completed.

Right, the uio_userirq_handler() function should use
disable_irq_nosync() instead of disable_irq(). Otherwise we'll end up
waiting forever for IRQ_INPROGRESS to be cleared in synchronize_irq()
in the case of CONFIG_SMP.

Thanks!

/ magnus

Re: [PATCH] uio: User IRQ Mode

[Magnus Damm]

Hi again Alan,

On Wed, Jul 2, 2008 at 8:11 PM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> On Wed, 02 Jul 2008 19:59:51 +0900
> Magnus Damm <magnus.damm@gmail.com> wrote:
>
>> From: Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>
>>
>> This patch adds a "User IRQ Mode" to UIO. In this mode the user space driver
>> is responsible for acknowledging and re-enabling the interrupt.
>> Shared interrupts are not supported by this mode.
>
> This doesn't work even for some non shared interrupts.
>
> If I take a level triggered interrupt then the IRQ handler code must
> clear the IRQ before the line can be unmasked.
>
> It might work for edge triggered providing you don't get too many edges
> before you respond (in which case we will decide its a stuck IRQ and turn
> it off for good).

Below is a little description of how this is supposed to work, see
point 5, 8, 9.

--- Regular in-kernel driver approach ---

1. hardware device signals the processor
   -> interrupt signal to interrupt controller gets asserted

2. interrupt controller passes through interrupt signal
   the source is enabled so the interrupt controller does not mask
   -> interrupt signal to processor gets asserted

3. processor saves context and starts executing kernel interrupt code
   the processor will mask interrupts for this certain interrupt level

4. mask_ack_irq()
   this modifies the state of the interrupt controller
   -> interrupt signal to processor is no longer asserted

5. kernel interrupt handler acknowledges interrupt
   this modifies the state of the hardware device
   -> interrupt signal to interrupt controller is no longer asserted

6. unmask_irq() - unmask interrupt controller
   this modifies the state of the interrupt controller
   -> interrupt signal to processor is let through, but is no longer asserted

7. processor restores context
   this will unmask interrupts for the current interrupt level

--- UIO "User IRQ Mode" approach ---

1. hardware device signals the processor
   -> interrupt signal to interrupt controller gets asserted

2. interrupt controller passes through interrupt signal
   the source is enabled so the interrupt controller does not mask
   -> interrupt signal to processor gets asserted

3. processor saves context and starts executing kernel interrupt code
   the processor will mask interrupts for this certain interrupt level

4. mask_ack_irq()
   this modifies the state of the interrupt controller
   -> interrupt signal to processor is no longer asserted

5. kernel interrupt handler _disables_ interrupts, notifies user space
   this may modify the state of the interrupt controller
   -> interrupt signal from device to interrupt controller is still asserted
   -> interrupt source is still masked by interrupt controller

6. unmask_irq() - unmask interrupt controller
   this may modify the sate of the interrupt controller
   -> interrupt source is still masked by interrupt controller

7. processor restores context
   this will unmask interrupts for the current interrupt level
   -> interrupt signal from device to interrupt controller is still asserted
   -> interrupt source is still masked by interrupt controller

8. user space driver get scheduled, acknowledges interrupt
   this modifies the state of the hardware device
   -> interrupt signal from device to interrupt controller gets deasserted
   -> interrupt source is still masked by interrupt controller

9. user space driver uses write() to re-enable the interrupt
   this modifies the state of the interrupt controller
   -> interrupt source is no longer masked by interrupt controller

Hope this clarifies a bit. Thank for your feedback!

/ magnus

Re: [PATCH] uio: User IRQ Mode

[Hans J. Koch]

On Wed, Jul 02, 2008 at 07:59:51PM +0900, Magnus Damm wrote:
> From: Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>
> 
> This patch adds a "User IRQ Mode" to UIO. In this mode the user space driver
> is responsible for acknowledging and re-enabling the interrupt.

This can easily be done without your patch.

> Shared interrupts are not supported by this mode.
> 
> Signed-off-by: Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>
> Signed-off-by: Magnus Damm <damm@igel.co.jp>
> ---
> 
>  Similar code has been posted some time ago as:
>  "[PATCH] uio_pdrv: Unique IRQ Mode"
>  "[PATCH 00/03][RFC] Reusable UIO Platform Driver".

Yes, and in that thread I gave detailed explanations why I won't accept
that.

> 
>  Changes since Uwe's last version:
>   - flags should be unsigned long
>   - simplify uio_userirq_handler()

That's nearly nothing. All you do is sending the same stuff three weeks
later in the hope somebody will merge it this time. NAK.

Thanks,
Hans

> 
>  Needs "[PATCH 0/1] UIO: Add a write() function to enable/disable interrupts"
> 
>  include/linux/uio_driver.h |   29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> --- 0020/include/linux/uio_driver.h
> +++ work/include/linux/uio_driver.h	2008-07-02 19:45:55.000000000 +0900
> @@ -17,6 +17,7 @@
>  #include <linux/module.h>
>  #include <linux/fs.h>
>  #include <linux/interrupt.h>
> +#include <linux/bitops.h>
>  
>  struct uio_map;
>  
> @@ -63,6 +64,7 @@ struct uio_info {
>  	long			irq;
>  	unsigned long		irq_flags;
>  	void			*priv;
> +	unsigned long		flags;
>  	irqreturn_t (*handler)(int irq, struct uio_info *dev_info);
>  	int (*mmap)(struct uio_info *info, struct vm_area_struct *vma);
>  	int (*open)(struct uio_info *info, struct inode *inode);
> @@ -92,4 +94,31 @@ extern void uio_event_notify(struct uio_
>  #define UIO_MEM_LOGICAL	2
>  #define UIO_MEM_VIRTUAL 3
>  
> +/* defines for uio_info->flags */
> +#define UIO_FLAGS_IRQDISABLED 0
> +
> +static inline irqreturn_t uio_userirq_handler(int irq,
> +		struct uio_info *dev_info)
> +{
> +	int ret = -1;
> +
> +	if (likely(dev_info->irqcontrol))
> +		ret = dev_info->irqcontrol(dev_info, 0);
> +
> +	return IRQ_RETVAL(ret == 0);
> +}
> +
> +static inline int uio_userirq_irqcontrol(struct uio_info *info, s32 irq_on)
> +{
> +	if (irq_on) {
> +		if (test_and_clear_bit(UIO_FLAGS_IRQDISABLED, &info->flags))
> +			enable_irq(info->irq);
> +	} else {
> +		if (!test_and_set_bit(UIO_FLAGS_IRQDISABLED, &info->flags))
> +			disable_irq(info->irq);
> +	}
> +
> +	return 0;
> +}
> +
>  #endif /* _LINUX_UIO_DRIVER_H_ */

Re: [PATCH] uio: User IRQ Mode

[Uwe Kleine-König]

Hans J. Koch wrote:
> On Wed, Jul 02, 2008 at 07:59:51PM +0900, Magnus Damm wrote:
> > From: Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>
> >
> > This patch adds a "User IRQ Mode" to UIO. In this mode the user space driver
> > is responsible for acknowledging and re-enabling the interrupt.
> 
> This can easily be done without your patch.
> 
> > Shared interrupts are not supported by this mode.
> >
> > Signed-off-by: Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>
> > Signed-off-by: Magnus Damm <damm@igel.co.jp>
> > ---
> >
> >  Similar code has been posted some time ago as:
> >  "[PATCH] uio_pdrv: Unique IRQ Mode"
> >  "[PATCH 00/03][RFC] Reusable UIO Platform Driver".
> 
> Yes, and in that thread I gave detailed explanations why I won't accept
> that.
I think for all of your concers one of the following is true:

  - they are not valid any more in this version; or
  - I cannot understand it.

I'll try to list them all below.  Please tell us if the list isn't
complete or if my comments doesn't convince you.  You might have to
repeat yourself, but for me it's hard to sort your arguments because
Magnus' suggestion changed over time.

And please, I try to work out the pros and cons in a constructive way
and hope there is nothing in it you will take personal.  It's really
that I consider the patch valuable and don't understand your concerns.

In the first thread[1] your unique and open concerns (to the best of my
knowledge and belief) with my comments are:

	- "This only works for embedded devices [...]"

	  OK, this doesn't work with shared IRQs which rules out x86.
	  I don't claim to know all the 23[2] other architectures but
	  IMHO if something is good for 3 archs and doesn't hurt the
	  other 21, you should do it.

	- "This would save somebody the trouble to add the above 5 lines
	  to the 30 lines of board/platform support code he has to write
	  anyway. That's the only gain, and that is not enough."

	  IMHO it's worth it.  Because if you add the five lines to a
	  central place you save 5 lines per platform using the driver.
	  Moreover this might prevent some bugs.  (And obviously this
	  function has the potential to have a buggy implementation as
	  the comment by Alan Cox shows.)

	- "And if you _know_ that on your platform the irq is not
	  shared, this might really be a one-liner that simply calls
	  irq_disable. That's OK in board specific code, but not in a
	  generic driver."
	  
	  Please note that the patch only introduces a helper that the
	  platform code *can* use.  You still have the freedom not to
	  use it without any overhead.

	- "I won't accept anything that changes the current UIO
	  behaviour."

	  Not valid anymore.  There is no change in behaviour.

In the second thread[3] I cannot find any open concerns that are not
already listed above.

> >  Changes since Uwe's last version:
> >   - flags should be unsigned long
> >   - simplify uio_userirq_handler()
> 
> That's nearly nothing. All you do is sending the same stuff three weeks
> later in the hope somebody will merge it this time. NAK.
I think nobody really is surprised that you're not happy with the new
post.  But note that Magnus just did what you told him. ("I'm [not] the
big boss who makes the final decision. I can be critized and overridden.
If Greg loves your patch and merges it, fine. Try it.")

In the hope not to have kicked off a flame,
Uwe

[1] http://thread.gmane.org/gmane.linux.kernel/689631
[2] ukleinek@zentaur:~/gsrc/linux-2.6$ ls -l arch/ | grep ^d | wc -l
    24
[3] http://thread.gmane.org/gmane.linux.ports.sh.devel/3917/

-- 
Uwe Kleine-König, Software Engineer
Digi International GmbH Branch Breisach, Küferstrasse 8, 79206 Breisach, Germany
Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962

Re: [PATCH] uio: User IRQ Mode

[Hans J. Koch]

On Thu, Jul 03, 2008 at 09:10:19AM +0200, Uwe Kleine-König wrote:
> Hans J. Koch wrote:
> > On Wed, Jul 02, 2008 at 07:59:51PM +0900, Magnus Damm wrote:
> > > From: Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>
> > >
> > > This patch adds a "User IRQ Mode" to UIO. In this mode the user space driver
> > > is responsible for acknowledging and re-enabling the interrupt.
> > 
> > This can easily be done without your patch.

BTW, the above wording "the user space driver is responsible for
acknowledging and re-enabling the interrupt" is misleading. The kernel
always has to acknowledge/disable/mask the interrupt. Userspace can only
reenable it, ideally by writing to a chip register. In some cornercases
for broken hardware we need the newly introduced write function.

> > 
> > > Shared interrupts are not supported by this mode.
> > >
> > > Signed-off-by: Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>
> > > Signed-off-by: Magnus Damm <damm@igel.co.jp>
> > > ---
> > >
> > >  Similar code has been posted some time ago as:
> > >  "[PATCH] uio_pdrv: Unique IRQ Mode"
> > >  "[PATCH 00/03][RFC] Reusable UIO Platform Driver".
> > 
> > Yes, and in that thread I gave detailed explanations why I won't accept
> > that.
> I think for all of your concers one of the following is true:
> 
>   - they are not valid any more in this version; or

I question the whole concept as such. The concept of having a generic
irq handler using disable_irq_nosync() makes no sense at all.

Reasons:

- We do not introduce new possibilities. Everything can be done without
  that patch.

- We offer users an irq handler that shouldn't be used. It is seldom the
  best solution to call disable_irq_nosync() to disable an interrupt. In
  almost all cases you should use the irq mask register of the chip your
  driver handles. What do you want to write into the docs? Here's an irq
  handler, but please use it only if you're really desperate?

- The only argument in favor of that concept was that it saves a few
  lines of irq handler code. Given the fact that all the handler has to
  do is toggle one bit in a hardware register, this is really not much.
  And you tempt people to delete 5 lines of good code and replace them
  with a sub-optimal generic irq handler.

- You introduce the need for an irqcontrol function. This was not the
  intention of that concept. Normal UIO devices don't need a write
  function, this is only used for broken hardware. If you have normal
  hardware, and implement a proper 5 lines irq handler, userspace can simply
  reenable the irq by writing to a hardware register it has mapped
  anyway. In your concept, it has to use write() on /dev/uioX, which
  means you have to go all the way through libc, vfs, and the UIO core
  to finally call your irqcontrol function, which in turn calls
  enable_irq. As I said, there is broken hardware around where this is
  the only way, but most chips allow you to do it properly.

>   - I cannot understand it.
> 
> I'll try to list them all below.  Please tell us if the list isn't
> complete or if my comments doesn't convince you.  You might have to
> repeat yourself, but for me it's hard to sort your arguments because
> Magnus' suggestion changed over time.

OK, the version Magnus sent last is different, so some of my arguments
are superfluous now.

Please, to make things simpler, let's only talk about the concept as
such and not go into implementation details. I deliberately do not review
that code (although I believe it has more bugs than the one Alan found),
because as long as the concept doesn't make sense, I don't care how it
is implemented.

> 
> And please, I try to work out the pros and cons in a constructive way
> and hope there is nothing in it you will take personal.  It's really
> that I consider the patch valuable and don't understand your concerns.

You both keep telling me how valuable that patch is but never answered my
question what the advantage would be. I cannot see it yet.

> 
> In the first thread[1] your unique and open concerns (to the best of my
> knowledge and belief) with my comments are:
> 
> 	- "This only works for embedded devices [...]"
> 
> 	  OK, this doesn't work with shared IRQs which rules out x86.
> 	  I don't claim to know all the 23[2] other architectures but
> 	  IMHO if something is good for 3 archs and doesn't hurt the
> 	  other 21, you should do it.
> 
> 	- "This would save somebody the trouble to add the above 5 lines
> 	  to the 30 lines of board/platform support code he has to write
> 	  anyway. That's the only gain, and that is not enough."
> 
> 	  IMHO it's worth it.  Because if you add the five lines to a
> 	  central place you save 5 lines per platform using the driver.

OK, that is one argument in favor. I always admitted that, but said that
this is not enough to compensate for the disadvantages mentioned above.

> 	  Moreover this might prevent some bugs.  (And obviously this
> 	  function has the potential to have a buggy implementation as
> 	  the comment by Alan Cox shows.)

For me, this shows two things:

- I never ever had to use disable_irq_nosync() in any UIO driver yet,
  otherwise I would have noticed.

- Magnus turned in a patch that he never tested.

> 
> 	- "And if you _know_ that on your platform the irq is not
> 	  shared, this might really be a one-liner that simply calls
> 	  irq_disable. That's OK in board specific code, but not in a
> 	  generic driver."
> 	  
> 	  Please note that the patch only introduces a helper that the
> 	  platform code *can* use.  You still have the freedom not to
> 	  use it without any overhead.

It's not a good idea to add nonsense code and tell the users to ignore
it whenever they can...

> 
> 	- "I won't accept anything that changes the current UIO
> 	  behaviour."
> 
> 	  Not valid anymore.  There is no change in behaviour.

Well, at least the whole stuff would have to be explained in the docs.
You add an element to struct uio_info, together with a new #define. And
a whole class of drivers using that stuff would have a write() function
without needing it.

> 
> In the second thread[3] I cannot find any open concerns that are not
> already listed above.
> 
> > >  Changes since Uwe's last version:
> > >   - flags should be unsigned long
> > >   - simplify uio_userirq_handler()
> > 
> > That's nearly nothing. All you do is sending the same stuff three weeks
> > later in the hope somebody will merge it this time. NAK.
> I think nobody really is surprised that you're not happy with the new
> post.  But note that Magnus just did what you told him. ("I'm [not] the
> big boss who makes the final decision. I can be critized and overridden.
> If Greg loves your patch and merges it, fine. Try it.")
> 
> In the hope not to have kicked off a flame,

Oh, no, stay cool ;-)

Thanks,
Hans

Re: [PATCH] uio: User IRQ Mode

[Paul Mundt]

On Thu, Jul 03, 2008 at 02:45:05PM +0200, Hans J. Koch wrote:
> On Thu, Jul 03, 2008 at 09:10:19AM +0200, Uwe Kleine-K??nig wrote:
> > 	  Moreover this might prevent some bugs.  (And obviously this
> > 	  function has the potential to have a buggy implementation as
> > 	  the comment by Alan Cox shows.)
> 
> For me, this shows two things:
> 
> - I never ever had to use disable_irq_nosync() in any UIO driver yet,
>   otherwise I would have noticed.
> 
> - Magnus turned in a patch that he never tested.
> 
Note that the deadlock in question is in relation to SMP, it's true that
the patch was never tested in an SMP environment and only on UP, but it
certainly was tested. The vast majority of driver writers don't have a
need to use disable_irq_nosync(), so whether you've had to use it or not
is largely irrelevant to the conversation at hand ;-)

Re: [PATCH] uio: User IRQ Mode

[Hans J. Koch]

On Thu, Jul 03, 2008 at 10:23:16PM +0900, Paul Mundt wrote:
> On Thu, Jul 03, 2008 at 02:45:05PM +0200, Hans J. Koch wrote:
> > On Thu, Jul 03, 2008 at 09:10:19AM +0200, Uwe Kleine-K??nig wrote:
> > > 	  Moreover this might prevent some bugs.  (And obviously this
> > > 	  function has the potential to have a buggy implementation as
> > > 	  the comment by Alan Cox shows.)
> > 
> > For me, this shows two things:
> > 
> > - I never ever had to use disable_irq_nosync() in any UIO driver yet,
> >   otherwise I would have noticed.
> > 
> > - Magnus turned in a patch that he never tested.
> > 
> Note that the deadlock in question is in relation to SMP, it's true that
> the patch was never tested in an SMP environment and only on UP, but it
> certainly was tested.

Ok, so I take back that accusation ;-)
Nonetheless, the patch changes a UIO core file, and everything in there
should work in all situations on all architectures unless there is a
_very_ good reason to do something different. This not only applies to
SMP issues but also to the limitation to non-shared interrupts.

> The vast majority of driver writers don't have a
> need to use disable_irq_nosync(), so whether you've had to use it or not
> is largely irrelevant to the conversation at hand ;-)

Sure ;-) I merely wanted to point out that this is an unusual way to
handle an interrupt.

Thanks,
Hans

Re: [PATCH] uio: User IRQ Mode

[Magnus Damm]

On Fri, Jul 4, 2008 at 4:55 AM, Hans J. Koch <hjk@linutronix.de> wrote:
> On Thu, Jul 03, 2008 at 10:23:16PM +0900, Paul Mundt wrote:
>> On Thu, Jul 03, 2008 at 02:45:05PM +0200, Hans J. Koch wrote:
>> > On Thu, Jul 03, 2008 at 09:10:19AM +0200, Uwe Kleine-K??nig wrote:
>> > >     Moreover this might prevent some bugs.  (And obviously this
>> > >     function has the potential to have a buggy implementation as
>> > >     the comment by Alan Cox shows.)
>> >
>> > For me, this shows two things:
>> >
>> > - I never ever had to use disable_irq_nosync() in any UIO driver yet,
>> >   otherwise I would have noticed.
>> >
>> > - Magnus turned in a patch that he never tested.
>> >
>> Note that the deadlock in question is in relation to SMP, it's true that
>> the patch was never tested in an SMP environment and only on UP, but it
>> certainly was tested.
>
> Ok, so I take back that accusation ;-)

That's good, thank you. In the future it would be even better if you
didn't accuse to begin with, since that will only heat up the
discussion.

> Nonetheless, the patch changes a UIO core file, and everything in there
> should work in all situations on all architectures unless there is a
> _very_ good reason to do something different. This not only applies to
> SMP issues but also to the limitation to non-shared interrupts.

I will resolve the SMP issue and repost, no problem.

>> The vast majority of driver writers don't have a
>> need to use disable_irq_nosync(), so whether you've had to use it or not
>> is largely irrelevant to the conversation at hand ;-)
>
> Sure ;-) I merely wanted to point out that this is an unusual way to
> handle an interrupt.

Grep shows that there is nothing unusual about it.

damm@rx1 ~/git/linux-2.6 $ find drivers/ | xargs grep -m 1 -e
[[:blank:]]disable_irq\( -e [[:blank:]]disable_irq_nosync\( | wc -l
105

/ magnus

Re: [PATCH] uio: User IRQ Mode

[Hans J. Koch]

On Fri, Jul 04, 2008 at 11:55:08AM +0900, Magnus Damm wrote:
> 
> I will resolve the SMP issue and repost, no problem.

That won't change anything. I already said I don't care about
implementation details if the concept itself is crap.

And meanwhile I'm really fed up with you ignoring what I say and not
answering my questions.

Thanks,
Hans

Re: [PATCH] uio: User IRQ Mode

[Magnus Damm]

On Thu, Jul 3, 2008 at 9:45 PM, Hans J. Koch <hjk@linutronix.de> wrote:
> On Thu, Jul 03, 2008 at 09:10:19AM +0200, Uwe Kleine-König wrote:
>> Hans J. Koch wrote:
>> > On Wed, Jul 02, 2008 at 07:59:51PM +0900, Magnus Damm wrote:
>> > > From: Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>
>> > >
>> > > This patch adds a "User IRQ Mode" to UIO. In this mode the user space driver
>> > > is responsible for acknowledging and re-enabling the interrupt.
>> >
>> > This can easily be done without your patch.
>
> BTW, the above wording "the user space driver is responsible for
> acknowledging and re-enabling the interrupt" is misleading. The kernel
> always has to acknowledge/disable/mask the interrupt. Userspace can only
> reenable it, ideally by writing to a chip register. In some cornercases
> for broken hardware we need the newly introduced write function.

You seem to be mixing up masking/acknowledging the interrupt
controller and masking/acknowledging the actual hardware device. In
User IRQ Mode, the only thing the user space driver is accessing is
the hardware device, with the exception of write() to re-enable
interrupts which results in a enable_irq() that touches the interrupt
controller.

I'm not sure what kind of hardware devices you are talking about, but
I have hardware devices here on my desk that need to be acknowledged
by the device-specific driver to deassert the irq. And acknowledging
from user space works just fine.

>> > > Shared interrupts are not supported by this mode.
>> > >
>> > > Signed-off-by: Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>
>> > > Signed-off-by: Magnus Damm <damm@igel.co.jp>
>> > > ---
>> > >
>> > >  Similar code has been posted some time ago as:
>> > >  "[PATCH] uio_pdrv: Unique IRQ Mode"
>> > >  "[PATCH 00/03][RFC] Reusable UIO Platform Driver".
>> >
>> > Yes, and in that thread I gave detailed explanations why I won't accept
>> > that.
>> I think for all of your concers one of the following is true:
>>
>>   - they are not valid any more in this version; or
>
> I question the whole concept as such. The concept of having a generic
> irq handler using disable_irq_nosync() makes no sense at all.
>
> Reasons:
>
> - We do not introduce new possibilities. Everything can be done without
>  that patch.

Isn't it possible to use the same argument against UIO? There is no
point in having it since it is possible to do it all in kernel space
instead. But for various reasons people want to write their drivers in
user space.

And for various reasons I'd like to acknowledge interrupts in user
space. And I do that by disabling the interrupt in kernel space,
notify the user space driver and acknowledge and re-enable the
interrupt from there. The reason behind it is that I want to provide
user space device driver people with consistent interface to the
device without the need for any device specific code in kernel space.

> - We offer users an irq handler that shouldn't be used. It is seldom the
>  best solution to call disable_irq_nosync() to disable an interrupt. In
>  almost all cases you should use the irq mask register of the chip your
>  driver handles. What do you want to write into the docs? Here's an irq
>  handler, but please use it only if you're really desperate?

What is wrong with using disable_irq_nosync() compared to using the
device specific irq mask register? It works just fine, and it provides
a nice abstraction in my mind.

> - The only argument in favor of that concept was that it saves a few
>  lines of irq handler code. Given the fact that all the handler has to
>  do is toggle one bit in a hardware register, this is really not much.
>  And you tempt people to delete 5 lines of good code and replace them
>  with a sub-optimal generic irq handler.

What is suboptimal about it? And since when is a device specific
solution better than a generic one?

> - You introduce the need for an irqcontrol function. This was not the
>  intention of that concept. Normal UIO devices don't need a write
>  function, this is only used for broken hardware. If you have normal
>  hardware, and implement a proper 5 lines irq handler, userspace can simply
>  reenable the irq by writing to a hardware register it has mapped
>  anyway. In your concept, it has to use write() on /dev/uioX, which
>  means you have to go all the way through libc, vfs, and the UIO core
>  to finally call your irqcontrol function, which in turn calls
>  enable_irq. As I said, there is broken hardware around where this is
>  the only way, but most chips allow you to do it properly.

You are the one who posted the irqcontrol code because it solved some
issues you are having with broken hardware. I'm happy to use your code
in a generic way. Actually, my first version of this patch avoided the
extra write() call overhead by re-enabling things automatically, but
since your irqcontrol patch is cleaner I'd rather use that and live
with the small performance penalty.

Performance in this case is really a non-issue. If we experience
performance problems in the future because of high interrupt overhead
then UIO is most likely no longer suitable for us. Right now it's a
perfect match.

>>   - I cannot understand it.
>>
>> I'll try to list them all below.  Please tell us if the list isn't
>> complete or if my comments doesn't convince you.  You might have to
>> repeat yourself, but for me it's hard to sort your arguments because
>> Magnus' suggestion changed over time.
>
> OK, the version Magnus sent last is different, so some of my arguments
> are superfluous now.
>
> Please, to make things simpler, let's only talk about the concept as
> such and not go into implementation details. I deliberately do not review
> that code (although I believe it has more bugs than the one Alan found),
> because as long as the concept doesn't make sense, I don't care how it
> is implemented.

I'd say it makes sense to Paul, Uwe and me.

>> And please, I try to work out the pros and cons in a constructive way
>> and hope there is nothing in it you will take personal.  It's really
>> that I consider the patch valuable and don't understand your concerns.
>
> You both keep telling me how valuable that patch is but never answered my
> question what the advantage would be. I cannot see it yet.

Avoid having device specific interrupts handlers in kernel space?

>>
>> In the first thread[1] your unique and open concerns (to the best of my
>> knowledge and belief) with my comments are:
>>
>>       - "This only works for embedded devices [...]"
>>
>>         OK, this doesn't work with shared IRQs which rules out x86.
>>         I don't claim to know all the 23[2] other architectures but
>>         IMHO if something is good for 3 archs and doesn't hurt the
>>         other 21, you should do it.
>>
>>       - "This would save somebody the trouble to add the above 5 lines
>>         to the 30 lines of board/platform support code he has to write
>>         anyway. That's the only gain, and that is not enough."
>>
>>         IMHO it's worth it.  Because if you add the five lines to a
>>         central place you save 5 lines per platform using the driver.
>
> OK, that is one argument in favor. I always admitted that, but said that
> this is not enough to compensate for the disadvantages mentioned above.

I still fail to see your technical arguments.

>>         Moreover this might prevent some bugs.  (And obviously this
>>         function has the potential to have a buggy implementation as
>>         the comment by Alan Cox shows.)
>
> For me, this shows two things:
>
> - I never ever had to use disable_irq_nosync() in any UIO driver yet,
>  otherwise I would have noticed.

Is that so? Given the stunning UIO patch throughput it wouldn't
surprise me if people end up using local patches instead of even
trying to merge things upstream. Which means you wouldn't see the
code.

> - Magnus turned in a patch that he never tested.

Is that so?

I've tested the patch using two different processors on three
different boards. I have written a user space uio driver that
interfaces to mplayer using vidix, providing hardware accelerated
color space conversion and stretching. So don't call my patch untested
please.

I plan on submitting the vidix driver upstream soon after the uio
interface gets stabilized, ie this patch gets merged.

>>
>>       - "And if you _know_ that on your platform the irq is not
>>         shared, this might really be a one-liner that simply calls
>>         irq_disable. That's OK in board specific code, but not in a
>>         generic driver."
>>
>>         Please note that the patch only introduces a helper that the
>>         platform code *can* use.  You still have the freedom not to
>>         use it without any overhead.
>
> It's not a good idea to add nonsense code and tell the users to ignore
> it whenever they can...

This nonsense code, exactly which code are you talking about? Could it
be one of your significant kernel contributions? =)

>>
>>       - "I won't accept anything that changes the current UIO
>>         behaviour."
>>
>>         Not valid anymore.  There is no change in behaviour.
>
> Well, at least the whole stuff would have to be explained in the docs.

Sure, documentation is always good.

> You add an element to struct uio_info, together with a new #define. And
> a whole class of drivers using that stuff would have a write() function
> without needing it.

Another way to see it is that a whole class of drivers would need
kernel space interrupt handling code without really needing it.

>>
>> In the second thread[3] I cannot find any open concerns that are not
>> already listed above.
>>
>> > >  Changes since Uwe's last version:
>> > >   - flags should be unsigned long
>> > >   - simplify uio_userirq_handler()
>> >
>> > That's nearly nothing. All you do is sending the same stuff three weeks
>> > later in the hope somebody will merge it this time. NAK.
>> I think nobody really is surprised that you're not happy with the new
>> post.  But note that Magnus just did what you told him. ("I'm [not] the
>> big boss who makes the final decision. I can be critized and overridden.
>> If Greg loves your patch and merges it, fine. Try it.")
>>
>> In the hope not to have kicked off a flame,
>
> Oh, no, stay cool ;-)

Good, let's stay cool.

/ magnus

Re: [PATCH] uio: User IRQ Mode

[Uwe Kleine-König]

Magnus Damm wrote:
> On Thu, Jul 3, 2008 at 9:45 PM, Hans J. Koch <hjk@linutronix.de> wrote:
> > On Thu, Jul 03, 2008 at 09:10:19AM +0200, Uwe Kleine-König wrote:
> >> Hans J. Koch wrote:
> >> > On Wed, Jul 02, 2008 at 07:59:51PM +0900, Magnus Damm wrote:
> >> > > From: Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>
> >> > >
> >> > > This patch adds a "User IRQ Mode" to UIO. In this mode the user space driver
> >> > > is responsible for acknowledging and re-enabling the interrupt.
> >> >
> >> > This can easily be done without your patch.
> >
> > BTW, the above wording "the user space driver is responsible for
> > acknowledging and re-enabling the interrupt" is misleading. The kernel
> > always has to acknowledge/disable/mask the interrupt. Userspace can only
> > reenable it, ideally by writing to a chip register. In some cornercases
> > for broken hardware we need the newly introduced write function.
> 
> You seem to be mixing up masking/acknowledging the interrupt
> controller and masking/acknowledging the actual hardware device. In
> User IRQ Mode, the only thing the user space driver is accessing is
> the hardware device, with the exception of write() to re-enable
> interrupts which results in a enable_irq() that touches the interrupt
> controller.
But to be honest Hans is right here, the commit log wording is not
optimal.  I suggest:

	This patch adds a "User IRQ Mode" to UIO. In this mode the
	kernel space simply disables the serviced interrupt in the
	interrupt controller and the user space driver is responsible
	for acknowledging it in the device and reenabling it.

	Note that this implies that the interrupt might be disabled for
	long periods, so this isn't usable for shared interrupt lines.

Maybe it's sensible to add the User IRQ Mode functions at least for now
into platform code.  Then at a later time if and when there are several
copies the discussion to move it to the generic part might be easier.

BTW, I currently have a situation where it IMHO really makes sense to
use the User IRQ Mode:  We sell a cpu module to a customer with
Linux.  I provide a uio device for some memory mapped periphal on the
customers board that I don't know in detail.  With the User IRQ Mode I
only need to know the chip select and the irq line, no further
information is needed for the device.

Best regards,
Uwe

-- 
Uwe Kleine-König, Software Engineer
Digi International GmbH Branch Breisach, Küferstrasse 8, 79206 Breisach, Germany
Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962

Re: [PATCH] uio: User IRQ Mode

[Magnus Damm]

On Fri, Jul 4, 2008 at 3:01 PM, Uwe Kleine-König
<Uwe.Kleine-Koenig@digi.com> wrote:
> Magnus Damm wrote:
>> On Thu, Jul 3, 2008 at 9:45 PM, Hans J. Koch <hjk@linutronix.de> wrote:
>> > On Thu, Jul 03, 2008 at 09:10:19AM +0200, Uwe Kleine-König wrote:
>> >> Hans J. Koch wrote:
>> >> > On Wed, Jul 02, 2008 at 07:59:51PM +0900, Magnus Damm wrote:
>> >> > > From: Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>
>> >> > >
>> >> > > This patch adds a "User IRQ Mode" to UIO. In this mode the user space driver
>> >> > > is responsible for acknowledging and re-enabling the interrupt.
>> >> >
>> >> > This can easily be done without your patch.
>> >
>> > BTW, the above wording "the user space driver is responsible for
>> > acknowledging and re-enabling the interrupt" is misleading. The kernel
>> > always has to acknowledge/disable/mask the interrupt. Userspace can only
>> > reenable it, ideally by writing to a chip register. In some cornercases
>> > for broken hardware we need the newly introduced write function.
>>
>> You seem to be mixing up masking/acknowledging the interrupt
>> controller and masking/acknowledging the actual hardware device. In
>> User IRQ Mode, the only thing the user space driver is accessing is
>> the hardware device, with the exception of write() to re-enable
>> interrupts which results in a enable_irq() that touches the interrupt
>> controller.
> But to be honest Hans is right here, the commit log wording is not
> optimal.  I suggest:
>
>        This patch adds a "User IRQ Mode" to UIO. In this mode the
>        kernel space simply disables the serviced interrupt in the
>        interrupt controller and the user space driver is responsible
>        for acknowledging it in the device and reenabling it.

Right, that's better. Thank you.

>        Note that this implies that the interrupt might be disabled for
>        long periods, so this isn't usable for shared interrupt lines.

While I think it's a good idea to write that the interrupt latency
will be increased, I wouldn't say the above is clear enough.

Shared interrupts are not supported by the User IRQ Mode since the
generic in-kernel interrupt handler will disable the shared interrupt
line while servicing interrupts. Disabling the shared interrupt line
will prevent other devices sharing the same interrupt line from being
serviced properly. This fact makes User IRQ Mode unsuitable for shared
interrupts.

> Maybe it's sensible to add the User IRQ Mode functions at least for now
> into platform code.  Then at a later time if and when there are several
> copies the discussion to move it to the generic part might be easier.

Do you mean your uio_pdrv driver?

> BTW, I currently have a situation where it IMHO really makes sense to
> use the User IRQ Mode:  We sell a cpu module to a customer with
> Linux.  I provide a uio device for some memory mapped periphal on the
> customers board that I don't know in detail.  With the User IRQ Mode I
> only need to know the chip select and the irq line, no further
> information is needed for the device.

Yes, and this is a very similar to our requirements. We want to expose
various memory mapped devices inside our SoCs to user space, providing
only device base address and unique interrupt number.

Thank you!

/ magnus

Re: [PATCH] uio: User IRQ Mode

[Uwe Kleine-König]

Hello Magnus,

Magnus Damm wrote:
> > Maybe it's sensible to add the User IRQ Mode functions at least for now
> > into platform code.  Then at a later time if and when there are several
> > copies the discussion to move it to the generic part might be easier.
> 
> Do you mean your uio_pdrv driver?
No, I don't.  I meant arch/whatever/...

If you want to add it to uio_pdrv you either have to introduce a new
header file or you need to add it to uio_driver.h.  IMHO the first is
ugly and I'm sure Hans will object the latter.

Best regards
Uwe

-- 
Uwe Kleine-König, Software Engineer
Digi International GmbH Branch Breisach, Küferstrasse 8, 79206 Breisach, Germany
Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962

Re: [PATCH] uio: User IRQ Mode

[Paul Mundt]

On Fri, Jul 04, 2008 at 10:11:25AM +0200, Uwe Kleine-K?nig wrote:
> Magnus Damm wrote:
> > > Maybe it's sensible to add the User IRQ Mode functions at least for now
> > > into platform code.  Then at a later time if and when there are several
> > > copies the discussion to move it to the generic part might be easier.
> > 
> > Do you mean your uio_pdrv driver?
> No, I don't.  I meant arch/whatever/...
> 
Placing it in arch/ makes no sense, as there is nothing platform or
architecture specific about it. It is a generic bit of functionality that
various platforms may or may not want to enable, it should be reflected
in UIO directly and simply enabled by those that care. Copying it around
all over the place to make up for the fact the UIO people don't want to
take patches is not a solution.

> If you want to add it to uio_pdrv you either have to introduce a new
> header file or you need to add it to uio_driver.h.  IMHO the first is
> ugly and I'm sure Hans will object the latter.
> 
I'm sure Hans will object to pretty much any UIO patch that adds
functionality he didn't envision from the beginning, but that doesn't
justify burying this crap in the architecture code. Likewise, without any
serious technical objections to the user IRQ mode, it's also difficult to
care. So far all of the technical issues raised in this thread and the
ones before it have all been readily addressed, and I'm unaware of any
outstanding issues here.

Having said that, it would be nice to respin this encapsulating your
rewrite of the commit log so there's less confusion, and then see about
having Greg or Andrew merge this. The patch can easily be reworked if
anyone else raises any technical concerns about this particular approach.

Re: [PATCH] uio: User IRQ Mode

[Hans J. Koch]

On Fri, Jul 04, 2008 at 05:29:59PM +0900, Paul Mundt wrote:
> On Fri, Jul 04, 2008 at 10:11:25AM +0200, Uwe Kleine-K?nig wrote:
> > Magnus Damm wrote:
> > > > Maybe it's sensible to add the User IRQ Mode functions at least for now
> > > > into platform code.  Then at a later time if and when there are several
> > > > copies the discussion to move it to the generic part might be easier.
> > > 
> > > Do you mean your uio_pdrv driver?
> > No, I don't.  I meant arch/whatever/...
> > 
> Placing it in arch/ makes no sense, as there is nothing platform or
> architecture specific about it. It is a generic bit of functionality that
> various platforms may or may not want to enable, it should be reflected
> in UIO directly and simply enabled by those that care. Copying it around
> all over the place to make up for the fact the UIO people don't want to
> take patches is not a solution.

Why don't you make it a driver of it's own? Maybe some uio_pdrv_genirq?
I'm not really sure if it's a good idea (need to think about it), but
it's certainly easier to accept.

No need to change the UIO core.

> 
> > If you want to add it to uio_pdrv you either have to introduce a new
> > header file or you need to add it to uio_driver.h.  IMHO the first is
> > ugly and I'm sure Hans will object the latter.
> > 
> I'm sure Hans will object to pretty much any UIO patch that adds
> functionality he didn't envision from the beginning,

Like uio_pdrv?

> but that doesn't
> justify burying this crap in the architecture code. Likewise, without any
> serious technical objections to the user IRQ mode, it's also difficult to
> care. So far all of the technical issues raised in this thread and the
> ones before it have all been readily addressed, and I'm unaware of any
> outstanding issues here.
> 
> Having said that, it would be nice to respin this encapsulating your
> rewrite of the commit log so there's less confusion, and then see about
> having Greg or Andrew merge this. The patch can easily be reworked if
> anyone else raises any technical concerns about this particular approach.

Please read my other postings in this thread. You cannot ignore
everything just because it's not important on the SH board in front of
you.

Thanks,
Hans

Re: [PATCH] uio: User IRQ Mode

[Alan Cox]

> 	Note that this implies that the interrupt might be disabled for
> 	long periods, so this isn't usable for shared interrupt lines.

It isn't usable for shared lines anyway - because you would have deadlocks

Re: [PATCH] uio: User IRQ Mode

[Hans J. Koch]

On Fri, Jul 04, 2008 at 08:01:08AM +0200, Uwe Kleine-K?nig wrote:
> Magnus Damm wrote:
> > On Thu, Jul 3, 2008 at 9:45 PM, Hans J. Koch <hjk@linutronix.de> wrote:
> > > On Thu, Jul 03, 2008 at 09:10:19AM +0200, Uwe Kleine-K?nig wrote:
> > >> Hans J. Koch wrote:
> > >> > On Wed, Jul 02, 2008 at 07:59:51PM +0900, Magnus Damm wrote:
> > >> > > From: Uwe Kleine-K?nig <Uwe.Kleine-Koenig@digi.com>
> > >> > >
> > >> > > This patch adds a "User IRQ Mode" to UIO. In this mode the user space driver
> > >> > > is responsible for acknowledging and re-enabling the interrupt.
> > >> >
> > >> > This can easily be done without your patch.
> > >
> > > BTW, the above wording "the user space driver is responsible for
> > > acknowledging and re-enabling the interrupt" is misleading. The kernel
> > > always has to acknowledge/disable/mask the interrupt. Userspace can only
> > > reenable it, ideally by writing to a chip register. In some cornercases
> > > for broken hardware we need the newly introduced write function.
> > 
> > You seem to be mixing up masking/acknowledging the interrupt
> > controller and masking/acknowledging the actual hardware device. In
> > User IRQ Mode, the only thing the user space driver is accessing is
> > the hardware device, with the exception of write() to re-enable
> > interrupts which results in a enable_irq() that touches the interrupt
> > controller.
> But to be honest Hans is right here, the commit log wording is not
> optimal.  I suggest:
> 
> 	This patch adds a "User IRQ Mode" to UIO. In this mode the
> 	kernel space simply disables the serviced interrupt in the
> 	interrupt controller and the user space driver is responsible
> 	for acknowledging it in the device and reenabling it.
> 
> 	Note that this implies that the interrupt might be disabled for
> 	long periods, so this isn't usable for shared interrupt lines.
> 
> Maybe it's sensible to add the User IRQ Mode functions at least for now
> into platform code.  Then at a later time if and when there are several
> copies the discussion to move it to the generic part might be easier.

Thanks for this suggestion. I agree. Maybe we find a different solution
until then.

> 
> BTW, I currently have a situation where it IMHO really makes sense to
> use the User IRQ Mode:  We sell a cpu module to a customer with
> Linux.  I provide a uio device for some memory mapped periphal on the
> customers board that I don't know in detail.  With the User IRQ Mode I
> only need to know the chip select and the irq line, no further
> information is needed for the device.

The only additional information you need now is which bit in which
register you have to set/clear to mask the irq. I also have customer
chips here where this one information is all I know about the chip.

Thanks,
Hans

Re: [PATCH] uio: User IRQ Mode

[Hans J. Koch]

On Fri, Jul 04, 2008 at 01:03:21PM +0900, Magnus Damm wrote:
> On Thu, Jul 3, 2008 at 9:45 PM, Hans J. Koch <hjk@linutronix.de> wrote:
> > On Thu, Jul 03, 2008 at 09:10:19AM +0200, Uwe Kleine-K?nig wrote:
> >> Hans J. Koch wrote:
> >> > On Wed, Jul 02, 2008 at 07:59:51PM +0900, Magnus Damm wrote:
> >> > > From: Uwe Kleine-K?nig <Uwe.Kleine-Koenig@digi.com>
> >> > >
> >> > > This patch adds a "User IRQ Mode" to UIO. In this mode the user space driver
> >> > > is responsible for acknowledging and re-enabling the interrupt.
> >> >
> >> > This can easily be done without your patch.
> >
> > BTW, the above wording "the user space driver is responsible for
> > acknowledging and re-enabling the interrupt" is misleading. The kernel
> > always has to acknowledge/disable/mask the interrupt. Userspace can only
> > reenable it, ideally by writing to a chip register. In some cornercases
> > for broken hardware we need the newly introduced write function.
> 
> You seem to be mixing up masking/acknowledging the interrupt
> controller and masking/acknowledging the actual hardware device. In
> User IRQ Mode, the only thing the user space driver is accessing is
> the hardware device, with the exception of write() to re-enable
> interrupts which results in a enable_irq() that touches the interrupt
> controller.
> 
> I'm not sure what kind of hardware devices you are talking about, but
> I have hardware devices here on my desk that need to be acknowledged
> by the device-specific driver to deassert the irq. And acknowledging
> from user space works just fine.

Yes, I know such devices. If you _need_ to do it this way, it's simply
broken hardware design. You frequently find that in FPGAs designed by
industry amateurs. It can easily be handled by UIO as it is.

If you do it without need, then it's simply broken software design
because you use a kernel function which shows different behaviour on
different archs and machines where a simple hardware register access
would suffice.

> 
> >> > > Shared interrupts are not supported by this mode.
> >> > >
> >> > > Signed-off-by: Uwe Kleine-K?nig <Uwe.Kleine-Koenig@digi.com>
> >> > > Signed-off-by: Magnus Damm <damm@igel.co.jp>
> >> > > ---
> >> > >
> >> > >  Similar code has been posted some time ago as:
> >> > >  "[PATCH] uio_pdrv: Unique IRQ Mode"
> >> > >  "[PATCH 00/03][RFC] Reusable UIO Platform Driver".
> >> >
> >> > Yes, and in that thread I gave detailed explanations why I won't accept
> >> > that.
> >> I think for all of your concers one of the following is true:
> >>
> >>   - they are not valid any more in this version; or
> >
> > I question the whole concept as such. The concept of having a generic
> > irq handler using disable_irq_nosync() makes no sense at all.
> >
> > Reasons:
> >
> > - We do not introduce new possibilities. Everything can be done without
> >  that patch.
> 
> Isn't it possible to use the same argument against UIO? There is no
> point in having it since it is possible to do it all in kernel space
> instead. But for various reasons people want to write their drivers in
> user space.

That's why we have UIO, mainly to allow clean interrupt handling.

> 
> And for various reasons I'd like to acknowledge interrupts in user
> space. And I do that by disabling the interrupt in kernel space,
> notify the user space driver and acknowledge and re-enable the
> interrupt from there.

This is all possible with UIO as it is.

> The reason behind it is that I want to provide
> user space device driver people with consistent interface to the
> device without the need for any device specific code in kernel space.

And if it were a solution that was machine- and platform-independent and
worked with all kinds of interrupts, there wouldn't be much objections.

BTW, you still need to touch kernel code to setup the structs for
uio_pdrv, or did I misunderstand something? So where's the advantage?

> 
> > - We offer users an irq handler that shouldn't be used. It is seldom the
> >  best solution to call disable_irq_nosync() to disable an interrupt. In
> >  almost all cases you should use the irq mask register of the chip your
> >  driver handles. What do you want to write into the docs? Here's an irq
> >  handler, but please use it only if you're really desperate?
> 
> What is wrong with using disable_irq_nosync() compared to using the
> device specific irq mask register? It works just fine, and it provides
> a nice abstraction in my mind.

It's the difference between one write to a chip register compared to a
call to an arch specific kernel function. The performance and latency
issues might be negligible on some systems, but there's no guarantee. A
register access is one write on all platforms.

> 
> > - The only argument in favor of that concept was that it saves a few
> >  lines of irq handler code. Given the fact that all the handler has to
> >  do is toggle one bit in a hardware register, this is really not much.
> >  And you tempt people to delete 5 lines of good code and replace them
> >  with a sub-optimal generic irq handler.
> 
> What is suboptimal about it? And since when is a device specific
> solution better than a generic one?

Drivers are always device specific. And disabling or masking an
interrupt in the way the chip designer wanted you to do it is the
fastest, most elegant, and most natural thing for a driver to do.

> 
> > - You introduce the need for an irqcontrol function. This was not the
> >  intention of that concept. Normal UIO devices don't need a write
> >  function, this is only used for broken hardware. If you have normal
> >  hardware, and implement a proper 5 lines irq handler, userspace can simply
> >  reenable the irq by writing to a hardware register it has mapped
> >  anyway. In your concept, it has to use write() on /dev/uioX, which
> >  means you have to go all the way through libc, vfs, and the UIO core
> >  to finally call your irqcontrol function, which in turn calls
> >  enable_irq. As I said, there is broken hardware around where this is
> >  the only way, but most chips allow you to do it properly.
> 
> You are the one who posted the irqcontrol code because it solved some
> issues you are having with broken hardware. I'm happy to use your code
> in a generic way. Actually, my first version of this patch avoided the
> extra write() call overhead by re-enabling things automatically, but
> since your irqcontrol patch is cleaner I'd rather use that and live
> with the small performance penalty.
> 
> Performance in this case is really a non-issue.

Your talking about the board you've got in front of you. I'm talking
about the UIO framework and the 20+ platforms it supports.

> If we experience
> performance problems in the future because of high interrupt overhead
> then UIO is most likely no longer suitable for us. Right now it's a
> perfect match.

That's your personal experience.

> 
> >>   - I cannot understand it.
> >>
> >> I'll try to list them all below.  Please tell us if the list isn't
> >> complete or if my comments doesn't convince you.  You might have to
> >> repeat yourself, but for me it's hard to sort your arguments because
> >> Magnus' suggestion changed over time.
> >
> > OK, the version Magnus sent last is different, so some of my arguments
> > are superfluous now.
> >
> > Please, to make things simpler, let's only talk about the concept as
> > such and not go into implementation details. I deliberately do not review
> > that code (although I believe it has more bugs than the one Alan found),
> > because as long as the concept doesn't make sense, I don't care how it
> > is implemented.
> 
> I'd say it makes sense to Paul, Uwe and me.

Seems UIO gets tested intensively on SH at the moment :-)

Don't misunderstand me, even I can imagine cases where I _have_ to use
similar solutions. But that's no reason to patch it into the generic UIO
core. Because it should only be used if there's no other solution.
There's no point in inviting people to do it that way.

> 
> >> And please, I try to work out the pros and cons in a constructive way
> >> and hope there is nothing in it you will take personal.  It's really
> >> that I consider the patch valuable and don't understand your concerns.
> >
> > You both keep telling me how valuable that patch is but never answered my
> > question what the advantage would be. I cannot see it yet.
> 
> Avoid having device specific interrupts handlers in kernel space?

By replacing them with suboptimal generic handlers that work only in
special cases?

> 
> >>
> >> In the first thread[1] your unique and open concerns (to the best of my
> >> knowledge and belief) with my comments are:
> >>
> >>       - "This only works for embedded devices [...]"
> >>
> >>         OK, this doesn't work with shared IRQs which rules out x86.
> >>         I don't claim to know all the 23[2] other architectures but
> >>         IMHO if something is good for 3 archs and doesn't hurt the
> >>         other 21, you should do it.
> >>
> >>       - "This would save somebody the trouble to add the above 5 lines
> >>         to the 30 lines of board/platform support code he has to write
> >>         anyway. That's the only gain, and that is not enough."
> >>
> >>         IMHO it's worth it.  Because if you add the five lines to a
> >>         central place you save 5 lines per platform using the driver.
> >
> > OK, that is one argument in favor. I always admitted that, but said that
> > this is not enough to compensate for the disadvantages mentioned above.
> 
> I still fail to see your technical arguments.
> 
> >>         Moreover this might prevent some bugs.  (And obviously this
> >>         function has the potential to have a buggy implementation as
> >>         the comment by Alan Cox shows.)
> >
> > For me, this shows two things:
> >
> > - I never ever had to use disable_irq_nosync() in any UIO driver yet,
> >  otherwise I would have noticed.
> 
> Is that so? Given the stunning UIO patch throughput it wouldn't
> surprise me if people end up using local patches instead of even
> trying to merge things upstream. Which means you wouldn't see the
> code.
> 
> > - Magnus turned in a patch that he never tested.
> 
> Is that so?
> 
> I've tested the patch using two different processors on three
> different boards. I have written a user space uio driver that
> interfaces to mplayer using vidix, providing hardware accelerated
> color space conversion and stretching. So don't call my patch untested
> please.

OK, I apologize for that statement.

> 
> I plan on submitting the vidix driver upstream soon after the uio
> interface gets stabilized, ie this patch gets merged.

I'm sure you get your vidix driver to work without this patch.

> 
> >>
> >>       - "And if you _know_ that on your platform the irq is not
> >>         shared, this might really be a one-liner that simply calls
> >>         irq_disable. That's OK in board specific code, but not in a
> >>         generic driver."
> >>
> >>         Please note that the patch only introduces a helper that the
> >>         platform code *can* use.  You still have the freedom not to
> >>         use it without any overhead.
> >
> > It's not a good idea to add nonsense code and tell the users to ignore
> > it whenever they can...
> 
> This nonsense code, exactly which code are you talking about?

The concept of having a generic irq handler that works only with
non-shared interrupts.

> Could it
> be one of your significant kernel contributions? =)

You think I have some? Thank you ;-)

> 
> >>
> >>       - "I won't accept anything that changes the current UIO
> >>         behaviour."
> >>
> >>         Not valid anymore.  There is no change in behaviour.
> >
> > Well, at least the whole stuff would have to be explained in the docs.
> 
> Sure, documentation is always good.
> 
> > You add an element to struct uio_info, together with a new #define. And
> > a whole class of drivers using that stuff would have a write() function
> > without needing it.
> 
> Another way to see it is that a whole class of drivers would need
> kernel space interrupt handling code without really needing it.
> 
> >>
> >> In the second thread[3] I cannot find any open concerns that are not
> >> already listed above.
> >>
> >> > >  Changes since Uwe's last version:
> >> > >   - flags should be unsigned long
> >> > >   - simplify uio_userirq_handler()
> >> >
> >> > That's nearly nothing. All you do is sending the same stuff three weeks
> >> > later in the hope somebody will merge it this time. NAK.
> >> I think nobody really is surprised that you're not happy with the new
> >> post.  But note that Magnus just did what you told him. ("I'm [not] the
> >> big boss who makes the final decision. I can be critized and overridden.
> >> If Greg loves your patch and merges it, fine. Try it.")
> >>
> >> In the hope not to have kicked off a flame,
> >
> > Oh, no, stay cool ;-)
> 
> Good, let's stay cool.

Thanks,
Hans

Re: [PATCH] uio: User IRQ Mode

[Magnus Damm]

On Fri, Jul 4, 2008 at 10:26 PM, Hans J. Koch <hjk@linutronix.de> wrote:
> On Fri, Jul 04, 2008 at 01:03:21PM +0900, Magnus Damm wrote:
>> On Thu, Jul 3, 2008 at 9:45 PM, Hans J. Koch <hjk@linutronix.de> wrote:
>> > On Thu, Jul 03, 2008 at 09:10:19AM +0200, Uwe Kleine-K?nig wrote:
>> >> Hans J. Koch wrote:
>> >> > On Wed, Jul 02, 2008 at 07:59:51PM +0900, Magnus Damm wrote:
>> >> > > From: Uwe Kleine-K?nig <Uwe.Kleine-Koenig@digi.com>
>> >> > >
>> >> > > This patch adds a "User IRQ Mode" to UIO. In this mode the user space driver
>> >> > > is responsible for acknowledging and re-enabling the interrupt.
>> >> >
>> >> > This can easily be done without your patch.
>> >
>> > BTW, the above wording "the user space driver is responsible for
>> > acknowledging and re-enabling the interrupt" is misleading. The kernel
>> > always has to acknowledge/disable/mask the interrupt. Userspace can only
>> > reenable it, ideally by writing to a chip register. In some cornercases
>> > for broken hardware we need the newly introduced write function.
>>
>> You seem to be mixing up masking/acknowledging the interrupt
>> controller and masking/acknowledging the actual hardware device. In
>> User IRQ Mode, the only thing the user space driver is accessing is
>> the hardware device, with the exception of write() to re-enable
>> interrupts which results in a enable_irq() that touches the interrupt
>> controller.
>>
>> I'm not sure what kind of hardware devices you are talking about, but
>> I have hardware devices here on my desk that need to be acknowledged
>> by the device-specific driver to deassert the irq. And acknowledging
>> from user space works just fine.
>
> Yes, I know such devices. If you _need_ to do it this way, it's simply
> broken hardware design. You frequently find that in FPGAs designed by
> industry amateurs. It can easily be handled by UIO as it is.

This is crap. I'm sure you're having all sorts of problems with broken
FPGAs, but that's outside the scope of this discussion. If a device
needs to be acknowledged or not to deassert the interrupt line is
totally device specific and it has nothing to do with broken hardware
design.

> If you do it without need, then it's simply broken software design
> because you use a kernel function which shows different behaviour on
> different archs and machines where a simple hardware register access
> would suffice.

As much as I enjoy arguing with you, we are doing this because there
is need for it. =)

>> >> > > Shared interrupts are not supported by this mode.
>> >> > >
>> >> > > Signed-off-by: Uwe Kleine-K?nig <Uwe.Kleine-Koenig@digi.com>
>> >> > > Signed-off-by: Magnus Damm <damm@igel.co.jp>
>> >> > > ---
>> >> > >
>> >> > >  Similar code has been posted some time ago as:
>> >> > >  "[PATCH] uio_pdrv: Unique IRQ Mode"
>> >> > >  "[PATCH 00/03][RFC] Reusable UIO Platform Driver".
>> >> >
>> >> > Yes, and in that thread I gave detailed explanations why I won't accept
>> >> > that.
>> >> I think for all of your concers one of the following is true:
>> >>
>> >>   - they are not valid any more in this version; or
>> >
>> > I question the whole concept as such. The concept of having a generic
>> > irq handler using disable_irq_nosync() makes no sense at all.
>> >
>> > Reasons:
>> >
>> > - We do not introduce new possibilities. Everything can be done without
>> >  that patch.
>>
>> Isn't it possible to use the same argument against UIO? There is no
>> point in having it since it is possible to do it all in kernel space
>> instead. But for various reasons people want to write their drivers in
>> user space.
>
> That's why we have UIO, mainly to allow clean interrupt handling.

Yes, and that's fine.

>> And for various reasons I'd like to acknowledge interrupts in user
>> space. And I do that by disabling the interrupt in kernel space,
>> notify the user space driver and acknowledge and re-enable the
>> interrupt from there.
>
> This is all possible with UIO as it is.

It's not a matter if it's possible or not. It's a matter of abstraction.

>> The reason behind it is that I want to provide
>> user space device driver people with consistent interface to the
>> device without the need for any device specific code in kernel space.
>
> And if it were a solution that was machine- and platform-independent and
> worked with all kinds of interrupts, there wouldn't be much objections.

Exactly what do you mean by "so much objections"? You're the only one
blocking this, and it looks to me like you're just pushing your own
policy without thinking outside your little device driver box.

I'm not doing this because i want to change how UIO is working in
general or push some generic solution and force everyone to use it. I
_only_ want to export _our_ devices. If other people want to export
their devices the same way, then that's great. Uwe seems to want to do
that. Why do you feel you have to stop us? It's pretty natural for you
to have no insight in we are doing on the SuperH side, and because of
that you should just take our word for what we need and work with us
to resolve technical issues.

> BTW, you still need to touch kernel code to setup the structs for
> uio_pdrv, or did I misunderstand something? So where's the advantage?

The advantage is that we don't need to be aware of device-specific
details in the kernel.

You seem to be happy with the usual UIO way of splitting the driver
code in a kernel stub and a user space driver. Good for you, but we
want to skip the device specific kernel stub.

For our needs - on-chip memory mapped hardware blocks - we'd like to
replace the kernel stub with a single reusable implementation, and
that way we end up with a single per-hardware block specific driver -
in user space. This way we can let people with RTOS experience do
their thing in user space, without worrying about them overwriting our
precious kernel data structures.

You are right that we still need to feed the platform driver
information about the device. We only want to pass the base address of
the hardware block and the interrupt number. There is no point for us
to be aware about the inner workings of the hardware block - we only
want to export the thing to user space and be done with it. And in
some rare cases we don't even have documentation for a certain block,
so acknowledging interrupts from inside the kernel is out of the
question.

>> > - We offer users an irq handler that shouldn't be used. It is seldom the
>> >  best solution to call disable_irq_nosync() to disable an interrupt. In
>> >  almost all cases you should use the irq mask register of the chip your
>> >  driver handles. What do you want to write into the docs? Here's an irq
>> >  handler, but please use it only if you're really desperate?
>>
>> What is wrong with using disable_irq_nosync() compared to using the
>> device specific irq mask register? It works just fine, and it provides
>> a nice abstraction in my mind.
>
> It's the difference between one write to a chip register compared to a
> call to an arch specific kernel function. The performance and latency
> issues might be negligible on some systems, but there's no guarantee. A
> register access is one write on all platforms.

Wrong. It's not always that easy. The way to acknowledge interrupts is
very device specific. A good in-kernel example of not so easy
acknowledging would be drivers/input/touchscreen/ads7846.c. Look at
the interrupt handler code ads7846_irq() - what does it do? Exactly
what the User IRQ Mode is doing - using disable_irq(). The reason for
this is slow SPI bus speed. It takes a long time to write to the touch
screen controller over the serial bus, and the interrupt line will
remain asserted until the device has been acknowledged properly.

>> > - The only argument in favor of that concept was that it saves a few
>> >  lines of irq handler code. Given the fact that all the handler has to
>> >  do is toggle one bit in a hardware register, this is really not much.
>> >  And you tempt people to delete 5 lines of good code and replace them
>> >  with a sub-optimal generic irq handler.
>>
>> What is suboptimal about it? And since when is a device specific
>> solution better than a generic one?
>
> Drivers are always device specific. And disabling or masking an
> interrupt in the way the chip designer wanted you to do it is the
> fastest, most elegant, and most natural thing for a driver to do.

Again, exactly how to acknowledge an interrupt is device or hardware
block specific. And it's not always as easy as writing to a single
register. Of course we will acknowledge the device in the way the chip
designer want us to, but we will do it from user space.

>> > - You introduce the need for an irqcontrol function. This was not the
>> >  intention of that concept. Normal UIO devices don't need a write
>> >  function, this is only used for broken hardware. If you have normal
>> >  hardware, and implement a proper 5 lines irq handler, userspace can simply
>> >  reenable the irq by writing to a hardware register it has mapped
>> >  anyway. In your concept, it has to use write() on /dev/uioX, which
>> >  means you have to go all the way through libc, vfs, and the UIO core
>> >  to finally call your irqcontrol function, which in turn calls
>> >  enable_irq. As I said, there is broken hardware around where this is
>> >  the only way, but most chips allow you to do it properly.
>>
>> You are the one who posted the irqcontrol code because it solved some
>> issues you are having with broken hardware. I'm happy to use your code
>> in a generic way. Actually, my first version of this patch avoided the
>> extra write() call overhead by re-enabling things automatically, but
>> since your irqcontrol patch is cleaner I'd rather use that and live
>> with the small performance penalty.
>>
>> Performance in this case is really a non-issue.
>
> Your talking about the board you've got in front of you. I'm talking
> about the UIO framework and the 20+ platforms it supports.

I'm not forcing you to use my code on your platforms. Feel free to use
it if it does what you want to do, if not you should just keep on
using your regular code. How can this be my problem? Don't NAK just
because the code isn't suitable for all your needs. I'm not trying to
solve your needs. I'm posting this code because _we_ need it.

>> > Please, to make things simpler, let's only talk about the concept as
>> > such and not go into implementation details. I deliberately do not review
>> > that code (although I believe it has more bugs than the one Alan found),
>> > because as long as the concept doesn't make sense, I don't care how it
>> > is implemented.
>>
>> I'd say it makes sense to Paul, Uwe and me.
>
> Seems UIO gets tested intensively on SH at the moment :-)

No, you are wrong. It really doesn't get tested. Mainly because you
are refusing to accept my code. UIO _would_ be used and tested if we
can agree on these 50 lines of code and get on with our lives.

> Don't misunderstand me, even I can imagine cases where I _have_ to use
> similar solutions. But that's no reason to patch it into the generic UIO
> core. Because it should only be used if there's no other solution.
> There's no point in inviting people to do it that way.

You still haven't given any real technical reason why it's better to
acknowledge in kernel space over acknowledging in user space. Having
all hardware block / device specific code in a single place will keep
interfaces clean and make maintainance easier for us.

SuperH hardware just happens to use unique interrupt lines for most
internal hardware blocks, and we'd like to take advantage of that and
skip the entire UIO kernel stub thing.

>> >> And please, I try to work out the pros and cons in a constructive way
>> >> and hope there is nothing in it you will take personal.  It's really
>> >> that I consider the patch valuable and don't understand your concerns.
>> >
>> > You both keep telling me how valuable that patch is but never answered my
>> > question what the advantage would be. I cannot see it yet.
>>
>> Avoid having device specific interrupts handlers in kernel space?
>
> By replacing them with suboptimal generic handlers that work only in
> special cases?

There is nothing suboptimal about it.

>> > - Magnus turned in a patch that he never tested.
>>
>> Is that so?
>>
>> I've tested the patch using two different processors on three
>> different boards. I have written a user space uio driver that
>> interfaces to mplayer using vidix, providing hardware accelerated
>> color space conversion and stretching. So don't call my patch untested
>> please.
>
> OK, I apologize for that statement.

Thank you.

>> I plan on submitting the vidix driver upstream soon after the uio
>> interface gets stabilized, ie this patch gets merged.
>
> I'm sure you get your vidix driver to work without this patch.

It is and has been working for more than a month. It's not suitable
for submission yet though, mainly since the UIO kernel interface is
unstable until we've agreed on this patch. The kernel interface
changed with the write() re-enabling method, remember? And I feel this
may happen again since it seems very hard for us to agree on things.

>> > It's not a good idea to add nonsense code and tell the users to ignore
>> > it whenever they can...
>>
>> This nonsense code, exactly which code are you talking about?
>
> The concept of having a generic irq handler that works only with
> non-shared interrupts.

That may look like nonsense to you, but you fail to see the big picture if so.

>> Could it
>> be one of your significant kernel contributions? =)
>
> You think I have some? Thank you ;-)

No, I don't think so. But I do think you should work a bit on
understanding your position in the community and pay closer attention
to people with more experience than you. I may understand that you
feel the need to argue with me - since I only have a few small
contributions behind me - but when you're not listening to what Paul
has to say then you're just stupid. Stop ignoring people above you in
the food chain.

You can use git shortlog to get statistics.

/ magnus