| Message ID | 20240305-i2c_exynos5-v3-1-17a749688806@axis.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] i2c: exynos5: Init data before registering interrupt handler | expand |
Hi On Tue, 05 Mar 2024 11:50:00 +0100, Jesper Nilsson wrote: > devm_request_irq() is called before we initialize the "variant" > member variable from of_device_get_match_data(), so if an interrupt > is triggered inbetween, we can end up following a NULL pointer > in the interrupt handler. > > This problem was exposed when the I2C controller in question was > (mis)configured to be used in both secure world and Linux. > > [...] Applied to i2c/i2c-host-fixes on git://git.kernel.org/pub/scm/linux/kernel/git/andi.shyti/linux.git Thank you, Andi Patches applied =============== [1/1] i2c: exynos5: Init data before registering interrupt handler commit: 51130d52b84c473f5da5378aa7e7633611f79313
On 06/03/2024 12:55, Andi Shyti wrote: > Hi > > On Tue, 05 Mar 2024 11:50:00 +0100, Jesper Nilsson wrote: >> devm_request_irq() is called before we initialize the "variant" >> member variable from of_device_get_match_data(), so if an interrupt >> is triggered inbetween, we can end up following a NULL pointer >> in the interrupt handler. >> >> This problem was exposed when the I2C controller in question was >> (mis)configured to be used in both secure world and Linux. >> >> [...] > > Applied to i2c/i2c-host-fixes on > > git://git.kernel.org/pub/scm/linux/kernel/git/andi.shyti/linux.git If for any reason this is going to fixes, which marks its importance as fix, then it should go to stable as well. Please always cc-stable for such cases. Best regards, Krzysztof
Hi Krzysztof, On Wed, Mar 06, 2024 at 04:14:30PM +0100, Krzysztof Kozlowski wrote: > On 06/03/2024 12:55, Andi Shyti wrote: > > Hi > > > > On Tue, 05 Mar 2024 11:50:00 +0100, Jesper Nilsson wrote: > >> devm_request_irq() is called before we initialize the "variant" > >> member variable from of_device_get_match_data(), so if an interrupt > >> is triggered inbetween, we can end up following a NULL pointer > >> in the interrupt handler. > >> > >> This problem was exposed when the I2C controller in question was > >> (mis)configured to be used in both secure world and Linux. > >> > >> [...] > > > > Applied to i2c/i2c-host-fixes on > > > > git://git.kernel.org/pub/scm/linux/kernel/git/andi.shyti/linux.git > > If for any reason this is going to fixes, which marks its importance as > fix, then it should go to stable as well. Please always cc-stable for > such cases. yes, thank you! I should have checked that, as well. Andi
On 05.03.2024 11:50, Jesper Nilsson wrote: > devm_request_irq() is called before we initialize the "variant" > member variable from of_device_get_match_data(), so if an interrupt > is triggered inbetween, we can end up following a NULL pointer > in the interrupt handler. > > This problem was exposed when the I2C controller in question was > (mis)configured to be used in both secure world and Linux. > > That this can happen is also reflected by the existing code that > clears any pending interrupts from "u-boot or misc causes". > > Move the clearing of pending interrupts and the call to > devm_request_irq() to the end of probe. > > Reviewed-by: Andi Shyti <andi.shyti@kernel.org> > Fixes: 218e1496135e ("i2c: exynos5: add support for HSI2C on Exynos5260 SoC") > Signed-off-by: Jesper Nilsson <jesper.nilsson@axis.com> > --- > Changes in v3: > - Avoid multiple assignment > - Link to v2: https://lore.kernel.org/r/20240304-i2c_exynos5-v2-1-7b9c312be719@axis.com > > Changes in v2: > - Use dev_err_probe() instead of open coding it > - Dropped the return failure if we can't find a match in devicetree > - Link to v1: https://lore.kernel.org/r/20240304-i2c_exynos5-v1-1-e91c889d2025@axis.com > --- > drivers/i2c/busses/i2c-exynos5.c | 29 +++++++++++++++-------------- > 1 file changed, 15 insertions(+), 14 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-exynos5.c b/drivers/i2c/busses/i2c-exynos5.c > index 385ef9d9e4d4..8458e22313a7 100644 > --- a/drivers/i2c/busses/i2c-exynos5.c > +++ b/drivers/i2c/busses/i2c-exynos5.c > @@ -906,23 +906,9 @@ static int exynos5_i2c_probe(struct platform_device *pdev) > i2c->adap.algo_data = i2c; > i2c->adap.dev.parent = &pdev->dev; > > - /* Clear pending interrupts from u-boot or misc causes */ > - exynos5_i2c_clr_pend_irq(i2c); > - > spin_lock_init(&i2c->lock); > init_completion(&i2c->msg_complete); > > - i2c->irq = ret = platform_get_irq(pdev, 0); > - if (ret < 0) > - goto err_clk; > - > - ret = devm_request_irq(&pdev->dev, i2c->irq, exynos5_i2c_irq, > - IRQF_NO_SUSPEND, dev_name(&pdev->dev), i2c); > - if (ret != 0) { > - dev_err(&pdev->dev, "cannot request HS-I2C IRQ %d\n", i2c->irq); > - goto err_clk; > - } > - > i2c->variant = of_device_get_match_data(&pdev->dev); > > ret = exynos5_hsi2c_clock_setup(i2c); > @@ -940,6 +926,21 @@ static int exynos5_i2c_probe(struct platform_device *pdev) > clk_disable(i2c->clk); > clk_disable(i2c->pclk); > > + /* Clear pending interrupts from u-boot or misc causes */ > + exynos5_i2c_clr_pend_irq(i2c); Just above this call the clocks have been disabled, so any access to the i2c host registers will result in freeze or external abort (depending on the soc/cpu). To make things worse, this patch moved registering the interrupt handler after the i2c_add_adapter() call. This means that all i2c devices that will be probbed directly from i2c_add_adapter() won't be able to access the i2c bus, as the host controller is still not fully functional that time yet. This breaks today's linux-next on all Exynos5+ platforms. Has anyone tested this change? > + > + ret = platform_get_irq(pdev, 0); > + if (ret < 0) > + goto err_clk; > + i2c->irq = ret; > + > + ret = devm_request_irq(&pdev->dev, i2c->irq, exynos5_i2c_irq, > + IRQF_NO_SUSPEND, dev_name(&pdev->dev), i2c); > + if (ret != 0) { > + dev_err(&pdev->dev, "cannot request HS-I2C IRQ %d\n", i2c->irq); > + goto err_clk; > + } > + > return 0; > > err_clk: > Best regards
On 07/03/2024 12:41, Marek Szyprowski wrote: > On 05.03.2024 11:50, Jesper Nilsson wrote: >> devm_request_irq() is called before we initialize the "variant" >> member variable from of_device_get_match_data(), so if an interrupt >> is triggered inbetween, we can end up following a NULL pointer >> in the interrupt handler. >> >> This problem was exposed when the I2C controller in question was >> (mis)configured to be used in both secure world and Linux. >> >> That this can happen is also reflected by the existing code that >> clears any pending interrupts from "u-boot or misc causes". >> >> Move the clearing of pending interrupts and the call to >> devm_request_irq() to the end of probe. >> >> Reviewed-by: Andi Shyti <andi.shyti@kernel.org> >> Fixes: 218e1496135e ("i2c: exynos5: add support for HSI2C on Exynos5260 SoC") >> Signed-off-by: Jesper Nilsson <jesper.nilsson@axis.com> >> --- >> Changes in v3: >> - Avoid multiple assignment >> - Link to v2: https://lore.kernel.org/r/20240304-i2c_exynos5-v2-1-7b9c312be719@axis.com >> >> Changes in v2: >> - Use dev_err_probe() instead of open coding it >> - Dropped the return failure if we can't find a match in devicetree >> - Link to v1: https://lore.kernel.org/r/20240304-i2c_exynos5-v1-1-e91c889d2025@axis.com >> --- >> drivers/i2c/busses/i2c-exynos5.c | 29 +++++++++++++++-------------- >> 1 file changed, 15 insertions(+), 14 deletions(-) >> >> diff --git a/drivers/i2c/busses/i2c-exynos5.c b/drivers/i2c/busses/i2c-exynos5.c >> index 385ef9d9e4d4..8458e22313a7 100644 >> --- a/drivers/i2c/busses/i2c-exynos5.c >> +++ b/drivers/i2c/busses/i2c-exynos5.c >> @@ -906,23 +906,9 @@ static int exynos5_i2c_probe(struct platform_device *pdev) >> i2c->adap.algo_data = i2c; >> i2c->adap.dev.parent = &pdev->dev; >> >> - /* Clear pending interrupts from u-boot or misc causes */ >> - exynos5_i2c_clr_pend_irq(i2c); >> - >> spin_lock_init(&i2c->lock); >> init_completion(&i2c->msg_complete); >> >> - i2c->irq = ret = platform_get_irq(pdev, 0); >> - if (ret < 0) >> - goto err_clk; >> - >> - ret = devm_request_irq(&pdev->dev, i2c->irq, exynos5_i2c_irq, >> - IRQF_NO_SUSPEND, dev_name(&pdev->dev), i2c); >> - if (ret != 0) { >> - dev_err(&pdev->dev, "cannot request HS-I2C IRQ %d\n", i2c->irq); >> - goto err_clk; >> - } >> - >> i2c->variant = of_device_get_match_data(&pdev->dev); >> >> ret = exynos5_hsi2c_clock_setup(i2c); >> @@ -940,6 +926,21 @@ static int exynos5_i2c_probe(struct platform_device *pdev) >> clk_disable(i2c->clk); >> clk_disable(i2c->pclk); >> >> + /* Clear pending interrupts from u-boot or misc causes */ >> + exynos5_i2c_clr_pend_irq(i2c); > > Just above this call the clocks have been disabled, so any access to the > i2c host registers will result in freeze or external abort (depending on > the soc/cpu). > > To make things worse, this patch moved registering the interrupt handler > after the i2c_add_adapter() call. This means that all i2c devices that > will be probbed directly from i2c_add_adapter() won't be able to access > the i2c bus, as the host controller is still not fully functional that > time yet. > > This breaks today's linux-next on all Exynos5+ platforms. Has anyone > tested this change? I don't think so. So that's the reason my boards fail on today's next/master and next/pending-fixes. Untested code should not be send as fixes :/ Thanks for reporting Marek (and saving me some bisecting). Best regards, Krzysztof
Hi Marek, Am 07.03.24 um 12:41 schrieb Marek Szyprowski: > On 05.03.2024 11:50, Jesper Nilsson wrote: > > devm_request_irq() is called before we initialize the "variant" > > member variable from of_device_get_match_data(), so if an interrupt > > is triggered inbetween, we can end up following a NULL pointer > > in the interrupt handler. > > > > This problem was exposed when the I2C controller in question was > > (mis)configured to be used in both secure world and Linux. > > > > That this can happen is also reflected by the existing code that > > clears any pending interrupts from "u-boot or misc causes". > > > > Move the clearing of pending interrupts and the call to > > devm_request_irq() to the end of probe. > > > > Reviewed-by: Andi Shyti <andi.shyti@kernel.org> > > Fixes: 218e1496135e ("i2c: exynos5: add support for HSI2C on Exynos5260 SoC") > > Signed-off-by: Jesper Nilsson <jesper.nilsson@axis.com> > > --- > > Changes in v3: > > - Avoid multiple assignment > > - Link to v2: https://lore.kernel.org/r/20240304-i2c_exynos5-v2-1-7b9c312be719@axis.com > > > > Changes in v2: > > - Use dev_err_probe() instead of open coding it > > - Dropped the return failure if we can't find a match in devicetree > > - Link to v1: https://lore.kernel.org/r/20240304-i2c_exynos5-v1-1-e91c889d2025@axis.com > > --- > > drivers/i2c/busses/i2c-exynos5.c | 29 +++++++++++++++-------------- > > 1 file changed, 15 insertions(+), 14 deletions(-) > > > > diff --git a/drivers/i2c/busses/i2c-exynos5.c b/drivers/i2c/busses/i2c-exynos5.c > > index 385ef9d9e4d4..8458e22313a7 100644 > > --- a/drivers/i2c/busses/i2c-exynos5.c > > +++ b/drivers/i2c/busses/i2c-exynos5.c > > @@ -906,23 +906,9 @@ static int exynos5_i2c_probe(struct platform_device *pdev) > > i2c->adap.algo_data = i2c; > > i2c->adap.dev.parent = &pdev->dev; > > > > - /* Clear pending interrupts from u-boot or misc causes */ > > - exynos5_i2c_clr_pend_irq(i2c); > > - > > spin_lock_init(&i2c->lock); > > init_completion(&i2c->msg_complete); > > > > - i2c->irq = ret = platform_get_irq(pdev, 0); > > - if (ret < 0) > > - goto err_clk; > > - > > - ret = devm_request_irq(&pdev->dev, i2c->irq, exynos5_i2c_irq, > > - IRQF_NO_SUSPEND, dev_name(&pdev->dev), i2c); > > - if (ret != 0) { > > - dev_err(&pdev->dev, "cannot request HS-I2C IRQ %d\n", i2c->irq); > > - goto err_clk; > > - } > > - > > i2c->variant = of_device_get_match_data(&pdev->dev); > > > > ret = exynos5_hsi2c_clock_setup(i2c); > > @@ -940,6 +926,21 @@ static int exynos5_i2c_probe(struct platform_device *pdev) > > clk_disable(i2c->clk); > > clk_disable(i2c->pclk); > > > > + /* Clear pending interrupts from u-boot or misc causes */ > > + exynos5_i2c_clr_pend_irq(i2c); > > Just above this call the clocks have been disabled, so any access to the > i2c host registers will result in freeze or external abort (depending on > the soc/cpu). > > To make things worse, this patch moved registering the interrupt handler > after the i2c_add_adapter() call. This means that all i2c devices that > will be probbed directly from i2c_add_adapter() won't be able to access > the i2c bus, as the host controller is still not fully functional that > time yet. > > This breaks today's linux-next on all Exynos5+ platforms. Has anyone > tested this change? Tested this patch on top of stable 6.7.9, breaks booting odroid-xu4. Gruß, -- Markus > > > + > > + ret = platform_get_irq(pdev, 0); > > + if (ret < 0) > > + goto err_clk; > > + i2c->irq = ret; > > + > > + ret = devm_request_irq(&pdev->dev, i2c->irq, exynos5_i2c_irq, > > + IRQF_NO_SUSPEND, dev_name(&pdev->dev), i2c); > > + if (ret != 0) { > > + dev_err(&pdev->dev, "cannot request HS-I2C IRQ %d\n", i2c->irq); > > + goto err_clk; > > + } > > + > > return 0; > > > > err_clk: > > > Best regards
diff --git a/drivers/i2c/busses/i2c-exynos5.c b/drivers/i2c/busses/i2c-exynos5.c index 385ef9d9e4d4..8458e22313a7 100644 --- a/drivers/i2c/busses/i2c-exynos5.c +++ b/drivers/i2c/busses/i2c-exynos5.c @@ -906,23 +906,9 @@ static int exynos5_i2c_probe(struct platform_device *pdev) i2c->adap.algo_data = i2c; i2c->adap.dev.parent = &pdev->dev; - /* Clear pending interrupts from u-boot or misc causes */ - exynos5_i2c_clr_pend_irq(i2c); - spin_lock_init(&i2c->lock); init_completion(&i2c->msg_complete); - i2c->irq = ret = platform_get_irq(pdev, 0); - if (ret < 0) - goto err_clk; - - ret = devm_request_irq(&pdev->dev, i2c->irq, exynos5_i2c_irq, - IRQF_NO_SUSPEND, dev_name(&pdev->dev), i2c); - if (ret != 0) { - dev_err(&pdev->dev, "cannot request HS-I2C IRQ %d\n", i2c->irq); - goto err_clk; - } - i2c->variant = of_device_get_match_data(&pdev->dev); ret = exynos5_hsi2c_clock_setup(i2c); @@ -940,6 +926,21 @@ static int exynos5_i2c_probe(struct platform_device *pdev) clk_disable(i2c->clk); clk_disable(i2c->pclk); + /* Clear pending interrupts from u-boot or misc causes */ + exynos5_i2c_clr_pend_irq(i2c); + + ret = platform_get_irq(pdev, 0); + if (ret < 0) + goto err_clk; + i2c->irq = ret; + + ret = devm_request_irq(&pdev->dev, i2c->irq, exynos5_i2c_irq, + IRQF_NO_SUSPEND, dev_name(&pdev->dev), i2c); + if (ret != 0) { + dev_err(&pdev->dev, "cannot request HS-I2C IRQ %d\n", i2c->irq); + goto err_clk; + } + return 0; err_clk: