Re: [PATCH] pinctrl: pinmux: Don't free pins requested by other devices
Hi Linus, On Thu, Aug 8, 2013 at 12:23 AM, Linus Walleij wrote: > On Wed, Jul 17, 2013 at 7:31 AM, Sonic Zhang wrote: > > I'd like Stephen and Axel to have a look at this as well... > >> From: Sonic Zhang >> >> in pinmux_disable_setting after current device fails to request >> the same pins. >> >> Signed-off-by: Sonic Zhang > > I don't quite understand the patch. Can you provide more context? > >> diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c >> index 88cc509..9ebcf3b 100644 >> --- a/drivers/pinctrl/pinmux.c >> +++ b/drivers/pinctrl/pinmux.c >> @@ -482,13 +482,14 @@ void pinmux_disable_setting(struct pinctrl_setting >> const *setting) >> pins[i]); >> continue; >> } >> + /* And release the pins */ >> + if (desc->mux_usecount && >> + !strcmp(desc->mux_owner, setting->dev_name)) >> + pin_free(pctldev, pins[i], NULL); >> + >> desc->mux_setting = NULL; >> } >> >> - /* And release the pins */ >> - for (i = 0; i < num_pins; i++) >> - pin_free(pctldev, pins[i], NULL); >> - > > For pinmux_disable_setting() to inspect desc->mux_usecount seems > assymetric. This is something pin_free() should do, shouldn't it? > > Should not this codepath be kept and a change made inside pin_free() > for the check above instead? > You can't move this codepath into pin_free(), because the pointer to structure pinctrl_setting is not passed through pin_free(). But yes, checking desc->mux_usecount is not necessary here, because pin_free() has already handled that. I will remove desc->mux_usecount checking in next patch. Regards, Sonic -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] pinctrl: pinmux: Don't free pins requested by other devices
Hi Stephen, On Thu, Aug 8, 2013 at 1:09 AM, Stephen Warren wrote: > On 08/07/2013 10:23 AM, Linus Walleij wrote: >> On Wed, Jul 17, 2013 at 7:31 AM, Sonic Zhang wrote: >> >> I'd like Stephen and Axel to have a look at this as well... >> >>> From: Sonic Zhang >>> >>> in pinmux_disable_setting after current device fails to request >>> the same pins. >>> >>> Signed-off-by: Sonic Zhang >> >> I don't quite understand the patch. Can you provide more context? > > Yes, the commit description needs to describe the problem this solves. > > I'm *guessing* the issue is: > > Something tries to enable a new mux setting on some pins. One of those > pins is already owned by something else. So, applying the current > setting fails. So, pinctrl core attempts to unapply the partially > applied setting. This ends up incorrectly over-writing the conflicting > ownership of the pins with NULL, and hence forgetting about it. > > I think a better change would be something more along the lines of: > > for (i = 0; i < num_pins; i++) > + if (this_device_owns_pin(pins[i]) > pin_free(pctldev, pins[i], NULL); > > ? > > Where this_device_owns_pin() might be someting like: > > desc->owning_setting == setting > > (which would be a new field that needed to be assigned during > pinmux_enable_setting). > > Or perhaps the strcmp() is fine. > You are right. 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. I am fine to compare owner's pinctrl_setting structure other than name string. Regards, Sonic -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] pinctrl: pinmux: Don't free pins requested by other devices
On 08/07/2013 10:23 AM, Linus Walleij wrote: > On Wed, Jul 17, 2013 at 7:31 AM, Sonic Zhang wrote: > > I'd like Stephen and Axel to have a look at this as well... > >> From: Sonic Zhang >> >> in pinmux_disable_setting after current device fails to request >> the same pins. >> >> Signed-off-by: Sonic Zhang > > I don't quite understand the patch. Can you provide more context? Yes, the commit description needs to describe the problem this solves. I'm *guessing* the issue is: Something tries to enable a new mux setting on some pins. One of those pins is already owned by something else. So, applying the current setting fails. So, pinctrl core attempts to unapply the partially applied setting. This ends up incorrectly over-writing the conflicting ownership of the pins with NULL, and hence forgetting about it. I think a better change would be something more along the lines of: for (i = 0; i < num_pins; i++) + if (this_device_owns_pin(pins[i]) pin_free(pctldev, pins[i], NULL); ? Where this_device_owns_pin() might be someting like: desc->owning_setting == setting (which would be a new field that needed to be assigned during pinmux_enable_setting). Or perhaps the strcmp() is fine. >> diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c >> index 88cc509..9ebcf3b 100644 >> --- a/drivers/pinctrl/pinmux.c >> +++ b/drivers/pinctrl/pinmux.c >> @@ -482,13 +482,14 @@ void pinmux_disable_setting(struct pinctrl_setting >> const *setting) >> pins[i]); >> continue; >> } >> + /* And release the pins */ >> + if (desc->mux_usecount && >> + !strcmp(desc->mux_owner, setting->dev_name)) >> + pin_free(pctldev, pins[i], NULL); >> + >> desc->mux_setting = NULL; >> } >> >> - /* And release the pins */ >> - for (i = 0; i < num_pins; i++) >> - pin_free(pctldev, pins[i], NULL); >> - > > For pinmux_disable_setting() to inspect desc->mux_usecount seems > assymetric. This is something pin_free() should do, shouldn't it? > > Should not this codepath be kept and a change made inside pin_free() > for the check above instead? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] pinctrl: pinmux: Don't free pins requested by other devices
On Wed, Jul 17, 2013 at 7:31 AM, Sonic Zhang wrote: I'd like Stephen and Axel to have a look at this as well... > From: Sonic Zhang > > in pinmux_disable_setting after current device fails to request > the same pins. > > Signed-off-by: Sonic Zhang I don't quite understand the patch. Can you provide more context? > diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c > index 88cc509..9ebcf3b 100644 > --- a/drivers/pinctrl/pinmux.c > +++ b/drivers/pinctrl/pinmux.c > @@ -482,13 +482,14 @@ void pinmux_disable_setting(struct pinctrl_setting > const *setting) > pins[i]); > continue; > } > + /* And release the pins */ > + if (desc->mux_usecount && > + !strcmp(desc->mux_owner, setting->dev_name)) > + pin_free(pctldev, pins[i], NULL); > + > desc->mux_setting = NULL; > } > > - /* And release the pins */ > - for (i = 0; i < num_pins; i++) > - pin_free(pctldev, pins[i], NULL); > - For pinmux_disable_setting() to inspect desc->mux_usecount seems assymetric. This is something pin_free() should do, shouldn't it? Should not this codepath be kept and a change made inside pin_free() for the check above instead? Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] pinctrl: pinmux: Don't free pins requested by other devices
On Wed, Jul 17, 2013 at 7:31 AM, Sonic Zhang sonic@gmail.com wrote: I'd like Stephen and Axel to have a look at this as well... From: Sonic Zhang sonic.zh...@analog.com in pinmux_disable_setting after current device fails to request the same pins. Signed-off-by: Sonic Zhang sonic.zh...@analog.com I don't quite understand the patch. Can you provide more context? diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c index 88cc509..9ebcf3b 100644 --- a/drivers/pinctrl/pinmux.c +++ b/drivers/pinctrl/pinmux.c @@ -482,13 +482,14 @@ void pinmux_disable_setting(struct pinctrl_setting const *setting) pins[i]); continue; } + /* And release the pins */ + if (desc-mux_usecount + !strcmp(desc-mux_owner, setting-dev_name)) + pin_free(pctldev, pins[i], NULL); + desc-mux_setting = NULL; } - /* And release the pins */ - for (i = 0; i num_pins; i++) - pin_free(pctldev, pins[i], NULL); - For pinmux_disable_setting() to inspect desc-mux_usecount seems assymetric. This is something pin_free() should do, shouldn't it? Should not this codepath be kept and a change made inside pin_free() for the check above instead? Yours, Linus Walleij -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] pinctrl: pinmux: Don't free pins requested by other devices
On 08/07/2013 10:23 AM, Linus Walleij wrote: On Wed, Jul 17, 2013 at 7:31 AM, Sonic Zhang sonic@gmail.com wrote: I'd like Stephen and Axel to have a look at this as well... From: Sonic Zhang sonic.zh...@analog.com in pinmux_disable_setting after current device fails to request the same pins. Signed-off-by: Sonic Zhang sonic.zh...@analog.com I don't quite understand the patch. Can you provide more context? Yes, the commit description needs to describe the problem this solves. I'm *guessing* the issue is: Something tries to enable a new mux setting on some pins. One of those pins is already owned by something else. So, applying the current setting fails. So, pinctrl core attempts to unapply the partially applied setting. This ends up incorrectly over-writing the conflicting ownership of the pins with NULL, and hence forgetting about it. I think a better change would be something more along the lines of: for (i = 0; i num_pins; i++) + if (this_device_owns_pin(pins[i]) pin_free(pctldev, pins[i], NULL); ? Where this_device_owns_pin() might be someting like: desc-owning_setting == setting (which would be a new field that needed to be assigned during pinmux_enable_setting). Or perhaps the strcmp() is fine. diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c index 88cc509..9ebcf3b 100644 --- a/drivers/pinctrl/pinmux.c +++ b/drivers/pinctrl/pinmux.c @@ -482,13 +482,14 @@ void pinmux_disable_setting(struct pinctrl_setting const *setting) pins[i]); continue; } + /* And release the pins */ + if (desc-mux_usecount + !strcmp(desc-mux_owner, setting-dev_name)) + pin_free(pctldev, pins[i], NULL); + desc-mux_setting = NULL; } - /* And release the pins */ - for (i = 0; i num_pins; i++) - pin_free(pctldev, pins[i], NULL); - For pinmux_disable_setting() to inspect desc-mux_usecount seems assymetric. This is something pin_free() should do, shouldn't it? Should not this codepath be kept and a change made inside pin_free() for the check above instead? -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] pinctrl: pinmux: Don't free pins requested by other devices
Hi Stephen, On Thu, Aug 8, 2013 at 1:09 AM, Stephen Warren swar...@wwwdotorg.org wrote: On 08/07/2013 10:23 AM, Linus Walleij wrote: On Wed, Jul 17, 2013 at 7:31 AM, Sonic Zhang sonic@gmail.com wrote: I'd like Stephen and Axel to have a look at this as well... From: Sonic Zhang sonic.zh...@analog.com in pinmux_disable_setting after current device fails to request the same pins. Signed-off-by: Sonic Zhang sonic.zh...@analog.com I don't quite understand the patch. Can you provide more context? Yes, the commit description needs to describe the problem this solves. I'm *guessing* the issue is: Something tries to enable a new mux setting on some pins. One of those pins is already owned by something else. So, applying the current setting fails. So, pinctrl core attempts to unapply the partially applied setting. This ends up incorrectly over-writing the conflicting ownership of the pins with NULL, and hence forgetting about it. I think a better change would be something more along the lines of: for (i = 0; i num_pins; i++) + if (this_device_owns_pin(pins[i]) pin_free(pctldev, pins[i], NULL); ? Where this_device_owns_pin() might be someting like: desc-owning_setting == setting (which would be a new field that needed to be assigned during pinmux_enable_setting). Or perhaps the strcmp() is fine. You are right. 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. I am fine to compare owner's pinctrl_setting structure other than name string. Regards, Sonic -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] pinctrl: pinmux: Don't free pins requested by other devices
Hi Linus, On Thu, Aug 8, 2013 at 12:23 AM, Linus Walleij linus.wall...@linaro.org wrote: On Wed, Jul 17, 2013 at 7:31 AM, Sonic Zhang sonic@gmail.com wrote: I'd like Stephen and Axel to have a look at this as well... From: Sonic Zhang sonic.zh...@analog.com in pinmux_disable_setting after current device fails to request the same pins. Signed-off-by: Sonic Zhang sonic.zh...@analog.com I don't quite understand the patch. Can you provide more context? diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c index 88cc509..9ebcf3b 100644 --- a/drivers/pinctrl/pinmux.c +++ b/drivers/pinctrl/pinmux.c @@ -482,13 +482,14 @@ void pinmux_disable_setting(struct pinctrl_setting const *setting) pins[i]); continue; } + /* And release the pins */ + if (desc-mux_usecount + !strcmp(desc-mux_owner, setting-dev_name)) + pin_free(pctldev, pins[i], NULL); + desc-mux_setting = NULL; } - /* And release the pins */ - for (i = 0; i num_pins; i++) - pin_free(pctldev, pins[i], NULL); - For pinmux_disable_setting() to inspect desc-mux_usecount seems assymetric. This is something pin_free() should do, shouldn't it? Should not this codepath be kept and a change made inside pin_free() for the check above instead? You can't move this codepath into pin_free(), because the pointer to structure pinctrl_setting is not passed through pin_free(). But yes, checking desc-mux_usecount is not necessary here, because pin_free() has already handled that. I will remove desc-mux_usecount checking in next patch. Regards, Sonic -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] pinctrl: pinmux: Don't free pins requested by other devices
Ping On Wed, Jul 17, 2013 at 1:31 PM, Sonic Zhang wrote: > From: Sonic Zhang > > in pinmux_disable_setting after current device fails to request > the same pins. > > Signed-off-by: Sonic Zhang > --- > drivers/pinctrl/pinmux.c | 9 + > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c > index 88cc509..9ebcf3b 100644 > --- a/drivers/pinctrl/pinmux.c > +++ b/drivers/pinctrl/pinmux.c > @@ -482,13 +482,14 @@ void pinmux_disable_setting(struct pinctrl_setting > const *setting) > pins[i]); > continue; > } > + /* And release the pins */ > + if (desc->mux_usecount && > + !strcmp(desc->mux_owner, setting->dev_name)) > + pin_free(pctldev, pins[i], NULL); > + > desc->mux_setting = 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 > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] pinctrl: pinmux: Don't free pins requested by other devices
Ping On Wed, Jul 17, 2013 at 1:31 PM, Sonic Zhang sonic@gmail.com wrote: From: Sonic Zhang sonic.zh...@analog.com in pinmux_disable_setting after current device fails to request the same pins. Signed-off-by: Sonic Zhang sonic.zh...@analog.com --- drivers/pinctrl/pinmux.c | 9 + 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c index 88cc509..9ebcf3b 100644 --- a/drivers/pinctrl/pinmux.c +++ b/drivers/pinctrl/pinmux.c @@ -482,13 +482,14 @@ void pinmux_disable_setting(struct pinctrl_setting const *setting) pins[i]); continue; } + /* And release the pins */ + if (desc-mux_usecount + !strcmp(desc-mux_owner, setting-dev_name)) + pin_free(pctldev, pins[i], NULL); + desc-mux_setting = 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
