[U-Boot] [PATCH] power: regulator: Return success on attempt to disable an always-on regulator
commit 4f86a724e82c0 ("power: regulator: denied disable on always-on regulator") throws an error when requested to disable an always-on regulator. It is right that an always-on regulator should not be attempted to be disabled. But at the same time regulator framework should not return an error when such request is received. Instead it should just return success without attempting to disable the specified regulator. This is because the requesting driver will not have the idea if the regulator is always-on or not. The requesting driver will always try to enable/disable regulator as per the required flow. So it is upto regulator framework to not break such scenarios.
Fixes: 4f86a724e82c0 ("power: regulator: denied disable on always-on regulator") Reported-by: Jean-Jacques Hiblot jjhiblot@ti.com Signed-off-by: Lokesh Vutla lokeshvutla@ti.com --- - Without this mmc driver is broken on TI's DRA7 platfroms. - kernel as well follows the same logic while disabling an always-on regulator[1] Logs: Without $patch: https://pastebin.ubuntu.com/p/cNyKTxgKCY/ with $patch: https://pastebin.ubuntu.com/p/PZbgfMp77k/
[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/driv...
drivers/power/regulator/regulator-uclass.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/power/regulator/regulator-uclass.c b/drivers/power/regulator/regulator-uclass.c index 4511625ff2..39e46279d5 100644 --- a/drivers/power/regulator/regulator-uclass.c +++ b/drivers/power/regulator/regulator-uclass.c @@ -113,7 +113,7 @@ int regulator_set_enable(struct udevice *dev, bool enable)
uc_pdata = dev_get_uclass_platdata(dev); if (!enable && uc_pdata->always_on) - return -EACCES; + return 0;
return ops->set_enable(dev, enable); }
Hi,
On 24/12/18 4:37 PM, Lokesh Vutla wrote:
commit 4f86a724e82c0 ("power: regulator: denied disable on always-on regulator") throws an error when requested to disable an always-on regulator. It is right that an always-on regulator should not be attempted to be disabled. But at the same time regulator framework should not return an error when such request is received. Instead it should just return success without attempting to disable the specified regulator. This is because the requesting driver will not have the idea if the regulator is always-on or not. The requesting driver will always try to enable/disable regulator as per the required flow. So it is upto regulator framework to not break such scenarios.
Fixes: 4f86a724e82c0 ("power: regulator: denied disable on always-on regulator") Reported-by: Jean-Jacques Hiblot jjhiblot@ti.com Signed-off-by: Lokesh Vutla lokeshvutla@ti.com
Reviewed-by: Faiz Abbas faiz_abbas@ti.com
Thanks, Faiz
On Mon, Dec 24, 2018 at 04:37:41PM +0530, Lokesh Vutla wrote:
commit 4f86a724e82c0 ("power: regulator: denied disable on always-on regulator") throws an error when requested to disable an always-on regulator. It is right that an always-on regulator should not be attempted to be disabled. But at the same time regulator framework should not return an error when such request is received. Instead it should just return success without attempting to disable the specified regulator. This is because the requesting driver will not have the idea if the regulator is always-on or not. The requesting driver will always try to enable/disable regulator as per the required flow. So it is upto regulator framework to not break such scenarios.
Fixes: 4f86a724e82c0 ("power: regulator: denied disable on always-on regulator") Reported-by: Jean-Jacques Hiblot jjhiblot@ti.com Signed-off-by: Lokesh Vutla lokeshvutla@ti.com
Applied to u-boot/master, thanks!
Hi Lokesh,
On Mon, 24 Dec 2018 at 04:08, Lokesh Vutla lokeshvutla@ti.com wrote:
commit 4f86a724e82c0 ("power: regulator: denied disable on always-on regulator") throws an error when requested to disable an always-on regulator. It is right that an always-on regulator should not be attempted to be disabled. But at the same time regulator framework should not return an error when such request is received. Instead it should just return success without attempting to disable the specified regulator. This is because the requesting driver will not have the idea if the regulator is always-on or not. The requesting driver will always try to enable/disable regulator as per the required flow. So it is upto regulator framework to not break such scenarios.
Can the caller not check the error code? It is -EACCES in this case.
Regards, Simon
Fixes: 4f86a724e82c0 ("power: regulator: denied disable on always-on regulator") Reported-by: Jean-Jacques Hiblot jjhiblot@ti.com Signed-off-by: Lokesh Vutla lokeshvutla@ti.com
- Without this mmc driver is broken on TI's DRA7 platfroms.
- kernel as well follows the same logic while disabling an always-on regulator[1]
Logs: Without $patch: https://pastebin.ubuntu.com/p/cNyKTxgKCY/ with $patch: https://pastebin.ubuntu.com/p/PZbgfMp77k/
[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/driv...
drivers/power/regulator/regulator-uclass.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/power/regulator/regulator-uclass.c b/drivers/power/regulator/regulator-uclass.c index 4511625ff2..39e46279d5 100644 --- a/drivers/power/regulator/regulator-uclass.c +++ b/drivers/power/regulator/regulator-uclass.c @@ -113,7 +113,7 @@ int regulator_set_enable(struct udevice *dev, bool enable)
uc_pdata = dev_get_uclass_platdata(dev); if (!enable && uc_pdata->always_on)
return -EACCES;
return 0; return ops->set_enable(dev, enable);}
2.19.2
Hi Simon,
On 28/12/18 3:57 AM, Simon Glass wrote:
Hi Lokesh,
On Mon, 24 Dec 2018 at 04:08, Lokesh Vutla lokeshvutla@ti.com wrote:
commit 4f86a724e82c0 ("power: regulator: denied disable on always-on regulator") throws an error when requested to disable an always-on regulator. It is right that an always-on regulator should not be attempted to be disabled. But at the same time regulator framework should not return an error when such request is received. Instead it should just return success without attempting to disable the specified regulator. This is because the requesting driver will not have the idea if the regulator is always-on or not. The requesting driver will always try to enable/disable regulator as per the required flow. So it is upto regulator framework to not break such scenarios.
Can the caller not check the error code? It is -EACCES in this case.
We considered this an one of the option but I ended up fixing regulator framework due to the following reasons: - If regulator framework returns -EACCES on this scenario then: - -EACCES should be checked in all the existing usage of the api[1] or else someone else might encounter the same problem. - Any future usage of the api should take of handling this error. - From a client driver perspective it is not really an error. It is doing the right thing and receiving an error might be confusing.
Hope this is clear. Also just to add one more point, I adapted this error handling from Linux kernel[2].
[1] ➜ u-boot git:(master) git grep -in "regulator_set_enable(.*, false)" cmd/regulator.c:410: ret = regulator_set_enable(dev, false); drivers/mmc/mmc.c:2552: int ret = regulator_set_enable(mmc->vmmc_supply, false); drivers/mmc/omap_hsmmc.c:473: ret = regulator_set_enable(priv->pbias_supply, false); drivers/mmc/omap_hsmmc.c:478: ret = regulator_set_enable(mmc->vqmmc_supply, false); drivers/net/fec_mxc.c:1414: regulator_set_enable(priv->phy_supply, false); drivers/phy/meson-gxl-usb2.c:174: int ret = regulator_set_enable(priv->phy_supply, false); drivers/phy/phy-rcar-gen3.c:101: return regulator_set_enable(priv->vbus_supply, false); drivers/phy/phy-stm32-usbphyc.c:251: ret = regulator_set_enable(usbphyc_phy->vdda1v1, false); drivers/phy/phy-stm32-usbphyc.c:257: ret = regulator_set_enable(usbphyc_phy->vdda1v8, false); drivers/phy/phy-stm32-usbphyc.c:263: ret = regulator_set_enable(usbphyc_phy->vdd, false); drivers/usb/host/dwc2.c:199: ret = regulator_set_enable(priv->vbus_supply, false); drivers/usb/host/ehci-generic.c:60: return regulator_set_enable(priv->vbus_supply, false); drivers/usb/host/xhci-rockchip.c:161: ret = regulator_set_enable(plat->vbus_supply, false); drivers/video/pwm_backlight.c:160: ret = regulator_set_enable(priv->reg, false); test/dm/adc.c:74: ut_assertok(regulator_set_enable(supply, false));
[2] [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/driv...
Thanks and regards, Lokesh
Hi Lokesh,
On Thu, 27 Dec 2018 at 22:33, Lokesh Vutla lokeshvutla@ti.com wrote:
Hi Simon,
On 28/12/18 3:57 AM, Simon Glass wrote:
Hi Lokesh,
On Mon, 24 Dec 2018 at 04:08, Lokesh Vutla lokeshvutla@ti.com wrote:
commit 4f86a724e82c0 ("power: regulator: denied disable on always-on regulator") throws an error when requested to disable an always-on regulator. It is right that an always-on regulator should not be attempted to be disabled. But at the same time regulator framework should not return an error when such request is received. Instead it should just return success without attempting to disable the specified regulator. This is because the requesting driver will not have the idea if the regulator is always-on or not. The requesting driver will always try to enable/disable regulator as per the required flow. So it is upto regulator framework to not break such scenarios.
Can the caller not check the error code? It is -EACCES in this case.
We considered this an one of the option but I ended up fixing regulator framework due to the following reasons:
- If regulator framework returns -EACCES on this scenario then: - -EACCES should be checked in all the existing usage of the api[1] or else
someone else might encounter the same problem.
Yes. Some already check for -ENOSYS, e.g. omap_hsmmc.c
- Any future usage of the api should take of handling this error.
Yes, and it should be commented too.
- From a client driver perspective it is not really an error. It is doing theright thing and receiving an error might be confusing.
The error means that the request was not handled. There is no way to find out that requesting this was actually wrong.
Hope this is clear. Also just to add one more point, I adapted this error handling from Linux kernel[2].
The only question for me whether anything would need to detect that the request to disable a regulator is not supported.
Your linux link appears to lead me to regulator_ena_gpio_ctrl(), related to regulator GPIOs. Is that right? It's hard for me to understand what the code there is doing.
Once we make this change we will not be able to go back without breaking things.
I am not really convinced that this patch is the best approach. I do understand your point though. It just worries me that we are hiding something and it will be hard to unhide it later.
What do you think about adding something like regulator_disable_if_allowed() which silently ignored -ENOSYS and -EACCES?
Thanks for the detailed response.
Regards, Simon
[1] ➜ u-boot git:(master) git grep -in "regulator_set_enable(.*, false)" cmd/regulator.c:410: ret = regulator_set_enable(dev, false); drivers/mmc/mmc.c:2552: int ret = regulator_set_enable(mmc->vmmc_supply, false); drivers/mmc/omap_hsmmc.c:473: ret = regulator_set_enable(priv->pbias_supply, false); drivers/mmc/omap_hsmmc.c:478: ret = regulator_set_enable(mmc->vqmmc_supply, false); drivers/net/fec_mxc.c:1414: regulator_set_enable(priv->phy_supply, false); drivers/phy/meson-gxl-usb2.c:174: int ret = regulator_set_enable(priv->phy_supply, false); drivers/phy/phy-rcar-gen3.c:101: return regulator_set_enable(priv->vbus_supply, false); drivers/phy/phy-stm32-usbphyc.c:251: ret = regulator_set_enable(usbphyc_phy->vdda1v1, false); drivers/phy/phy-stm32-usbphyc.c:257: ret = regulator_set_enable(usbphyc_phy->vdda1v8, false); drivers/phy/phy-stm32-usbphyc.c:263: ret = regulator_set_enable(usbphyc_phy->vdd, false); drivers/usb/host/dwc2.c:199: ret = regulator_set_enable(priv->vbus_supply, false); drivers/usb/host/ehci-generic.c:60: return regulator_set_enable(priv->vbus_supply, false); drivers/usb/host/xhci-rockchip.c:161: ret = regulator_set_enable(plat->vbus_supply, false); drivers/video/pwm_backlight.c:160: ret = regulator_set_enable(priv->reg, false); test/dm/adc.c:74: ut_assertok(regulator_set_enable(supply, false));
[2] [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/driv...
Thanks and regards, Lokesh
Hi Simon,
On 29/12/18 6:58 PM, Simon Glass wrote:
Hi Lokesh,
On Thu, 27 Dec 2018 at 22:33, Lokesh Vutla lokeshvutla@ti.com wrote:
Hi Simon,
On 28/12/18 3:57 AM, Simon Glass wrote:
Hi Lokesh,
On Mon, 24 Dec 2018 at 04:08, Lokesh Vutla lokeshvutla@ti.com wrote:
commit 4f86a724e82c0 ("power: regulator: denied disable on always-on regulator") throws an error when requested to disable an always-on regulator. It is right that an always-on regulator should not be attempted to be disabled. But at the same time regulator framework should not return an error when such request is received. Instead it should just return success without attempting to disable the specified regulator. This is because the requesting driver will not have the idea if the regulator is always-on or not. The requesting driver will always try to enable/disable regulator as per the required flow. So it is upto regulator framework to not break such scenarios.
Can the caller not check the error code? It is -EACCES in this case.
We considered this an one of the option but I ended up fixing regulator framework due to the following reasons:
- If regulator framework returns -EACCES on this scenario then: - -EACCES should be checked in all the existing usage of the api[1] or else
someone else might encounter the same problem.
Yes. Some already check for -ENOSYS, e.g. omap_hsmmc.c
- Any future usage of the api should take of handling this error.Yes, and it should be commented too.
- From a client driver perspective it is not really an error. It is doing theright thing and receiving an error might be confusing.
The error means that the request was not handled. There is no way to find out that requesting this was actually wrong.
Hope this is clear. Also just to add one more point, I adapted this error handling from Linux kernel[2].
The only question for me whether anything would need to detect that the request to disable a regulator is not supported.
Your linux link appears to lead me to regulator_ena_gpio_ctrl(), related to regulator GPIOs. Is that right? It's hard for me to understand what the code there is doing.
Looks like functions are moving around too fast. I am referring to the function _regulator_disable() in the same file[1]. So logic of _regulator_disable() looks something like below:
_regulator_disable() { if (use_count == 1 && !always_on_regulator) ..... ret = _regulator_do_disable() ..... use_count = 0; else use_count--;
return ret; }
Obviously there are more things happening in the function but I just mentioned the details what we require.
Once we make this change we will not be able to go back without breaking things.
I am not really convinced that this patch is the best approach. I do understand your point though. It just worries me that we are hiding something and it will be hard to unhide it later.
What do you think about adding something like regulator_disable_if_allowed() which silently ignored -ENOSYS and -EACCES?
hmm...not sure if this is necessary. But if you feel "detecting the request to disable is not supported" might be needed in future, I can make something regulator_set_enable_if_allowed() and discard -ENOSYS and -EACCESS as you suggested.
[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/driv...
Thanks and regards, Lokesh
Hi Lokesh,
On Sun, 30 Dec 2018 at 23:26, Lokesh Vutla lokeshvutla@ti.com wrote:
Hi Simon,
On 29/12/18 6:58 PM, Simon Glass wrote:
Hi Lokesh,
On Thu, 27 Dec 2018 at 22:33, Lokesh Vutla lokeshvutla@ti.com wrote:
Hi Simon,
On 28/12/18 3:57 AM, Simon Glass wrote:
Hi Lokesh,
On Mon, 24 Dec 2018 at 04:08, Lokesh Vutla lokeshvutla@ti.com wrote:
commit 4f86a724e82c0 ("power: regulator: denied disable on always-on regulator") throws an error when requested to disable an always-on regulator. It is right that an always-on regulator should not be attempted to be disabled. But at the same time regulator framework should not return an error when such request is received. Instead it should just return success without attempting to disable the specified regulator. This is because the requesting driver will not have the idea if the regulator is always-on or not. The requesting driver will always try to enable/disable regulator as per the required flow. So it is upto regulator framework to not break such scenarios.
Can the caller not check the error code? It is -EACCES in this case.
We considered this an one of the option but I ended up fixing regulator framework due to the following reasons:
- If regulator framework returns -EACCES on this scenario then: - -EACCES should be checked in all the existing usage of the api[1] or else
someone else might encounter the same problem.
Yes. Some already check for -ENOSYS, e.g. omap_hsmmc.c
- Any future usage of the api should take of handling this error.Yes, and it should be commented too.
- From a client driver perspective it is not really an error. It is doing theright thing and receiving an error might be confusing.
The error means that the request was not handled. There is no way to find out that requesting this was actually wrong.
Hope this is clear. Also just to add one more point, I adapted this error handling from Linux kernel[2].
The only question for me whether anything would need to detect that the request to disable a regulator is not supported.
Your linux link appears to lead me to regulator_ena_gpio_ctrl(), related to regulator GPIOs. Is that right? It's hard for me to understand what the code there is doing.
Looks like functions are moving around too fast. I am referring to the function _regulator_disable() in the same file[1]. So logic of _regulator_disable() looks something like below:
_regulator_disable() { if (use_count == 1 && !always_on_regulator) ..... ret = _regulator_do_disable() ..... use_count = 0; else use_count--;
return ret;}
Obviously there are more things happening in the function but I just mentioned the details what we require.
Once we make this change we will not be able to go back without breaking things.
I am not really convinced that this patch is the best approach. I do understand your point though. It just worries me that we are hiding something and it will be hard to unhide it later.
What do you think about adding something like regulator_disable_if_allowed() which silently ignored -ENOSYS and -EACCES?
hmm...not sure if this is necessary. But if you feel "detecting the request to disable is not supported" might be needed in future, I can make something regulator_set_enable_if_allowed() and discard -ENOSYS and -EACCESS as you suggested.
I think that would be useful. Another concern I have is that errors that are suppressed at a low level can result in people checking for success in other ways (e.g. reading data back in a separate transaction). I think it is better to explicitly ignore specific error numbers at the level where a decision can be made as to its importance.
If I say 'please do this' I think it should mean 'return an error if you cannot'. With driver model there are quite a few places where errors are returned which might be harmless. For example. gpio_request_by_name() returns -ENOENT if there is no GPIO by that name, but returns -EINVAL if the config is invalid. We may want to ignore the first one (e.g. if the GPIO is optional) but should never ignore the second.
So a function like 'please do this if allowed' makes sense to me in this context.
[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/driv...
Thanks and regards, Lokesh
Regards, Simon
participants (4)
-
Faiz Abbas -
Lokesh Vutla -
Simon Glass -
Tom Rini