| Message ID | 20180215073144.12979-3-xypron.glpk@gmx.de |
|---|---|
| State | Superseded, archived |
| Delegated to: | Alexander Graf |
| Headers | show |
| Series | efi_loader: error handling cmd/bootefi.c | expand |
Hi Heinrich, On 15 February 2018 at 00:31, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > All initialization routines should return a status code instead of > a boolean. > > Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > --- > v2 > new patch > --- > include/efi_loader.h | 2 +- > lib/efi_loader/efi_gop.c | 34 ++++++++++++++++++++++------------ > 2 files changed, 23 insertions(+), 13 deletions(-) > > diff --git a/include/efi_loader.h b/include/efi_loader.h > index 72c83fd5033..779b8bde2e3 100644 > --- a/include/efi_loader.h > +++ b/include/efi_loader.h > @@ -179,7 +179,7 @@ int efi_disk_create_partitions(efi_handle_t parent, struct blk_desc *desc, > const char *if_typename, int diskid, > const char *pdevname); > /* Called by bootefi to make GOP (graphical) interface available */ > -int efi_gop_register(void); > +efi_status_t efi_gop_register(void); > /* Called by bootefi to make the network interface available */ > int efi_net_register(void); > /* Called by bootefi to make the watchdog available */ > diff --git a/lib/efi_loader/efi_gop.c b/lib/efi_loader/efi_gop.c > index 3caddd5f844..91b0b6a064b 100644 > --- a/lib/efi_loader/efi_gop.c > +++ b/lib/efi_loader/efi_gop.c > @@ -125,8 +125,13 @@ efi_status_t EFIAPI gop_blt(struct efi_gop *this, void *buffer, > return EFI_EXIT(EFI_SUCCESS); > } > > -/* This gets called from do_bootefi_exec(). */ > -int efi_gop_register(void) > +/* > + * Install graphical output protocol. > + * > + * If no supported video device exists this is not considered as an > + * error. > + */ This comment should be in the header file so people can see the API in one place. It's unfortunate that U-Boot error codes get lost here. Perhaps it does not make sense to return them and have the caller report the error? I'm not sure what is best, but one symptom of the current approach is the use of printf() to report (and suppress) the error. Regards, Simon
On 03/23/2018 03:29 PM, Simon Glass wrote: > Hi Heinrich, > > On 15 February 2018 at 00:31, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: >> All initialization routines should return a status code instead of >> a boolean. >> >> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> >> --- >> v2 >> new patch >> --- >> include/efi_loader.h | 2 +- >> lib/efi_loader/efi_gop.c | 34 ++++++++++++++++++++++------------ >> 2 files changed, 23 insertions(+), 13 deletions(-) >> >> diff --git a/include/efi_loader.h b/include/efi_loader.h >> index 72c83fd5033..779b8bde2e3 100644 >> --- a/include/efi_loader.h >> +++ b/include/efi_loader.h >> @@ -179,7 +179,7 @@ int efi_disk_create_partitions(efi_handle_t parent, struct blk_desc *desc, >> const char *if_typename, int diskid, >> const char *pdevname); >> /* Called by bootefi to make GOP (graphical) interface available */ >> -int efi_gop_register(void); >> +efi_status_t efi_gop_register(void); >> /* Called by bootefi to make the network interface available */ >> int efi_net_register(void); >> /* Called by bootefi to make the watchdog available */ >> diff --git a/lib/efi_loader/efi_gop.c b/lib/efi_loader/efi_gop.c >> index 3caddd5f844..91b0b6a064b 100644 >> --- a/lib/efi_loader/efi_gop.c >> +++ b/lib/efi_loader/efi_gop.c >> @@ -125,8 +125,13 @@ efi_status_t EFIAPI gop_blt(struct efi_gop *this, void *buffer, >> return EFI_EXIT(EFI_SUCCESS); >> } >> >> -/* This gets called from do_bootefi_exec(). */ >> -int efi_gop_register(void) >> +/* >> + * Install graphical output protocol. >> + * >> + * If no supported video device exists this is not considered as an >> + * error. >> + */ > > This comment should be in the header file so people can see the API in > one place. > > It's unfortunate that U-Boot error codes get lost here. Perhaps it > does not make sense to return them and have the caller report the > error? I'm not sure what is best, but one symptom of the current > approach is the use of printf() to report (and suppress) the error. I understand why using log() is a better choice than printf(). We already added a log category for the EFI subsystem without using it yet. Do I get your idea right: You would return an error code to the caller. And if the caller thinks that the GOP protocol is not needed he should output a log message and pass on. As Alex has already picked this patch I would prefer to put such a change into an extra patch. Regards Heinrich
diff --git a/include/efi_loader.h b/include/efi_loader.h index 72c83fd5033..779b8bde2e3 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -179,7 +179,7 @@ int efi_disk_create_partitions(efi_handle_t parent, struct blk_desc *desc, const char *if_typename, int diskid, const char *pdevname); /* Called by bootefi to make GOP (graphical) interface available */ -int efi_gop_register(void); +efi_status_t efi_gop_register(void); /* Called by bootefi to make the network interface available */ int efi_net_register(void); /* Called by bootefi to make the watchdog available */ diff --git a/lib/efi_loader/efi_gop.c b/lib/efi_loader/efi_gop.c index 3caddd5f844..91b0b6a064b 100644 --- a/lib/efi_loader/efi_gop.c +++ b/lib/efi_loader/efi_gop.c @@ -125,8 +125,13 @@ efi_status_t EFIAPI gop_blt(struct efi_gop *this, void *buffer, return EFI_EXIT(EFI_SUCCESS); } -/* This gets called from do_bootefi_exec(). */ -int efi_gop_register(void) +/* + * Install graphical output protocol. + * + * If no supported video device exists this is not considered as an + * error. + */ +efi_status_t efi_gop_register(void) { struct efi_gop_obj *gopobj; u32 bpix, col, row; @@ -136,12 +141,15 @@ int efi_gop_register(void) #ifdef CONFIG_DM_VIDEO struct udevice *vdev; + struct video_priv *priv; /* We only support a single video output device for now */ - if (uclass_first_device(UCLASS_VIDEO, &vdev) || !vdev) - return -1; + if (uclass_first_device(UCLASS_VIDEO, &vdev) || !vdev) { + printf("WARNING: No video device\n"); + return EFI_SUCCESS; + } - struct video_priv *priv = dev_get_uclass_priv(vdev); + priv = dev_get_uclass_priv(vdev); bpix = priv->bpix; col = video_get_xsize(vdev); row = video_get_ysize(vdev); @@ -170,13 +178,14 @@ int efi_gop_register(void) break; default: /* So far, we only work in 16 or 32 bit mode */ - return -1; + printf("WARNING: Unsupported video mode\n"); + return EFI_SUCCESS; } gopobj = calloc(1, sizeof(*gopobj)); if (!gopobj) { printf("ERROR: Out of memory\n"); - return 1; + return EFI_OUT_OF_RESOURCES; } /* Hook up to the device list */ @@ -186,8 +195,8 @@ int efi_gop_register(void) ret = efi_add_protocol(gopobj->parent.handle, &efi_gop_guid, &gopobj->ops); if (ret != EFI_SUCCESS) { - printf("ERROR: Out of memory\n"); - return 1; + printf("ERROR: Failure adding gop protocol\n"); + return ret; } gopobj->ops.query_mode = gop_query_mode; gopobj->ops.set_mode = gop_set_mode; @@ -199,10 +208,11 @@ int efi_gop_register(void) gopobj->mode.info_size = sizeof(gopobj->info); #ifdef CONFIG_DM_VIDEO - if (bpix == VIDEO_BPP32) { + if (bpix == VIDEO_BPP32) #else - if (bpix == LCD_COLOR32) { + if (bpix == LCD_COLOR32) #endif + { /* With 32bit color space we can directly expose the fb */ gopobj->mode.fb_base = fb_base; gopobj->mode.fb_size = fb_size; @@ -217,5 +227,5 @@ int efi_gop_register(void) gopobj->bpix = bpix; gopobj->fb = fb; - return 0; + return EFI_SUCCESS; }
All initialization routines should return a status code instead of a boolean. Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> --- v2 new patch --- include/efi_loader.h | 2 +- lib/efi_loader/efi_gop.c | 34 ++++++++++++++++++++++------------ 2 files changed, 23 insertions(+), 13 deletions(-)