| Message ID | 359b588928b7e58b009f786b17ddc088c6a7d18b.1597292833.git.viresh.kumar@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | viresh kumar |
| Headers | show |
| Series | [V2,1/4] opp: Enable resources again if they were disabled earlier | expand |
Quoting Viresh Kumar (2020-08-12 21:29:00) > The common "enabled" flag can be used here instead of > "regulator_enabled" now. > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> Why not put this before the other patch? And mention that it will be reused in another place soon? Then the previous patch won't look like we're adding a variable to the struct when it is only used inside a single function. Or at least squash it with the previous patch. > --- > drivers/opp/core.c | 13 +++---------- > drivers/opp/opp.h | 2 -- > 2 files changed, 3 insertions(+), 12 deletions(-) > > diff --git a/drivers/opp/core.c b/drivers/opp/core.c > index e8882e7fd8a5..5f5da257f58a 100644 > --- a/drivers/opp/core.c > +++ b/drivers/opp/core.c > @@ -703,12 +703,10 @@ static int _generic_set_opp_regulator(struct opp_table *opp_table, > * Enable the regulator after setting its voltages, otherwise it breaks > * some boot-enabled regulators. > */ > - if (unlikely(!opp_table->regulator_enabled)) { > + if (unlikely(!opp_table->enabled)) { > ret = regulator_enable(reg); > if (ret < 0) > dev_warn(dev, "Failed to enable regulator: %d", ret); > - else > - opp_table->regulator_enabled = true; A quick glance makes this look unsafe now because we're only checking 'enabled' and not actually setting it when this function is called. I have to go back to the previous patch to understand where enabled is now set to confirm that it is OK. If it was all one patch all the context would be here.
On 8/13/2020 9:59 AM, Viresh Kumar wrote: > The common "enabled" flag can be used here instead of > "regulator_enabled" now. > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > drivers/opp/core.c | 13 +++---------- > drivers/opp/opp.h | 2 -- > 2 files changed, 3 insertions(+), 12 deletions(-) > > diff --git a/drivers/opp/core.c b/drivers/opp/core.c > index e8882e7fd8a5..5f5da257f58a 100644 > --- a/drivers/opp/core.c > +++ b/drivers/opp/core.c > @@ -703,12 +703,10 @@ static int _generic_set_opp_regulator(struct opp_table *opp_table, > * Enable the regulator after setting its voltages, otherwise it breaks > * some boot-enabled regulators. > */ > - if (unlikely(!opp_table->regulator_enabled)) { > + if (unlikely(!opp_table->enabled)) { > ret = regulator_enable(reg); > if (ret < 0) > dev_warn(dev, "Failed to enable regulator: %d", ret); > - else > - opp_table->regulator_enabled = true; > } > > return 0; > @@ -905,10 +903,7 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq) > if (ret) > goto put_opp_table; > > - if (opp_table->regulator_enabled) { > - regulator_disable(opp_table->regulators[0]); > - opp_table->regulator_enabled = false; > - } > + regulator_disable(opp_table->regulators[0]); unconditionally calling regulator_disable() here based on the common 'enabled' flag results in a crash on platforms without regulators associated with the opp table. > > ret = _set_required_opps(dev, opp_table, NULL); > if (!ret) > @@ -1795,11 +1790,9 @@ void dev_pm_opp_put_regulators(struct opp_table *opp_table) > /* Make sure there are no concurrent readers while updating opp_table */ > WARN_ON(!list_empty(&opp_table->opp_list)); > > - if (opp_table->regulator_enabled) { > + if (opp_table->enabled) { > for (i = opp_table->regulator_count - 1; i >= 0; i--) > regulator_disable(opp_table->regulators[i]); > - > - opp_table->regulator_enabled = false; > } > > for (i = opp_table->regulator_count - 1; i >= 0; i--) > diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h > index bd35802acc6e..0c3de3f6db5c 100644 > --- a/drivers/opp/opp.h > +++ b/drivers/opp/opp.h > @@ -147,7 +147,6 @@ enum opp_table_access { > * @clk: Device's clock handle > * @regulators: Supply regulators > * @regulator_count: Number of power supply regulators. Its value can be -1 > - * @regulator_enabled: Set to true if regulators were previously enabled. > * (uninitialized), 0 (no opp-microvolt property) or > 0 (has opp-microvolt > * property). > * @paths: Interconnect path handles > @@ -196,7 +195,6 @@ struct opp_table { > struct clk *clk; > struct regulator **regulators; > int regulator_count; > - bool regulator_enabled; > struct icc_path **paths; > unsigned int path_count; > bool enabled; >
On 15-08-20, 00:55, Stephen Boyd wrote: > Quoting Viresh Kumar (2020-08-12 21:29:00) > > diff --git a/drivers/opp/core.c b/drivers/opp/core.c > > index e8882e7fd8a5..5f5da257f58a 100644 > > --- a/drivers/opp/core.c > > +++ b/drivers/opp/core.c > > @@ -703,12 +703,10 @@ static int _generic_set_opp_regulator(struct opp_table *opp_table, > > * Enable the regulator after setting its voltages, otherwise it breaks > > * some boot-enabled regulators. > > */ > > - if (unlikely(!opp_table->regulator_enabled)) { > > + if (unlikely(!opp_table->enabled)) { > > ret = regulator_enable(reg); > > if (ret < 0) > > dev_warn(dev, "Failed to enable regulator: %d", ret); > > - else > > - opp_table->regulator_enabled = true; > > A quick glance makes this look unsafe now because we're only checking > 'enabled' and not actually setting it when this function is called. I > have to go back to the previous patch to understand where enabled is now > set to confirm that it is OK. If it was all one patch all the context > would be here. The only case where things can go crazy are the cases where (for example) clk_set_rate() fails, or something like that which would be a bug and it shouldn't bother in the normal working of this code.
diff --git a/drivers/opp/core.c b/drivers/opp/core.c index e8882e7fd8a5..5f5da257f58a 100644 --- a/drivers/opp/core.c +++ b/drivers/opp/core.c @@ -703,12 +703,10 @@ static int _generic_set_opp_regulator(struct opp_table *opp_table, * Enable the regulator after setting its voltages, otherwise it breaks * some boot-enabled regulators. */ - if (unlikely(!opp_table->regulator_enabled)) { + if (unlikely(!opp_table->enabled)) { ret = regulator_enable(reg); if (ret < 0) dev_warn(dev, "Failed to enable regulator: %d", ret); - else - opp_table->regulator_enabled = true; } return 0; @@ -905,10 +903,7 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq) if (ret) goto put_opp_table; - if (opp_table->regulator_enabled) { - regulator_disable(opp_table->regulators[0]); - opp_table->regulator_enabled = false; - } + regulator_disable(opp_table->regulators[0]); ret = _set_required_opps(dev, opp_table, NULL); if (!ret) @@ -1795,11 +1790,9 @@ void dev_pm_opp_put_regulators(struct opp_table *opp_table) /* Make sure there are no concurrent readers while updating opp_table */ WARN_ON(!list_empty(&opp_table->opp_list)); - if (opp_table->regulator_enabled) { + if (opp_table->enabled) { for (i = opp_table->regulator_count - 1; i >= 0; i--) regulator_disable(opp_table->regulators[i]); - - opp_table->regulator_enabled = false; } for (i = opp_table->regulator_count - 1; i >= 0; i--) diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h index bd35802acc6e..0c3de3f6db5c 100644 --- a/drivers/opp/opp.h +++ b/drivers/opp/opp.h @@ -147,7 +147,6 @@ enum opp_table_access { * @clk: Device's clock handle * @regulators: Supply regulators * @regulator_count: Number of power supply regulators. Its value can be -1 - * @regulator_enabled: Set to true if regulators were previously enabled. * (uninitialized), 0 (no opp-microvolt property) or > 0 (has opp-microvolt * property). * @paths: Interconnect path handles @@ -196,7 +195,6 @@ struct opp_table { struct clk *clk; struct regulator **regulators; int regulator_count; - bool regulator_enabled; struct icc_path **paths; unsigned int path_count; bool enabled;
The common "enabled" flag can be used here instead of "regulator_enabled" now. Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- drivers/opp/core.c | 13 +++---------- drivers/opp/opp.h | 2 -- 2 files changed, 3 insertions(+), 12 deletions(-)