Re: [PATCH] PCI: rockchip-dwc: Potential error pointer dereference in probe

From: Mark Brown <broonie@kernel.org>
To: Robin Murphy <robin.murphy@arm.com>
Cc: "Dan Carpenter" <dan.carpenter@oracle.com>,
	"Lorenzo Pieralisi" <lorenzo.pieralisi@arm.com>,
	"Simon Xue" <xxm@rock-chips.com>, "Rob Herring" <robh@kernel.org>,
	"Krzysztof Wilczyński" <kw@linux.com>,
	"Bjorn Helgaas" <bhelgaas@google.com>,
	"Heiko Stuebner" <heiko@sntech.de>,
	"Liam Girdwood" <lgirdwood@gmail.com>,
	"Kever Yang" <kever.yang@rock-chips.com>,
	"Shawn Lin" <shawn.lin@rock-chips.com>,
	linux-pci@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-rockchip@lists.infradead.org,
	kernel-janitors@vger.kernel.org
Subject: Re: [PATCH] PCI: rockchip-dwc: Potential error pointer dereference in probe
Date: Fri, 13 Aug 2021 16:30:43 +0100	[thread overview]
Message-ID: <20210813153043.GB5209@sirena.org.uk> (raw)
In-Reply-To: <566c26bb-c488-8c86-f47e-6a748b9b6c77@arm.com>

[-- Attachment #1.1: Type: text/plain, Size: 1667 bytes --]

On Fri, Aug 13, 2021 at 04:00:45PM +0100, Robin Murphy wrote:
> On 2021-08-13 15:32, Mark Brown wrote:

> > It also encourages *really* bad practice with error handling, and in
> > general there are few use cases for optional regulators where there's
> > not some other actions that need to be taken in the case where the
> > supply isn't there (elimintating some operating points or features,
> > reconfiguring power internally and so on).  If we genuninely don't need
> > to do anything special one wonders why we're trying to turn the power on
> > in the first place.

> Sure, once you get into it, regulators are arguably a rather deeper area
> than GPIOs, so in terms of the NULL-safe aspect anything beyond
> enable/disable - for the sake of keeping trivial usage simple - would be
> pretty questionable for sure.

enable and disable are among my biggest worries frankly, if the device
was supposed to have power and that didn't work then there's probably
some kind of issue.

> A lot of the usage of regulator_get_optional() seems to be just making sure
> some external thing is powered between probe() and remove() if it's not
> hard-wired already, so maybe something like a
> devm_regulator_get_optional_enabled() could be an answer to that argument
> without even touching the underlying API.

I'm fairly sure a lot of the regulator_get_optional() usage is just
abuse of the API, every so often I get fed up enough to send patches
converting users that look like you describe to normal regulator API
calls.  People really don't get the dummy regulator stuff and seem to
think that _get_optional() is what you use to avoid writing any error
handling code :(

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

[-- Attachment #2: Type: text/plain, Size: 170 bytes --]

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip