Patchwork [01/13] i2c: bcm2835: Use devm_request_irq()

login
register
mail settings
Submitter Jingoo Han
Date Dec. 17, 2013, 6:46 a.m.
Message ID <001d01cefaf3$b743e4c0$25cbae40$%han@samsung.com>
Download mbox | patch
Permalink /patch/302001/
State Deferred
Headers show

Comments

Jingoo Han - Dec. 17, 2013, 6:46 a.m.
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(-)
Stephen Warren - Dec. 18, 2013, 2:21 a.m.
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
Jingoo Han - Dec. 18, 2013, 4:15 a.m.
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
Wolfram Sang - Jan. 4, 2014, 9:06 p.m.
> 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.

Patch

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;