| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 26 Jun 2023 17:04:58 +0300
From: Matti Vaittinen <mazziesaccount@...il.com>
To: Benjamin Bara <bbara93@...il.com>,
Liam Girdwood <lgirdwood@...il.com>,
Mark Brown <broonie@...nel.org>
Cc: support.opensource@...semi.com,
DLG-Adam.Ward.opensource@...renesas.com,
Martin Fuzzey <martin.fuzzey@...wbird.group>,
linux-kernel@...r.kernel.org,
Benjamin Bara <benjamin.bara@...data.com>
Subject: Re: [PATCH RFC v4 08/13] regulator: move monitor handling into own
function
On 6/20/23 23:03, Benjamin Bara wrote:
> From: Benjamin Bara <benjamin.bara@...data.com>
>
> Extract the current monitor handling into an own function and create
> helper for initialization, disabling and re-enabling of monitors.
> For reenabling the monitors, the current state and mode is considered to
> avoid entering an invalid state on regulators with enabled workarounds.
>
> Additionally, monitors of disabled regulators are not disabled before
> changing state. The mon_disable_reg_disabled property is still respected
> in this case, because turning off the monitor happens when the regulator
> is still enabled.
>
> Differ between initialization and normal "workaround handling" when an
> EOPNOTSUPP is returned.
>
> Signed-off-by: Benjamin Bara <benjamin.bara@...data.com>
> ---
> drivers/regulator/core.c | 234 +++++++++++++++++++++++++++++++++++------------
> 1 file changed, 178 insertions(+), 56 deletions(-)
>
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index 9bddab17450e..873e53633698 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -1426,7 +1426,7 @@ static int notif_set_limit(struct regulator_dev *rdev,
>
> static int handle_notify_limits(struct regulator_dev *rdev,
> int (*set)(struct regulator_dev *, int, int, bool),
> - struct notification_limit *limits)
> + const struct notification_limit *limits)
> {
> int ret = 0;
>
> @@ -1451,6 +1451,180 @@ static int handle_notify_limits(struct regulator_dev *rdev,
>
> return ret;
> }
> +
> +static const struct notification_limit disable_limits = {
> + .prot = REGULATOR_NOTIF_LIMIT_DISABLE,
> + .err = REGULATOR_NOTIF_LIMIT_DISABLE,
> + .warn = REGULATOR_NOTIF_LIMIT_DISABLE,
> +};
> +
> +static int monitors_set_state(struct regulator_dev *rdev, bool enable,
> + unsigned int mons)
> +{
> + const struct regulation_constraints *reg_c = rdev->constraints;
> + const struct regulator_ops *ops = rdev->desc->ops;
> + int tmp, ret = 0;
> +
> + rdev_dbg(rdev, "%s: en: %d, mons: %x\n", __func__, enable, mons);
> +
> + /* only set the state if monitoring is activated in the device-tree. */
> + if ((mons & REGULATOR_MONITOR_OVER_VOLTAGE) && reg_c->over_voltage_detection) {
> + tmp = handle_notify_limits(rdev, ops->set_over_voltage_protection,
> + enable ? ®_c->over_voltage_limits
> + : &disable_limits);
> + if (tmp) {
> + if (tmp != -EOPNOTSUPP) {
> + rdev_err(rdev, "failed to set over voltage limits %pe\n",
> + ERR_PTR(tmp));
> + return tmp;
> + }
> + rdev_warn(rdev,
> + "IC does not support requested over voltage limits\n");
> + ret = tmp;
> + }
> + }
> + if ((mons & REGULATOR_MONITOR_UNDER_VOLTAGE) && reg_c->under_voltage_detection) {
> + tmp = handle_notify_limits(rdev, ops->set_under_voltage_protection,
> + enable ? ®_c->under_voltage_limits
> + : &disable_limits);
> + if (tmp) {
> + if (tmp != -EOPNOTSUPP) {
> + rdev_err(rdev, "failed to set under voltage limits %pe\n",
> + ERR_PTR(tmp));
> + return ret;
> + }
> + rdev_warn(rdev,
> + "IC does not support requested under voltage limits\n");
> + ret = tmp;
> + }
> + }
> + if ((mons & REGULATOR_MONITOR_OVER_CURRENT) && reg_c->over_current_detection) {
> + tmp = handle_notify_limits(rdev, ops->set_over_current_protection,
> + enable ? ®_c->over_curr_limits
> + : &disable_limits);
> + if (ret) {
> + if (tmp != -EOPNOTSUPP) {
> + rdev_err(rdev, "failed to set over current limits: %pe\n",
> + ERR_PTR(tmp));
> + return tmp;
> + }
> + rdev_warn(rdev,
> + "IC does not support requested over-current limits\n");
> + ret = tmp;
> + }
> + }
> + if ((mons & REGULATOR_MONITOR_OVER_TEMPERATURE) && reg_c->over_temp_detection) {
> + tmp = handle_notify_limits(rdev, ops->set_thermal_protection,
> + enable ? ®_c->temp_limits
> + : &disable_limits);
> + if (tmp) {
> + if (tmp != -EOPNOTSUPP) {
> + rdev_err(rdev, "failed to set temperature limits %pe\n",
> + ERR_PTR(tmp));
> + return tmp;
> + }
> + rdev_warn(rdev,
> + "IC does not support requested temperature limits\n");
> + ret = tmp;
> + }
> + }
> +
> + return ret;
> +}
> +
> +/**
> + * monitors_disable - disables given monitors if the regulator is enabled
> + * @rdev: regulator source
> + * @mons: monitors to enable
> + */
> +static int monitors_disable(struct regulator_dev *rdev, unsigned int mons)
> +{
> + int reg_enabled;
> +
> + if (!mons)
> + return 0;
Just a minor thing but can we do this check already at the caller side?
I think that would show the logic more clearly already in the functions
implementing the actual action requested by the user.
(disable/enable/change voltage). Eg, the logic in those functions would
be clear:
if (flag_to_do_magic_monitor_toggling)
monitors_disable();
and similarly for the monitors_reenable()...
> +
> + reg_enabled = _regulator_is_enabled(rdev);
> + if (reg_enabled <= 0)
> + return reg_enabled;
> +
> + return monitors_set_state(rdev, false, mons);
> +}
> +
> +/**
> + * monitors_enable - enables given monitors
> + * @rdev: regulator source
> + * @mons: monitors to enable
> + *
> + * Enables monitors based on their workaround properties and the current state
> + * or mode.
> + */
> +static int monitors_enable(struct regulator_dev *rdev, unsigned int mons)
> +{
> + const struct regulator_desc *desc = rdev->desc;
> + const struct regulator_ops *ops = desc->ops;
> +
> + /* don't enable monitors if regulator is in unsupported mode. */
> + if (desc->mon_unsupported_reg_modes &&
> + (desc->mon_unsupported_reg_modes & ops->get_mode(rdev)))
> + return 0;
> +
> + /* don't enable monitor on disabled regulator with workaround active. */
> + if (mons & desc->mon_disable_reg_disabled) {
> + int reg_enabled = _regulator_is_enabled(rdev);
> +
> + if (reg_enabled < 0)
> + return reg_enabled;
> + if (!reg_enabled)
> + mons &= ~desc->mon_disable_reg_disabled;
> + }
> +
> + return monitors_set_state(rdev, true, mons);
> +}
> +
> +static int monitors_init(struct regulator_dev *rdev)
> +{
> + unsigned int mons = REGULATOR_MONITOR_NONE;
> + int reg_enabled = _regulator_is_enabled(rdev);
> + int ret;
> +
> + /*
> + * Ensure that monitors of disabled regulators with respective
> + * workaround active are disabled during initialization.
> + */
> + if (reg_enabled < 0)
> + return reg_enabled;
> + if (!reg_enabled && rdev->desc->mon_disable_reg_disabled) {
> + mons = rdev->desc->mon_disable_reg_disabled;
> + ret = monitors_set_state(rdev, false, mons);
> + }
> +
> + /* Ignore EOPNOTSUPP at initialization, but not during workarounds. */
> + ret = monitors_enable(rdev, ~mons);
> + if (ret && ret != -EOPNOTSUPP)
> + return ret;
> +
> + return 0;
> +}
> +
> +static int monitors_reenable(struct regulator_dev *rdev, unsigned int mons)
> +{
> + int reg_enabled;
> +
> + if (!mons)
> + return 0;
> +
...here.
> + /*
> + * Monitors of disabled regulators are not turned off, therefore skip
> + * turning on.
> + */
> + reg_enabled = _regulator_is_enabled(rdev);
> + if (reg_enabled <= 0)
> + return reg_enabled;
> +
> + return monitors_enable(rdev, mons);
> +}
> +
Sorry but my flight landed so I need to stop reviewing for now... This
will be a busy week for me. It may be I can't go through rest of the
patches until later :/
Yours,
-- Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
Powered by blists - more mailing lists