Re: [PATCH v5 1/4] device property: Allow error pointer to be passed to fwnode APIs

[Andy Shevchenko]

On Thu, Apr 07, 2022 at 01:19:44PM +0300, Andy Shevchenko wrote:
> On Wed, Apr 06, 2022 at 08:05:23PM +0200, Michael Walle wrote:

...

> > > +	if (IS_ERR_OR_NULL(fwnode))
> > > +		return -ENOENT;
> > > +
> > >  	ret = fwnode_call_int_op(fwnode, get_reference_args, prop, nargs_prop,
> > >  				 nargs, index, args);
> > > +	if (ret == 0)
> > > +		return ret;
> > > 
> > > -	if (ret < 0 && !IS_ERR_OR_NULL(fwnode) &&
> > > -	    !IS_ERR_OR_NULL(fwnode->secondary))
> > > -		ret = fwnode_call_int_op(fwnode->secondary, get_reference_args,
> > > -					 prop, nargs_prop, nargs, index, args);
> > > +	if (IS_ERR_OR_NULL(fwnode->secondary))
> > > +		return -ENOENT;
> > 
> > Doesn't this mean you overwrite any return code != 0 with -ENOENT?
> > Is this intended?
> 
> Indeed, it would shadow the error code.

I was thinking more on this and am not sure about the best approach here.
On one hand in the original code this returns the actual error code from
the call against primary fwnode. But it can be at least -ENOENT or -EINVAL.

But when we check the secondary fwnode we want to have understanding that it's
secondary fwnode which has not been found, but this requires to have a good
distinguishing between error codes from the callback.

That said, the error codes convention of ->get_reference_args() simply
sucks. Sakari, do you have it on your TODO to fix this mess out, if it's
even feasible?

To be on safest side, I will change as suggested in previous mail (see below)
so it won't have impact on -EINVAL case.

> So, it should go with
> 
> 	if (IS_ERR_OR_NULL(fwnode->secondary))
> 		return ret;
> 
> then.

-- 
With Best Regards,
Andy Shevchenko