i2c: designware-pci: Fix BUG_ON during device removal
diff mbox series

Message ID 20200213151503.545269-1-jarkko.nikula@linux.intel.com
State New
Headers show
Series
  • i2c: designware-pci: Fix BUG_ON during device removal
Related show

Commit Message

Jarkko Nikula Feb. 13, 2020, 3:15 p.m. UTC
Function i2c_dw_pci_remove() -> pci_free_irq_vectors() ->
pci_disable_msi() -> free_msi_irqs() will throw a BUG_ON() for MSI
enabled device since the driver has not released the requested IRQ before
calling the pci_free_irq_vectors().

Here driver requests an IRQ using devm_request_irq() but automatic
release happens only after remove callback. Fix this by explicitly
freeing the IRQ before calling pci_free_irq_vectors().

Fixes: 21aa3983d619 ("i2c: designware-pci: Switch over to MSI interrupts")
Cc: stable@vger.kernel.org # v5.4+
Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
---
 drivers/i2c/busses/i2c-designware-pcidrv.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Wolfram Sang Feb. 13, 2020, 4:18 p.m. UTC | #1
On Thu, Feb 13, 2020 at 05:15:03PM +0200, Jarkko Nikula wrote:
> Function i2c_dw_pci_remove() -> pci_free_irq_vectors() ->
> pci_disable_msi() -> free_msi_irqs() will throw a BUG_ON() for MSI
> enabled device since the driver has not released the requested IRQ before
> calling the pci_free_irq_vectors().
> 
> Here driver requests an IRQ using devm_request_irq() but automatic
> release happens only after remove callback. Fix this by explicitly
> freeing the IRQ before calling pci_free_irq_vectors().

Does it make sense to keep devm for irq handling, then?
Andy Shevchenko Feb. 13, 2020, 5:10 p.m. UTC | #2
On Thu, Feb 13, 2020 at 05:15:03PM +0200, Jarkko Nikula wrote:
> Function i2c_dw_pci_remove() -> pci_free_irq_vectors() ->
> pci_disable_msi() -> free_msi_irqs() will throw a BUG_ON() for MSI
> enabled device since the driver has not released the requested IRQ before
> calling the pci_free_irq_vectors().
> 
> Here driver requests an IRQ using devm_request_irq() but automatic
> release happens only after remove callback. Fix this by explicitly
> freeing the IRQ before calling pci_free_irq_vectors().
> 

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Fixes: 21aa3983d619 ("i2c: designware-pci: Switch over to MSI interrupts")
> Cc: stable@vger.kernel.org # v5.4+
> Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> ---
>  drivers/i2c/busses/i2c-designware-pcidrv.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c b/drivers/i2c/busses/i2c-designware-pcidrv.c
> index 050adda7c1bd..05b35ac33ce3 100644
> --- a/drivers/i2c/busses/i2c-designware-pcidrv.c
> +++ b/drivers/i2c/busses/i2c-designware-pcidrv.c
> @@ -313,6 +313,7 @@ static void i2c_dw_pci_remove(struct pci_dev *pdev)
>  	pm_runtime_get_noresume(&pdev->dev);
>  
>  	i2c_del_adapter(&dev->adapter);
> +	devm_free_irq(&pdev->dev, dev->irq, dev);
>  	pci_free_irq_vectors(pdev);
>  }
>  
> -- 
> 2.25.0
>
Andy Shevchenko Feb. 13, 2020, 5:11 p.m. UTC | #3
On Thu, Feb 13, 2020 at 05:18:54PM +0100, Wolfram Sang wrote:
> On Thu, Feb 13, 2020 at 05:15:03PM +0200, Jarkko Nikula wrote:
> > Function i2c_dw_pci_remove() -> pci_free_irq_vectors() ->
> > pci_disable_msi() -> free_msi_irqs() will throw a BUG_ON() for MSI
> > enabled device since the driver has not released the requested IRQ before
> > calling the pci_free_irq_vectors().
> > 
> > Here driver requests an IRQ using devm_request_irq() but automatic
> > release happens only after remove callback. Fix this by explicitly
> > freeing the IRQ before calling pci_free_irq_vectors().
> 
> Does it make sense to keep devm for irq handling, then?

Only for sake of better error handling in error path in the ->probe().
Jarkko Nikula Feb. 14, 2020, 9:26 a.m. UTC | #4
On 2/13/20 7:11 PM, Andy Shevchenko wrote:
> On Thu, Feb 13, 2020 at 05:18:54PM +0100, Wolfram Sang wrote:
>> On Thu, Feb 13, 2020 at 05:15:03PM +0200, Jarkko Nikula wrote:
>>> Function i2c_dw_pci_remove() -> pci_free_irq_vectors() ->
>>> pci_disable_msi() -> free_msi_irqs() will throw a BUG_ON() for MSI
>>> enabled device since the driver has not released the requested IRQ before
>>> calling the pci_free_irq_vectors().
>>>
>>> Here driver requests an IRQ using devm_request_irq() but automatic
>>> release happens only after remove callback. Fix this by explicitly
>>> freeing the IRQ before calling pci_free_irq_vectors().
>>
>> Does it make sense to keep devm for irq handling, then?
> 
I thought about it and decided devm still saves one line of code in 
drivers/i2c/busses/i2c-designware-platdrv.c: dw_i2c_plat_remove().

> Only for sake of better error handling in error path in the ->probe().
> 
Yeah, that too.

Patch
diff mbox series

diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c b/drivers/i2c/busses/i2c-designware-pcidrv.c
index 050adda7c1bd..05b35ac33ce3 100644
--- a/drivers/i2c/busses/i2c-designware-pcidrv.c
+++ b/drivers/i2c/busses/i2c-designware-pcidrv.c
@@ -313,6 +313,7 @@  static void i2c_dw_pci_remove(struct pci_dev *pdev)
 	pm_runtime_get_noresume(&pdev->dev);
 
 	i2c_del_adapter(&dev->adapter);
+	devm_free_irq(&pdev->dev, dev->irq, dev);
 	pci_free_irq_vectors(pdev);
 }