gpio: nomadik: Add check for clk_enable()

PATCH Applied Chenyuan Yang 4 participants 7 messages Apr 12, 2025 – Apr 16, 2025

Subsystems (2)

GPIO Arm/nomadik/Ux500 architectures

lore

Share & tools

lore

Chenyuan Yang

author

2025-04-12 19:31 UTC #0 lore raw

PATCH 1/1 gpio: nomadik: Add check for clk_enable()

Add check for the return value of clk_enable() to catch the potential error.

This is similar to the commit 8332e6670997
("spi: zynq-qspi: Add check for clk_enable()").

Signed-off-by Chenyuan Yang <[email protected]>

Fixes 966942ae4936 ("gpio: nomadik: extract GPIO platform driver from drivers/pinctrl/nomadik/")

---
 drivers/gpio/gpio-nomadik.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

drivers/gpio/gpio-nomadik.c +4 -1

index fa19a44943fd..dbc4cdddf4df 100644
@@ -262,8 +262,11 @@ static unsigned int nmk_gpio_irq_startup(struct irq_data *d)
 {
 	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
 	struct nmk_gpio_chip *nmk_chip = gpiochip_get_data(gc);
+	int ret;
 
-	clk_enable(nmk_chip->clk);
+	ret = clk_enable(nmk_chip->clk);
+	if (ret)
+		return ret;
 	nmk_gpio_irq_unmask(d);
 	return 0;
 }

Signature · 1 line

2.34.1

Linus Walleij

Reviewed-by on v1 1/1

2025-04-13 07:44 UTC #1 lore raw

Quoted 8 lines · Chenyuan Yang

Add check for the return value of clk_enable() to catch the potential error.
This is similar to the commit 8332e6670997
("spi: zynq-qspi: Add check for clk_enable()").
Signed-off-by Chenyuan Yang <[email protected]>
Fixes 966942ae4936 ("gpio: nomadik: extract GPIO platform driver from drivers/pinctrl/nomadik/")

Reviewed-by Linus Walleij <[email protected]>

Yours,
Linus Walleij

Bartosz Golaszewski

Applied

2025-04-13 20:00 UTC #2 lore raw

In reply to Chenyuan Yang

Quoted 8 lines · Chenyuan Yang

Add check for the return value of clk_enable() to catch the potential error.
This is similar to the commit 8332e6670997
("spi: zynq-qspi: Add check for clk_enable()").
[...]

Applied, thanks!

[1/1] gpio: nomadik: Add check for clk_enable()

      https://git.kernel.org/brgl/linux/c/4521e0884c261fd286c02da942e9e8596bf2e7cf

Best regards,

Signature · 1 line

Bartosz Golaszewski <[email protected]>

Théo Lebrun

2025-04-14 09:24 UTC #3 lore raw

In reply to Chenyuan Yang

Hello Chenyuan, Linus, Bartosz,

Quoted 19 lines · Chenyuan Yang

Add check for the return value of clk_enable() to catch the potential error.
This is similar to the commit 8332e6670997
("spi: zynq-qspi: Add check for clk_enable()").
Signed-off-by Chenyuan Yang <[email protected]>
Fixes 966942ae4936 ("gpio: nomadik: extract GPIO platform driver from drivers/pinctrl/nomadik/")
---
 drivers/gpio/gpio-nomadik.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
drivers/gpio/gpio-nomadik.c +0 -0
index fa19a44943fd..dbc4cdddf4df 100644
@@ -262,8 +262,11 @@ static unsigned int nmk_gpio_irq_startup(struct irq_data *d)
 {
 	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);

drivers/gpio/gpio-nomadik.c static unsigned int nmk_gpio_irq_startup(struct irq_data *d) · line 272

Quoted 10 lines

drivers/gpio/gpio-nomadik.c +4 -1

 	struct nmk_gpio_chip *nmk_chip = gpiochip_get_data(gc);
+	int ret;
 
-	clk_enable(nmk_chip->clk);
+	ret = clk_enable(nmk_chip->clk);
+	if (ret)
+		return ret;
 	nmk_gpio_irq_unmask(d);
 	return 0;
 }

Returning a negative value whereas the ->irq_startup() [0] return value is an unsigned int? From some quick godbolt testing and briefly reading the spec it looks safe to do a round trip (signed->unsigned->signed), though not ideal to my eyes.

The caller is __irq_startup() [1].

As for why irq_startup returns an unsigned int, I am unsure. The kernel Git history isn't enough to know more. The startup field in struct hw_interrupt_type appeared on v2.3.14 [2], so no commit message to explain decisions.

Seeing the __irq_startup() code, my proposal would be to turn the return value to a signed int, but I haven't exhaustively checked codepaths.

Thanks,

[0]: https://elixir.bootlin.com/linux/v6.13.7/source/include/linux/irq.h#L503
[1]: https://elixir.bootlin.com/linux/v6.13.7/source/kernel/irq/chip.c#L244
[2]: https://elixir.bootlin.com/linux/2.3.14/source/include/linux/irq.h#L21

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Chenyuan Yang

author

2025-04-14 12:11 UTC #4 lore raw

Quoted 41 lines · Théo Lebrun

Hello Chenyuan, Linus, Bartosz,
Quoted 19 lines · Chenyuan Yang
Add check for the return value of clk_enable() to catch the potential error.
This is similar to the commit 8332e6670997
("spi: zynq-qspi: Add check for clk_enable()").
Signed-off-by Chenyuan Yang <[email protected]>
Fixes 966942ae4936 ("gpio: nomadik: extract GPIO platform driver from drivers/pinctrl/nomadik/")
---
 drivers/gpio/gpio-nomadik.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
drivers/gpio/gpio-nomadik.c +0 -0
index fa19a44943fd..dbc4cdddf4df 100644
@@ -262,8 +262,11 @@ static unsigned int nmk_gpio_irq_startup(struct irq_data *d)
 {
      struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
drivers/gpio/gpio-nomadik.c static unsigned int nmk_gpio_irq_startup(struct irq_data *d) · line 272
Quoted 10 lines
drivers/gpio/gpio-nomadik.c +4 -1
      struct nmk_gpio_chip *nmk_chip = gpiochip_get_data(gc);
+     int ret;
 
-     clk_enable(nmk_chip->clk);
+     ret = clk_enable(nmk_chip->clk);
+     if (ret)
+             return ret;
      nmk_gpio_irq_unmask(d);
      return 0;
 }
Returning a negative value whereas the ->irq_startup() [0] return value is an unsigned int? From some quick godbolt testing and briefly reading the spec it looks safe to do a round trip (signed->unsigned->signed), though not ideal to my eyes.
The caller is __irq_startup() [1].

Quoted 7 lines

As for why irq_startup returns an unsigned int, I am unsure. The kernel Git history isn't enough to know more. The startup field in struct hw_interrupt_type appeared on v2.3.14 [2], so no commit message to explain decisions.
Seeing the __irq_startup() code, my proposal would be to turn the return value to a signed int, but I haven't exhaustively checked codepaths.

Good catch! I agree that using a signed int could be a better option.

Dear Linus and Bartosz, could you please share your thoughts? If you’re on board with the change, I’ll go ahead and send a new patch.

Quoted 10 lines

Thanks,
[0]: https://elixir.bootlin.com/linux/v6.13.7/source/include/linux/irq.h#L503
[1]: https://elixir.bootlin.com/linux/v6.13.7/source/kernel/irq/chip.c#L244
[2]: https://elixir.bootlin.com/linux/2.3.14/source/include/linux/irq.h#L21
--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

-Chenyuan

Bartosz Golaszewski

2025-04-14 18:30 UTC #5 lore raw

Quoted 8 lines · Chenyuan Yang

Quoted 6 lines · Théo Lebrun
As for why irq_startup returns an unsigned int, I am unsure. The kernel Git history isn't enough to know more. The startup field in struct hw_interrupt_type appeared on v2.3.14 [2], so no commit message to explain decisions.

Quoted 8 lines

Quoted 2 lines
Seeing the __irq_startup() code, my proposal would be to turn the return value to a signed int, but I haven't exhaustively checked codepaths.
Good catch! I agree that using a signed int could be a better option.
Dear Linus and Bartosz, could you please share your thoughts? If you’re on board with the change, I’ll go ahead and send a new patch.

Ok, I'm backing out this one then.

Bartosz

Linus Walleij

2025-04-16 08:35 UTC #6 lore raw

In reply to Chenyuan Yang

Quoted 9 lines · Chenyuan Yang

Quoted 2 lines · Théo Lebrun
Seeing the __irq_startup() code, my proposal would be to turn the return value to a signed int, but I haven't exhaustively checked codepaths.
Good catch! I agree that using a signed int could be a better option.
Dear Linus and Bartosz, could you please share your thoughts? If you’re on board with the change, I’ll go ahead and send a new patch.

Yeah that would be great, thanks!

Thanks for noticing this as well Theo, I think your team is the most important user of this code right now.

Yours,
Linus Walleij