chained_irq_enter(() should be paired with a corresponding
chained_irq_exit().

Here, if clk_enable() fails, a early return occurs and chained_irq_exit()
is not called.

Add a new label and a goto for fix it.

Fixes: f9c744747973 ("pinctrl: samsung: support a bus clock")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
Compile tested only.

Review with care, irq handling is sometimes tricky...
---
 drivers/pinctrl/samsung/pinctrl-exynos.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/pinctrl/samsung/pinctrl-exynos.c b/drivers/pinctrl/samsung/pinctrl-exynos.c
index b79c211c0374..ac6dc22b37c9 100644
--- a/drivers/pinctrl/samsung/pinctrl-exynos.c
+++ b/drivers/pinctrl/samsung/pinctrl-exynos.c
@@ -636,7 +636,7 @@ static void exynos_irq_demux_eint16_31(struct irq_desc *desc)
 		if (clk_enable(b->drvdata->pclk)) {
 			dev_err(b->gpio_chip.parent,
 				"unable to enable clock for pending IRQs\n");
-			return;
+			goto out;
 		}
 	}
 
@@ -652,6 +652,7 @@ static void exynos_irq_demux_eint16_31(struct irq_desc *desc)
 	if (eintd->nr_banks)
 		clk_disable(eintd->banks[0]->drvdata->pclk);
 
+out:
 	chained_irq_exit(chip, desc);
 }
 
-- 
2.47.0

On Sun, 17 Nov 2024 13:03:32 +0100, Christophe JAILLET wrote:
> chained_irq_enter(() should be paired with a corresponding
> chained_irq_exit().
> 
> Here, if clk_enable() fails, a early return occurs and chained_irq_exit()
> is not called.
> 
> Add a new label and a goto for fix it.
> 
> [...]

Applied, thanks!

[1/1] pinctrl: samsung: Fix irq handling if an error occurs in exynos_irq_demux_eint16_31()
      https://git.kernel.org/pinctrl/samsung/c/f686a2b52e9d78cf401f1b7f446bf0c3a81ebcc0

Best regards,
-- 
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

On Sun, 2024-11-17 at 13:03 +0100, Christophe JAILLET wrote:
> chained_irq_enter(() should be paired with a corresponding
> chained_irq_exit().
> 
> Here, if clk_enable() fails, a early return occurs and chained_irq_exit()
> is not called.
> 
> Add a new label and a goto for fix it.
> 
> Fixes: f9c744747973 ("pinctrl: samsung: support a bus clock")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> Compile tested only.
> 
> Review with care, irq handling is sometimes tricky...

Well spotted, thanks.

It looks like there is a similar problem in exynos_irq_request_resources()
in same file. It should likely call gpiochip_unlock_as_irq() if clk_enable()
failed.

Care to fix that as well?

That said,

Reviewed-by: André Draszik <andre.draszik@linaro.org>


Cheers,
Andre'

Le 18/11/2024 à 10:40, André Draszik a écrit :
> On Sun, 2024-11-17 at 13:03 +0100, Christophe JAILLET wrote:
>> chained_irq_enter(() should be paired with a corresponding
>> chained_irq_exit().
>>
>> Here, if clk_enable() fails, a early return occurs and chained_irq_exit()
>> is not called.
>>
>> Add a new label and a goto for fix it.
>>
>> Fixes: f9c744747973 ("pinctrl: samsung: support a bus clock")
>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>> ---
>> Compile tested only.
>>
>> Review with care, irq handling is sometimes tricky...
> 
> Well spotted, thanks.
> 
> It looks like there is a similar problem in exynos_irq_request_resources()
> in same file. It should likely call gpiochip_unlock_as_irq() if clk_enable()
> failed.

Also wondering if it is needed in exynos_irq_release_resources() if 
clk_enable() fails and we early return.

I don't know how these callbacks are used and if we could dead-lock in 
such a situation.

What do you think?

CJ

> 
> Care to fix that as well?
> 
> That said,
> 
> Reviewed-by: André Draszik <andre.draszik@linaro.org>
> 
> 
> Cheers,
> Andre'
> 
> 
> 

Hi Christophe,

On Mon, 2024-11-18 at 21:40 +0100, Christophe JAILLET wrote:
> Also wondering if it is needed in exynos_irq_release_resources() if 
> clk_enable() fails and we early return.
> 
> I don't know how these callbacks are used and if we could dead-lock in 
> such a situation.
> 
> What do you think?

This was pointed out indeed in
https://lore.kernel.org/all/9a960401-f41f-4902-bcbd-8f30f318ba98@kernel.org/
but irq_chip::irq_release_resources() is not expected to fail.

_mask(), _unmask(), and _ack() have a similar issue. In practice, I don't
think the enable has ever failed in our usecase - it's just a simple bit
flip after all.

There are two options, update the callback signatures (and all users...), or
keep the clock on for the whole duration. Given the clock really is needed
for register access only, we didn't do the latter originally:
https://lore.kernel.org/all/0106b6f58ce19752c2c685d128e5a480103ee91c.camel@linaro.org/

Not sure what the preference would be, 2nd option is likely easier to do and
it would be surprising if _mask() etc. suddenly could fail.


Cheers,
Andre'

Le 18/11/2024 à 10:40, André Draszik a écrit :
> On Sun, 2024-11-17 at 13:03 +0100, Christophe JAILLET wrote:
>> chained_irq_enter(() should be paired with a corresponding
>> chained_irq_exit().
>>
>> Here, if clk_enable() fails, a early return occurs and chained_irq_exit()
>> is not called.
>>
>> Add a new label and a goto for fix it.
>>
>> Fixes: f9c744747973 ("pinctrl: samsung: support a bus clock")
>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>> ---
>> Compile tested only.
>>
>> Review with care, irq handling is sometimes tricky...
> 
> Well spotted, thanks.
> 
> It looks like there is a similar problem in exynos_irq_request_resources()
> in same file. It should likely call gpiochip_unlock_as_irq() if clk_enable()
> failed.

Agreed.

> 
> Care to fix that as well?

NP, I'll send a patch.

CJ

> 
> That said,
> 
> Reviewed-by: André Draszik <andre.draszik@linaro.org>
> 
> 
> Cheers,
> Andre'
> 
> 
>