Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1728178 > unrolled thread

[PATCH v4 0/6] irqdomain, gpio: expand irq_domain_push_irq() for DT use and use it for GPIO

Started byMasahiro Yamada <yamada.masahiro@socionext.com>
First post2017-09-07 13:50 +0200
Last post2017-09-08 17:10 +0200
Articles 3 — 2 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH v4 0/6] irqdomain, gpio: expand irq_domain_push_irq() for DT use and use it for GPIO Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-09-07 13:50 +0200
    Re: [PATCH v4 0/6] irqdomain, gpio: expand irq_domain_push_irq() for  DT use and use it for GPIO Marc Zyngier <marc.zyngier@arm.com> - 2017-09-07 14:40 +0200
      Re: [PATCH v4 0/6] irqdomain, gpio: expand irq_domain_push_irq() for  DT use and use it for GPIO Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-09-08 17:10 +0200

#1728178 — [PATCH v4 0/6] irqdomain, gpio: expand irq_domain_push_irq() for DT use and use it for GPIO

FromMasahiro Yamada <yamada.masahiro@socionext.com>
Date2017-09-07 13:50 +0200
Subject[PATCH v4 0/6] irqdomain, gpio: expand irq_domain_push_irq() for DT use and use it for GPIO
Message-ID<un3tM-7Qh-5@gated-at.bofh.it>
This series adds a GPIO controller for UniPhier SoC family.
It also works as an irqchip in hierarchy domain manner.

My problem is mapping of IRQ from this controller to the parent
irqchip is not contiguous.

  IRQ line of GPIO  --->  Parent interrupt
        0           --->     48
        1           --->     49
                ...
        15          --->     63
        16          --->    154
        17          --->    155
                ...
        20          --->    158
        21          --->    217
        22          --->    218
                ...

So, I need to have an array of parent hwirqs somehow.

Probably, most of people will try to use "interrupts" DT property,
but I noticed a potential problem for hierarchy IRQ domain.
If "interrupts" property exists in the device node, IRQ resource
may be statically allocated when platform devices are populated
from DT.  I asked this question some time ago:
https://lkml.org/lkml/2017/7/6/758

After I tackled this, I decided to put the array in the driver,
but I could not get a positive response for this.
The discussion mostly happened in v1 thread:
http://patchwork.ozlabs.org/patch/797145/

Recently, the new API irq_domain_push_irq() was merged in the
mainline.  I thought this might be useful to solve the hierarchy
domain issue.  Hence, here is a trial.

I found patch 2 is needed to avoid "type mismatch" error.

One more thing, I am worried about a race condition.

I think there is a possibility where a device tries to get IRQ
after irq_domain_create_hierarchy(), but before irq_domain_push_irq().

	priv->domain = irq_domain_create_hierarchy(...)
	if (!priv->domain)
		return -ENOMEM;

        [  *** What if a irq consumer device request the irq here? *** ]

	for (i = 0; i < nirqs; i++) {
		virq = platform_get_irq(pdev, i);
		if (virq < 0)
			continue;

		ret = irq_domain_push_irq(priv->domain, virq, (void *)(long)i);
		if (ret)
			return ret;
	}

By the time irq_domain_create_hierarchy() finished,
the irqdomain will be added to the "irq_domain_list".

If a device calls platform_get_irq(),
the domain is registered, but virq is not allocated yet.

So, irq_create_fwspec_mapping() will call irq_domain_alloc_irqs(),
then irqchip's .alloc() hook is invoked with fwspec passed as arg.

I tried to fix this by patch 3 and 4.

This topic is related to both irqdomain and GPIO,
so includes them in a series.

Comments are appreciated.

I am not sure if my approach is correct.
If I am doing wrong, I will go back to the previous adhoc solution.



Masahiro Yamada (6):
  irqdomain: rename variables in irq_domain_{push,pop}_irq()
  irqdomain: clear trigger type in irq_domain_push_irq()
  irqdomain: move IRQ_DOMAIN_NAME_ALLOCATED define to the original
    position
  irqdomain: set irq domain flags before the irq domain becomes visible
  irqdomain: add IRQ_DOMAIN_FLAG_NO_CREATE flag
  gpio: uniphier: add UniPhier GPIO controller driver

 .../devicetree/bindings/gpio/gpio-uniphier.txt     |  43 ++
 MAINTAINERS                                        |   1 +
 drivers/gpio/Kconfig                               |   8 +
 drivers/gpio/Makefile                              |   1 +
 drivers/gpio/gpio-uniphier.c                       | 486 +++++++++++++++++++++
 include/dt-bindings/gpio/uniphier-gpio.h           |  18 +
 include/linux/irqdomain.h                          |  22 +-
 kernel/irq/irqdomain.c                             |  93 ++--
 8 files changed, 619 insertions(+), 53 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-uniphier.txt
 create mode 100644 drivers/gpio/gpio-uniphier.c
 create mode 100644 include/dt-bindings/gpio/uniphier-gpio.h

-- 
2.7.4

[toc] | [next] | [standalone]

#1728198 — Re: [PATCH v4 0/6] irqdomain, gpio: expand irq_domain_push_irq() for DT use and use it for GPIO

FromMarc Zyngier <marc.zyngier@arm.com>
Date2017-09-07 14:40 +0200
SubjectRe: [PATCH v4 0/6] irqdomain, gpio: expand irq_domain_push_irq() for DT use and use it for GPIO
Message-ID<un4g9-8ni-5@gated-at.bofh.it>
In reply to#1728178
On 07/09/17 12:41, Masahiro Yamada wrote:
> 
> This series adds a GPIO controller for UniPhier SoC family.
> It also works as an irqchip in hierarchy domain manner.
> 
> My problem is mapping of IRQ from this controller to the parent
> irqchip is not contiguous.
> 
>   IRQ line of GPIO  --->  Parent interrupt
>         0           --->     48
>         1           --->     49
>                 ...
>         15          --->     63
>         16          --->    154
>         17          --->    155
>                 ...
>         20          --->    158
>         21          --->    217
>         22          --->    218
>                 ...
> 
> So, I need to have an array of parent hwirqs somehow.
> 
> Probably, most of people will try to use "interrupts" DT property,
> but I noticed a potential problem for hierarchy IRQ domain.
> If "interrupts" property exists in the device node, IRQ resource
> may be statically allocated when platform devices are populated
> from DT.  I asked this question some time ago:
> https://lkml.org/lkml/2017/7/6/758
> 
> After I tackled this, I decided to put the array in the driver,
> but I could not get a positive response for this.
> The discussion mostly happened in v1 thread:
> http://patchwork.ozlabs.org/patch/797145/
> 
> Recently, the new API irq_domain_push_irq() was merged in the
> mainline.  I thought this might be useful to solve the hierarchy
> domain issue.  Hence, here is a trial.
> 
> I found patch 2 is needed to avoid "type mismatch" error.
> 
> One more thing, I am worried about a race condition.
> 
> I think there is a possibility where a device tries to get IRQ
> after irq_domain_create_hierarchy(), but before irq_domain_push_irq().
> 
> 	priv->domain = irq_domain_create_hierarchy(...)
> 	if (!priv->domain)
> 		return -ENOMEM;
> 
>         [  *** What if a irq consumer device request the irq here? *** ]

We've explicitly forbidden such a use case. There is a (not exactly fool
proof) check in irq_domain_push_irq(), but it is pretty easy to bypass
it. "Don't do it" is the conclusion we reached with David Daney.

If you don't want these interrupts to be requested, you might as well
flag them as IRQ_NOREQUEST, and unflag them when the hierarchy is ready.

Would that work for you?

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[toc] | [prev] | [next] | [standalone]

#1729015 — Re: [PATCH v4 0/6] irqdomain, gpio: expand irq_domain_push_irq() for DT use and use it for GPIO

FromMasahiro Yamada <yamada.masahiro@socionext.com>
Date2017-09-08 17:10 +0200
SubjectRe: [PATCH v4 0/6] irqdomain, gpio: expand irq_domain_push_irq() for DT use and use it for GPIO
Message-ID<unt4S-jD-33@gated-at.bofh.it>
In reply to#1728198
Hi Marc.

2017-09-07 21:39 GMT+09:00 Marc Zyngier <marc.zyngier@arm.com>:
>> I think there is a possibility where a device tries to get IRQ
>> after irq_domain_create_hierarchy(), but before irq_domain_push_irq().
>>
>>       priv->domain = irq_domain_create_hierarchy(...)
>>       if (!priv->domain)
>>               return -ENOMEM;
>>
>>         [  *** What if a irq consumer device request the irq here? *** ]
>
> We've explicitly forbidden such a use case. There is a (not exactly fool
> proof) check in irq_domain_push_irq(), but it is pretty easy to bypass
> it. "Don't do it" is the conclusion we reached with David Daney.
>
> If you don't want these interrupts to be requested, you might as well
> flag them as IRQ_NOREQUEST, and unflag them when the hierarchy is ready.
>
> Would that work for you?


Sorry if my description was unclear.

I do not think IRQ_NOREQUEST is equivalent
to IRQ_DOMAIN_FLAG_NO_CREATE I am trying to add in 5/6.


My intention is to prevent platform_get_irq()
from allocating a new virq.

I think IRQ_NOREQUEST only affects request_irq().



Having said that, this series got negative response
as a whole.

My motivation is to get my GPIO driver (6/6) in
by hook or by crook.
If you do not like this series, please feel free to throw it away.




-- 
Best Regards
Masahiro Yamada

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web