tegra: gpio: fix null label regression
Subsystems (2)
PATCH 1/1 tegra: gpio: fix null label regression
Fix Tegra GPIO driver to not crash resp. misbehave upon requesting GPIOs with an empty aka NULL label. As the driver uses exclusively the label to check for reservation status actually supplying one is mandatory!
This fixes a regression introduced by commit:
2fccd2d96badcdf6165658a99771a4c475586279
tegra: Convert tegra GPIO driver to use driver model
--- drivers/gpio/tegra_gpio.c | 3 +++ 1 file changed, 3 insertions(+)
drivers/gpio/tegra_gpio.c +3 -0
@@ -171,6 +171,9 @@ static int tegra_gpio_request(struct udevice *dev, unsigned offset, {struct tegra_port_info *state =dev_get_priv (dev); +if (!label) +return -EINVAL ; +if (*state->label[offset])return -EBUSY ;
Signature · 1 line
1.9.3
PATCH 1/1 tegra: gpio: fix null label regression
Hi Marcel,
Quoted 20 lines · Marcel Ziswiler
Fix Tegra GPIO driver to not crash resp. misbehave upon requesting GPIOs with an empty aka NULL label. As the driver uses exclusively the label to check for reservation status actually supplying one is mandatory!
This fixes a regression introduced by commit:
2fccd2d96badcdf6165658a99771a4c475586279
tegra: Convert tegra GPIO driver to use driver modelSigned-off-by Marcel Ziswiler <[email protected]>--- drivers/gpio/tegra_gpio.c | 3 +++ 1 file changed, 3 insertions(+)drivers/gpio/tegra_gpio.c +0
-0
drivers/gpio/tegra_gpio.c static int tegra_gpio_request(struct udevice *dev, unsigned offset, · line 176
Quoted 6 lines
drivers/gpio/tegra_gpio.c +3
-0@@ -171,6 +171,9 @@ static int tegra_gpio_request(struct udevice *dev, unsigned offset, {struct tegra_port_info *state =dev_get_priv (dev); +if (!label) +return -EINVAL ; +
Does this patch fix anything? What exactly does it change with your board?
Quoted 6 lines
drivers/gpio/tegra_gpio.c +0
-1if (*state->label[offset])return -EBUSY ; --1.9.3
Regards,
Simon
PATCH 1/1 tegra: gpio: fix null label regression
Hi Simon
Quoted 1 line · Simon Glass
Does this patch fix anything? What exactly does it change with your board?
Well, yes. It does stop Colibri T30 from crashing with rc3 right after printing DRAM: 1 GiB.
Looking at the former code the string copy stuff was actually gated by a label null check while your latest patch omits that.
Cheers
Marcel
PATCH 1/1 tegra: gpio: fix null label regression
Hi Marcel,
Quoted 10 lines · Marcel Ziswiler
Hi Simon
Quoted 1 line · Simon Glass
Does this patch fix anything? What exactly does it change with your board?
Well, yes. It does stop Colibri T30 from crashing with rc3 right after printing DRAM: 1 GiB.
Looking at the former code the string copy stuff was actually gated by a label null check while your latest patch omits that.
OK, is it possible to stop calling the function with NULL instead? That is a bug IMO.
Regards,
Simon
PATCH 1/1 tegra: gpio: fix null label regression
Hi Simon
Quoted 1 line · Simon Glass
OK, is it possible to stop calling the function with NULL instead?
Yes, sure. Remember
http://article.gmane.org/gmane.comp.boot-loaders.u-boot/198189
Quoted 1 line
That is a bug IMO.
Well, that's what I tried to find out digging through the header file. But I could not quite make out a place where it would state that. Anyway crashing like that seems rather harsh, not?
Cheers
Marcel
PATCH 1/1 tegra: gpio: fix null label regression
Hi Marcel,
Quoted 7 lines · Marcel Ziswiler
Hi Simon
Quoted 1 line · Simon Glass
OK, is it possible to stop calling the function with NULL instead?
Yes, sure. Remember
Quoted 7 lines
http://article.gmane.org/gmane.comp.boot-loaders.u-boot/198189
Quoted 1 line
That is a bug IMO.
Well, that's what I tried to find out digging through the header file. But I could not quite make out a place where it would state that. Anyway crashing like that seems rather harsh, not?
Well you fixed that bug. Are there any others? Checking for obviously invalid args is not typically done due to the code overhead (e.g try to use assert() which is compiled out in production code).
Regards,
Simon
PATCH 1/1 tegra: gpio: fix null label regression
Hi Simon
Quoted 1 line · Simon Glass
Well you fixed that bug. Are there any others?
Well, not in any of our boards but a short grep through the sources reveals dozens of places where GPIOs are still reserved with NULL labels. Happy crashing and subsequent bisecting for all them folks I guess.
Quoted 3 lines
Checking for obviously
invalid args is not typically done due to the code overhead (e.g try to useassert()which is compiled out in production code).
Agreed but rather sad if the semantics certainly changes.
Anyway, I give in. Let's just drop it then.
PATCH 1/1 tegra: gpio: fix null label regression
Hi Marcel,
Quoted 9 lines · Marcel Ziswiler
Hi Simon
Quoted 1 line · Simon Glass
Well you fixed that bug. Are there any others?
Well, not in any of our boards but a short grep through the sources reveals dozens of places where GPIOs are still reserved with NULL labels. Happy crashing and subsequent bisecting for all them folks I guess.
Ah OK. It sounds like people know that the driver ignores the name and so NULL is OK, and that is no longer true. The function signature for gpio_request() just says:
* @param label User label for this GPIO
with no indication that NULL is OK. So I did not see this as a semantic change. But we have to deal with reality.
Quoted 9 lines
Quoted 4 lines
Checking for obviously
invalid args is not typically done due to the code overhead (e.g try to useassert()which is compiled out in production code).Agreed but rather sad if the semantics certainly changes.
Anyway, I give in. Let's just drop it then.
I think we need to have this patch due to the existing code. Thanks for explaining it and sorry for making it so painful.