[PATCH v2 0/2] i2c: bcm2835: Fixes for clock changes in probe function

[PATCH v2 0/2] i2c: bcm2835: Fixes for clock changes in probe function

[Annaliese McDermond]

An issue was reported in [1] and [2] that the latest version of the i2c
driver was not properly loading.  After analysis it was determined that
the new clock code was failiing because the i2c driver was trying to
load before the bcm2835-clk driver when not loaded as a module. This is
fixed by actually attempting to grab a reference to the clock and failing
out with a EPROBE_DEFER if it's not there.  This gives the other drivers
an opportunity to load.

This series also fixes a related bug where the clock setup code in the
probe function could cause an issue where the IRQ would be requested
by the driver and never freed in case of some clock setup failure.  The
patch moves this IRQ code to the end of the probe function where it will
not cause this issue.

[1] - https://www.raspberrypi.org/forums/viewtopic.php?f=44&t=242856
[2] - https://archlinuxarm.org/forum/viewtopic.php?f=23&t=13719

Annaliese McDermond (2):
  i2c: bcm2835: Move IRQ request after clock code in probe
  i2c: bcm2835: Ensure clock exists when probing

 drivers/i2c/busses/i2c-bcm2835.c | 42 +++++++++++++++++++-------------
 1 file changed, 25 insertions(+), 17 deletions(-)

-- 
2.19.1

[PATCH v2 1/2] i2c: bcm2835: Move IRQ request after clock code in probe

[Annaliese McDermond]

If any of the clock code in the probe fails and returns, the IRQ
will not be freed.  Moving the IRQ request to last allows it to
be freed on any errors further up in the probe function.  devm_
calls can apparently not be used because there are some potential
race conditions that will arise.

Fixes: bebff81fb8b9 ("i2c: bcm2835: Model Divider in CCF")

Signed-off-by: Annaliese McDermond <nh6z@nh6z.net>
---
 drivers/i2c/busses/i2c-bcm2835.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c
index 108d2ae4632c..27b2f204c693 100644
--- a/drivers/i2c/busses/i2c-bcm2835.c
+++ b/drivers/i2c/busses/i2c-bcm2835.c
@@ -521,20 +521,6 @@ static int bcm2835_i2c_probe(struct platform_device *pdev)
 	if (IS_ERR(i2c_dev->regs))
 		return PTR_ERR(i2c_dev->regs);
 
-	irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
-	if (!irq) {
-		dev_err(&pdev->dev, "No IRQ resource\n");
-		return -ENODEV;
-	}
-	i2c_dev->irq = irq->start;
-
-	ret = request_irq(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;
-	}
-
 	mclk_name = of_clk_get_parent_name(pdev->dev.of_node, 0);
 
 	bus_clk = bcm2835_i2c_register_div(&pdev->dev, mclk_name, i2c_dev);
@@ -564,6 +550,20 @@ static int bcm2835_i2c_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+	if (!irq) {
+		dev_err(&pdev->dev, "No IRQ resource\n");
+		return -ENODEV;
+	}
+	i2c_dev->irq = irq->start;
+
+	ret = request_irq(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;
+	}
+
 	adap = &i2c_dev->adapter;
 	i2c_set_adapdata(adap, i2c_dev);
 	adap->owner = THIS_MODULE;
-- 
2.19.1

Re: [PATCH v2 1/2] i2c: bcm2835: Move IRQ request after clock code in probe

[Wolfram Sang]

[-- Attachment #1.1: Type: text/plain, Size: 535 bytes --]

On Fri, Jun 21, 2019 at 03:52:49AM -0700, Annaliese McDermond wrote:
> If any of the clock code in the probe fails and returns, the IRQ
> will not be freed.  Moving the IRQ request to last allows it to
> be freed on any errors further up in the probe function.  devm_
> calls can apparently not be used because there are some potential
> race conditions that will arise.
> 
> Fixes: bebff81fb8b9 ("i2c: bcm2835: Model Divider in CCF")
> 
> Signed-off-by: Annaliese McDermond <nh6z@nh6z.net>

Applied to for-next, thanks!


[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 2/2] i2c: bcm2835: Ensure clock exists when probing

[Annaliese McDermond]

Probe function fails to recognize that upstream clock actually
doesn't yet exist because clock driver has not been initialized.
Actually try to go get the clock and test for its existence
before trying to set up a downstream clock based upon it.

This fixes a bug that causes the i2c driver not to work with
monolithic kernels.

Fixes: bebff81fb8b9 ("i2c: bcm2835: Model Divider in CCF")

Signed-off-by: Annaliese McDermond <nh6z@nh6z.net>
---
 drivers/i2c/busses/i2c-bcm2835.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c
index 27b2f204c693..79d06286b926 100644
--- a/drivers/i2c/busses/i2c-bcm2835.c
+++ b/drivers/i2c/busses/i2c-bcm2835.c
@@ -244,15 +244,18 @@ static const struct clk_ops clk_bcm2835_i2c_ops = {
 };
 
 static struct clk *bcm2835_i2c_register_div(struct device *dev,
-					const char *mclk_name,
+					struct clk *mclk,
 					struct bcm2835_i2c_dev *i2c_dev)
 {
 	struct clk_init_data init;
 	struct clk_bcm2835_i2c *priv;
 	char name[32];
+	const char *mclk_name;
 
 	snprintf(name, sizeof(name), "%s_div", dev_name(dev));
 
+	mclk_name = __clk_get_name(mclk);
+
 	init.ops = &clk_bcm2835_i2c_ops;
 	init.name = name;
 	init.parent_names = (const char* []) { mclk_name };
@@ -505,8 +508,8 @@ static int bcm2835_i2c_probe(struct platform_device *pdev)
 	struct resource *mem, *irq;
 	int ret;
 	struct i2c_adapter *adap;
-	const char *mclk_name;
 	struct clk *bus_clk;
+	struct clk *mclk;
 	u32 bus_clk_rate;
 
 	i2c_dev = devm_kzalloc(&pdev->dev, sizeof(*i2c_dev), GFP_KERNEL);
@@ -521,9 +524,14 @@ static int bcm2835_i2c_probe(struct platform_device *pdev)
 	if (IS_ERR(i2c_dev->regs))
 		return PTR_ERR(i2c_dev->regs);
 
-	mclk_name = of_clk_get_parent_name(pdev->dev.of_node, 0);
+	mclk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(mclk)) {
+		if (PTR_ERR(mclk) != -EPROBE_DEFER)
+			dev_err(&pdev->dev, "Could not get clock\n");
+		return PTR_ERR(mclk);
+	}
 
-	bus_clk = bcm2835_i2c_register_div(&pdev->dev, mclk_name, i2c_dev);
+	bus_clk = bcm2835_i2c_register_div(&pdev->dev, mclk, i2c_dev);
 
 	if (IS_ERR(bus_clk)) {
 		dev_err(&pdev->dev, "Could not register clock\n");
-- 
2.19.1

Re: [PATCH v2 2/2] i2c: bcm2835: Ensure clock exists when probing

[Wolfram Sang]

[-- Attachment #1.1: Type: text/plain, Size: 577 bytes --]

On Fri, Jun 21, 2019 at 03:52:50AM -0700, Annaliese McDermond wrote:
> Probe function fails to recognize that upstream clock actually
> doesn't yet exist because clock driver has not been initialized.
> Actually try to go get the clock and test for its existence
> before trying to set up a downstream clock based upon it.
> 
> This fixes a bug that causes the i2c driver not to work with
> monolithic kernels.
> 
> Fixes: bebff81fb8b9 ("i2c: bcm2835: Model Divider in CCF")
> 
> Signed-off-by: Annaliese McDermond <nh6z@nh6z.net>

Applied to for-next, thanks!


[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 0/2] i2c: bcm2835: Fixes for clock changes in probe function

[Stefan Wahren]

Am 21.06.19 um 12:52 schrieb Annaliese McDermond:
> An issue was reported in [1] and [2] that the latest version of the i2c
> driver was not properly loading.  After analysis it was determined that
> the new clock code was failiing because the i2c driver was trying to
> load before the bcm2835-clk driver when not loaded as a module. This is
> fixed by actually attempting to grab a reference to the clock and failing
> out with a EPROBE_DEFER if it's not there.  This gives the other drivers
> an opportunity to load.
>
> This series also fixes a related bug where the clock setup code in the
> probe function could cause an issue where the IRQ would be requested
> by the driver and never freed in case of some clock setup failure.  The
> patch moves this IRQ code to the end of the probe function where it will
> not cause this issue.
>
> [1] - https://www.raspberrypi.org/forums/viewtopic.php?f=44&t=242856
> [2] - https://archlinuxarm.org/forum/viewtopic.php?f=23&t=13719

This whole series is:

Acked-by: Stefan Wahren <wahrenst@gmx.net>