Message ID | 48ADA62D-33BD-4E2A-8247-EAA46F36CDD1@Delien.nl |
---|---|
State | Changes Requested |
Delegated to: | Stefano Babic |
Headers | show |
this one is mangled as well -mike
> This patch adds a validity check for GPIO pins to prevent clobbering > reserved bit or even registers, per suggestion of Marek Vasut. > > Please note: > 1. gpio_request no longer checks validity. Pin validity must be checked > explicitly use gpio_invalid prior to request and hence use. Previous > validity check in gpio_request was incomplete anyway. 2. MX233 is not > supported yet. Until it is, macros defining the number of pins for each > bank reside in arch/arm/include/asm/arch-mx28/iomux.h > > Signed-off-by: Robert Deliën <robert at delien.nl<http://delien.nl/>> AARGH! Please add the following lines to the commit message: Cc: Marek Vasut <marek.vasut@gmail.com> Cc: Stefano Babic <sbabic@denx.de> !!!! That way, you won't have to care for the Cc, which you forgot again! > > diff --git a/arch/arm/include/asm/arch-mx28/gpio.h > b/arch/arm/include/asm/arch-mx28/gpio.h index be1c944..5abd7b1 100644 > --- a/arch/arm/include/asm/arch-mx28/gpio.h > +++ b/arch/arm/include/asm/arch-mx28/gpio.h > @@ -25,6 +25,10 @@ > > #ifdef CONFIG_MXS_GPIO > void mxs_gpio_init(void); > + > +extern int gpio_invalid(int gp); > +#define gpio_invalid(gpio) gpio_invalid(gpio) > + Please, gpio_is_valid() and invert the logic. > #else > inline void mxs_gpio_init(void) {} > #endif > diff --git a/arch/arm/include/asm/arch-mx28/iomux.h > b/arch/arm/include/asm/arch-mx28/iomux.h index 7abdf58..2326fdb 100644 > --- a/arch/arm/include/asm/arch-mx28/iomux.h > +++ b/arch/arm/include/asm/arch-mx28/iomux.h > @@ -56,6 +56,16 @@ typedef u32 iomux_cfg_t; > #define MXS_PAD_PULL_VALID_SHIFT 16 > #define MXS_PAD_PULL_VALID_MASK ((iomux_cfg_t)0x1 << > MXS_PAD_PULL_VALID_SHIFT) > > +#define MX28_BANK0_PINS 29 > +#define MX28_BANK1_PINS 32 > +#define MX28_BANK2_PINS 28 > +#define MX28_BANK3_PINS 31 > +#define MX28_BANK4_PINS 21 > + > +#define MX23_BANK0_PINS 32 > +#define MX23_BANK1_PINS 31 > +#define MX23_BANK2_PINS 32 > + It's not used anywhere else than mxs_gpio.c, so define it there to reduce the scope. Also, I'd be just for using array of numbers with explanation comment to avoid making the code unnecessarily bigger. > #define PAD_MUXSEL_0 0 > #define PAD_MUXSEL_1 1 > #define PAD_MUXSEL_2 2 > diff --git a/common/cmd_gpio.c b/common/cmd_gpio.c > index 9cc790a..51e11e7 100644 > --- a/common/cmd_gpio.c > +++ b/common/cmd_gpio.c > @@ -15,6 +15,10 @@ > #define name_to_gpio(name) simple_strtoul(name, NULL, 10) > #endif > > +#ifndef gpio_invalid > +#define gpio_invalid(gpio) (0) > +#endif > + > enum gpio_cmd { > GPIO_INPUT, > GPIO_SET, > @@ -56,6 +60,12 @@ static int do_gpio(cmd_tbl_t *cmdtp, int flag, int argc, > char * const argv[]) if (gpio < 0) > goto show_usage; > > + /* check bank and pin number for validity */ > + if (gpio_invalid(gpio)) { > + printf("gpio: invalid pin %u\n", gpio); > + return -1; > + } > + Please separate this cmd_gpio() part out! Are you ignoring my previous comments completely ? > /* grab the pin before we tweak it */ > if (gpio_request(gpio, "cmd_gpio")) { > printf("gpio: requesting pin %u failed\n", gpio); > diff --git a/drivers/gpio/mxs_gpio.c b/drivers/gpio/mxs_gpio.c > index b7e9591..dabee90 100644 > --- a/drivers/gpio/mxs_gpio.c > +++ b/drivers/gpio/mxs_gpio.c > @@ -31,7 +31,6 @@ > #include <asm/arch/imx-regs.h> > > #if defined(CONFIG_MX23) > -#define PINCTRL_BANKS 3 > #define PINCTRL_DOUT(n) (0x0500 + ((n) * 0x10)) > #define PINCTRL_DIN(n) (0x0600 + ((n) * 0x10)) > #define PINCTRL_DOE(n) (0x0700 + ((n) * 0x10)) > @@ -39,7 +38,6 @@ > #define PINCTRL_IRQEN(n) (0x0900 + ((n) * 0x10)) > #define PINCTRL_IRQSTAT(n) (0x0c00 + ((n) * 0x10)) > #elif defined(CONFIG_MX28) > -#define PINCTRL_BANKS 5 > #define PINCTRL_DOUT(n) (0x0700 + ((n) * 0x10)) > #define PINCTRL_DIN(n) (0x0900 + ((n) * 0x10)) > #define PINCTRL_DOE(n) (0x0b00 + ((n) * 0x10)) > @@ -57,11 +55,25 @@ > #define GPIO_INT_LEV_MASK (1 << 0) > #define GPIO_INT_POL_MASK (1 << 1) > > +static const int mxs_bank_pins[] = { > +#if defined(CONFIG_MX23) > + MX23_BANK0_PINS, > + MX23_BANK1_PINS, > + MX23_BANK2_PINS > +#elif defined(CONFIG_MX28) > + MX28_BANK0_PINS, > + MX28_BANK1_PINS, > + MX28_BANK2_PINS, > + MX28_BANK3_PINS, > + MX28_BANK4_PINS > +#endif Ignoring? I asked you to define it above and don't duplicate the ifdefs. > +}; > + > void mxs_gpio_init(void) > { > int i; > > - for (i = 0; i < PINCTRL_BANKS; i++) { > + for (i = 0; i < ARRAY_SIZE(mxs_bank_pins); i++) { > writel(0, MXS_PINCTRL_BASE + PINCTRL_PIN2IRQ(i)); > writel(0, MXS_PINCTRL_BASE + PINCTRL_IRQEN(i)); > /* Use SCT address here to clear the IRQSTAT bits */ > @@ -69,6 +81,16 @@ void mxs_gpio_init(void) > } > } > > +int gpio_invalid(int gp) > +{ > + if ( (gp & ~(MXS_PAD_BANK_MASK | MXS_PAD_PIN_MASK)) || > + (PAD_BANK(gp) >= ARRAY_SIZE(mxs_bank_pins)) || > + (PAD_PIN(gp) >= mxs_bank_pins[PAD_BANK(gp)]) ) > + return -EINVAL; > + > + return 0; > +} > + > int gpio_get_value(int gp) > { > uint32_t bank = PAD_BANK(gp); > @@ -120,9 +142,6 @@ int gpio_direction_output(int gp, int value) > > int gpio_request(int gp, const char *label) > { > - if (PAD_BANK(gp) > PINCTRL_BANKS) > - return -EINVAL; > - > return 0; > } Please make a checklist before resubmitting. Also, one more thing, submit this as a series to avoid this mess of patches: git format-patch --cover-letter HEAD~3 -o my_awesome_series vim my_awesome_series/0000-* <create some sane cover letter, add Cc: lines there too!!!> git send-email --annotate --to="u-boot@lists.denx.de" my_awesome_series/* Then when someone tells you it's completely wrong ;-) git format-patch etc. git send-email only the patches which you changed, and use the Message-ID for in-reply-to to particular patches, so the mail threading is right. M
Sorry guys; I'm bailing. Out. On Nov 24, 2011, at 1:15, Marek Vasut wrote: >> This patch adds a validity check for GPIO pins to prevent clobbering >> reserved bit or even registers, per suggestion of Marek Vasut. >> >> Please note: >> 1. gpio_request no longer checks validity. Pin validity must be checked >> explicitly use gpio_invalid prior to request and hence use. Previous >> validity check in gpio_request was incomplete anyway. 2. MX233 is not >> supported yet. Until it is, macros defining the number of pins for each >> bank reside in arch/arm/include/asm/arch-mx28/iomux.h >> >> Signed-off-by: Robert Deliën <robert at delien.nl<http://delien.nl/>> > > AARGH! Please add the following lines to the commit message: > > Cc: Marek Vasut <marek.vasut@gmail.com> > Cc: Stefano Babic <sbabic@denx.de> > > !!!! > > That way, you won't have to care for the Cc, which you forgot again! >> >> diff --git a/arch/arm/include/asm/arch-mx28/gpio.h >> b/arch/arm/include/asm/arch-mx28/gpio.h index be1c944..5abd7b1 100644 >> --- a/arch/arm/include/asm/arch-mx28/gpio.h >> +++ b/arch/arm/include/asm/arch-mx28/gpio.h >> @@ -25,6 +25,10 @@ >> >> #ifdef CONFIG_MXS_GPIO >> void mxs_gpio_init(void); >> + >> +extern int gpio_invalid(int gp); >> +#define gpio_invalid(gpio) gpio_invalid(gpio) >> + > > Please, gpio_is_valid() and invert the logic. > >> #else >> inline void mxs_gpio_init(void) {} >> #endif >> diff --git a/arch/arm/include/asm/arch-mx28/iomux.h >> b/arch/arm/include/asm/arch-mx28/iomux.h index 7abdf58..2326fdb 100644 >> --- a/arch/arm/include/asm/arch-mx28/iomux.h >> +++ b/arch/arm/include/asm/arch-mx28/iomux.h >> @@ -56,6 +56,16 @@ typedef u32 iomux_cfg_t; >> #define MXS_PAD_PULL_VALID_SHIFT 16 >> #define MXS_PAD_PULL_VALID_MASK ((iomux_cfg_t)0x1 << >> MXS_PAD_PULL_VALID_SHIFT) >> >> +#define MX28_BANK0_PINS 29 >> +#define MX28_BANK1_PINS 32 >> +#define MX28_BANK2_PINS 28 >> +#define MX28_BANK3_PINS 31 >> +#define MX28_BANK4_PINS 21 >> + >> +#define MX23_BANK0_PINS 32 >> +#define MX23_BANK1_PINS 31 >> +#define MX23_BANK2_PINS 32 >> + > > It's not used anywhere else than mxs_gpio.c, so define it there to reduce the > scope. Also, I'd be just for using array of numbers with explanation comment to > avoid making the code unnecessarily bigger. > >> #define PAD_MUXSEL_0 0 >> #define PAD_MUXSEL_1 1 >> #define PAD_MUXSEL_2 2 >> diff --git a/common/cmd_gpio.c b/common/cmd_gpio.c >> index 9cc790a..51e11e7 100644 >> --- a/common/cmd_gpio.c >> +++ b/common/cmd_gpio.c >> @@ -15,6 +15,10 @@ >> #define name_to_gpio(name) simple_strtoul(name, NULL, 10) >> #endif >> >> +#ifndef gpio_invalid >> +#define gpio_invalid(gpio) (0) >> +#endif >> + >> enum gpio_cmd { >> GPIO_INPUT, >> GPIO_SET, >> @@ -56,6 +60,12 @@ static int do_gpio(cmd_tbl_t *cmdtp, int flag, int argc, >> char * const argv[]) if (gpio < 0) >> goto show_usage; >> >> + /* check bank and pin number for validity */ >> + if (gpio_invalid(gpio)) { >> + printf("gpio: invalid pin %u\n", gpio); >> + return -1; >> + } >> + > > Please separate this cmd_gpio() part out! Are you ignoring my previous comments > completely ? > >> /* grab the pin before we tweak it */ >> if (gpio_request(gpio, "cmd_gpio")) { >> printf("gpio: requesting pin %u failed\n", gpio); >> diff --git a/drivers/gpio/mxs_gpio.c b/drivers/gpio/mxs_gpio.c >> index b7e9591..dabee90 100644 >> --- a/drivers/gpio/mxs_gpio.c >> +++ b/drivers/gpio/mxs_gpio.c >> @@ -31,7 +31,6 @@ >> #include <asm/arch/imx-regs.h> >> >> #if defined(CONFIG_MX23) >> -#define PINCTRL_BANKS 3 >> #define PINCTRL_DOUT(n) (0x0500 + ((n) * 0x10)) >> #define PINCTRL_DIN(n) (0x0600 + ((n) * 0x10)) >> #define PINCTRL_DOE(n) (0x0700 + ((n) * 0x10)) >> @@ -39,7 +38,6 @@ >> #define PINCTRL_IRQEN(n) (0x0900 + ((n) * 0x10)) >> #define PINCTRL_IRQSTAT(n) (0x0c00 + ((n) * 0x10)) >> #elif defined(CONFIG_MX28) >> -#define PINCTRL_BANKS 5 >> #define PINCTRL_DOUT(n) (0x0700 + ((n) * 0x10)) >> #define PINCTRL_DIN(n) (0x0900 + ((n) * 0x10)) >> #define PINCTRL_DOE(n) (0x0b00 + ((n) * 0x10)) >> @@ -57,11 +55,25 @@ >> #define GPIO_INT_LEV_MASK (1 << 0) >> #define GPIO_INT_POL_MASK (1 << 1) >> >> +static const int mxs_bank_pins[] = { >> +#if defined(CONFIG_MX23) >> + MX23_BANK0_PINS, >> + MX23_BANK1_PINS, >> + MX23_BANK2_PINS >> +#elif defined(CONFIG_MX28) >> + MX28_BANK0_PINS, >> + MX28_BANK1_PINS, >> + MX28_BANK2_PINS, >> + MX28_BANK3_PINS, >> + MX28_BANK4_PINS >> +#endif > > Ignoring? I asked you to define it above and don't duplicate the ifdefs. > >> +}; >> + >> void mxs_gpio_init(void) >> { >> int i; >> >> - for (i = 0; i < PINCTRL_BANKS; i++) { >> + for (i = 0; i < ARRAY_SIZE(mxs_bank_pins); i++) { >> writel(0, MXS_PINCTRL_BASE + PINCTRL_PIN2IRQ(i)); >> writel(0, MXS_PINCTRL_BASE + PINCTRL_IRQEN(i)); >> /* Use SCT address here to clear the IRQSTAT bits */ >> @@ -69,6 +81,16 @@ void mxs_gpio_init(void) >> } >> } >> >> +int gpio_invalid(int gp) >> +{ >> + if ( (gp & ~(MXS_PAD_BANK_MASK | MXS_PAD_PIN_MASK)) || >> + (PAD_BANK(gp) >= ARRAY_SIZE(mxs_bank_pins)) || >> + (PAD_PIN(gp) >= mxs_bank_pins[PAD_BANK(gp)]) ) >> + return -EINVAL; >> + >> + return 0; >> +} >> + >> int gpio_get_value(int gp) >> { >> uint32_t bank = PAD_BANK(gp); >> @@ -120,9 +142,6 @@ int gpio_direction_output(int gp, int value) >> >> int gpio_request(int gp, const char *label) >> { >> - if (PAD_BANK(gp) > PINCTRL_BANKS) >> - return -EINVAL; >> - >> return 0; >> } > > Please make a checklist before resubmitting. Also, one more thing, submit this > as a series to avoid this mess of patches: > > git format-patch --cover-letter HEAD~3 -o my_awesome_series > vim my_awesome_series/0000-* > <create some sane cover letter, add Cc: lines there too!!!> > git send-email --annotate --to="u-boot@lists.denx.de" my_awesome_series/* > > Then when someone tells you it's completely wrong ;-) > > git format-patch etc. > git send-email only the patches which you changed, and use the Message-ID for > in-reply-to to particular patches, so the mail threading is right. > > M
> Sorry guys; I'm bailing. > > Out. Well ... it can't be helped. M > > On Nov 24, 2011, at 1:15, Marek Vasut wrote: > >> This patch adds a validity check for GPIO pins to prevent clobbering > >> reserved bit or even registers, per suggestion of Marek Vasut. > >> > >> Please note: > >> 1. gpio_request no longer checks validity. Pin validity must be checked > >> explicitly use gpio_invalid prior to request and hence use. Previous > >> validity check in gpio_request was incomplete anyway. 2. MX233 is not > >> supported yet. Until it is, macros defining the number of pins for each > >> bank reside in arch/arm/include/asm/arch-mx28/iomux.h > >> > >> Signed-off-by: Robert Deliën <robert at delien.nl<http://delien.nl/>> > > > > AARGH! Please add the following lines to the commit message: > > > > Cc: Marek Vasut <marek.vasut@gmail.com> > > Cc: Stefano Babic <sbabic@denx.de> > > > > !!!! > > > > That way, you won't have to care for the Cc, which you forgot again! > > > >> diff --git a/arch/arm/include/asm/arch-mx28/gpio.h > >> b/arch/arm/include/asm/arch-mx28/gpio.h index be1c944..5abd7b1 100644 > >> --- a/arch/arm/include/asm/arch-mx28/gpio.h > >> +++ b/arch/arm/include/asm/arch-mx28/gpio.h > >> @@ -25,6 +25,10 @@ > >> > >> #ifdef CONFIG_MXS_GPIO > >> void mxs_gpio_init(void); > >> + > >> +extern int gpio_invalid(int gp); > >> +#define gpio_invalid(gpio) gpio_invalid(gpio) > >> + > > > > Please, gpio_is_valid() and invert the logic. > > > >> #else > >> inline void mxs_gpio_init(void) {} > >> #endif > >> diff --git a/arch/arm/include/asm/arch-mx28/iomux.h > >> b/arch/arm/include/asm/arch-mx28/iomux.h index 7abdf58..2326fdb 100644 > >> --- a/arch/arm/include/asm/arch-mx28/iomux.h > >> +++ b/arch/arm/include/asm/arch-mx28/iomux.h > >> @@ -56,6 +56,16 @@ typedef u32 iomux_cfg_t; > >> #define MXS_PAD_PULL_VALID_SHIFT 16 > >> #define MXS_PAD_PULL_VALID_MASK ((iomux_cfg_t)0x1 << > >> MXS_PAD_PULL_VALID_SHIFT) > >> > >> +#define MX28_BANK0_PINS 29 > >> +#define MX28_BANK1_PINS 32 > >> +#define MX28_BANK2_PINS 28 > >> +#define MX28_BANK3_PINS 31 > >> +#define MX28_BANK4_PINS 21 > >> + > >> +#define MX23_BANK0_PINS 32 > >> +#define MX23_BANK1_PINS 31 > >> +#define MX23_BANK2_PINS 32 > >> + > > > > It's not used anywhere else than mxs_gpio.c, so define it there to reduce > > the scope. Also, I'd be just for using array of numbers with explanation > > comment to avoid making the code unnecessarily bigger. > > > >> #define PAD_MUXSEL_0 0 > >> #define PAD_MUXSEL_1 1 > >> #define PAD_MUXSEL_2 2 > >> diff --git a/common/cmd_gpio.c b/common/cmd_gpio.c > >> index 9cc790a..51e11e7 100644 > >> --- a/common/cmd_gpio.c > >> +++ b/common/cmd_gpio.c > >> @@ -15,6 +15,10 @@ > >> #define name_to_gpio(name) simple_strtoul(name, NULL, 10) > >> #endif > >> > >> +#ifndef gpio_invalid > >> +#define gpio_invalid(gpio) (0) > >> +#endif > >> + > >> enum gpio_cmd { > >> > >> GPIO_INPUT, > >> GPIO_SET, > >> > >> @@ -56,6 +60,12 @@ static int do_gpio(cmd_tbl_t *cmdtp, int flag, int > >> argc, char * const argv[]) if (gpio < 0) > >> > >> goto show_usage; > >> > >> + /* check bank and pin number for validity */ > >> + if (gpio_invalid(gpio)) { > >> + printf("gpio: invalid pin %u\n", gpio); > >> + return -1; > >> + } > >> + > > > > Please separate this cmd_gpio() part out! Are you ignoring my previous > > comments completely ? > > > >> /* grab the pin before we tweak it */ > >> if (gpio_request(gpio, "cmd_gpio")) { > >> printf("gpio: requesting pin %u failed\n", gpio); > >> > >> diff --git a/drivers/gpio/mxs_gpio.c b/drivers/gpio/mxs_gpio.c > >> index b7e9591..dabee90 100644 > >> --- a/drivers/gpio/mxs_gpio.c > >> +++ b/drivers/gpio/mxs_gpio.c > >> @@ -31,7 +31,6 @@ > >> #include <asm/arch/imx-regs.h> > >> > >> #if defined(CONFIG_MX23) > >> -#define PINCTRL_BANKS 3 > >> #define PINCTRL_DOUT(n) (0x0500 + ((n) * 0x10)) > >> #define PINCTRL_DIN(n) (0x0600 + ((n) * 0x10)) > >> #define PINCTRL_DOE(n) (0x0700 + ((n) * 0x10)) > >> @@ -39,7 +38,6 @@ > >> #define PINCTRL_IRQEN(n) (0x0900 + ((n) * 0x10)) > >> #define PINCTRL_IRQSTAT(n) (0x0c00 + ((n) * 0x10)) > >> #elif defined(CONFIG_MX28) > >> -#define PINCTRL_BANKS 5 > >> #define PINCTRL_DOUT(n) (0x0700 + ((n) * 0x10)) > >> #define PINCTRL_DIN(n) (0x0900 + ((n) * 0x10)) > >> #define PINCTRL_DOE(n) (0x0b00 + ((n) * 0x10)) > >> @@ -57,11 +55,25 @@ > >> #define GPIO_INT_LEV_MASK (1 << 0) > >> #define GPIO_INT_POL_MASK (1 << 1) > >> > >> +static const int mxs_bank_pins[] = { > >> +#if defined(CONFIG_MX23) > >> + MX23_BANK0_PINS, > >> + MX23_BANK1_PINS, > >> + MX23_BANK2_PINS > >> +#elif defined(CONFIG_MX28) > >> + MX28_BANK0_PINS, > >> + MX28_BANK1_PINS, > >> + MX28_BANK2_PINS, > >> + MX28_BANK3_PINS, > >> + MX28_BANK4_PINS > >> +#endif > > > > Ignoring? I asked you to define it above and don't duplicate the ifdefs. > > > >> +}; > >> + > >> void mxs_gpio_init(void) > >> { > >> > >> int i; > >> > >> - for (i = 0; i < PINCTRL_BANKS; i++) { > >> + for (i = 0; i < ARRAY_SIZE(mxs_bank_pins); i++) { > >> > >> writel(0, MXS_PINCTRL_BASE + PINCTRL_PIN2IRQ(i)); > >> writel(0, MXS_PINCTRL_BASE + PINCTRL_IRQEN(i)); > >> /* Use SCT address here to clear the IRQSTAT bits */ > >> > >> @@ -69,6 +81,16 @@ void mxs_gpio_init(void) > >> > >> } > >> > >> } > >> > >> +int gpio_invalid(int gp) > >> +{ > >> + if ( (gp & ~(MXS_PAD_BANK_MASK | MXS_PAD_PIN_MASK)) || > >> + (PAD_BANK(gp) >= ARRAY_SIZE(mxs_bank_pins)) || > >> + (PAD_PIN(gp) >= mxs_bank_pins[PAD_BANK(gp)]) ) > >> + return -EINVAL; > >> + > >> + return 0; > >> +} > >> + > >> int gpio_get_value(int gp) > >> { > >> > >> uint32_t bank = PAD_BANK(gp); > >> > >> @@ -120,9 +142,6 @@ int gpio_direction_output(int gp, int value) > >> > >> int gpio_request(int gp, const char *label) > >> { > >> - if (PAD_BANK(gp) > PINCTRL_BANKS) > >> - return -EINVAL; > >> - > >> > >> return 0; > >> > >> } > > > > Please make a checklist before resubmitting. Also, one more thing, submit > > this as a series to avoid this mess of patches: > > > > git format-patch --cover-letter HEAD~3 -o my_awesome_series > > vim my_awesome_series/0000-* > > <create some sane cover letter, add Cc: lines there too!!!> > > git send-email --annotate --to="u-boot@lists.denx.de" my_awesome_series/* > > > > Then when someone tells you it's completely wrong ;-) > > > > git format-patch etc. > > git send-email only the patches which you changed, and use the Message-ID > > for in-reply-to to particular patches, so the mail threading is right. > > > > M
diff --git a/arch/arm/include/asm/arch-mx28/gpio.h b/arch/arm/include/asm/arch-mx28/gpio.h index be1c944..5abd7b1 100644 --- a/arch/arm/include/asm/arch-mx28/gpio.h +++ b/arch/arm/include/asm/arch-mx28/gpio.h @@ -25,6 +25,10 @@ #ifdef CONFIG_MXS_GPIO void mxs_gpio_init(void); + +extern int gpio_invalid(int gp); +#define gpio_invalid(gpio) gpio_invalid(gpio) + #else inline void mxs_gpio_init(void) {} #endif diff --git a/arch/arm/include/asm/arch-mx28/iomux.h b/arch/arm/include/asm/arch-mx28/iomux.h index 7abdf58..2326fdb 100644 --- a/arch/arm/include/asm/arch-mx28/iomux.h +++ b/arch/arm/include/asm/arch-mx28/iomux.h @@ -56,6 +56,16 @@ typedef u32 iomux_cfg_t; #define MXS_PAD_PULL_VALID_SHIFT 16 #define MXS_PAD_PULL_VALID_MASK ((iomux_cfg_t)0x1 << MXS_PAD_PULL_VALID_SHIFT) +#define MX28_BANK0_PINS 29 +#define MX28_BANK1_PINS 32 +#define MX28_BANK2_PINS 28 +#define MX28_BANK3_PINS 31 +#define MX28_BANK4_PINS 21 + +#define MX23_BANK0_PINS 32 +#define MX23_BANK1_PINS 31 +#define MX23_BANK2_PINS 32 + #define PAD_MUXSEL_0 0 #define PAD_MUXSEL_1 1 #define PAD_MUXSEL_2 2 diff --git a/common/cmd_gpio.c b/common/cmd_gpio.c index 9cc790a..51e11e7 100644 --- a/common/cmd_gpio.c +++ b/common/cmd_gpio.c @@ -15,6 +15,10 @@ #define name_to_gpio(name) simple_strtoul(name, NULL, 10) #endif +#ifndef gpio_invalid +#define gpio_invalid(gpio) (0) +#endif + enum gpio_cmd { GPIO_INPUT, GPIO_SET, @@ -56,6 +60,12 @@ static int do_gpio(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) if (gpio < 0) goto show_usage; + /* check bank and pin number for validity */ + if (gpio_invalid(gpio)) { + printf("gpio: invalid pin %u\n", gpio); + return -1; + } + /* grab the pin before we tweak it */ if (gpio_request(gpio, "cmd_gpio")) { printf("gpio: requesting pin %u failed\n", gpio); diff --git a/drivers/gpio/mxs_gpio.c b/drivers/gpio/mxs_gpio.c index b7e9591..dabee90 100644 --- a/drivers/gpio/mxs_gpio.c +++ b/drivers/gpio/mxs_gpio.c @@ -31,7 +31,6 @@ #include <asm/arch/imx-regs.h> #if defined(CONFIG_MX23) -#define PINCTRL_BANKS 3 #define PINCTRL_DOUT(n) (0x0500 + ((n) * 0x10)) #define PINCTRL_DIN(n) (0x0600 + ((n) * 0x10)) #define PINCTRL_DOE(n) (0x0700 + ((n) * 0x10)) @@ -39,7 +38,6 @@ #define PINCTRL_IRQEN(n) (0x0900 + ((n) * 0x10)) #define PINCTRL_IRQSTAT(n) (0x0c00 + ((n) * 0x10)) #elif defined(CONFIG_MX28) -#define PINCTRL_BANKS 5 #define PINCTRL_DOUT(n) (0x0700 + ((n) * 0x10)) #define PINCTRL_DIN(n) (0x0900 + ((n) * 0x10)) #define PINCTRL_DOE(n) (0x0b00 + ((n) * 0x10)) @@ -57,11 +55,25 @@ #define GPIO_INT_LEV_MASK (1 << 0) #define GPIO_INT_POL_MASK (1 << 1) +static const int mxs_bank_pins[] = { +#if defined(CONFIG_MX23) + MX23_BANK0_PINS, + MX23_BANK1_PINS, + MX23_BANK2_PINS +#elif defined(CONFIG_MX28) + MX28_BANK0_PINS, + MX28_BANK1_PINS, + MX28_BANK2_PINS, + MX28_BANK3_PINS, + MX28_BANK4_PINS +#endif +}; + void mxs_gpio_init(void) { int i; - for (i = 0; i < PINCTRL_BANKS; i++) { + for (i = 0; i < ARRAY_SIZE(mxs_bank_pins); i++) { writel(0, MXS_PINCTRL_BASE + PINCTRL_PIN2IRQ(i)); writel(0, MXS_PINCTRL_BASE + PINCTRL_IRQEN(i)); /* Use SCT address here to clear the IRQSTAT bits */ @@ -69,6 +81,16 @@ void mxs_gpio_init(void) } } +int gpio_invalid(int gp) +{ + if ( (gp & ~(MXS_PAD_BANK_MASK | MXS_PAD_PIN_MASK)) || + (PAD_BANK(gp) >= ARRAY_SIZE(mxs_bank_pins)) || + (PAD_PIN(gp) >= mxs_bank_pins[PAD_BANK(gp)]) ) + return -EINVAL; + + return 0; +} + int gpio_get_value(int gp) { uint32_t bank = PAD_BANK(gp); @@ -120,9 +142,6 @@ int gpio_direction_output(int gp, int value) int gpio_request(int gp, const char *label) { - if (PAD_BANK(gp) > PINCTRL_BANKS) - return -EINVAL; - return 0; }
This patch adds a validity check for GPIO pins to prevent clobbering reserved bit or even registers, per suggestion of Marek Vasut. Please note: 1. gpio_request no longer checks validity. Pin validity must be checked explicitly use gpio_invalid prior to request and hence use. Previous validity check in gpio_request was incomplete anyway. 2. MX233 is not supported yet. Until it is, macros defining the number of pins for each bank reside in arch/arm/include/asm/arch-mx28/iomux.h Signed-off-by: Robert Deliën <robert at delien.nl<http://delien.nl/>>