| Message ID | 16422f295a230ad074f09642660a6f70b7c4aaae.1412952689.git.marcel@ziswiler.com |
|---|---|
| State | Accepted |
| Delegated to: | Tom Warren |
| Headers | show |
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>
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;
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(+)