| Message ID | 20250412193153.49138-1-chenyuan0y@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: nomadik: Add check for clk_enable() | expand |
On Sat, Apr 12, 2025 at 9:31 PM Chenyuan Yang <chenyuan0y@gmail.com> wrote: > 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 <chenyuan0y@gmail.com> > Fixes: 966942ae4936 ("gpio: nomadik: extract GPIO platform driver from drivers/pinctrl/nomadik/") Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On Sat, 12 Apr 2025 14:31:53 -0500, Chenyuan Yang wrote: > 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,
Hello Chenyuan, Linus, Bartosz, On Sat Apr 12, 2025 at 9:31 PM CEST, Chenyuan Yang wrote: > 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 <chenyuan0y@gmail.com> > 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(-) > > diff --git a/drivers/gpio/gpio-nomadik.c b/drivers/gpio/gpio-nomadik.c > index fa19a44943fd..dbc4cdddf4df 100644 > --- a/drivers/gpio/gpio-nomadik.c > +++ b/drivers/gpio/gpio-nomadik.c > @@ -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; > } 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
On Mon, Apr 14, 2025 at 4:24 AM Théo Lebrun <theo.lebrun@bootlin.com> wrote: > > Hello Chenyuan, Linus, Bartosz, > > On Sat Apr 12, 2025 at 9:31 PM CEST, Chenyuan Yang wrote: > > 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 <chenyuan0y@gmail.com> > > 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(-) > > > > diff --git a/drivers/gpio/gpio-nomadik.c b/drivers/gpio/gpio-nomadik.c > > index fa19a44943fd..dbc4cdddf4df 100644 > > --- a/drivers/gpio/gpio-nomadik.c > > +++ b/drivers/gpio/gpio-nomadik.c > > @@ -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; > > } > > 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. 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. > 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
On Mon, Apr 14, 2025 at 2:11 PM Chenyuan Yang <chenyuan0y@gmail.com> wrote: > > On Mon, Apr 14, 2025 at 4:24 AM Théo Lebrun <theo.lebrun@bootlin.com> wrote: > > > > 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. > Ok, I'm backing out this one then. Bartosz
diff --git a/drivers/gpio/gpio-nomadik.c b/drivers/gpio/gpio-nomadik.c index fa19a44943fd..dbc4cdddf4df 100644 --- a/drivers/gpio/gpio-nomadik.c +++ b/drivers/gpio/gpio-nomadik.c @@ -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; }
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 <chenyuan0y@gmail.com> 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(-)