2020-03-01 12:23:22

by afzal mohammed

[permalink] [raw]
Subject: [PATCH v3] ARM: footbridge: replace setup_irq() by request_irq()

request_irq() is preferred over setup_irq(). Invocations of setup_irq()
occur after memory allocators are ready.

Per tglx[1], setup_irq() existed in olden days when allocators were not
ready by the time early interrupts were initialized.

Hence replace setup_irq() by request_irq().

[1] https://lkml.kernel.org/r/alpine.DEB.2.20.1710191609480.1971@nanos

Signed-off-by: afzal mohammed <[email protected]>
---
Hi sub-arch maintainers,

If the patch is okay, please take it thr' your tree.

Regards
afzal

v3:
* Split out from series, also create subarch level patch as Thomas
suggested to take it thr' respective maintainers
* Modify string displayed in case of error as suggested by Thomas
* Re-arrange code as required to improve readability
* Remove irrelevant parts from commit message & improve

v2:
* Replace pr_err("request_irq() on %s failed" by
pr_err("%s: request_irq() failed"
* Commit message massage

arch/arm/mach-footbridge/dc21285-timer.c | 11 +++--------
arch/arm/mach-footbridge/isa-irq.c | 10 ++++------
arch/arm/mach-footbridge/isa-timer.c | 11 +++--------
3 files changed, 10 insertions(+), 22 deletions(-)

diff --git a/arch/arm/mach-footbridge/dc21285-timer.c b/arch/arm/mach-footbridge/dc21285-timer.c
index f76212d2dbf1..2908c9ef3c9b 100644
--- a/arch/arm/mach-footbridge/dc21285-timer.c
+++ b/arch/arm/mach-footbridge/dc21285-timer.c
@@ -101,13 +101,6 @@ static irqreturn_t timer1_interrupt(int irq, void *dev_id)
return IRQ_HANDLED;
}

-static struct irqaction footbridge_timer_irq = {
- .name = "dc21285_timer1",
- .handler = timer1_interrupt,
- .flags = IRQF_TIMER | IRQF_IRQPOLL,
- .dev_id = &ckevt_dc21285,
-};
-
/*
* Set up timer interrupt.
*/
@@ -118,7 +111,9 @@ void __init footbridge_timer_init(void)

clocksource_register_hz(&cksrc_dc21285, rate);

- setup_irq(ce->irq, &footbridge_timer_irq);
+ if (request_irq(ce->irq, timer1_interrupt, IRQF_TIMER | IRQF_IRQPOLL,
+ "dc21285_timer1", &ckevt_dc21285))
+ pr_err("Failed to request irq %d (dc21285_timer1)", ce->irq);

ce->cpumask = cpumask_of(smp_processor_id());
clockevents_config_and_register(ce, rate, 0x4, 0xffffff);
diff --git a/arch/arm/mach-footbridge/isa-irq.c b/arch/arm/mach-footbridge/isa-irq.c
index 88a553932c33..16c5455199e8 100644
--- a/arch/arm/mach-footbridge/isa-irq.c
+++ b/arch/arm/mach-footbridge/isa-irq.c
@@ -96,11 +96,6 @@ static void isa_irq_handler(struct irq_desc *desc)
generic_handle_irq(isa_irq);
}

-static struct irqaction irq_cascade = {
- .handler = no_action,
- .name = "cascade",
-};
-
static struct resource pic1_resource = {
.name = "pic1",
.start = 0x20,
@@ -146,6 +141,8 @@ void __init isa_init_irq(unsigned int host_irq)
}

if (host_irq != (unsigned int)-1) {
+ int irq = IRQ_ISA_CASCADE;
+
for (irq = _ISA_IRQ(0); irq < _ISA_IRQ(8); irq++) {
irq_set_chip_and_handler(irq, &isa_lo_chip,
handle_level_irq);
@@ -160,7 +157,8 @@ void __init isa_init_irq(unsigned int host_irq)

request_resource(&ioport_resource, &pic1_resource);
request_resource(&ioport_resource, &pic2_resource);
- setup_irq(IRQ_ISA_CASCADE, &irq_cascade);
+ if (request_irq(irq, no_action, 0, "cascade", NULL))
+ pr_err("Failed to request irq %d (cascade)\n", irq);

irq_set_chained_handler(host_irq, isa_irq_handler);

diff --git a/arch/arm/mach-footbridge/isa-timer.c b/arch/arm/mach-footbridge/isa-timer.c
index 82f45591fb2c..723e3eae995d 100644
--- a/arch/arm/mach-footbridge/isa-timer.c
+++ b/arch/arm/mach-footbridge/isa-timer.c
@@ -25,17 +25,12 @@ static irqreturn_t pit_timer_interrupt(int irq, void *dev_id)
return IRQ_HANDLED;
}

-static struct irqaction pit_timer_irq = {
- .name = "pit",
- .handler = pit_timer_interrupt,
- .flags = IRQF_TIMER | IRQF_IRQPOLL,
- .dev_id = &i8253_clockevent,
-};
-
void __init isa_timer_init(void)
{
clocksource_i8253_init();

- setup_irq(i8253_clockevent.irq, &pit_timer_irq);
+ if (request_irq(i8253_clockevent.irq, pit_timer_interrupt,
+ IRQF_TIMER | IRQF_IRQPOLL, "pit", &i8253_clockevent))
+ pr_err("Failed to request irq %d(pit)\n", i8253_clockevent.irq);
clockevent_i8253_init(false);
}
--
2.25.1


2020-03-12 12:35:46

by Russell King (Oracle)

[permalink] [raw]
Subject: Re: [PATCH v3] ARM: footbridge: replace setup_irq() by request_irq()

On Sun, Mar 01, 2020 at 05:51:31PM +0530, afzal mohammed wrote:
> request_irq() is preferred over setup_irq(). Invocations of setup_irq()
> occur after memory allocators are ready.
>
> Per tglx[1], setup_irq() existed in olden days when allocators were not
> ready by the time early interrupts were initialized.
>
> Hence replace setup_irq() by request_irq().
>
> [1] https://lkml.kernel.org/r/alpine.DEB.2.20.1710191609480.1971@nanos
>
> Signed-off-by: afzal mohammed <[email protected]>
> ---
> Hi sub-arch maintainers,
>
> If the patch is okay, please take it thr' your tree.

This patch causes a build warning:

arch/arm/mach-footbridge/isa-irq.c:113:15: warning: unused variable 'irq' [-Wunused-variable]

because you introduce a new 'int irq' variable in a sub-block where the
parent already declares 'irq', causing the parent 'irq' to be unused.

Hence, I'm dropping this patch.

I think you need to look more carefully at the code you are modifying,
and maybe even build test it. Cross compilers are available from
kernel.org.

Russell.

>
> Regards
> afzal
>
> v3:
> * Split out from series, also create subarch level patch as Thomas
> suggested to take it thr' respective maintainers
> * Modify string displayed in case of error as suggested by Thomas
> * Re-arrange code as required to improve readability
> * Remove irrelevant parts from commit message & improve
>
> v2:
> * Replace pr_err("request_irq() on %s failed" by
> pr_err("%s: request_irq() failed"
> * Commit message massage
>
> arch/arm/mach-footbridge/dc21285-timer.c | 11 +++--------
> arch/arm/mach-footbridge/isa-irq.c | 10 ++++------
> arch/arm/mach-footbridge/isa-timer.c | 11 +++--------
> 3 files changed, 10 insertions(+), 22 deletions(-)
>
> diff --git a/arch/arm/mach-footbridge/dc21285-timer.c b/arch/arm/mach-footbridge/dc21285-timer.c
> index f76212d2dbf1..2908c9ef3c9b 100644
> --- a/arch/arm/mach-footbridge/dc21285-timer.c
> +++ b/arch/arm/mach-footbridge/dc21285-timer.c
> @@ -101,13 +101,6 @@ static irqreturn_t timer1_interrupt(int irq, void *dev_id)
> return IRQ_HANDLED;
> }
>
> -static struct irqaction footbridge_timer_irq = {
> - .name = "dc21285_timer1",
> - .handler = timer1_interrupt,
> - .flags = IRQF_TIMER | IRQF_IRQPOLL,
> - .dev_id = &ckevt_dc21285,
> -};
> -
> /*
> * Set up timer interrupt.
> */
> @@ -118,7 +111,9 @@ void __init footbridge_timer_init(void)
>
> clocksource_register_hz(&cksrc_dc21285, rate);
>
> - setup_irq(ce->irq, &footbridge_timer_irq);
> + if (request_irq(ce->irq, timer1_interrupt, IRQF_TIMER | IRQF_IRQPOLL,
> + "dc21285_timer1", &ckevt_dc21285))
> + pr_err("Failed to request irq %d (dc21285_timer1)", ce->irq);
>
> ce->cpumask = cpumask_of(smp_processor_id());
> clockevents_config_and_register(ce, rate, 0x4, 0xffffff);
> diff --git a/arch/arm/mach-footbridge/isa-irq.c b/arch/arm/mach-footbridge/isa-irq.c
> index 88a553932c33..16c5455199e8 100644
> --- a/arch/arm/mach-footbridge/isa-irq.c
> +++ b/arch/arm/mach-footbridge/isa-irq.c
> @@ -96,11 +96,6 @@ static void isa_irq_handler(struct irq_desc *desc)
> generic_handle_irq(isa_irq);
> }
>
> -static struct irqaction irq_cascade = {
> - .handler = no_action,
> - .name = "cascade",
> -};
> -
> static struct resource pic1_resource = {
> .name = "pic1",
> .start = 0x20,
> @@ -146,6 +141,8 @@ void __init isa_init_irq(unsigned int host_irq)
> }
>
> if (host_irq != (unsigned int)-1) {
> + int irq = IRQ_ISA_CASCADE;
> +
> for (irq = _ISA_IRQ(0); irq < _ISA_IRQ(8); irq++) {
> irq_set_chip_and_handler(irq, &isa_lo_chip,
> handle_level_irq);
> @@ -160,7 +157,8 @@ void __init isa_init_irq(unsigned int host_irq)
>
> request_resource(&ioport_resource, &pic1_resource);
> request_resource(&ioport_resource, &pic2_resource);
> - setup_irq(IRQ_ISA_CASCADE, &irq_cascade);
> + if (request_irq(irq, no_action, 0, "cascade", NULL))
> + pr_err("Failed to request irq %d (cascade)\n", irq);
>
> irq_set_chained_handler(host_irq, isa_irq_handler);
>
> diff --git a/arch/arm/mach-footbridge/isa-timer.c b/arch/arm/mach-footbridge/isa-timer.c
> index 82f45591fb2c..723e3eae995d 100644
> --- a/arch/arm/mach-footbridge/isa-timer.c
> +++ b/arch/arm/mach-footbridge/isa-timer.c
> @@ -25,17 +25,12 @@ static irqreturn_t pit_timer_interrupt(int irq, void *dev_id)
> return IRQ_HANDLED;
> }
>
> -static struct irqaction pit_timer_irq = {
> - .name = "pit",
> - .handler = pit_timer_interrupt,
> - .flags = IRQF_TIMER | IRQF_IRQPOLL,
> - .dev_id = &i8253_clockevent,
> -};
> -
> void __init isa_timer_init(void)
> {
> clocksource_i8253_init();
>
> - setup_irq(i8253_clockevent.irq, &pit_timer_irq);
> + if (request_irq(i8253_clockevent.irq, pit_timer_interrupt,
> + IRQF_TIMER | IRQF_IRQPOLL, "pit", &i8253_clockevent))
> + pr_err("Failed to request irq %d(pit)\n", i8253_clockevent.irq);
> clockevent_i8253_init(false);
> }
> --
> 2.25.1
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>

--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

2020-03-12 13:13:38

by afzal mohammed

[permalink] [raw]
Subject: Re: [PATCH v3] ARM: footbridge: replace setup_irq() by request_irq()

Hi Russell,

On Thu, Mar 12, 2020 at 12:34:32PM +0000, Russell King - ARM Linux admin wrote:

> This patch causes a build warning:

> Hence, I'm dropping this patch.
>
> I think you need to look more carefully at the code you are modifying,
> and maybe even build test it. Cross compilers are available from
> kernel.org.

Sorry for the sloppy approach.

i build & boot tested only using ARM multi_v7_defconfig which was
building only few of my changes, i had the cross compiler. i was trying
to rely on kbuild test robot for the other configs earlier, from the
begining instead i should have made sure that each modified code builds
w/o warning.

i am fixing it.

Regards
afzal

2020-03-12 13:28:27

by afzal mohammed

[permalink] [raw]
Subject: [PATCH v4] ARM: footbridge: replace setup_irq() by request_irq()

request_irq() is preferred over setup_irq(). Invocations of setup_irq()
occur after memory allocators are ready.

Per tglx[1], setup_irq() existed in olden days when allocators were not
ready by the time early interrupts were initialized.

Hence replace setup_irq() by request_irq().

[1] https://lkml.kernel.org/r/alpine.DEB.2.20.1710191609480.1971@nanos

Signed-off-by: afzal mohammed <[email protected]>
---

v4:
* Fix build warning in isa-irq.c & ensure no build warnings
v3:
* Split out from series, also create subarch level patch as Thomas
suggested to take it thr' respective maintainers
* Modify string displayed in case of error as suggested by Thomas
* Re-arrange code as required to improve readability
* Remove irrelevant parts from commit message & improve

v2:
* Replace pr_err("request_irq() on %s failed" by
pr_err("%s: request_irq() failed"
* Commit message massage

arch/arm/mach-footbridge/dc21285-timer.c | 11 +++--------
arch/arm/mach-footbridge/isa-irq.c | 10 ++++------
arch/arm/mach-footbridge/isa-timer.c | 11 +++--------
3 files changed, 10 insertions(+), 22 deletions(-)

diff --git a/arch/arm/mach-footbridge/dc21285-timer.c b/arch/arm/mach-footbridge/dc21285-timer.c
index f76212d2dbf1..2908c9ef3c9b 100644
--- a/arch/arm/mach-footbridge/dc21285-timer.c
+++ b/arch/arm/mach-footbridge/dc21285-timer.c
@@ -101,13 +101,6 @@ static irqreturn_t timer1_interrupt(int irq, void *dev_id)
return IRQ_HANDLED;
}

-static struct irqaction footbridge_timer_irq = {
- .name = "dc21285_timer1",
- .handler = timer1_interrupt,
- .flags = IRQF_TIMER | IRQF_IRQPOLL,
- .dev_id = &ckevt_dc21285,
-};
-
/*
* Set up timer interrupt.
*/
@@ -118,7 +111,9 @@ void __init footbridge_timer_init(void)

clocksource_register_hz(&cksrc_dc21285, rate);

- setup_irq(ce->irq, &footbridge_timer_irq);
+ if (request_irq(ce->irq, timer1_interrupt, IRQF_TIMER | IRQF_IRQPOLL,
+ "dc21285_timer1", &ckevt_dc21285))
+ pr_err("Failed to request irq %d (dc21285_timer1)", ce->irq);

ce->cpumask = cpumask_of(smp_processor_id());
clockevents_config_and_register(ce, rate, 0x4, 0xffffff);
diff --git a/arch/arm/mach-footbridge/isa-irq.c b/arch/arm/mach-footbridge/isa-irq.c
index 88a553932c33..842ddb4121ef 100644
--- a/arch/arm/mach-footbridge/isa-irq.c
+++ b/arch/arm/mach-footbridge/isa-irq.c
@@ -96,11 +96,6 @@ static void isa_irq_handler(struct irq_desc *desc)
generic_handle_irq(isa_irq);
}

-static struct irqaction irq_cascade = {
- .handler = no_action,
- .name = "cascade",
-};
-
static struct resource pic1_resource = {
.name = "pic1",
.start = 0x20,
@@ -160,7 +155,10 @@ void __init isa_init_irq(unsigned int host_irq)

request_resource(&ioport_resource, &pic1_resource);
request_resource(&ioport_resource, &pic2_resource);
- setup_irq(IRQ_ISA_CASCADE, &irq_cascade);
+
+ irq = IRQ_ISA_CASCADE;
+ if (request_irq(irq, no_action, 0, "cascade", NULL))
+ pr_err("Failed to request irq %u (cascade)\n", irq);

irq_set_chained_handler(host_irq, isa_irq_handler);

diff --git a/arch/arm/mach-footbridge/isa-timer.c b/arch/arm/mach-footbridge/isa-timer.c
index 82f45591fb2c..723e3eae995d 100644
--- a/arch/arm/mach-footbridge/isa-timer.c
+++ b/arch/arm/mach-footbridge/isa-timer.c
@@ -25,17 +25,12 @@ static irqreturn_t pit_timer_interrupt(int irq, void *dev_id)
return IRQ_HANDLED;
}

-static struct irqaction pit_timer_irq = {
- .name = "pit",
- .handler = pit_timer_interrupt,
- .flags = IRQF_TIMER | IRQF_IRQPOLL,
- .dev_id = &i8253_clockevent,
-};
-
void __init isa_timer_init(void)
{
clocksource_i8253_init();

- setup_irq(i8253_clockevent.irq, &pit_timer_irq);
+ if (request_irq(i8253_clockevent.irq, pit_timer_interrupt,
+ IRQF_TIMER | IRQF_IRQPOLL, "pit", &i8253_clockevent))
+ pr_err("Failed to request irq %d(pit)\n", i8253_clockevent.irq);
clockevent_i8253_init(false);
}
--
2.25.1