[PATCH 1/1] device properties: Fix return codes for __acpi_node_get_property_reference

[PATCH 1/1] device properties: Fix return codes for __acpi_node_get_property_reference

[Sakari Ailus]

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(-)

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;
-- 
2.11.0

Re: [PATCH 1/1] device properties: Fix return codes for __acpi_node_get_property_reference

[Rafael J. Wysocki]

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;
> 

Re: [PATCH 1/1] device properties: Fix return codes for __acpi_node_get_property_reference

[Sakari Ailus]

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;
> > 
> 
> 

-- 
Regards,

Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH 1/1] device properties: Fix return codes for __acpi_node_get_property_reference

[Rafael J. Wysocki]

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

Re: [PATCH 1/1] device properties: Fix return codes for __acpi_node_get_property_reference

[Rafael J. Wysocki]

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

Re: [PATCH 1/1] device properties: Fix return codes for __acpi_node_get_property_reference

[Sakari Ailus]

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.

-- 
Regards,

Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH 1/1] device properties: Fix return codes for __acpi_node_get_property_reference

[Rafael J. Wysocki]

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

Re: [PATCH 1/1] device properties: Fix return codes for __acpi_node_get_property_reference

[Sakari Ailus]

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.

-- 
Kind regards,

Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH 1/1] device properties: Fix return codes for __acpi_node_get_property_reference

[Sakari Ailus]

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.

-- 
Sakari Ailus
sakari.ailus@linux.intel.com