| Message ID | 20171005060424.16780-1-sakari.ailus@linux.intel.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Thursday, October 5, 2017 8:04:24 AM CEST Sakari Ailus wrote: > Fix more return codes for device property: Align return codes of > __acpi_node_get_property_reference. In particular what was missed > previously: > > -EPROTO could be returned in certain cases, now -EINVAL; > -EINVAL was returned if the property was not found, now -ENOENT; > -EINVAL was returned also if the index was higher than the number of > entries in a package, now -ENOENT. > > Fixes: ("device property: Align return codes of __acpi_node_get_property_reference") > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > Tested-by: Hyungwoo Yang <hyungwoo.yang@intel.com> > --- > Hi Rafael, > > Unfortunately the patch I posted the previous time to remedy the issue > ("device property: Align return codes of > _acpi_node_get_property_reference") did not fully fix the issue. OK, thanks for letting me know, but why didn't it? > drivers/acpi/property.c | 20 +++++++++++++++----- > 1 file changed, 15 insertions(+), 5 deletions(-) > > diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c > index 5a8ac5e1081b..8c28c516e7ec 100644 > --- a/drivers/acpi/property.c > +++ b/drivers/acpi/property.c > @@ -592,8 +592,16 @@ int __acpi_node_get_property_reference(const struct fwnode_handle *fwnode, > return -ENOENT; > > ret = acpi_data_get_property(data, propname, ACPI_TYPE_ANY, &obj); > - if (ret) > - return ret; > + switch (ret) { > + case -EINVAL: > + return -ENOENT; > + case -EPROTO: > + return -EINVAL; > + default: > + if (ret) > + return ret; > + break; > + } To be clear, I'm not going to apply anything like the above. > > /* > * The simplest case is when the value is a single reference. Just > @@ -605,7 +613,7 @@ int __acpi_node_get_property_reference(const struct fwnode_handle *fwnode, > > ret = acpi_bus_get_device(obj->reference.handle, &device); > if (ret) > - return ret; > + return ret == -ENODEV ? -EINVAL : ret; > > args->adev = device; > args->nargs = 0; > @@ -621,8 +629,10 @@ int __acpi_node_get_property_reference(const struct fwnode_handle *fwnode, > * The index argument is then used to determine which reference > * the caller wants (along with the arguments). > */ > - if (obj->type != ACPI_TYPE_PACKAGE || index >= obj->package.count) > - return -EPROTO; > + if (obj->type != ACPI_TYPE_PACKAGE) > + return -EINVAL; > + if (index >= obj->package.count) > + return -ENOENT; > > element = obj->package.elements; > end = element + obj->package.count; > -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Rafael, On Thu, Oct 05, 2017 at 02:59:45PM +0200, Rafael J. Wysocki wrote: > On Thursday, October 5, 2017 8:04:24 AM CEST Sakari Ailus wrote: > > Fix more return codes for device property: Align return codes of > > __acpi_node_get_property_reference. In particular what was missed > > previously: > > > > -EPROTO could be returned in certain cases, now -EINVAL; > > -EINVAL was returned if the property was not found, now -ENOENT; > > -EINVAL was returned also if the index was higher than the number of > > entries in a package, now -ENOENT. > > > > Fixes: ("device property: Align return codes of __acpi_node_get_property_reference") > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > Tested-by: Hyungwoo Yang <hyungwoo.yang@intel.com> > > --- > > Hi Rafael, > > > > Unfortunately the patch I posted the previous time to remedy the issue > > ("device property: Align return codes of > > _acpi_node_get_property_reference") did not fully fix the issue. > > OK, thanks for letting me know, but why didn't it? My testing appears to have been more limited than I thought, Hyungwoo later on found this out. (Reported-by: Hyungwoo... would be appropriate, I'll add that the next time.) > > > drivers/acpi/property.c | 20 +++++++++++++++----- > > 1 file changed, 15 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c > > index 5a8ac5e1081b..8c28c516e7ec 100644 > > --- a/drivers/acpi/property.c > > +++ b/drivers/acpi/property.c > > @@ -592,8 +592,16 @@ int __acpi_node_get_property_reference(const struct fwnode_handle *fwnode, > > return -ENOENT; > > > > ret = acpi_data_get_property(data, propname, ACPI_TYPE_ANY, &obj); > > - if (ret) > > - return ret; > > + switch (ret) { > > + case -EINVAL: > > + return -ENOENT; > > + case -EPROTO: > > + return -EINVAL; > > + default: > > + if (ret) > > + return ret; > > + break; > > + } > > To be clear, I'm not going to apply anything like the above. On exactly what grounds? You don't like the combination of switch and if, or because of the return values themselves? Or something else? > > > > > /* > > * The simplest case is when the value is a single reference. Just > > @@ -605,7 +613,7 @@ int __acpi_node_get_property_reference(const struct fwnode_handle *fwnode, > > > > ret = acpi_bus_get_device(obj->reference.handle, &device); > > if (ret) > > - return ret; > > + return ret == -ENODEV ? -EINVAL : ret; > > > > args->adev = device; > > args->nargs = 0; > > @@ -621,8 +629,10 @@ int __acpi_node_get_property_reference(const struct fwnode_handle *fwnode, > > * The index argument is then used to determine which reference > > * the caller wants (along with the arguments). > > */ > > - if (obj->type != ACPI_TYPE_PACKAGE || index >= obj->package.count) > > - return -EPROTO; > > + if (obj->type != ACPI_TYPE_PACKAGE) > > + return -EINVAL; > > + if (index >= obj->package.count) > > + return -ENOENT; > > > > element = obj->package.elements; > > end = element + obj->package.count; > > > >
On Thursday, October 5, 2017 4:01:48 PM CEST Sakari Ailus wrote: > Hi Rafael, > > On Thu, Oct 05, 2017 at 02:59:45PM +0200, Rafael J. Wysocki wrote: > > On Thursday, October 5, 2017 8:04:24 AM CEST Sakari Ailus wrote: > > > Fix more return codes for device property: Align return codes of > > > __acpi_node_get_property_reference. In particular what was missed > > > previously: > > > > > > -EPROTO could be returned in certain cases, now -EINVAL; > > > -EINVAL was returned if the property was not found, now -ENOENT; > > > -EINVAL was returned also if the index was higher than the number of > > > entries in a package, now -ENOENT. > > > > > > Fixes: ("device property: Align return codes of __acpi_node_get_property_reference") > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > > Tested-by: Hyungwoo Yang <hyungwoo.yang@intel.com> > > > --- > > > Hi Rafael, > > > > > > Unfortunately the patch I posted the previous time to remedy the issue > > > ("device property: Align return codes of > > > _acpi_node_get_property_reference") did not fully fix the issue. > > > > OK, thanks for letting me know, but why didn't it? > > My testing appears to have been more limited than I thought, Hyungwoo later > on found this out. (Reported-by: Hyungwoo... would be appropriate, I'll add > that the next time.) > > > > > > drivers/acpi/property.c | 20 +++++++++++++++----- > > > 1 file changed, 15 insertions(+), 5 deletions(-) > > > > > > diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c > > > index 5a8ac5e1081b..8c28c516e7ec 100644 > > > --- a/drivers/acpi/property.c > > > +++ b/drivers/acpi/property.c > > > @@ -592,8 +592,16 @@ int __acpi_node_get_property_reference(const struct fwnode_handle *fwnode, > > > return -ENOENT; > > > > > > ret = acpi_data_get_property(data, propname, ACPI_TYPE_ANY, &obj); > > > - if (ret) > > > - return ret; > > > + switch (ret) { > > > + case -EINVAL: > > > + return -ENOENT; > > > + case -EPROTO: > > > + return -EINVAL; > > > + default: > > > + if (ret) > > > + return ret; > > > + break; > > > + } > > > > To be clear, I'm not going to apply anything like the above. > > On exactly what grounds? You don't like the combination of switch and if, > or because of the return values themselves? Or something else? I just don't like changing error codes into different ones on the fly like this. It always indicates some bad design somewhere and this particular piece of code just goes over the top with that IMO. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Oct 5, 2017 at 4:01 PM, Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > Hi Rafael, > > On Thu, Oct 05, 2017 at 02:59:45PM +0200, Rafael J. Wysocki wrote: >> On Thursday, October 5, 2017 8:04:24 AM CEST Sakari Ailus wrote: >> > Fix more return codes for device property: Align return codes of >> > __acpi_node_get_property_reference. In particular what was missed >> > previously: >> > >> > -EPROTO could be returned in certain cases, now -EINVAL; >> > -EINVAL was returned if the property was not found, now -ENOENT; >> > -EINVAL was returned also if the index was higher than the number of >> > entries in a package, now -ENOENT. >> > >> > Fixes: ("device property: Align return codes of __acpi_node_get_property_reference") >> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> >> > Tested-by: Hyungwoo Yang <hyungwoo.yang@intel.com> >> > --- >> > Hi Rafael, >> > >> > Unfortunately the patch I posted the previous time to remedy the issue >> > ("device property: Align return codes of >> > _acpi_node_get_property_reference") did not fully fix the issue. >> >> OK, thanks for letting me know, but why didn't it? > > My testing appears to have been more limited than I thought, Hyungwoo later > on found this out. (Reported-by: Hyungwoo... would be appropriate, I'll add > that the next time.) Well, you're not saying what exactly still doesn't work with the previous patch applied which really is what I was asking about. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Rafael, On Thu, Oct 05, 2017 at 05:30:37PM +0200, Rafael J. Wysocki wrote: > On Thursday, October 5, 2017 4:01:48 PM CEST Sakari Ailus wrote: > > Hi Rafael, > > > > On Thu, Oct 05, 2017 at 02:59:45PM +0200, Rafael J. Wysocki wrote: > > > On Thursday, October 5, 2017 8:04:24 AM CEST Sakari Ailus wrote: > > > > Fix more return codes for device property: Align return codes of > > > > __acpi_node_get_property_reference. In particular what was missed > > > > previously: > > > > > > > > -EPROTO could be returned in certain cases, now -EINVAL; > > > > -EINVAL was returned if the property was not found, now -ENOENT; > > > > -EINVAL was returned also if the index was higher than the number of > > > > entries in a package, now -ENOENT. > > > > > > > > Fixes: ("device property: Align return codes of __acpi_node_get_property_reference") > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > > > Tested-by: Hyungwoo Yang <hyungwoo.yang@intel.com> > > > > --- > > > > Hi Rafael, > > > > > > > > Unfortunately the patch I posted the previous time to remedy the issue > > > > ("device property: Align return codes of > > > > _acpi_node_get_property_reference") did not fully fix the issue. > > > > > > OK, thanks for letting me know, but why didn't it? > > > > My testing appears to have been more limited than I thought, Hyungwoo later > > on found this out. (Reported-by: Hyungwoo... would be appropriate, I'll add > > that the next time.) > > > > > > > > > drivers/acpi/property.c | 20 +++++++++++++++----- > > > > 1 file changed, 15 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c > > > > index 5a8ac5e1081b..8c28c516e7ec 100644 > > > > --- a/drivers/acpi/property.c > > > > +++ b/drivers/acpi/property.c > > > > @@ -592,8 +592,16 @@ int __acpi_node_get_property_reference(const struct fwnode_handle *fwnode, > > > > return -ENOENT; > > > > > > > > ret = acpi_data_get_property(data, propname, ACPI_TYPE_ANY, &obj); > > > > - if (ret) > > > > - return ret; > > > > + switch (ret) { > > > > + case -EINVAL: > > > > + return -ENOENT; > > > > + case -EPROTO: > > > > + return -EINVAL; > > > > + default: > > > > + if (ret) > > > > + return ret; > > > > + break; > > > > + } > > > > > > To be clear, I'm not going to apply anything like the above. > > > > On exactly what grounds? You don't like the combination of switch and if, > > or because of the return values themselves? Or something else? > > I just don't like changing error codes into different ones on the fly like > this. It always indicates some bad design somewhere and this particular > piece of code just goes over the top with that IMO. acpi_data_get_property() is used in quite a few places and the error codes it returns are more or less used as such elsewhere. The ACPI and OF frameworks tend to use slightly different error codes to signal various error conditions. In this case, the OF archetype of fwnode_property_get_reference_args(), of_parse_phandle_with_args(), also uses different error codes than the rest of the OF framework. We decided to align the error codes with the OF framework, as has been done with the rest of the functions. Therefore the error codes returned by ACPI functions need to be converted to what is expected to be seen on the device property API (or OF API). My original proposal was to switch between the conventions in the ACPI fwnode callback acpi_fwnode_get_reference_args() but based on the review comments I made the changes to __acpi_node_get_property_reference() instead. Which is where we're now. fwnode_property_get_reference_args() (as does of_parse_phandle_with_args()) returns -ENOENT on - no reference found at given index (but entry exists), - no index exists in property or - the property does not exist. Originally __acpi_node_get_property_reference() used -ENOENT, -ENODATA and -EINVAL respectively. Consequenty -EINVAL returned by acpi_data_get_property(), called by __acpi_node_get_property_reference() needs to be turned to -ENOENT instead. Likewise -EPROTO changes to -EINVAL, as fwnode_property_get_reference_args() isn't expected to return -EPROTO.
On Fri, Oct 6, 2017 at 12:04 AM, Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > Hi Rafael, > > On Thu, Oct 05, 2017 at 05:30:37PM +0200, Rafael J. Wysocki wrote: >> On Thursday, October 5, 2017 4:01:48 PM CEST Sakari Ailus wrote: >> > Hi Rafael, >> > >> > On Thu, Oct 05, 2017 at 02:59:45PM +0200, Rafael J. Wysocki wrote: >> > > On Thursday, October 5, 2017 8:04:24 AM CEST Sakari Ailus wrote: >> > > > Fix more return codes for device property: Align return codes of >> > > > __acpi_node_get_property_reference. In particular what was missed >> > > > previously: >> > > > >> > > > -EPROTO could be returned in certain cases, now -EINVAL; >> > > > -EINVAL was returned if the property was not found, now -ENOENT; >> > > > -EINVAL was returned also if the index was higher than the number of >> > > > entries in a package, now -ENOENT. >> > > > >> > > > Fixes: ("device property: Align return codes of __acpi_node_get_property_reference") >> > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> >> > > > Tested-by: Hyungwoo Yang <hyungwoo.yang@intel.com> >> > > > --- >> > > > Hi Rafael, >> > > > >> > > > Unfortunately the patch I posted the previous time to remedy the issue >> > > > ("device property: Align return codes of >> > > > _acpi_node_get_property_reference") did not fully fix the issue. >> > > >> > > OK, thanks for letting me know, but why didn't it? >> > >> > My testing appears to have been more limited than I thought, Hyungwoo later >> > on found this out. (Reported-by: Hyungwoo... would be appropriate, I'll add >> > that the next time.) >> > >> > > >> > > > drivers/acpi/property.c | 20 +++++++++++++++----- >> > > > 1 file changed, 15 insertions(+), 5 deletions(-) >> > > > >> > > > diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c >> > > > index 5a8ac5e1081b..8c28c516e7ec 100644 >> > > > --- a/drivers/acpi/property.c >> > > > +++ b/drivers/acpi/property.c >> > > > @@ -592,8 +592,16 @@ int __acpi_node_get_property_reference(const struct fwnode_handle *fwnode, >> > > > return -ENOENT; >> > > > >> > > > ret = acpi_data_get_property(data, propname, ACPI_TYPE_ANY, &obj); >> > > > - if (ret) >> > > > - return ret; >> > > > + switch (ret) { >> > > > + case -EINVAL: >> > > > + return -ENOENT; >> > > > + case -EPROTO: >> > > > + return -EINVAL; >> > > > + default: >> > > > + if (ret) >> > > > + return ret; >> > > > + break; >> > > > + } >> > > >> > > To be clear, I'm not going to apply anything like the above. >> > >> > On exactly what grounds? You don't like the combination of switch and if, >> > or because of the return values themselves? Or something else? >> >> I just don't like changing error codes into different ones on the fly like >> this. It always indicates some bad design somewhere and this particular >> piece of code just goes over the top with that IMO. > > acpi_data_get_property() is used in quite a few places and the error codes > it returns are more or less used as such elsewhere. > > The ACPI and OF frameworks tend to use slightly different error codes to > signal various error conditions. In this case, the OF archetype of > fwnode_property_get_reference_args(), of_parse_phandle_with_args(), also > uses different error codes than the rest of the OF framework. We decided to > align the error codes with the OF framework, as has been done with the rest > of the functions. Therefore the error codes returned by ACPI functions need > to be converted to what is expected to be seen on the device property API > (or OF API). > > My original proposal was to switch between the conventions in the ACPI > fwnode callback acpi_fwnode_get_reference_args() but based on the review > comments I made the changes to __acpi_node_get_property_reference() > instead. Which is where we're now. > > fwnode_property_get_reference_args() (as does of_parse_phandle_with_args()) > returns -ENOENT on > > - no reference found at given index (but entry exists), > > - no index exists in property or > > - the property does not exist. > > Originally __acpi_node_get_property_reference() used -ENOENT, -ENODATA and > -EINVAL respectively. Consequenty -EINVAL returned by > acpi_data_get_property(), called by __acpi_node_get_property_reference() > needs to be turned to -ENOENT instead. Likewise -EPROTO changes to -EINVAL, > as fwnode_property_get_reference_args() isn't expected to return -EPROTO. Can we just return -EINVAL (or whatever generic error) on all errors from acpi_data_get_property() and avoid this super-ugliness, please? Or if some caller really cares, you can do if (ret) return ret == -EINVAL ? -ENOENT : -EINVAL; which at least is one line of code instead of several. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Rafael, On Fri, Oct 06, 2017 at 12:22:54AM +0200, Rafael J. Wysocki wrote: > On Fri, Oct 6, 2017 at 12:04 AM, Sakari Ailus > <sakari.ailus@linux.intel.com> wrote: > > Hi Rafael, > > > > On Thu, Oct 05, 2017 at 05:30:37PM +0200, Rafael J. Wysocki wrote: > >> On Thursday, October 5, 2017 4:01:48 PM CEST Sakari Ailus wrote: > >> > Hi Rafael, > >> > > >> > On Thu, Oct 05, 2017 at 02:59:45PM +0200, Rafael J. Wysocki wrote: > >> > > On Thursday, October 5, 2017 8:04:24 AM CEST Sakari Ailus wrote: > >> > > > Fix more return codes for device property: Align return codes of > >> > > > __acpi_node_get_property_reference. In particular what was missed > >> > > > previously: > >> > > > > >> > > > -EPROTO could be returned in certain cases, now -EINVAL; > >> > > > -EINVAL was returned if the property was not found, now -ENOENT; > >> > > > -EINVAL was returned also if the index was higher than the number of > >> > > > entries in a package, now -ENOENT. > >> > > > > >> > > > Fixes: ("device property: Align return codes of __acpi_node_get_property_reference") > >> > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > >> > > > Tested-by: Hyungwoo Yang <hyungwoo.yang@intel.com> > >> > > > --- > >> > > > Hi Rafael, > >> > > > > >> > > > Unfortunately the patch I posted the previous time to remedy the issue > >> > > > ("device property: Align return codes of > >> > > > _acpi_node_get_property_reference") did not fully fix the issue. > >> > > > >> > > OK, thanks for letting me know, but why didn't it? > >> > > >> > My testing appears to have been more limited than I thought, Hyungwoo later > >> > on found this out. (Reported-by: Hyungwoo... would be appropriate, I'll add > >> > that the next time.) > >> > > >> > > > >> > > > drivers/acpi/property.c | 20 +++++++++++++++----- > >> > > > 1 file changed, 15 insertions(+), 5 deletions(-) > >> > > > > >> > > > diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c > >> > > > index 5a8ac5e1081b..8c28c516e7ec 100644 > >> > > > --- a/drivers/acpi/property.c > >> > > > +++ b/drivers/acpi/property.c > >> > > > @@ -592,8 +592,16 @@ int __acpi_node_get_property_reference(const struct fwnode_handle *fwnode, > >> > > > return -ENOENT; > >> > > > > >> > > > ret = acpi_data_get_property(data, propname, ACPI_TYPE_ANY, &obj); > >> > > > - if (ret) > >> > > > - return ret; > >> > > > + switch (ret) { > >> > > > + case -EINVAL: > >> > > > + return -ENOENT; > >> > > > + case -EPROTO: > >> > > > + return -EINVAL; > >> > > > + default: > >> > > > + if (ret) > >> > > > + return ret; > >> > > > + break; > >> > > > + } > >> > > > >> > > To be clear, I'm not going to apply anything like the above. > >> > > >> > On exactly what grounds? You don't like the combination of switch and if, > >> > or because of the return values themselves? Or something else? > >> > >> I just don't like changing error codes into different ones on the fly like > >> this. It always indicates some bad design somewhere and this particular > >> piece of code just goes over the top with that IMO. > > > > acpi_data_get_property() is used in quite a few places and the error codes > > it returns are more or less used as such elsewhere. > > > > The ACPI and OF frameworks tend to use slightly different error codes to > > signal various error conditions. In this case, the OF archetype of > > fwnode_property_get_reference_args(), of_parse_phandle_with_args(), also > > uses different error codes than the rest of the OF framework. We decided to > > align the error codes with the OF framework, as has been done with the rest > > of the functions. Therefore the error codes returned by ACPI functions need > > to be converted to what is expected to be seen on the device property API > > (or OF API). > > > > My original proposal was to switch between the conventions in the ACPI > > fwnode callback acpi_fwnode_get_reference_args() but based on the review > > comments I made the changes to __acpi_node_get_property_reference() > > instead. Which is where we're now. > > > > fwnode_property_get_reference_args() (as does of_parse_phandle_with_args()) > > returns -ENOENT on > > > > - no reference found at given index (but entry exists), > > > > - no index exists in property or > > > > - the property does not exist. > > > > Originally __acpi_node_get_property_reference() used -ENOENT, -ENODATA and > > -EINVAL respectively. Consequenty -EINVAL returned by > > acpi_data_get_property(), called by __acpi_node_get_property_reference() > > needs to be turned to -ENOENT instead. Likewise -EPROTO changes to -EINVAL, > > as fwnode_property_get_reference_args() isn't expected to return -EPROTO. > > Can we just return -EINVAL (or whatever generic error) on all errors > from acpi_data_get_property() and avoid this super-ugliness, please? acpi_data_get_property() is used by a number of functions and those functions have well defined error codes for various conditions. In particular, these conditions are more finely separated for use in other fwnode_property_read_*() functions than they are for fwnode_property_get_reference_args(). So we can't change what acpi_data_get_property() returns without breaking the rest. If you want it to be clean, then the best option to implement it is in the fwnode property framework which uses OF-like error codes, not ACPI-like. > > Or if some caller really cares, you can do > > if (ret) > return ret == -EINVAL ? -ENOENT : -EINVAL; > > which at least is one line of code instead of several. Ah, acpi_data_get_property() only returns the two. Yes, I can change the implementation to that. I'll submit v2.
On Thu, Oct 05, 2017 at 10:40:42PM +0200, Rafael J. Wysocki wrote: > On Thu, Oct 5, 2017 at 4:01 PM, Sakari Ailus > <sakari.ailus@linux.intel.com> wrote: > > Hi Rafael, > > > > On Thu, Oct 05, 2017 at 02:59:45PM +0200, Rafael J. Wysocki wrote: > >> On Thursday, October 5, 2017 8:04:24 AM CEST Sakari Ailus wrote: > >> > Fix more return codes for device property: Align return codes of > >> > __acpi_node_get_property_reference. In particular what was missed > >> > previously: > >> > > >> > -EPROTO could be returned in certain cases, now -EINVAL; > >> > -EINVAL was returned if the property was not found, now -ENOENT; > >> > -EINVAL was returned also if the index was higher than the number of > >> > entries in a package, now -ENOENT. > >> > > >> > Fixes: ("device property: Align return codes of __acpi_node_get_property_reference") > >> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > >> > Tested-by: Hyungwoo Yang <hyungwoo.yang@intel.com> > >> > --- > >> > Hi Rafael, > >> > > >> > Unfortunately the patch I posted the previous time to remedy the issue > >> > ("device property: Align return codes of > >> > _acpi_node_get_property_reference") did not fully fix the issue. > >> > >> OK, thanks for letting me know, but why didn't it? > > > > My testing appears to have been more limited than I thought, Hyungwoo later > > on found this out. (Reported-by: Hyungwoo... would be appropriate, I'll add > > that the next time.) > > Well, you're not saying what exactly still doesn't work with the > previous patch applied which really is what I was asking about. Hmm. The subject says it but I'll add it to the commit message as well.
diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c index 5a8ac5e1081b..8c28c516e7ec 100644 --- a/drivers/acpi/property.c +++ b/drivers/acpi/property.c @@ -592,8 +592,16 @@ int __acpi_node_get_property_reference(const struct fwnode_handle *fwnode, return -ENOENT; ret = acpi_data_get_property(data, propname, ACPI_TYPE_ANY, &obj); - if (ret) - return ret; + switch (ret) { + case -EINVAL: + return -ENOENT; + case -EPROTO: + return -EINVAL; + default: + if (ret) + return ret; + break; + } /* * The simplest case is when the value is a single reference. Just @@ -605,7 +613,7 @@ int __acpi_node_get_property_reference(const struct fwnode_handle *fwnode, ret = acpi_bus_get_device(obj->reference.handle, &device); if (ret) - return ret; + return ret == -ENODEV ? -EINVAL : ret; args->adev = device; args->nargs = 0; @@ -621,8 +629,10 @@ int __acpi_node_get_property_reference(const struct fwnode_handle *fwnode, * The index argument is then used to determine which reference * the caller wants (along with the arguments). */ - if (obj->type != ACPI_TYPE_PACKAGE || index >= obj->package.count) - return -EPROTO; + if (obj->type != ACPI_TYPE_PACKAGE) + return -EINVAL; + if (index >= obj->package.count) + return -ENOENT; element = obj->package.elements; end = element + obj->package.count;
Fix more return codes for device property: Align return codes of __acpi_node_get_property_reference. In particular what was missed previously: -EPROTO could be returned in certain cases, now -EINVAL; -EINVAL was returned if the property was not found, now -ENOENT; -EINVAL was returned also if the index was higher than the number of entries in a package, now -ENOENT. Fixes: ("device property: Align return codes of __acpi_node_get_property_reference") Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> Tested-by: Hyungwoo Yang <hyungwoo.yang@intel.com> --- Hi Rafael, Unfortunately the patch I posted the previous time to remedy the issue ("device property: Align return codes of _acpi_node_get_property_reference") did not fully fix the issue. drivers/acpi/property.c | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-)