[PATCH] ARM i.MX: Fix a kernel panic in i2c_imx_clk_notifier_call().

[PATCH] ARM i.MX: Fix a kernel panic in i2c_imx_clk_notifier_call().

[Krzysztof Hałasa]

Signed-off-by: Krzysztof Halasa <khalasa@piap.pl>

--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -510,9 +510,9 @@ static int i2c_imx_clk_notifier_call(struct notifier_block *nb,
 				     unsigned long action, void *data)
 {
 	struct clk_notifier_data *ndata = data;
-	struct imx_i2c_struct *i2c_imx = container_of(&ndata->clk,
+	struct imx_i2c_struct *i2c_imx = container_of(nb,
 						      struct imx_i2c_struct,
-						      clk);
+						      clk_change_nb);
 
 	if (action & POST_RATE_CHANGE)
 		i2c_imx_set_clk(i2c_imx, ndata->new_rate);

Re: [PATCH] ARM i.MX: Fix a kernel panic in i2c_imx_clk_notifier_call().

[Fabio Estevam]

Hi Krzysztof,

On Mon, Dec 3, 2018 at 9:13 AM Krzysztof Hałasa <khalasa@piap.pl> wrote:
>
> Signed-off-by: Krzysztof Halasa <khalasa@piap.pl>

Please provide a commit log, giving some context to your fix.

Is this a regression?

Re: [PATCH] ARM i.MX: Fix a kernel panic in i2c_imx_clk_notifier_call().

[Krzysztof Hałasa]

Hi Fabio,

Fabio Estevam <festevam@gmail.com> writes:

> Please provide a commit log, giving some context to your fix.

Well, I hope Lucas could add something here. I am uncertain how it was
supposed to work, the ndata->clk (the pointer, not the clk pointed by
it) can't be at the same time a member of imx_i2c_struct, and I believe
the macro only does simple arithmetics to get to the outer struct.

@@ -510,9 +510,9 @@ static int i2c_imx_clk_notifier_call(struct notifier_block *nb,
 				     unsigned long action, void *data)
 {
 	struct clk_notifier_data *ndata = data;
-	struct imx_i2c_struct *i2c_imx = container_of(&ndata->clk,
+	struct imx_i2c_struct *i2c_imx = container_of(nb,
 						      struct imx_i2c_struct,
-						      clk);
+						      clk_change_nb);

> Is this a regression?

Probably (it went in between 4.16 and 4.17, commit id is
90ad2cbe88c22d0215225ab9594eeead0eb24fde). However this part may be
unused on many boards (apparently it only fires up if the "IPG" clock
rate changes), so it may not manifest itself. I only hit it when I added
a custom driver (using/requesting a special clock derived from IPG).
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

[PATCH] ARM i.MX: Fix a kernel panic in i2c_imx_clk_notifier_call().

[Krzysztof Hałasa]

90ad2cbe88c22d0215225ab9594eeead0eb24fde changed the i.MX I2C bus driver
to use a notifier whenever the base clock ("ipg" - 66 MHz peripheral
clock) rate changes.

Unfortunately one can't use the container_of() macro this way - the
first argument has to point to a member of the bigger struct (last
argument). Merely pointing to the same value isn't enough (the clk
variable which has its address passed to the macro is the clk in
notifier_block, not the one in imx_i2c_struct, even though both pointers
point to the same clk struct).

This bug causes kernel panic when the IPG clock rate changes (e.g. if
any clock derived from IPG changes).

Signed-off-by: Krzysztof Halasa <khalasa@piap.pl>

--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -510,9 +510,9 @@ static int i2c_imx_clk_notifier_call(struct notifier_block *nb,
 				     unsigned long action, void *data)
 {
 	struct clk_notifier_data *ndata = data;
-	struct imx_i2c_struct *i2c_imx = container_of(&ndata->clk,
+	struct imx_i2c_struct *i2c_imx = container_of(nb,
 						      struct imx_i2c_struct,
-						      clk);
+						      clk_change_nb);
 
 	if (action & POST_RATE_CHANGE)
 		i2c_imx_set_clk(i2c_imx, ndata->new_rate);

Re: [PATCH] ARM i.MX: Fix a kernel panic in i2c_imx_clk_notifier_call().

[Uwe Kleine-König]

On Mon, Dec 17, 2018 at 10:12:14AM +0100, Krzysztof Hałasa wrote:
> 90ad2cbe88c22d0215225ab9594eeead0eb24fde changed the i.MX I2C bus driver
> to use a notifier whenever the base clock ("ipg" - 66 MHz peripheral
> clock) rate changes.
> 
> Unfortunately one can't use the container_of() macro this way - the
> first argument has to point to a member of the bigger struct (last
> argument). Merely pointing to the same value isn't enough (the clk
> variable which has its address passed to the macro is the clk in
> notifier_block, not the one in imx_i2c_struct, even though both pointers
> point to the same clk struct).
> 
> This bug causes kernel panic when the IPG clock rate changes (e.g. if
> any clock derived from IPG changes).
> 
> Signed-off-by: Krzysztof Halasa <khalasa@piap.pl>

I didn't look at the patch, but I suggest a Fixes: line here à la:

Fixes: 90ad2cbe88c2 ("i2c: imx: use clk notifier for rate changes")

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

Re: [PATCH] ARM i.MX: Fix a kernel panic in i2c_imx_clk_notifier_call().

[Peter Rosin]

On 2018-12-17 10:12, Krzysztof Hałasa wrote:
> 90ad2cbe88c22d0215225ab9594eeead0eb24fde changed the i.MX I2C bus driver

This reference should ideally be in a fixes-tag, below...

> to use a notifier whenever the base clock ("ipg" - 66 MHz peripheral
> clock) rate changes.
> 
> Unfortunately one can't use the container_of() macro this way - the
> first argument has to point to a member of the bigger struct (last
> argument). Merely pointing to the same value isn't enough (the clk
> variable which has its address passed to the macro is the clk in
> notifier_block, not the one in imx_i2c_struct, even though both pointers
> point to the same clk struct).
> 
> This bug causes kernel panic when the IPG clock rate changes (e.g. if
> any clock derived from IPG changes).
> 

...right here.

Fixes: 90ad2cbe88c2 ("i2c: imx: use clk notifier for rate changes")
> Signed-off-by: Krzysztof Halasa <khalasa@piap.pl>
> 
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -510,9 +510,9 @@ static int i2c_imx_clk_notifier_call(struct notifier_block *nb,
>  				     unsigned long action, void *data)
>  {
>  	struct clk_notifier_data *ndata = data;
> -	struct imx_i2c_struct *i2c_imx = container_of(&ndata->clk,
> +	struct imx_i2c_struct *i2c_imx = container_of(nb,
>  						      struct imx_i2c_struct,
> -						      clk);
> +						      clk_change_nb);
>  
>  	if (action & POST_RATE_CHANGE)
>  		i2c_imx_set_clk(i2c_imx, ndata->new_rate);
>