* [PATCH 0/3] aspeed:lpc: Fix lpc-snoop probe exception
@ 2022-08-21 16:06 Igor Kononenko
2022-08-21 15:54 ` [PATCH 1/3] ipmi:kcs_bmc: Add cleanup regmap(interrupt-regs) on a shutdown Igor Kononenko
` (3 more replies)
0 siblings, 4 replies; 11+ messages in thread
From: Igor Kononenko @ 2022-08-21 16:06 UTC (permalink / raw)
To: linux-aspeed
The bmc might be rebooted while the host is still reachable and the host
might send requests through configured lpc-kcs channels in same time.
That leads to raise lpc-snoop/lpc-kcs interrupts that haven't adjusted IRQ
handlers yet on next early kernel boot, because on the aspeed-chip reboot
might keep lpc-registers on a unclean state that is configured on the last
boot.
The described way might raise the next exception:
```
[ 1.360110] irq 35: nobody cared (try booting with the "irqpoll" option)
[ 1.360145] CPU: 0 PID: 1 Comm: swapper Not tainted 5.4.43-c109de3-24cc5b6 #1
[ 1.360158] Hardware name: Generic DT based system
[ 1.360168] Backtrace:
[ 1.360228] [<80107f5c>] (dump_backtrace) from [<80108184>] (show_stack+0x20/0x24)
[ 1.360250] r7:00000023 r6:00000000 r5:00000000 r4:9d12d560
[ 1.360283] [<80108164>] (show_stack) from [<8084ae54>] (dump_stack+0x20/0x28)
[ 1.360316] [<8084ae34>] (dump_stack) from [<80156790>] (__report_bad_irq+0x40/0xc0)
[ 1.360344] [<80156750>] (__report_bad_irq) from [<801566c0>] (note_interrupt+0x238/0x290)
[ 1.360366] r9:9d0ae000 r8:9d00c600 r7:00000023 r6:00000000 r5:00000000 r4:9d12d560
[ 1.360408] [<80156488>] (note_interrupt) from [<80153594>] (handle_irq_event+0xb4/0xc4)
[ 1.360429] r10:00000000 r9:9d0ae000 r8:9d00c600 r7:00000001 r6:00000000 r5:00000000
[ 1.360440] r4:9d12d560 r3:00000000
[ 1.360466] [<801534e0>] (handle_irq_event) from [<8015788c>] (handle_level_irq+0xac/0x180)
[ 1.360480] r5:80c7d35c r4:9d12d560
[ 1.360503] [<801577e0>] (handle_level_irq) from [<80152a5c>] (__handle_domain_irq+0x6c/0xc8)
[ 1.360519] r5:80c7d35c r4:9d12d560
[ 1.360545] [<801529f0>] (__handle_domain_irq) from [<801021cc>] (avic_handle_irq+0x68/0x70)
[ 1.360568] r9:9d0ae000 r8:9d12d608 r7:9d0afc84 r6:ffffffff r5:9d0afc50 r4:9d002380
[ 1.360587] [<80102164>] (avic_handle_irq) from [<80101a6c>] (__irq_svc+0x6c/0x90)
[ 1.360603] Exception stack(0x9d0afc50 to 0x9d0afc98)
[ 1.360620] fc40: 00000000 00000100 00000000 00000000
[ 1.360640] fc60: 9d12d560 98bbd0c0 00000000 00000023 9d12d608 60000053 00000000 9d0afcd4
[ 1.360657] fc80: 9d0afc80 9d0afca0 801570ec 80154cdc 40000053 ffffffff
[ 1.360670] r5:40000053 r4:80154cdc
[ 1.360693] [<801549e0>] (__setup_irq) from [<801555e4>] (request_threaded_irq+0xdc/0x15c)
[ 1.360715] r9:98bbb340 r8:00000023 r7:9d12d570 r6:9d12d560 r5:00000000 r4:98bbd0c0
[ 1.360741] [<80155508>] (request_threaded_irq) from [<801589d8>] (devm_request_threaded_irq+0x70/0xc4)
[ 1.360762] r10:80a7353c r9:00000000 r8:98bbb340 r7:9d130e10 r6:00000023 r5:804f4a70
[ 1.360774] r4:98bbd020 r3:00000080
[ 1.360800] [<80158968>] (devm_request_threaded_irq) from [<804f4ebc>] (aspeed_lpc_snoop_probe+0x100/0x2ac)
[ 1.360821] r10:00000000 r9:9d130e10 r8:98bbd040 r7:00000000 r6:9d130e00 r5:98bbb340
[ 1.360830] r4:00000000
[ 1.360851] [<804f4dbc>] (aspeed_lpc_snoop_probe) from [<8056a5c4>] (platform_drv_probe+0x44/0x80)
[ 1.360873] r9:80c5ef90 r8:00000000 r7:80cc2938 r6:00000000 r5:80c5ef90 r4:9d130e10
[ 1.360910] [<8056a580>] (platform_drv_probe) from [<80568420>] (really_probe+0x26c/0x498)
[ 1.360924] r5:80cc282c r4:9d130e10
[ 1.360949] [<805681b4>] (really_probe) from [<80568c28>] (driver_probe_device+0x138/0x184)
[ 1.360970] r10:80b0050c r9:80adadb0 r8:00000007 r7:80c5ef90 r6:9d130e10 r5:00000000
[ 1.360981] r4:80c5ef90
[ 1.361004] [<80568af0>] (driver_probe_device) from [<80568fe4>] (device_driver_attach+0xb8/0xc0)
[ 1.361020] r7:80c5ef90 r6:9d130e54 r5:00000000 r4:9d130e10
[ 1.361046] [<80568f2c>] (device_driver_attach) from [<80569070>] (__driver_attach+0x84/0x16c)
[ 1.361063] r7:80c61128 r6:9d130e10 r5:00000001 r4:80c5ef90
[ 1.361088] [<80568fec>] (__driver_attach) from [<80566068>] (bus_for_each_dev+0x84/0xcc)
[ 1.361106] r7:80c61128 r6:80568fec r5:80c5ef90 r4:00000000
[ 1.361130] [<80565fe4>] (bus_for_each_dev) from [<80569180>] (driver_attach+0x28/0x30)
[ 1.361147] r6:00000000 r5:9d1a3d40 r4:80c5ef90
[ 1.361169] [<80569158>] (driver_attach) from [<80566a08>] (bus_add_driver+0x114/0x200)
[ 1.361195] [<805668f4>] (bus_add_driver) from [<80569814>] (driver_register+0x98/0x128)
[ 1.361214] r7:00000000 r6:80ca0ca0 r5:00000000 r4:80c5ef90
[ 1.361241] [<8056977c>] (driver_register) from [<8056b528>] (__platform_driver_register+0x40/0x54)
[ 1.361256] r5:000000b8 r4:80b2575c
[ 1.361294] [<8056b4e8>] (__platform_driver_register) from [<80b2577c>] (aspeed_lpc_snoop_driver_init+0x20/0x28)
[ 1.361331] [<80b2575c>] (aspeed_lpc_snoop_driver_init) from [<80b01318>] (do_one_initcall+0x84/0x184)
[ 1.361356] [<80b01294>] (do_one_initcall) from [<80b01540>] (kernel_init_freeable+0x128/0x1ec)
[ 1.361375] r7:80b3f858 r6:80ca0ca0 r5:000000b8 r4:80b61914
[ 1.361412] [<80b01418>] (kernel_init_freeable) from [<80864060>] (kernel_init+0x18/0x11c)
[ 1.361435] r10:00000000 r9:00000000 r8:00000000 r7:00000000 r6:00000000 r5:80864048
[ 1.361444] r4:00000000
[ 1.361470] [<80864048>] (kernel_init) from [<801010e8>] (ret_from_fork+0x14/0x2c)
[ 1.361483] Exception stack(0x9d0affb0 to 0x9d0afff8)
[ 1.361500] ffa0: 00000000 00000000 00000000 00000000
[ 1.361518] ffc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[ 1.361535] ffe0: 00000000 00000000 00000000 00000000 00000013 00000000
[ 1.361547] r5:80864048 r4:00000000
[ 1.361555] handlers:
[ 1.361592] [<(ptrval)>] aspeed_lpc_snoop_irq
[ 1.361609] Disabling IRQ #35
```
Main caught of that state is the lpc-snoop driver found on the same
1e789080 address and have same IRQ#35 as for lpc-kcs, and lpc-snoop
registering earlier than lpc-kcs. So, on the lpc-snoop initialization
the lpc-snoop IRQ will be registried for IRQ#35, but it will
passthrough it for the LPC KCS handlers that is not registried yet.
Summary we got the `nobody cared` warning about more 100.000 unhandled
IRQ#35
Steps to reproduce:
* Turn-on BMC
* Turn-on HOST
* Run `watch 'ipmitool sensor'` on the host that is configured to pass
requests via kcs-channel(3,4)
* Reboot BMC.
* On the next BMC boot kernel(BMC) dmesg have exception mentioned above.
The following patchset aims to fixup described issue.
Igor Kononenko (3):
ipmi:kcs_bmc: Add cleanup regmap(interrupt-regs) on a shutdown.
drivers/misc: (aspeed-lpc-snoop): Add regmap cleanup on a shutdown.
drivers/misc: (aspeed-lpc-snoop): Fix platform_get_irq() error
checking
drivers/char/ipmi/kcs_bmc_aspeed.c | 8 +++++
drivers/soc/aspeed/aspeed-lpc-snoop.c | 46 ++++++++++++++++++++++++++-
2 files changed, 53 insertions(+), 1 deletion(-)
--
2.25.1
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH 1/3] ipmi:kcs_bmc: Add cleanup regmap(interrupt-regs) on a shutdown. 2022-08-21 16:06 [PATCH 0/3] aspeed:lpc: Fix lpc-snoop probe exception Igor Kononenko @ 2022-08-21 15:54 ` Igor Kononenko 2022-08-21 19:46 ` kernel test robot 2022-08-22 6:35 ` [PATCH v2 " Igor Kononenko 2022-08-21 15:54 ` [PATCH 2/3] drivers/misc: (aspeed-lpc-snoop): Add regmap cleanup " Igor Kononenko ` (2 subsequent siblings) 3 siblings, 2 replies; 11+ messages in thread From: Igor Kononenko @ 2022-08-21 15:54 UTC (permalink / raw) To: linux-aspeed The bmc might be rebooted while the host is still reachable and the host might send requests through configured lpc-kcs channels. That leads to raise lpc-snoop interrupts that haven't adjusted IRQ handlers on next early kernel boot, because on the aspeed-chip reboot it might keep unclear registers on a state that is configured on the last boot. The described case get a `nobody cared` warning about more than 100.000 requests when another one driver configures the same shared IRQ(e.g. 35) and the interrupt will be passed through for lpc-kcs IRQ handler by `IRQ_NONE` state flag. The kernel output of described way is bellow: ``` [ 1.360110] irq 35: nobody cared (try booting with the "irqpoll" option) [ 1.360145] CPU: 0 PID: 1 Comm: swapper Not tainted 5.4.43-c109de3-24cc5b6 #1 [ 1.360158] Hardware name: Generic DT based system [ 1.360168] Backtrace: [ 1.360228] [<80107f5c>] (dump_backtrace) from [<80108184>] (show_stack+0x20/0x24) [ 1.360250] r7:00000023 r6:00000000 r5:00000000 r4:9d12d560 [ 1.360283] [<80108164>] (show_stack) from [<8084ae54>] (dump_stack+0x20/0x28) [ 1.360316] [<8084ae34>] (dump_stack) from [<80156790>] (__report_bad_irq+0x40/0xc0) [ 1.360344] [<80156750>] (__report_bad_irq) from [<801566c0>] (note_interrupt+0x238/0x290) [ 1.360366] r9:9d0ae000 r8:9d00c600 r7:00000023 r6:00000000 r5:00000000 r4:9d12d560 [ 1.360408] [<80156488>] (note_interrupt) from [<80153594>] (handle_irq_event+0xb4/0xc4) [ 1.360429] r10:00000000 r9:9d0ae000 r8:9d00c600 r7:00000001 r6:00000000 r5:00000000 [ 1.360440] r4:9d12d560 r3:00000000 [ 1.360466] [<801534e0>] (handle_irq_event) from [<8015788c>] (handle_level_irq+0xac/0x180) [ 1.360480] r5:80c7d35c r4:9d12d560 [ 1.360503] [<801577e0>] (handle_level_irq) from [<80152a5c>] (__handle_domain_irq+0x6c/0xc8) [ 1.360519] r5:80c7d35c r4:9d12d560 [ 1.360545] [<801529f0>] (__handle_domain_irq) from [<801021cc>] (avic_handle_irq+0x68/0x70) [ 1.360568] r9:9d0ae000 r8:9d12d608 r7:9d0afc84 r6:ffffffff r5:9d0afc50 r4:9d002380 [ 1.360587] [<80102164>] (avic_handle_irq) from [<80101a6c>] (__irq_svc+0x6c/0x90) [ 1.360603] Exception stack(0x9d0afc50 to 0x9d0afc98) [ 1.360620] fc40: 00000000 00000100 00000000 00000000 [ 1.360640] fc60: 9d12d560 98bbd0c0 00000000 00000023 9d12d608 60000053 00000000 9d0afcd4 [ 1.360657] fc80: 9d0afc80 9d0afca0 801570ec 80154cdc 40000053 ffffffff [ 1.360670] r5:40000053 r4:80154cdc [ 1.360693] [<801549e0>] (__setup_irq) from [<801555e4>] (request_threaded_irq+0xdc/0x15c) [ 1.360715] r9:98bbb340 r8:00000023 r7:9d12d570 r6:9d12d560 r5:00000000 r4:98bbd0c0 [ 1.360741] [<80155508>] (request_threaded_irq) from [<801589d8>] (devm_request_threaded_irq+0x70/0xc4) [ 1.360762] r10:80a7353c r9:00000000 r8:98bbb340 r7:9d130e10 r6:00000023 r5:804f4a70 [ 1.360774] r4:98bbd020 r3:00000080 [ 1.360800] [<80158968>] (devm_request_threaded_irq) from [<804f4ebc>] (aspeed_lpc_snoop_probe+0x100/0x2ac) [ 1.360821] r10:00000000 r9:9d130e10 r8:98bbd040 r7:00000000 r6:9d130e00 r5:98bbb340 [ 1.360830] r4:00000000 [ 1.360851] [<804f4dbc>] (aspeed_lpc_snoop_probe) from [<8056a5c4>] (platform_drv_probe+0x44/0x80) [ 1.360873] r9:80c5ef90 r8:00000000 r7:80cc2938 r6:00000000 r5:80c5ef90 r4:9d130e10 [ 1.360910] [<8056a580>] (platform_drv_probe) from [<80568420>] (really_probe+0x26c/0x498) [ 1.360924] r5:80cc282c r4:9d130e10 [ 1.360949] [<805681b4>] (really_probe) from [<80568c28>] (driver_probe_device+0x138/0x184) [ 1.360970] r10:80b0050c r9:80adadb0 r8:00000007 r7:80c5ef90 r6:9d130e10 r5:00000000 [ 1.360981] r4:80c5ef90 [ 1.361004] [<80568af0>] (driver_probe_device) from [<80568fe4>] (device_driver_attach+0xb8/0xc0) [ 1.361020] r7:80c5ef90 r6:9d130e54 r5:00000000 r4:9d130e10 [ 1.361046] [<80568f2c>] (device_driver_attach) from [<80569070>] (__driver_attach+0x84/0x16c) [ 1.361063] r7:80c61128 r6:9d130e10 r5:00000001 r4:80c5ef90 [ 1.361088] [<80568fec>] (__driver_attach) from [<80566068>] (bus_for_each_dev+0x84/0xcc) [ 1.361106] r7:80c61128 r6:80568fec r5:80c5ef90 r4:00000000 [ 1.361130] [<80565fe4>] (bus_for_each_dev) from [<80569180>] (driver_attach+0x28/0x30) [ 1.361147] r6:00000000 r5:9d1a3d40 r4:80c5ef90 [ 1.361169] [<80569158>] (driver_attach) from [<80566a08>] (bus_add_driver+0x114/0x200) [ 1.361195] [<805668f4>] (bus_add_driver) from [<80569814>] (driver_register+0x98/0x128) [ 1.361214] r7:00000000 r6:80ca0ca0 r5:00000000 r4:80c5ef90 [ 1.361241] [<8056977c>] (driver_register) from [<8056b528>] (__platform_driver_register+0x40/0x54) [ 1.361256] r5:000000b8 r4:80b2575c [ 1.361294] [<8056b4e8>] (__platform_driver_register) from [<80b2577c>] (aspeed_lpc_snoop_driver_init+0x20/0x28) [ 1.361331] [<80b2575c>] (aspeed_lpc_snoop_driver_init) from [<80b01318>] (do_one_initcall+0x84/0x184) [ 1.361356] [<80b01294>] (do_one_initcall) from [<80b01540>] (kernel_init_freeable+0x128/0x1ec) [ 1.361375] r7:80b3f858 r6:80ca0ca0 r5:000000b8 r4:80b61914 [ 1.361412] [<80b01418>] (kernel_init_freeable) from [<80864060>] (kernel_init+0x18/0x11c) [ 1.361435] r10:00000000 r9:00000000 r8:00000000 r7:00000000 r6:00000000 r5:80864048 [ 1.361444] r4:00000000 [ 1.361470] [<80864048>] (kernel_init) from [<801010e8>] (ret_from_fork+0x14/0x2c) [ 1.361483] Exception stack(0x9d0affb0 to 0x9d0afff8) [ 1.361500] ffa0: 00000000 00000000 00000000 00000000 [ 1.361518] ffc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 [ 1.361535] ffe0: 00000000 00000000 00000000 00000000 00000013 00000000 [ 1.361547] r5:80864048 r4:00000000 [ 1.361555] handlers: [ 1.361592] [<(ptrval)>] aspeed_lpc_snoop_irq [ 1.361609] Disabling IRQ #35 ``` Note: * The lpc-snoop driver found on the same 1e789080 address and have same IRQ#35 as for lpc-kcs * The lpc-snoop initizalied earlier than lpc-kcs. The patch brings a way to masking lpc-kcs interrupt all through a bmc-rebooting time. Tested on the YADRO VEGMAN N110 stand. Signed-off-by: Igor Kononenko <i.kononenko@yadro.com> --- drivers/char/ipmi/kcs_bmc_aspeed.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/drivers/char/ipmi/kcs_bmc_aspeed.c b/drivers/char/ipmi/kcs_bmc_aspeed.c index cdc88cde1e9a..440f31d96bd3 100644 --- a/drivers/char/ipmi/kcs_bmc_aspeed.c +++ b/drivers/char/ipmi/kcs_bmc_aspeed.c @@ -636,6 +636,13 @@ static int aspeed_kcs_remove(struct platform_device *pdev) return 0; } +static void aspeed_kcs_shutdown(struct platform_device *pdev) +{ + struct kcs_bmc *kcs_bmc = dev_get_drvdata(&pdev->dev); + + aspeed_kcs_irq_mask_update(kcs_bmc, (KCS_BMC_EVENT_TYPE_IBF), 0); +} + static const struct of_device_id ast_kcs_bmc_match[] = { { .compatible = "aspeed,ast2400-kcs-bmc-v2" }, { .compatible = "aspeed,ast2500-kcs-bmc-v2" }, @@ -651,6 +658,7 @@ static struct platform_driver ast_kcs_bmc_driver = { }, .probe = aspeed_kcs_probe, .remove = aspeed_kcs_remove, + .shutdown = aspeed_kcs_shutdown, }; module_platform_driver(ast_kcs_bmc_driver); -- 2.25.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 1/3] ipmi:kcs_bmc: Add cleanup regmap(interrupt-regs) on a shutdown. 2022-08-21 15:54 ` [PATCH 1/3] ipmi:kcs_bmc: Add cleanup regmap(interrupt-regs) on a shutdown Igor Kononenko @ 2022-08-21 19:46 ` kernel test robot 2022-08-22 6:35 ` [PATCH v2 " Igor Kononenko 1 sibling, 0 replies; 11+ messages in thread From: kernel test robot @ 2022-08-21 19:46 UTC (permalink / raw) To: linux-aspeed Hi Igor, Thank you for the patch! Yet something to improve: [auto build test ERROR on cminyard-ipmi/for-next] [also build test ERROR on soc/for-next linus/master v6.0-rc1 next-20220819] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Igor-Kononenko/aspeed-lpc-Fix-lpc-snoop-probe-exception/20220822-000836 base: https://github.com/cminyard/linux-ipmi for-next config: riscv-randconfig-r042-20220821 (https://download.01.org/0day-ci/archive/20220822/202208220317.cROZCTcB-lkp at intel.com/config) compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 01ffe31cbb54bfd8e38e71b3cf804a1d67ebf9c1) reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # install riscv cross compiling tool for clang build # apt-get install binutils-riscv64-linux-gnu # https://github.com/intel-lab-lkp/linux/commit/9c523bc00c11d0e9499bf6e3d3c5cc2fcf3fff8f git remote add linux-review https://github.com/intel-lab-lkp/linux git fetch --no-tags linux-review Igor-Kononenko/aspeed-lpc-Fix-lpc-snoop-probe-exception/20220822-000836 git checkout 9c523bc00c11d0e9499bf6e3d3c5cc2fcf3fff8f # save the config file mkdir build_dir && cp config build_dir/.config COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=riscv SHELL=/bin/bash If you fix the issue, kindly add following tag where applicable Reported-by: kernel test robot <lkp@intel.com> All errors (new ones prefixed by >>): In file included from drivers/char/ipmi/kcs_bmc_aspeed.c:10: In file included from include/linux/interrupt.h:11: In file included from include/linux/hardirq.h:11: In file included from ./arch/riscv/include/generated/asm/hardirq.h:1: In file included from include/asm-generic/hardirq.h:17: In file included from include/linux/irq.h:20: In file included from include/linux/io.h:13: In file included from arch/riscv/include/asm/io.h:136: include/asm-generic/io.h:464:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] val = __raw_readb(PCI_IOBASE + addr); ~~~~~~~~~~ ^ include/asm-generic/io.h:477:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr)); ~~~~~~~~~~ ^ include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu' #define __le16_to_cpu(x) ((__force __u16)(__le16)(x)) ^ In file included from drivers/char/ipmi/kcs_bmc_aspeed.c:10: In file included from include/linux/interrupt.h:11: In file included from include/linux/hardirq.h:11: In file included from ./arch/riscv/include/generated/asm/hardirq.h:1: In file included from include/asm-generic/hardirq.h:17: In file included from include/linux/irq.h:20: In file included from include/linux/io.h:13: In file included from arch/riscv/include/asm/io.h:136: include/asm-generic/io.h:490:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr)); ~~~~~~~~~~ ^ include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu' #define __le32_to_cpu(x) ((__force __u32)(__le32)(x)) ^ In file included from drivers/char/ipmi/kcs_bmc_aspeed.c:10: In file included from include/linux/interrupt.h:11: In file included from include/linux/hardirq.h:11: In file included from ./arch/riscv/include/generated/asm/hardirq.h:1: In file included from include/asm-generic/hardirq.h:17: In file included from include/linux/irq.h:20: In file included from include/linux/io.h:13: In file included from arch/riscv/include/asm/io.h:136: include/asm-generic/io.h:501:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] __raw_writeb(value, PCI_IOBASE + addr); ~~~~~~~~~~ ^ include/asm-generic/io.h:511:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr); ~~~~~~~~~~ ^ include/asm-generic/io.h:521:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr); ~~~~~~~~~~ ^ include/asm-generic/io.h:1024:55: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] return (port > MMIO_UPPER_LIMIT) ? NULL : PCI_IOBASE + port; ~~~~~~~~~~ ^ >> drivers/char/ipmi/kcs_bmc_aspeed.c:643:29: error: incompatible pointer types passing 'struct kcs_bmc *' to parameter of type 'struct kcs_bmc_device *' [-Werror,-Wincompatible-pointer-types] aspeed_kcs_irq_mask_update(kcs_bmc, (KCS_BMC_EVENT_TYPE_IBF), 0); ^~~~~~~ drivers/char/ipmi/kcs_bmc_aspeed.c:399:63: note: passing argument to parameter 'kcs_bmc' here static void aspeed_kcs_irq_mask_update(struct kcs_bmc_device *kcs_bmc, u8 mask, u8 state) ^ 7 warnings and 1 error generated. vim +643 drivers/char/ipmi/kcs_bmc_aspeed.c 638 639 static void aspeed_kcs_shutdown(struct platform_device *pdev) 640 { 641 struct kcs_bmc *kcs_bmc = dev_get_drvdata(&pdev->dev); 642 > 643 aspeed_kcs_irq_mask_update(kcs_bmc, (KCS_BMC_EVENT_TYPE_IBF), 0); 644 } 645 -- 0-DAY CI Kernel Test Service https://01.org/lkp ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 1/3] ipmi:kcs_bmc: Add cleanup regmap(interrupt-regs) on a shutdown. 2022-08-21 15:54 ` [PATCH 1/3] ipmi:kcs_bmc: Add cleanup regmap(interrupt-regs) on a shutdown Igor Kononenko 2022-08-21 19:46 ` kernel test robot @ 2022-08-22 6:35 ` Igor Kononenko 2022-08-23 0:22 ` Joel Stanley 1 sibling, 1 reply; 11+ messages in thread From: Igor Kononenko @ 2022-08-22 6:35 UTC (permalink / raw) To: linux-aspeed The previos v1 [PATCH 1/3] have error of getting `struct kcs_bmc_device` pointer from `struct platform_device *`. Replaced to retriveing pointer by `platform_get_drvdata()` Reported-by: kernel test robot <lkp@intel.com> Signed-off-by: Igor Kononenko <i.kononenko@yadro.com> --- drivers/char/ipmi/kcs_bmc_aspeed.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/drivers/char/ipmi/kcs_bmc_aspeed.c b/drivers/char/ipmi/kcs_bmc_aspeed.c index cdc88cde1e9a..8437f3cfe3f4 100644 --- a/drivers/char/ipmi/kcs_bmc_aspeed.c +++ b/drivers/char/ipmi/kcs_bmc_aspeed.c @@ -636,6 +636,14 @@ static int aspeed_kcs_remove(struct platform_device *pdev) return 0; } +static void aspeed_kcs_shutdown(struct platform_device *pdev) +{ + struct aspeed_kcs_bmc *priv = platform_get_drvdata(pdev); + struct kcs_bmc_device *kcs_bmc = &priv->kcs_bmc; + + aspeed_kcs_irq_mask_update(kcs_bmc, (KCS_BMC_EVENT_TYPE_IBF), 0); +} + static const struct of_device_id ast_kcs_bmc_match[] = { { .compatible = "aspeed,ast2400-kcs-bmc-v2" }, { .compatible = "aspeed,ast2500-kcs-bmc-v2" }, @@ -651,6 +659,7 @@ static struct platform_driver ast_kcs_bmc_driver = { }, .probe = aspeed_kcs_probe, .remove = aspeed_kcs_remove, + .shutdown = aspeed_kcs_shutdown, }; module_platform_driver(ast_kcs_bmc_driver); -- 2.25.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2 1/3] ipmi:kcs_bmc: Add cleanup regmap(interrupt-regs) on a shutdown. 2022-08-22 6:35 ` [PATCH v2 " Igor Kononenko @ 2022-08-23 0:22 ` Joel Stanley 2022-08-23 6:54 ` i.kononenko 0 siblings, 1 reply; 11+ messages in thread From: Joel Stanley @ 2022-08-23 0:22 UTC (permalink / raw) To: linux-aspeed On Mon, 22 Aug 2022 at 06:36, Igor Kononenko <i.kononenko@yadro.com> wrote: > > The previos v1 [PATCH 1/3] have error of getting `struct kcs_bmc_device` > pointer from `struct platform_device *`. Replaced to retriveing pointer by > `platform_get_drvdata()` Can we get a v3 with your original commit message added? You had a good write up in v1 but it was dropped in v2. This change looks like the right thing to do. We should get Andrew to review too, as he spends more time with the KCS controllers. The missed irq issue was seen with the other LPC sub drivers because the clock wasn't enabled. We ended up with this patch: https://lore.kernel.org/all/20201208091748.1920-1-wangzhiqiang.bj at bytedance.com/ Do we need to do something similar for KCS? > > Reported-by: kernel test robot <lkp@intel.com> > Signed-off-by: Igor Kononenko <i.kononenko@yadro.com> > --- > drivers/char/ipmi/kcs_bmc_aspeed.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/drivers/char/ipmi/kcs_bmc_aspeed.c b/drivers/char/ipmi/kcs_bmc_aspeed.c > index cdc88cde1e9a..8437f3cfe3f4 100644 > --- a/drivers/char/ipmi/kcs_bmc_aspeed.c > +++ b/drivers/char/ipmi/kcs_bmc_aspeed.c > @@ -636,6 +636,14 @@ static int aspeed_kcs_remove(struct platform_device *pdev) > return 0; > } > > +static void aspeed_kcs_shutdown(struct platform_device *pdev) > +{ > + struct aspeed_kcs_bmc *priv = platform_get_drvdata(pdev); > + struct kcs_bmc_device *kcs_bmc = &priv->kcs_bmc; > + > + aspeed_kcs_irq_mask_update(kcs_bmc, (KCS_BMC_EVENT_TYPE_IBF), 0); > +} > + > static const struct of_device_id ast_kcs_bmc_match[] = { > { .compatible = "aspeed,ast2400-kcs-bmc-v2" }, > { .compatible = "aspeed,ast2500-kcs-bmc-v2" }, > @@ -651,6 +659,7 @@ static struct platform_driver ast_kcs_bmc_driver = { > }, > .probe = aspeed_kcs_probe, > .remove = aspeed_kcs_remove, > + .shutdown = aspeed_kcs_shutdown, > }; > module_platform_driver(ast_kcs_bmc_driver); > > -- > 2.25.1 > ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 1/3] ipmi:kcs_bmc: Add cleanup regmap(interrupt-regs) on a shutdown. 2022-08-23 0:22 ` Joel Stanley @ 2022-08-23 6:54 ` i.kononenko 0 siblings, 0 replies; 11+ messages in thread From: i.kononenko @ 2022-08-23 6:54 UTC (permalink / raw) To: linux-aspeed On 23.08.2022 03:22, Joel Stanley wrote: > ?????????! ?????? ?????? ?? ???????? ????????!? > > On Mon, 22 Aug 2022 at 06:36, Igor Kononenko <i.kononenko@yadro.com> wrote: >> >> The previos v1 [PATCH 1/3] have error of getting `struct kcs_bmc_device` >> pointer from `struct platform_device *`. Replaced to retriveing pointer by >> `platform_get_drvdata()` > > Can we get a v3 with your original commit message added? You had a > good write up in v1 but it was dropped in v2. > Thanks for the review. Ok, I'll include the origin commit message to a v3. > This change looks like the right thing to do. We should get Andrew to > review too, as he spends more time with the KCS controllers. > > The missed irq issue was seen with the other LPC sub drivers because > the clock wasn't enabled. We ended up with this patch: > > https://lore.kernel.org/all/20201208091748.1920-1-wangzhiqiang.bj at bytedance.com/ >> Do we need to do something similar for KCS? As far as I see by the LPC 'nobody cared irq' issue there had the feature of enabling LCLK individually earlier, there is patch about: https://lore.kernel.org/all/20211101233751.49222-5-jae.hyun.yoo at intel.com/ Originally I found the bug on the linux-dev.v5.4 that includes the 'LCLK individually enabling' feature. It seems to me the issue is that lpc-snoop and the lpc-kcs has same IRQ#35 that is used in separated drivers(by the IRQF_SHARED flag). The IRQ handler determinate request purpose(for kcs or snoop) by LPC interrupt registers state, and if such interrupt is not for any one of them, the irq-handler passthrough request to a next handler by returning `IRQ_NONE`. So, even if lpc-kcs will be having adjusted own individual LCLK, that is doesn't solve issue, because when lpc-snoop will had configured irq-handler the irq-manager will know that for IRQ#35 already registered a good handler, but such handler will skip all requests by `IRQ_NONE` because such irqs are intended for lpc-kcs. I guess that is the point of bug. > >> >> Reported-by: kernel test robot <lkp@intel.com> >> Signed-off-by: Igor Kononenko <i.kononenko@yadro.com> >> --- >> drivers/char/ipmi/kcs_bmc_aspeed.c | 9 +++++++++ >> 1 file changed, 9 insertions(+) >> >> diff --git a/drivers/char/ipmi/kcs_bmc_aspeed.c b/drivers/char/ipmi/kcs_bmc_aspeed.c >> index cdc88cde1e9a..8437f3cfe3f4 100644 >> --- a/drivers/char/ipmi/kcs_bmc_aspeed.c >> +++ b/drivers/char/ipmi/kcs_bmc_aspeed.c >> @@ -636,6 +636,14 @@ static int aspeed_kcs_remove(struct platform_device *pdev) >> return 0; >> } >> >> +static void aspeed_kcs_shutdown(struct platform_device *pdev) >> +{ >> + struct aspeed_kcs_bmc *priv = platform_get_drvdata(pdev); >> + struct kcs_bmc_device *kcs_bmc = &priv->kcs_bmc; >> + >> + aspeed_kcs_irq_mask_update(kcs_bmc, (KCS_BMC_EVENT_TYPE_IBF), 0); >> +} >> + >> static const struct of_device_id ast_kcs_bmc_match[] = { >> { .compatible = "aspeed,ast2400-kcs-bmc-v2" }, >> { .compatible = "aspeed,ast2500-kcs-bmc-v2" }, >> @@ -651,6 +659,7 @@ static struct platform_driver ast_kcs_bmc_driver = { >> }, >> .probe = aspeed_kcs_probe, >> .remove = aspeed_kcs_remove, >> + .shutdown = aspeed_kcs_shutdown, >> }; >> module_platform_driver(ast_kcs_bmc_driver); >> >> -- >> 2.25.1 >> -- Best regards, Igor Kononenko ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/3] drivers/misc: (aspeed-lpc-snoop): Add regmap cleanup on a shutdown. 2022-08-21 16:06 [PATCH 0/3] aspeed:lpc: Fix lpc-snoop probe exception Igor Kononenko 2022-08-21 15:54 ` [PATCH 1/3] ipmi:kcs_bmc: Add cleanup regmap(interrupt-regs) on a shutdown Igor Kononenko @ 2022-08-21 15:54 ` Igor Kononenko 2022-08-23 0:30 ` Joel Stanley 2022-08-21 15:54 ` [PATCH 3/3] drivers/misc: (aspeed-lpc-snoop): Fix platform_get_irq() error checking Igor Kononenko 2022-08-23 0:22 ` [PATCH 0/3] aspeed:lpc: Fix lpc-snoop probe exception Joel Stanley 3 siblings, 1 reply; 11+ messages in thread From: Igor Kononenko @ 2022-08-21 15:54 UTC (permalink / raw) To: linux-aspeed The bmc might be rebooted while the host is still reachable and the host might send requests through configured lpc-kcs channels in same time. That leads to raise lpc-snoop interrupts that haven't adjusted IRQ handlers on next early kernel boot, because on the aspeed-chip reboot might keep registers on a unclean state that is configured on the last boot. The patch brings an way to masking lpc-snoop interrupts all through a bmc-rebooting time. Tested on a YADRO VEGMAN N110 stand. Signed-off-by: Igor Kononenko <i.kononenko@yadro.com> --- drivers/soc/aspeed/aspeed-lpc-snoop.c | 44 +++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/drivers/soc/aspeed/aspeed-lpc-snoop.c b/drivers/soc/aspeed/aspeed-lpc-snoop.c index eceeaf8dfbeb..6ec47bf1dc6b 100644 --- a/drivers/soc/aspeed/aspeed-lpc-snoop.c +++ b/drivers/soc/aspeed/aspeed-lpc-snoop.c @@ -235,6 +235,41 @@ static int aspeed_lpc_enable_snoop(struct aspeed_lpc_snoop *lpc_snoop, return rc; } +static void aspeed_lpc_reset_regmap(struct aspeed_lpc_snoop *lpc_snoop) +{ + u8 index; + struct lpc_regman_cleanup { + u32 offset; + u32 mask; + u8 val; + }; + static struct lpc_regman_cleanup cleanup_list[] = { + // Prevent handling ENINIT_SNPxW + { .offset = HICR5, + .mask = HICR5_EN_SNP0W | HICR5_ENINT_SNP0W, + .val = 0 }, + { .offset = HICR5, + .mask = HICR5_EN_SNP1W | HICR5_ENINT_SNP1W, + .val = 0 }, + { .offset = HICRB, + .mask = HICRB_ENSNP0D | HICRB_ENSNP1D, + .val = 0 }, + // Reset SNOOP channel IRQ status + { .offset = HICR6, + .mask = HICR6_STR_SNP0W | HICR6_STR_SNP1W, + .val = 1 }, + }; + for (index = 0; index < ARRAY_SIZE(cleanup_list); index++) { + u32 val = 0; + + if (cleanup_list[index].val) + val = cleanup_list[index].mask; + regmap_update_bits(lpc_snoop->regmap, + cleanup_list[index].offset, + cleanup_list[index].mask, val); + } +} + static void aspeed_lpc_disable_snoop(struct aspeed_lpc_snoop *lpc_snoop, int channel) { @@ -285,6 +320,7 @@ static int aspeed_lpc_snoop_probe(struct platform_device *pdev) return -ENODEV; } + aspeed_lpc_reset_regmap(lpc_snoop); dev_set_drvdata(&pdev->dev, lpc_snoop); rc = of_property_read_u32_index(dev->of_node, "snoop-ports", 0, &port); @@ -345,6 +381,13 @@ static int aspeed_lpc_snoop_remove(struct platform_device *pdev) return 0; } +static void aspeed_lpc_snoop_shutdown(struct platform_device *pdev) +{ + struct aspeed_lpc_snoop *lpc_snoop = dev_get_drvdata(&pdev->dev); + + aspeed_lpc_reset_regmap(lpc_snoop); +} + static const struct aspeed_lpc_snoop_model_data ast2400_model_data = { .has_hicrb_ensnp = 0, }; @@ -370,6 +413,7 @@ static struct platform_driver aspeed_lpc_snoop_driver = { }, .probe = aspeed_lpc_snoop_probe, .remove = aspeed_lpc_snoop_remove, + .shutdown = aspeed_lpc_snoop_shutdown, }; module_platform_driver(aspeed_lpc_snoop_driver); -- 2.25.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/3] drivers/misc: (aspeed-lpc-snoop): Add regmap cleanup on a shutdown. 2022-08-21 15:54 ` [PATCH 2/3] drivers/misc: (aspeed-lpc-snoop): Add regmap cleanup " Igor Kononenko @ 2022-08-23 0:30 ` Joel Stanley 2022-08-23 7:02 ` i.kononenko 0 siblings, 1 reply; 11+ messages in thread From: Joel Stanley @ 2022-08-23 0:30 UTC (permalink / raw) To: linux-aspeed On Sun, 21 Aug 2022 at 15:54, Igor Kononenko <i.kononenko@yadro.com> wrote: > > The bmc might be rebooted while the host is still reachable and the host > might send requests through configured lpc-kcs channels in same time. > That leads to raise lpc-snoop interrupts that haven't adjusted IRQ > handlers on next early kernel boot, because on the aspeed-chip reboot > might keep registers on a unclean state that is configured on the last > boot. > > The patch brings an way to masking lpc-snoop interrupts all through > a bmc-rebooting time. > > Tested on a YADRO VEGMAN N110 stand. > > Signed-off-by: Igor Kononenko <i.kononenko@yadro.com> > --- > drivers/soc/aspeed/aspeed-lpc-snoop.c | 44 +++++++++++++++++++++++++++ > 1 file changed, 44 insertions(+) > > diff --git a/drivers/soc/aspeed/aspeed-lpc-snoop.c b/drivers/soc/aspeed/aspeed-lpc-snoop.c > index eceeaf8dfbeb..6ec47bf1dc6b 100644 > --- a/drivers/soc/aspeed/aspeed-lpc-snoop.c > +++ b/drivers/soc/aspeed/aspeed-lpc-snoop.c > @@ -235,6 +235,41 @@ static int aspeed_lpc_enable_snoop(struct aspeed_lpc_snoop *lpc_snoop, > return rc; > } > > +static void aspeed_lpc_reset_regmap(struct aspeed_lpc_snoop *lpc_snoop) > +{ > + u8 index; > + struct lpc_regman_cleanup { > + u32 offset; > + u32 mask; > + u8 val; > + }; > + static struct lpc_regman_cleanup cleanup_list[] = { > + // Prevent handling ENINIT_SNPxW > + { .offset = HICR5, > + .mask = HICR5_EN_SNP0W | HICR5_ENINT_SNP0W, > + .val = 0 }, > + { .offset = HICR5, > + .mask = HICR5_EN_SNP1W | HICR5_ENINT_SNP1W, > + .val = 0 }, > + { .offset = HICRB, > + .mask = HICRB_ENSNP0D | HICRB_ENSNP1D, > + .val = 0 }, > + // Reset SNOOP channel IRQ status > + { .offset = HICR6, > + .mask = HICR6_STR_SNP0W | HICR6_STR_SNP1W, > + .val = 1 }, > + }; > + for (index = 0; index < ARRAY_SIZE(cleanup_list); index++) { Did you consider open coding the various updates instead of using a for loop? I don't think the for loop help us here. You could instead make these three updates: /* Prevent handling ENINIT_SNPxW */ regmap_clear_bits(lpc_snoop->regmap, HICR5, HICR5_EN_SNP0W | HICR5_ENINT_SNP0W | HICR5_EN_SNP1W | HICR5_ENINT_SNP1W); regmap_clear_bits(lpc_snoop->regmap, HICRB, HICRB_ENSNP0D | HICRB_ENSNP1D); /* Reset SNOOP channel IRQ status */ regmap_set_bits(lpc_snoop->regmap, HICR6, HICR6_STR_SNP0W | HICR6_STR_SNP1W); > + u32 val = 0; > + > + if (cleanup_list[index].val) > + val = cleanup_list[index].mask; > + regmap_update_bits(lpc_snoop->regmap, > + cleanup_list[index].offset, > + cleanup_list[index].mask, val); > + } > +} > + > static void aspeed_lpc_disable_snoop(struct aspeed_lpc_snoop *lpc_snoop, > int channel) > { > @@ -285,6 +320,7 @@ static int aspeed_lpc_snoop_probe(struct platform_device *pdev) > return -ENODEV; > } > > + aspeed_lpc_reset_regmap(lpc_snoop); > dev_set_drvdata(&pdev->dev, lpc_snoop); > > rc = of_property_read_u32_index(dev->of_node, "snoop-ports", 0, &port); > @@ -345,6 +381,13 @@ static int aspeed_lpc_snoop_remove(struct platform_device *pdev) > return 0; > } > > +static void aspeed_lpc_snoop_shutdown(struct platform_device *pdev) > +{ > + struct aspeed_lpc_snoop *lpc_snoop = dev_get_drvdata(&pdev->dev); > + > + aspeed_lpc_reset_regmap(lpc_snoop); > +} > + > static const struct aspeed_lpc_snoop_model_data ast2400_model_data = { > .has_hicrb_ensnp = 0, > }; > @@ -370,6 +413,7 @@ static struct platform_driver aspeed_lpc_snoop_driver = { > }, > .probe = aspeed_lpc_snoop_probe, > .remove = aspeed_lpc_snoop_remove, > + .shutdown = aspeed_lpc_snoop_shutdown, > }; > > module_platform_driver(aspeed_lpc_snoop_driver); > -- > 2.25.1 > ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/3] drivers/misc: (aspeed-lpc-snoop): Add regmap cleanup on a shutdown. 2022-08-23 0:30 ` Joel Stanley @ 2022-08-23 7:02 ` i.kononenko 0 siblings, 0 replies; 11+ messages in thread From: i.kononenko @ 2022-08-23 7:02 UTC (permalink / raw) To: linux-aspeed On 23.08.2022 03:30, Joel Stanley wrote: > ?????????! ?????? ?????? ?? ???????? ????????!? > > On Sun, 21 Aug 2022 at 15:54, Igor Kononenko <i.kononenko@yadro.com> wrote: >> >> The bmc might be rebooted while the host is still reachable and the host >> might send requests through configured lpc-kcs channels in same time. >> That leads to raise lpc-snoop interrupts that haven't adjusted IRQ >> handlers on next early kernel boot, because on the aspeed-chip reboot >> might keep registers on a unclean state that is configured on the last >> boot. >> >> The patch brings an way to masking lpc-snoop interrupts all through >> a bmc-rebooting time. >> >> Tested on a YADRO VEGMAN N110 stand. >> >> Signed-off-by: Igor Kononenko <i.kononenko@yadro.com> >> --- >> drivers/soc/aspeed/aspeed-lpc-snoop.c | 44 +++++++++++++++++++++++++++ >> 1 file changed, 44 insertions(+) >> >> diff --git a/drivers/soc/aspeed/aspeed-lpc-snoop.c b/drivers/soc/aspeed/aspeed-lpc-snoop.c >> index eceeaf8dfbeb..6ec47bf1dc6b 100644 >> --- a/drivers/soc/aspeed/aspeed-lpc-snoop.c >> +++ b/drivers/soc/aspeed/aspeed-lpc-snoop.c >> @@ -235,6 +235,41 @@ static int aspeed_lpc_enable_snoop(struct aspeed_lpc_snoop *lpc_snoop, >> return rc; >> } >> >> +static void aspeed_lpc_reset_regmap(struct aspeed_lpc_snoop *lpc_snoop) >> +{ >> + u8 index; >> + struct lpc_regman_cleanup { >> + u32 offset; >> + u32 mask; >> + u8 val; >> + }; >> + static struct lpc_regman_cleanup cleanup_list[] = { >> + // Prevent handling ENINIT_SNPxW >> + { .offset = HICR5, >> + .mask = HICR5_EN_SNP0W | HICR5_ENINT_SNP0W, >> + .val = 0 }, >> + { .offset = HICR5, >> + .mask = HICR5_EN_SNP1W | HICR5_ENINT_SNP1W, >> + .val = 0 }, >> + { .offset = HICRB, >> + .mask = HICRB_ENSNP0D | HICRB_ENSNP1D, >> + .val = 0 }, >> + // Reset SNOOP channel IRQ status >> + { .offset = HICR6, >> + .mask = HICR6_STR_SNP0W | HICR6_STR_SNP1W, >> + .val = 1 }, >> + }; >> + for (index = 0; index < ARRAY_SIZE(cleanup_list); index++) { > > Did you consider open coding the various updates instead of using a > for loop? I don't think the for loop help us here. > > You could instead make these three updates: > > /* Prevent handling ENINIT_SNPxW */ > regmap_clear_bits(lpc_snoop->regmap, HICR5, > HICR5_EN_SNP0W | HICR5_ENINT_SNP0W | > HICR5_EN_SNP1W | HICR5_ENINT_SNP1W); > > regmap_clear_bits(lpc_snoop->regmap, HICRB, > HICRB_ENSNP0D | HICRB_ENSNP1D); > > /* Reset SNOOP channel IRQ status */ > regmap_set_bits(lpc_snoop->regmap, HICR6, > HICR6_STR_SNP0W | HICR6_STR_SNP1W); > > Thanks! I'll take a notice for the further patches. Will fix in a v3 patchset. > >> + u32 val = 0; >> + >> + if (cleanup_list[index].val) >> + val = cleanup_list[index].mask; >> + regmap_update_bits(lpc_snoop->regmap, >> + cleanup_list[index].offset, >> + cleanup_list[index].mask, val); >> + } >> +} >> + >> static void aspeed_lpc_disable_snoop(struct aspeed_lpc_snoop *lpc_snoop, >> int channel) >> { >> @@ -285,6 +320,7 @@ static int aspeed_lpc_snoop_probe(struct platform_device *pdev) >> return -ENODEV; >> } >> >> + aspeed_lpc_reset_regmap(lpc_snoop); >> dev_set_drvdata(&pdev->dev, lpc_snoop); >> >> rc = of_property_read_u32_index(dev->of_node, "snoop-ports", 0, &port); >> @@ -345,6 +381,13 @@ static int aspeed_lpc_snoop_remove(struct platform_device *pdev) >> return 0; >> } >> >> +static void aspeed_lpc_snoop_shutdown(struct platform_device *pdev) >> +{ >> + struct aspeed_lpc_snoop *lpc_snoop = dev_get_drvdata(&pdev->dev); >> + >> + aspeed_lpc_reset_regmap(lpc_snoop); >> +} >> + >> static const struct aspeed_lpc_snoop_model_data ast2400_model_data = { >> .has_hicrb_ensnp = 0, >> }; >> @@ -370,6 +413,7 @@ static struct platform_driver aspeed_lpc_snoop_driver = { >> }, >> .probe = aspeed_lpc_snoop_probe, >> .remove = aspeed_lpc_snoop_remove, >> + .shutdown = aspeed_lpc_snoop_shutdown, >> }; >> >> module_platform_driver(aspeed_lpc_snoop_driver); >> -- >> 2.25.1 >> -- Best regards, Igor Kononenko ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 3/3] drivers/misc: (aspeed-lpc-snoop): Fix platform_get_irq() error checking 2022-08-21 16:06 [PATCH 0/3] aspeed:lpc: Fix lpc-snoop probe exception Igor Kononenko 2022-08-21 15:54 ` [PATCH 1/3] ipmi:kcs_bmc: Add cleanup regmap(interrupt-regs) on a shutdown Igor Kononenko 2022-08-21 15:54 ` [PATCH 2/3] drivers/misc: (aspeed-lpc-snoop): Add regmap cleanup " Igor Kononenko @ 2022-08-21 15:54 ` Igor Kononenko 2022-08-23 0:22 ` [PATCH 0/3] aspeed:lpc: Fix lpc-snoop probe exception Joel Stanley 3 siblings, 0 replies; 11+ messages in thread From: Igor Kononenko @ 2022-08-21 15:54 UTC (permalink / raw) To: linux-aspeed The `platform_get_irq` return negative code that is indicate an error. So `!lpc_snoop->irq` leads to misscought a failure. This patch brings fix of checking negative `rc`. Signed-off-by: Igor Kononenko <i.kononenko@yadro.com> --- drivers/soc/aspeed/aspeed-lpc-snoop.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/soc/aspeed/aspeed-lpc-snoop.c b/drivers/soc/aspeed/aspeed-lpc-snoop.c index 6ec47bf1dc6b..25731cf0342d 100644 --- a/drivers/soc/aspeed/aspeed-lpc-snoop.c +++ b/drivers/soc/aspeed/aspeed-lpc-snoop.c @@ -167,7 +167,7 @@ static int aspeed_lpc_snoop_config_irq(struct aspeed_lpc_snoop *lpc_snoop, int rc; lpc_snoop->irq = platform_get_irq(pdev, 0); - if (!lpc_snoop->irq) + if (lpc_snoop->irq < 0) return -ENODEV; rc = devm_request_irq(dev, lpc_snoop->irq, -- 2.25.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 0/3] aspeed:lpc: Fix lpc-snoop probe exception 2022-08-21 16:06 [PATCH 0/3] aspeed:lpc: Fix lpc-snoop probe exception Igor Kononenko ` (2 preceding siblings ...) 2022-08-21 15:54 ` [PATCH 3/3] drivers/misc: (aspeed-lpc-snoop): Fix platform_get_irq() error checking Igor Kononenko @ 2022-08-23 0:22 ` Joel Stanley 3 siblings, 0 replies; 11+ messages in thread From: Joel Stanley @ 2022-08-23 0:22 UTC (permalink / raw) To: linux-aspeed On Sun, 21 Aug 2022 at 16:07, Igor Kononenko <i.kononenko@yadro.com> wrote: > > The bmc might be rebooted while the host is still reachable and the host > might send requests through configured lpc-kcs channels in same time. > That leads to raise lpc-snoop/lpc-kcs interrupts that haven't adjusted IRQ > handlers yet on next early kernel boot, because on the aspeed-chip reboot > might keep lpc-registers on a unclean state that is configured on the last > boot. Thanks for the bug report and the patches Igor. When you resend then please add a Fixes: line to help ensure the patches get backported. Cheers, Joel > > The described way might raise the next exception: > ``` > [ 1.360110] irq 35: nobody cared (try booting with the "irqpoll" option) > [ 1.360145] CPU: 0 PID: 1 Comm: swapper Not tainted 5.4.43-c109de3-24cc5b6 #1 > [ 1.360158] Hardware name: Generic DT based system > [ 1.360168] Backtrace: > [ 1.360228] [<80107f5c>] (dump_backtrace) from [<80108184>] (show_stack+0x20/0x24) > [ 1.360250] r7:00000023 r6:00000000 r5:00000000 r4:9d12d560 > [ 1.360283] [<80108164>] (show_stack) from [<8084ae54>] (dump_stack+0x20/0x28) > [ 1.360316] [<8084ae34>] (dump_stack) from [<80156790>] (__report_bad_irq+0x40/0xc0) > [ 1.360344] [<80156750>] (__report_bad_irq) from [<801566c0>] (note_interrupt+0x238/0x290) > [ 1.360366] r9:9d0ae000 r8:9d00c600 r7:00000023 r6:00000000 r5:00000000 r4:9d12d560 > [ 1.360408] [<80156488>] (note_interrupt) from [<80153594>] (handle_irq_event+0xb4/0xc4) > [ 1.360429] r10:00000000 r9:9d0ae000 r8:9d00c600 r7:00000001 r6:00000000 r5:00000000 > [ 1.360440] r4:9d12d560 r3:00000000 > [ 1.360466] [<801534e0>] (handle_irq_event) from [<8015788c>] (handle_level_irq+0xac/0x180) > [ 1.360480] r5:80c7d35c r4:9d12d560 > [ 1.360503] [<801577e0>] (handle_level_irq) from [<80152a5c>] (__handle_domain_irq+0x6c/0xc8) > [ 1.360519] r5:80c7d35c r4:9d12d560 > [ 1.360545] [<801529f0>] (__handle_domain_irq) from [<801021cc>] (avic_handle_irq+0x68/0x70) > [ 1.360568] r9:9d0ae000 r8:9d12d608 r7:9d0afc84 r6:ffffffff r5:9d0afc50 r4:9d002380 > [ 1.360587] [<80102164>] (avic_handle_irq) from [<80101a6c>] (__irq_svc+0x6c/0x90) > [ 1.360603] Exception stack(0x9d0afc50 to 0x9d0afc98) > [ 1.360620] fc40: 00000000 00000100 00000000 00000000 > [ 1.360640] fc60: 9d12d560 98bbd0c0 00000000 00000023 9d12d608 60000053 00000000 9d0afcd4 > [ 1.360657] fc80: 9d0afc80 9d0afca0 801570ec 80154cdc 40000053 ffffffff > [ 1.360670] r5:40000053 r4:80154cdc > [ 1.360693] [<801549e0>] (__setup_irq) from [<801555e4>] (request_threaded_irq+0xdc/0x15c) > [ 1.360715] r9:98bbb340 r8:00000023 r7:9d12d570 r6:9d12d560 r5:00000000 r4:98bbd0c0 > [ 1.360741] [<80155508>] (request_threaded_irq) from [<801589d8>] (devm_request_threaded_irq+0x70/0xc4) > [ 1.360762] r10:80a7353c r9:00000000 r8:98bbb340 r7:9d130e10 r6:00000023 r5:804f4a70 > [ 1.360774] r4:98bbd020 r3:00000080 > [ 1.360800] [<80158968>] (devm_request_threaded_irq) from [<804f4ebc>] (aspeed_lpc_snoop_probe+0x100/0x2ac) > [ 1.360821] r10:00000000 r9:9d130e10 r8:98bbd040 r7:00000000 r6:9d130e00 r5:98bbb340 > [ 1.360830] r4:00000000 > [ 1.360851] [<804f4dbc>] (aspeed_lpc_snoop_probe) from [<8056a5c4>] (platform_drv_probe+0x44/0x80) > [ 1.360873] r9:80c5ef90 r8:00000000 r7:80cc2938 r6:00000000 r5:80c5ef90 r4:9d130e10 > [ 1.360910] [<8056a580>] (platform_drv_probe) from [<80568420>] (really_probe+0x26c/0x498) > [ 1.360924] r5:80cc282c r4:9d130e10 > [ 1.360949] [<805681b4>] (really_probe) from [<80568c28>] (driver_probe_device+0x138/0x184) > [ 1.360970] r10:80b0050c r9:80adadb0 r8:00000007 r7:80c5ef90 r6:9d130e10 r5:00000000 > [ 1.360981] r4:80c5ef90 > [ 1.361004] [<80568af0>] (driver_probe_device) from [<80568fe4>] (device_driver_attach+0xb8/0xc0) > [ 1.361020] r7:80c5ef90 r6:9d130e54 r5:00000000 r4:9d130e10 > [ 1.361046] [<80568f2c>] (device_driver_attach) from [<80569070>] (__driver_attach+0x84/0x16c) > [ 1.361063] r7:80c61128 r6:9d130e10 r5:00000001 r4:80c5ef90 > [ 1.361088] [<80568fec>] (__driver_attach) from [<80566068>] (bus_for_each_dev+0x84/0xcc) > [ 1.361106] r7:80c61128 r6:80568fec r5:80c5ef90 r4:00000000 > [ 1.361130] [<80565fe4>] (bus_for_each_dev) from [<80569180>] (driver_attach+0x28/0x30) > [ 1.361147] r6:00000000 r5:9d1a3d40 r4:80c5ef90 > [ 1.361169] [<80569158>] (driver_attach) from [<80566a08>] (bus_add_driver+0x114/0x200) > [ 1.361195] [<805668f4>] (bus_add_driver) from [<80569814>] (driver_register+0x98/0x128) > [ 1.361214] r7:00000000 r6:80ca0ca0 r5:00000000 r4:80c5ef90 > [ 1.361241] [<8056977c>] (driver_register) from [<8056b528>] (__platform_driver_register+0x40/0x54) > [ 1.361256] r5:000000b8 r4:80b2575c > [ 1.361294] [<8056b4e8>] (__platform_driver_register) from [<80b2577c>] (aspeed_lpc_snoop_driver_init+0x20/0x28) > [ 1.361331] [<80b2575c>] (aspeed_lpc_snoop_driver_init) from [<80b01318>] (do_one_initcall+0x84/0x184) > [ 1.361356] [<80b01294>] (do_one_initcall) from [<80b01540>] (kernel_init_freeable+0x128/0x1ec) > [ 1.361375] r7:80b3f858 r6:80ca0ca0 r5:000000b8 r4:80b61914 > [ 1.361412] [<80b01418>] (kernel_init_freeable) from [<80864060>] (kernel_init+0x18/0x11c) > [ 1.361435] r10:00000000 r9:00000000 r8:00000000 r7:00000000 r6:00000000 r5:80864048 > [ 1.361444] r4:00000000 > [ 1.361470] [<80864048>] (kernel_init) from [<801010e8>] (ret_from_fork+0x14/0x2c) > [ 1.361483] Exception stack(0x9d0affb0 to 0x9d0afff8) > [ 1.361500] ffa0: 00000000 00000000 00000000 00000000 > [ 1.361518] ffc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 > [ 1.361535] ffe0: 00000000 00000000 00000000 00000000 00000013 00000000 > [ 1.361547] r5:80864048 r4:00000000 > [ 1.361555] handlers: > [ 1.361592] [<(ptrval)>] aspeed_lpc_snoop_irq > [ 1.361609] Disabling IRQ #35 > > ``` > > Main caught of that state is the lpc-snoop driver found on the same > 1e789080 address and have same IRQ#35 as for lpc-kcs, and lpc-snoop > registering earlier than lpc-kcs. So, on the lpc-snoop initialization > the lpc-snoop IRQ will be registried for IRQ#35, but it will > passthrough it for the LPC KCS handlers that is not registried yet. > > Summary we got the `nobody cared` warning about more 100.000 unhandled > IRQ#35 > > Steps to reproduce: > * Turn-on BMC > * Turn-on HOST > * Run `watch 'ipmitool sensor'` on the host that is configured to pass > requests via kcs-channel(3,4) > * Reboot BMC. > * On the next BMC boot kernel(BMC) dmesg have exception mentioned above. > > The following patchset aims to fixup described issue. > > Igor Kononenko (3): > ipmi:kcs_bmc: Add cleanup regmap(interrupt-regs) on a shutdown. > drivers/misc: (aspeed-lpc-snoop): Add regmap cleanup on a shutdown. > drivers/misc: (aspeed-lpc-snoop): Fix platform_get_irq() error > checking > > drivers/char/ipmi/kcs_bmc_aspeed.c | 8 +++++ > drivers/soc/aspeed/aspeed-lpc-snoop.c | 46 ++++++++++++++++++++++++++- > 2 files changed, 53 insertions(+), 1 deletion(-) > > -- > 2.25.1 > ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2022-08-23 7:02 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-08-21 16:06 [PATCH 0/3] aspeed:lpc: Fix lpc-snoop probe exception Igor Kononenko 2022-08-21 15:54 ` [PATCH 1/3] ipmi:kcs_bmc: Add cleanup regmap(interrupt-regs) on a shutdown Igor Kononenko 2022-08-21 19:46 ` kernel test robot 2022-08-22 6:35 ` [PATCH v2 " Igor Kononenko 2022-08-23 0:22 ` Joel Stanley 2022-08-23 6:54 ` i.kononenko 2022-08-21 15:54 ` [PATCH 2/3] drivers/misc: (aspeed-lpc-snoop): Add regmap cleanup " Igor Kononenko 2022-08-23 0:30 ` Joel Stanley 2022-08-23 7:02 ` i.kononenko 2022-08-21 15:54 ` [PATCH 3/3] drivers/misc: (aspeed-lpc-snoop): Fix platform_get_irq() error checking Igor Kononenko 2022-08-23 0:22 ` [PATCH 0/3] aspeed:lpc: Fix lpc-snoop probe exception Joel Stanley
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).