[U-Boot] [PATCH] 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
Signed-off-by: Marcel Ziswiler marcel@ziswiler.com --- drivers/gpio/tegra_gpio.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/gpio/tegra_gpio.c b/drivers/gpio/tegra_gpio.c index 1cc4abb..70663fc 100644 --- a/drivers/gpio/tegra_gpio.c +++ b/drivers/gpio/tegra_gpio.c @@ -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;
Hi Marcel,
On 10 October 2014 08:56, Marcel Ziswiler marcel@ziswiler.com wrote:
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
Signed-off-by: Marcel Ziswiler marcel@ziswiler.com
drivers/gpio/tegra_gpio.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/gpio/tegra_gpio.c b/drivers/gpio/tegra_gpio.c index 1cc4abb..70663fc 100644 --- a/drivers/gpio/tegra_gpio.c +++ b/drivers/gpio/tegra_gpio.c @@ -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?
if (*state->label[offset]) return -EBUSY;-- 1.9.3
Regards, Simon
Hi Simon
On Fri, 2014-10-10 at 09:26 -0600, Simon Glass wrote:
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
Hi Marcel,
On 10 October 2014 09:37, Marcel Ziswiler marcel@ziswiler.com wrote:
Hi Simon
On Fri, 2014-10-10 at 09:26 -0600, Simon Glass wrote:
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
Hi Simon
On Fri, 2014-10-10 at 09:42 -0600, Simon Glass wrote:
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
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
Hi Marcel,
On 10 October 2014 09:53, Marcel Ziswiler marcel@ziswiler.com wrote:
Hi Simon
On Fri, 2014-10-10 at 09:42 -0600, Simon Glass wrote:
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
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
Hi Simon
On Fri, 2014-10-10 at 16:22 -0600, Simon Glass wrote:
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.
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).
Agreed but rather sad if the semantics certainly changes.
Anyway, I give in. Let's just drop it then.
Hi Marcel,
On 10 October 2014 16:44, Marcel Ziswiler marcel@ziswiler.com wrote:
Hi Simon
On Fri, 2014-10-10 at 16:22 -0600, Simon Glass wrote:
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.
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).
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.
Acked-by: Simon Glass sjg@chromium.org
participants (2)
-
Marcel Ziswiler -
Simon Glass