| Message ID | eca2bf56-7abd-55c0-a423-dce47c38fd66@axentia.se |
|---|---|
| State | Accepted |
| Commit | 1a22aabf20adf89cb216f566913196128766f25b |
| Headers | show |
| Series | i2c: mux: demux-pinctrl: do not deactivate a master that is not active | expand |
> Attempting to rollback the activation of the current master when > the current master has not been activated is bad. priv->cur_chan But the master is activated? 259 /* switch to first parent as active master */ 260 i2c_demux_activate_master(priv, 0); Or am I missing something? :)
On 2022-03-02 22:22, Wolfram Sang wrote: > On Wed, Mar 02, 2022 at 10:17:20PM +0100, Wolfram Sang wrote: >> >>> Attempting to rollback the activation of the current master when >>> the current master has not been activated is bad. priv->cur_chan >> >> But the master is activated? >> >> 259 /* switch to first parent as active master */ >> 260 i2c_demux_activate_master(priv, 0); >> >> Or am I missing something? :) > > But _this_ code is affected by the problem you describe, or? > > 237 if (!adap_np) { > 238 dev_err(&pdev->dev, "can't get phandle for parent %d\n", i); > 239 err = -ENOENT; > 240 goto err_rollback; > 241 } > Yes. That is the error path that, if taken, results in the faulty deactivation. Sorry for not spelling it out... Cheers, Peter
On Wed, Mar 02, 2022 at 12:22:35PM +0100, Peter Rosin wrote: > Attempting to rollback the activation of the current master when > the current master has not been activated is bad. priv->cur_chan > and priv->cur_adap are both still zeroed out and the rollback > may result in attempts to revert an of changeset that has not been > applied and do result in calls to both del and put the zeroed out > i2c_adapter. Maybe it crashes, or whatever, but it's bad in any > case. > > Fixes: e9d1a0a41d44 ("i2c: mux: demux-pinctrl: Fix an error handling path in 'i2c_demux_pinctrl_probe()'") > Cc: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > Signed-off-by: Peter Rosin <peda@axentia.se> Applied to for-next, thanks!
diff --git a/drivers/i2c/muxes/i2c-demux-pinctrl.c b/drivers/i2c/muxes/i2c-demux-pinctrl.c index 5365199a31f4..f7a7405d4350 100644 --- a/drivers/i2c/muxes/i2c-demux-pinctrl.c +++ b/drivers/i2c/muxes/i2c-demux-pinctrl.c @@ -261,7 +261,7 @@ static int i2c_demux_pinctrl_probe(struct platform_device *pdev) err = device_create_file(&pdev->dev, &dev_attr_available_masters); if (err) - goto err_rollback; + goto err_rollback_activation; err = device_create_file(&pdev->dev, &dev_attr_current_master); if (err) @@ -271,8 +271,9 @@ static int i2c_demux_pinctrl_probe(struct platform_device *pdev) err_rollback_available: device_remove_file(&pdev->dev, &dev_attr_available_masters); -err_rollback: +err_rollback_activation: i2c_demux_deactivate_master(priv); +err_rollback: for (j = 0; j < i; j++) { of_node_put(priv->chan[j].parent_np); of_changeset_destroy(&priv->chan[j].chgset);
Attempting to rollback the activation of the current master when the current master has not been activated is bad. priv->cur_chan and priv->cur_adap are both still zeroed out and the rollback may result in attempts to revert an of changeset that has not been applied and do result in calls to both del and put the zeroed out i2c_adapter. Maybe it crashes, or whatever, but it's bad in any case. Fixes: e9d1a0a41d44 ("i2c: mux: demux-pinctrl: Fix an error handling path in 'i2c_demux_pinctrl_probe()'") Cc: Christophe JAILLET <christophe.jaillet@wanadoo.fr> Signed-off-by: Peter Rosin <peda@axentia.se> --- drivers/i2c/muxes/i2c-demux-pinctrl.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) Hi! Either that, or I'm missing something. I found this issue when reading code, and have made no tests to verify if my analysis is correct. Cheers, Peter