| 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
| ||
|
Message-ID: <s6lapsx6dukr4ebknbbalbhi7232ivxl2apaqtardfi6f2ybbf@zvlhxlngbgx3>
Date: Sun, 23 Jun 2024 11:32:45 +0200
From: Uwe Kleine-König <u.kleine-koenig@...libre.com>
To: Martin Blumenstingl <martin.blumenstingl@...glemail.com>
Cc: linux-pwm@...r.kernel.org, linux-kernel@...r.kernel.org,
broonie@...nel.org, lgirdwood@...il.com
Subject: Re: [RFC PATCH v2] regulator: pwm-regulator: Make assumptions about
disabled PWM consistent
Hello Martin,
On Sat, Jun 22, 2024 at 09:15:04PM +0200, Martin Blumenstingl wrote:
> Generally it's not known how a disabled PWM behaves. However if the
> bootloader hands over a disabled PWM that drives a regulator and it's
> claimed the regulator is enabled, then the most likely assumption is
> that the PWM emits the inactive level. This is represented by duty_cycle
> = 0 even for .polarity == PWM_POLARITY_INVERSED.
I'd write: "This is represented by duty_cycle = 0 independent of the
polarity."
> Change the implementation to always use duty_cycle = 0, regardless of
> the polarity. Also update the code so it keeps a copy of the pwm_state
> around. Both of these changes result in the fact that the logic from
> pwm_regulator_init_boot_on() is much less complex and can be simplified
> to a point where the whole function is not needed anymore.
In my (German) ear the following sounds a bit nicer:
Both of these changes reduce the complexity of
pwm_regulator_init_boot_on() to a point where the whole function
is not needed anymore.
> Suggested-by: Uwe Kleine-König <u.kleine-koenig@...libre.com>
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@...glemail.com>
> ---
> Changes from v1 [0] (which was sent by Uwe):
> - keep the struct pwm_state around to prevent a regression on Meson8b
> Odroid-C1 boards where just calling pwm_enable() without adjusting
> the duty_cycle to 0 would hang the board
> - I'm actively looking for input on the commit description and
> suggestions whether/why Fixes tags should be applied
Apart of the nitpicking above, I like the commit description.
Regarding a Fixes tag: I'm unsure if without this patch, the
pwm-regulator driver is broken for your Odroid-C1 board. It's not,
right?
I think I wouldn't add a Fixes tag and just consider this patch a
cleanup then.
> [0] https://lore.kernel.org/lkml/CAFBinCB+S1wOpD-fCbcTORqXSV00Sd7-1GHUKY+rO859_dkhOA@mail.gmail.com/T/
>
>
> drivers/regulator/pwm-regulator.c | 119 ++++++++++++++----------------
> 1 file changed, 55 insertions(+), 64 deletions(-)
>
> diff --git a/drivers/regulator/pwm-regulator.c b/drivers/regulator/pwm-regulator.c
> index 7434b6b22d32..5ea354a0654a 100644
> --- a/drivers/regulator/pwm-regulator.c
> +++ b/drivers/regulator/pwm-regulator.c
> @@ -41,6 +41,8 @@ struct pwm_regulator_data {
>
> /* Enable GPIO */
> struct gpio_desc *enb_gpio;
> +
> + struct pwm_state pwm_state;
> };
>
> struct pwm_voltages {
> @@ -48,18 +50,33 @@ struct pwm_voltages {
> unsigned int dutycycle;
> };
>
> +static int pwm_regulator_apply_state(struct regulator_dev *rdev,
> + struct pwm_state *new_state)
> +{
> + struct pwm_regulator_data *drvdata = rdev_get_drvdata(rdev);
> + int ret;
> +
> + ret = pwm_apply_might_sleep(drvdata->pwm, new_state);
> + if (ret) {
> + dev_err(&rdev->dev, "Failed to configure PWM: %d\n", ret);
> + return ret;
> + }
> +
> + drvdata->pwm_state = *new_state;
> +
> + return 0;
> +}
> +
> /*
> * Voltage table call-backs
> */
> static void pwm_regulator_init_state(struct regulator_dev *rdev)
> {
> struct pwm_regulator_data *drvdata = rdev_get_drvdata(rdev);
> - struct pwm_state pwm_state;
> unsigned int dutycycle;
> int i;
>
> - pwm_get_state(drvdata->pwm, &pwm_state);
> - dutycycle = pwm_get_relative_duty_cycle(&pwm_state, 100);
> + dutycycle = pwm_get_relative_duty_cycle(&drvdata->pwm_state, 100);
>
> for (i = 0; i < rdev->desc->n_voltages; i++) {
> if (dutycycle == drvdata->duty_cycle_table[i].dutycycle) {
> @@ -83,18 +100,15 @@ static int pwm_regulator_set_voltage_sel(struct regulator_dev *rdev,
> unsigned selector)
> {
> struct pwm_regulator_data *drvdata = rdev_get_drvdata(rdev);
> - struct pwm_state pstate;
> + struct pwm_state pstate = drvdata->pwm_state;
> int ret;
>
> - pwm_init_state(drvdata->pwm, &pstate);
> pwm_set_relative_duty_cycle(&pstate,
> drvdata->duty_cycle_table[selector].dutycycle, 100);
>
> - ret = pwm_apply_might_sleep(drvdata->pwm, &pstate);
> - if (ret) {
> - dev_err(&rdev->dev, "Failed to configure PWM: %d\n", ret);
> + ret = pwm_regulator_apply_state(rdev, &pstate);
> + if (ret)
> return ret;
> - }
If you drop the local variable pstate and just do
pwm_set_relative_duty_cycle(drvdata->pwm_state,
drvdata->duty_cycle_table[selector].dutycycle, 100);
you might get a mismatch between actual configuration and
drvdata->pwm_state if pwm_regulator_apply_state() fails, but I think
that doesn't matter and simplifies the code a bit. (Drop the assignment
in pwm_regulator_apply_state() then.)
> drvdata->state = selector;
>
> @@ -115,17 +129,26 @@ static int pwm_regulator_list_voltage(struct regulator_dev *rdev,
> static int pwm_regulator_enable(struct regulator_dev *dev)
> {
> struct pwm_regulator_data *drvdata = rdev_get_drvdata(dev);
> + struct pwm_state pstate = drvdata->pwm_state;
>
> gpiod_set_value_cansleep(drvdata->enb_gpio, 1);
>
> - return pwm_enable(drvdata->pwm);
> + pstate.enabled = true;
> +
> + return pwm_regulator_apply_state(dev, &pstate);
> }
>
> static int pwm_regulator_disable(struct regulator_dev *dev)
> {
> struct pwm_regulator_data *drvdata = rdev_get_drvdata(dev);
> + struct pwm_state pstate = drvdata->pwm_state;
> + int ret;
> +
> + pstate.enabled = false;
>
> - pwm_disable(drvdata->pwm);
> + ret = pwm_regulator_apply_state(dev, &pstate);
> + if (ret)
> + return ret;
With that part I'm a bit unhappy. You don't know what the pwm does when
disabled (it might yield a 100% relative duty cycle). So the safe bet is
to use .enabled=true, .duty_cycle=0 here.
The only code that "knows" if it's safe to disable the PWM is the
lowlevel pwm driver.
> gpiod_set_value_cansleep(drvdata->enb_gpio, 0);
>
> @@ -139,7 +162,7 @@ static int pwm_regulator_is_enabled(struct regulator_dev *dev)
> if (drvdata->enb_gpio && !gpiod_get_value_cansleep(drvdata->enb_gpio))
> return false;
>
> - return pwm_is_enabled(drvdata->pwm);
> + return drvdata->pwm_state.enabled;
> }
Thanks for picking up the task to improve my patch!
Best regards
Uwe
Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)
Powered by blists - more mailing lists