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

Groups > linux.kernel > #1256880 > unrolled thread

Re: [PATCH] gpio: zynq: Implement irq_(request|release)_resources

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] gpio: zynq: Implement irq_(request|release)_resources Linus Walleij <linus.walleij@linaro.org> - 2015-10-27 17:00 +0100
    Re: [PATCH] gpio: zynq: Implement irq_(request|release)_resources Grygorii Strashko <grygorii.strashko@ti.com> - 2015-10-27 17:20 +0100
      Re: [PATCH] gpio: zynq: Implement irq_(request|release)_resources Linus Walleij <linus.walleij@linaro.org> - 2015-10-27 17:30 +0100
        Re: [PATCH] gpio: zynq: Implement irq_(request|release)_resources Grygorii Strashko <grygorii.strashko@ti.com> - 2015-10-27 19:00 +0100
          Re: [PATCH] gpio: zynq: Implement irq_(request|release)_resources Linus Walleij <linus.walleij@linaro.org> - 2015-10-28 16:30 +0100
    Re: [PATCH] gpio: zynq: Implement irq_(request|release)_resources Lars-Peter Clausen <lars@metafoo.de> - 2015-10-27 17:40 +0100
      Re: [PATCH] gpio: zynq: Implement irq_(request|release)_resources Thomas Gleixner <tglx@linutronix.de> - 2015-10-30 11:10 +0100

#1256880 — Re: [PATCH] gpio: zynq: Implement irq_(request|release)_resources

On Fri, Oct 23, 2015 at 3:36 PM, Soren Brinkmann
<soren.brinkmann@xilinx.com> wrote:

> The driver uses runtime PM to leverage low power techniques. For
> use-cases using GPIO as interrupt the device needs to be in an
> appropriate state.
>
> Reported-by: John Linn <linnj@xilinx.com>
> Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
> Tested-by: John Linn <linnj@xilinx.com>

As pointed out by Grygorii in
commit aca82d1cbb49af34b69ecd4571a0fe48ad9247c1:

    The PM runtime API can't be used in atomic contex on -RT even if
    it's configured as irqsafe. As result, below error report can
    be seen when PM runtime API called from IRQ chip's callbacks
    irq_startup/irq_shutdown/irq_set_type, because they are
    protected by RAW spinlock:
(...)
    The IRQ chip interface defines only two callbacks which are executed in
    non-atomic contex - irq_bus_lock/irq_bus_sync_unlock, so lets move
    PM runtime calls there.

I.e. these calls are atomic context and it's just luck that it works
and this is fragile.

Can you please check if you can move it to
irq_bus_lock()/irq_sync_unlock()
like Grygorii does?

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1256900

On 10/27/2015 05:53 PM, Linus Walleij wrote:
> On Fri, Oct 23, 2015 at 3:36 PM, Soren Brinkmann
> <soren.brinkmann@xilinx.com> wrote:
>
>> The driver uses runtime PM to leverage low power techniques. For
>> use-cases using GPIO as interrupt the device needs to be in an
>> appropriate state.
>>
>> Reported-by: John Linn <linnj@xilinx.com>
>> Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
>> Tested-by: John Linn <linnj@xilinx.com>
>
> As pointed out by Grygorii in
> commit aca82d1cbb49af34b69ecd4571a0fe48ad9247c1:
>
>      The PM runtime API can't be used in atomic contex on -RT even if
>      it's configured as irqsafe. As result, below error report can
>      be seen when PM runtime API called from IRQ chip's callbacks
>      irq_startup/irq_shutdown/irq_set_type, because they are
>      protected by RAW spinlock:
> (...)
>      The IRQ chip interface defines only two callbacks which are executed in
>      non-atomic contex - irq_bus_lock/irq_bus_sync_unlock, so lets move
>      PM runtime calls there.
>
> I.e. these calls are atomic context and it's just luck that it works
> and this is fragile.
>
> Can you please check if you can move it to
> irq_bus_lock()/irq_sync_unlock()
> like Grygorii does?
>

This patch rises the question not only about PM runtime, but also
about gpiochip_irq_reqres()/gpiochip_irq_relres().


-- 
regards,
-grygorii
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1256909

On Tue, Oct 27, 2015 at 5:18 PM, Grygorii Strashko
<grygorii.strashko@ti.com> wrote:
> On 10/27/2015 05:53 PM, Linus Walleij wrote:
>>
>> On Fri, Oct 23, 2015 at 3:36 PM, Soren Brinkmann
>> <soren.brinkmann@xilinx.com> wrote:
>>
>>> The driver uses runtime PM to leverage low power techniques. For
>>> use-cases using GPIO as interrupt the device needs to be in an
>>> appropriate state.
>>>
>>> Reported-by: John Linn <linnj@xilinx.com>
>>> Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
>>> Tested-by: John Linn <linnj@xilinx.com>
>>
>>
>> As pointed out by Grygorii in
>> commit aca82d1cbb49af34b69ecd4571a0fe48ad9247c1:
>>
>>      The PM runtime API can't be used in atomic contex on -RT even if
>>      it's configured as irqsafe. As result, below error report can
>>      be seen when PM runtime API called from IRQ chip's callbacks
>>      irq_startup/irq_shutdown/irq_set_type, because they are
>>      protected by RAW spinlock:
>> (...)
>>      The IRQ chip interface defines only two callbacks which are executed
>> in
>>      non-atomic contex - irq_bus_lock/irq_bus_sync_unlock, so lets move
>>      PM runtime calls there.
>>
>> I.e. these calls are atomic context and it's just luck that it works
>> and this is fragile.
>>
>> Can you please check if you can move it to
>> irq_bus_lock()/irq_sync_unlock()
>> like Grygorii does?
>>
>
> This patch rises the question not only about PM runtime, but also
> about gpiochip_irq_reqres()/gpiochip_irq_relres().

Do you mean that these functions contain calls to non-atomic
functions?

I mainly reacted to this because it was pm_* calls, that you
mentioned explicitly in your patch.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1257080

On 10/27/2015 06:23 PM, Linus Walleij wrote:
> On Tue, Oct 27, 2015 at 5:18 PM, Grygorii Strashko
> <grygorii.strashko@ti.com> wrote:
>> On 10/27/2015 05:53 PM, Linus Walleij wrote:
>>>
>>> On Fri, Oct 23, 2015 at 3:36 PM, Soren Brinkmann
>>> <soren.brinkmann@xilinx.com> wrote:
>>>
>>>> The driver uses runtime PM to leverage low power techniques. For
>>>> use-cases using GPIO as interrupt the device needs to be in an
>>>> appropriate state.
>>>>
>>>> Reported-by: John Linn <linnj@xilinx.com>
>>>> Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
>>>> Tested-by: John Linn <linnj@xilinx.com>
>>>
>>>
>>> As pointed out by Grygorii in
>>> commit aca82d1cbb49af34b69ecd4571a0fe48ad9247c1:
>>>
>>>       The PM runtime API can't be used in atomic contex on -RT even if
>>>       it's configured as irqsafe. As result, below error report can
>>>       be seen when PM runtime API called from IRQ chip's callbacks
>>>       irq_startup/irq_shutdown/irq_set_type, because they are
>>>       protected by RAW spinlock:
>>> (...)
>>>       The IRQ chip interface defines only two callbacks which are executed
>>> in
>>>       non-atomic contex - irq_bus_lock/irq_bus_sync_unlock, so lets move
>>>       PM runtime calls there.
>>>
>>> I.e. these calls are atomic context and it's just luck that it works
>>> and this is fragile.
>>>
>>> Can you please check if you can move it to
>>> irq_bus_lock()/irq_sync_unlock()
>>> like Grygorii does?
>>>
>>
>> This patch rises the question not only about PM runtime, but also
>> about gpiochip_irq_reqres()/gpiochip_irq_relres().
> 
> Do you mean that these functions contain calls to non-atomic
> functions?
> 

Oh. No, I have to be more specific :(
if GPIOx driver defines custom .irq_(request|release)_resources() callbacks
they will *overwrite* standard GPIOirqchip callbacks.
(commit: 8b67a1f "gpio: don't override irq_*_resources() callbacks")

As result, such GPIOx driver should *re-implement* the same functionality in
its .irq_(request|release)_resources() callbacks as implemented in
gpiochip_irq_reqres()/gpiochip_irq_relres().

> I mainly reacted to this because it was pm_* calls, that you
> mentioned explicitly in your patch.
> 
-- 
regards,
-grygorii
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1258246

On Tue, Oct 27, 2015 at 6:54 PM, Grygorii Strashko
<grygorii.strashko@ti.com> wrote:
> [Me]
>> Do you mean that these functions contain calls to non-atomic
>> functions?
>>
>
> Oh. No, I have to be more specific :(
> if GPIOx driver defines custom .irq_(request|release)_resources() callbacks
> they will *overwrite* standard GPIOirqchip callbacks.
> (commit: 8b67a1f "gpio: don't override irq_*_resources() callbacks")
>
> As result, such GPIOx driver should *re-implement* the same functionality in
> its .irq_(request|release)_resources() callbacks as implemented in
> gpiochip_irq_reqres()/gpiochip_irq_relres().

Yes that goes for all drivers not using gpiochip_add_irqchip().
Not everyone uses that ... and for those supplying their own
implementations of these functions.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1256926

On 10/27/2015 04:53 PM, Linus Walleij wrote:
> On Fri, Oct 23, 2015 at 3:36 PM, Soren Brinkmann
> <soren.brinkmann@xilinx.com> wrote:
> 
>> The driver uses runtime PM to leverage low power techniques. For
>> use-cases using GPIO as interrupt the device needs to be in an
>> appropriate state.
>>
>> Reported-by: John Linn <linnj@xilinx.com>
>> Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
>> Tested-by: John Linn <linnj@xilinx.com>
> 
> As pointed out by Grygorii in
> commit aca82d1cbb49af34b69ecd4571a0fe48ad9247c1:
> 
>     The PM runtime API can't be used in atomic contex on -RT even if
>     it's configured as irqsafe. As result, below error report can
>     be seen when PM runtime API called from IRQ chip's callbacks
>     irq_startup/irq_shutdown/irq_set_type, because they are
>     protected by RAW spinlock:
> (...)
>     The IRQ chip interface defines only two callbacks which are executed in
>     non-atomic contex - irq_bus_lock/irq_bus_sync_unlock, so lets move
>     PM runtime calls there.
> 
> I.e. these calls are atomic context and it's just luck that it works
> and this is fragile.
> 
> Can you please check if you can move it to
> irq_bus_lock()/irq_sync_unlock()
> like Grygorii does?

That only powers up the chip when the chip is accessed. For proper IRQ
operation the chip needs to be powered up though as long as the IRQ is
enabled. request_irq() and free_irq() must always be called from sleepable
context. The thing is just that request_resource/release_resource are called
from within a raw spinlock, which is necessary since otherwise you can't
guarantee that they are only called once for shared interrupts.

It might make sense to add a separate set of callbacks to the irq_chip
struct that are called from the sleepable sections of
request_irq()/free_irq() which are meant for power management purposes and
which wont have the guarantee that they are only called once for shared IRQs
(but are still balanced).

Thomas, do you have any thoughts on this?

- Lars

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1259369

On Tue, 27 Oct 2015, Lars-Peter Clausen wrote:
> On 10/27/2015 04:53 PM, Linus Walleij wrote:
> > On Fri, Oct 23, 2015 at 3:36 PM, Soren Brinkmann
> > <soren.brinkmann@xilinx.com> wrote:
> > 
> >> The driver uses runtime PM to leverage low power techniques. For
> >> use-cases using GPIO as interrupt the device needs to be in an
> >> appropriate state.
> >>
> >> Reported-by: John Linn <linnj@xilinx.com>
> >> Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
> >> Tested-by: John Linn <linnj@xilinx.com>
> > 
> > As pointed out by Grygorii in
> > commit aca82d1cbb49af34b69ecd4571a0fe48ad9247c1:
> > 
> >     The PM runtime API can't be used in atomic contex on -RT even if
> >     it's configured as irqsafe. As result, below error report can
> >     be seen when PM runtime API called from IRQ chip's callbacks
> >     irq_startup/irq_shutdown/irq_set_type, because they are
> >     protected by RAW spinlock:
> > (...)
> >     The IRQ chip interface defines only two callbacks which are executed in
> >     non-atomic contex - irq_bus_lock/irq_bus_sync_unlock, so lets move
> >     PM runtime calls there.
> > 
> > I.e. these calls are atomic context and it's just luck that it works
> > and this is fragile.
> > 
> > Can you please check if you can move it to
> > irq_bus_lock()/irq_sync_unlock()
> > like Grygorii does?
> 
> That only powers up the chip when the chip is accessed. For proper IRQ
> operation the chip needs to be powered up though as long as the IRQ is
> enabled. request_irq() and free_irq() must always be called from sleepable
> context. The thing is just that request_resource/release_resource are called
> from within a raw spinlock, which is necessary since otherwise you can't
> guarantee that they are only called once for shared interrupts.
> 
> It might make sense to add a separate set of callbacks to the irq_chip
> struct that are called from the sleepable sections of
> request_irq()/free_irq() which are meant for power management purposes and
> which wont have the guarantee that they are only called once for shared IRQs
> (but are still balanced).
> 
> Thomas, do you have any thoughts on this?

If you want to keep the chip powered as long as an interrupt is
enabled, then having a irq chip callback might be the proper solution.

Thanks,

	tglx


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web