On Thu, Aug 29, 2019 at 02:03:23PM +0100, Mark Brown wrote:
> 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.
One recent example, which is actually what spawned this series of
cleanups, that comes to mind here is the case where we need regulators
to supply power to a PCI device. The PCI device that consumes power is
itself not listed in the device tree, because it is usually enumerated
via the PCI bus. However, we still have to provide power to a PCI slot
to make sure the device powers up and can be enumerated in the first
place. For this particular case it's pretty obvious that the supplies
are in fact required.
However, the same PCI controller can also be used with onboard devices
where no standard 3.3 V and 12 V need to be supplied (as in the PCIe
slot case above). Instead there are usually different power supplies
to power the device. Also, since these supplies are usually required to
bring the device up and make it detectable on the PCI bus, these
supplies are typically always on. Also, since the PCI controller is not
the consumer of the power supplies, it's impossible to specify which
supplies exactly are needed by the device (they'd have to be described
by a binding for these devices, but drivers couldn't be requesting them
because without the supplies being enabled the device would never show
up on the PCI bus and the driver would never bind).
Do you think those 3.3 V and 12 V regulators would qualify for use of
the regulator_get_optional() API? Because in the case where the PCIe
controller doesn't drive a PCIe slot, the 3.3 V and 12 V supplies really
are not there.
> > > > 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).
People would of course still need to check the return value for errors.
Returning NULL would only make sure that if there's really no regulator
there's a standard way to signal that. And drivers could always use
similar code patterns to deal with optional regulators. Right now there
are two patterns: set regulator to NULL if the regulator is not
available (and use !supply checks before calling regulator APIs) or
leave the regulator set to whatever error pointer was returned (and use
!IS_ERR() checks before calling regulator APIs).
In many cases above, the drivers will continue without a regulator
irrespective of the error code returned. If we return NULL in exactly
only the case where the regulator is not there and an ERR_PTR()
otherwise, it becomes much clearer that all errors should just be
propagated to the caller (including -EPROBE_DEFER) and otherwise we can
continue with appropriate NULL checks.
Again, yeah, this might be marginal.
> > > 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.
True, even if the regulator is specified as optional in the bindings,
using regulator_get() would effectively make it optional for the driver
given that it returns the dummy.
Thierry
Attachment:
signature.asc
Description: PGP signature
