| Message ID | 1523210867-3806-1-git-send-email-pawel.mikolaj.chmiel@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Pawel, 2018-04-09 3:07 GMT+09:00 Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com>: > All banks with GPIO interrupts should be at beginning > of bank array and without any other types of banks between them. > This order is expected by exynos_eint_gpio_irq, when doing > interrupt group to bank translation. > Otherwise, kernel NULL pointer dereference would happen > when trying to handle interrupt, due to wrong bank being looked up. > Observed on s5pv210, when trying to handle gpj0 interrupt, > where kernel was mapping it to gpi bank. Thanks for the patch! Looks like it might be fixing quite an ugly bug indeed. Just one comment for exynos3250 change below. > > Signed-off-by: Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com> > --- > drivers/pinctrl/samsung/pinctrl-exynos-arm.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/drivers/pinctrl/samsung/pinctrl-exynos-arm.c b/drivers/pinctrl/samsung/pinctrl-exynos-arm.c > index 90c2744..de4ab07 100644 > --- a/drivers/pinctrl/samsung/pinctrl-exynos-arm.c > +++ b/drivers/pinctrl/samsung/pinctrl-exynos-arm.c > @@ -105,12 +105,12 @@ static const struct samsung_pin_bank_data s5pv210_pin_bank[] __initconst = { > EXYNOS_PIN_BANK_EINTG(7, 0x1c0, "gpg1", 0x38), > EXYNOS_PIN_BANK_EINTG(7, 0x1e0, "gpg2", 0x3c), > EXYNOS_PIN_BANK_EINTG(7, 0x200, "gpg3", 0x40), > - EXYNOS_PIN_BANK_EINTN(7, 0x220, "gpi"), > EXYNOS_PIN_BANK_EINTG(8, 0x240, "gpj0", 0x44), > EXYNOS_PIN_BANK_EINTG(6, 0x260, "gpj1", 0x48), > EXYNOS_PIN_BANK_EINTG(8, 0x280, "gpj2", 0x4c), > EXYNOS_PIN_BANK_EINTG(8, 0x2a0, "gpj3", 0x50), > EXYNOS_PIN_BANK_EINTG(5, 0x2c0, "gpj4", 0x54), > + EXYNOS_PIN_BANK_EINTN(7, 0x220, "gpi"), > EXYNOS_PIN_BANK_EINTN(8, 0x2e0, "mp01"), > EXYNOS_PIN_BANK_EINTN(4, 0x300, "mp02"), > EXYNOS_PIN_BANK_EINTN(8, 0x320, "mp03"), > @@ -158,9 +158,6 @@ static const struct samsung_pin_bank_data exynos3250_pin_banks0[] __initconst = > > /* pin banks of exynos3250 pin-controller 1 */ > static const struct samsung_pin_bank_data exynos3250_pin_banks1[] __initconst = { > - EXYNOS_PIN_BANK_EINTN(8, 0x120, "gpe0"), > - EXYNOS_PIN_BANK_EINTN(8, 0x140, "gpe1"), > - EXYNOS_PIN_BANK_EINTN(3, 0x180, "gpe2"), > EXYNOS_PIN_BANK_EINTG(8, 0x040, "gpk0", 0x08), It looks like gpk0 starts with eint_offset = 0x08. Depending on what the SVC register returns on this SoC, it might be group 0, which would be fine, but also group 2, which would require this bank to be at exynos3250_pin_banks1[2]... (or changing the way group is translated to bank pointer, e.g. by subtracting (eint_offset / 4) from the group number. Sylwester, would you be able to check which group number is returned for GPK0 bank in SVC register on Exynos 3250? Best regards, Tomasz
On Sun, Apr 8, 2018 at 8:07 PM, Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com> wrote: > All banks with GPIO interrupts should be at beginning > of bank array and without any other types of banks between them. > This order is expected by exynos_eint_gpio_irq, when doing > interrupt group to bank translation. > Otherwise, kernel NULL pointer dereference would happen > when trying to handle interrupt, due to wrong bank being looked up. > Observed on s5pv210, when trying to handle gpj0 interrupt, > where kernel was mapping it to gpi bank. Thanks for the patch. The issue looks real although one thing was missed - there is a gap in SVC group between GPK2 and GPL0 (pointed by Marek Szyprowski): 0x0 - EINT_23 - gpk0 0x1 - EINT_24 - gpk1 0x2 - EINT_25 - gpk2 0x4 - EINT_27 - gpl0 0x7 - EINT_8 - gpm0 Maybe this should be done differently - to remove such hidden requirement entirely in favor of another parameter of EXYNOS_PIN_BANK_EINTG argument? Anyway if such hidden requirement stays, then please document it in the source code (it maybe next to PIN order... or next macro... or also in exynos_eint_gpio_irq()). Beside that please add cc-stable and appropriate fixes tag, Best regards, Krzysztof
2018-04-10 16:06 GMT+09:00 Krzysztof Kozlowski <krzk@kernel.org>: > On Sun, Apr 8, 2018 at 8:07 PM, Paweł Chmiel > <pawel.mikolaj.chmiel@gmail.com> wrote: >> All banks with GPIO interrupts should be at beginning >> of bank array and without any other types of banks between them. >> This order is expected by exynos_eint_gpio_irq, when doing >> interrupt group to bank translation. >> Otherwise, kernel NULL pointer dereference would happen >> when trying to handle interrupt, due to wrong bank being looked up. >> Observed on s5pv210, when trying to handle gpj0 interrupt, >> where kernel was mapping it to gpi bank. > > Thanks for the patch. The issue looks real although one thing was > missed - there is a gap in SVC group between GPK2 and GPL0 (pointed by > Marek Szyprowski): > > 0x0 - EINT_23 - gpk0 > 0x1 - EINT_24 - gpk1 > 0x2 - EINT_25 - gpk2 > 0x4 - EINT_27 - gpl0 > 0x7 - EINT_8 - gpm0 > > Maybe this should be done differently - to remove such hidden > requirement entirely in favor of another parameter of > EXYNOS_PIN_BANK_EINTG argument? Perhaps let's limit this patch to s5pv210 and Exynos5410 alone, where a simple swap of bank order in the arrays should be okay. We might also need to have some fixes on 4x12, because I noticed that in exynos4x12_pin_banks0[] there is a hole in eint_offsets between gpd1 and gpf0 and exynos4x12_pin_banks1[] starts with gpk0 that has eint_offset equal to 0x08 (not 0). > Anyway if such hidden requirement > stays, then please document it in the source code (it maybe next to > PIN order... or next macro... or also in exynos_eint_gpio_irq()). > > Beside that please add cc-stable and appropriate fixes tag, Agreed. Probably the only safe way of documenting this is to put it inside each bank array, so that when someone creates a copy/paste for new SoC, the comment is clearly visible... Perhaps something like: /* Must start with EINTG banks, ordered by EINT group number. */ Best regards, Tomasz
2018-04-10 17:38 GMT+09:00 Tomasz Figa <tomasz.figa@gmail.com>: > 2018-04-10 16:06 GMT+09:00 Krzysztof Kozlowski <krzk@kernel.org>: >> On Sun, Apr 8, 2018 at 8:07 PM, Paweł Chmiel >> <pawel.mikolaj.chmiel@gmail.com> wrote: >>> All banks with GPIO interrupts should be at beginning >>> of bank array and without any other types of banks between them. >>> This order is expected by exynos_eint_gpio_irq, when doing >>> interrupt group to bank translation. >>> Otherwise, kernel NULL pointer dereference would happen >>> when trying to handle interrupt, due to wrong bank being looked up. >>> Observed on s5pv210, when trying to handle gpj0 interrupt, >>> where kernel was mapping it to gpi bank. >> >> Thanks for the patch. The issue looks real although one thing was >> missed - there is a gap in SVC group between GPK2 and GPL0 (pointed by >> Marek Szyprowski): >> >> 0x0 - EINT_23 - gpk0 >> 0x1 - EINT_24 - gpk1 >> 0x2 - EINT_25 - gpk2 >> 0x4 - EINT_27 - gpl0 >> 0x7 - EINT_8 - gpm0 >> >> Maybe this should be done differently - to remove such hidden >> requirement entirely in favor of another parameter of >> EXYNOS_PIN_BANK_EINTG argument? > > Perhaps let's limit this patch to s5pv210 and Exynos5410 alone, where > a simple swap of bank order in the arrays should be okay. > > We might also need to have some fixes on 4x12, because I noticed that > in exynos4x12_pin_banks0[] there is a hole in eint_offsets between > gpd1 and gpf0 and exynos4x12_pin_banks1[] starts with gpk0 that has > eint_offset equal to 0x08 (not 0). To close the loop, after talking offline and checking the documentation, Exynos4x12 is fine, because the group numbers in SVC register actually match what is defined in bank arrays. Best regards, Tomasz
On Wed, Apr 11, 2018 at 10:36 AM, Tomasz Figa <tomasz.figa@gmail.com> wrote: > 2018-04-10 17:38 GMT+09:00 Tomasz Figa <tomasz.figa@gmail.com>: >> 2018-04-10 16:06 GMT+09:00 Krzysztof Kozlowski <krzk@kernel.org>: >>> On Sun, Apr 8, 2018 at 8:07 PM, Paweł Chmiel >>> <pawel.mikolaj.chmiel@gmail.com> wrote: >>>> All banks with GPIO interrupts should be at beginning >>>> of bank array and without any other types of banks between them. >>>> This order is expected by exynos_eint_gpio_irq, when doing >>>> interrupt group to bank translation. >>>> Otherwise, kernel NULL pointer dereference would happen >>>> when trying to handle interrupt, due to wrong bank being looked up. >>>> Observed on s5pv210, when trying to handle gpj0 interrupt, >>>> where kernel was mapping it to gpi bank. >>> >>> Thanks for the patch. The issue looks real although one thing was >>> missed - there is a gap in SVC group between GPK2 and GPL0 (pointed by >>> Marek Szyprowski): >>> >>> 0x0 - EINT_23 - gpk0 >>> 0x1 - EINT_24 - gpk1 >>> 0x2 - EINT_25 - gpk2 >>> 0x4 - EINT_27 - gpl0 >>> 0x7 - EINT_8 - gpm0 >>> >>> Maybe this should be done differently - to remove such hidden >>> requirement entirely in favor of another parameter of >>> EXYNOS_PIN_BANK_EINTG argument? >> >> Perhaps let's limit this patch to s5pv210 and Exynos5410 alone, where >> a simple swap of bank order in the arrays should be okay. >> >> We might also need to have some fixes on 4x12, because I noticed that >> in exynos4x12_pin_banks0[] there is a hole in eint_offsets between >> gpd1 and gpf0 and exynos4x12_pin_banks1[] starts with gpk0 that has >> eint_offset equal to 0x08 (not 0). > > To close the loop, after talking offline and checking the > documentation, Exynos4x12 is fine, because the group numbers in SVC > register actually match what is defined in bank arrays. Great! Thanks for checking. Best regards, Krzysztof
On Wednesday, April 11, 2018 11:52:44 AM CEST Krzysztof Kozlowski wrote: > On Wed, Apr 11, 2018 at 10:36 AM, Tomasz Figa <tomasz.figa@gmail.com> wrote: > > 2018-04-10 17:38 GMT+09:00 Tomasz Figa <tomasz.figa@gmail.com>: > >> 2018-04-10 16:06 GMT+09:00 Krzysztof Kozlowski <krzk@kernel.org>: > >>> On Sun, Apr 8, 2018 at 8:07 PM, Paweł Chmiel > >>> <pawel.mikolaj.chmiel@gmail.com> wrote: > >>>> All banks with GPIO interrupts should be at beginning > >>>> of bank array and without any other types of banks between them. > >>>> This order is expected by exynos_eint_gpio_irq, when doing > >>>> interrupt group to bank translation. > >>>> Otherwise, kernel NULL pointer dereference would happen > >>>> when trying to handle interrupt, due to wrong bank being looked up. > >>>> Observed on s5pv210, when trying to handle gpj0 interrupt, > >>>> where kernel was mapping it to gpi bank. > >>> > >>> Thanks for the patch. The issue looks real although one thing was > >>> missed - there is a gap in SVC group between GPK2 and GPL0 (pointed by > >>> Marek Szyprowski): > >>> > >>> 0x0 - EINT_23 - gpk0 > >>> 0x1 - EINT_24 - gpk1 > >>> 0x2 - EINT_25 - gpk2 > >>> 0x4 - EINT_27 - gpl0 > >>> 0x7 - EINT_8 - gpm0 > >>> > >>> Maybe this should be done differently - to remove such hidden > >>> requirement entirely in favor of another parameter of > >>> EXYNOS_PIN_BANK_EINTG argument? > >> > >> Perhaps let's limit this patch to s5pv210 and Exynos5410 alone, where > >> a simple swap of bank order in the arrays should be okay. > >> > >> We might also need to have some fixes on 4x12, because I noticed that > >> in exynos4x12_pin_banks0[] there is a hole in eint_offsets between > >> gpd1 and gpf0 and exynos4x12_pin_banks1[] starts with gpk0 that has > >> eint_offset equal to 0x08 (not 0). > > > > To close the loop, after talking offline and checking the > > documentation, Exynos4x12 is fine, because the group numbers in SVC > > register actually match what is defined in bank arrays. > > Great! Thanks for checking. > > Best regards, > Krzysztof > Thanks for all comments. I'll prepare new version of patches, with all fixes and documentation. Best regards Paweł
diff --git a/drivers/pinctrl/samsung/pinctrl-exynos-arm.c b/drivers/pinctrl/samsung/pinctrl-exynos-arm.c index 90c2744..de4ab07 100644 --- a/drivers/pinctrl/samsung/pinctrl-exynos-arm.c +++ b/drivers/pinctrl/samsung/pinctrl-exynos-arm.c @@ -105,12 +105,12 @@ static const struct samsung_pin_bank_data s5pv210_pin_bank[] __initconst = { EXYNOS_PIN_BANK_EINTG(7, 0x1c0, "gpg1", 0x38), EXYNOS_PIN_BANK_EINTG(7, 0x1e0, "gpg2", 0x3c), EXYNOS_PIN_BANK_EINTG(7, 0x200, "gpg3", 0x40), - EXYNOS_PIN_BANK_EINTN(7, 0x220, "gpi"), EXYNOS_PIN_BANK_EINTG(8, 0x240, "gpj0", 0x44), EXYNOS_PIN_BANK_EINTG(6, 0x260, "gpj1", 0x48), EXYNOS_PIN_BANK_EINTG(8, 0x280, "gpj2", 0x4c), EXYNOS_PIN_BANK_EINTG(8, 0x2a0, "gpj3", 0x50), EXYNOS_PIN_BANK_EINTG(5, 0x2c0, "gpj4", 0x54), + EXYNOS_PIN_BANK_EINTN(7, 0x220, "gpi"), EXYNOS_PIN_BANK_EINTN(8, 0x2e0, "mp01"), EXYNOS_PIN_BANK_EINTN(4, 0x300, "mp02"), EXYNOS_PIN_BANK_EINTN(8, 0x320, "mp03"), @@ -158,9 +158,6 @@ static const struct samsung_pin_bank_data exynos3250_pin_banks0[] __initconst = /* pin banks of exynos3250 pin-controller 1 */ static const struct samsung_pin_bank_data exynos3250_pin_banks1[] __initconst = { - EXYNOS_PIN_BANK_EINTN(8, 0x120, "gpe0"), - EXYNOS_PIN_BANK_EINTN(8, 0x140, "gpe1"), - EXYNOS_PIN_BANK_EINTN(3, 0x180, "gpe2"), EXYNOS_PIN_BANK_EINTG(8, 0x040, "gpk0", 0x08), EXYNOS_PIN_BANK_EINTG(7, 0x060, "gpk1", 0x0c), EXYNOS_PIN_BANK_EINTG(7, 0x080, "gpk2", 0x10), @@ -170,6 +167,9 @@ static const struct samsung_pin_bank_data exynos3250_pin_banks1[] __initconst = EXYNOS_PIN_BANK_EINTG(5, 0x2a0, "gpm2", 0x2c), EXYNOS_PIN_BANK_EINTG(8, 0x2c0, "gpm3", 0x30), EXYNOS_PIN_BANK_EINTG(8, 0x2e0, "gpm4", 0x34), + EXYNOS_PIN_BANK_EINTN(8, 0x120, "gpe0"), + EXYNOS_PIN_BANK_EINTN(8, 0x140, "gpe1"), + EXYNOS_PIN_BANK_EINTN(3, 0x180, "gpe2"), EXYNOS_PIN_BANK_EINTW(8, 0xc00, "gpx0", 0x00), EXYNOS_PIN_BANK_EINTW(8, 0xc20, "gpx1", 0x04), EXYNOS_PIN_BANK_EINTW(8, 0xc40, "gpx2", 0x08), @@ -630,7 +630,6 @@ static const struct samsung_pin_bank_data exynos5410_pin_banks0[] __initconst = EXYNOS_PIN_BANK_EINTG(4, 0x100, "gpc3", 0x20), EXYNOS_PIN_BANK_EINTG(7, 0x120, "gpc1", 0x24), EXYNOS_PIN_BANK_EINTG(7, 0x140, "gpc2", 0x28), - EXYNOS_PIN_BANK_EINTN(2, 0x160, "gpm5"), EXYNOS_PIN_BANK_EINTG(8, 0x180, "gpd1", 0x2c), EXYNOS_PIN_BANK_EINTG(8, 0x1A0, "gpe0", 0x30), EXYNOS_PIN_BANK_EINTG(2, 0x1C0, "gpe1", 0x34), @@ -641,6 +640,7 @@ static const struct samsung_pin_bank_data exynos5410_pin_banks0[] __initconst = EXYNOS_PIN_BANK_EINTG(2, 0x260, "gpg2", 0x48), EXYNOS_PIN_BANK_EINTG(4, 0x280, "gph0", 0x4c), EXYNOS_PIN_BANK_EINTG(8, 0x2A0, "gph1", 0x50), + EXYNOS_PIN_BANK_EINTN(2, 0x160, "gpm5"), EXYNOS_PIN_BANK_EINTN(8, 0x2C0, "gpm7"), EXYNOS_PIN_BANK_EINTN(6, 0x2E0, "gpy0"), EXYNOS_PIN_BANK_EINTN(4, 0x300, "gpy1"),
All banks with GPIO interrupts should be at beginning of bank array and without any other types of banks between them. This order is expected by exynos_eint_gpio_irq, when doing interrupt group to bank translation. Otherwise, kernel NULL pointer dereference would happen when trying to handle interrupt, due to wrong bank being looked up. Observed on s5pv210, when trying to handle gpj0 interrupt, where kernel was mapping it to gpi bank. Signed-off-by: Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com> --- drivers/pinctrl/samsung/pinctrl-exynos-arm.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)