| Message ID | 20200213151503.545269-1-jarkko.nikula@linux.intel.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | i2c: designware-pci: Fix BUG_ON during device removal | expand |
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?
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 >
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().
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.
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(). > > 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> Applied to for-current, thanks!
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); }
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(+)