Re: [PATCH 5/5] PCI: iproc: Properly handle optional PHYs

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 5568 bytes --]

On Thu, Aug 29, 2019 at 01:46:03PM +0200, Thierry Reding wrote:
> On Thu, Aug 29, 2019 at 12:17:29PM +0100, Mark Brown wrote:

> > Why would we return NULL?  The caller is going to have to check the
> > error code anyway since they need to handle -EPROBE_DEFER and NULL is
> > never a valid thing to use with the regulator API.

> I think the idea is that consumers would do something like this:

> 	supply = regulator_get_optional(dev, "foo");
> 	if (IS_ERR(supply)) {
> 		/* -EPROBE_DEFER handled as part of this */
> 		return PTR_ERR(supply);
> 	}

> 	/*
> 	 * Optional supply is NULL here, making it "easier" to check
> 	 * whether or not to use it.
> 	 */

> I suppose checking using IS_ERR() is equally simple, so this mostly
> boils down to taste...

Or setting a flag saying which mode to drive the chip in for example.

> The PHY subsystem, for instance, uses a similar approach but it goes one
> step further. All APIs can take NULL as their struct phy * argument and
> return success in that case, which makes it really trivial to deal with
> optional PHYs. You really don't have to care about them at all after you
> get NULL from phy_get_optional().

That case really doesn't exist for the regulator API, that's the case
which is handled by the dummy regulator in the regulator API - it really
is rare to see devices that operate without power.  I suspect the PHY
case is similar in that there always is a PHY of some kind.  If a
consumer can be written like that then it just shouldn't be using
regulator_get_optional() in the first place.

> > > Looking at some of the consumer drivers I can see that lots of them don't
> > > correctly handle the return value of regulator_get_optional:

> > This API is *really* commonly abused, especially in the graphics
> > subsystem which for some reason has lots of users even though I don't
> > think I've ever seen a sensible use of the API there.  As far as I can
> > tell the graphics subsystem mostly suffers from people cut'n'pasting
> > from the Qualcomm BSP which is full of really bad practice.  It's really
> > frustrating since I will intermittently go through and clean things up
> > but the uses just keep coming back into the subsystem.

> The intention here is to make it more difficult to abuse. Obviously if
> people keep copying abuses from one driver to another that's a different
> story. But if there was no way to abuse the API and if it automatically
> did the right thing, even copy/paste abuse would be difficult to pull
> off.

I fail to see how returning NULL would change anything with regard to
the API being abused, if anything I think it'd make it more likely to
have people write things that break for probe deferral since it's not
reminding people about error codes (that's very marginal though).

> > No, they'd all still be broken for probe deferral (which is commonly
> > caused by off-chip devices) and they'd crash as soon as they try to call
> > any further regulator API functions.

> I think what Andrew was suggesting here was to make it easier for people
> to determine whether or not an optional regulator was in fact requested
> successfully or not. Many consumers already set the optional supply to
> NULL and then check for that before calling any regulator API.

I think that really is very marginal.

> If regulator_get_optional() returned NULL for absent optional supplies,
> this could be unified across all drivers. And it would allow treating
> NULL regulators special, if that's something you'd be willing to do.

Not really, no.  What we're doing at the minute at least mitigates
against the problems we used to have with people just not handling
errors at all and having the dummy regulator gives us some opportunity
to do checks on API usage in the consumers that would otherwise not get
run which helps robustness for the systems where there are real,
controllable regulators providing those supplies.

> In either case, the number of abuses shows that people clearly don't
> understand how to use this. So there are two options: a) fix abuse every
> time we come across it or b) try to change the API to make it more
> difficult to abuse.

I don't think there's much that can be done to avoid abuse of the API, 
I've thought of things like having a #define around the prototype for
"yes I really meant to use this" which consumers would have to define in
an effort to try to flag up to developers and reviewers that it's not
normal.

> > > I guess I'm looking here for something that can simplify consumer error
> > > handling - it's easy to get wrong and it seems that many drivers may be wrong.

> > The overwhelming majority of the users just shouldn't be using this API.
> > If there weren't devices that absolutely *need* this API it wouldn't be
> > there in the first place since it seems like a magent for bad code, it's
> > so disappointing how bad the majority of the consumer code is.

> Yeah, I guess in many cases this API is used as a cheap way to model
> always-on, fixed-voltage regulators. It's pretty hard to change those
> after the fact, though, because they're usually happening as part of
> device tree bindings implementation, so by the time we notice them,
> they've often become ABI...

I don't follow here?  Such users are going to be the common case for the
regulator API and shouldn't have any problems using normal regulator_get().  
When I say users shouldn't be using this API I mean _get_optional()
specifically.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]