[PATCH] uio_pdrv: Unique IRQ Mode

[PATCH] uio_pdrv: Unique IRQ Mode

[Magnus Damm]

From: Magnus Damm <damm@igel.co.jp>

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

Signed-off-by: Magnus Damm <damm@igel.co.jp>
---

Think of this as V2 of "[PATCH 00/03][RFC] Reusable UIO Platform Driver".
Needs "[PATCH 0/1] UIO: Add a write() function to enable/disable interrupts"

 drivers/uio/uio_pdrv.c |   43 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

--- 0006/drivers/uio/uio_pdrv.c
+++ work/drivers/uio/uio_pdrv.c	2008-06-04 14:51:56.000000000 +0900
@@ -16,8 +16,34 @@
 
 struct uio_platdata {
 	struct uio_info *uioinfo;
+	unsigned long irq_disabled;
 };
 
+static irqreturn_t uio_pdrv_unique_handler(int irq, struct uio_info *dev_info)
+{
+	struct uio_platdata *priv = dev_info->priv;
+
+	/* In "Unique IRQ Mode", just disable the interrupt and remember
+	 * the state so we can enable it later.
+	 */
+	disable_irq(irq);
+	set_bit(0, &priv->irq_disabled);
+	return IRQ_HANDLED;
+}
+
+static int uio_pdrv_unique_irqcontrol(struct uio_info *dev_info, s32 irq_on)
+{
+	struct uio_platdata *priv = dev_info->priv;
+
+	/* "Unique IRQ Mode" allows re-enabling of the interrupt */
+	if (irq_on && test_and_clear_bit(0, &priv->irq_disabled)) {
+		enable_irq(dev_info->irq);
+		return 0;
+	}
+
+	return -ENOSYS;
+}
+
 static int uio_pdrv_probe(struct platform_device *pdev)
 {
 	struct uio_info *uioinfo = pdev->dev.platform_data;
@@ -68,6 +94,23 @@ static int uio_pdrv_probe(struct platfor
 
 	pdata->uioinfo->priv = pdata;
 
+	/* This driver supports a special "Unique IRQ Mode".
+	 *
+	 * In this mode, no hardware specific kernel code is required to
+	 * acknowledge interrupts. Instead, the interrupt is disabled by
+	 * the interrupt handler. User space is responsible for performing
+	 * hardware specific acknowledge and enabling of interrupts.
+	 *
+	 * Interrupt sharing is _not_ supported by the "Unique IRQ Mode".
+	 *
+	 * Enable this mode by passing IRQ number but omitting irq callbacks.
+	 */
+	if (!uioinfo->handler && !uioinfo->irqcontrol && uioinfo->irq >= 0) {
+		uioinfo->irq_flags = IRQF_DISABLED;
+		uioinfo->handler = uio_pdrv_unique_handler;
+		uioinfo->irqcontrol = uio_pdrv_unique_irqcontrol;
+	}
+
 	ret = uio_register_device(&pdev->dev, pdata->uioinfo);
 
 	if (ret) {

Re: [PATCH] uio_pdrv: Unique IRQ Mode

[Hans J. Koch]

On Wed, Jun 04, 2008 at 03:08:26PM +0900, Magnus Damm wrote:
> From: Magnus Damm <damm@igel.co.jp>
> 
> This patch adds a "Unique IRQ Mode" to the uio_pdrv UIO platform driver.
> In this mode the user space driver is responsible for acknowledging and
> re-enabling the interrupt. Shared interrupts are not supported.

I still don't see any gain in this. This only works for embedded
devices, so a user has to setup hardware specific code in his board
support anyway. With your code, we would have to add something like this
to the docs:

IF you define an irq AND ommit the irq handler THEN we silently add a
handler that blindly assumes the irq is not shared...

In my opinion, this is confusing, and all it does is saving the need for a
three-lines irq handler in the board support.

So, NAK to this until somebody convinces me that I completely missed the
point.

Thanks,
Hans

> 
> Signed-off-by: Magnus Damm <damm@igel.co.jp>
> ---
> 
> Think of this as V2 of "[PATCH 00/03][RFC] Reusable UIO Platform Driver".
> Needs "[PATCH 0/1] UIO: Add a write() function to enable/disable interrupts"
> 
>  drivers/uio/uio_pdrv.c |   43 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 43 insertions(+)
> 
> --- 0006/drivers/uio/uio_pdrv.c
> +++ work/drivers/uio/uio_pdrv.c	2008-06-04 14:51:56.000000000 +0900
> @@ -16,8 +16,34 @@
>  
>  struct uio_platdata {
>  	struct uio_info *uioinfo;
> +	unsigned long irq_disabled;
>  };
>  
> +static irqreturn_t uio_pdrv_unique_handler(int irq, struct uio_info *dev_info)
> +{
> +	struct uio_platdata *priv = dev_info->priv;
> +
> +	/* In "Unique IRQ Mode", just disable the interrupt and remember
> +	 * the state so we can enable it later.
> +	 */
> +	disable_irq(irq);
> +	set_bit(0, &priv->irq_disabled);
> +	return IRQ_HANDLED;
> +}
> +
> +static int uio_pdrv_unique_irqcontrol(struct uio_info *dev_info, s32 irq_on)
> +{
> +	struct uio_platdata *priv = dev_info->priv;
> +
> +	/* "Unique IRQ Mode" allows re-enabling of the interrupt */
> +	if (irq_on && test_and_clear_bit(0, &priv->irq_disabled)) {
> +		enable_irq(dev_info->irq);
> +		return 0;
> +	}
> +
> +	return -ENOSYS;
> +}
> +
>  static int uio_pdrv_probe(struct platform_device *pdev)
>  {
>  	struct uio_info *uioinfo = pdev->dev.platform_data;
> @@ -68,6 +94,23 @@ static int uio_pdrv_probe(struct platfor
>  
>  	pdata->uioinfo->priv = pdata;
>  
> +	/* This driver supports a special "Unique IRQ Mode".
> +	 *
> +	 * In this mode, no hardware specific kernel code is required to
> +	 * acknowledge interrupts. Instead, the interrupt is disabled by
> +	 * the interrupt handler. User space is responsible for performing
> +	 * hardware specific acknowledge and enabling of interrupts.
> +	 *
> +	 * Interrupt sharing is _not_ supported by the "Unique IRQ Mode".
> +	 *
> +	 * Enable this mode by passing IRQ number but omitting irq callbacks.
> +	 */
> +	if (!uioinfo->handler && !uioinfo->irqcontrol && uioinfo->irq >= 0) {
> +		uioinfo->irq_flags = IRQF_DISABLED;
> +		uioinfo->handler = uio_pdrv_unique_handler;
> +		uioinfo->irqcontrol = uio_pdrv_unique_irqcontrol;
> +	}
> +
>  	ret = uio_register_device(&pdev->dev, pdata->uioinfo);
>  
>  	if (ret) {

Re: [PATCH] uio_pdrv: Unique IRQ Mode

[Magnus Damm]

On Wed, Jun 4, 2008 at 7:11 PM, Hans J. Koch <hjk@linutronix.de> wrote:
> On Wed, Jun 04, 2008 at 03:08:26PM +0900, Magnus Damm wrote:
>> From: Magnus Damm <damm@igel.co.jp>
>>
>> This patch adds a "Unique IRQ Mode" to the uio_pdrv UIO platform driver.
>> In this mode the user space driver is responsible for acknowledging and
>> re-enabling the interrupt. Shared interrupts are not supported.
>
> I still don't see any gain in this. This only works for embedded
> devices, so a user has to setup hardware specific code in his board
> support anyway.

Exactly what in my patch makes this platform driver only suitable for
embedded devices?

> With your code, we would have to add something like this
> to the docs:
>
> IF you define an irq AND ommit the irq handler THEN we silently add a
> handler that blindly assumes the irq is not shared...

I'm not sure if silently and blindly are the first words that pop into
my mind, but sure. Documentation is a good idea. Just let me know
which uio_pdrv document you want me to modify.

> In my opinion, this is confusing, and all it does is saving the need for a
> three-lines irq handler in the board support.

You propose that I put the callbacks in my board support code instead
of modifying the driver. I don't think the board support level is the
proper place for this code. The patch contains no board specific code,
and it is independent of both architecture and cpu model.

> So, NAK to this until somebody convinces me that I completely missed the
> point.

We can reuse this driver for _many_ different SuperH processor models.
Most of these processor models even have more than one hardware block
that can be exported to user space using this uio_pdrv driver in
"Unique IRQ Mode". There is nothing board specific with this at all,
so yes, I think you are missing the point.

Maybe you prefer that I repost my "Reusable UIO Platform Driver"
instead of modifying uio_pdrv?

/ magnus

Re: [PATCH] uio_pdrv: Unique IRQ Mode

[Uwe Kleine-König]

Hello Magnus,

Magnus Damm wrote:
> ... "Unique IRQ Mode". ...
BTW, I wouldn't call it "Unique IRQ Mode" because the non-shared irq is
only a result from automatically disabling the irq.  IMHO something like
"No IRQ Handler Mode" is more suitable.

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_pdrv: Unique IRQ Mode

[Magnus Damm]

Hi Uwe,

On Thu, Jun 5, 2008 at 3:49 PM, Uwe Kleine-König
<Uwe.Kleine-Koenig@digi.com> wrote:
> Hello Magnus,
>
> Magnus Damm wrote:
>> ... "Unique IRQ Mode". ...
> BTW, I wouldn't call it "Unique IRQ Mode" because the non-shared irq is
> only a result from automatically disabling the irq.  IMHO something like
> "No IRQ Handler Mode" is more suitable.

That may be a good idea. Any name is fine with me.

Hopefully a better name will make people less confused. =)

Thanks!

/ magnus

Re: [PATCH] uio_pdrv: Unique IRQ Mode

[Hans J. Koch]

On Fri, Jun 06, 2008 at 11:55:30AM +0900, Magnus Damm wrote:
> Hi Uwe,
> 
> On Thu, Jun 5, 2008 at 3:49 PM, Uwe Kleine-König
> <Uwe.Kleine-Koenig@digi.com> wrote:
> > Hello Magnus,
> >
> > Magnus Damm wrote:
> >> ... "Unique IRQ Mode". ...
> > BTW, I wouldn't call it "Unique IRQ Mode" because the non-shared irq is
> > only a result from automatically disabling the irq.  IMHO something like
> > "No IRQ Handler Mode" is more suitable.
> 
> That may be a good idea. Any name is fine with me.
> 
> Hopefully a better name will make people less confused. =)

I didn't criticize the name. My objection is that you want to introduce
an optional special handling for cases where the interrupt line is not
shared, e.g. a GPIO line. In that case, the handler _could_ look like
this:

static irqreturn_t my_handler(int irq, struct uio_info *info)
{
	irq_disable(MY_GPIO_LINE);
	return IRQ_HANDLED;
}

This solution is only second best, if possible, the irq should be
properly acknowledged within the chip, but it could be done like this.
Note that this doesn't work on every platform, it assumes that each GPIO
line has its own irq number. (Did you ever think about fixing Kconfig so
that this option is disabled on platforms where it is not possible or
not sensible to do this?)

You now suggest that if somebody doesn't fill in an irq handler, we
should make the above the default. 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.

Giving it a different name doesn't make it better.

Thanks,
Hans

Re: [PATCH] uio_pdrv: Unique IRQ Mode

[Magnus Damm]

On Fri, Jun 6, 2008 at 7:04 PM, Hans J. Koch <hjk@linutronix.de> wrote:
> On Fri, Jun 06, 2008 at 11:55:30AM +0900, Magnus Damm wrote:
>> On Thu, Jun 5, 2008 at 3:49 PM, Uwe Kleine-König
>> <Uwe.Kleine-Koenig@digi.com> wrote:
>> > Magnus Damm wrote:
>> >> ... "Unique IRQ Mode". ...
>> > BTW, I wouldn't call it "Unique IRQ Mode" because the non-shared irq is
>> > only a result from automatically disabling the irq.  IMHO something like
>> > "No IRQ Handler Mode" is more suitable.
>>
>> That may be a good idea. Any name is fine with me.
>>
>> Hopefully a better name will make people less confused. =)
>
> I didn't criticize the name. My objection is that you want to introduce
> an optional special handling for cases where the interrupt line is not
> shared, e.g. a GPIO line. In that case, the handler _could_ look like
> this:

First of all, I don't really see how this is related to GPIO. The
hardware blocks I'm exporting to user space are media accelerator
blocks - for instance providing hardware color space conversion. Any
type of device can be exported using this technique as long as it is
memory mapped and it comes with a unique interrupt.

> static irqreturn_t my_handler(int irq, struct uio_info *info)
> {
>        irq_disable(MY_GPIO_LINE);
>        return IRQ_HANDLED;
> }

Why did you remove the atomic_set()? I suggest that you try out the
code, or maybe even look at the irq_enable()/irq_disable()
implementation in kernel/irq/manage.c. Hint: the tricky part is
"desc->depth" in __enable_irq(). Basically, you shouldn't enable the
the interrupt more times than you disable it. So we need to keep track
of things and prevent user space from mucking up the depth for us.

> This solution is only second best, if possible, the irq should be
> properly acknowledged within the chip, but it could be done like this.

Of course the interrupt should be acknowledged at some point. It's
just a question of when and where you do that. I can't see the reason
of having device specific UIO code in kernel space when you can have a
generic kernel space driver and let user space do everything.

> Note that this doesn't work on every platform, it assumes that each GPIO
> line has its own irq number. (Did you ever think about fixing Kconfig so
> that this option is disabled on platforms where it is not possible or
> not sensible to do this?)

Is MY_GPIO_LINE a Kconfig option? Using the "irq" parameter makes more
sense for us since we want to have multiple platform UIO instances up
and running simultaneously.

Would you like me to wrap the above lines in #ifdefs?

> You now suggest that if somebody doesn't fill in an irq handler, we
> should make the above the default. 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.

But the code in this patch is reusable. Any platform that has a unique
interrupt assigned to a memory mapped device can just export it to
user space as a regular platform driver. No other kernel space code
needed.

Maybe 5 lines doesn't matter to you, but we want to use this for many
different devices. Maybe we'll start with 5 devices and work our way
from there.

We can of course just do this somewhere under the arch/sh tree, but
why should we make the code architecture specific when it's something
that many architectures can make use of?

> Giving it a different name doesn't make it better.

So exactly what makes it better?

Thank you.

/ magnus

Re: [PATCH] uio_pdrv: Unique IRQ Mode

[Hans J. Koch]

On Thu, Jun 05, 2008 at 10:25:27AM +0900, Magnus Damm wrote:
> On Wed, Jun 4, 2008 at 7:11 PM, Hans J. Koch <hjk@linutronix.de> wrote:
> > On Wed, Jun 04, 2008 at 03:08:26PM +0900, Magnus Damm wrote:
> >> From: Magnus Damm <damm@igel.co.jp>
> >>
> >> This patch adds a "Unique IRQ Mode" to the uio_pdrv UIO platform driver.
> >> In this mode the user space driver is responsible for acknowledging and
> >> re-enabling the interrupt. Shared interrupts are not supported.
> >
> > I still don't see any gain in this. This only works for embedded
> > devices, so a user has to setup hardware specific code in his board
> > support anyway.
> 
> Exactly what in my patch makes this platform driver only suitable for
> embedded devices?

You assume the interrupt is not shared. You can never do that on a
normal x86 PC, for example. E.g. for a PCI card you don't know which irq
it'll get and if it is shared or not.

> 
> > With your code, we would have to add something like this
> > to the docs:
> >
> > IF you define an irq AND ommit the irq handler THEN we silently add a
> > handler that blindly assumes the irq is not shared...
> 
> I'm not sure if silently and blindly are the first words that pop into
> my mind, but sure. Documentation is a good idea. Just let me know
> which uio_pdrv document you want me to modify.

Stuff that is useful for other driver writers and not only for userspace
people should be added to Documentation/Docbook/uio_howto.tmpl.
Unfortunately, I forgot to ask Uwe to do this, so I'll probably have to
do it myself one day ;-)

> 
> > In my opinion, this is confusing, and all it does is saving the need for a
> > three-lines irq handler in the board support.
> 
> You propose that I put the callbacks in my board support code instead
> of modifying the driver.

That's what uio_pdrv does.

> I don't think the board support level is the
> proper place for this code.

You have to write code there anyway, e.g. code that configures your GPIO
as input, makes it generate interrupts and so on. And of course, you
have to setup your platform device as well. If you simply add the irq
handler, you can use uio_pdrv as-is. 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.

> The patch contains no board specific code,
> and it is independent of both architecture and cpu model.

Every platform device driver depends on board support.

> 
> > So, NAK to this until somebody convinces me that I completely missed the
> > point.
> 
> We can reuse this driver for _many_ different SuperH processor models.
> Most of these processor models even have more than one hardware block
> that can be exported to user space using this uio_pdrv driver in
> "Unique IRQ Mode". There is nothing board specific with this at all,
> so yes, I think you are missing the point.

First, I won't accept anything that changes the current UIO behaviour.
If uio_info->irq is not set, then uio_register_device will fail. That's
it. Your patch redefines the meaning of irq-not-set if uio_pdrv is used.

Second, if we decide to introduce new UIO capabilities, there must be an
obvious advantage. E.g., uio_pdrv saves the trouble of writing almost
identical UIO platform drivers over and over again. A programmer only
needs to touch the board support file he has to set up anyway which
makes his board support patches simpler and easier to maintain. Your
patch adds code that is not obvious, just to save a few lines of board
support code. It doesn't add new possibilities, everything can be done
with uio_pdrv alone.

> 
> Maybe you prefer that I repost my "Reusable UIO Platform Driver"
> instead of modifying uio_pdrv?

If you have an idea that adds new possibilities, feel free to post it.
If you come up with something that just adds confusion but no
advantages, I'll never accept it.

Thanks,
Hans

Re: [PATCH] uio_pdrv: Unique IRQ Mode

[Magnus Damm]

On Thu, Jun 5, 2008 at 6:09 PM, Hans J. Koch <hjk@linutronix.de> wrote:
> On Thu, Jun 05, 2008 at 10:25:27AM +0900, Magnus Damm wrote:
>> On Wed, Jun 4, 2008 at 7:11 PM, Hans J. Koch <hjk@linutronix.de> wrote:
>> > On Wed, Jun 04, 2008 at 03:08:26PM +0900, Magnus Damm wrote:
>> >> From: Magnus Damm <damm@igel.co.jp>
>> >>
>> >> This patch adds a "Unique IRQ Mode" to the uio_pdrv UIO platform driver.
>> >> In this mode the user space driver is responsible for acknowledging and
>> >> re-enabling the interrupt. Shared interrupts are not supported.
>> >
>> > I still don't see any gain in this. This only works for embedded
>> > devices, so a user has to setup hardware specific code in his board
>> > support anyway.
>>
>> Exactly what in my patch makes this platform driver only suitable for
>> embedded devices?
>
> You assume the interrupt is not shared. You can never do that on a
> normal x86 PC, for example. E.g. for a PCI card you don't know which irq
> it'll get and if it is shared or not.

So your main objection against this patch is that you cannot use it
with shared interrupts?

>> > With your code, we would have to add something like this
>> > to the docs:
>> >
>> > IF you define an irq AND ommit the irq handler THEN we silently add a
>> > handler that blindly assumes the irq is not shared...
>>
>> I'm not sure if silently and blindly are the first words that pop into
>> my mind, but sure. Documentation is a good idea. Just let me know
>> which uio_pdrv document you want me to modify.
>
> Stuff that is useful for other driver writers and not only for userspace
> people should be added to Documentation/Docbook/uio_howto.tmpl.
> Unfortunately, I forgot to ask Uwe to do this, so I'll probably have to
> do it myself one day ;-)

Right.

>> > In my opinion, this is confusing, and all it does is saving the need for a
>> > three-lines irq handler in the board support.
>>
>> You propose that I put the callbacks in my board support code instead
>> of modifying the driver.
>
> That's what uio_pdrv does.

Yes.

>> I don't think the board support level is the
>> proper place for this code.
>
> You have to write code there anyway, e.g. code that configures your GPIO
> as input, makes it generate interrupts and so on. And of course, you
> have to setup your platform device as well. If you simply add the irq
> handler, you can use uio_pdrv as-is. 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.

Ever heard about system on chip? Not all platform devices need board
specific setup.

>> The patch contains no board specific code,
>> and it is independent of both architecture and cpu model.
>
> Every platform device driver depends on board support.

Is that so? I suggest that you have a look at the mfd drivers and think again.

>> > So, NAK to this until somebody convinces me that I completely missed the
>> > point.
>>
>> We can reuse this driver for _many_ different SuperH processor models.
>> Most of these processor models even have more than one hardware block
>> that can be exported to user space using this uio_pdrv driver in
>> "Unique IRQ Mode". There is nothing board specific with this at all,
>> so yes, I think you are missing the point.
>
> First, I won't accept anything that changes the current UIO behaviour.
> If uio_info->irq is not set, then uio_register_device will fail. That's
> it. Your patch redefines the meaning of irq-not-set if uio_pdrv is used.

How is this changing the UIO behavior? I'm modifying the uio_pdrv
driver, which is a driver that you didn't even write yourself. And yet
you seem to have very strong feelings against this patch.

> Second, if we decide to introduce new UIO capabilities, there must be an
> obvious advantage. E.g., uio_pdrv saves the trouble of writing almost
> identical UIO platform drivers over and over again. A programmer only
> needs to touch the board support file he has to set up anyway which
> makes his board support patches simpler and easier to maintain. Your
> patch adds code that is not obvious, just to save a few lines of board
> support code. It doesn't add new possibilities, everything can be done
> with uio_pdrv alone.

Again, it's not board support code. And it's pretty obvious.

>> Maybe you prefer that I repost my "Reusable UIO Platform Driver"
>> instead of modifying uio_pdrv?
>
> If you have an idea that adds new possibilities, feel free to post it.
> If you come up with something that just adds confusion but no
> advantages, I'll never accept it.

You don't have to accept it. I'll just repost my original driver and
let someone else merge it then.

/ magnus

Re: [PATCH] uio_pdrv: Unique IRQ Mode

[Hans J. Koch]

On Thu, Jun 05, 2008 at 06:46:35PM +0900, Magnus Damm wrote:
> On Thu, Jun 5, 2008 at 6:09 PM, Hans J. Koch <hjk@linutronix.de> wrote:
> > On Thu, Jun 05, 2008 at 10:25:27AM +0900, Magnus Damm wrote:
> >> On Wed, Jun 4, 2008 at 7:11 PM, Hans J. Koch <hjk@linutronix.de> wrote:
> >> > On Wed, Jun 04, 2008 at 03:08:26PM +0900, Magnus Damm wrote:
> >> >> From: Magnus Damm <damm@igel.co.jp>
> >> >>
> >> >> This patch adds a "Unique IRQ Mode" to the uio_pdrv UIO platform driver.
> >> >> In this mode the user space driver is responsible for acknowledging and
> >> >> re-enabling the interrupt. Shared interrupts are not supported.
> >> >
> >> > I still don't see any gain in this. This only works for embedded
> >> > devices, so a user has to setup hardware specific code in his board
> >> > support anyway.
> >>
> >> Exactly what in my patch makes this platform driver only suitable for
> >> embedded devices?
> >
> > You assume the interrupt is not shared. You can never do that on a
> > normal x86 PC, for example. E.g. for a PCI card you don't know which irq
> > it'll get and if it is shared or not.
> 
> So your main objection against this patch is that you cannot use it
> with shared interrupts?

I think I've explained my objections detailed enough.

> 
> >> > With your code, we would have to add something like this
> >> > to the docs:
> >> >
> >> > IF you define an irq AND ommit the irq handler THEN we silently add a
> >> > handler that blindly assumes the irq is not shared...
> >>
> >> I'm not sure if silently and blindly are the first words that pop into
> >> my mind, but sure. Documentation is a good idea. Just let me know
> >> which uio_pdrv document you want me to modify.
> >
> > Stuff that is useful for other driver writers and not only for userspace
> > people should be added to Documentation/Docbook/uio_howto.tmpl.
> > Unfortunately, I forgot to ask Uwe to do this, so I'll probably have to
> > do it myself one day ;-)
> 
> Right.
> 
> >> > In my opinion, this is confusing, and all it does is saving the need for a
> >> > three-lines irq handler in the board support.
> >>
> >> You propose that I put the callbacks in my board support code instead
> >> of modifying the driver.
> >
> > That's what uio_pdrv does.
> 
> Yes.
> 
> >> I don't think the board support level is the
> >> proper place for this code.
> >
> > You have to write code there anyway, e.g. code that configures your GPIO
> > as input, makes it generate interrupts and so on. And of course, you
> > have to setup your platform device as well. If you simply add the irq
> > handler, you can use uio_pdrv as-is. 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.
> 
> Ever heard about system on chip?

ATM, I work with iMX31 and AT91SAM9263, before that I had a PXA270,
can't remember what was before that...
So yes, I've heard of SoC.

> Not all platform devices need board
> specific setup.

If it's a device within the SoC, you won't use UIO for that. If you did,
your platform would depend on certain userspace software which is simply
crap. And devices outside the SoC are board specific.

> 
> >> The patch contains no board specific code,
> >> and it is independent of both architecture and cpu model.
> >
> > Every platform device driver depends on board support.
> 
> Is that so? I suggest that you have a look at the mfd drivers and think again.

All I said about board support also applies to platform support files
like at91sam9263_devices.c, I'm simply talking about the file where you
define your struct platform_device.

> 
> >> > So, NAK to this until somebody convinces me that I completely missed the
> >> > point.
> >>
> >> We can reuse this driver for _many_ different SuperH processor models.
> >> Most of these processor models even have more than one hardware block
> >> that can be exported to user space using this uio_pdrv driver in
> >> "Unique IRQ Mode". There is nothing board specific with this at all,
> >> so yes, I think you are missing the point.
> >
> > First, I won't accept anything that changes the current UIO behaviour.
> > If uio_info->irq is not set, then uio_register_device will fail. That's
> > it. Your patch redefines the meaning of irq-not-set if uio_pdrv is used.
> 
> How is this changing the UIO behavior? I'm modifying the uio_pdrv
> driver, which is a driver that you didn't even write yourself.

uio_pdrv is a generic driver, so I consider it part of the UIO
framework. It adds new possibilities for authors of UIO platform device
drivers (which are the vast majority of all UIO drivers). It is not just
another UIO driver, but part of the system. It'll appear in UIO
documentation, I'll explain it in future UIO presentations, and so on.

And I consider it my job to make sure that such generic code is clean,
obvious, and consistent.

> And yet
> you seem to have very strong feelings against this patch.

I explained why. My reasons are purely technical, please don't take this
as a personal offense.

> 
> > Second, if we decide to introduce new UIO capabilities, there must be an
> > obvious advantage. E.g., uio_pdrv saves the trouble of writing almost
> > identical UIO platform drivers over and over again. A programmer only
> > needs to touch the board support file he has to set up anyway which
> > makes his board support patches simpler and easier to maintain. Your
> > patch adds code that is not obvious, just to save a few lines of board
> > support code. It doesn't add new possibilities, everything can be done
> > with uio_pdrv alone.
> 
> Again, it's not board support code. And it's pretty obvious.
> 
> >> Maybe you prefer that I repost my "Reusable UIO Platform Driver"
> >> instead of modifying uio_pdrv?
> >
> > If you have an idea that adds new possibilities, feel free to post it.
> > If you come up with something that just adds confusion but no
> > advantages, I'll never accept it.
> 
> You don't have to accept it. I'll just repost my original driver and
> let someone else merge it then.

Unfortunately, I'm one of the two UIO maintainers, so I feel obliged to
review your patch and give my opinion. That doesn't mean I'm
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.

Thanks,
Hans

Re: [PATCH] uio_pdrv: Unique IRQ Mode

[Magnus Damm]

On Thu, Jun 5, 2008 at 8:27 PM, Hans J. Koch <hjk@linutronix.de> wrote:
> On Thu, Jun 05, 2008 at 06:46:35PM +0900, Magnus Damm wrote:
>> On Thu, Jun 5, 2008 at 6:09 PM, Hans J. Koch <hjk@linutronix.de> wrote:
>> > On Thu, Jun 05, 2008 at 10:25:27AM +0900, Magnus Damm wrote:
>> >> On Wed, Jun 4, 2008 at 7:11 PM, Hans J. Koch <hjk@linutronix.de> wrote:
>> >> > On Wed, Jun 04, 2008 at 03:08:26PM +0900, Magnus Damm wrote:
>> >> >> From: Magnus Damm <damm@igel.co.jp>
>> >> >>
>> >> >> This patch adds a "Unique IRQ Mode" to the uio_pdrv UIO platform driver.
>> >> >> In this mode the user space driver is responsible for acknowledging and
>> >> >> re-enabling the interrupt. Shared interrupts are not supported.
>> >> >
>> >> > I still don't see any gain in this. This only works for embedded
>> >> > devices, so a user has to setup hardware specific code in his board
>> >> > support anyway.
>> >>
>> >> Exactly what in my patch makes this platform driver only suitable for
>> >> embedded devices?
>> >
>> > You assume the interrupt is not shared. You can never do that on a
>> > normal x86 PC, for example. E.g. for a PCI card you don't know which irq
>> > it'll get and if it is shared or not.
>>
>> So your main objection against this patch is that you cannot use it
>> with shared interrupts?
>
> I think I've explained my objections detailed enough.

It's still unclear to me. Please make a brief summary of your objections.

>> >> I don't think the board support level is the
>> >> proper place for this code.
>> >
>> > You have to write code there anyway, e.g. code that configures your GPIO
>> > as input, makes it generate interrupts and so on. And of course, you
>> > have to setup your platform device as well. If you simply add the irq
>> > handler, you can use uio_pdrv as-is. 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.
>>
>> Ever heard about system on chip?
>
> ATM, I work with iMX31 and AT91SAM9263, before that I had a PXA270,
> can't remember what was before that...
> So yes, I've heard of SoC.
>
>> Not all platform devices need board
>> specific setup.
>
> If it's a device within the SoC, you won't use UIO for that. If you did,
> your platform would depend on certain userspace software which is simply
> crap. And devices outside the SoC are board specific.

Why wouldn't we use UIO for device within the Soc? I've been doing
that for quite some time now.

>> >> The patch contains no board specific code,
>> >> and it is independent of both architecture and cpu model.
>> >
>> > Every platform device driver depends on board support.
>>
>> Is that so? I suggest that you have a look at the mfd drivers and think again.
>
> All I said about board support also applies to platform support files
> like at91sam9263_devices.c, I'm simply talking about the file where you
> define your struct platform_device.

Oh, I see. That's cpu specific code in my mind.

>> >> > So, NAK to this until somebody convinces me that I completely missed the
>> >> > point.
>> >>
>> >> We can reuse this driver for _many_ different SuperH processor models.
>> >> Most of these processor models even have more than one hardware block
>> >> that can be exported to user space using this uio_pdrv driver in
>> >> "Unique IRQ Mode". There is nothing board specific with this at all,
>> >> so yes, I think you are missing the point.
>> >
>> > First, I won't accept anything that changes the current UIO behaviour.
>> > If uio_info->irq is not set, then uio_register_device will fail. That's
>> > it. Your patch redefines the meaning of irq-not-set if uio_pdrv is used.
>>
>> How is this changing the UIO behavior? I'm modifying the uio_pdrv
>> driver, which is a driver that you didn't even write yourself.
>
> uio_pdrv is a generic driver, so I consider it part of the UIO
> framework. It adds new possibilities for authors of UIO platform device
> drivers (which are the vast majority of all UIO drivers). It is not just
> another UIO driver, but part of the system. It'll appear in UIO
> documentation, I'll explain it in future UIO presentations, and so on.
>
> And I consider it my job to make sure that such generic code is clean,
> obvious, and consistent.

Would you like me to write longer comments? Or some slides?

>> And yet
>> you seem to have very strong feelings against this patch.
>
> I explained why. My reasons are purely technical, please don't take this
> as a personal offense.

No offense taken. But I can't really see your technical arguments. If
something in my code is unclear please ask before NAK.

> Unfortunately, I'm one of the two UIO maintainers, so I feel obliged to
> review your patch and give my opinion. That doesn't mean I'm
> 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.

Maybe I will. =)

Thank you.

/ magnus

Re: [PATCH] uio_pdrv: Unique IRQ Mode

[Hans J. Koch]

On Sun, Jun 08, 2008 at 07:19:25PM +0900, Magnus Damm wrote:
> >>
> >> So your main objection against this patch is that you cannot use it
> >> with shared interrupts?
> >
> > I think I've explained my objections detailed enough.
> 
> It's still unclear to me. Please make a brief summary of your objections.

The objection is that your code offers no advantages. What can we do
with your patch applied that we cannot do with uio_pdrv alone?

> >
> > If it's a device within the SoC, you won't use UIO for that. If you did,
> > your platform would depend on certain userspace software which is simply
> > crap. And devices outside the SoC are board specific.
> 
> Why wouldn't we use UIO for device within the Soc? 

Can't you read? I've answered that in the lines above your question.

> I've been doing
> that for quite some time now.

Really? I haven't seen any such driver yet. IMO, support for everything
inside a SoC should be completely within the kernel, I wouldn't do it
with UIO. But it's up to the arch maintainer to decide that. Did you ask
him?

> 
> >> >> The patch contains no board specific code,
> >> >> and it is independent of both architecture and cpu model.
> >> >
> >> > Every platform device driver depends on board support.
> >>
> >> Is that so? I suggest that you have a look at the mfd drivers and think again.
> >
> > All I said about board support also applies to platform support files
> > like at91sam9263_devices.c, I'm simply talking about the file where you
> > define your struct platform_device.
> 
> Oh, I see. That's cpu specific code in my mind.

It's completely unimportant if your struct platform_device appears in
platform-, board-, or cpu-support. Or elsewhere. I won't let myself be
tricked into a discussion about terms.

> 
> >> >> > So, NAK to this until somebody convinces me that I completely missed the
> >> >> > point.
> >> >>
> >> >> We can reuse this driver for _many_ different SuperH processor models.
> >> >> Most of these processor models even have more than one hardware block
> >> >> that can be exported to user space using this uio_pdrv driver in
> >> >> "Unique IRQ Mode". There is nothing board specific with this at all,
> >> >> so yes, I think you are missing the point.
> >> >
> >> > First, I won't accept anything that changes the current UIO behaviour.
> >> > If uio_info->irq is not set, then uio_register_device will fail. That's
> >> > it. Your patch redefines the meaning of irq-not-set if uio_pdrv is used.
> >>
> >> How is this changing the UIO behavior? I'm modifying the uio_pdrv
> >> driver, which is a driver that you didn't even write yourself.
> >
> > uio_pdrv is a generic driver, so I consider it part of the UIO
> > framework. It adds new possibilities for authors of UIO platform device
> > drivers (which are the vast majority of all UIO drivers). It is not just
> > another UIO driver, but part of the system. It'll appear in UIO
> > documentation, I'll explain it in future UIO presentations, and so on.
> >
> > And I consider it my job to make sure that such generic code is clean,
> > obvious, and consistent.
> 
> Would you like me to write longer comments? Or some slides?

;-)

> 
> >> And yet
> >> you seem to have very strong feelings against this patch.
> >
> > I explained why. My reasons are purely technical, please don't take this
> > as a personal offense.
> 
> No offense taken. But I can't really see your technical arguments. 

Once more (for the last time): The technical argument against your patch
is that it offers no advantages. It makes other code more complicated,
less obvious, and less consistent. This might be OK if we had big
advantages in return, but this isn't the case.
Furthermore, your approach works only on certain platforms. But that
doesn't matter anymore, because the above is already enough to NAK your
patch. We won't add code just because it could be done.

> If something in my code is unclear please ask before NAK.

If I post a patch, it is my job to make it clear what advantages we have
if we apply it. And no, nothing is unclear with your patch.

> 
> > Unfortunately, I'm one of the two UIO maintainers, so I feel obliged to
> > review your patch and give my opinion. That doesn't mean I'm
> > 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.
> 
> Maybe I will. =)

Did you notice that in this thread nobody spoke up to support your
patch?

Thanks,
Hans

Re: [PATCH] uio_pdrv: Unique IRQ Mode

[Magnus Damm]

On Mon, Jun 9, 2008 at 5:54 AM, Hans J. Koch <hjk@linutronix.de> wrote:
> On Sun, Jun 08, 2008 at 07:19:25PM +0900, Magnus Damm wrote:
>> >>
>> >> So your main objection against this patch is that you cannot use it
>> >> with shared interrupts?
>> >
>> > I think I've explained my objections detailed enough.
>>
>> It's still unclear to me. Please make a brief summary of your objections.
>
> The objection is that your code offers no advantages. What can we do
> with your patch applied that we cannot do with uio_pdrv alone?

Yes, it is possible to have board or architecture specific hooks, but
does that really make sense if the code is generic and can be reused
by multiple architectures? I say it doesn't make sense at all.

>> > If it's a device within the SoC, you won't use UIO for that. If you did,
>> > your platform would depend on certain userspace software which is simply
>> > crap. And devices outside the SoC are board specific.
>>
>> Why wouldn't we use UIO for device within the Soc?
>
> Can't you read? I've answered that in the lines above your question.

I'm sure there are blocks within the SoC that must be managed by the
kernel, but that's not always the case. Certain things can be managed
by user space just fine. For instance, video acceleration hardware.

>> I've been doing
>> that for quite some time now.
>
> Really? I haven't seen any such driver yet. IMO, support for everything
> inside a SoC should be completely within the kernel, I wouldn't do it
> with UIO. But it's up to the arch maintainer to decide that. Did you ask
> him?

Regarding driver source, I have posted a user space test driver here:

http://article.gmane.org/gmane.linux.ports.sh.devel/3927

As for kernel driver source, you have it earlier in this thread. I'm
planning on pushing my user space VIDIX driver upstream, but I'd like
to get the kernel parts merged first or at least acked. This UIO
specific piece of the puzzle unfortunately seems to take forever.
Which really is a shame, because it's all very simple.

>> >> >> The patch contains no board specific code,
>> >> >> and it is independent of both architecture and cpu model.
>> >> >
>> >> > Every platform device driver depends on board support.
>> >>
>> >> Is that so? I suggest that you have a look at the mfd drivers and think again.
>> >
>> > All I said about board support also applies to platform support files
>> > like at91sam9263_devices.c, I'm simply talking about the file where you
>> > define your struct platform_device.
>>
>> Oh, I see. That's cpu specific code in my mind.
>
> It's completely unimportant if your struct platform_device appears in
> platform-, board-, or cpu-support. Or elsewhere. I won't let myself be
> tricked into a discussion about terms.

No of course, please keep on referring to everything outside of
drivers/ as board code.

>> >> >> > So, NAK to this until somebody convinces me that I completely missed the
>> >> >> > point.
>> >> >>
>> >> >> We can reuse this driver for _many_ different SuperH processor models.
>> >> >> Most of these processor models even have more than one hardware block
>> >> >> that can be exported to user space using this uio_pdrv driver in
>> >> >> "Unique IRQ Mode". There is nothing board specific with this at all,
>> >> >> so yes, I think you are missing the point.
>> >> >
>> >> > First, I won't accept anything that changes the current UIO behaviour.
>> >> > If uio_info->irq is not set, then uio_register_device will fail. That's
>> >> > it. Your patch redefines the meaning of irq-not-set if uio_pdrv is used.
>> >>
>> >> How is this changing the UIO behavior? I'm modifying the uio_pdrv
>> >> driver, which is a driver that you didn't even write yourself.
>> >
>> > uio_pdrv is a generic driver, so I consider it part of the UIO
>> > framework. It adds new possibilities for authors of UIO platform device
>> > drivers (which are the vast majority of all UIO drivers). It is not just
>> > another UIO driver, but part of the system. It'll appear in UIO
>> > documentation, I'll explain it in future UIO presentations, and so on.
>> >
>> > And I consider it my job to make sure that such generic code is clean,
>> > obvious, and consistent.
>>
>> Would you like me to write longer comments? Or some slides?
>
> ;-)
>
>>
>> >> And yet
>> >> you seem to have very strong feelings against this patch.
>> >
>> > I explained why. My reasons are purely technical, please don't take this
>> > as a personal offense.
>>
>> No offense taken. But I can't really see your technical arguments.
>
> Once more (for the last time): The technical argument against your patch
> is that it offers no advantages. It makes other code more complicated,
> less obvious, and less consistent. This might be OK if we had big
> advantages in return, but this isn't the case.

I say:
a) We need this for 5+ different SuperH hardware blocks.
b) This approach can be used by most architectures.
c) The code is architecture independent.

You say "it offers no advantages".

> Furthermore, your approach works only on certain platforms. But that
> doesn't matter anymore, because the above is already enough to NAK your
> patch. We won't add code just because it could be done.
>
>> If something in my code is unclear please ask before NAK.
>
> If I post a patch, it is my job to make it clear what advantages we have
> if we apply it. And no, nothing is unclear with your patch.

I'm glad to hear that you understand it all.

>>
>> > Unfortunately, I'm one of the two UIO maintainers, so I feel obliged to
>> > review your patch and give my opinion. That doesn't mean I'm
>> > 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.
>>
>> Maybe I will. =)
>
> Did you notice that in this thread nobody spoke up to support your
> patch?

Maybe no one really cares about arguing about a few lines of code? And
I mean, what is it to argue about - it's not exactly rocket science.

Thanks for your help.

/ magnus

Re: [PATCH] uio_pdrv: Unique IRQ Mode

[Hans J. Koch]

On Mon, Jun 09, 2008 at 10:12:09AM +0900, Magnus Damm wrote:
> >
> > The objection is that your code offers no advantages. What can we do
> > with your patch applied that we cannot do with uio_pdrv alone?
> 
> Yes, it is possible to have board or architecture specific hooks, but
> does that really make sense if the code is generic and can be reused
> by multiple architectures? I say it doesn't make sense at all.

So that's your answer when I ask what your code has to offer?

> 
> >> > If it's a device within the SoC, you won't use UIO for that. If you did,
> >> > your platform would depend on certain userspace software which is simply
> >> > crap. And devices outside the SoC are board specific.
> >>
> >> Why wouldn't we use UIO for device within the Soc?
> >
> > Can't you read? I've answered that in the lines above your question.
> 
> I'm sure there are blocks within the SoC that must be managed by the
> kernel, but that's not always the case. Certain things can be managed
> by user space just fine. For instance, video acceleration hardware.

There are standard kernel subsystems to handle such devices. You should
only make it a UIO driver if v4l, drm, fb doesn't work for some
important reason.

> 
> >> I've been doing
> >> that for quite some time now.
> >
> > Really? I haven't seen any such driver yet. IMO, support for everything
> > inside a SoC should be completely within the kernel, I wouldn't do it
> > with UIO. But it's up to the arch maintainer to decide that. Did you ask
> > him?
> 
> Regarding driver source, I have posted a user space test driver here:
> 
> http://article.gmane.org/gmane.linux.ports.sh.devel/3927
> 
> As for kernel driver source, you have it earlier in this thread. I'm
> planning on pushing my user space VIDIX driver upstream, but I'd like
> to get the kernel parts merged first or at least acked. This UIO
> specific piece of the puzzle unfortunately seems to take forever.
> Which really is a shame, because it's all very simple.

Why do you need that stuff that works only with irqs that are not
shared? Why don't you simply do it with uio_pdrv? Or a normal UIO
driver?
After a brief look over your userspace code, I cannot see why you need
any additions to the UIO core code.

> > Once more (for the last time): The technical argument against your patch
> > is that it offers no advantages. It makes other code more complicated,
> > less obvious, and less consistent. This might be OK if we had big
> > advantages in return, but this isn't the case.
> 
> I say:
> a) We need this for 5+ different SuperH hardware blocks.
> b) This approach can be used by most architectures.
> c) The code is architecture independent.
> 
> You say "it offers no advantages".

Yes, and your answer is the proof.

Thanks,
Hans

Re: [PATCH] uio_pdrv: Unique IRQ Mode

[Paul Mundt]

On Mon, Jun 09, 2008 at 10:44:12AM +0200, Hans J. Koch wrote:
> On Mon, Jun 09, 2008 at 10:12:09AM +0900, Magnus Damm wrote:
> > >> > If it's a device within the SoC, you won't use UIO for that. If you did,
> > >> > your platform would depend on certain userspace software which is simply
> > >> > crap. And devices outside the SoC are board specific.
> > >>
> > >> Why wouldn't we use UIO for device within the Soc?
> > >
> > > Can't you read? I've answered that in the lines above your question.
> > 
> > I'm sure there are blocks within the SoC that must be managed by the
> > kernel, but that's not always the case. Certain things can be managed
> > by user space just fine. For instance, video acceleration hardware.
> 
> There are standard kernel subsystems to handle such devices. You should
> only make it a UIO driver if v4l, drm, fb doesn't work for some
> important reason.
> 
It's _precisely_ because the kernel is not the place to manage the
devices in question that UIO is being used in the first place. Some
things simply don't belong in the kernel. This has been stated several
times. Are you purposely only selectively reading this thread?

> > > Once more (for the last time): The technical argument against your patch
> > > is that it offers no advantages. It makes other code more complicated,
> > > less obvious, and less consistent. This might be OK if we had big
> > > advantages in return, but this isn't the case.
> > 
> > I say:
> > a) We need this for 5+ different SuperH hardware blocks.
> > b) This approach can be used by most architectures.
> > c) The code is architecture independent.
> > 
> > You say "it offers no advantages".
> 
> Yes, and your answer is the proof.
> 
Given that stunning technical rebuttal, the constructive way forward
seems to be to follow Uwe's suggestions and respin the patch.

Re: [PATCH] uio_pdrv: Unique IRQ Mode

[Uwe Kleine-König]

Hello Paul,

Paul Mundt wrote:
>                                   [...] the constructive way forward
> seems to be to follow Uwe's suggestions and respin the patch.
Just to let you know: I'm willing to look on a new version.  And I like
"User IRQ Mode".

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_pdrv: Unique IRQ Mode

[Greg KH]

On Mon, Jun 09, 2008 at 02:34:06PM +0200, Uwe Kleine-K?nig wrote:
> Hello Paul,
> 
> Paul Mundt wrote:
> >                                   [...] the constructive way forward
> > seems to be to follow Uwe's suggestions and respin the patch.
> Just to let you know: I'm willing to look on a new version.  And I like
> "User IRQ Mode".


Yes, please, someone respin the patch as I am totally confused as to
what everyone is arguing about :)

thanks,

greg k-h

Re: [PATCH] uio_pdrv: Unique IRQ Mode

[Magnus Damm]

Hi Uwe,

Thanks for your help.

On Mon, Jun 9, 2008 at 9:34 PM, Uwe Kleine-König
<Uwe.Kleine-Koenig@digi.com> wrote:
> Paul Mundt wrote:
>>                                   [...] the constructive way forward
>> seems to be to follow Uwe's suggestions and respin the patch.
> Just to let you know: I'm willing to look on a new version.  And I like
> "User IRQ Mode".

Sure, let's go with that name then.

But shall we really go down the path with separate exported functions?
I think we discussed that earlier. Why I'm asking is that this adds
extra complexity - function prototypes in header files,
EXPORT_SYMBOL() stuff and more lines of code needed for each driver
instance.

Isn't it enough to add a kconfig option for CONFIG_UIO_PDRV_USER_IRQ
which depends on CONFIG_SUPERH (if we really want that) and wrap the
code in CONFIG_UIO_PDRV_USER_IRQ?

Let me know what you want and I'll respin the patch. Thanks!

/ magnus

Re: [PATCH] uio_pdrv: Unique IRQ Mode

[Uwe Kleine-König]

Magnus Damm wrote:
> Hi Uwe,
> 
> Thanks for your help.
> 
> On Mon, Jun 9, 2008 at 9:34 PM, Uwe Kleine-König
> <Uwe.Kleine-Koenig@digi.com> wrote:
> > Paul Mundt wrote:
> >>                                   [...] the constructive way forward
> >> seems to be to follow Uwe's suggestions and respin the patch.
> > Just to let you know: I'm willing to look on a new version.  And I like
> > "User IRQ Mode".
> 
> Sure, let's go with that name then.
> 
> But shall we really go down the path with separate exported functions?
> I think we discussed that earlier. Why I'm asking is that this adds
> extra complexity - function prototypes in header files,
> EXPORT_SYMBOL() stuff and more lines of code needed for each driver
> instance.
Well, at this point I agree with Hans that being explicit here is
preferable.

I think you can implement the handler and irqcontrol functions static
inline.  Then the need to statically compile uio goes away.  And if all
uio_pdrv devices are registered in the same .c file you don't even have
a (binary) size penalty.

> Isn't it enough to add a kconfig option for CONFIG_UIO_PDRV_USER_IRQ
> which depends on CONFIG_SUPERH (if we really want that) and wrap the
> code in CONFIG_UIO_PDRV_USER_IRQ?
No, I definitely don't like that.  Your last patch is completly
compatible with the current implementation.  It only uses a situation
(i.e. registering a uio_pdrv with no handler) a feature.  So
CONFIG_UIO_PDRV_USER_IRQ wouldn't break anything, only not having it.
So it should be enabled by default and then you don't need it.
(Note, this should not count as a +1 for implementing it without the
Kconfig symbol.)
 
> Let me know what you want and I'll respin the patch. Thanks!
... I started describing in detail my suggestion, but actually it's
easier to do the patch myself.  So here it comes.  Note it's not even
compile tested, but you should be able to get my intentions.

As a reply on this mail I will send a small cleanup patch that should
not conflict with any of the pending patches for uio.

Best regards
Uwe


From: Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>
Date: Tue, 10 Jun 2008 09:06:24 +0200
Subject: [PATCH] UIO: wip

Signed-off-by: Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>
---
 include/linux/uio_driver.h |   33 +++++++++++++++++++++++++++++++++
 1 files changed, 33 insertions(+), 0 deletions(-)

diff --git a/include/linux/uio_driver.h b/include/linux/uio_driver.h
index cdf338d..671a7a8 100644
--- a/include/linux/uio_driver.h
+++ b/include/linux/uio_driver.h
@@ -63,6 +63,7 @@ struct uio_info {
 	long			irq;
 	unsigned long		irq_flags;
 	void			*priv;
+	unsigned int		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 +93,36 @@ extern void uio_event_notify(struct uio_info *info);
 #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;
+
+	if (likely(dev_info->irqcontrol)) {
+		ret = dev_info->irqcontrol(dev_info, 0);
+		if (ret) {
+			pr_warning("%s: failed to disable irq\n", __func__);
+			return IRQ_NONE;
+		} else
+			return IRQ_HANDLED;
+	} else
+		return IRQ_NONE;
+}
+
+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_ */
-- 
1.5.5.3


-- 
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

[PATCH] UIO: minor style and comment fixes

[Uwe Kleine-König]

Signed-off-by: Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>
---
 include/linux/uio_driver.h |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/linux/uio_driver.h b/include/linux/uio_driver.h
index cf65e96..cdf338d 100644
--- a/include/linux/uio_driver.h
+++ b/include/linux/uio_driver.h
@@ -36,7 +36,7 @@ struct uio_mem {
 	struct uio_map		*map;
 };
 
-#define MAX_UIO_MAPS 	5
+#define MAX_UIO_MAPS	5
 
 struct uio_device;
 
@@ -82,11 +82,11 @@ static inline int __must_check
 extern void uio_unregister_device(struct uio_info *info);
 extern void uio_event_notify(struct uio_info *info);
 
-/* defines for uio_device->irq */
+/* defines for uio_info->irq */
 #define UIO_IRQ_CUSTOM	-1
 #define UIO_IRQ_NONE	-2
 
-/* defines for uio_device->memtype */
+/* defines for uio_mem->memtype */
 #define UIO_MEM_NONE	0
 #define UIO_MEM_PHYS	1
 #define UIO_MEM_LOGICAL	2
-- 
1.5.5.3

Re: [PATCH] UIO: minor style and comment fixes

[Hans J. Koch]

On Tue, Jun 10, 2008 at 09:14:48AM +0200, Uwe Kleine-König wrote:
> Signed-off-by: Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>

Signed-off-by: Hans J. Koch <hjk@linutronix.de>

> ---
>  include/linux/uio_driver.h |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/uio_driver.h b/include/linux/uio_driver.h
> index cf65e96..cdf338d 100644
> --- a/include/linux/uio_driver.h
> +++ b/include/linux/uio_driver.h
> @@ -36,7 +36,7 @@ struct uio_mem {
>  	struct uio_map		*map;
>  };
>  
> -#define MAX_UIO_MAPS 	5
> +#define MAX_UIO_MAPS	5
>  
>  struct uio_device;
>  
> @@ -82,11 +82,11 @@ static inline int __must_check
>  extern void uio_unregister_device(struct uio_info *info);
>  extern void uio_event_notify(struct uio_info *info);
>  
> -/* defines for uio_device->irq */
> +/* defines for uio_info->irq */
>  #define UIO_IRQ_CUSTOM	-1
>  #define UIO_IRQ_NONE	-2
>  
> -/* defines for uio_device->memtype */
> +/* defines for uio_mem->memtype */
>  #define UIO_MEM_NONE	0
>  #define UIO_MEM_PHYS	1
>  #define UIO_MEM_LOGICAL	2
> -- 
> 1.5.5.3

Re: [PATCH] uio_pdrv: Unique IRQ Mode

[Magnus Damm]

On Tue, Jun 10, 2008 at 4:10 PM, Uwe Kleine-König
<Uwe.Kleine-Koenig@digi.com> wrote:
> I think you can implement the handler and irqcontrol functions static
> inline.  Then the need to statically compile uio goes away.  And if all
> uio_pdrv devices are registered in the same .c file you don't even have
> a (binary) size penalty.

Good idea, that's nice and clean.

>> Let me know what you want and I'll respin the patch. Thanks!
> ... I started describing in detail my suggestion, but actually it's
> easier to do the patch myself.  So here it comes.  Note it's not even
> compile tested, but you should be able to get my intentions.

Excellent! I wrote some code to export the VEU in sh7722 on top of
your patch and things work just fine. Slightly more structure members
to fill in with this approach, but that's fine. This less automatic
approach hopefully causes less confusion. =)

> From: Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>
> Date: Tue, 10 Jun 2008 09:06:24 +0200
> Subject: [PATCH] UIO: wip
>
> Signed-off-by: Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>

Acked-by: Magnus Damm <damm@igel.co.jp>

Thanks for your help!

/ magnus

Re: [PATCH] uio_pdrv: Unique IRQ Mode

[Paul Mundt]

On Tue, Jun 10, 2008 at 09:10:32AM +0200, Uwe Kleine-K?nig wrote:
> Signed-off-by: Uwe Kleine-K?nig <Uwe.Kleine-Koenig@digi.com>
> ---
>  include/linux/uio_driver.h |   33 +++++++++++++++++++++++++++++++++
>  1 files changed, 33 insertions(+), 0 deletions(-)
> 
> diff --git a/include/linux/uio_driver.h b/include/linux/uio_driver.h
> index cdf338d..671a7a8 100644
> --- a/include/linux/uio_driver.h
> +++ b/include/linux/uio_driver.h
> @@ -63,6 +63,7 @@ struct uio_info {
>  	long			irq;
>  	unsigned long		irq_flags;
>  	void			*priv;
> +	unsigned int		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);

This should be unsigned long.

> @@ -92,4 +93,36 @@ extern void uio_event_notify(struct uio_info *info);
>  #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;
> +
> +	if (likely(dev_info->irqcontrol)) {
> +		ret = dev_info->irqcontrol(dev_info, 0);
> +		if (ret) {
> +			pr_warning("%s: failed to disable irq\n", __func__);
> +			return IRQ_NONE;
> +		} else
> +			return IRQ_HANDLED;
> +	} else
> +		return IRQ_NONE;
> +}
> +
You can simplify this by just using IRQ_RETVAL() on
dev_info->irqcontrol(...). The printk() shouldn't be necessary, as the
other layers should already catch that.

Re: [PATCH] uio_pdrv: Unique IRQ Mode

[Uwe Kleine-König]

Hello Paul,

> > diff --git a/include/linux/uio_driver.h b/include/linux/uio_driver.h
> > index cdf338d..671a7a8 100644
> > --- a/include/linux/uio_driver.h
> > +++ b/include/linux/uio_driver.h
> > @@ -63,6 +63,7 @@ struct uio_info {
> >       long                    irq;
> >       unsigned long           irq_flags;
> >       void                    *priv;
> > +     unsigned int            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);
> 
> This should be unsigned long.
Is this only because test_and_set_bit takes an unsigned long?  Or is a
long type preferable in general?

> > +static inline irqreturn_t uio_userirq_handler(int irq,
> > +             struct uio_info *dev_info)
> > +{
> > +     int ret;
> > +
> > +     if (likely(dev_info->irqcontrol)) {
> > +             ret = dev_info->irqcontrol(dev_info, 0);
> > +             if (ret) {
> > +                     pr_warning("%s: failed to disable irq\n", __func__);
> > +                     return IRQ_NONE;
> > +             } else
> > +                     return IRQ_HANDLED;
> > +     } else
> > +             return IRQ_NONE;
> > +}
> > +
> You can simplify this by just using IRQ_RETVAL() on
> dev_info->irqcontrol(...). The printk() shouldn't be necessary, as the
> other layers should already catch that.
Right, we can make this:

	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);
	}

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_pdrv: Unique IRQ Mode

[Paul Mundt]

On Sun, Jun 08, 2008 at 10:54:57PM +0200, Hans J. Koch wrote:
> On Sun, Jun 08, 2008 at 07:19:25PM +0900, Magnus Damm wrote:
> > >>
> > >> So your main objection against this patch is that you cannot use it
> > >> with shared interrupts?
> > >
> > > I think I've explained my objections detailed enough.
> > 
> > It's still unclear to me. Please make a brief summary of your objections.
> 
> The objection is that your code offers no advantages. What can we do
> with your patch applied that we cannot do with uio_pdrv alone?
> 
Re-use it across similar devices, without endless duplication for one.
This point has been re-iterated multiple times in these threads, yet
seems to be completely ignored each time it is brought up. There is zero
point in reproducing the same code over and over and over again for each
CPU variant, especially when the existing UIO framework is a pretty good
match for the problem.

This is behaviour that we both need and wish to take advantage of through
UIO. You constantly advocate uio_pdrv as the one-stop solution, which
might even fit this case if the unique IRQ mode was factored in to it. If
you're not able to live with that, then a separate driver is necessary.

> > > If it's a device within the SoC, you won't use UIO for that. If you did,
> > > your platform would depend on certain userspace software which is simply
> > > crap. And devices outside the SoC are board specific.
> > 
> > Why wouldn't we use UIO for device within the Soc? 
> 
> Can't you read? I've answered that in the lines above your question.
> 
> > I've been doing
> > that for quite some time now.
> 
> Really? I haven't seen any such driver yet. IMO, support for everything
> inside a SoC should be completely within the kernel, I wouldn't do it
> with UIO. But it's up to the arch maintainer to decide that. Did you ask
> him?
> 
Yes, and this was the conclusion we came to. There is simply no in-kernel
infrastructure for dealing with the class of devices that are being
exposed to userspace. A small amount of refcounting and management glue
in the kernel is necessary, but the rest is userspace's problem. It's
also hardly architecture-specific, anyone with a complex SoC containing
equally complex IP blocks where no in-kernel infrastructure exists that
still needs to do basic accounting on the kernel side is going to be
faced with this situation.

UIO is generally a pretty good match for this problem, and I'd really
rather not have something buried in my architecture directory to work
around this because the subsystem people decide to be resistant to other
approaches.

> Once more (for the last time): The technical argument against your patch
> is that it offers no advantages. It makes other code more complicated,
> less obvious, and less consistent. This might be OK if we had big
> advantages in return, but this isn't the case.

Great, so lets throw an ifdef around it and be done with it. Some people
need this functionality, while others do not. The only way this can be
perceived as confusing is if it's default-enabled, which then also
requires the semantics to be documented. Having said that, there are
plenty of places in the kernel where we flip between IRQ and polling mode
based on IRQ specifier automatically, so it's not obvious that this sort
of use would qualify as a source of confusion.

> Furthermore, your approach works only on certain platforms. But that
> doesn't matter anymore, because the above is already enough to NAK your
> patch. We won't add code just because it could be done.
> 
I'm not sure what your point is. Your uio_pdrv approach only works on
certain platforms too, so what? We add code because it solves a
particular problem, not for the hell of it.

> If I post a patch, it is my job to make it clear what advantages we have
> if we apply it. And no, nothing is unclear with your patch.

The advantages are primarly around reusability. UIO offers a fairly clean
interface to work with, and the only thing that needs to be addressed is
how to deal with the "unique" IRQ mode. For the blocks Magnus is
exposing, they all have this quality, and none of them have any place in
the kernel outside of the bare management bits. While Magnus could easily
run off and create yet another user interface for these things, it would
be nice to have this solved in a more generic fashion. I'm not sure
what's confusing about any of this or why you don't see these reasons as
an advantage.

All of the code you've wanted to see has been posted in previous threads.

> > > Unfortunately, I'm one of the two UIO maintainers, so I feel obliged to
> > > review your patch and give my opinion. That doesn't mean I'm
> > > 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.
> > 
> > Maybe I will. =)
> 
> Did you notice that in this thread nobody spoke up to support your
> patch?
> 
It's difficult to jump in and support a patch when it's unclear what the
technical arguments against it are. Usually if there's a need for a
driver and no technical arguments against it, the problem works itself
out. Whether we go with this patch or the "other" driver approach is
immaterial, if you aren't going to allow uio_pdrv to be modified, then
please stop peddling it as the solution to the given problem.

Re: [PATCH] uio_pdrv: Unique IRQ Mode

[Uwe Kleine-König]

Hello Hans,

> Did you notice that in this thread nobody spoke up to support your
> patch?
Actually I like what the patch tries to achieve.  I'd like to have it a
bit more explicit tough:

- Provide the irq disabling handler in uio_pdrv.c (or even uio.c) with a
  prototype in an adequate header.  Then the platforms that want this
  kind of handling can request it explicitly.

- Don't use this handler automatically.

- Provide the function named uio_pdrv_unique_irqcontrol in Magnus' patch
  in uio_pdrv.c and in an adequate header.

- Either rely on userspace to enable the irq before reading/polling or
  assert that in kernel space.  See also
  http://thread.gmane.org/gmane.linux.kernel/684683/focus=689635
  (I asked tglx about the race condition via irc, but without a response
  so far.)
  Currently the former is done, but if we decide to let it as it is, I'd
  like to have it documented.  (I.e. something like:  "Before
  polling/reading /dev/uioX assert that irqs are enabled.")

The last point is a bit independent from that mode, but applies to
devices that have a irqcontrol function in general.

Apart from the general things above, I'd change a few things in the
implementation:

 - call dev_info->irqcontrol(OFF) in the handler (instead of
   disable_irq()) and demand that calling this is idempotent.
   With this change it isn't uio_pdrv specific any more and could go to
   uio.c.

 - rename "Unique IRQ Mode" to something like "No IRQ Handler Mode".
   I'm not completely lucky with that name, but it's better than the
   former.  Of course the function should be renamed accordingly.

 - s/unsigned long irq_disabled/unsigned int irq_disabled : 1/ in struct
   uio_platdata.

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_pdrv: Unique IRQ Mode

[Paul Mundt]

On Mon, Jun 09, 2008 at 09:57:01AM +0200, Uwe Kleine-K?nig wrote:
>  - rename "Unique IRQ Mode" to something like "No IRQ Handler Mode".
>    I'm not completely lucky with that name, but it's better than the
>    former.  Of course the function should be renamed accordingly.
> 
User IRQ mode?

Re: [PATCH] uio_pdrv: Unique IRQ Mode

[Hans J. Koch]

On Mon, Jun 09, 2008 at 09:57:01AM +0200, Uwe Kleine-König wrote:
> Hello Hans,
> 
> > Did you notice that in this thread nobody spoke up to support your
> > patch?
> Actually I like what the patch tries to achieve.  I'd like to have it a
> bit more explicit tough:
> 
> - Provide the irq disabling handler in uio_pdrv.c (or even uio.c) with a
>   prototype in an adequate header.  Then the platforms that want this
>   kind of handling can request it explicitly.

You could provide an irqcontrol() function in uio_pdrv that calls a function
defined in board support. If no function is defined there, it returns
-ENOSYS. That would be consistent behaviour and not limited to
non-shared interrupts. Note that this requires the add-write-function
patch I recently posted.

> 
> - Don't use this handler automatically.
> 
> - Provide the function named uio_pdrv_unique_irqcontrol in Magnus' patch
>   in uio_pdrv.c and in an adequate header.

Why invent a new name? The approach above works with all kinds of irqs on
all platforms.

> 
> - Either rely on userspace to enable the irq before reading/polling or
>   assert that in kernel space.  See also
>   http://thread.gmane.org/gmane.linux.kernel/684683/focus=689635
>   (I asked tglx about the race condition via irc, but without a response
>   so far.)

There are two problems:
1) If the hardware is designed in such a broken way that userspace needs
a read-modify-write operation on a combined irq mask/status register to
re-enable the irq, then this is racy against a new interrupt that occurs
simultaneously. We have seen this on two devices so far.

2) If we wanted to make sure the interrupt is enabled in read() and
poll(), we would have the problem that userspace usually calls poll()
and then read() immediately afterwards. This would enable the irq twice,
which can lead to two interrupts being seen in some cases.

For both reasons, we decided that introducing the write() function to
enable and disable irqs is the best solution. Greg already added that
patch to his tree, so it should appear in one of the next kernels.

>   Currently the former is done, but if we decide to let it as it is, I'd
>   like to have it documented.  (I.e. something like:  "Before
>   polling/reading /dev/uioX assert that irqs are enabled.")

We cannot do this, at least not in a clean way.

> 
> The last point is a bit independent from that mode, but applies to
> devices that have a irqcontrol function in general.
> 
> Apart from the general things above, I'd change a few things in the
> implementation:
> 
>  - call dev_info->irqcontrol(OFF) in the handler (instead of
>    disable_irq()) and demand that calling this is idempotent.
>    With this change it isn't uio_pdrv specific any more and could go to
>    uio.c.

Why should we want to do this? You save five lines of irq handler code
by introducing the need for an irqcontrol() function.
I already said that in the discussion with Magnus, I don't see any
advantage in this. Magnus cannot tell me either, he just keeps telling
me "but we can do it" over and over again.
With the modifications mentioned above, it would be a little better, but
I still don't see what we really gain. Your uio_pdrv is a nice and clean
thing, I don't want to add code that makes it less obvious just to save
five lines of irq handler code in some cornercases (or nothing at all,
if we need an irqcontrol() function instead).

Thanks,
Hans

Re: [PATCH] uio_pdrv: Unique IRQ Mode

[Uwe Kleine-König]

Hi Hans,

Hans J. Koch wrote:
> On Mon, Jun 09, 2008 at 09:57:01AM +0200, Uwe Kleine-König wrote:
> > Hello Hans,
> >
> > > Did you notice that in this thread nobody spoke up to support your
> > > patch?
> > Actually I like what the patch tries to achieve.  I'd like to have it a
> > bit more explicit tough:
> >
> > - Provide the irq disabling handler in uio_pdrv.c (or even uio.c) with a
> >   prototype in an adequate header.  Then the platforms that want this
> >   kind of handling can request it explicitly.
> 
> You could provide an irqcontrol() function in uio_pdrv that calls a function
> defined in board support. If no function is defined there, it returns
> -ENOSYS. That would be consistent behaviour and not limited to
> non-shared interrupts. Note that this requires the add-write-function
> patch I recently posted.
I didn't check, but I think this is what is happening just now, though
with a different implementation: board support passed the uio_info which
might or might not include a irqcontrol() function.  This is given
unchanged to uio_register.  Assuming that writing without an
irqcontrol() function yields -ENOSYS we're already there.
 
> > - Don't use this handler automatically.
> >
> > - Provide the function named uio_pdrv_unique_irqcontrol in Magnus' patch
> >   in uio_pdrv.c and in an adequate header.
> 
> Why invent a new name? The approach above works with all kinds of irqs on
> all platforms.
> 
> >
> > - Either rely on userspace to enable the irq before reading/polling or
> >   assert that in kernel space.  See also
> >   http://thread.gmane.org/gmane.linux.kernel/684683/focus=689635
> >   (I asked tglx about the race condition via irc, but without a response
> >   so far.)
> 
> There are two problems:
> 1) If the hardware is designed in such a broken way that userspace needs
> a read-modify-write operation on a combined irq mask/status register to
> re-enable the irq, then this is racy against a new interrupt that occurs
> simultaneously. We have seen this on two devices so far.
You didn't understand what I want.  (Probably because I choosed a poor
wording.)

IMHO it should be asserted that irqs are on before waiting for the irq
in poll and read.  So I suggest to call irqcontrol(ON) before doing so.
This should allow to work with that kind of hardware, right?
 
> 2) If we wanted to make sure the interrupt is enabled in read() and
> poll(), we would have the problem that userspace usually calls poll()
> and then read() immediately afterwards. This would enable the irq twice,
> which can lead to two interrupts being seen in some cases.
OK, for this case a pending flag would be needed.  (This doesn't mean I
suggest to implement it that way.)  I'll think about that a bit.

> For both reasons, we decided that introducing the write() function to
> enable and disable irqs is the best solution. Greg already added that
> patch to his tree, so it should appear in one of the next kernels.
> 
> >   Currently the former is done, but if we decide to let it as it is, I'd
> >   like to have it documented.  (I.e. something like:  "Before
> >   polling/reading /dev/uioX assert that irqs are enabled.")
> 
> We cannot do this, at least not in a clean way.
We cannot document it in a clean way?  (Probably not, I assume "this"
still refers to "enable the irq in read and poll"?)
 
> > The last point is a bit independent from that mode, but applies to
> > devices that have a irqcontrol function in general.
> >
> > Apart from the general things above, I'd change a few things in the
> > implementation:
> >
> >  - call dev_info->irqcontrol(OFF) in the handler (instead of
> >    disable_irq()) and demand that calling this is idempotent.
> >    With this change it isn't uio_pdrv specific any more and could go to
> >    uio.c.
> 
> Why should we want to do this? You save five lines of irq handler code
> by introducing the need for an irqcontrol() function.
Taking Magnus' patch there is a default irqcontrol() function that does
the right thing in this case.  This should probably go to uio_pdrv.c.

> I already said that in the discussion with Magnus, I don't see any
> advantage in this. Magnus cannot tell me either, he just keeps telling
> me "but we can do it" over and over again.
I think the benefit is to add some code to uio_pdrv and/or uio and in
turn save some code in board support code.  In fact this is similar to
the whole uio_pdrv driver.  Each platform could implement it without
much hassle itself.  But having all that in one central place makes it
easier for most people.

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_pdrv: Unique IRQ Mode

[Hans J. Koch]

On Mon, Jun 09, 2008 at 02:32:04PM +0200, Uwe Kleine-König wrote:
> Hi Hans,
> 
> Hans J. Koch wrote:
> > On Mon, Jun 09, 2008 at 09:57:01AM +0200, Uwe Kleine-König wrote:
> > > Hello Hans,
> > >
> > > > Did you notice that in this thread nobody spoke up to support your
> > > > patch?
> > > Actually I like what the patch tries to achieve.  I'd like to have it a
> > > bit more explicit tough:
> > >
> > > - Provide the irq disabling handler in uio_pdrv.c (or even uio.c) with a
> > >   prototype in an adequate header.  Then the platforms that want this
> > >   kind of handling can request it explicitly.
> > 
> > You could provide an irqcontrol() function in uio_pdrv that calls a function
> > defined in board support. If no function is defined there, it returns
> > -ENOSYS. That would be consistent behaviour and not limited to
> > non-shared interrupts. Note that this requires the add-write-function
> > patch I recently posted.
> I didn't check, but I think this is what is happening just now, though
> with a different implementation: board support passed the uio_info which
> might or might not include a irqcontrol() function.  This is given
> unchanged to uio_register.  Assuming that writing without an
> irqcontrol() function yields -ENOSYS we're already there.

Yes, of course, you're right.

>  
> > > - Don't use this handler automatically.
> > >
> > > - Provide the function named uio_pdrv_unique_irqcontrol in Magnus' patch
> > >   in uio_pdrv.c and in an adequate header.
> > 
> > Why invent a new name? The approach above works with all kinds of irqs on
> > all platforms.
> > 
> > >
> > > - Either rely on userspace to enable the irq before reading/polling or
> > >   assert that in kernel space.  See also
> > >   http://thread.gmane.org/gmane.linux.kernel/684683/focus=689635
> > >   (I asked tglx about the race condition via irc, but without a response
> > >   so far.)
> > 
> > There are two problems:
> > 1) If the hardware is designed in such a broken way that userspace needs
> > a read-modify-write operation on a combined irq mask/status register to
> > re-enable the irq, then this is racy against a new interrupt that occurs
> > simultaneously. We have seen this on two devices so far.
> You didn't understand what I want.  (Probably because I choosed a poor
> wording.)
> 
> IMHO it should be asserted that irqs are on before waiting for the irq
> in poll and read.  So I suggest to call irqcontrol(ON) before doing so.
> This should allow to work with that kind of hardware, right?

Yes. But userspace can simply write() a 1 to /dev/uioX to achieve the
same result. This would clearly show what's happening. Remember, this is
only needed for certain (broken) hardware. If we hide some automagic irq
enabling in the kernel, it'll become less obvious and might even have
some bad side effects. I want to avoid this kind of trickery, especially
as it is not needed. Userspace should use write() to control irqs. It's
like this with any normal UIO driver, and we shouldn't have a different
handling in uio_pdrv.
Think of a chip that's directly connected to the bus on some embedded
board. You use uio_pdrv to handle it. Then the very same chip appears on
a PCI card in a normal PC. You write a normal UIO driver for it. The
userspace part of both drivers could be exactly the same. But if
uio_pdrv automagically reenabled the irq, we would need different
handling in userspace, without reasons obvious to the user.

Everything within the UIO framework (uio_pdrv is just becoming a part of
it) should be seen from such a general point of view.
I can understand that you or Magnus are mainly concerned with the embedded
boards on your desk. But we want to support ALL architectures that can
use UIO (in fact, all except S390). Please consider examples like the
one above when you think about changes.

>  
> > 2) If we wanted to make sure the interrupt is enabled in read() and
> > poll(), we would have the problem that userspace usually calls poll()
> > and then read() immediately afterwards. This would enable the irq twice,
> > which can lead to two interrupts being seen in some cases.
> OK, for this case a pending flag would be needed.  (This doesn't mean I
> suggest to implement it that way.)  I'll think about that a bit.
> 
> > For both reasons, we decided that introducing the write() function to
> > enable and disable irqs is the best solution. Greg already added that
> > patch to his tree, so it should appear in one of the next kernels.
> > 
> > >   Currently the former is done, but if we decide to let it as it is, I'd
> > >   like to have it documented.  (I.e. something like:  "Before
> > >   polling/reading /dev/uioX assert that irqs are enabled.")
> > 
> > We cannot do this, at least not in a clean way.
> We cannot document it in a clean way?  (Probably not, I assume "this"
> still refers to "enable the irq in read and poll"?)

Yes ;-)

>  
> > > The last point is a bit independent from that mode, but applies to
> > > devices that have a irqcontrol function in general.
> > >
> > > Apart from the general things above, I'd change a few things in the
> > > implementation:
> > >
> > >  - call dev_info->irqcontrol(OFF) in the handler (instead of
> > >    disable_irq()) and demand that calling this is idempotent.
> > >    With this change it isn't uio_pdrv specific any more and could go to
> > >    uio.c.
> > 
> > Why should we want to do this? You save five lines of irq handler code
> > by introducing the need for an irqcontrol() function.
> Taking Magnus' patch there is a default irqcontrol() function that does
> the right thing in this case.  This should probably go to uio_pdrv.c.

Just doing irq_disable() limits it to irqs that are not shared. If there
was a huge advantage, I'd think about it. But as it is, I'll never
accept that. Magnus' patch is not needed, not even by himself.

> 
> > I already said that in the discussion with Magnus, I don't see any
> > advantage in this. Magnus cannot tell me either, he just keeps telling
> > me "but we can do it" over and over again.
> I think the benefit is to add some code to uio_pdrv and/or uio and in
> turn save some code in board support code. 

Yes, but the savings (if any) are small compared with the disadvantages.

> In fact this is similar to
> the whole uio_pdrv driver.

No. uio_pdrv is a nice idea, cleanly implemented. Lots of people can use
it on different platforms. You could implement a driver for a PCI card
as a platform device if you wanted to. It does one thing, and does it
well. I don't want to spoil that by adding if..thens that introduce
special behaviour for some cornercases, especially if this doesn't even
add a significant advantage.

Thanks,
Hans

Re: [PATCH] uio_pdrv: Unique IRQ Mode

[Uwe Kleine-König]

Hello Hans,

> > > > - Either rely on userspace to enable the irq before reading/polling or
> > > >   assert that in kernel space.  See also
> > > >   http://thread.gmane.org/gmane.linux.kernel/684683/focus=689635
> > > >   (I asked tglx about the race condition via irc, but without a response
> > > >   so far.)
> > >
> > > There are two problems:
> > > 1) If the hardware is designed in such a broken way that userspace needs
> > > a read-modify-write operation on a combined irq mask/status register to
> > > re-enable the irq, then this is racy against a new interrupt that occurs
> > > simultaneously. We have seen this on two devices so far.
> > You didn't understand what I want.  (Probably because I choosed a poor
> > wording.)
> >
> > IMHO it should be asserted that irqs are on before waiting for the irq
> > in poll and read.  So I suggest to call irqcontrol(ON) before doing so.
> > This should allow to work with that kind of hardware, right?
> 
> Yes. But userspace can simply write() a 1 to /dev/uioX to achieve the
> same result. This would clearly show what's happening. Remember, this is
> only needed for certain (broken) hardware. If we hide some automagic irq
> enabling in the kernel, it'll become less obvious and might even have
> some bad side effects. I want to avoid this kind of trickery, especially
> as it is not needed. Userspace should use write() to control irqs. It's
> like this with any normal UIO driver, and we shouldn't have a different
> handling in uio_pdrv.
> Think of a chip that's directly connected to the bus on some embedded
> board. You use uio_pdrv to handle it. Then the very same chip appears on
> a PCI card in a normal PC. You write a normal UIO driver for it. The
> userspace part of both drivers could be exactly the same. But if
> uio_pdrv automagically reenabled the irq, we would need different
> handling in userspace, without reasons obvious to the user.
Note that my intention is to enable irqs in uio.c, not uio_pdrv.c.  So
you could still use the same driver for a PCI card and similar a memory
mapped chip.

Probably I should show some code, but I think I won't have time today to
do so and then I will be in vacation for two weeks.  So this has to
wait.
 
> > > > The last point is a bit independent from that mode, but applies to
> > > > devices that have a irqcontrol function in general.
> > > >
> > > > Apart from the general things above, I'd change a few things in the
> > > > implementation:
> > > >
> > > >  - call dev_info->irqcontrol(OFF) in the handler (instead of
> > > >    disable_irq()) and demand that calling this is idempotent.
> > > >    With this change it isn't uio_pdrv specific any more and could go to
> > > >    uio.c.
> > >
> > > Why should we want to do this? You save five lines of irq handler code
> > > by introducing the need for an irqcontrol() function.
> > Taking Magnus' patch there is a default irqcontrol() function that does
> > the right thing in this case.  This should probably go to uio_pdrv.c.
> 
> Just doing irq_disable() limits it to irqs that are not shared. If there
> was a huge advantage, I'd think about it. But as it is, I'll never
> accept that. Magnus' patch is not needed, not even by himself.
I don't suggest to *use* that function per default, just provide it and
allow board support to use it as a call back.

> > > I already said that in the discussion with Magnus, I don't see any
> > > advantage in this. Magnus cannot tell me either, he just keeps telling
> > > me "but we can do it" over and over again.
> > I think the benefit is to add some code to uio_pdrv and/or uio and in
> > turn save some code in board support code.
> 
> Yes, but the savings (if any) are small compared with the disadvantages.
Currently I don't see any disadvantages.  IMHO we should wait on a new
version of Magnus' patch.  Then we can discuss this more effective
referering to code.
 
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_pdrv: Unique IRQ Mode

[Hans J. Koch]

On Tue, Jun 10, 2008 at 08:11:21AM +0200, Uwe Kleine-König wrote:
> > >
> > > IMHO it should be asserted that irqs are on before waiting for the irq
> > > in poll and read.  So I suggest to call irqcontrol(ON) before doing so.
> > > This should allow to work with that kind of hardware, right?
> > 
> > Yes. But userspace can simply write() a 1 to /dev/uioX to achieve the
> > same result. This would clearly show what's happening. Remember, this is
> > only needed for certain (broken) hardware. If we hide some automagic irq
> > enabling in the kernel, it'll become less obvious and might even have
> > some bad side effects. I want to avoid this kind of trickery, especially
> > as it is not needed. Userspace should use write() to control irqs. It's
> > like this with any normal UIO driver, and we shouldn't have a different
> > handling in uio_pdrv.
> > Think of a chip that's directly connected to the bus on some embedded
> > board. You use uio_pdrv to handle it. Then the very same chip appears on
> > a PCI card in a normal PC. You write a normal UIO driver for it. The
> > userspace part of both drivers could be exactly the same. But if
> > uio_pdrv automagically reenabled the irq, we would need different
> > handling in userspace, without reasons obvious to the user.

> Note that my intention is to enable irqs in uio.c, not uio_pdrv.c.

Forget it. We won't have some crap that automagically enables irqs in
read() and poll(), neither with irq_enable() nor with irqcontrol(). It's
not a clean solution. We now have a clean solution how userspace can
enable interrupts by using write(), and this (together with uio_pdrv)
can handle all cases we were talking about.

You keep arguing about code that neither introduces new features nor
offers any other advantages. I'm really tired of that.

Thanks,
Hans

Re: [PATCH] uio_pdrv: Unique IRQ Mode

[Uwe Kleine-König]

Hello Magnus,

Magnus Damm wrote:
> >> I don't think the board support level is the
> >> proper place for this code.
> >
> > You have to write code there anyway, e.g. code that configures your GPIO
> > as input, makes it generate interrupts and so on. And of course, you
> > have to setup your platform device as well. If you simply add the irq
> > handler, you can use uio_pdrv as-is. 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.
> 
> Ever heard about system on chip? Not all platform devices need board
> specific setup.
> 
> >> The patch contains no board specific code,
> >> and it is independent of both architecture and cpu model.
> >
> > Every platform device driver depends on board support.
> 
> Is that so?
This depends on the definition of "board support".  I think of it as
"code for that (type of) machine", i.e. everything below arch/.  With
that definition really each platform device depends on "board support"
because some code have to create and provide the platform_device.

>             I suggest that you have a look at the mfd drivers and think again.
I haven't done that though.

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