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

From: Mark Brown <broonie@kernel.org>
To: Andrew Murray <andrew.murray@arm.com>
Cc: Thierry Reding <thierry.reding@gmail.com>,
	Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
	Bjorn Helgaas <bhelgaas@google.com>,
	linux-pci@vger.kernel.org, Ray Jui <rjui@broadcom.com>,
	Scott Branden <sbranden@broadcom.com>,
	bcm-kernel-feedback-list@broadcom.com,
	Liam Girdwood <lgirdwood@gmail.com>
Subject: Re: [PATCH 5/5] PCI: iproc: Properly handle optional PHYs
Date: Thu, 29 Aug 2019 12:17:29 +0100	[thread overview]
Message-ID: <20190829111728.GC4118@sirena.co.uk> (raw)
In-Reply-To: <20190829100933.GH14582@e119886-lin.cambridge.arm.com>

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

On Thu, Aug 29, 2019 at 11:09:34AM +0100, Andrew Murray wrote:

> But why do we return -ENODEV and not NULL for OPTIONAL_GET?

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.

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

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

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

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