[PATCH] i2c: imx: Make sure to unregister adapter on remove()

[PATCH] i2c: imx: Make sure to unregister adapter on remove()

[Uwe Kleine-König]

If for whatever reasons pm_runtime_resume_and_get() fails and .remove() is
exited early, the i2c adapter stays around and the irq still calls its
handler, while the driver data and the register mapping go away. So if
later the i2c adapter is accessed or the irq triggers this results in
havoc accessing freed memory and unmapped registers.

So unregister the software resources even if resume failed, and only skip
the hardware access in that case.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/i2c/busses/i2c-imx.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index e9e2db68b9fb..7395560c13d0 100644
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -1572,9 +1572,7 @@ static int i2c_imx_remove(struct platform_device *pdev)
 	struct imx_i2c_struct *i2c_imx = platform_get_drvdata(pdev);
 	int irq, ret;
 
-	ret = pm_runtime_resume_and_get(&pdev->dev);
-	if (ret < 0)
-		return ret;
+	ret = pm_runtime_get_sync(&pdev->dev);
 
 	hrtimer_cancel(&i2c_imx->slave_timer);
 
@@ -1585,17 +1583,21 @@ static int i2c_imx_remove(struct platform_device *pdev)
 	if (i2c_imx->dma)
 		i2c_imx_dma_free(i2c_imx);
 
-	/* setup chip registers to defaults */
-	imx_i2c_write_reg(0, i2c_imx, IMX_I2C_IADR);
-	imx_i2c_write_reg(0, i2c_imx, IMX_I2C_IFDR);
-	imx_i2c_write_reg(0, i2c_imx, IMX_I2C_I2CR);
-	imx_i2c_write_reg(0, i2c_imx, IMX_I2C_I2SR);
+	if (ret == 0) {
+		/* setup chip registers to defaults */
+		imx_i2c_write_reg(0, i2c_imx, IMX_I2C_IADR);
+		imx_i2c_write_reg(0, i2c_imx, IMX_I2C_IFDR);
+		imx_i2c_write_reg(0, i2c_imx, IMX_I2C_I2CR);
+		imx_i2c_write_reg(0, i2c_imx, IMX_I2C_I2SR);
+		clk_disable(i2c_imx->clk);
+	}
 
 	clk_notifier_unregister(i2c_imx->clk, &i2c_imx->clk_change_nb);
 	irq = platform_get_irq(pdev, 0);
 	if (irq >= 0)
 		free_irq(irq, i2c_imx);
-	clk_disable_unprepare(i2c_imx->clk);
+
+	clk_unprepare(i2c_imx->clk);
 
 	pm_runtime_put_noidle(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);

base-commit: 6014cfa5bf32cf8c5c58b3cfd5ee0e1542c8a825
-- 
2.36.1

[PATCH] i2c: imx: Make sure to unregister adapter on remove()

[Uwe Kleine-König]

If for whatever reasons pm_runtime_resume_and_get() fails and .remove() is
exited early, the i2c adapter stays around and the irq still calls its
handler, while the driver data and the register mapping go away. So if
later the i2c adapter is accessed or the irq triggers this results in
havoc accessing freed memory and unmapped registers.

So unregister the software resources even if resume failed, and only skip
the hardware access in that case.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/i2c/busses/i2c-imx.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index e9e2db68b9fb..7395560c13d0 100644
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -1572,9 +1572,7 @@ static int i2c_imx_remove(struct platform_device *pdev)
 	struct imx_i2c_struct *i2c_imx = platform_get_drvdata(pdev);
 	int irq, ret;
 
-	ret = pm_runtime_resume_and_get(&pdev->dev);
-	if (ret < 0)
-		return ret;
+	ret = pm_runtime_get_sync(&pdev->dev);
 
 	hrtimer_cancel(&i2c_imx->slave_timer);
 
@@ -1585,17 +1583,21 @@ static int i2c_imx_remove(struct platform_device *pdev)
 	if (i2c_imx->dma)
 		i2c_imx_dma_free(i2c_imx);
 
-	/* setup chip registers to defaults */
-	imx_i2c_write_reg(0, i2c_imx, IMX_I2C_IADR);
-	imx_i2c_write_reg(0, i2c_imx, IMX_I2C_IFDR);
-	imx_i2c_write_reg(0, i2c_imx, IMX_I2C_I2CR);
-	imx_i2c_write_reg(0, i2c_imx, IMX_I2C_I2SR);
+	if (ret == 0) {
+		/* setup chip registers to defaults */
+		imx_i2c_write_reg(0, i2c_imx, IMX_I2C_IADR);
+		imx_i2c_write_reg(0, i2c_imx, IMX_I2C_IFDR);
+		imx_i2c_write_reg(0, i2c_imx, IMX_I2C_I2CR);
+		imx_i2c_write_reg(0, i2c_imx, IMX_I2C_I2SR);
+		clk_disable(i2c_imx->clk);
+	}
 
 	clk_notifier_unregister(i2c_imx->clk, &i2c_imx->clk_change_nb);
 	irq = platform_get_irq(pdev, 0);
 	if (irq >= 0)
 		free_irq(irq, i2c_imx);
-	clk_disable_unprepare(i2c_imx->clk);
+
+	clk_unprepare(i2c_imx->clk);
 
 	pm_runtime_put_noidle(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);

base-commit: 6014cfa5bf32cf8c5c58b3cfd5ee0e1542c8a825
-- 
2.36.1


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

Re: [PATCH] i2c: imx: Make sure to unregister adapter on remove()

[Oleksij Rempel]

Hi Uwe,

thank you for your work.

On Wed, Jul 20, 2022 at 05:09:33PM +0200, Uwe Kleine-König wrote:
> If for whatever reasons pm_runtime_resume_and_get() fails and .remove() is
> exited early, the i2c adapter stays around and the irq still calls its
> handler, while the driver data and the register mapping go away. So if
> later the i2c adapter is accessed or the irq triggers this results in
> havoc accessing freed memory and unmapped registers.
> 
> So unregister the software resources even if resume failed, and only skip
> the hardware access in that case.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Can you please add Fixes tag. I assume this patch can got to stable
kernel version too.

Otherwise:
Acked-by: Oleksij Rempel <o.rempel@pengutronix.de>

> ---
>  drivers/i2c/busses/i2c-imx.c | 20 +++++++++++---------
>  1 file changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index e9e2db68b9fb..7395560c13d0 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -1572,9 +1572,7 @@ static int i2c_imx_remove(struct platform_device *pdev)
>  	struct imx_i2c_struct *i2c_imx = platform_get_drvdata(pdev);
>  	int irq, ret;
>  
> -	ret = pm_runtime_resume_and_get(&pdev->dev);
> -	if (ret < 0)
> -		return ret;
> +	ret = pm_runtime_get_sync(&pdev->dev);
>  
>  	hrtimer_cancel(&i2c_imx->slave_timer);
>  
> @@ -1585,17 +1583,21 @@ static int i2c_imx_remove(struct platform_device *pdev)
>  	if (i2c_imx->dma)
>  		i2c_imx_dma_free(i2c_imx);
>  
> -	/* setup chip registers to defaults */
> -	imx_i2c_write_reg(0, i2c_imx, IMX_I2C_IADR);
> -	imx_i2c_write_reg(0, i2c_imx, IMX_I2C_IFDR);
> -	imx_i2c_write_reg(0, i2c_imx, IMX_I2C_I2CR);
> -	imx_i2c_write_reg(0, i2c_imx, IMX_I2C_I2SR);
> +	if (ret == 0) {
> +		/* setup chip registers to defaults */
> +		imx_i2c_write_reg(0, i2c_imx, IMX_I2C_IADR);
> +		imx_i2c_write_reg(0, i2c_imx, IMX_I2C_IFDR);
> +		imx_i2c_write_reg(0, i2c_imx, IMX_I2C_I2CR);
> +		imx_i2c_write_reg(0, i2c_imx, IMX_I2C_I2SR);
> +		clk_disable(i2c_imx->clk);
> +	}
>  
>  	clk_notifier_unregister(i2c_imx->clk, &i2c_imx->clk_change_nb);
>  	irq = platform_get_irq(pdev, 0);
>  	if (irq >= 0)
>  		free_irq(irq, i2c_imx);
> -	clk_disable_unprepare(i2c_imx->clk);
> +
> +	clk_unprepare(i2c_imx->clk);
>  
>  	pm_runtime_put_noidle(&pdev->dev);
>  	pm_runtime_disable(&pdev->dev);
> 
> base-commit: 6014cfa5bf32cf8c5c58b3cfd5ee0e1542c8a825
> -- 
> 2.36.1
> 
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] i2c: imx: Make sure to unregister adapter on remove()

[Oleksij Rempel]

Hi Uwe,

thank you for your work.

On Wed, Jul 20, 2022 at 05:09:33PM +0200, Uwe Kleine-König wrote:
> If for whatever reasons pm_runtime_resume_and_get() fails and .remove() is
> exited early, the i2c adapter stays around and the irq still calls its
> handler, while the driver data and the register mapping go away. So if
> later the i2c adapter is accessed or the irq triggers this results in
> havoc accessing freed memory and unmapped registers.
> 
> So unregister the software resources even if resume failed, and only skip
> the hardware access in that case.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Can you please add Fixes tag. I assume this patch can got to stable
kernel version too.

Otherwise:
Acked-by: Oleksij Rempel <o.rempel@pengutronix.de>

> ---
>  drivers/i2c/busses/i2c-imx.c | 20 +++++++++++---------
>  1 file changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index e9e2db68b9fb..7395560c13d0 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -1572,9 +1572,7 @@ static int i2c_imx_remove(struct platform_device *pdev)
>  	struct imx_i2c_struct *i2c_imx = platform_get_drvdata(pdev);
>  	int irq, ret;
>  
> -	ret = pm_runtime_resume_and_get(&pdev->dev);
> -	if (ret < 0)
> -		return ret;
> +	ret = pm_runtime_get_sync(&pdev->dev);
>  
>  	hrtimer_cancel(&i2c_imx->slave_timer);
>  
> @@ -1585,17 +1583,21 @@ static int i2c_imx_remove(struct platform_device *pdev)
>  	if (i2c_imx->dma)
>  		i2c_imx_dma_free(i2c_imx);
>  
> -	/* setup chip registers to defaults */
> -	imx_i2c_write_reg(0, i2c_imx, IMX_I2C_IADR);
> -	imx_i2c_write_reg(0, i2c_imx, IMX_I2C_IFDR);
> -	imx_i2c_write_reg(0, i2c_imx, IMX_I2C_I2CR);
> -	imx_i2c_write_reg(0, i2c_imx, IMX_I2C_I2SR);
> +	if (ret == 0) {
> +		/* setup chip registers to defaults */
> +		imx_i2c_write_reg(0, i2c_imx, IMX_I2C_IADR);
> +		imx_i2c_write_reg(0, i2c_imx, IMX_I2C_IFDR);
> +		imx_i2c_write_reg(0, i2c_imx, IMX_I2C_I2CR);
> +		imx_i2c_write_reg(0, i2c_imx, IMX_I2C_I2SR);
> +		clk_disable(i2c_imx->clk);
> +	}
>  
>  	clk_notifier_unregister(i2c_imx->clk, &i2c_imx->clk_change_nb);
>  	irq = platform_get_irq(pdev, 0);
>  	if (irq >= 0)
>  		free_irq(irq, i2c_imx);
> -	clk_disable_unprepare(i2c_imx->clk);
> +
> +	clk_unprepare(i2c_imx->clk);
>  
>  	pm_runtime_put_noidle(&pdev->dev);
>  	pm_runtime_disable(&pdev->dev);
> 
> base-commit: 6014cfa5bf32cf8c5c58b3cfd5ee0e1542c8a825
> -- 
> 2.36.1
> 
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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

Re: [PATCH] i2c: imx: Make sure to unregister adapter on remove()

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1342 bytes --]

On Fri, Jul 29, 2022 at 06:29:22AM +0200, Oleksij Rempel wrote:
> Hi Uwe,
> 
> thank you for your work.
> 
> On Wed, Jul 20, 2022 at 05:09:33PM +0200, Uwe Kleine-König wrote:
> > If for whatever reasons pm_runtime_resume_and_get() fails and .remove() is
> > exited early, the i2c adapter stays around and the irq still calls its
> > handler, while the driver data and the register mapping go away. So if
> > later the i2c adapter is accessed or the irq triggers this results in
> > havoc accessing freed memory and unmapped registers.
> > 
> > So unregister the software resources even if resume failed, and only skip
> > the hardware access in that case.
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> 
> Can you please add Fixes tag. I assume this patch can got to stable
> kernel version too.

That would be:

Fixes: 588eb93ea49f ("i2c: imx: add runtime pm support to improve the performance")

and that was merged around v4.4. I'm not sure it's sensible to backport
that given the bug is that old and didn't even pop up in a user story
but by a cleanup effort. *shrug* Up to you and Wolfram to decide.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

Re: [PATCH] i2c: imx: Make sure to unregister adapter on remove()

[Uwe Kleine-König]

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

On Fri, Jul 29, 2022 at 06:29:22AM +0200, Oleksij Rempel wrote:
> Hi Uwe,
> 
> thank you for your work.
> 
> On Wed, Jul 20, 2022 at 05:09:33PM +0200, Uwe Kleine-König wrote:
> > If for whatever reasons pm_runtime_resume_and_get() fails and .remove() is
> > exited early, the i2c adapter stays around and the irq still calls its
> > handler, while the driver data and the register mapping go away. So if
> > later the i2c adapter is accessed or the irq triggers this results in
> > havoc accessing freed memory and unmapped registers.
> > 
> > So unregister the software resources even if resume failed, and only skip
> > the hardware access in that case.
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> 
> Can you please add Fixes tag. I assume this patch can got to stable
> kernel version too.

That would be:

Fixes: 588eb93ea49f ("i2c: imx: add runtime pm support to improve the performance")

and that was merged around v4.4. I'm not sure it's sensible to backport
that given the bug is that old and didn't even pop up in a user story
but by a cleanup effort. *shrug* Up to you and Wolfram to decide.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 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] i2c: imx: Make sure to unregister adapter on remove()

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 213 bytes --]


> That would be:
> 
> Fixes: 588eb93ea49f ("i2c: imx: add runtime pm support to improve the performance")

FTR, I added the Fixes tag because I think this is a real bug despite
being unlikely to happen.


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

Re: [PATCH] i2c: imx: Make sure to unregister adapter on remove()

[Wolfram Sang]

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


> That would be:
> 
> Fixes: 588eb93ea49f ("i2c: imx: add runtime pm support to improve the performance")

FTR, I added the Fixes tag because I think this is a real bug despite
being unlikely to happen.


[-- 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] i2c: imx: Make sure to unregister adapter on remove()

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 651 bytes --]

On Wed, Jul 20, 2022 at 05:09:33PM +0200, Uwe Kleine-König wrote:
> If for whatever reasons pm_runtime_resume_and_get() fails and .remove() is
> exited early, the i2c adapter stays around and the irq still calls its
> handler, while the driver data and the register mapping go away. So if
> later the i2c adapter is accessed or the irq triggers this results in
> havoc accessing freed memory and unmapped registers.
> 
> So unregister the software resources even if resume failed, and only skip
> the hardware access in that case.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Applied to for-current, thanks!


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

Re: [PATCH] i2c: imx: Make sure to unregister adapter on remove()

[Wolfram Sang]

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

On Wed, Jul 20, 2022 at 05:09:33PM +0200, Uwe Kleine-König wrote:
> If for whatever reasons pm_runtime_resume_and_get() fails and .remove() is
> exited early, the i2c adapter stays around and the irq still calls its
> handler, while the driver data and the register mapping go away. So if
> later the i2c adapter is accessed or the irq triggers this results in
> havoc accessing freed memory and unmapped registers.
> 
> So unregister the software resources even if resume failed, and only skip
> the hardware access in that case.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Applied to for-current, 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