On 01/09/2021 14:28, Jackie Liu wrote:
>
> Hi Krzysztof, Thanks for you message.
>
> 在 2021/9/1 下午8:09, Krzysztof Kozlowski 写道:
>> 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.
>
> 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.
>
>>
>>
>>> 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
>>
>
> Would you mind review v2?
Sure, please send it.
Best regards,
Krzysztof
