| 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: <2e157deb-de8d-9f12-b4f2-04e04ff2e5ed@linaro.org>
Date: Fri, 27 Jul 2018 12:32:33 +0100
From: Daniel Thompson <daniel.thompson@...aro.org>
To: Enric Balletbo i Serra <enric.balletbo@...labora.com>,
linux-kernel@...r.kernel.org
Cc: kernel@...labora.com, cl@...k-chips.com, linux-pwm@...r.kernel.org,
linux-fbdev@...r.kernel.org,
Thierry Reding <thierry.reding@...il.com>,
Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>,
dri-devel@...ts.freedesktop.org, Jingoo Han <jingoohan1@...il.com>,
Lee Jones <lee.jones@...aro.org>
Subject: Re: [PATCH] backlight: pwm_bl: switch to using "atomic" PWM API
On 26/07/18 10:15, Enric Balletbo i Serra wrote:
> The "atomic" API allows us to configure PWM period and duty_cycle and
> enable it in one call.
>
> The patch also moves the pwm_init_state just before any use of the
> pwm_state struct, this fixes a potential bug where pwm_get_state
> can be called before pwm_init_state.
>
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@...labora.com>
> ---
>
> drivers/video/backlight/pwm_bl.c | 48 ++++++++++++++++++++------------
> 1 file changed, 30 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> index bdfcc0a71db1..2c734d55d607 100644
> --- a/drivers/video/backlight/pwm_bl.c
> +++ b/drivers/video/backlight/pwm_bl.c
> @@ -46,7 +46,8 @@ struct pwm_bl_data {
> void (*exit)(struct device *);
> };
>
> -static void pwm_backlight_power_on(struct pwm_bl_data *pb, int brightness)
> +static void pwm_backlight_power_on(struct pwm_bl_data *pb,
> + struct pwm_state *state)
> {
> int err;
>
> @@ -57,7 +58,8 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb, int brightness)
> if (err < 0)
> dev_err(pb->dev, "failed to enable power supply\n");
>
> - pwm_enable(pb->pwm);
> + state->enabled = true;
> + pwm_apply_state(pb->pwm, state);
>
> if (pb->post_pwm_on_delay)
> msleep(pb->post_pwm_on_delay);
> @@ -70,6 +72,8 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb, int brightness)
>
> static void pwm_backlight_power_off(struct pwm_bl_data *pb)
> {
> + struct pwm_state state;
> +
> if (!pb->enabled)
> return;
>
> @@ -79,8 +83,11 @@ static void pwm_backlight_power_off(struct pwm_bl_data *pb)
> if (pb->pwm_off_delay)
> msleep(pb->pwm_off_delay);
>
> - pwm_config(pb->pwm, 0, pb->period);
> - pwm_disable(pb->pwm);
> + pwm_get_state(pb->pwm, &state);
> + state.enabled = false;
> + state.period = pb->period;
> + state.duty_cycle = 0;
> + pwm_apply_state(pb->pwm, &state);
>
> regulator_disable(pb->power_supply);
> pb->enabled = false;
> @@ -106,6 +113,7 @@ static int pwm_backlight_update_status(struct backlight_device *bl)
> {
> struct pwm_bl_data *pb = bl_get_data(bl);
> int brightness = bl->props.brightness;
> + struct pwm_state state;
> int duty_cycle;
>
> if (bl->props.power != FB_BLANK_UNBLANK ||
> @@ -118,8 +126,13 @@ static int pwm_backlight_update_status(struct backlight_device *bl)
>
> if (brightness > 0) {
> duty_cycle = compute_duty_cycle(pb, brightness);
> - pwm_config(pb->pwm, duty_cycle, pb->period);
> - pwm_backlight_power_on(pb, brightness);
> + pwm_get_state(pb->pwm, &state);
> + state.duty_cycle = duty_cycle;
> + state.period = pb->period;
> + if (!state.enabled)
> + pwm_backlight_power_on(pb, &state);
> + else
> + pwm_apply_state(pb->pwm, &state);
> } else
> pwm_backlight_power_off(pb);
>
> @@ -447,7 +460,6 @@ static int pwm_backlight_probe(struct platform_device *pdev)
> struct device_node *node = pdev->dev.of_node;
> struct pwm_bl_data *pb;
> struct pwm_state state;
> - struct pwm_args pargs;
> unsigned int i;
> int ret;
>
> @@ -539,10 +551,17 @@ static int pwm_backlight_probe(struct platform_device *pdev)
>
> dev_dbg(&pdev->dev, "got pwm for backlight\n");
>
> - if (!data->levels) {
> - /* Get the PWM period (in nanoseconds) */
> - pwm_get_state(pb->pwm, &state);
> + /* Sync up PWM state and ensure it is off. */
> + pwm_init_state(pb->pwm, &state);
> + state.enabled = false;
> + ret = pwm_apply_state(pb->pwm, &state);
Why do we ensure the PWM is off? Does this cause backlight flickers or
make some of the code in pwm_backlight_initial_power_state() unreachable?
> + if (ret) {
> + dev_err(&pdev->dev, "failed to apply initial PWM state: %d\n",
> + ret);
> + goto err_alloc;
> + }
>
> + if (!data->levels) {
> ret = pwm_backlight_brightness_default(&pdev->dev, data,
> state.period);
> if (ret < 0) {
> @@ -559,20 +578,13 @@ static int pwm_backlight_probe(struct platform_device *pdev)
> pb->levels = data->levels;
> }
>
> - /*
> - * FIXME: pwm_apply_args() should be removed when switching to
> - * the atomic PWM API.
> - */
> - pwm_apply_args(pb->pwm);
> -
> /*
> * The DT case will set the pwm_period_ns field to 0 and store the
> * period, parsed from the DT, in the PWM device. For the non-DT case,
> * set the period from platform data if it has not already been set
> * via the PWM lookup table.
> */
> - pwm_get_args(pb->pwm, &pargs);
> - pb->period = pargs.period;
> + pb->period = state.period;
> if (!pb->period && (data->pwm_period_ns > 0))
> pb->period = data->pwm_period_ns;
Could we have delayed applying the state until we know what the period
is supposed to be? No other call to pwm_apply_state() has its error
value checked... so if there are problems with the period we could
detect them here.
Note also that we can guarantee the period is set before the probe
completes then I think pb->period could be removed entirely. It was only
really being carried around to help with calls to pwm_config() and these
no longer exist.
Daniel.
Powered by blists - more mailing lists