+linux-pm, Rafael
On 17 December 2015 at 11:48, Jon Hunter <jonathanh@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
