| Message ID | 1427474362-3903-1-git-send-email-clg@fr.ibm.com |
|---|---|
| State | Not Applicable |
| Headers | show |
On Fri, 2015-03-27 at 17:39 +0100, Cédric Le Goater wrote: > OPAL has its own list of return codes. The patch provides a translation > of such codes in errnos for the opal_sensor_read call, and possibly > others if needed. > > Index: linux.git/arch/powerpc/platforms/powernv/opal.c > =================================================================== > --- linux.git.orig/arch/powerpc/platforms/powernv/opal.c > +++ linux.git/arch/powerpc/platforms/powernv/opal.c > @@ -894,6 +894,23 @@ void opal_free_sg_list(struct opal_sg_li > } > } > > +int opal_error_code(int rc) > +{ > + switch (rc) { > + case OPAL_SUCCESS: return 0; Obviously correct. > + case OPAL_PARAMETER: return -EINVAL; Yep. > + case OPAL_UNSUPPORTED: return -ENOSYS; You shouldn't use ENOSYS here, that should only ever mean "no such syscall", otherwise you get very confusing results like read() returning ENOSYS. > + case OPAL_ASYNC_COMPLETION: return -EAGAIN; EAGAIN means "try what you did again", I don't think that's what ASYNC_COMPLETION means, is it? It looks like it means, "don't try again, but you need to wait for the result to be ready". I'm not sure it maps well to any of the Linux codes, maybe EINPROGRESS ? > + case OPAL_BUSY_EVENT: return -EBUSY; Yep. > + case OPAL_NO_MEM: return -ENOMEM; Yep. > + case OPAL_HARDWARE: return -ENOENT; This is another one which I think you shouldn't use as it can lead to confusing results at user level. eg: $ cat /sysfs/some/file Error: No such file or directory Huh? Looking at the skiboot code this looks like EIO is a good match. > + case OPAL_INTERNAL_ERROR: return -EIO; Yeah as good as anything I guess. > + default: > + pr_err("%s: unexpected OPAL error %d\n", __func__, rc); > + return -ERANGE; I'm not sure about this one honestly, it means "Math result not representable". I suspect the reason RTAS chose it was just that it's not EINVAL. This should probably also just be EIO. cheers
On 03/30/2015 04:05 AM, Michael Ellerman wrote: > On Fri, 2015-03-27 at 17:39 +0100, Cédric Le Goater wrote: >> OPAL has its own list of return codes. The patch provides a translation >> of such codes in errnos for the opal_sensor_read call, and possibly >> others if needed. >> >> Index: linux.git/arch/powerpc/platforms/powernv/opal.c >> =================================================================== >> --- linux.git.orig/arch/powerpc/platforms/powernv/opal.c >> +++ linux.git/arch/powerpc/platforms/powernv/opal.c >> @@ -894,6 +894,23 @@ void opal_free_sg_list(struct opal_sg_li >> } >> } >> >> +int opal_error_code(int rc) >> +{ >> + switch (rc) { >> + case OPAL_SUCCESS: return 0; > > Obviously correct. He. Initially, I didn't put a case for SUCCESS, but we have code doing : ret = be64_to_cpu(msg.params[1]); >> + case OPAL_PARAMETER: return -EINVAL; > > Yep. > >> + case OPAL_UNSUPPORTED: return -ENOSYS; > > You shouldn't use ENOSYS here, that should only ever mean "no such syscall", > otherwise you get very confusing results like read() returning ENOSYS. Indeed. How about ENODEV then ? >> + case OPAL_ASYNC_COMPLETION: return -EAGAIN; > > EAGAIN means "try what you did again", I don't think that's what > ASYNC_COMPLETION means, is it? It looks like it means, "don't try again, but > you need to wait for the result to be ready". > > I'm not sure it maps well to any of the Linux codes, maybe EINPROGRESS ? Yes. This is better. >> + case OPAL_BUSY_EVENT: return -EBUSY; > > Yep. > >> + case OPAL_NO_MEM: return -ENOMEM; > > Yep. > >> + case OPAL_HARDWARE: return -ENOENT; > > This is another one which I think you shouldn't use as it can lead to confusing > results at user level. eg: > > $ cat /sysfs/some/file > Error: No such file or directory > > Huh? > > Looking at the skiboot code this looks like EIO is a good match. ok. >> + case OPAL_INTERNAL_ERROR: return -EIO; > > Yeah as good as anything I guess. > >> + default: >> + pr_err("%s: unexpected OPAL error %d\n", __func__, rc); >> + return -ERANGE; > > I'm not sure about this one honestly, it means "Math result not representable". > > I suspect the reason RTAS chose it was just that it's not EINVAL. > > This should probably also just be EIO. ok. I will change it. Thanks, C.
On Mon, 2015-03-30 at 08:37 +0200, Cedric Le Goater wrote: > On 03/30/2015 04:05 AM, Michael Ellerman wrote: > > On Fri, 2015-03-27 at 17:39 +0100, Cédric Le Goater wrote: > >> OPAL has its own list of return codes. The patch provides a translation > >> of such codes in errnos for the opal_sensor_read call, and possibly > >> others if needed. > >> > >> + case OPAL_UNSUPPORTED: return -ENOSYS; > > > > You shouldn't use ENOSYS here, that should only ever mean "no such syscall", > > otherwise you get very confusing results like read() returning ENOSYS. > > Indeed. How about ENODEV then ? That can also be confusing from userspace. I think it's probably best just to use EIO, as far as userspace is concerned if the kernel lets it call an unsupported OPAL routine that is more or less a kernel bug. cheers
On 03/30/2015 08:54 AM, Michael Ellerman wrote: > On Mon, 2015-03-30 at 08:37 +0200, Cedric Le Goater wrote: >> On 03/30/2015 04:05 AM, Michael Ellerman wrote: >>> On Fri, 2015-03-27 at 17:39 +0100, Cédric Le Goater wrote: >>>> OPAL has its own list of return codes. The patch provides a translation >>>> of such codes in errnos for the opal_sensor_read call, and possibly >>>> others if needed. >>>> >>>> + case OPAL_UNSUPPORTED: return -ENOSYS; >>> >>> You shouldn't use ENOSYS here, that should only ever mean "no such syscall", >>> otherwise you get very confusing results like read() returning ENOSYS. >> >> Indeed. How about ENODEV then ? > > That can also be confusing from userspace. > > I think it's probably best just to use EIO, as far as userspace is concerned if > the kernel lets it call an unsupported OPAL routine that is more or less a > kernel bug. OK. Will do. Thanks, C.
Index: linux.git/arch/powerpc/platforms/powernv/opal-sensor.c =================================================================== --- linux.git.orig/arch/powerpc/platforms/powernv/opal-sensor.c +++ linux.git/arch/powerpc/platforms/powernv/opal-sensor.c @@ -46,8 +46,10 @@ int opal_get_sensor_data(u32 sensor_hndl mutex_lock(&opal_sensor_mutex); ret = opal_sensor_read(sensor_hndl, token, &data); - if (ret != OPAL_ASYNC_COMPLETION) + if (ret != OPAL_ASYNC_COMPLETION) { + ret = opal_error_code(ret); goto out_token; + } ret = opal_async_wait_response(token, &msg); if (ret) { @@ -57,7 +59,7 @@ int opal_get_sensor_data(u32 sensor_hndl } *sensor_data = be32_to_cpu(data); - ret = be64_to_cpu(msg.params[1]); + ret = opal_error_code(be64_to_cpu(msg.params[1])); out_token: mutex_unlock(&opal_sensor_mutex); Index: linux.git/arch/powerpc/include/asm/opal.h =================================================================== --- linux.git.orig/arch/powerpc/include/asm/opal.h +++ linux.git/arch/powerpc/include/asm/opal.h @@ -983,6 +983,8 @@ struct opal_sg_list *opal_vmalloc_to_sg_ unsigned long vmalloc_size); void opal_free_sg_list(struct opal_sg_list *sg); +extern int opal_error_code(int rc); + /* * Dump region ID range usable by the OS */ Index: linux.git/arch/powerpc/platforms/powernv/opal.c =================================================================== --- linux.git.orig/arch/powerpc/platforms/powernv/opal.c +++ linux.git/arch/powerpc/platforms/powernv/opal.c @@ -894,6 +894,23 @@ void opal_free_sg_list(struct opal_sg_li } } +int opal_error_code(int rc) +{ + switch (rc) { + case OPAL_SUCCESS: return 0; + case OPAL_PARAMETER: return -EINVAL; + case OPAL_UNSUPPORTED: return -ENOSYS; + case OPAL_ASYNC_COMPLETION: return -EAGAIN; + case OPAL_BUSY_EVENT: return -EBUSY; + case OPAL_NO_MEM: return -ENOMEM; + case OPAL_HARDWARE: return -ENOENT; + case OPAL_INTERNAL_ERROR: return -EIO; + default: + pr_err("%s: unexpected OPAL error %d\n", __func__, rc); + return -ERANGE; + } +} + EXPORT_SYMBOL_GPL(opal_poll_events); EXPORT_SYMBOL_GPL(opal_rtc_read); EXPORT_SYMBOL_GPL(opal_rtc_write);
OPAL has its own list of return codes. The patch provides a translation of such codes in errnos for the opal_sensor_read call, and possibly others if needed. Signed-off-by: Cédric Le Goater <clg@fr.ibm.com> --- Changes since v2 : - renamed and moved the routine to opal.[ch] - changed default value to ERANGE like rtas arch/powerpc/include/asm/opal.h | 2 ++ arch/powerpc/platforms/powernv/opal-sensor.c | 6 ++++-- arch/powerpc/platforms/powernv/opal.c | 17 +++++++++++++++++ 3 files changed, 23 insertions(+), 2 deletions(-)