| Message ID | 1528120764-14316-6-git-send-email-m.purski@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@samsung.com> > --- > 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; > } -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/04/2018 04:20 PM, Lucas Stach wrote: > 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 > Thanks for that. You are right and it needs fixing. I'll consider it in my next version. However, that error should arise only, when we have the actual coupled regulators, so it shouldn't be a problem in Tony's case. Best regards, Maciej Purski >> Signed-off-by: Maciej Purski <m.purski@samsung.com> >> --- >> 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; >> } > > > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; }
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. Signed-off-by: Maciej Purski <m.purski@samsung.com> --- drivers/regulator/core.c | 75 +++++++++++++++++++++++++++++++++--------------- 1 file changed, 52 insertions(+), 23 deletions(-)