| Message ID | 001d01cefaf3$b743e4c0$25cbae40$%han@samsung.com |
|---|---|
| State | Deferred |
| Headers | show |
On 12/16/2013 11:46 PM, Jingoo Han wrote:
> Use devm_request_irq() to make cleanup paths simpler.
This may not be safe. The interrupt used by the I2C controllers on the
BCM2835 chip is shared between two controllers. In theory, you could run
into a condition where you're remove()ing the driver for one of the
controller (a), while the driver for controller (b) is still active, yet
the HW for controller (a) comes along and triggers an interrupt after
remove() but before the devm IRQ cleanup. That would result in the IRQ
handler for controller (a) executing after the remove() of the
associated device, which could cause a variety of problems.
You might be able to make this safe by explicitly clearing any IRQ
enable registers in remove(), but leaving the code using plain
request_irq() might be simpler.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday, December 18, 2013 11:21 AM, Stephen Warren wrote: > On 12/16/2013 11:46 PM, Jingoo Han wrote: > > Use devm_request_irq() to make cleanup paths simpler. > > This may not be safe. The interrupt used by the I2C controllers on the > BCM2835 chip is shared between two controllers. In theory, you could run > into a condition where you're remove()ing the driver for one of the > controller (a), while the driver for controller (b) is still active, yet > the HW for controller (a) comes along and triggers an interrupt after > remove() but before the devm IRQ cleanup. That would result in the IRQ > handler for controller (a) executing after the remove() of the > associated device, which could cause a variety of problems. Hi Stephen, I appreciate you detailed and kind description. :-) If there is possible side effect, the patch should not be merged. Wolfram Sang, Please ignore this patch. Thank you. Best regards, Jingoo Han > You might be able to make this safe by explicitly clearing any IRQ > enable registers in remove(), but leaving the code using plain > request_irq() might be simpler. -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> If there is possible side effect, the patch should not be > merged. > > Wolfram Sang, > Please ignore this patch. Thank you. Even more so, I will ignore this series unless there is someone having actually tested the patch on real HW. Thanks for your effort, yet I need to be sure it really works on the actual HW.
diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c index 77df97b..af60181 100644 --- a/drivers/i2c/busses/i2c-bcm2835.c +++ b/drivers/i2c/busses/i2c-bcm2835.c @@ -285,8 +285,8 @@ static int bcm2835_i2c_probe(struct platform_device *pdev) } i2c_dev->irq = irq->start; - ret = request_irq(i2c_dev->irq, bcm2835_i2c_isr, IRQF_SHARED, - dev_name(&pdev->dev), i2c_dev); + ret = devm_request_irq(&pdev->dev, i2c_dev->irq, bcm2835_i2c_isr, + IRQF_SHARED, dev_name(&pdev->dev), i2c_dev); if (ret) { dev_err(&pdev->dev, "Could not request IRQ\n"); return -ENODEV; @@ -304,8 +304,6 @@ static int bcm2835_i2c_probe(struct platform_device *pdev) bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_C, 0); ret = i2c_add_adapter(adap); - if (ret) - free_irq(i2c_dev->irq, i2c_dev); return ret; } @@ -314,7 +312,6 @@ static int bcm2835_i2c_remove(struct platform_device *pdev) { struct bcm2835_i2c_dev *i2c_dev = platform_get_drvdata(pdev); - free_irq(i2c_dev->irq, i2c_dev); i2c_del_adapter(&i2c_dev->adapter); return 0;
Use devm_request_irq() to make cleanup paths simpler. Signed-off-by: Jingoo Han <jg1.han@samsung.com> --- drivers/i2c/busses/i2c-bcm2835.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)