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.

s3c_intc[0] is not NULL or ERR, we can simplify the code.

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 | 21 +++++++++++++++++----
  1 file changed, 17 insertions(+), 4 deletions(-)

arch/arm/mach-s3c/irq-s3c24xx.c +17 -4

index 0c631c14a817..df471d322493 100644
@@ -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;
 

Hi, krzysztof. Any suggestion?

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.

s3c_intc[0] is not NULL or ERR, we can simplify the code.

[...]

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

+               /* 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 11 lines · Jackie Liu

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

+               /* 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.

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

Quoted 10 lines

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

+                *
+                * 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;

I just saw this in the pull request. I'm taking the pull request since it's a bugfix and
the resulting code is technically correct, but I'd point out that this is particularly
ugly. Any use of IS_ERR_OR_NULL() essentially means we have a misdefined interface, and it's clear that this is one of them.

Nothing actually uses the return code of s3c24xx_init_intc(), so returning NULL on error there and changing all the checks to that would be a much more straightforward solution.

Any chance you could send a follow-up to do that?

Quoted 1 line · Arnd Bergmann

On Wed, Sep 1, 2021 at 2:35 PM Jackie Liu <[email protected]> wrote:

Quoted 5 lines

Nothing actually uses the return code of s3c24xx_init_intc(), so returning NULL on error there and changing all the checks to that would be a much more straightforward solution.

Any chance you could send a follow-up to do that?

I can work on this but you know this is a legacy platform and none of us can test it?