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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20ba69f0-7ed9-48e7-8239-74a55c993c0b@enneenne.com>
Date: Tue, 27 May 2025 12:19:16 +0200
From: Rodolfo Giometti <giometti@...eenne.com>
To: "Farber, Eliav" <farbere@...zon.com>
Cc: "Chocron, Jonathan" <jonnyc@...zon.com>,
 "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
 "mschmidt@...hat.com" <mschmidt@...hat.com>,
 "calvin@...nvd.org" <calvin@...nvd.org>,
 "tglx@...utronix.de" <tglx@...utronix.de>,
 "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] pps: clients: gpio: fix interrupt handling order in
 remove path

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@...eenne.com
Linux Device Driver                          giometti@...ux.it
Embedded Systems                     phone:  +39 349 2432127
UNIX programming