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

[Rodolfo Giometti <giometti@enneenne.com>]

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