Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1382365 > unrolled thread

Re: [PATCH 1/5] regulator: core: Resolve supply earlier

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 1/5] regulator: core: Resolve supply earlier Jon Hunter <jonathanh@nvidia.com> - 2016-04-19 12:20 +0200
    Re: [PATCH 1/5] regulator: core: Resolve supply earlier Thierry Reding <thierry.reding@gmail.com> - 2016-04-19 13:10 +0200
    Re: [PATCH 1/5] regulator: core: Resolve supply earlier Mark Brown <broonie@kernel.org> - 2016-04-19 17:50 +0200
      Re: [PATCH 1/5] regulator: core: Resolve supply earlier Jon Hunter <jonathanh@nvidia.com> - 2016-04-19 18:20 +0200
        Re: [PATCH 1/5] regulator: core: Resolve supply earlier Mark Brown <broonie@kernel.org> - 2016-04-20 17:30 +0200
          Re: [PATCH 1/5] regulator: core: Resolve supply earlier Jon Hunter <jonathanh@nvidia.com> - 2016-04-21 17:40 +0200

#1382365 — Re: [PATCH 1/5] regulator: core: Resolve supply earlier

On 11/04/16 15:16, Mark Brown wrote:
> * PGP Signed by an unknown key
> 
> On Mon, Apr 11, 2016 at 04:11:01PM +0200, Thierry Reding wrote:
>> On Mon, Apr 11, 2016 at 03:03:00PM +0100, Mark Brown wrote:
> 
>>> This shouldn't be a hard dependency: most regulators won't be in bypass
>>> mode or otherwise depend on their parents enough to need this.
> 
>> I had initially proposed to resolve the supply only when necessary
>> during regulator_get_voltage() when checking for bypass, perhaps that
>> would after all be more appropriate here?
> 
> Yes, that had been what I'd expected.

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()?

Jon

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 61d3918f329e..742d10371e2d 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -3126,8 +3126,13 @@ static int _regulator_get_voltage(struct regulator_dev *rdev)
 			return ret;
 		if (bypassed) {
 			/* if bypassed the regulator must have a supply */
-			if (!rdev->supply)
-				return -EINVAL;
+			if (!rdev->supply) {
+				ret = regulator_resolve_supply(rdev);
+				if (ret < 0)
+					return ret;
+				if (!rdev->supply)
+					return -EINVAL;
+			}
 
 			return _regulator_get_voltage(rdev->supply->rdev);
 		}
@@ -3939,8 +3944,6 @@ regulator_register(const struct regulator_desc *regulator_desc,
 		rdev->dev.of_node = of_node_get(config->of_node);
 	}
 
-	mutex_lock(&regulator_list_mutex);
-
 	mutex_init(&rdev->mutex);
 	rdev->reg_data = config->driver_data;
 	rdev->owner = regulator_desc->owner;
@@ -3983,23 +3986,26 @@ regulator_register(const struct regulator_desc *regulator_desc,
 	if (init_data)
 		constraints = &init_data->constraints;
 
+	if (init_data && init_data->supply_regulator)
+		rdev->supply_name = init_data->supply_regulator;
+	else if (regulator_desc->supply_name)
+		rdev->supply_name = regulator_desc->supply_name;
+
 	ret = set_machine_constraints(rdev, constraints);
 	if (ret < 0)
 		goto wash;
 
+	mutex_lock(&regulator_list_mutex);
+
 	ret = device_register(&rdev->dev);
 	if (ret != 0) {
 		put_device(&rdev->dev);
+		mutex_unlock(&regulator_list_mutex);
 		goto wash;
 	}
 
 	dev_set_drvdata(&rdev->dev, rdev);
 
-	if (init_data && init_data->supply_regulator)
-		rdev->supply_name = init_data->supply_regulator;
-	else if (regulator_desc->supply_name)
-		rdev->supply_name = regulator_desc->supply_name;
-
 	/* add consumers devices */
 	if (init_data) {
 		for (i = 0; i < init_data->num_consumer_supplies; i++) {
@@ -4009,6 +4015,7 @@ regulator_register(const struct regulator_desc *regulator_desc,
 			if (ret < 0) {
 				dev_err(dev, "Failed to set supply %s\n",
 					init_data->consumer_supplies[i].supply);
+				mutex_unlock(&regulator_list_mutex);
 				goto unset_supplies;
 			}
 		}
@@ -4036,7 +4043,6 @@ wash:
 clean:
 	kfree(rdev);
 out:
-	mutex_unlock(&regulator_list_mutex);
 	kfree(config);
 	return ERR_PTR(ret);
 }

[toc] | [next] | [standalone]

#1382395

[Multipart message — attachments visible in raw view] — view raw

On Tue, Apr 19, 2016 at 11:16:59AM +0100, Jon Hunter wrote:
> 
> On 11/04/16 15:16, Mark Brown wrote:
> > * PGP Signed by an unknown key
> > 
> > On Mon, Apr 11, 2016 at 04:11:01PM +0200, Thierry Reding wrote:
> >> On Mon, Apr 11, 2016 at 03:03:00PM +0100, Mark Brown wrote:
> > 
> >>> This shouldn't be a hard dependency: most regulators won't be in bypass
> >>> mode or otherwise depend on their parents enough to need this.
> > 
> >> I had initially proposed to resolve the supply only when necessary
> >> during regulator_get_voltage() when checking for bypass, perhaps that
> >> would after all be more appropriate here?
> > 
> > Yes, that had been what I'd expected.
> 
> 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()?
> 
> Jon
> 
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index 61d3918f329e..742d10371e2d 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -3126,8 +3126,13 @@ static int _regulator_get_voltage(struct regulator_dev *rdev)
>  			return ret;
>  		if (bypassed) {
>  			/* if bypassed the regulator must have a supply */
> -			if (!rdev->supply)
> -				return -EINVAL;
> +			if (!rdev->supply) {
> +				ret = regulator_resolve_supply(rdev);
> +				if (ret < 0)
> +					return ret;
> +				if (!rdev->supply)
> +					return -EINVAL;
> +			}
>  
>  			return _regulator_get_voltage(rdev->supply->rdev);
>  		}
> @@ -3939,8 +3944,6 @@ regulator_register(const struct regulator_desc *regulator_desc,
>  		rdev->dev.of_node = of_node_get(config->of_node);
>  	}
>  
> -	mutex_lock(&regulator_list_mutex);

It seems like this is used to protect accesses to the list of enable
GPIOs (regulator_ena_gpio_list), which is modified in the call to the
regulator_ena_gpio_request() function below.

That would be easily solved giving that its own lock, though.

Thierry

[toc] | [prev] | [next] | [standalone]

#1382621

[Multipart message — attachments visible in raw view] — view raw

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()?  

[toc] | [prev] | [next] | [standalone]

#1382675

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

[toc] | [prev] | [next] | [standalone]

#1383491

[Multipart message — attachments visible in raw view] — view raw

On Tue, Apr 19, 2016 at 05:09:59PM +0100, Jon Hunter wrote:
> On 19/04/16 16:40, Mark Brown wrote:

> > This is *really* weird.  Why would we need the list lock to do a
> > device_register()?  

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

What that's saying to me is that the device_register() is too early and
we shouldn't be registering the device until we're ready for it to be
used.

[toc] | [prev] | [next] | [standalone]

#1384371

On 20/04/16 16:21, Mark Brown wrote:
> * PGP Signed by an unknown key
> 
> On Tue, Apr 19, 2016 at 05:09:59PM +0100, Jon Hunter wrote:
>> On 19/04/16 16:40, Mark Brown wrote:
> 
>>> This is *really* weird.  Why would we need the list lock to do a
>>> device_register()?  
> 
>> 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.
> 
> What that's saying to me is that the device_register() is too early and
> we shouldn't be registering the device until we're ready for it to be
> used.

True and in fact looking at the code some more I am not sure that the
mutex actually would prevent someone from getting the regulator before
it is completely setup. I have moved this to the end of the registration
and seems to be fine. I will send out some patches for review.

Cheers
Jon

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web