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@xxxxxxxxxxxxxxx> 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@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.
>
> 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@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
