pinctrl/samsung: Correct EINTG banks order

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(-)

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
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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ł
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html