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

Groups > linux.kernel > #1311593 > unrolled thread

Re: [RFC PATCH V2 3/8] genirq: Add runtime power management support for IRQ chips

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: [RFC PATCH V2 3/8] genirq: Add runtime power management support  for IRQ chips Ulf Hansson <ulf.hansson@linaro.org> - 2016-01-18 15:50 +0100
    Re: [RFC PATCH V2 3/8] genirq: Add runtime power management support  for IRQ chips Jon Hunter <jonathanh@nvidia.com> - 2016-01-19 11:50 +0100
      Re: [RFC PATCH V2 3/8] genirq: Add runtime power management support  for IRQ chips Thomas Gleixner <tglx@linutronix.de> - 2016-01-20 16:40 +0100
        Re: [RFC PATCH V2 3/8] genirq: Add runtime power management support  for IRQ chips Jon Hunter <jonathanh@nvidia.com> - 2016-01-21 09:40 +0100
        Re: [RFC PATCH V2 3/8] genirq: Add runtime power management support  for IRQ chips Ulf Hansson <ulf.hansson@linaro.org> - 2016-01-21 13:50 +0100
          Re: [RFC PATCH V2 3/8] genirq: Add runtime power management support  for IRQ chips Thomas Gleixner <tglx@linutronix.de> - 2016-01-21 21:00 +0100
            Re: [RFC PATCH V2 3/8] genirq: Add runtime power management support  for IRQ chips Ulf Hansson <ulf.hansson@linaro.org> - 2016-01-22 12:10 +0100
              Re: [RFC PATCH V2 3/8] genirq: Add runtime power management support  for IRQ chips Thomas Gleixner <tglx@linutronix.de> - 2016-01-26 18:20 +0100

#1311593 — Re: [RFC PATCH V2 3/8] genirq: Add runtime power management support for IRQ chips

+linux-pm, Rafael

On 17 December 2015 at 11:48, Jon Hunter <jonathanh@nvidia.com> wrote:
> Some IRQ chips may be located in a power domain outside of the CPU
> subsystem and hence will require device specific runtime power management.
> In order to support such IRQ chips, add a pointer for a device structure
> to the irq_chip structure, and if this pointer is populated by the IRQ
> chip driver and the flag CHIP_HAS_RPM is set, then the pm_runtime_get/put
> APIs for this chip will be called when an IRQ is requested/freed,
> respectively.

Overall I like the idea of this patch(set), as it will allow us to
save power for "unused" irqchips.

>
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> ---
>  include/linux/irq.h    |  4 ++++
>  kernel/irq/internals.h | 24 ++++++++++++++++++++++++
>  kernel/irq/manage.c    |  7 +++++++
>  3 files changed, 35 insertions(+)
>
> diff --git a/include/linux/irq.h b/include/linux/irq.h
> index 3c1c96786248..7a61a7f76177 100644
> --- a/include/linux/irq.h
> +++ b/include/linux/irq.h
> @@ -307,6 +307,7 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
>  /**
>   * struct irq_chip - hardware interrupt chip descriptor
>   *
> + * @dev:               pointer to associated device
>   * @name:              name for /proc/interrupts
>   * @irq_startup:       start up the interrupt (defaults to ->enable if NULL)
>   * @irq_shutdown:      shut down the interrupt (defaults to ->disable if NULL)
> @@ -344,6 +345,7 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
>   * @flags:             chip specific flags
>   */
>  struct irq_chip {
> +       struct device   *dev;
>         const char      *name;
>         unsigned int    (*irq_startup)(struct irq_data *data);
>         void            (*irq_shutdown)(struct irq_data *data);
> @@ -399,6 +401,7 @@ struct irq_chip {
>   * IRQCHIP_SKIP_SET_WAKE:      Skip chip.irq_set_wake(), for this irq chip
>   * IRQCHIP_ONESHOT_SAFE:       One shot does not require mask/unmask
>   * IRQCHIP_EOI_THREADED:       Chip requires eoi() on unmask in threaded mode
> + * IRQCHIP_HAS_PM:             Chip requires runtime power management

Perhaps we don't need to add a specific flag for this, but instead
just check if the ->dev pointer has been assigned and then perform
runtime PM management?

>   */
>  enum {
>         IRQCHIP_SET_TYPE_MASKED         = (1 <<  0),
> @@ -408,6 +411,7 @@ enum {
>         IRQCHIP_SKIP_SET_WAKE           = (1 <<  4),
>         IRQCHIP_ONESHOT_SAFE            = (1 <<  5),
>         IRQCHIP_EOI_THREADED            = (1 <<  6),
> +       IRQCHIP_HAS_RPM                 = (1 <<  7),
>  };
>
>  #include <linux/irqdesc.h>
> diff --git a/kernel/irq/internals.h b/kernel/irq/internals.h
> index fcab63c66905..30a2add7cae6 100644
> --- a/kernel/irq/internals.h
> +++ b/kernel/irq/internals.h
> @@ -7,6 +7,7 @@
>   */
>  #include <linux/irqdesc.h>
>  #include <linux/kernel_stat.h>
> +#include <linux/pm_runtime.h>
>
>  #ifdef CONFIG_SPARSE_IRQ
>  # define IRQ_BITMAP_BITS       (NR_IRQS + 8196)
> @@ -125,6 +126,29 @@ static inline void chip_bus_sync_unlock(struct irq_desc *desc)
>                 desc->irq_data.chip->irq_bus_sync_unlock(&desc->irq_data);
>  }
>
> +/* Inline functions for support of irq chips that require runtime pm */
> +static inline int chip_pm_get(struct irq_desc *desc)

Why does these new get/put functions need to be inline functions and
thus defined in the header file? Perhaps move them to manage.c are
better?

> +{
> +       int retval = 0;
> +
> +       if (desc->irq_data.chip->dev &&
> +           desc->irq_data.chip->flags & IRQCHIP_HAS_RPM)
> +               retval = pm_runtime_get_sync(desc->irq_data.chip->dev);
> +
> +       return (retval < 0) ? retval : 0;
> +}
> +
> +static inline int chip_pm_put(struct irq_desc *desc)
> +{
> +       int retval = 0;
> +
> +       if (desc->irq_data.chip->dev &&
> +           desc->irq_data.chip->flags & IRQCHIP_HAS_RPM)
> +               retval = pm_runtime_put(desc->irq_data.chip->dev);
> +
> +       return (retval < 0) ? retval : 0;

This won't play nicely when CONFIG_PM is unset, as pm_runtime_put()
would return -ENOSYS. In such cases I guess you would like to ignore
the error!?

> +}
> +
>  #define _IRQ_DESC_CHECK                (1 << 0)
>  #define _IRQ_DESC_PERCPU       (1 << 1)
>
> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
> index 2a429b061171..8a96e4f1e985 100644
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -1116,6 +1116,10 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
>         if (!try_module_get(desc->owner))
>                 return -ENODEV;
>
> +       ret = chip_pm_get(desc);
> +       if (ret < 0)
> +               return ret;
> +
>         new->irq = irq;
>
>         /*
> @@ -1400,6 +1404,7 @@ out_thread:
>                 put_task_struct(t);
>         }
>  out_mput:
> +       chip_pm_put(desc);
>         module_put(desc->owner);
>         return ret;
>  }
> @@ -1513,6 +1518,7 @@ static struct irqaction *__free_irq(unsigned int irq, void *dev_id)
>                 }
>         }

I don't think using __free_irq() is the correct place to decrease the
runtime PM usage count. It will keep the irqchip runtime resumed even
if there are no irqs enabled for it.

Instead I would rather allow the irqchip to be runtime suspended, when
there are no irqs enabled on it.

Therefore you should rather use __enable|disable_irq() from where you
increase/decrease the runtime PM usage count.

Although, I realize that may become a bit troublesome as in some of
the execution paths where these functions are invoked, are done while
holding a spinlock with irqs disabled. Invoking pm_runtime_get_sync()
thus leads to that the irqchip's runtime PM callbacks needs to be
irqsafe. Another option is to somehow make use the asynchronous API;
pm_runtime_get() instead.

>
> +       chip_pm_put(desc);
>         module_put(desc->owner);
>         kfree(action->secondary);
>         return action;
> @@ -1799,6 +1805,7 @@ static struct irqaction *__free_percpu_irq(unsigned int irq, void __percpu *dev_
>
>         unregister_handler_proc(irq, action);
>
> +       chip_pm_put(desc);
>         module_put(desc->owner);
>         return action;

Kind regards
Uffe

[toc] | [next] | [standalone]

#1312100

On 18/01/16 14:47, Ulf Hansson wrote:
> +linux-pm, Rafael
> 
> On 17 December 2015 at 11:48, Jon Hunter <jonathanh@nvidia.com> wrote:
>> Some IRQ chips may be located in a power domain outside of the CPU
>> subsystem and hence will require device specific runtime power management.
>> In order to support such IRQ chips, add a pointer for a device structure
>> to the irq_chip structure, and if this pointer is populated by the IRQ
>> chip driver and the flag CHIP_HAS_RPM is set, then the pm_runtime_get/put
>> APIs for this chip will be called when an IRQ is requested/freed,
>> respectively.
> 
> Overall I like the idea of this patch(set), as it will allow us to
> save power for "unused" irqchips.

Great, thanks.

>>
>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
>> ---
>>  include/linux/irq.h    |  4 ++++
>>  kernel/irq/internals.h | 24 ++++++++++++++++++++++++
>>  kernel/irq/manage.c    |  7 +++++++
>>  3 files changed, 35 insertions(+)
>>
>> diff --git a/include/linux/irq.h b/include/linux/irq.h
>> index 3c1c96786248..7a61a7f76177 100644
>> --- a/include/linux/irq.h
>> +++ b/include/linux/irq.h
>> @@ -307,6 +307,7 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
>>  /**
>>   * struct irq_chip - hardware interrupt chip descriptor
>>   *
>> + * @dev:               pointer to associated device
>>   * @name:              name for /proc/interrupts
>>   * @irq_startup:       start up the interrupt (defaults to ->enable if NULL)
>>   * @irq_shutdown:      shut down the interrupt (defaults to ->disable if NULL)
>> @@ -344,6 +345,7 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
>>   * @flags:             chip specific flags
>>   */
>>  struct irq_chip {
>> +       struct device   *dev;
>>         const char      *name;
>>         unsigned int    (*irq_startup)(struct irq_data *data);
>>         void            (*irq_shutdown)(struct irq_data *data);
>> @@ -399,6 +401,7 @@ struct irq_chip {
>>   * IRQCHIP_SKIP_SET_WAKE:      Skip chip.irq_set_wake(), for this irq chip
>>   * IRQCHIP_ONESHOT_SAFE:       One shot does not require mask/unmask
>>   * IRQCHIP_EOI_THREADED:       Chip requires eoi() on unmask in threaded mode
>> + * IRQCHIP_HAS_PM:             Chip requires runtime power management
> 
> Perhaps we don't need to add a specific flag for this, but instead
> just check if the ->dev pointer has been assigned and then perform
> runtime PM management?

Yes it may not be necessary. However, I was not sure if someone would
make use of the dev structure but not use RPM. For now we could drop it.

>>   */
>>  enum {
>>         IRQCHIP_SET_TYPE_MASKED         = (1 <<  0),
>> @@ -408,6 +411,7 @@ enum {
>>         IRQCHIP_SKIP_SET_WAKE           = (1 <<  4),
>>         IRQCHIP_ONESHOT_SAFE            = (1 <<  5),
>>         IRQCHIP_EOI_THREADED            = (1 <<  6),
>> +       IRQCHIP_HAS_RPM                 = (1 <<  7),
>>  };
>>
>>  #include <linux/irqdesc.h>
>> diff --git a/kernel/irq/internals.h b/kernel/irq/internals.h
>> index fcab63c66905..30a2add7cae6 100644
>> --- a/kernel/irq/internals.h
>> +++ b/kernel/irq/internals.h
>> @@ -7,6 +7,7 @@
>>   */
>>  #include <linux/irqdesc.h>
>>  #include <linux/kernel_stat.h>
>> +#include <linux/pm_runtime.h>
>>
>>  #ifdef CONFIG_SPARSE_IRQ
>>  # define IRQ_BITMAP_BITS       (NR_IRQS + 8196)
>> @@ -125,6 +126,29 @@ static inline void chip_bus_sync_unlock(struct irq_desc *desc)
>>                 desc->irq_data.chip->irq_bus_sync_unlock(&desc->irq_data);
>>  }
>>
>> +/* Inline functions for support of irq chips that require runtime pm */
>> +static inline int chip_pm_get(struct irq_desc *desc)
> 
> Why does these new get/put functions need to be inline functions and
> thus defined in the header file? Perhaps move them to manage.c are
> better?

They don't have to be, and so I can move them.

>> +{
>> +       int retval = 0;
>> +
>> +       if (desc->irq_data.chip->dev &&
>> +           desc->irq_data.chip->flags & IRQCHIP_HAS_RPM)
>> +               retval = pm_runtime_get_sync(desc->irq_data.chip->dev);
>> +
>> +       return (retval < 0) ? retval : 0;
>> +}
>> +
>> +static inline int chip_pm_put(struct irq_desc *desc)
>> +{
>> +       int retval = 0;
>> +
>> +       if (desc->irq_data.chip->dev &&
>> +           desc->irq_data.chip->flags & IRQCHIP_HAS_RPM)
>> +               retval = pm_runtime_put(desc->irq_data.chip->dev);
>> +
>> +       return (retval < 0) ? retval : 0;
> 
> This won't play nicely when CONFIG_PM is unset, as pm_runtime_put()
> would return -ENOSYS. In such cases I guess you would like to ignore
> the error!?

Ok, yes good point.

>> +}
>> +
>>  #define _IRQ_DESC_CHECK                (1 << 0)
>>  #define _IRQ_DESC_PERCPU       (1 << 1)
>>
>> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
>> index 2a429b061171..8a96e4f1e985 100644
>> --- a/kernel/irq/manage.c
>> +++ b/kernel/irq/manage.c
>> @@ -1116,6 +1116,10 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
>>         if (!try_module_get(desc->owner))
>>                 return -ENODEV;
>>
>> +       ret = chip_pm_get(desc);
>> +       if (ret < 0)
>> +               return ret;
>> +
>>         new->irq = irq;
>>
>>         /*
>> @@ -1400,6 +1404,7 @@ out_thread:
>>                 put_task_struct(t);
>>         }
>>  out_mput:
>> +       chip_pm_put(desc);
>>         module_put(desc->owner);
>>         return ret;
>>  }
>> @@ -1513,6 +1518,7 @@ static struct irqaction *__free_irq(unsigned int irq, void *dev_id)
>>                 }
>>         }
> 
> I don't think using __free_irq() is the correct place to decrease the
> runtime PM usage count. It will keep the irqchip runtime resumed even
> if there are no irqs enabled for it.
> 
> Instead I would rather allow the irqchip to be runtime suspended, when
> there are no irqs enabled on it.
> 
> Therefore you should rather use __enable|disable_irq() from where you
> increase/decrease the runtime PM usage count.
> 
> Although, I realize that may become a bit troublesome as in some of
> the execution paths where these functions are invoked, are done while
> holding a spinlock with irqs disabled. Invoking pm_runtime_get_sync()
> thus leads to that the irqchip's runtime PM callbacks needs to be
> irqsafe. Another option is to somehow make use the asynchronous API;
> pm_runtime_get() instead.

So that would be ideal, however, I don't think it is that trivial and
hence this is why I have not done this for now.

I don't think that the async callbacks will really help here because if
you call enable_irq() and the chip is runtime suspended, you need to
wait for the chip to resume before you can enable the IRQ. I think that
the disable path would be ok, but not the enable. Plus there could be
some "hot" paths where enable/disable are used and I did not want to
make any assumptions about these.

This may appear ugly, but for something like this, we may need to have a
separate enable/disable API, such as
enable_irq_lazy()/disable_irq_lazy() which could be used to runtime
suspend/resume the chip and must not be used in critical sections.

I was hoping that we could get some initially functionality in place to
allow some basic support for these chips and then enhance it later.

Cheers
Jon

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

#1313306

On Tue, 19 Jan 2016, Jon Hunter wrote:
> On 18/01/16 14:47, Ulf Hansson wrote:
> >> +/* Inline functions for support of irq chips that require runtime pm */
> >> +static inline int chip_pm_get(struct irq_desc *desc)
> > 
> > Why does these new get/put functions need to be inline functions and
> > thus defined in the header file? Perhaps move them to manage.c are
> > better?
> 
> They don't have to be, and so I can move them.

Yes, please make them proper functions. The proper place for them is chip.c
 
> > This won't play nicely when CONFIG_PM is unset, as pm_runtime_put()
> > would return -ENOSYS. In such cases I guess you would like to ignore
> > the error!?
> 
> Ok, yes good point.

So you need a CONFIG_PM variant and stubs which return 0 for the !PM case.
 
> >> @@ -1116,6 +1116,10 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
> >>         if (!try_module_get(desc->owner))
> >>                 return -ENODEV;
> >>
> >> +       ret = chip_pm_get(desc);
> >> +       if (ret < 0)
> >> +               return ret;

That leaks the module refcount.

> > I don't think using __free_irq() is the correct place to decrease the
> > runtime PM usage count. It will keep the irqchip runtime resumed even
> > if there are no irqs enabled for it.
> >
> > Instead I would rather allow the irqchip to be runtime suspended, when
> > there are no irqs enabled on it.

Which is a no no, as you might lose interrupts that way. We disable interrupts
lazy, i.e. we do not mask them. So no, you cannot do that from
enable/disable_irq().
 
> This may appear ugly, but for something like this, we may need to have a
> separate enable/disable API, such as
> enable_irq_lazy()/disable_irq_lazy() which could be used to runtime
> suspend/resume the chip and must not be used in critical sections.

enable_irq_lazy is a misnomer. enable_irq_pm or such might be acceptable.

But before we go there I really want to see a proper use case for such
functions.

Thanks,

	tglx

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

#1314003

On 20/01/16 15:30, Thomas Gleixner wrote:
> On Tue, 19 Jan 2016, Jon Hunter wrote:
>> On 18/01/16 14:47, Ulf Hansson wrote:
>>>> +/* Inline functions for support of irq chips that require runtime pm */
>>>> +static inline int chip_pm_get(struct irq_desc *desc)
>>>
>>> Why does these new get/put functions need to be inline functions and
>>> thus defined in the header file? Perhaps move them to manage.c are
>>> better?
>>
>> They don't have to be, and so I can move them.
> 
> Yes, please make them proper functions. The proper place for them is chip.c
>  
>>> This won't play nicely when CONFIG_PM is unset, as pm_runtime_put()
>>> would return -ENOSYS. In such cases I guess you would like to ignore
>>> the error!?
>>
>> Ok, yes good point.
> 
> So you need a CONFIG_PM variant and stubs which return 0 for the !PM case.
>  
>>>> @@ -1116,6 +1116,10 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
>>>>         if (!try_module_get(desc->owner))
>>>>                 return -ENODEV;
>>>>
>>>> +       ret = chip_pm_get(desc);
>>>> +       if (ret < 0)
>>>> +               return ret;
> 
> That leaks the module refcount.

Ok, I will fix that.

>>> I don't think using __free_irq() is the correct place to decrease the
>>> runtime PM usage count. It will keep the irqchip runtime resumed even
>>> if there are no irqs enabled for it.
>>>
>>> Instead I would rather allow the irqchip to be runtime suspended, when
>>> there are no irqs enabled on it.
> 
> Which is a no no, as you might lose interrupts that way. We disable interrupts
> lazy, i.e. we do not mask them. So no, you cannot do that from
> enable/disable_irq().
>  
>> This may appear ugly, but for something like this, we may need to have a
>> separate enable/disable API, such as
>> enable_irq_lazy()/disable_irq_lazy() which could be used to runtime
>> suspend/resume the chip and must not be used in critical sections.
> 
> enable_irq_lazy is a misnomer. enable_irq_pm or such might be acceptable.

That's fine with me.

> But before we go there I really want to see a proper use case for such
> functions.

Ok, that makes sense.

Cheers
Jon

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

#1314168

[...]

>> > I don't think using __free_irq() is the correct place to decrease the
>> > runtime PM usage count. It will keep the irqchip runtime resumed even
>> > if there are no irqs enabled for it.
>> >
>> > Instead I would rather allow the irqchip to be runtime suspended, when
>> > there are no irqs enabled on it.
>
> Which is a no no, as you might lose interrupts that way. We disable interrupts
> lazy, i.e. we do not mask them. So no, you cannot do that from
> enable/disable_irq().

Thanks for the input!

My main point around the approach suggested in $subject patch, is that
I don't think it's *enough* fine grained.

From a runtime PM perspective, we should allow the irqchip to enter a
low power state when there are no IRQs enabled for it.

As enable|disable_irq() isn't the place to do runtime PM reference
counting from, perhaps there is another option? Maybe the irqchip
driver itself is better suited to take these decisions?

[...]

Kind regards
Uffe

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

#1314473

On Thu, 21 Jan 2016, Ulf Hansson wrote:
> >> > I don't think using __free_irq() is the correct place to decrease the
> >> > runtime PM usage count. It will keep the irqchip runtime resumed even
> >> > if there are no irqs enabled for it.
> >> >
> >> > Instead I would rather allow the irqchip to be runtime suspended, when
> >> > there are no irqs enabled on it.
> >
> > Which is a no no, as you might lose interrupts that way. We disable interrupts
> > lazy, i.e. we do not mask them. So no, you cannot do that from
> > enable/disable_irq().
> 
> Thanks for the input!
> 
> My main point around the approach suggested in $subject patch, is that
> I don't think it's *enough* fine grained.
> 
> From a runtime PM perspective, we should allow the irqchip to enter a
> low power state when there are no IRQs enabled for it.
> 
> As enable|disable_irq() isn't the place to do runtime PM reference
> counting from, perhaps there is another option? Maybe the irqchip
> driver itself is better suited to take these decisions?

The irq chip driver does not have more information than the core code,
actually it has less.

Let's look at disable_irq(). It's normaly used for short periods of time where
a driver needs to make sure that the interrupt is not handled. That's hardly a
point to do power management, because its going to be reenabled right
away. That's one of the reasons for doing the lazy masking, though the main
reason is to prevent losing interrupt on edge driven irq lines.

So as long as an interrupt handler is installed, there is no sane way that we
can decide to power down the irq chip, unless that chip has the magic ability
to relay incoming interrupts while powered down :)

There is also the issue with shared interrupts....

So the only sane way to power down the irq chip is to remove the handlers when
the device is not in use. Actually some of the subsystems/drivers do that
already.

Though there might be a case where the device driver is still in use, but it
has brought down the device into a quiescent state, which guarantees that no
interrupts happen unless its powered up again. That would be an opportunity to
shut down the interrupt and do power management as well. So currently we only
have the option of freeing the interrupt and requesting it again.

It might be conveniant to avoid that by having extra functions which are less
heavyweight. Something like irq_[de]activate(irq, void *dev_id), which would
mask/unmask the interrupt and do power management on the irq chip.

If you can come up with a simple comparision of such an approach with the
free/request_irq() method which shows that it is superior, I'm surely not in
the way. No need to code that core part, just show how a few drivers would use
those functions and why this is less intrusive and simpler to use for the
developer that going for the free/request() solution.

Thanks,

	tglx

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

#1314904

On 21 January 2016 at 20:51, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Thu, 21 Jan 2016, Ulf Hansson wrote:
>> >> > I don't think using __free_irq() is the correct place to decrease the
>> >> > runtime PM usage count. It will keep the irqchip runtime resumed even
>> >> > if there are no irqs enabled for it.
>> >> >
>> >> > Instead I would rather allow the irqchip to be runtime suspended, when
>> >> > there are no irqs enabled on it.
>> >
>> > Which is a no no, as you might lose interrupts that way. We disable interrupts
>> > lazy, i.e. we do not mask them. So no, you cannot do that from
>> > enable/disable_irq().
>>
>> Thanks for the input!
>>
>> My main point around the approach suggested in $subject patch, is that
>> I don't think it's *enough* fine grained.
>>
>> From a runtime PM perspective, we should allow the irqchip to enter a
>> low power state when there are no IRQs enabled for it.
>>
>> As enable|disable_irq() isn't the place to do runtime PM reference
>> counting from, perhaps there is another option? Maybe the irqchip
>> driver itself is better suited to take these decisions?
>
> The irq chip driver does not have more information than the core code,
> actually it has less.
>
> Let's look at disable_irq(). It's normaly used for short periods of time where
> a driver needs to make sure that the interrupt is not handled. That's hardly a
> point to do power management, because its going to be reenabled right
> away. That's one of the reasons for doing the lazy masking, though the main
> reason is to prevent losing interrupt on edge driven irq lines.

I didn't think of the edge driven irq case.

I totally agree, disable|enable_irq() can't be used for power management.

>
> So as long as an interrupt handler is installed, there is no sane way that we
> can decide to power down the irq chip, unless that chip has the magic ability
> to relay incoming interrupts while powered down :)
>
> There is also the issue with shared interrupts....
>
> So the only sane way to power down the irq chip is to remove the handlers when
> the device is not in use. Actually some of the subsystems/drivers do that
> already.
>
> Though there might be a case where the device driver is still in use, but it
> has brought down the device into a quiescent state, which guarantees that no
> interrupts happen unless its powered up again. That would be an opportunity to
> shut down the interrupt and do power management as well. So currently we only
> have the option of freeing the interrupt and requesting it again.

I agree, this is exactly the case I have been thinking of.

Closely related to this topic.

Currently there are several cases where subsystems/drivers don't
protects themselves from receiving an IRQ, when the device is in a
quiescent state.
This may lead to hangs at device register accesses etc, especially
when the IRQ handler expects the device to be functional and not in a
low power state.

This is a different issue, but the solution I had in mind was to use
disable|enable_irq() as protection. I now realize that
freeing/re-requesting the IRQ handler is better, as that would
potentially allow power management for the irqchip as well.

>
> It might be conveniant to avoid that by having extra functions which are less
> heavyweight. Something like irq_[de]activate(irq, void *dev_id), which would
> mask/unmask the interrupt and do power management on the irq chip.

I like that idea!

We may also want to distinguish between the PM case and by just having
an IRQ handler registered, for whatever reason.

More importantly, we don't want to introduce unnecessary latencies
when bringing devices back to full power from a quiescent power state.

Perhaps the functions should be named something with "pm" to better
reflect their purpose?

>
> If you can come up with a simple comparision of such an approach with the
> free/request_irq() method which shows that it is superior, I'm surely not in
> the way. No need to code that core part, just show how a few drivers would use
> those functions and why this is less intrusive and simpler to use for the
> developer that going for the free/request() solution.

Here's a small collection of drivers that I easily picked up as
candidates for using these new APIs.
In principle, they would invoke these new APIs from their runtime PM callbacks.

drivers/spi/spi-atmel.c
drivers/spi/spi-pl022.c
drivers/i2c/busses/i2c-omap.c
drivers/i2c/busses/i2c-nomadik.c
drivers/i2c/busses/i2c-sh_mobile.c
drivers/mmc/host/mtk-sd.c
drivers/mmc/host/mmci.c

Kind regards
Uffe

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

#1318167

On Fri, 22 Jan 2016, Ulf Hansson wrote:
> Here's a small collection of drivers that I easily picked up as
> candidates for using these new APIs.
> In principle, they would invoke these new APIs from their runtime PM callbacks.
> 
> drivers/spi/spi-atmel.c
> drivers/spi/spi-pl022.c
> drivers/i2c/busses/i2c-omap.c
> drivers/i2c/busses/i2c-nomadik.c
> drivers/i2c/busses/i2c-sh_mobile.c
> drivers/mmc/host/mtk-sd.c
> drivers/mmc/host/mmci.c

Instead of adding those calls to each driver, we can be smart and flag the
interrupt as AUTO_RUNTIME_SUSPEND or such.

So the runtime_pm core can handle it when invoking the
dev_pm_ops->runtime_suspend()/resume() callbacks.

Unfortunately the devres stuff is exceptionally bad to be used for this, but
with some surgery it should be doable.

Thanks,

	tglx

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web