| Message ID | 1780c3205893be8567fa29ccd86674e2f32555b4.1401192160.git.viresh.kumar@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Tue, May 27, 2014 at 05:37:29PM +0530, Viresh Kumar wrote: > Liam/Broonie: Please see if this can go through Rafael as 2nd patch is dependent > on it. Is that likely to happen before the merge window? > +static inline int regulator_set_voltage_time(struct regulator *regulator, > + int old_uV, int new_uV) > +{ > + return 0; > +} > + Hrm, I'd have expected this to return -EINVAL when stubbed. I'd also have expected regulator_set_voltage() to return -EINVAL mind you. I *suppose* that something that doesn't actually depend on regulator like cpufreq might not care if the voltage really did change (I bet this was added for cpufreq) but it's not awesome.
On Tuesday, May 27, 2014 08:29:23 PM Mark Brown wrote: > > --wJgPvdDu+gj56kJ8 > Content-Type: text/plain; charset=us-ascii > Content-Disposition: inline > > On Tue, May 27, 2014 at 05:37:29PM +0530, Viresh Kumar wrote: > > > Liam/Broonie: Please see if this can go through Rafael as 2nd patch is dependent > > on it. > > Is that likely to happen before the merge window? Depending. If there's -rc8, then maybe. Otherwise, nope. Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 28 May 2014 00:59, Mark Brown <broonie@kernel.org> wrote: > Hrm, I'd have expected this to return -EINVAL when stubbed. I'd also > have expected regulator_set_voltage() to return -EINVAL mind you. I Or ENOSYS ? I will fix both routines once you confirm.. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, May 28, 2014 at 01:12:57AM +0200, Rafael J. Wysocki wrote: > On Tuesday, May 27, 2014 08:29:23 PM Mark Brown wrote: > > On Tue, May 27, 2014 at 05:37:29PM +0530, Viresh Kumar wrote: > > > Liam/Broonie: Please see if this can go through Rafael as 2nd patch is dependent > > > on it. > > Is that likely to happen before the merge window? > Depending. If there's -rc8, then maybe. Otherwise, nope. The reason I was asking was that I'm happy to just go ahead and apply the patch right now if you're likely to not apply the second patch until after the merge window. Let's just do that for now, if you decide you will apply then please add my ack and let me know so I can drop the duplicate.
On Wed, May 28, 2014 at 09:59:37PM +0530, Viresh Kumar wrote: > On 28 May 2014 00:59, Mark Brown <broonie@kernel.org> wrote: > > Hrm, I'd have expected this to return -EINVAL when stubbed. I'd also > > have expected regulator_set_voltage() to return -EINVAL mind you. I > Or ENOSYS ? I will fix both routines once you confirm.. Whatever - I don't think the particular code makes any practical difference. We would need to audit existing users who don't have a REGULATOR dependency for breakage though.
On 28 May 2014 23:08, Mark Brown <broonie@kernel.org> wrote: > Whatever - I don't think the particular code makes any practical > difference. We would need to audit existing users who don't have a > REGULATOR dependency for breakage though. I tried auditing all 29 files which had this symbol: regulator_set_voltage and couldn't find anything which might break with the proposed change. Either these are making sure that we have a valid regulator or they have code inside #ifdef CONFIG_REGULATOR .. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jun 02, 2014 at 12:50:59PM +0530, Viresh Kumar wrote: > On 28 May 2014 23:08, Mark Brown <broonie@kernel.org> wrote: > > Whatever - I don't think the particular code makes any practical > > difference. We would need to audit existing users who don't have a > > REGULATOR dependency for breakage though. > I tried auditing all 29 files which had this symbol: regulator_set_voltage > and couldn't find anything which might break with the proposed change. > Either these are making sure that we have a valid regulator or they have > code inside #ifdef CONFIG_REGULATOR .. When you say they check for a valid regulator how are they doing that? The stub will come into play if there isn't a dependency on REGULATOR.
On 2 June 2014 15:21, Mark Brown <broonie@kernel.org> wrote: > When you say they check for a valid regulator how are they doing that? > The stub will come into play if there isn't a dependency on REGULATOR. I meant they fail and quit if regulator_get() failed and so the first parameter to regulator_set_voltage() is guaranteed to be valid. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jun 02, 2014 at 03:24:05PM +0530, Viresh Kumar wrote: > On 2 June 2014 15:21, Mark Brown <broonie@kernel.org> wrote: > > When you say they check for a valid regulator how are they doing that? > > The stub will come into play if there isn't a dependency on REGULATOR. > I meant they fail and quit if regulator_get() failed and so the first parameter > to regulator_set_voltage() is guaranteed to be valid. No, think about what you're changing here. You're changing the stub - the stub has a regulator_get() which always succeeeds.
On 2 June 2014 15:32, Mark Brown <broonie@kernel.org> wrote: > No, think about what you're changing here. You're changing the stub - > the stub has a regulator_get() which always succeeeds. Right, things might start to break with the change to regulator_set_voltage().. When I compare this to clk-APIs, the dummy implementations always pass and so we are allowed to send NULL clk to any routine (the way we can do it here, probably to simply code).. Now, why do we want to return -EINVAL from set_voltage here ? Similar routines in clk-API are returning 0 and even clk_get_rate() returns zero, unlike in regulators, as we return -EINVAL.. Not sure which of these frameworks is doing the right thing. What do you suggest. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jun 02, 2014 at 03:45:37PM +0530, Viresh Kumar wrote: > On 2 June 2014 15:32, Mark Brown <broonie@kernel.org> wrote: > > No, think about what you're changing here. You're changing the stub - > > the stub has a regulator_get() which always succeeeds. > Now, why do we want to return -EINVAL from set_voltage here ? Similar > routines in clk-API are returning 0 and even clk_get_rate() returns zero, > unlike in regulators, as we return -EINVAL.. If the consumer tried to set a voltage presumably it cares if that voltage was set - for example if your cpufreq driver tries to increase the voltage of a core supply so that it can then raise the frequency the user is going to be upset if the voltage was not actually raised and it goes off and raises the clock rate causing the system to become unstable. > Not sure which of these frameworks is doing the right thing. I don't think the clock API should have clk_set_rate() report success if it was ignored.
On 2 June 2014 17:53, Mark Brown <broonie@kernel.org> wrote: > If the consumer tried to set a voltage presumably it cares if that > voltage was set - for example if your cpufreq driver tries to increase > the voltage of a core supply so that it can then raise the frequency the > user is going to be upset if the voltage was not actually raised and it > goes off and raises the clock rate causing the system to become unstable. If the driver continued despite getting regulator as NULL, it means that regulator isn't a MUST for it. For example a CPUFreq driver may work with or without a regulator. Now if the dummy calls return *error* for some cases then these driver will have to do if(xyz) API-call().. And so dummy APIs like clk_set_rate(), clk_get_rate(), regulator_set_voltage() must return zero.. To get rid of this in drivers these dummy routines *must* behave as they passed, if the drivers really care about them then they must quit as soon as regulator_get() returned NULL. This is why we have such implementations in clk framework which are very well thought earlier. Does this make sense? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On Mon, Jun 02, 2014 at 06:44:35PM +0530, Viresh Kumar wrote: > On 2 June 2014 17:53, Mark Brown <broonie@kernel.org> wrote: > > If the consumer tried to set a voltage presumably it cares if that > > voltage was set - for example if your cpufreq driver tries to increase > > the voltage of a core supply so that it can then raise the frequency the > > user is going to be upset if the voltage was not actually raised and it > > goes off and raises the clock rate causing the system to become unstable. > If the driver continued despite getting regulator as NULL, it means that > regulator isn't a MUST for it. For example a CPUFreq driver may work > with or without a regulator. No, NULL is a perfectly valid regulator - the *only* thing that a caller should check for is IS_ERR(). You are missing the point of the stubs, and indeed the fact that real physical supplies can have exactly the same limitations as the dummy supplies do and therefore the presence of a physical regulator should not in itself indcate anything about what you can do with it. > Now if the dummy calls return *error* for some cases then these driver > will have to do > if(xyz) > API-call().. Consumers should be implementing error checking code regardless. If we don't need to implement any error checking there's rather a lot of the kernel we can go and delete... > And so dummy APIs like clk_set_rate(), clk_get_rate(), > regulator_set_voltage() must return zero.. Please re-read and think about my CPUfreq example. How do you expect that to work sanely if we don't care if any of the operations worked? > To get rid of this in drivers these dummy routines *must* behave as > they passed, if the drivers really care about them then they must > quit as soon as regulator_get() returned NULL. > This is why we have such implementations in clk framework which are > very well thought earlier. > Does this make sense? No, not at all and I don't think it applies to the clock API either - it's got similar issues with real physical clocks not always supporting all operations. Consider for a moment what happens if we try to set and then use a clock rate ona fixed clock.
On 2 June 2014 20:50, Mark Brown <broonie@kernel.org> wrote: > No, not at all and I don't think it applies to the clock API either - > it's got similar issues with real physical clocks not always supporting > all operations. Consider for a moment what happens if we try to set and > then use a clock rate ona fixed clock. Okay, so the patch 1/3 isn't enough as we need to fix other drivers as well which are expected to work with CONFIG_REGULATOR=n. That would be some work and will try to send a patchset for that.. As Rafael has asked you to apply the patches, can you please apply patch 2-3 for now? -- viresh -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jun 02, 2014 at 10:25:18PM +0530, Viresh Kumar wrote: > As Rafael has asked you to apply the patches, can you please apply > patch 2-3 for now? Not right now, the merge window is open.
On 28 May 2014 23:08, Mark Brown <broonie@kernel.org> wrote: > Whatever - I don't think the particular code makes any practical > difference. We would need to audit existing users who don't have a > REGULATOR dependency for breakage though. Exactly what kind of drivers are we looking to fix here? These might be the possible cases: - We are checking 'regulator pointer' before calling and don't need to handle anything there.. - drivers depend on CONFIG_REGULATOR and so again we don't need to handle anything - None of above are true and drivers aren't checking return value of regulator_set_voltage() OR They are checking it and failing when it failed.. What do we want to do in these cases? -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jun 03, 2014 at 08:10:00PM +0530, Viresh Kumar wrote: > On 28 May 2014 23:08, Mark Brown <broonie@kernel.org> wrote: > > Whatever - I don't think the particular code makes any practical > > difference. We would need to audit existing users who don't have a > > REGULATOR dependency for breakage though. > Exactly what kind of drivers are we looking to fix here? These might > be the possible cases: > - We are checking 'regulator pointer' before calling and don't need to > handle anything there.. I'm not sure what you mean by this. > - None of above are true and drivers aren't checking return value of > regulator_set_voltage() > OR > They are checking it and failing when it failed.. > What do we want to do in these cases? Well, we would need to look at what the drivers were doing and figure out something sensible - it really depends why they're trying to set the regulator and what would happen if it doesn't work. For drivers that ignore the return value they won't be affected anyway.
On 3 June 2014 20:23, Mark Brown <broonie@kernel.org> wrote: >> - We are checking 'regulator pointer' before calling and don't need to >> handle anything there.. > > I'm not sure what you mean by this. I meant that sometimes regulator_set_voltage() is only called when pointer to regulator is valid. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 3 June 2014 20:23, Mark Brown <broonie@kernel.org> wrote: > Well, we would need to look at what the drivers were doing and figure > out something sensible - it really depends why they're trying to set the > regulator and what would happen if it doesn't work. For example, few cpufreq drivers are calling it during frequency transition and are checking return value as well.. And fail if it failed. One way out might be checking if pointer to regulator is valid or not and only call it if pointer is not NULL.. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On Tue, Jun 03, 2014 at 08:52:04PM +0530, Viresh Kumar wrote: > On 3 June 2014 20:23, Mark Brown <broonie@kernel.org> wrote: > >> - We are checking 'regulator pointer' before calling and don't need to > >> handle anything there.. > > I'm not sure what you mean by this. > I meant that sometimes regulator_set_voltage() is only called when pointer > to regulator is valid. Could you please be more explicit about what you mean by "valid"?
On 3 June 2014 21:02, Mark Brown <broonie@kernel.org> wrote:
> Could you please be more explicit about what you mean by "valid"?
I meant Not NULL..
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
On Tue, Jun 03, 2014 at 08:55:25PM +0530, Viresh Kumar wrote: > On 3 June 2014 20:23, Mark Brown <broonie@kernel.org> wrote: > > Well, we would need to look at what the drivers were doing and figure > > out something sensible - it really depends why they're trying to set the > > regulator and what would happen if it doesn't work. > For example, few cpufreq drivers are calling it during frequency > transition and are checking return value as well.. And fail if it failed. > One way out might be checking if pointer to regulator is valid or not > and only call it if pointer is not NULL.. No, as I've explained repeatedly NULL is a perfectly valid regulator and that's not going to work reliably. As I've previously requested please think about what happens to cpufreq if we fail to ramp voltages.
On Tue, Jun 03, 2014 at 09:05:56PM +0530, Viresh Kumar wrote: > On 3 June 2014 21:02, Mark Brown <broonie@kernel.org> wrote: > > Could you please be more explicit about what you mean by "valid"? > I meant Not NULL.. To repeat yet again: NULL is a perfectly valid regulator, anything checking for NULL is broken.
On 3 June 2014 21:18, Mark Brown <broonie@kernel.org> wrote: > No, as I've explained repeatedly NULL is a perfectly valid regulator and Okay, its been checked at multiple places already and that's obviously wrong then. > that's not going to work reliably. As I've previously requested please > think about what happens to cpufreq if we fail to ramp voltages. Okay, so here is the scenario: - driver is generic (like cpufreq-cpu0) and some user platforms may have regulator support and others might not.. - For platforms with regulators support, we _must_ check if the voltage change is successful or not and fail if regulator_set_voltage() failed. - But for platforms without regulators support (CONFIG_REGULATOR=n), regulator_get() will return NULL (a valid regulator though) and regulator_set_voltage() will fail. Because the platform doesn't care much about regulators it must go on and change frequency as if nothing happened. How can we achieve both these requirements by a generic piece of code? The only way I could think of currently is by returning something special like -ENOSYS from regulator_set_voltage() when regulators aren't configured in kernel and check return value of regulator_set_voltage() against this.. This also holds true for regulator_get_voltage() which is returning -EINVAL currently.. Please share if you have some other solution in mind.. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jun 04, 2014 at 11:16:37AM +0530, Viresh Kumar wrote: > - But for platforms without regulators support (CONFIG_REGULATOR=n), > regulator_get() will return NULL (a valid regulator though) and > regulator_set_voltage() will fail. Because the platform doesn't care much > about regulators it must go on and change frequency as if nothing > happened. No, approximately none of this is true. CONFIG_REGULATOR tells you nothing about the hardware, it tells you what someone selected in the kernel config. There is nothing stopping anyone enabling the API on any platform, and there is nothing stopping anyone disabling the API when building a kernel for a platform which can do something with regulators with CONFIG_REGULATOR disabled. Please, go and *think* about what's going on here. I've repeatedly asked you to consider the case where we need to raise the voltage prior to raising the frequency for cpufreq but you've not responded to these requests either directly or in showing any sign of having understood the issue. If the code fails to change the voltage it needs to handle that (including remembering that attempts to lower the voltage fail); if the code handles the errors sensibly I would expect that to handle everything.
On 4 June 2014 16:08, Mark Brown <broonie@kernel.org> wrote: > Please, go and *think* about what's going on here. I've repeatedly > asked you to consider the case where we need to raise the voltage prior > to raising the frequency for cpufreq but you've not responded to these > requests either directly or in showing any sign of having understood the > issue. I replied to that only when I said: - For platforms with regulators support, we _must_ check if the voltage change is successful or not and fail if regulator_set_voltage() failed. And what you wrote earlier was correct, we may end up with unstable systems if we ramp frequencies even when changing voltage failed. > If the code fails to change the voltage it needs to handle that > (including remembering that attempts to lower the voltage fail); if the > code handles the errors sensibly I would expect that to handle > everything. That's what I was asking, how should the code handle it? Code also has to take care of platforms which haven't configured regulators in kernel and want to use this generic driver. Sorry if I still didn't answer it properly :( -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On Wed, Jun 04, 2014 at 04:27:48PM +0530, Viresh Kumar wrote: > On 4 June 2014 16:08, Mark Brown <broonie@kernel.org> wrote: > > Please, go and *think* about what's going on here. I've repeatedly > > If the code fails to change the voltage it needs to handle that > > (including remembering that attempts to lower the voltage fail); if the > > code handles the errors sensibly I would expect that to handle > > everything. > That's what I was asking, how should the code handle it? Code also > has to take care of platforms which haven't configured regulators > in kernel and want to use this generic driver. If the code gets an error back when it tries to change the voltage then it should assume that the attempt to change the voltage did not succeed. Should an attempt to change the voltage not succeed it seems reasonable to assume that the voltage did not change and is the same as before. The obvious thing would therefore appear to be for the code to proceed with that assumption and act accordingly.
diff --git a/include/linux/regulator/consumer.h b/include/linux/regulator/consumer.h index 1a4a8c1..0cfc286 100644 --- a/include/linux/regulator/consumer.h +++ b/include/linux/regulator/consumer.h @@ -397,6 +397,12 @@ static inline int regulator_set_voltage(struct regulator *regulator, return 0; } +static inline int regulator_set_voltage_time(struct regulator *regulator, + int old_uV, int new_uV) +{ + return 0; +} + static inline int regulator_get_voltage(struct regulator *regulator) { return -EINVAL;
We already have dummy implementation for most of the regulators APIs for !CONFIG_REGULATOR case and were missing it for regulator_set_voltage_time(). Found this issue while compiling cpufreq-cpu0 driver without regulators support in kernel. drivers/cpufreq/cpufreq-cpu0.c: In function ‘cpu0_cpufreq_probe’: drivers/cpufreq/cpufreq-cpu0.c:186:3: error: implicit declaration of function ‘regulator_set_voltage_time’ [-Werror=implicit-function-declaration] Fix this by adding dummy definition for regulator_set_voltage_time(). Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- Liam/Broonie: Please see if this can go through Rafael as 2nd patch is dependent on it. include/linux/regulator/consumer.h | 6 ++++++ 1 file changed, 6 insertions(+)