* [PATCH] i2c: mux: demux-pinctrl: do not deactivate a master that is not active
@ 2022-03-02 11:22 Peter Rosin
2022-03-02 19:42 ` *** SPAM *** " Marion & Christophe JAILLET
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Peter Rosin @ 2022-03-02 11:22 UTC (permalink / raw)
To: linux-i2c; +Cc: Wolfram Sang, Christophe JAILLET, linux-kernel
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
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);
--
2.20.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: *** SPAM *** [PATCH] i2c: mux: demux-pinctrl: do not deactivate a master that is not active
2022-03-02 11:22 [PATCH] i2c: mux: demux-pinctrl: do not deactivate a master that is not active Peter Rosin
@ 2022-03-02 19:42 ` Marion & Christophe JAILLET
2022-03-02 21:17 ` Wolfram Sang
2022-03-19 23:55 ` Wolfram Sang
2 siblings, 0 replies; 6+ messages in thread
From: Marion & Christophe JAILLET @ 2022-03-02 19:42 UTC (permalink / raw)
To: Peter Rosin, linux-i2c; +Cc: Wolfram Sang, linux-kernel
Le 02/03/2022 à 12:22, Peter Rosin a écrit :
> 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
>
> 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);
For what it worth, LGTM.
CJ
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] i2c: mux: demux-pinctrl: do not deactivate a master that is not active
2022-03-02 11:22 [PATCH] i2c: mux: demux-pinctrl: do not deactivate a master that is not active Peter Rosin
2022-03-02 19:42 ` *** SPAM *** " Marion & Christophe JAILLET
@ 2022-03-02 21:17 ` Wolfram Sang
2022-03-02 21:22 ` Wolfram Sang
2022-03-19 23:55 ` Wolfram Sang
2 siblings, 1 reply; 6+ messages in thread
From: Wolfram Sang @ 2022-03-02 21:17 UTC (permalink / raw)
To: Peter Rosin; +Cc: linux-i2c, Christophe JAILLET, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 304 bytes --]
> 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? :)
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] i2c: mux: demux-pinctrl: do not deactivate a master that is not active
2022-03-02 21:17 ` Wolfram Sang
@ 2022-03-02 21:22 ` Wolfram Sang
2022-03-02 22:45 ` Peter Rosin
0 siblings, 1 reply; 6+ messages in thread
From: Wolfram Sang @ 2022-03-02 21:22 UTC (permalink / raw)
To: Peter Rosin, linux-i2c, Christophe JAILLET, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 706 bytes --]
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 }
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] i2c: mux: demux-pinctrl: do not deactivate a master that is not active
2022-03-02 21:22 ` Wolfram Sang
@ 2022-03-02 22:45 ` Peter Rosin
0 siblings, 0 replies; 6+ messages in thread
From: Peter Rosin @ 2022-03-02 22:45 UTC (permalink / raw)
To: Wolfram Sang, linux-i2c, Christophe JAILLET, linux-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] i2c: mux: demux-pinctrl: do not deactivate a master that is not active
2022-03-02 11:22 [PATCH] i2c: mux: demux-pinctrl: do not deactivate a master that is not active Peter Rosin
2022-03-02 19:42 ` *** SPAM *** " Marion & Christophe JAILLET
2022-03-02 21:17 ` Wolfram Sang
@ 2022-03-19 23:55 ` Wolfram Sang
2 siblings, 0 replies; 6+ messages in thread
From: Wolfram Sang @ 2022-03-19 23:55 UTC (permalink / raw)
To: Peter Rosin; +Cc: linux-i2c, Christophe JAILLET, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 732 bytes --]
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!
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-03-19 23:55 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-02 11:22 [PATCH] i2c: mux: demux-pinctrl: do not deactivate a master that is not active Peter Rosin
2022-03-02 19:42 ` *** SPAM *** " Marion & Christophe JAILLET
2022-03-02 21:17 ` Wolfram Sang
2022-03-02 21:22 ` Wolfram Sang
2022-03-02 22:45 ` Peter Rosin
2022-03-19 23:55 ` Wolfram Sang
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.