On Wed, Jan 19, 2022 at 9:43 PM Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote: > > On Wed, Jan 19, 2022 at 02:40:11PM +0800, Shuai Xue wrote: > > [+to Rafael, question about HEST/GHES/SDEI init] > > > > Hi, Bjorn, > > > > Thank you for your comments and quick reply. > > > > 在 2022/1/19 AM6:49, Bjorn Helgaas 写道: > > > On Sun, Jan 16, 2022 at 04:43:10PM +0800, Shuai Xue wrote: > > >> On an ACPI system, ACPI is initialised very early from a > > >> subsys_initcall(), while SDEI is not ready until a > > >> subsys_initcall_sync(). This patch is to reduce the time before GHES > > >> initialization. > > >> > > >> The SDEI driver provides functions (e.g. apei_sdei_register_ghes(), > > >> apei_sdei_unregister_ghes()) to register or unregister event callback > > >> for dispatcher in firmware. When the GHES driver probing, it registers > > >> the corresponding callback according to the notification type specified > > >> by GHES. If the GHES notification type is SDEI, the GHES driver will > > >> call apei_sdei_register_ghes() to register event call. > > >> > > >> When the firmware emits an event, it migrates the handling of the event > > >> into the kernel at the registered entry-point __sdei_asm_handler. And > > >> finally, the kernel will call the registered event callback and return > > >> status_code to indicate the status of event handling. SDEI_EV_FAILED > > >> indicates that the kernel failed to handle the event. > > >> > > >> Consequently, when an error occurs during kernel booting, the kernel is > > >> unable to handle and report errors until the GHES driver is initialized > > >> by device_initcall(), in which the event callback is registered. For > > >> example, when the kernel booting, the console logs many times from > > >> firmware before GHES drivers init in our platform: > > >> > > >> Trip in MM PCIe RAS handle(Intr:910) > > >> Clean PE[1.1.1] ERR_STS:0x4000100 -> 0 INT_STS:F0000000 > > >> Find RP(98:1.0) > > >> --Walk dev(98:1.0) CE:0 UCE:4000 > > >> ... > > >> ERROR: sdei_dispatch_event(32a) ret:-1 > > >> --handler(910) end > > > > > > If I understand correctly, the firmware noticed an error, tried to > > > report it to the kernel, and is complaining because the kernel isn't > > > ready to handle it yet. And the reason for this patch is to reduce > > > these complaints from the firmware. > > > > My thoughts exactly :) > > > > > That doesn't seem like a very good reason for this patch. There is > > > *always* a window before the kernel is ready to handle events from the > > > firmware. > > > > Yes, there is always a window. But if we could do better in kernel that > > reduces the window by 90% (from 33 seconds to 3 second), why not? > > > > > Why is the firmware noticing these errors in the first place? If > > > you're seeing these complaints regularly, my guess is that either you > > > have some terrible hardware or (more likely) the firmware isn't > > > clearing some expected error condition correctly. For example, maybe > > > the Unsupported Request errors that happen while enumerating PCIe > > > devices are being reported. > > > > > > If you register the callback function, the kernel will now start > > > seeing these error reports. What happens then? Does the kernel log > > > the errors somewhere? Is that better than the current situation where > > > the firmware logs them? > > > > Yep, it is a hardware issue. The firmware only logs in console > > (ttyAMA0) and we can not see it in kernel side. After the kernel > > starts seeing these error reports, we could see EDAC/ghes and > > efi/cper detailed logs in dmesg. We did not notice the problem until > > we check the console log, which inspired us to reduce the window > > when kernel startup, so that we can see the message clearly and > > properly. I think the intuition is to check the log of dmesg, not > > the console. > > > > However, I DO think that: > > > > > > - Removing acpi_hest_init() from acpi_pci_root_init(), and > > > > > > - Converting ghes_init() and sdei_init() from initcalls to explicit > > > calls > > > > > > are very good reasons to do something like this patch because HEST is > > > not PCI-specific, and IMO, explicit calls are better than initcalls > > > because initcall ordering is implicit and not well-defined within a > > > level. > > > > Haha, if the above reasons still don't convince you, I would like to > > accept yours :) Should we do it in one patch or separate it into two > > patches? > > IMO, this can be done in one patch, but this would probably go via > Rafael. Yes, that would make sense IMO.
