2013-08-13 04:50:08

by Sonic Zhang

Subject: [PATCH v3] pinctrl: pinmux: Don't free pins requested by other devices in pinmux_disable_setting.

From: Sonic Zhang <[email protected]>

One peripheral may share part of its pins with the 2nd
peripheral and the other pins with the 3rd. If it requests all pins
when part of them has already be requested and owned by the 2nd
peripheral, this request fails and pinmux_disable_setting() is called.
The pinmux_disable_setting() frees all pins of the first peripheral
without checking if the pin is owned by itself or the 2nd, which
results in the malfunction of the 2nd peripheral driver.

Signed-off-by: Sonic Zhang <[email protected]>
---
drivers/pinctrl/pinmux.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c
index 88cc509..ab41552 100644
--- a/drivers/pinctrl/pinmux.c
+++ b/drivers/pinctrl/pinmux.c
@@ -480,15 +480,17 @@ void pinmux_disable_setting(struct pinctrl_setting const *setting)
dev_warn(pctldev->dev,
"could not get pin desc for pin %d\n",
pins[i]);
+ /* And release the pin */
+ pin_free(pctldev, pins[i], NULL);
continue;
}
- desc->mux_setting = NULL;
+ if (desc->mux_setting == &(setting->data.mux)) {
+ desc->mux_setting = NULL;
+ /* And release the pin */
+ pin_free(pctldev, pins[i], NULL);
+ }
}

- /* And release the pins */
- for (i = 0; i < num_pins; i++)
- pin_free(pctldev, pins[i], NULL);
-
if (ops->disable)
ops->disable(pctldev, setting->data.mux.func, setting->data.mux.group);
}
--
1.8.2.3

2013-08-13 15:46:29

by Stephen Warren

Subject: Re: [PATCH v3] pinctrl: pinmux: Don't free pins requested by other devices in pinmux_disable_setting.

On 08/12/2013 10:54 PM, Sonic Zhang wrote:
> From: Sonic Zhang <[email protected]>
>
> One peripheral may share part of its pins with the 2nd
> peripheral and the other pins with the 3rd. If it requests all pins
> when part of them has already be requested and owned by the 2nd
> peripheral, this request fails and pinmux_disable_setting() is called.
> The pinmux_disable_setting() frees all pins of the first peripheral
> without checking if the pin is owned by itself or the 2nd, which
> results in the malfunction of the 2nd peripheral driver.

> diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c

> @@ -480,15 +480,17 @@ void pinmux_disable_setting(struct pinctrl_setting const *setting)
> dev_warn(pctldev->dev,
> "could not get pin desc for pin %d\n",
> pins[i]);
> + /* And release the pin */
> + pin_free(pctldev, pins[i], NULL);

This change doesn't seem useful. The 2 lines right above the patch
context are:

desc = pin_desc_get(pctldev, pins[i]);
if (desc == NULL) {

... and the first thing pin_free() does it call pin_desc_get() again.
So, pin_free() is simply going to fail and print another error.

The rest of the patch looks good.