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@xxxxxxxxxxxxxxx>
> > > > Tested-by: Hyungwoo Yang <hyungwoo.yang@xxxxxxxxx>
> > > > ---
> > > > 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@xxxxxxxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
