RE: [PATCH] pps: clients: gpio: fix interrupt handling order in remove path

RE: [PATCH] pps: clients: gpio: fix interrupt handling order in remove path

[Farber, Eliav]

>> @@ -228,6 +228,7 @@ static void pps_gpio_remove(struct platform_device *pdev)
>>   {
>>       struct pps_gpio_device_data *data = platform_get_drvdata(pdev);
>>
>> +     free_irq(data->irq, data);
>
> Why not just use devm_free_irq()?

As far as I understand, the main purpose of devm_*() is to provide
hands-off resource management. devm_request_irq() is intended to
eliminate the need for explicit cleanup in the remove() function by
automatically freeing the IRQ after remove() returns.

In my opinion, calling devm_free_irq() undermines the benefit of using
devm_request_irq() in the first place. If I need to explicitly free the
IRQ during remove(), then I’m no longer relying on devm’s automatic
cleanup - I’m effectively reverting to manual resource management while
still using devm-style registration, which I find unnecessary.

That said, if you still favor devm_free_irq(), I’ll revise the patch
accordingly.

Regards, Eliav

Re: [PATCH] pps: clients: gpio: fix interrupt handling order in remove path

[Rodolfo Giometti]

On 27/05/25 11:11, Farber, Eliav wrote:
>>> @@ -228,6 +228,7 @@ static void pps_gpio_remove(struct platform_device *pdev)
>>>    {
>>>        struct pps_gpio_device_data *data = platform_get_drvdata(pdev);
>>>
>>> +     free_irq(data->irq, data);
>>
>> Why not just use devm_free_irq()?
> 
> As far as I understand, the main purpose of devm_*() is to provide
> hands-off resource management. devm_request_irq() is intended to
> eliminate the need for explicit cleanup in the remove() function by
> automatically freeing the IRQ after remove() returns.

In linux/kernel/irq/devres.c we can read:

/**
  *      devm_free_irq - free an interrupt
  *      @dev: device to free interrupt for
  *      @irq: Interrupt line to free
  *      @dev_id: Device identity to free
  *
  *      Except for the extra @dev argument, this function takes the
  *      same arguments and performs the same function as free_irq().
  *      This function instead of free_irq() should be used to manually
  *      free IRQs allocated with devm_request_irq().
  */

> In my opinion, calling devm_free_irq() undermines the benefit of using
> devm_request_irq() in the first place. If I need to explicitly free the
> IRQ during remove(), then I’m no longer relying on devm’s automatic
> cleanup - I’m effectively reverting to manual resource management while
> still using devm-style registration, which I find unnecessary.
> 
> That said, if you still favor devm_free_irq(), I’ll revise the patch
> accordingly.

Since devm_free_irq() works exactly as free_irq() and can be used to manually 
free IRQs allocated with devm_request_irq(), I think it is less invasive. Isn't 
it? :-)

Ciao,

Rodolfo

-- 
GNU/Linux Solutions                  e-mail: giometti@enneenne.com
Linux Device Driver                          giometti@linux.it
Embedded Systems                     phone:  +39 349 2432127
UNIX programming

Re: [PATCH] pps: clients: gpio: fix interrupt handling order in remove path

[gregkh]

On Tue, May 27, 2025 at 12:19:16PM +0200, Rodolfo Giometti wrote:
> On 27/05/25 11:11, Farber, Eliav wrote:
> > > > @@ -228,6 +228,7 @@ static void pps_gpio_remove(struct platform_device *pdev)
> > > >    {
> > > >        struct pps_gpio_device_data *data = platform_get_drvdata(pdev);
> > > > 
> > > > +     free_irq(data->irq, data);
> > > 
> > > Why not just use devm_free_irq()?
> > 
> > As far as I understand, the main purpose of devm_*() is to provide
> > hands-off resource management. devm_request_irq() is intended to
> > eliminate the need for explicit cleanup in the remove() function by
> > automatically freeing the IRQ after remove() returns.
> 
> In linux/kernel/irq/devres.c we can read:
> 
> /**
>  *      devm_free_irq - free an interrupt
>  *      @dev: device to free interrupt for
>  *      @irq: Interrupt line to free
>  *      @dev_id: Device identity to free
>  *
>  *      Except for the extra @dev argument, this function takes the
>  *      same arguments and performs the same function as free_irq().
>  *      This function instead of free_irq() should be used to manually
>  *      free IRQs allocated with devm_request_irq().
>  */
> 
> > In my opinion, calling devm_free_irq() undermines the benefit of using
> > devm_request_irq() in the first place. If I need to explicitly free the
> > IRQ during remove(), then I’m no longer relying on devm’s automatic
> > cleanup - I’m effectively reverting to manual resource management while
> > still using devm-style registration, which I find unnecessary.
> > 
> > That said, if you still favor devm_free_irq(), I’ll revise the patch
> > accordingly.
> 
> Since devm_free_irq() works exactly as free_irq() and can be used to
> manually free IRQs allocated with devm_request_irq(), I think it is less
> invasive. Isn't it? :-)

Not always, no.  irq handling is tricky, and devm for irqs is a huge
source of bugs as how do you really know when your irq is going to be
removed from the system?   If you don't want to use devm calls, don't,
there's no requirement to do so.

thanks,

greg k-h

[PATCH] pps: clients: gpio: fix interrupt handling order in remove path

[Eliav Farber]

The interrupt handler in pps_gpio_probe() is registered after calling
pps_register_source() using devm_request_irq(). However, in the
corresponding remove function, pps_unregister_source() is called before
the IRQ is freed, since devm-managed resources are released after the
remove function completes.

This creates a potential race condition where an interrupt may occur
after the PPS source is unregistered but before the handler is removed,
possibly leading to a kernel panic.

To prevent this, switch from devm-managed IRQ registration to manual
management by using request_irq() and calling free_irq() explicitly in
the remove path before unregistering the PPS source. This ensures the
interrupt handler is safely removed before deactivating the PPS source.

Signed-off-by: Eliav Farber <farbere@amazon.com>
---
 drivers/pps/clients/pps-gpio.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/pps/clients/pps-gpio.c b/drivers/pps/clients/pps-gpio.c
index 374ceefd6f2a..2866636b0554 100644
--- a/drivers/pps/clients/pps-gpio.c
+++ b/drivers/pps/clients/pps-gpio.c
@@ -210,8 +210,8 @@ static int pps_gpio_probe(struct platform_device *pdev)
 	}
 
 	/* register IRQ interrupt handler */
-	ret = devm_request_irq(dev, data->irq, pps_gpio_irq_handler,
-			get_irqf_trigger_flags(data), data->info.name, data);
+	ret = request_irq(data->irq, pps_gpio_irq_handler,
+			  get_irqf_trigger_flags(data), data->info.name, data);
 	if (ret) {
 		pps_unregister_source(data->pps);
 		dev_err(dev, "failed to acquire IRQ %d\n", data->irq);
@@ -228,6 +228,7 @@ static void pps_gpio_remove(struct platform_device *pdev)
 {
 	struct pps_gpio_device_data *data = platform_get_drvdata(pdev);
 
+	free_irq(data->irq, data);
 	pps_unregister_source(data->pps);
 	timer_delete_sync(&data->echo_timer);
 	/* reset echo pin in any case */
-- 
2.47.1

Re: [PATCH] pps: clients: gpio: fix interrupt handling order in remove path

[Rodolfo Giometti]

On 27/05/25 07:33, Eliav Farber wrote:
> The interrupt handler in pps_gpio_probe() is registered after calling
> pps_register_source() using devm_request_irq(). However, in the
> corresponding remove function, pps_unregister_source() is called before
> the IRQ is freed, since devm-managed resources are released after the
> remove function completes.
> 
> This creates a potential race condition where an interrupt may occur
> after the PPS source is unregistered but before the handler is removed,
> possibly leading to a kernel panic.
> 
> To prevent this, switch from devm-managed IRQ registration to manual
> management by using request_irq() and calling free_irq() explicitly in
> the remove path before unregistering the PPS source. This ensures the
> interrupt handler is safely removed before deactivating the PPS source.
> 
> Signed-off-by: Eliav Farber <farbere@amazon.com>
> ---
>   drivers/pps/clients/pps-gpio.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pps/clients/pps-gpio.c b/drivers/pps/clients/pps-gpio.c
> index 374ceefd6f2a..2866636b0554 100644
> --- a/drivers/pps/clients/pps-gpio.c
> +++ b/drivers/pps/clients/pps-gpio.c
> @@ -210,8 +210,8 @@ static int pps_gpio_probe(struct platform_device *pdev)
>   	}
>   
>   	/* register IRQ interrupt handler */
> -	ret = devm_request_irq(dev, data->irq, pps_gpio_irq_handler,
> -			get_irqf_trigger_flags(data), data->info.name, data);
> +	ret = request_irq(data->irq, pps_gpio_irq_handler,
> +			  get_irqf_trigger_flags(data), data->info.name, data);
>   	if (ret) {
>   		pps_unregister_source(data->pps);
>   		dev_err(dev, "failed to acquire IRQ %d\n", data->irq);
> @@ -228,6 +228,7 @@ static void pps_gpio_remove(struct platform_device *pdev)
>   {
>   	struct pps_gpio_device_data *data = platform_get_drvdata(pdev);
>   
> +	free_irq(data->irq, data);

Why not just use devm_free_irq()?

>   	pps_unregister_source(data->pps);
>   	timer_delete_sync(&data->echo_timer);
>   	/* reset echo pin in any case */

Ciao,

Rodolfo

-- 
GNU/Linux Solutions                  e-mail: giometti@enneenne.com
Linux Device Driver                          giometti@linux.it
Embedded Systems                     phone:  +39 349 2432127
UNIX programming