Re: [PATCH v3 1/7] gpio: gpio-ep93xx: fix BUG_ON port F usage

From: Andy Shevchenko <andy.shevchenko@gmail.com>
To: Nikita Shubin <nikita.shubin@maquefel.me>
Cc: Linus Walleij <linus.walleij@linaro.org>,
	Bartosz Golaszewski <bgolaszewski@baylibre.com>,
	Alexander Sverdlin <alexander.sverdlin@gmail.com>,
	"open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v3 1/7] gpio: gpio-ep93xx: fix BUG_ON port F usage
Date: Thu, 28 Jan 2021 18:11:46 +0200	[thread overview]
Message-ID: <CAHp75VfBb5+K9cSAzj9EBD+KtswkHSNMZWoCaU=bKvOO3fXRjw@mail.gmail.com> (raw)
In-Reply-To: <20210128122123.25341-2-nikita.shubin@maquefel.me>

On Thu, Jan 28, 2021 at 2:21 PM Nikita Shubin <nikita.shubin@maquefel.me> wrote:
>
> The port F is index 2 not 5.
>
> ------------[ cut here ]------------
> kernel BUG at drivers/gpio/gpio-ep93xx.c:64!

Perhaps you missed my message, please cut this to have only related
information and not be so noisy!

> Internal error: Oops - BUG: 0 [#1] ARM
> Modules linked in:
> CPU: 0 PID: 403 Comm: gpio-event-mon Not tainted 5.9.10-00011-ge93e9618628b-dirty #19
> Hardware name: Technologic Systems TS-72xx SBC
> PC is at ep93xx_gpio_update_int_params+0x1c/0x80
> LR is at ep93xx_gpio_update_int_params+0x14/0x80
> pc : [<c03abc44>] lr : [<c03abc3c>] psr: 20000093
> sp : c158de00 ip : 00000000 fp : 00000001
> r10: c44154d4 r9 : 00000000 r8 : c4415020
> r7 : c04ef884 r6 : c051c842 r5 : c4415020 r4 : 00000005
> r3 : 00000000 r2 : 00000000 r1 : c04eb768 r0 : 00000008
> Flags: nzCv IRQs off FIQs on Mode SVC_32 ISA ARM Segment none
> Control: 0000717f Table: 01684000 DAC: 00000051
> Process gpio-event-mon (pid: 403, stack limit = 0x(ptrval))
> Stack: (0xc158de00 to 0xc158e000)
> de00: 00000005 00000002 c051c842 c0238dc0 c0238c98 c0238c98 c04ef874 00000000
> de20: 00000003 c04fcfcc 60000013 c04ef910 c04ef8d4 c00456f0 c04ef874 c15f1e00
> de40: 00000000 00000000 00000001 c0045d40 c15f1e00 c4400160 c0044ca8 c04ef8a8
> de60: 60000013 00000000 c15f1e00 c04ef874 c04ef884 00000001 c0235d70 c158b800
> de80: be825f0f c0045ec8 00000003 c158b800 c440aa00 be825bc8 00000003 00000001
> dea0: 00000000 c0236f00 c44ed3a0 c158b800 c45c2015 00000000 00000001 00000003
> dec0: 6f697067 6576652d 6d2d746e 00006e6f 00000000 00000000 00000000 00000000
> dee0: be825df4 c00abb0c c440c500 c00aabd4 c440c500 c528b840 c45c2010 c04e1228
> df00: 00000ff0 c4478d28 c030b404 be825bc8 c1550e20 00000003 c1550e20 c00c3388
> df20: c4478d28 c00c3d48 be825f0f c00abd00 c45c2000 c45c2000 c1550e20 c00bfea8
> df40: 00000003 c00b0714 00000000 c4450000 00000004 00000100 00000001 c04e1228
> df60: c158dfb0 ffffff9c 000231f8 00000003 00000142 c00b085c 00000000 c04e1228
> df80: 00000000 be825f0f 00000003 00000003 00000036 c00083c4 c158c000 00000000
> dfa0: be825f0f c00081e0 be825f0f 00000003 00000003 c030b404 be825bc8 00000000
> dfc0: be825f0f 00000003 00000003 00000036 00000001 00000000 00022070 be825f0f
> dfe0: b6f2e4e0 be825bac 00010acc b6f2e4ec 60000010 00000003 00000000 00000000
> [<c03abc44>] (ep93xx_gpio_update_int_params) from [<c0238dc0>] (ep93xx_gpio_irq_type+0x128/0x1c0)
> [<c0238dc0>] (ep93xx_gpio_irq_type) from [<c00456f0>] (__irq_set_trigger+0x6c/0x128)
> [<c00456f0>] (__irq_set_trigger) from [<c0045d40>] (__setup_irq+0x594/0x678)
> [<c0045d40>] (__setup_irq) from [<c0045ec8>] (request_threaded_irq+0xa4/0x128)
> [<c0045ec8>] (request_threaded_irq) from [<c0236f00>] (gpio_ioctl+0x300/0x4d8)
> [<c0236f00>] (gpio_ioctl) from [<c00c3388>] (vfs_ioctl+0x24/0x3c)
> [<c00c3388>] (vfs_ioctl) from [<c00c3d48>] (sys_ioctl+0xbc/0x768)
> [<c00c3d48>] (sys_ioctl) from [<c00081e0>] (ret_fast_syscall+0x0/0x50)
> Exception stack(0xc158dfa8 to 0xc158dff0)
> dfa0: be825f0f 00000003 00000003 c030b404 be825bc8 00000000
> dfc0: be825f0f 00000003 00000003 00000036 00000001 00000000 00022070 be825f0f
> dfe0: b6f2e4e0 be825bac 00010acc b6f2e4ec
> Code: e59f0060 ebfff3e1 e3540002 9a000000 (e7f001f2)
> ---[ end trace 3f6544e133e9f5ae ]---
>
> Fixes: fd935fc421e74 ("gpio: ep93xx: Do not pingpong irq numbers")
> Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>

...

> +/*
> + * F Port index in GPIOCHIP'S array is 5
> + * but we use index 2 for stored values and offsets
> + */
> +#define EP93XX_GPIO_F_PORT_INDEX 5

Hmm... Why not to use an array with holes instead.

...

> +       if (port == EP93XX_GPIO_F_PORT_INDEX)
> +               port = 2;

Sorry, but I'm not in favour of this as it adds confusion.
See above for the potential way to solve.

-- 
With Best Regards,
Andy Shevchenko