linux-kernel - Re: [PATCH v2 1/2] pwm: core: export pwm_get_state

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5nhyokdabeuw73btvkyyvoohbnqqyxpe64dxkrpwa4jvdpdqjr@zmpfnngcceq3>
Date: Wed, 30 Oct 2024 09:05:21 +0100
From: Uwe Kleine-König <u.kleine-koenig@...libre.com>
To: David Lechner <dlechner@...libre.com>
Cc: Jonathan Cameron <jic23@...nel.org>, 
	Guillaume Stols <gstols@...libre.com>, Michael Hennerich <Michael.Hennerich@...log.com>, 
	linux-pwm@...r.kernel.org, linux-kernel@...r.kernel.org, linux-iio@...r.kernel.org
Subject: Re: [PATCH v2 1/2] pwm: core: export pwm_get_state_hw()

Hello David,

On Tue, Oct 29, 2024 at 04:18:49PM -0500, David Lechner wrote:
> Export the pwm_get_state_hw() function. This is useful in cases where
> we want to know what the hardware is actually doing, rather than what
> what we requested it should do.
> 
> Locking had to be rearranged to ensure that the chip is still
> operational before trying to access ops now that this can be called
> from outside the pwm core.

Good point, I didn't notice that in my review of v1.
 
> Signed-off-by: David Lechner <dlechner@...libre.com>
> ---
> 
> v2 changes:
> * Dropped __pwm_get_state_hw() function.
> * Reworded commit message.
> ---
>  drivers/pwm/core.c  | 40 +++++++++++++++++++++++++++-------------
>  include/linux/pwm.h |  1 +
>  2 files changed, 28 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index 4399e793efaf..ccbdd6dd1410 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -718,40 +718,54 @@ int pwm_apply_atomic(struct pwm_device *pwm, const struct pwm_state *state)
>  }
>  EXPORT_SYMBOL_GPL(pwm_apply_atomic);
>  
> -static int pwm_get_state_hw(struct pwm_device *pwm, struct pwm_state *state)
> +/**
> + * pwm_get_state_hw() - get the current PWM state from hardware
> + * @pwm: PWM device
> + * @state: state to fill with the current PWM state
> + *
> + * Similar to pwm_get_state() but reads the current PWM state from hardware
> + * instead of the requested state.
> + *
> + * Returns: 0 on success or a negative error code on failure.
> + * Context: May sleep.
> + */
> +int pwm_get_state_hw(struct pwm_device *pwm, struct pwm_state *state)
>  {
>  	struct pwm_chip *chip = pwm->chip;
>  	const struct pwm_ops *ops = chip->ops;
>  	int ret = -EOPNOTSUPP;
>  
> +	might_sleep();

Maybe this should be better

	if (!chip->atomic)
		might_sleep();

but I'm open to keep it unconditional until someone wails about it.

> +	guard(pwmchip)(chip);
> +
> +	if (!chip->operational)
> +		return -ENODEV;
> +

Huh, that means that __pwm_read_waveform() et al were called before
without holding the chip lock. How embarrassing. I think nothing bad
happens (because at this stage the PWM wasn't exposed to its consumer
yet and so no concurrency could happen), but still.

>  	if (ops->read_waveform) {
>  		char wfhw[WFHWSIZE];
>  		struct pwm_waveform wf;
>  
>  		BUG_ON(WFHWSIZE < ops->sizeof_wfhw);
>  
> -		scoped_guard(pwmchip, chip) {
> +		ret = __pwm_read_waveform(chip, pwm, &wfhw);
> +		if (ret)
> +			return ret;
>  
> -			ret = __pwm_read_waveform(chip, pwm, &wfhw);
> -			if (ret)
> -				return ret;
> -
> -			ret = __pwm_round_waveform_fromhw(chip, pwm, &wfhw, &wf);
> -			if (ret)
> -				return ret;
> -		}
> +		ret = __pwm_round_waveform_fromhw(chip, pwm, &wfhw, &wf);
> +		if (ret)
> +			return ret;
>  
>  		pwm_wf2state(&wf, state);
>  
>  	} else if (ops->get_state) {
> -		scoped_guard(pwmchip, chip)
> -			ret = ops->get_state(chip, pwm, state);
> -
> +		ret = ops->get_state(chip, pwm, state);
>  		trace_pwm_get(pwm, state, ret);
>  	}
>  
>  	return ret;
>  }
> +EXPORT_SYMBOL_GPL(pwm_get_state_hw);
>  
>  /**
>   * pwm_adjust_config() - adjust the current PWM config to the PWM arguments

I applied that patch to
https://git.kernel.org/pub/scm/linux/kernel/git/ukleinek/linux.git pwm/for-next

Best regards
Uwe

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)