Re: [PATCH 5/7] regulator: core: Lock dependent regulators

[Lucas Stach]

Hi Maciej,

Am Montag, den 04.06.2018, 15:59 +0200 schrieb Maciej Purski:
> Implementing coupled regulators adds a new dependency between
> regulators. Therefore, the current locking model should be changed.
> Coupled regulators should be locked with regulator's supplies at the
> same time.
> 
> Add new function regulator_lock_dependent(), which locks all regulators
> related with the one, that is being changed.

Sort of high level comment, but this doesn't look right: With dependent
regulators you don't strictly lock the regulators in the direction of
the tree root, but also siblings at the same level. This is prone with
deadlocks, as you can't control the order of the regulator locks being
taken by different tasks. This really needs a ww_mutex to be
implemented in a robust way.

Regards,
Lucas

> Signed-off-by: Maciej Purski <m.purski@xxxxxxxxxxx>
> ---
> Âdrivers/regulator/core.c | 75 +++++++++++++++++++++++++++++++++---------------
> Â1 file changed, 52 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index 0b366c5..7c57268 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -201,38 +201,67 @@ static void regulator_unlock(struct regulator_dev *rdev)
> > Â	}
> Â}
> Â
> -/**
> - * regulator_lock_supply - lock a regulator and its supplies
> - * @rdev:ÂÂÂÂÂÂÂÂÂregulator source
> - */
> -static void regulator_lock_supply(struct regulator_dev *rdev)
> +static int regulator_lock_recursive(struct regulator_dev *rdev,
> > +				ÂÂÂÂunsigned int subclass)
> Â{
> > +	struct regulator_dev *c_rdev;
> > Â	int i;
> Â
> > -	for (i = 0; rdev; rdev = rdev_get_supply(rdev), i++)
> > -		regulator_lock_nested(rdev, i);
> > +	for (i = 0; i < rdev->coupling_desc.n_coupled; i++) {
> > +		c_rdev = rdev->coupling_desc.coupled_rdevs[i];
> +
> > +		if (!c_rdev)
> > +			continue;
> +
> > +		regulator_lock_nested(c_rdev, subclass++);
> +
> > +		if (c_rdev->supply)
> > +			subclass =
> > +				regulator_lock_recursive(c_rdev->supply->rdev,
> > +							Âsubclass);
> > +	}
> +
> > +	return subclass;
> Â}
> Â
> Â/**
> - * regulator_unlock_supply - unlock a regulator and its supplies
> - * @rdev:ÂÂÂÂÂÂÂÂÂregulator source
> + * regulator_unlock_dependent - unlock regulator's suppliers and coupled
> > + *				regulators
> > + * @rdev:			regulator source
> + *
> + * Unlock all regulators related with rdev by coupling or suppling.
> Â */
> -static void regulator_unlock_supply(struct regulator_dev *rdev)
> +static void regulator_unlock_dependent(struct regulator_dev *rdev)
> Â{
> > -	struct regulator *supply;
> > +	struct regulator_dev *c_rdev;
> > +	int i;
> Â
> > -	while (1) {
> > -		regulator_unlock(rdev);
> > -		supply = rdev->supply;
> > +	for (i = 0; i < rdev->coupling_desc.n_coupled; i++) {
> > +		c_rdev = rdev->coupling_desc.coupled_rdevs[i];
> Â
> > -		if (!rdev->supply)
> > -			return;
> > +		if (!c_rdev)
> > +			continue;
> +
> > +		regulator_unlock(c_rdev);
> Â
> > -		rdev = supply->rdev;
> > +		if (c_rdev->supply)
> > +			regulator_unlock_dependent(c_rdev->supply->rdev);
> > Â	}
> Â}
> Â
> Â/**
> + * regulator_lock_dependent - lock regulator's suppliers and coupled regulators
> > + * @rdev:			regulator source
> + *
> + * This function as a wrapper on regulator_lock_recursive(), which locks
> + * all regulators related with rdev by coupling or suppling.
> + */
> +static inline void regulator_lock_dependent(struct regulator_dev *rdev)
> +{
> > +	regulator_lock_recursive(rdev, 0);
> +}
> +
> +/**
> Â * of_get_regulator - get a regulator device node based on supply name
> Â * @dev: Device pointer for the consumer (of regulator) device
> Â * @supply: regulator supply name
> @@ -3332,12 +3361,12 @@ int regulator_set_voltage(struct regulator *regulator, int min_uV, int max_uV)
> > Â	int ret = 0;
> Â
> > Â	pr_err("%s: %d\n", __func__, __LINE__);
> > -	regulator_lock_supply(regulator->rdev);
> > +	regulator_lock_dependent(regulator->rdev);
> Â
> > Â	ret = regulator_set_voltage_unlocked(regulator, min_uV, max_uV,
> > Â					ÂÂÂÂÂPM_SUSPEND_ON);
> Â
> > -	regulator_unlock_supply(regulator->rdev);
> > +	regulator_unlock_dependent(regulator->rdev);
> Â
> > Â	return ret;
> Â}
> @@ -3415,12 +3444,12 @@ int regulator_set_suspend_voltage(struct regulator *regulator, int min_uV,
> > Â	if (regulator_check_states(state) || state == PM_SUSPEND_ON)
> > Â		return -EINVAL;
> Â
> > -	regulator_lock_supply(regulator->rdev);
> > +	regulator_lock_dependent(regulator->rdev);
> Â
> > Â	ret = _regulator_set_suspend_voltage(regulator, min_uV,
> > Â					ÂÂÂÂÂmax_uV, state);
> Â
> > -	regulator_unlock_supply(regulator->rdev);
> > +	regulator_unlock_dependent(regulator->rdev);
> Â
> > Â	return ret;
> Â}
> @@ -3612,11 +3641,11 @@ int regulator_get_voltage(struct regulator *regulator)
> Â{
> > Â	int ret;
> Â
> > -	regulator_lock_supply(regulator->rdev);
> > +	regulator_lock_dependent(regulator->rdev);
> Â
> > Â	ret = _regulator_get_voltage(regulator->rdev);
> Â
> > -	regulator_unlock_supply(regulator->rdev);
> > +	regulator_unlock_dependent(regulator->rdev);
> Â
> > Â	return ret;
> Â}