| Message ID | 20170815194245.1450-1-xypron.glpk@gmx.de |
|---|---|
| State | Superseded |
| Delegated to: | Alexander Graf |
| Headers | show |
On Tue, Aug 15, 2017 at 3:42 PM, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > Macro EFI_CALL was introduced to call an UEFI function. > Unfortunately is did not support return values. > Most UEFI functions have a return value. > > So let's rename EFI_CALL to EFI_CALL_VOID and introduce a > new EFI_CALL macro that supports return values. > > Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > --- > include/efi_loader.h | 16 ++++++++++++++-- > lib/efi_loader/efi_boottime.c | 3 ++- > 2 files changed, 16 insertions(+), 3 deletions(-) > > diff --git a/include/efi_loader.h b/include/efi_loader.h > index 037cc7c543..1cee10ea0c 100644 > --- a/include/efi_loader.h > +++ b/include/efi_loader.h > @@ -41,9 +41,21 @@ const char *__efi_nesting_dec(void); > }) > > /* > - * Callback into UEFI world from u-boot: > + * Call non-void UEFI function from u-boot and retrieve return value: > */ > -#define EFI_CALL(exp) do { \ > +#define EFI_CALL(exp) ({ \ > + debug("%sEFI: Call: %s\n", __efi_nesting_inc(), #exp); \ > + assert(__efi_exit_check()); \ > + typeof(exp) r = exp; \ > + assert(__efi_entry_check()); \ > + debug("%sEFI: Return From: %s\n", __efi_nesting_dec(), #exp); \ > + r; \ > +}) > + So, I had considered something similar, for the purposes of being able to print return value, but in the end decided that the EFI_EXIT() print was usually sufficient (except in cases where you enable DEBUG in some files but not others), and opted for the simpler MACRO(ret = somecall()) approach.. also the longer macro name makes not going over 80 chars harder in some cases ;-) I don't really feel strongly one way or another.. but I guess if we do this we might as well add the return value (cast to u64 and print as hex to cover both ptrs and efi_status_t?) to the "Return From" debug print. BR, -R > +/* > + * Call void UEFI function world from u-boot: > + */ > +#define EFI_CALL_VOID(exp) do { \ > debug("%sEFI: Call: %s\n", __efi_nesting_inc(), #exp); \ > assert(__efi_exit_check()); \ > exp; \ > diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c > index 43f32385fa..6489a32505 100644 > --- a/lib/efi_loader/efi_boottime.c > +++ b/lib/efi_loader/efi_boottime.c > @@ -158,7 +158,8 @@ void efi_signal_event(struct efi_event *event) > return; > event->signaled = 1; > if (event->type & EVT_NOTIFY_SIGNAL) { > - EFI_CALL(event->notify_function(event, event->notify_context)); > + EFI_CALL_VOID(event->notify_function(event, > + event->notify_context)); > } > } > > -- > 2.14.1 >
On 08/17/2017 01:49 PM, Rob Clark wrote: > On Tue, Aug 15, 2017 at 3:42 PM, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: >> Macro EFI_CALL was introduced to call an UEFI function. >> Unfortunately is did not support return values. >> Most UEFI functions have a return value. >> >> So let's rename EFI_CALL to EFI_CALL_VOID and introduce a >> new EFI_CALL macro that supports return values. >> >> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> >> --- >> include/efi_loader.h | 16 ++++++++++++++-- >> lib/efi_loader/efi_boottime.c | 3 ++- >> 2 files changed, 16 insertions(+), 3 deletions(-) >> >> diff --git a/include/efi_loader.h b/include/efi_loader.h >> index 037cc7c543..1cee10ea0c 100644 >> --- a/include/efi_loader.h >> +++ b/include/efi_loader.h >> @@ -41,9 +41,21 @@ const char *__efi_nesting_dec(void); >> }) >> >> /* >> - * Callback into UEFI world from u-boot: >> + * Call non-void UEFI function from u-boot and retrieve return value: >> */ >> -#define EFI_CALL(exp) do { \ >> +#define EFI_CALL(exp) ({ \ >> + debug("%sEFI: Call: %s\n", __efi_nesting_inc(), #exp); \ >> + assert(__efi_exit_check()); \ >> + typeof(exp) r = exp; \ >> + assert(__efi_entry_check()); \ >> + debug("%sEFI: Return From: %s\n", __efi_nesting_dec(), #exp); \ >> + r; \ >> +}) >> + > > So, I had considered something similar, for the purposes of being able > to print return value, but in the end decided that the EFI_EXIT() > print was usually sufficient (except in cases where you enable DEBUG > in some files but not others), and opted for the simpler MACRO(ret = > somecall()) approach.. also the longer macro name makes not going over > 80 chars harder in some cases ;-) > > I don't really feel strongly one way or another.. but I guess if we do > this we might as well add the return value (cast to u64 and print as > hex to cover both ptrs and efi_status_t?) to the "Return From" debug > print. We already print the return value in EFI_EXIT. Why should we print it twice? In c160d2f5ec9 Regards Heinrich > > BR, > -R > >> +/* >> + * Call void UEFI function world from u-boot: >> + */ >> +#define EFI_CALL_VOID(exp) do { \ >> debug("%sEFI: Call: %s\n", __efi_nesting_inc(), #exp); \ >> assert(__efi_exit_check()); \ >> exp; \ >> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c >> index 43f32385fa..6489a32505 100644 >> --- a/lib/efi_loader/efi_boottime.c >> +++ b/lib/efi_loader/efi_boottime.c >> @@ -158,7 +158,8 @@ void efi_signal_event(struct efi_event *event) >> return; >> event->signaled = 1; >> if (event->type & EVT_NOTIFY_SIGNAL) { >> - EFI_CALL(event->notify_function(event, event->notify_context)); >> + EFI_CALL_VOID(event->notify_function(event, >> + event->notify_context)); >> } >> } >> >> -- >> 2.14.1 >> >
On Thu, Aug 17, 2017 at 12:57 PM, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > On 08/17/2017 01:49 PM, Rob Clark wrote: >> On Tue, Aug 15, 2017 at 3:42 PM, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: >>> Macro EFI_CALL was introduced to call an UEFI function. >>> Unfortunately is did not support return values. >>> Most UEFI functions have a return value. >>> >>> So let's rename EFI_CALL to EFI_CALL_VOID and introduce a >>> new EFI_CALL macro that supports return values. >>> >>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> >>> --- >>> include/efi_loader.h | 16 ++++++++++++++-- >>> lib/efi_loader/efi_boottime.c | 3 ++- >>> 2 files changed, 16 insertions(+), 3 deletions(-) >>> >>> diff --git a/include/efi_loader.h b/include/efi_loader.h >>> index 037cc7c543..1cee10ea0c 100644 >>> --- a/include/efi_loader.h >>> +++ b/include/efi_loader.h >>> @@ -41,9 +41,21 @@ const char *__efi_nesting_dec(void); >>> }) >>> >>> /* >>> - * Callback into UEFI world from u-boot: >>> + * Call non-void UEFI function from u-boot and retrieve return value: >>> */ >>> -#define EFI_CALL(exp) do { \ >>> +#define EFI_CALL(exp) ({ \ >>> + debug("%sEFI: Call: %s\n", __efi_nesting_inc(), #exp); \ >>> + assert(__efi_exit_check()); \ >>> + typeof(exp) r = exp; \ >>> + assert(__efi_entry_check()); \ >>> + debug("%sEFI: Return From: %s\n", __efi_nesting_dec(), #exp); \ >>> + r; \ >>> +}) >>> + >> >> So, I had considered something similar, for the purposes of being able >> to print return value, but in the end decided that the EFI_EXIT() >> print was usually sufficient (except in cases where you enable DEBUG >> in some files but not others), and opted for the simpler MACRO(ret = >> somecall()) approach.. also the longer macro name makes not going over >> 80 chars harder in some cases ;-) >> >> I don't really feel strongly one way or another.. but I guess if we do >> this we might as well add the return value (cast to u64 and print as >> hex to cover both ptrs and efi_status_t?) to the "Return From" debug >> print. > > We already print the return value in EFI_EXIT. > Why should we print it twice? > > In c160d2f5ec9 > the main reason would be enabling debug in a subset of the files (such as the one calling the API but not the one implementing the API), which is something I do pretty regularly. And I guess in the future we could have payloads installing protocols? BR, -R
diff --git a/include/efi_loader.h b/include/efi_loader.h index 037cc7c543..1cee10ea0c 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -41,9 +41,21 @@ const char *__efi_nesting_dec(void); }) /* - * Callback into UEFI world from u-boot: + * Call non-void UEFI function from u-boot and retrieve return value: */ -#define EFI_CALL(exp) do { \ +#define EFI_CALL(exp) ({ \ + debug("%sEFI: Call: %s\n", __efi_nesting_inc(), #exp); \ + assert(__efi_exit_check()); \ + typeof(exp) r = exp; \ + assert(__efi_entry_check()); \ + debug("%sEFI: Return From: %s\n", __efi_nesting_dec(), #exp); \ + r; \ +}) + +/* + * Call void UEFI function world from u-boot: + */ +#define EFI_CALL_VOID(exp) do { \ debug("%sEFI: Call: %s\n", __efi_nesting_inc(), #exp); \ assert(__efi_exit_check()); \ exp; \ diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index 43f32385fa..6489a32505 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -158,7 +158,8 @@ void efi_signal_event(struct efi_event *event) return; event->signaled = 1; if (event->type & EVT_NOTIFY_SIGNAL) { - EFI_CALL(event->notify_function(event, event->notify_context)); + EFI_CALL_VOID(event->notify_function(event, + event->notify_context)); } }
Macro EFI_CALL was introduced to call an UEFI function. Unfortunately is did not support return values. Most UEFI functions have a return value. So let's rename EFI_CALL to EFI_CALL_VOID and introduce a new EFI_CALL macro that supports return values. Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> --- include/efi_loader.h | 16 ++++++++++++++-- lib/efi_loader/efi_boottime.c | 3 ++- 2 files changed, 16 insertions(+), 3 deletions(-)