Re: [edk2] PcRtcAcpiTableChangeCallback question
On 05/18/16 05:25, Zeng, Star wrote: > How about to call PcRtcAcpiTableChangeCallback(NULL, NULL) explicitly > before or after the event creation instead of signal the event? > The event is a group event, it is not same with the event in > EfiCreateProtocolNotifyEvent() that is a single event, signal a group > event may make other consumer of the event to think the table has > been installed really. I am not sure the impact. > > We have example in LegacyBios.c about the similar usage. > LegacyBios.c: > // > // Allocate reserved memory for SMBIOS table used in legacy boot if SMBIOS > table exists > // > InstallSmbiosEventCallback (NULL, NULL); > > // > // Create callback function to update the size of reserved memory after > LegacyBiosDxe starts > // > Status = gBS->CreateEventEx ( > EVT_NOTIFY_SIGNAL, > TPL_NOTIFY, > InstallSmbiosEventCallback, > NULL, > &gEfiSmbiosTableGuid, > &InstallSmbiosEvent > ); > ASSERT_EFI_ERROR (Status); I think in general event handlers should be able to deal with spurious signals, but I agree that your suggestion is safer. Thanks! Laszlo > > Thanks, > Star > -Original Message- > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Ni, > Ruiyu > Sent: Tuesday, May 17, 2016 6:29 PM > To: Laszlo Ersek ; Anbazhagan, Baraneedharan > ; edk2-devel@lists.01.org > Subject: Re: [edk2] PcRtcAcpiTableChangeCallback question > > Laszlo, > thanks for the quick proposal. > I will send out a formal patch to fix this issue. > > Regards, > Ray > >> -Original Message- >> From: Laszlo Ersek [mailto:ler...@redhat.com] >> Sent: Tuesday, May 17, 2016 5:42 PM >> To: Anbazhagan, Baraneedharan ; >> edk2-devel@lists.01.org ; Ni, Ruiyu >> >> Subject: Re: [edk2] PcRtcAcpiTableChangeCallback question >> >> On 05/17/16 01:20, Anbazhagan, Baraneedharan wrote: >> >>> PcRtcAcpiTableChangeCallback doesn't seem to be called if >>> gEfiAcpiTableGuid or gEfiAcpi10TableGuid is already installed before >>> PcRtc module gets loaded. Whether ACPI table Guid can be checked in >>> system configuration table and then register a callback if it's not >>> available or register a single callback on End of Dxe(assuming ACPI >>> table Guid will be available)? >> >> The right way for registering callbacks for protocol and sysconfig >> table installations is to kick the event immediately after registering >> the callback. (The callback has to be written defensively anyway.) >> >> This pattern can be seen for example in UefiLib, in the >> EfiCreateProtocolNotifyEvent() function. >> >> So, I'd propose: >> >>> diff --git a/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtcEntry.c >> b/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtcEntry.c >>> index 1cfb0cb19889..741c7ce625d0 100644 >>> --- a/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtcEntry.c >>> +++ b/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtcEntry.c >>> @@ -160,6 +160,11 @@ InitializePcRtc ( >>>); >>>ASSERT_EFI_ERROR (Status); >>> >>> + // >>> + // ACPI tables may have been installed already // >>> + gBS->SignalEvent (Event); >>> + >>>gRT->GetTime = PcRtcEfiGetTime; >>>gRT->SetTime = PcRtcEfiSetTime; >>>gRT->GetWakeupTime = PcRtcEfiGetWakeupTime; >> >> (totally untested) >> >> Thanks >> Laszlo > ___ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel > ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] PcRtcAcpiTableChangeCallback question
How about to call PcRtcAcpiTableChangeCallback(NULL, NULL) explicitly before or after the event creation instead of signal the event? The event is a group event, it is not same with the event in EfiCreateProtocolNotifyEvent() that is a single event, signal a group event may make other consumer of the event to think the table has been installed really. I am not sure the impact. We have example in LegacyBios.c about the similar usage. LegacyBios.c: // // Allocate reserved memory for SMBIOS table used in legacy boot if SMBIOS table exists // InstallSmbiosEventCallback (NULL, NULL); // // Create callback function to update the size of reserved memory after LegacyBiosDxe starts // Status = gBS->CreateEventEx ( EVT_NOTIFY_SIGNAL, TPL_NOTIFY, InstallSmbiosEventCallback, NULL, &gEfiSmbiosTableGuid, &InstallSmbiosEvent ); ASSERT_EFI_ERROR (Status); Thanks, Star -Original Message- From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Ni, Ruiyu Sent: Tuesday, May 17, 2016 6:29 PM To: Laszlo Ersek ; Anbazhagan, Baraneedharan ; edk2-devel@lists.01.org Subject: Re: [edk2] PcRtcAcpiTableChangeCallback question Laszlo, thanks for the quick proposal. I will send out a formal patch to fix this issue. Regards, Ray >-Original Message- >From: Laszlo Ersek [mailto:ler...@redhat.com] >Sent: Tuesday, May 17, 2016 5:42 PM >To: Anbazhagan, Baraneedharan ; >edk2-devel@lists.01.org ; Ni, Ruiyu > >Subject: Re: [edk2] PcRtcAcpiTableChangeCallback question > >On 05/17/16 01:20, Anbazhagan, Baraneedharan wrote: > >> PcRtcAcpiTableChangeCallback doesn't seem to be called if >> gEfiAcpiTableGuid or gEfiAcpi10TableGuid is already installed before >> PcRtc module gets loaded. Whether ACPI table Guid can be checked in >> system configuration table and then register a callback if it's not >> available or register a single callback on End of Dxe(assuming ACPI >> table Guid will be available)? > >The right way for registering callbacks for protocol and sysconfig >table installations is to kick the event immediately after registering >the callback. (The callback has to be written defensively anyway.) > >This pattern can be seen for example in UefiLib, in the >EfiCreateProtocolNotifyEvent() function. > >So, I'd propose: > >> diff --git a/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtcEntry.c >b/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtcEntry.c >> index 1cfb0cb19889..741c7ce625d0 100644 >> --- a/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtcEntry.c >> +++ b/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtcEntry.c >> @@ -160,6 +160,11 @@ InitializePcRtc ( >>); >>ASSERT_EFI_ERROR (Status); >> >> + // >> + // ACPI tables may have been installed already // >> + gBS->SignalEvent (Event); >> + >>gRT->GetTime = PcRtcEfiGetTime; >>gRT->SetTime = PcRtcEfiSetTime; >>gRT->GetWakeupTime = PcRtcEfiGetWakeupTime; > >(totally untested) > >Thanks >Laszlo ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] PcRtcAcpiTableChangeCallback question
Laszlo, thanks for the quick proposal. I will send out a formal patch to fix this issue. Regards, Ray >-Original Message- >From: Laszlo Ersek [mailto:ler...@redhat.com] >Sent: Tuesday, May 17, 2016 5:42 PM >To: Anbazhagan, Baraneedharan ; edk2-devel@lists.01.org >; Ni, Ruiyu > >Subject: Re: [edk2] PcRtcAcpiTableChangeCallback question > >On 05/17/16 01:20, Anbazhagan, Baraneedharan wrote: > >> PcRtcAcpiTableChangeCallback doesn't seem to be called if >> gEfiAcpiTableGuid or gEfiAcpi10TableGuid is already installed before >> PcRtc module gets loaded. Whether ACPI table Guid can be checked in >> system configuration table and then register a callback if it's not >> available or register a single callback on End of Dxe(assuming ACPI >> table Guid will be available)? > >The right way for registering callbacks for protocol and sysconfig table >installations is to kick the event immediately after >registering the callback. (The callback has to be written defensively anyway.) > >This pattern can be seen for example in UefiLib, in the >EfiCreateProtocolNotifyEvent() function. > >So, I'd propose: > >> diff --git a/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtcEntry.c >b/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtcEntry.c >> index 1cfb0cb19889..741c7ce625d0 100644 >> --- a/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtcEntry.c >> +++ b/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtcEntry.c >> @@ -160,6 +160,11 @@ InitializePcRtc ( >>); >>ASSERT_EFI_ERROR (Status); >> >> + // >> + // ACPI tables may have been installed already >> + // >> + gBS->SignalEvent (Event); >> + >>gRT->GetTime = PcRtcEfiGetTime; >>gRT->SetTime = PcRtcEfiSetTime; >>gRT->GetWakeupTime = PcRtcEfiGetWakeupTime; > >(totally untested) > >Thanks >Laszlo ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] PcRtcAcpiTableChangeCallback question
On 05/17/16 01:20, Anbazhagan, Baraneedharan wrote: > PcRtcAcpiTableChangeCallback doesn't seem to be called if > gEfiAcpiTableGuid or gEfiAcpi10TableGuid is already installed before > PcRtc module gets loaded. Whether ACPI table Guid can be checked in > system configuration table and then register a callback if it's not > available or register a single callback on End of Dxe(assuming ACPI > table Guid will be available)? The right way for registering callbacks for protocol and sysconfig table installations is to kick the event immediately after registering the callback. (The callback has to be written defensively anyway.) This pattern can be seen for example in UefiLib, in the EfiCreateProtocolNotifyEvent() function. So, I'd propose: > diff --git a/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtcEntry.c > b/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtcEntry.c > index 1cfb0cb19889..741c7ce625d0 100644 > --- a/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtcEntry.c > +++ b/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtcEntry.c > @@ -160,6 +160,11 @@ InitializePcRtc ( >); >ASSERT_EFI_ERROR (Status); > > + // > + // ACPI tables may have been installed already > + // > + gBS->SignalEvent (Event); > + >gRT->GetTime = PcRtcEfiGetTime; >gRT->SetTime = PcRtcEfiSetTime; >gRT->GetWakeupTime = PcRtcEfiGetWakeupTime; (totally untested) Thanks Laszlo ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
