linux-kernel - Re: [PATCH RFC v4 01/15] 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: <mavlxxjza7ud7ylgoewz6fz3chtuwljvcjjf6o3kcv555iolwa@wdnrsiow5u5w>
Date: Tue, 29 Oct 2024 09:05:21 +0100
From: Uwe Kleine-König <u.kleine-koenig@...libre.com>
To: David Lechner <dlechner@...libre.com>
Cc: Mark Brown <broonie@...nel.org>, Jonathan Cameron <jic23@...nel.org>, 
	Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, 
	Conor Dooley <conor+dt@...nel.org>, Nuno Sá <nuno.sa@...log.com>, 
	Michael Hennerich <Michael.Hennerich@...log.com>, Lars-Peter Clausen <lars@...afoo.de>, 
	David Jander <david@...tonic.nl>, Martin Sperl <kernel@...tin.sperl.org>, 
	linux-spi@...r.kernel.org, devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-iio@...r.kernel.org, linux-pwm@...r.kernel.org
Subject: Re: [PATCH RFC v4 01/15] pwm: core: export pwm_get_state_hw()

Hello David,

On Wed, Oct 23, 2024 at 03:59:08PM -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.
> 
> Signed-off-by: David Lechner <dlechner@...libre.com>
> ---
> 
> v4 changes: new patch in v4
> 
> And FYI for Uwe and Jonathan, there are a couple of other series
> introducing PWM conversion triggers that could make use of this
> so that the sampling_frequency attribute can return the actual rate
> rather than the requested rate.
> 
> Already applied:
> https://lore.kernel.org/linux-iio/20241015-ad7606_add_iio_backend_support-v5-4-654faf1ae08c@baylibre.com/
> 
> Under review:
> https://lore.kernel.org/linux-iio/aea7f92b-3d12-4ced-b1c8-90bcf1d992d3@baylibre.com/T/#m1377d5acd7e996acd1f59038bdd09f0742d3ac35
> ---
>  drivers/pwm/core.c  | 55 +++++++++++++++++++++++++++++++++++++----------------
>  include/linux/pwm.h |  1 +
>  2 files changed, 40 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index 634be56e204b..a214d0165d09 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -718,7 +718,7 @@ 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)
> +static 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;
> @@ -730,29 +730,50 @@ static int pwm_get_state_hw(struct pwm_device *pwm, struct pwm_state *state)
>  
>  		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;
>  }

I don't understand why you introduce __pwm_get_state_hw() (a variant of
pwm_get_state_hw() that expects the caller to hold the chip lock) when the
single caller (apart from plain pwm_get_state_hw()) could just continue
to use pwm_get_state_hw().

In principle I'm open to such a patch and wonder if there is already a
merge plan for this series. If you send a simpler patch soon with the
same objective, I'll make sure it goes into v6.13-rc1 in the assumption
that it's to late for the whole series to go in then. Or do you still
target 6.13-rc1 for the spi bits? Then it would probably better to let
this patch go in with the rest via the spi tree.

Best regards
Uwe

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