On 31/08/2021 11:57, Jackie Liu wrote:
> From: Jackie Liu <liuyun01@xxxxxxxxxx>
>
> 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 <liuyun01@xxxxxxxxxx>
Please use scripts/get_maintainers.pl to get list of mailing lists to
CC. You skipped two - arm and LKML.
> ---
> arch/arm/mach-s3c/irq-s3c24xx.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/mach-s3c/irq-s3c24xx.c b/arch/arm/mach-s3c/irq-s3c24xx.c
> index 0c631c14a817..d58bf0f9bf9a 100644
> --- a/arch/arm/mach-s3c/irq-s3c24xx.c
> +++ b/arch/arm/mach-s3c/irq-s3c24xx.c
> @@ -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.
> 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).
> if (s3c24xx_handle_intc(s3c_intc[2], regs, 64))
> continue;
>
>
Best regards,
Krzysztof
