Subject: Re: [PATCH 1/5] regulator: core: Resolve supply earlier
To: Mark Brown <broonie@kernel.org>
References: <1460038959-21592-1-git-send-email-thierry.reding@gmail.com>
 <20160411140300.GH3351@sirena.org.uk> <20160411141101.GB27400@ulmo.ba.sec>
 <20160411141621.GI3351@sirena.org.uk> <5716059B.3080503@nvidia.com>
 <20160419154059.GW3217@sirena.org.uk>
CC: Thierry Reding <thierry.reding@gmail.com>,
        Liam Girdwood <lgirdwood@gmail.com>, <linux-kernel@vger.kernel.org>
From: Jon Hunter <jonathanh@nvidia.com>
Message-ID: <57165857.5050809@nvidia.com>
Date: Tue, 19 Apr 2016 17:09:59 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101
 Thunderbird/38.6.0
MIME-Version: 1.0
In-Reply-To: <20160419154059.GW3217@sirena.org.uk>
Content-Type: text/plain; charset="windows-1252"
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1355
Lines: 46


On 19/04/16 16:40, Mark Brown wrote:
> * PGP Signed by an unknown key
> 
> On Tue, Apr 19, 2016 at 11:16:59AM +0100, Jon Hunter wrote:
> 
>> So the following seems to work, but only item I am uncertain about
>> is if it is ok to move the mutex_lock to after the
>> machine_set_constraints()?
> 
> We definitely don't need the list to apply constraints to a single
> regulator.
> 
>> +	mutex_lock(&regulator_list_mutex);
>> +
>>  	ret = device_register(&rdev->dev);
>>  	if (ret != 0) {
>>  		put_device(&rdev->dev);
>> +		mutex_unlock(&regulator_list_mutex);
>>  		goto wash;
>>  	}
> 
> This is *really* weird.  Why would we need the list lock to do a
> device_register()?  

The device_register() is going to add the regulator to the 
regulator class list and this means that after this, someone
could look up that regulator via ...

 static struct regulator_dev *of_find_regulator_by_node(struct device_node *np)
 {
         struct device *dev;
 
         dev = class_find_device(&regulator_class, NULL, np, of_node_match);
 
        return dev ? dev_to_rdev(dev) : NULL;
 }

So I did not think that we would want someone to be able to 
look-up the regulator via of_find_regulator_by_node() until
it had been registered successfully. In fact I believe that
not locking around device_register() was causing some crashes
when I was testing.

Cheers
Jon