Hi Thomas, Marc:
This is a small cleanup series on gic(v3) and genirq, in which:
- #1-2 are minor cleanup on gic/gic-v3 codes;
- #3 is a trival cleanup on genirq header;
- #4 is a fix on genirq;
Dawei Li (4):
irqchip/gic-v3: Implement read polling with dedicated API
irqchip/gic: Implement generic gic_irq() API for GIC & GIC-v3
genirq: Remove unneeded forward declaration
genirq: Initialize resend_node hlist for all irq_desc
drivers/irqchip/irq-gic-common.h | 5 +++++
drivers/irqchip/irq-gic-v3.c | 32 ++++++++------------------------
drivers/irqchip/irq-gic.c | 5 -----
include/linux/irqhandler.h | 2 +-
kernel/irq/irqdesc.c | 2 +-
5 files changed, 15 insertions(+), 31 deletions(-)
base-commit: 69ffab9b9e698248cbb4042e47f82afb00dc1bb4
Thanks,
Dawei
--
2.27.0
GIC & GIC-v3 share same gic_irq() implementations, unify them into a
generic API.
Signed-off-by: Dawei Li <[email protected]>
---
drivers/irqchip/irq-gic-common.h | 5 +++++
drivers/irqchip/irq-gic-v3.c | 5 -----
drivers/irqchip/irq-gic.c | 5 -----
3 files changed, 5 insertions(+), 10 deletions(-)
diff --git a/drivers/irqchip/irq-gic-common.h b/drivers/irqchip/irq-gic-common.h
index f407cce9ecaa..ed18db4ab2c5 100644
--- a/drivers/irqchip/irq-gic-common.h
+++ b/drivers/irqchip/irq-gic-common.h
@@ -19,6 +19,11 @@ struct gic_quirk {
u32 mask;
};
+static inline unsigned int gic_irq(struct irq_data *d)
+{
+ return d->hwirq;
+}
+
int gic_configure_irq(unsigned int irq, unsigned int type,
void __iomem *base, void (*sync_access)(void));
void gic_dist_config(void __iomem *base, int gic_irqs,
diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index b9d9375a3434..474a498a521e 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -181,11 +181,6 @@ static enum gic_intid_range get_intid_range(struct irq_data *d)
return __get_intid_range(d->hwirq);
}
-static inline unsigned int gic_irq(struct irq_data *d)
-{
- return d->hwirq;
-}
-
static inline bool gic_irq_in_rdist(struct irq_data *d)
{
switch (get_intid_range(d)) {
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 412196a7dad5..0d559effa172 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -162,11 +162,6 @@ static inline void __iomem *gic_cpu_base(struct irq_data *d)
return gic_data_cpu_base(gic_data);
}
-static inline unsigned int gic_irq(struct irq_data *d)
-{
- return d->hwirq;
-}
-
static inline bool cascading_gic_irq(struct irq_data *d)
{
void *data = irq_data_get_irq_handler_data(d);
--
2.27.0
Protoype of irq_flow_handler_t is independent of irq_data, so
remove unneeded forward declaration.
Signed-off-by: Dawei Li <[email protected]>
---
include/linux/irqhandler.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/irqhandler.h b/include/linux/irqhandler.h
index c30f454a9518..72dd1eb3a0e7 100644
--- a/include/linux/irqhandler.h
+++ b/include/linux/irqhandler.h
@@ -8,7 +8,7 @@
*/
struct irq_desc;
-struct irq_data;
+
typedef void (*irq_flow_handler_t)(struct irq_desc *desc);
#endif
--
2.27.0
For !CONFIG_SPARSE_IRQ kernel, early_irq_init() is supposed to
initialize all the desc entries in system, desc->resend_node
included.
Thus, initialize desc->resend_node for all irq_desc entries, rather
than irq_desc[0] only, which is the current implementation is about.
Fixes: bc06a9e08742 ("genirq: Use hlist for managing resend handlers")
Cc: [email protected]
Signed-off-by: Dawei Li <[email protected]>
---
kernel/irq/irqdesc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
index 27ca1c866f29..371eb1711d34 100644
--- a/kernel/irq/irqdesc.c
+++ b/kernel/irq/irqdesc.c
@@ -600,7 +600,7 @@ int __init early_irq_init(void)
mutex_init(&desc[i].request_mutex);
init_waitqueue_head(&desc[i].wait_for_threads);
desc_set_defaults(i, &desc[i], node, NULL, NULL);
- irq_resend_init(desc);
+ irq_resend_init(&desc[i]);
}
return arch_early_irq_init();
}
--
2.27.0
Kernel provide read*_poll_* API family to support looping based polling
code pattern like below:
while (...)
{
val = op(addr);
condition = cond(val);
if (condition)
break;
/* Maybe some timeout handling stuff */
cpu_relax();
udelay();
}
As such, use readl_relaxed_poll_timeout_atomic() to implement atomic
register polling logic in gic-v3.
Signed-off-by: Dawei Li <[email protected]>
---
drivers/irqchip/irq-gic-v3.c | 27 ++++++++-------------------
1 file changed, 8 insertions(+), 19 deletions(-)
diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index 98b0329b7154..b9d9375a3434 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -19,6 +19,7 @@
#include <linux/percpu.h>
#include <linux/refcount.h>
#include <linux/slab.h>
+#include <linux/iopoll.h>
#include <linux/irqchip.h>
#include <linux/irqchip/arm-gic-common.h>
@@ -251,17 +252,11 @@ static inline void __iomem *gic_dist_base(struct irq_data *d)
static void gic_do_wait_for_rwp(void __iomem *base, u32 bit)
{
- u32 count = 1000000; /* 1s! */
+ u32 val;
- while (readl_relaxed(base + GICD_CTLR) & bit) {
- count--;
- if (!count) {
- pr_err_ratelimited("RWP timeout, gone fishing\n");
- return;
- }
- cpu_relax();
- udelay(1);
- }
+ if (readl_relaxed_poll_timeout_atomic(base + GICD_CTLR,
+ val, !(val & bit), 1, 1000000) == -ETIMEDOUT)
+ pr_err_ratelimited("RWP timeout, gone fishing\n");
}
/* Wait for completion of a distributor change */
@@ -279,7 +274,6 @@ static void gic_redist_wait_for_rwp(void)
static void gic_enable_redist(bool enable)
{
void __iomem *rbase;
- u32 count = 1000000; /* 1s! */
u32 val;
if (gic_data.flags & FLAGS_WORKAROUND_GICR_WAKER_MSM8996)
@@ -301,14 +295,9 @@ static void gic_enable_redist(bool enable)
return; /* No PM support in this redistributor */
}
- while (--count) {
- val = readl_relaxed(rbase + GICR_WAKER);
- if (enable ^ (bool)(val & GICR_WAKER_ChildrenAsleep))
- break;
- cpu_relax();
- udelay(1);
- }
- if (!count)
+ if (readl_relaxed_poll_timeout_atomic(rbase + GICR_WAKER,
+ val, enable ^ (bool)(val & GICR_WAKER_ChildrenAsleep),
+ 1, 1000000) == -ETIMEDOUT)
pr_err_ratelimited("redistributor failed to %s...\n",
enable ? "wakeup" : "sleep");
}
--
2.27.0
On Thu, 18 Jan 2024 11:27:36 +0000,
Dawei Li <[email protected]> wrote:
>
> Kernel provide read*_poll_* API family to support looping based polling
> code pattern like below:
> while (...)
> {
> val = op(addr);
> condition = cond(val);
> if (condition)
> break;
>
> /* Maybe some timeout handling stuff */
>
> cpu_relax();
> udelay();
> }
>
> As such, use readl_relaxed_poll_timeout_atomic() to implement atomic
> register polling logic in gic-v3.
>
> Signed-off-by: Dawei Li <[email protected]>
> ---
> drivers/irqchip/irq-gic-v3.c | 27 ++++++++-------------------
> 1 file changed, 8 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> index 98b0329b7154..b9d9375a3434 100644
> --- a/drivers/irqchip/irq-gic-v3.c
> +++ b/drivers/irqchip/irq-gic-v3.c
> @@ -19,6 +19,7 @@
> #include <linux/percpu.h>
> #include <linux/refcount.h>
> #include <linux/slab.h>
> +#include <linux/iopoll.h>
>
> #include <linux/irqchip.h>
> #include <linux/irqchip/arm-gic-common.h>
> @@ -251,17 +252,11 @@ static inline void __iomem *gic_dist_base(struct irq_data *d)
>
> static void gic_do_wait_for_rwp(void __iomem *base, u32 bit)
> {
> - u32 count = 1000000; /* 1s! */
> + u32 val;
>
> - while (readl_relaxed(base + GICD_CTLR) & bit) {
> - count--;
> - if (!count) {
> - pr_err_ratelimited("RWP timeout, gone fishing\n");
> - return;
> - }
> - cpu_relax();
> - udelay(1);
> - }
> + if (readl_relaxed_poll_timeout_atomic(base + GICD_CTLR,
> + val, !(val & bit), 1, 1000000) == -ETIMEDOUT)
If you are doing this, please use a constant such as USEC_PER_SEC for
the timeout. And fix the alignment of the second line so that the
parameters are aligned vertically.
> + pr_err_ratelimited("RWP timeout, gone fishing\n");
> }
>
> /* Wait for completion of a distributor change */
> @@ -279,7 +274,6 @@ static void gic_redist_wait_for_rwp(void)
> static void gic_enable_redist(bool enable)
> {
> void __iomem *rbase;
> - u32 count = 1000000; /* 1s! */
> u32 val;
>
> if (gic_data.flags & FLAGS_WORKAROUND_GICR_WAKER_MSM8996)
> @@ -301,14 +295,9 @@ static void gic_enable_redist(bool enable)
> return; /* No PM support in this redistributor */
> }
>
> - while (--count) {
> - val = readl_relaxed(rbase + GICR_WAKER);
> - if (enable ^ (bool)(val & GICR_WAKER_ChildrenAsleep))
> - break;
> - cpu_relax();
> - udelay(1);
> - }
> - if (!count)
> + if (readl_relaxed_poll_timeout_atomic(rbase + GICR_WAKER,
> + val, enable ^ (bool)(val & GICR_WAKER_ChildrenAsleep),
> + 1, 1000000) == -ETIMEDOUT)
> pr_err_ratelimited("redistributor failed to %s...\n",
> enable ? "wakeup" : "sleep");
> }
Same thing here.
M.
--
Without deviation from the norm, progress is not possible.
On Thu, 18 Jan 2024 11:27:39 +0000,
Dawei Li <[email protected]> wrote:
>
> For !CONFIG_SPARSE_IRQ kernel, early_irq_init() is supposed to
> initialize all the desc entries in system, desc->resend_node
> included.
>
> Thus, initialize desc->resend_node for all irq_desc entries, rather
> than irq_desc[0] only, which is the current implementation is about.
>
> Fixes: bc06a9e08742 ("genirq: Use hlist for managing resend handlers")
> Cc: [email protected]
>
> Signed-off-by: Dawei Li <[email protected]>
> ---
> kernel/irq/irqdesc.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
> index 27ca1c866f29..371eb1711d34 100644
> --- a/kernel/irq/irqdesc.c
> +++ b/kernel/irq/irqdesc.c
> @@ -600,7 +600,7 @@ int __init early_irq_init(void)
> mutex_init(&desc[i].request_mutex);
> init_waitqueue_head(&desc[i].wait_for_threads);
> desc_set_defaults(i, &desc[i], node, NULL, NULL);
> - irq_resend_init(desc);
> + irq_resend_init(&desc[i]);
> }
> return arch_early_irq_init();
> }
Well spotted. It would probably be worth having a helper that fully
initialises one desc, but that's out of the scope of this fix.
Acked-by: Marc Zyngier <[email protected]>
M.
--
Without deviation from the norm, progress is not possible.
On Thu, 18 Jan 2024 11:27:37 +0000,
Dawei Li <[email protected]> wrote:
>
> GIC & GIC-v3 share same gic_irq() implementations, unify them into a
> generic API.
>
> Signed-off-by: Dawei Li <[email protected]>
> ---
> drivers/irqchip/irq-gic-common.h | 5 +++++
> drivers/irqchip/irq-gic-v3.c | 5 -----
> drivers/irqchip/irq-gic.c | 5 -----
> 3 files changed, 5 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/irqchip/irq-gic-common.h b/drivers/irqchip/irq-gic-common.h
> index f407cce9ecaa..ed18db4ab2c5 100644
> --- a/drivers/irqchip/irq-gic-common.h
> +++ b/drivers/irqchip/irq-gic-common.h
> @@ -19,6 +19,11 @@ struct gic_quirk {
> u32 mask;
> };
>
> +static inline unsigned int gic_irq(struct irq_data *d)
> +{
> + return d->hwirq;
> +}
> +
> int gic_configure_irq(unsigned int irq, unsigned int type,
> void __iomem *base, void (*sync_access)(void));
> void gic_dist_config(void __iomem *base, int gic_irqs,
> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> index b9d9375a3434..474a498a521e 100644
> --- a/drivers/irqchip/irq-gic-v3.c
> +++ b/drivers/irqchip/irq-gic-v3.c
> @@ -181,11 +181,6 @@ static enum gic_intid_range get_intid_range(struct irq_data *d)
> return __get_intid_range(d->hwirq);
> }
>
> -static inline unsigned int gic_irq(struct irq_data *d)
> -{
> - return d->hwirq;
> -}
> -
I'd rather not do that. If anything, I'd get rid of the helper
altogether, as we have irqd_to_hwirq() that does the same job, and
actually preserves the typing.
M.
--
Without deviation from the norm, progress is not possible.
Hi Marc,
Thanks for the quick review.
On Thu, Jan 18, 2024 at 02:00:15PM +0000, Marc Zyngier wrote:
> On Thu, 18 Jan 2024 11:27:36 +0000,
> Dawei Li <[email protected]> wrote:
> >
> > Kernel provide read*_poll_* API family to support looping based polling
> > code pattern like below:
> > while (...)
> > {
> > val = op(addr);
> > condition = cond(val);
> > if (condition)
> > break;
> >
> > /* Maybe some timeout handling stuff */
> >
> > cpu_relax();
> > udelay();
> > }
> >
> > As such, use readl_relaxed_poll_timeout_atomic() to implement atomic
> > register polling logic in gic-v3.
> >
> > Signed-off-by: Dawei Li <[email protected]>
> > ---
> > drivers/irqchip/irq-gic-v3.c | 27 ++++++++-------------------
> > 1 file changed, 8 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> > index 98b0329b7154..b9d9375a3434 100644
> > --- a/drivers/irqchip/irq-gic-v3.c
> > +++ b/drivers/irqchip/irq-gic-v3.c
> > @@ -19,6 +19,7 @@
> > #include <linux/percpu.h>
> > #include <linux/refcount.h>
> > #include <linux/slab.h>
> > +#include <linux/iopoll.h>
> >
> > #include <linux/irqchip.h>
> > #include <linux/irqchip/arm-gic-common.h>
> > @@ -251,17 +252,11 @@ static inline void __iomem *gic_dist_base(struct irq_data *d)
> >
> > static void gic_do_wait_for_rwp(void __iomem *base, u32 bit)
> > {
> > - u32 count = 1000000; /* 1s! */
> > + u32 val;
> >
> > - while (readl_relaxed(base + GICD_CTLR) & bit) {
> > - count--;
> > - if (!count) {
> > - pr_err_ratelimited("RWP timeout, gone fishing\n");
> > - return;
> > - }
> > - cpu_relax();
> > - udelay(1);
> > - }
> > + if (readl_relaxed_poll_timeout_atomic(base + GICD_CTLR,
> > + val, !(val & bit), 1, 1000000) == -ETIMEDOUT)
>
> If you are doing this, please use a constant such as USEC_PER_SEC for
> the timeout. And fix the alignment of the second line so that the
> parameters are aligned vertically.
Agreed, well defined constant is always preferable than magic number;
And yes, alignment is for better readability.
>
> > + pr_err_ratelimited("RWP timeout, gone fishing\n");
> > }
> >
> > /* Wait for completion of a distributor change */
> > @@ -279,7 +274,6 @@ static void gic_redist_wait_for_rwp(void)
> > static void gic_enable_redist(bool enable)
> > {
> > void __iomem *rbase;
> > - u32 count = 1000000; /* 1s! */
> > u32 val;
> >
> > if (gic_data.flags & FLAGS_WORKAROUND_GICR_WAKER_MSM8996)
> > @@ -301,14 +295,9 @@ static void gic_enable_redist(bool enable)
> > return; /* No PM support in this redistributor */
> > }
> >
> > - while (--count) {
> > - val = readl_relaxed(rbase + GICR_WAKER);
> > - if (enable ^ (bool)(val & GICR_WAKER_ChildrenAsleep))
> > - break;
> > - cpu_relax();
> > - udelay(1);
> > - }
> > - if (!count)
> > + if (readl_relaxed_poll_timeout_atomic(rbase + GICR_WAKER,
> > + val, enable ^ (bool)(val & GICR_WAKER_ChildrenAsleep),
> > + 1, 1000000) == -ETIMEDOUT)
> > pr_err_ratelimited("redistributor failed to %s...\n",
> > enable ? "wakeup" : "sleep");
> > }
>
> Same thing here.
Ack.
I will address two issues above and send respin of V2.
Thanks,
Dawei
>
> M.
>
> --
> Without deviation from the norm, progress is not possible.
>
Hi Marc,
Thanks for the review.
On Thu, Jan 18, 2024 at 02:03:08PM +0000, Marc Zyngier wrote:
> On Thu, 18 Jan 2024 11:27:37 +0000,
> Dawei Li <[email protected]> wrote:
> >
> > GIC & GIC-v3 share same gic_irq() implementations, unify them into a
> > generic API.
> >
> > Signed-off-by: Dawei Li <[email protected]>
> > ---
> > drivers/irqchip/irq-gic-common.h | 5 +++++
> > drivers/irqchip/irq-gic-v3.c | 5 -----
> > drivers/irqchip/irq-gic.c | 5 -----
> > 3 files changed, 5 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/irqchip/irq-gic-common.h b/drivers/irqchip/irq-gic-common.h
> > index f407cce9ecaa..ed18db4ab2c5 100644
> > --- a/drivers/irqchip/irq-gic-common.h
> > +++ b/drivers/irqchip/irq-gic-common.h
> > @@ -19,6 +19,11 @@ struct gic_quirk {
> > u32 mask;
> > };
> >
> > +static inline unsigned int gic_irq(struct irq_data *d)
> > +{
> > + return d->hwirq;
> > +}
> > +
> > int gic_configure_irq(unsigned int irq, unsigned int type,
> > void __iomem *base, void (*sync_access)(void));
> > void gic_dist_config(void __iomem *base, int gic_irqs,
> > diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> > index b9d9375a3434..474a498a521e 100644
> > --- a/drivers/irqchip/irq-gic-v3.c
> > +++ b/drivers/irqchip/irq-gic-v3.c
> > @@ -181,11 +181,6 @@ static enum gic_intid_range get_intid_range(struct irq_data *d)
> > return __get_intid_range(d->hwirq);
> > }
> >
> > -static inline unsigned int gic_irq(struct irq_data *d)
> > -{
> > - return d->hwirq;
> > -}
> > -
>
> I'd rather not do that. If anything, I'd get rid of the helper
> altogether, as we have irqd_to_hwirq() that does the same job, and
> actually preserves the typing.
Yes, your solution is much better for it's truly generic, independent of
irq chip.
I will send respin of V2 as you suggested, and if I may, add your
suggested-by?
Thanks,
Dawei
>
> M.
>
> --
> Without deviation from the norm, progress is not possible.
>