From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1762041AbdAINhh (ORCPT ); Mon, 9 Jan 2017 08:37:37 -0500 Received: from hqemgate14.nvidia.com ([216.228.121.143]:14790 "EHLO hqemgate14.nvidia.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752908AbdAINh3 (ORCPT ); Mon, 9 Jan 2017 08:37:29 -0500 X-PGP-Universal: processed; by hqnvupgp07.nvidia.com on Sun, 08 Jan 2017 17:33:30 -0800 Subject: Re: [PATCH] regulator: core: Unset supplies when regulators are unregistered To: Mark Brown References: <1483703678-19575-1-git-send-email-jonathanh@nvidia.com> <20170106182953.ayvailx5sxtiyzwe@sirena.org.uk> CC: Liam Girdwood , , , Javier Martinez Canillas From: Jon Hunter Message-ID: <084533f5-d611-59ac-7329-657b0523bdb3@nvidia.com> Date: Mon, 9 Jan 2017 13:37:18 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.5.1 MIME-Version: 1.0 In-Reply-To: <20170106182953.ayvailx5sxtiyzwe@sirena.org.uk> X-Originating-IP: [10.26.11.200] X-ClientProxiedBy: DRUKMAIL102.nvidia.com (10.25.59.20) To UKMAIL101.nvidia.com (10.26.138.13) Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 06/01/17 18:29, Mark Brown wrote: > * PGP Signed by an unknown key > > On Fri, Jan 06, 2017 at 11:54:38AM +0000, Jon Hunter wrote: > >> There is a case where a child regulator is registered before its supply >> and when the supply is registered successfully, the supply for the child >> is then set. Although, this in itself is not a problem, a problem arises >> when the supply is then unregistered again, due to the parent device >> being probed deferred, after successfully registering the supply. This >> leaves the regulator with an invalid reference to supply and can >> eventually result in a kernel panic. > > Why is a parent device doing this? This doesn't seem like safe or > helpful behaviour and with probe deferral we'd generally expect the > device to acquire resources before it starts making use of them. It is not really the parent that is doing this. The child regulator in this case is physically a separate regulator from the main PMIC device that is registering the supply regulators. The PMIC is registering N regulators and after registering some successfully one fails because it's supply is not found and the regulator core returns -EPROBE_DEFER. Normally not finding supply would not fail but in this case it is necessary because the regulator is in bypass and the supply is needed to get the voltage. Actually, this particular regulator was the cause of some other issues in the past where we needed to fix-up the handling of regulators in bypass during registration. See commit 45389c47526d ('regulator: core: Add early supply resolution for regulators'). The real problem is that after one of the PMIC's regulators fails to register, by then one of the PMIC's other regulators (that has already been registered) has been added as a supply for another external regulator. >> Addtionally, even in the normal case when a regulator is unregistered, >> by unloading a driver, if the regulator happens to be a supply for >> another regulator it is not removed. > > We can't completely stop people doing this but we do make fairly strong > efforts to stop people pulling in use devices. Right, but before removing/unregistering a regulator today, we never check to see if it is the supply for any other regulator. >> Fix this by scanning all the registered regulators when a regulator is >> removed and remove it as a supply to any other regulator. There is a >> possibility that a child regulator is in use when the supply is removed >> and so WARN if this happens. > > This seems like storing up trouble for the future, we'll end up with > live child devices with parents that weren't around or being refcounted > through some of the lifetime of the device which will doubtless come > back and bite us later. Yes I am not completely happy. Maybe we should wait for the parent device to be bound before allowing any of its's regulators to be added as supplies for other regulators? I gave the following a quick test and this seems to work too ... diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c index 04baac9a165b..e17915861a54 100644 --- a/drivers/regulator/core.c +++ b/drivers/regulator/core.c @@ -1553,6 +1553,11 @@ static int regulator_resolve_supply(struct regulator_dev *rdev) } } + if (!device_is_bound(r->dev.parent)) { + put_device(&r->dev); + return -EPROBE_DEFER; + } + /* Recursively resolve the supply of the supply */ ret = regulator_resolve_supply(r); if (ret < 0) { The above prevents anyone from using a regulator as a supply if the parent has not been bound yet. Therefore, if one of the parent's regulators fails to register, after some have already been successfully registered, there is no chance any of the successfully added regulators will have been already added as a supply to some other regulator in the system. Hope that's clear! >> Note that the debugfs node for the regulator supply must be freed before >> the debugfs node for the regulator_dev and so ensure the regulator_dev >> debugfs node is freed after the any supplies have been removed. > > Please don't mix different changes into one commit, as covered in > SubmittingPatches please send one patch per change. This makes things > easier to Sorry, may be I worded this badly, however, it is a consequence of this particular change and otherwise it would not be needed. Cheers Jon -- nvpublic