[PATCH] clocksource/drivers/timer-of: Remove percpu irq related code

[PATCH] clocksource/drivers/timer-of: Remove percpu irq related code

[Daniel Lezcano]

Found by GCC's named address space checks:

timer-of.c:29:46: warning: incorrect type in argument 2 (different address spaces)
timer-of.c:29:46:    expected void [noderef] __percpu *
timer-of.c:29:46:    got struct clock_event_device *clkevt
timer-of.c:74:51: warning: incorrect type in argument 4 (different address spaces)
timer-of.c:74:51:    expected void [noderef] __percpu *percpu_dev_id
timer-of.c:74:51:    got struct clock_event_device *clkevt

It appears the code is incorrect as reported by Uros Bizjak:

"The referred code is questionable as it tries to reuse
the clkevent pointer once as percpu pointer and once as generic
pointer, which should be avoided."

The initial change proposed to do a coercitive cast which is probably
not right given the correct is incorrect.

This change removes the percpu related code as no drivers is using it.

Fixes: dc11bae785295 ("clocksource/drivers: Add timer-of common init routine")
Reported-by: Uros Bizjak <ubizjak@gmail.com>
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/clocksource/timer-of.c | 17 ++++-------------
 drivers/clocksource/timer-of.h |  1 -
 2 files changed, 4 insertions(+), 14 deletions(-)

diff --git a/drivers/clocksource/timer-of.c b/drivers/clocksource/timer-of.c
index c3f54d9912be..420202bf76e4 100644
--- a/drivers/clocksource/timer-of.c
+++ b/drivers/clocksource/timer-of.c
@@ -25,10 +25,7 @@ static __init void timer_of_irq_exit(struct of_timer_irq *of_irq)
 
 	struct clock_event_device *clkevt = &to->clkevt;
 
-	if (of_irq->percpu)
-		free_percpu_irq(of_irq->irq, clkevt);
-	else
-		free_irq(of_irq->irq, clkevt);
+	free_irq(of_irq->irq, clkevt);
 }
 
 /**
@@ -42,9 +39,6 @@ static __init void timer_of_irq_exit(struct of_timer_irq *of_irq)
  * - Get interrupt number by name
  * - Get interrupt number by index
  *
- * When the interrupt is per CPU, 'request_percpu_irq()' is called,
- * otherwise 'request_irq()' is used.
- *
  * Returns 0 on success, < 0 otherwise
  */
 static __init int timer_of_irq_init(struct device_node *np,
@@ -69,12 +63,9 @@ static __init int timer_of_irq_init(struct device_node *np,
 		return -EINVAL;
 	}
 
-	ret = of_irq->percpu ?
-		request_percpu_irq(of_irq->irq, of_irq->handler,
-				   np->full_name, clkevt) :
-		request_irq(of_irq->irq, of_irq->handler,
-			    of_irq->flags ? of_irq->flags : IRQF_TIMER,
-			    np->full_name, clkevt);
+	ret = request_irq(of_irq->irq, of_irq->handler,
+			  of_irq->flags ? of_irq->flags : IRQF_TIMER,
+			  np->full_name, clkevt);
 	if (ret) {
 		pr_err("Failed to request irq %d for %pOF\n", of_irq->irq, np);
 		return ret;
diff --git a/drivers/clocksource/timer-of.h b/drivers/clocksource/timer-of.h
index a5478f3e8589..01a2c6b7db06 100644
--- a/drivers/clocksource/timer-of.h
+++ b/drivers/clocksource/timer-of.h
@@ -11,7 +11,6 @@
 struct of_timer_irq {
 	int irq;
 	int index;
-	int percpu;
 	const char *name;
 	unsigned long flags;
 	irq_handler_t handler;
-- 
2.43.0

Re: [PATCH] clocksource/drivers/timer-of: Remove percpu irq related code

[Uros Bizjak]

On Mon, Aug 19, 2024 at 12:03 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> Found by GCC's named address space checks:
>
> timer-of.c:29:46: warning: incorrect type in argument 2 (different address spaces)
> timer-of.c:29:46:    expected void [noderef] __percpu *
> timer-of.c:29:46:    got struct clock_event_device *clkevt
> timer-of.c:74:51: warning: incorrect type in argument 4 (different address spaces)
> timer-of.c:74:51:    expected void [noderef] __percpu *percpu_dev_id
> timer-of.c:74:51:    got struct clock_event_device *clkevt

Actually, the above is what sparse reports. GCC's named address space
checks errors out with:

drivers/clocksource/timer-of.c: In function ‘timer_of_irq_exit’:
drivers/clocksource/timer-of.c:29:46: error: passing argument 2 of
‘free_percpu_irq’ from pointer to non-enclosed address space
  29 |                 free_percpu_irq(of_irq->irq, clkevt);
     |                                              ^~~~~~
In file included from drivers/clocksource/timer-of.c:8:
./include/linux/interrupt.h:201:43: note: expected ‘__seg_gs void *’
but argument is of type ‘struct clock_event_device *’
 201 | extern void free_percpu_irq(unsigned int, void __percpu *);
     |                                           ^~~~~~~~~~~~~~~
drivers/clocksource/timer-of.c: In function ‘timer_of_irq_init’:
drivers/clocksource/timer-of.c:74:51: error: passing argument 4 of
‘request_percpu_irq’ from pointer to non-enclosed address space
  74 |                                    np->full_name, clkevt) :
     |                                                   ^~~~~~
./include/linux/interrupt.h:190:56: note: expected ‘__seg_gs void *’
but argument is of type ‘struct clock_event_device *’
 190 |                    const char *devname, void __percpu *percpu_dev_id)
     |                                         ~~~~~~~~~~~~~~~^~~~~~~~~~~~~

Anyway, your patch fixes the above build errors, as well as sparse
checking warnings.

Tested-by: Uros Bizjak <ubizjak@gmail.com>

Thanks,
Uros.

Re: [PATCH] clocksource/drivers/timer-of: Remove percpu irq related code

[Daniel Lezcano]

On 19/08/2024 13:12, Uros Bizjak wrote:
> On Mon, Aug 19, 2024 at 12:03 PM Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
>>
>> Found by GCC's named address space checks:
>>
>> timer-of.c:29:46: warning: incorrect type in argument 2 (different address spaces)
>> timer-of.c:29:46:    expected void [noderef] __percpu *
>> timer-of.c:29:46:    got struct clock_event_device *clkevt
>> timer-of.c:74:51: warning: incorrect type in argument 4 (different address spaces)
>> timer-of.c:74:51:    expected void [noderef] __percpu *percpu_dev_id
>> timer-of.c:74:51:    got struct clock_event_device *clkevt
> 
> Actually, the above is what sparse reports. GCC's named address space
> checks errors out with:
> 
> drivers/clocksource/timer-of.c: In function ‘timer_of_irq_exit’:
> drivers/clocksource/timer-of.c:29:46: error: passing argument 2 of
> ‘free_percpu_irq’ from pointer to non-enclosed address space
>    29 |                 free_percpu_irq(of_irq->irq, clkevt);
>       |                                              ^~~~~~
> In file included from drivers/clocksource/timer-of.c:8:
> ./include/linux/interrupt.h:201:43: note: expected ‘__seg_gs void *’
> but argument is of type ‘struct clock_event_device *’
>   201 | extern void free_percpu_irq(unsigned int, void __percpu *);
>       |                                           ^~~~~~~~~~~~~~~
> drivers/clocksource/timer-of.c: In function ‘timer_of_irq_init’:
> drivers/clocksource/timer-of.c:74:51: error: passing argument 4 of
> ‘request_percpu_irq’ from pointer to non-enclosed address space
>    74 |                                    np->full_name, clkevt) :
>       |                                                   ^~~~~~
> ./include/linux/interrupt.h:190:56: note: expected ‘__seg_gs void *’
> but argument is of type ‘struct clock_event_device *’
>   190 |                    const char *devname, void __percpu *percpu_dev_id)
>       |                                         ~~~~~~~~~~~~~~~^~~~~~~~~~~~~
> 
> Anyway, your patch fixes the above build errors, as well as sparse
> checking warnings.

Thanks I'll amend the description with your comment

> Tested-by: Uros Bizjak <ubizjak@gmail.com>

Thanks for testing

-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

[tip: timers/urgent] clocksource/drivers/timer-of: Remove percpu irq related code

[tip-bot2 for Daniel Lezcano]

The following commit has been merged into the timers/urgent branch of tip:

Commit-ID:     471ef0b5a8aaca4296108e756b970acfc499ede4
Gitweb:        https://git.kernel.org/tip/471ef0b5a8aaca4296108e756b970acfc499ede4
Author:        Daniel Lezcano <daniel.lezcano@linaro.org>
AuthorDate:    Mon, 19 Aug 2024 12:03:35 +02:00
Committer:     Daniel Lezcano <daniel.lezcano@linaro.org>
CommitterDate: Mon, 02 Sep 2024 10:04:15 +02:00

clocksource/drivers/timer-of: Remove percpu irq related code

GCC's named address space checks errors out with:

drivers/clocksource/timer-of.c: In function ‘timer_of_irq_exit’:
drivers/clocksource/timer-of.c:29:46: error: passing argument 2 of
‘free_percpu_irq’ from pointer to non-enclosed address space
  29 |                 free_percpu_irq(of_irq->irq, clkevt);
     |                                              ^~~~~~
In file included from drivers/clocksource/timer-of.c:8:
./include/linux/interrupt.h:201:43: note: expected ‘__seg_gs void *’
but argument is of type ‘struct clock_event_device *’
 201 | extern void free_percpu_irq(unsigned int, void __percpu *);
     |                                           ^~~~~~~~~~~~~~~
drivers/clocksource/timer-of.c: In function ‘timer_of_irq_init’:
drivers/clocksource/timer-of.c:74:51: error: passing argument 4 of
‘request_percpu_irq’ from pointer to non-enclosed address space
  74 |                                    np->full_name, clkevt) :
     |                                                   ^~~~~~
./include/linux/interrupt.h:190:56: note: expected ‘__seg_gs void *’
but argument is of type ‘struct clock_event_device *’
 190 |                    const char *devname, void __percpu *percpu_dev_id)

Sparse warns about:

timer-of.c:29:46: warning: incorrect type in argument 2 (different address spaces)
timer-of.c:29:46:    expected void [noderef] __percpu *
timer-of.c:29:46:    got struct clock_event_device *clkevt
timer-of.c:74:51: warning: incorrect type in argument 4 (different address spaces)
timer-of.c:74:51:    expected void [noderef] __percpu *percpu_dev_id
timer-of.c:74:51:    got struct clock_event_device *clkevt

It appears the code is incorrect as reported by Uros Bizjak:

"The referred code is questionable as it tries to reuse
the clkevent pointer once as percpu pointer and once as generic
pointer, which should be avoided."

This change removes the percpu related code as no drivers is using it.

[Daniel: Fixed the description]

Fixes: dc11bae785295 ("clocksource/drivers: Add timer-of common init routine")
Reported-by: Uros Bizjak <ubizjak@gmail.com>
Tested-by: Uros Bizjak <ubizjak@gmail.com>
Link: https://lore.kernel.org/r/20240819100335.2394751-1-daniel.lezcano@linaro.org
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/clocksource/timer-of.c | 17 ++++-------------
 drivers/clocksource/timer-of.h |  1 -
 2 files changed, 4 insertions(+), 14 deletions(-)

diff --git a/drivers/clocksource/timer-of.c b/drivers/clocksource/timer-of.c
index c3f54d9..420202b 100644
--- a/drivers/clocksource/timer-of.c
+++ b/drivers/clocksource/timer-of.c
@@ -25,10 +25,7 @@ static __init void timer_of_irq_exit(struct of_timer_irq *of_irq)
 
 	struct clock_event_device *clkevt = &to->clkevt;
 
-	if (of_irq->percpu)
-		free_percpu_irq(of_irq->irq, clkevt);
-	else
-		free_irq(of_irq->irq, clkevt);
+	free_irq(of_irq->irq, clkevt);
 }
 
 /**
@@ -42,9 +39,6 @@ static __init void timer_of_irq_exit(struct of_timer_irq *of_irq)
  * - Get interrupt number by name
  * - Get interrupt number by index
  *
- * When the interrupt is per CPU, 'request_percpu_irq()' is called,
- * otherwise 'request_irq()' is used.
- *
  * Returns 0 on success, < 0 otherwise
  */
 static __init int timer_of_irq_init(struct device_node *np,
@@ -69,12 +63,9 @@ static __init int timer_of_irq_init(struct device_node *np,
 		return -EINVAL;
 	}
 
-	ret = of_irq->percpu ?
-		request_percpu_irq(of_irq->irq, of_irq->handler,
-				   np->full_name, clkevt) :
-		request_irq(of_irq->irq, of_irq->handler,
-			    of_irq->flags ? of_irq->flags : IRQF_TIMER,
-			    np->full_name, clkevt);
+	ret = request_irq(of_irq->irq, of_irq->handler,
+			  of_irq->flags ? of_irq->flags : IRQF_TIMER,
+			  np->full_name, clkevt);
 	if (ret) {
 		pr_err("Failed to request irq %d for %pOF\n", of_irq->irq, np);
 		return ret;
diff --git a/drivers/clocksource/timer-of.h b/drivers/clocksource/timer-of.h
index a5478f3..01a2c6b 100644
--- a/drivers/clocksource/timer-of.h
+++ b/drivers/clocksource/timer-of.h
@@ -11,7 +11,6 @@
 struct of_timer_irq {
 	int irq;
 	int index;
-	int percpu;
 	const char *name;
 	unsigned long flags;
 	irq_handler_t handler;