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

[Thierry Reding <thierry.reding@gmail.com>]

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

On Thu, Aug 29, 2019 at 11:09:34AM +0100, Andrew Murray wrote:
> On Wed, Aug 28, 2019 at 10:49:01PM +0100, Mark Brown wrote:
> > On Wed, Aug 28, 2019 at 10:26:55PM +0100, Andrew Murray wrote:
> > 
> > > I initially thought that you forgot to check for -ENODEV - though I can see
> > > that the implementation of devm_phy_optional_get very helpfully does this for
> > > us and returns NULL instead of an error.
> > 
> > > What is also confusing is that devm_regulator_get_optional, despite its
> > > _optional suffix doesn't do this and returns an error. I wonder if
> > > devm_phy_optional_get should be changed to return NULL instead of an error
> > > instead of -ENODEV. I've copied Liam/Mark for feedback.
> > 
> > The regulator API has an assumption that people will write bad DTs and
> > not describe all the regulators in the system, this is especially likely
> > in cases where consumer drivers initially don't have regulator support
> > and then get it added since people often only describe supplies actively
> > used by drivers.  In order to handle this gracefully the API will
> > substitute in a dummy regulator if it sees that the regulator just isn't
> > drescribed in the system but a consumer requests it, this will ensure
> > that for most simple uses the consumer will function fine even if the DT
> > is not fully described.  Since most devices won't physically work if
> > some of their supplies are missing this is a good default assumption.  
> 
> Right, if I understand correctly this is the behaviour when regulator_get
> is called (e.g. NORMAL_GET) - you get a dummy instead of an error.
> 
> > 
> > If a consumer could genuinely have some missing supplies (some devices
> > do support this for various reasons) then this support would mean that
> > the consumer would have to have some extra property to say that the
> > regulator is intentionally missing which would be bad.  Instead what we
> > do is let the consumer say that real systems could actually be missing
> > the regulator and that the dummy shouldn't be used so that the consumer
> > can handle this.
> 
> And if I understand correctly this is the behaviour when
> regulator_get_optional is called (e.g. OPTIONAL_GET) - you get -ENODEV
> instead of a dummy.
> 
> But why do we return -ENODEV and not NULL for OPTIONAL_GET?
> 
> 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:
> 
>  - some fail their probes and return upon IS_ERR(ret) - for example even
>    if -ENODEV is returned.
> 
>  - some don't fail their probes and assume the regulator isn't present upon
>    IS_ERR(ret) - yet this may not be correct as the regulator may be present
>    but -ENOMEM was returned.
> 
> Given that a common pattern is to set a consumer regulator pointer to NULL
> upon -ENODEV - if regulator_get_optional did this for them, then it would
> be more difficult for consumer drivers to get the error handling wrong and
> would remove some consumer boiler plate code.
> 
> (Of course some consumers won't set a regulator pointer to NULL and instead
> test it against IS_ERR instead of NULL everywhere (IS_ERR(NULL) is false) -
> but such a change may be a reason to not use IS_ERR everywhere).
> 
> As I understand, if a consumer wants to fail upon an absent regulator
> it seems the only way they can do this is call regulator_get_optional (which
> seems odd) and test for -ENODEV. I'm not sure if there is actually a use-case
> for this.
> 
> 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.

Agreed. However, this requires a thorough audit of all callers of
regulator_get_optional() to make sure they behave in a sane way. To
further complicate things, unless we want to convert all ~100 callers
in a single patch we need to convert all of them to set the regulator
pointer to NULL on -ENODEV. After that we can make the change to
regulator_get_optional() and only then can we remove the now obsolete
boilerplate from those ~100 callers. Not impossible, but pretty time-
consuming.

While at it, we could also add optional variants to some of the
*phy*_get() functions to convert those as well. Currently there's only
optional variants for phy_get() and devm_phy_get(), but a bunch of
drivers use of_phy_get() or of_phy_get_by_index(). Though especially the
latter isn't very common with optional PHYs, I think.

I also noticed a slightly similar pattern for GPIOs. Perhaps this would
be a good task for someone with good semantic patch skills. Or perhaps
something to add to the janitors' TODO list? Not sure if that's still a
thing, though.

Thierry

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