ARM i.MX: Fix a kernel panic in i2c_imx_clk_notifier_call().
PATCH 1/1 ARM i.MX: Fix a kernel panic in i2c_imx_clk_notifier_call().
drivers/i2c/busses/i2c-imx.c +2 -2
@@ -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);
Fabio Estevam
reviewerHi Krzysztof,
Quoted 2 lines · Krzysztof Hałasa
Signed-off-by Krzysztof Halasa <[email protected]>
Please provide a commit log, giving some context to your fix.
Is this a regression?
Hi Fabio,
Quoted 1 line · Fabio Estevam
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);
Quoted 1 line
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).
Signature · 4 lines
Krzysztof Halasa Industrial Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland
PATCH 1/1 ARM i.MX: Fix a kernel panic in i2c_imx_clk_notifier_call().
In reply to Fabio Estevam90ad2cbe88c22d0215225ab9594eeead0eb24fde 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).
drivers/i2c/busses/i2c-imx.c +2 -2
@@ -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);
Quoted 11 lines · Krzysztof Hałasa
90ad2cbe88c22d0215225ab9594eeead0eb24fdechanged 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 innotifier_block, not the one inimx_i2c_struct, even though both pointers point to the same clk struct).
Quoted 4 lines
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 <[email protected]>
I didn't look at the patch, but I suggest a Fixes: line here à la:
Best regards
Uwe
Signature · 2 lines
Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | http://www.pengutronix.de/ |
Quoted 1 line · Krzysztof Hałasa
90ad2cbe88c22d0215225ab9594eeead0eb24fdechanged the i.MX I2C bus driver
This reference should ideally be in a fixes-tag, below...
Quoted 10 lines
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 innotifier_block, not the one inimx_i2c_struct, even though both pointers point to the same clk struct).
Quoted 3 lines
This bug causes kernel panic when the IPG clock rate changes (e.g. if any clock derived from IPG changes).
...right here.
Quoted 17 lines
Signed-off-by Krzysztof Halasa <[email protected]>drivers/i2c/busses/i2c-imx.c +2
-2@@ -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);