ARM: s3c: irq-s3c24xx: Fix return value check for s3c24xx_init_intc()

PATCH Superseded Jackie Liu 3 participants 4 messages Aug 31, 2021 – Sep 1, 2021

Subsystems (3)

Arm (32-bit) Arm sub-architectures Arm Samsung Exynos

lore

Share & tools

lore

Jackie Liu

author

2021-08-31 09:57 UTC #0 lore raw

PATCH 1/1 ARM: s3c: irq-s3c24xx: Fix return value check for s3c24xx_init_intc()

From: Jackie Liu <[email protected]>

The s3c24xx_init_intc() returns an error pointer upon failure, not NULL. let's add an error pointer check in s3c24xx_handle_irq.

Fixes 1f629b7a3ced ("ARM: S3C24XX: transform irq handling into a declarative form")

Signed-off-by Jackie Liu <[email protected]>

---
 arch/arm/mach-s3c/irq-s3c24xx.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

arch/arm/mach-s3c/irq-s3c24xx.c +2 -2

index 0c631c14a817..d58bf0f9bf9a 100644
@@ -362,11 +362,11 @@ static inline int s3c24xx_handle_intc(struct s3c_irq_intc *intc,
 static asmlinkage void __exception_irq_entry s3c24xx_handle_irq(struct pt_regs *regs)
 {
 	do {
-		if (likely(s3c_intc[0]))
+		if (likely(!IS_ERR_OR_NULL(s3c_intc[0])))
 			if (s3c24xx_handle_intc(s3c_intc[0], regs, 0))
 				continue;
 
-		if (s3c_intc[2])
+		if (!IS_ERR_OR_NULL(s3c_intc[2]))
 			if (s3c24xx_handle_intc(s3c_intc[2], regs, 64))
 				continue;

Signature · 1 line

2.25.1

Krzysztof Kozlowski

2021-09-01 12:09 UTC #1 lore raw

Quoted 7 lines · Jackie Liu

From: Jackie Liu <[email protected]>
The s3c24xx_init_intc() returns an error pointer upon failure, not NULL. let's add an error pointer check in s3c24xx_handle_irq.
Fixes 1f629b7a3ced ("ARM: S3C24XX: transform irq handling into a declarative form")
Signed-off-by Jackie Liu <[email protected]>

Please use scripts/get_maintainers.pl to get list of mailing lists to CC. You skipped two - arm and LKML.

Quoted 9 lines

---
 arch/arm/mach-s3c/irq-s3c24xx.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

arch/arm/mach-s3c/irq-s3c24xx.c +0 -0

index 0c631c14a817..d58bf0f9bf9a 100644

arch/arm/mach-s3c/irq-s3c24xx.c static inline int s3c24xx_handle_intc(struct s3c_irq_intc *intc, · line 366

Quoted 6 lines

arch/arm/mach-s3c/irq-s3c24xx.c +1 -1

@@ -362,11 +362,11 @@ static inline int s3c24xx_handle_intc(struct s3c_irq_intc *intc,
 static asmlinkage void __exception_irq_entry s3c24xx_handle_irq(struct pt_regs *regs)
 {
 	do {
-		if (likely(s3c_intc[0]))
+		if (likely(!IS_ERR_OR_NULL(s3c_intc[0])))
 			if (s3c24xx_handle_intc(s3c_intc[0], regs, 0))

Thanks for the patch but it does not look entirely correct.

For platform based machines, neither ERR nor NULL can happen here. The s3c24xx_handle_irq() will be set as IRQ handler iff this succeeds:

    s3c_intc[0] = s3c24xx_init_intc()

If this fails, the next calls to s3c24xx_init_intc() won't be executed.

For DT machine, s3c_init_intc_of() could set the IRQ handler without setting s3c_intc[0] only if it was called with num_ctrl=0. There is no such code path, so again the s3c_intc[0] will have a valid pointer if set_handle_irq() is called.

Therefore in s3c24xx_handle_irq(), the s3c_intc[0] is always something.

The code can be simplified by removing if(), if we really wanted and were sure about it.

arch/arm/mach-s3c/irq-s3c24xx.c static inline int s3c24xx_handle_intc(struct s3c_irq_intc *intc, · line 369

Quoted 4 lines

arch/arm/mach-s3c/irq-s3c24xx.c +1 -1

 				continue;
 
-		if (s3c_intc[2])
+		if (!IS_ERR_OR_NULL(s3c_intc[2]))

For the non-DT case, this seems ugly but proper solution. The s3c_intc[2] could be NULL (not set at all) or set as ERR (if s3c24xx_init_intc() fails).

arch/arm/mach-s3c/irq-s3c24xx.c static inline int s3c24xx_handle_intc(struct s3c_irq_intc *intc, · line 372

Quoted 4 lines

arch/arm/mach-s3c/irq-s3c24xx.c +0 -0

 			if (s3c24xx_handle_intc(s3c_intc[2], regs, 64))
 				continue;

Best regards,
Krzysztof

Jackie Liu

2021-09-01 12:29 UTC #2 lore raw

Hi Krzysztof, Thanks for you message.

Quoted 41 lines · Krzysztof Kozlowski

Quoted 7 lines · Jackie Liu
From: Jackie Liu <[email protected]>
The s3c24xx_init_intc() returns an error pointer upon failure, not NULL. let's add an error pointer check in s3c24xx_handle_irq.
Fixes 1f629b7a3ced ("ARM: S3C24XX: transform irq handling into a declarative form")
Signed-off-by Jackie Liu <[email protected]>
Please use scripts/get_maintainers.pl to get list of mailing lists to CC. You skipped two - arm and LKML.
Quoted 9 lines
---
  arch/arm/mach-s3c/irq-s3c24xx.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)
arch/arm/mach-s3c/irq-s3c24xx.c +0 -0
index 0c631c14a817..d58bf0f9bf9a 100644
arch/arm/mach-s3c/irq-s3c24xx.c static inline int s3c24xx_handle_intc(struct s3c_irq_intc *intc, · line 366
Quoted 6 lines
arch/arm/mach-s3c/irq-s3c24xx.c +1 -1
@@ -362,11 +362,11 @@ static inline int s3c24xx_handle_intc(struct s3c_irq_intc *intc,
  static asmlinkage void __exception_irq_entry s3c24xx_handle_irq(struct pt_regs *regs)
  {
  	do {
-		if (likely(s3c_intc[0]))
+		if (likely(!IS_ERR_OR_NULL(s3c_intc[0])))
  			if (s3c24xx_handle_intc(s3c_intc[0], regs, 0))
Thanks for the patch but it does not look entirely correct.
For platform based machines, neither ERR nor NULL can happen here. The s3c24xx_handle_irq() will be set as IRQ handler iff this succeeds:
     s3c_intc[0] = s3c24xx_init_intc()
If this fails, the next calls to s3c24xx_init_intc() won't be executed.
For DT machine, s3c_init_intc_of() could set the IRQ handler without setting s3c_intc[0] only if it was called with num_ctrl=0. There is no such code path, so again the s3c_intc[0] will have a valid pointer if set_handle_irq() is called.

Quoted 4 lines

Therefore in s3c24xx_handle_irq(), the s3c_intc[0] is always something.
The code can be simplified by removing if(), if we really wanted and were sure about it.

In fact, I didn't study his underlying logic in depth, but found that this place was not particularly perfect based on the return value of the function, because I happened to encounter a similar problem elsewhere.

Quoted 11 lines

arch/arm/mach-s3c/irq-s3c24xx.c static inline int s3c24xx_handle_intc(struct s3c_irq_intc *intc, · line 369
Quoted 6 lines
arch/arm/mach-s3c/irq-s3c24xx.c +1 -1
  				continue;
 
-		if (s3c_intc[2])
+		if (!IS_ERR_OR_NULL(s3c_intc[2]))
For the non-DT case, this seems ugly but proper solution. The s3c_intc[2] could be NULL (not set at all) or set as ERR (if s3c24xx_init_intc() fails).

Quoted 9 lines

arch/arm/mach-s3c/irq-s3c24xx.c static inline int s3c24xx_handle_intc(struct s3c_irq_intc *intc, · line 372
Quoted 4 lines
arch/arm/mach-s3c/irq-s3c24xx.c +0 -0
  			if (s3c24xx_handle_intc(s3c_intc[2], regs, 64))
  				continue;
 
Best regards,
Krzysztof

Would you mind review v2?

--------------------------------------------------------------

(quoted diff) +0 -0

b/arch/arm/mach-s3c/irq-s3c24xx.c
index 0c631c14a817..df471d322493 100644

arch/arm/mach-s3c/irq-s3c24xx.c +0 -0

@@ -362,11 +362,24 @@ static inline int s3c24xx_handle_intc(struct

s3c_irq_intc *intc,
static asmlinkage void __exception_irq_entry s3c24xx_handle_irq(struct
pt_regs *regs)

  {
         do {

- if (likely(s3c_intc[0]))
- if (s3c24xx_handle_intc(s3c_intc[0], regs, 0))
- continue;
+ /* For platform based machines, neither ERR nor NULL can
happen here.
+ * The s3c24xx_handle_irq() will be set as IRQ handler
iff this succeeds:

+                *
+                *    s3c_intc[0] = s3c24xx_init_intc()
+                *
+                * If this fails, the next calls to s3c24xx_init_intc()

won't be executed.
+ *
+ * For DT machine, s3c_init_intc_of() could set the IRQ
handler without
+ * setting s3c_intc[0] only if it was called with
num_ctrl=0. There is no
+ * such code path, so again the s3c_intc[0] will have a
valid pointer if

+                * set_handle_irq() is called.
+                *
+                * Therefore in s3c24xx_handle_irq(), the s3c_intc[0] is

always something.
+ */
+ if (s3c24xx_handle_intc(s3c_intc[0], regs, 0))
+ continue;

- if (s3c_intc[2])
+ if (!IS_ERR_OR_NULL(s3c_intc[2]))

                         if (s3c24xx_handle_intc(s3c_intc[2], regs, 64))
                                 continue;

---
Thanks, Jackie

Krzysztof Kozlowski

2021-09-01 12:35 UTC #3 lore raw

Quoted 71 lines · Jackie Liu

Hi Krzysztof, Thanks for you message.
Quoted 41 lines · Krzysztof Kozlowski
Quoted 7 lines · Jackie Liu
From: Jackie Liu <[email protected]>
The s3c24xx_init_intc() returns an error pointer upon failure, not NULL. let's add an error pointer check in s3c24xx_handle_irq.
Fixes 1f629b7a3ced ("ARM: S3C24XX: transform irq handling into a declarative form")
Signed-off-by Jackie Liu <[email protected]>
Please use scripts/get_maintainers.pl to get list of mailing lists to CC. You skipped two - arm and LKML.
Quoted 9 lines
---
  arch/arm/mach-s3c/irq-s3c24xx.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)
arch/arm/mach-s3c/irq-s3c24xx.c +0 -0
index 0c631c14a817..d58bf0f9bf9a 100644
arch/arm/mach-s3c/irq-s3c24xx.c static inline int s3c24xx_handle_intc(struct s3c_irq_intc *intc, · line 366
Quoted 6 lines
arch/arm/mach-s3c/irq-s3c24xx.c +1 -1
@@ -362,11 +362,11 @@ static inline int s3c24xx_handle_intc(struct s3c_irq_intc *intc,
  static asmlinkage void __exception_irq_entry s3c24xx_handle_irq(struct pt_regs *regs)
  {
  	do {
-		if (likely(s3c_intc[0]))
+		if (likely(!IS_ERR_OR_NULL(s3c_intc[0])))
  			if (s3c24xx_handle_intc(s3c_intc[0], regs, 0))
Thanks for the patch but it does not look entirely correct.
For platform based machines, neither ERR nor NULL can happen here. The s3c24xx_handle_irq() will be set as IRQ handler iff this succeeds:
     s3c_intc[0] = s3c24xx_init_intc()
If this fails, the next calls to s3c24xx_init_intc() won't be executed.
For DT machine, s3c_init_intc_of() could set the IRQ handler without setting s3c_intc[0] only if it was called with num_ctrl=0. There is no such code path, so again the s3c_intc[0] will have a valid pointer if set_handle_irq() is called.
Quoted 4 lines
Therefore in s3c24xx_handle_irq(), the s3c_intc[0] is always something.
The code can be simplified by removing if(), if we really wanted and were sure about it.
In fact, I didn't study his underlying logic in depth, but found that this place was not particularly perfect based on the return value of the function, because I happened to encounter a similar problem elsewhere.
Quoted 17 lines
arch/arm/mach-s3c/irq-s3c24xx.c static inline int s3c24xx_handle_intc(struct s3c_irq_intc *intc, · line 369
Quoted 6 lines
arch/arm/mach-s3c/irq-s3c24xx.c +1 -1
  				continue;
 
-		if (s3c_intc[2])
+		if (!IS_ERR_OR_NULL(s3c_intc[2]))
For the non-DT case, this seems ugly but proper solution. The s3c_intc[2] could be NULL (not set at all) or set as ERR (if s3c24xx_init_intc() fails).
Quoted 4 lines
arch/arm/mach-s3c/irq-s3c24xx.c +0 -0
  			if (s3c24xx_handle_intc(s3c_intc[2], regs, 64))
  				continue;
 

Quoted 5 lines

Quoted 3 lines
Best regards,
Krzysztof
Would you mind review v2?

Sure, please send it.

Best regards,
Krzysztof