[PATCH] pwm: soften potential loss of precision in compat code

[PATCH] pwm: soften potential loss of precision in compat code

[Uwe Kleine-König]

The legacy callback .config() only uses int for period and duty_cycle
while the corresponding values in struct pwm_state are u64. To prevent
that a value bigger than INT_MAX is discarded to a very small value,
explicitly check for big values and pass INT_MAX instead of discarding.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/core.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index c4d5c0667137..4058d3c86a45 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -599,9 +599,20 @@ int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state)
 
 		if (state->period != pwm->state.period ||
 		    state->duty_cycle != pwm->state.duty_cycle) {
+			int duty_cycle, period;
+
+			if (state->period < INT_MAX)
+				period = state->period;
+			else
+				period = INT_MAX;
+
+			if (state->duty_cycle < INT_MAX)
+				duty_cycle = state->duty_cycle;
+			else
+				duty_cycle = INT_MAX;
+
 			err = chip->ops->config(pwm->chip, pwm,
-						state->duty_cycle,
-						state->period);
+						duty_cycle, period);
 			if (err)
 				return err;
 
-- 
2.30.1

Re: [PATCH] pwm: soften potential loss of precision in compat code

[Guru Das Srinagesh]

On Mon, Mar 08, 2021 at 10:23:22AM +0100, Uwe Kleine-König wrote:
>  		if (state->period != pwm->state.period ||
>  		    state->duty_cycle != pwm->state.duty_cycle) {
> +			int duty_cycle, period;
> +
> +			if (state->period < INT_MAX)
> +				period = state->period;
> +			else
> +				period = INT_MAX;

Using a MIN() macro here might improve readability:
	period = MIN(state->period, INT_MAX);
> +
> +			if (state->duty_cycle < INT_MAX)
> +				duty_cycle = state->duty_cycle;
> +			else
> +				duty_cycle = INT_MAX;

Same here.

> +
>  			err = chip->ops->config(pwm->chip, pwm,
> -						state->duty_cycle,
> -						state->period);
> +						duty_cycle, period);
>  			if (err)
>  				return err;
>  
> -- 
> 2.30.1
> 

Re: [PATCH] pwm: soften potential loss of precision in compat code

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1414 bytes --]

On Thu, Mar 11, 2021 at 01:06:39PM -0800, Guru Das Srinagesh wrote:
> On Mon, Mar 08, 2021 at 10:23:22AM +0100, Uwe Kleine-König wrote:
> >  		if (state->period != pwm->state.period ||
> >  		    state->duty_cycle != pwm->state.duty_cycle) {
> > +			int duty_cycle, period;
> > +
> > +			if (state->period < INT_MAX)
> > +				period = state->period;
> > +			else
> > +				period = INT_MAX;
> 
> Using a MIN() macro here might improve readability:
> 	period = MIN(state->period, INT_MAX);

Which MIN macro. There are 17 defined in the kernel and none of them in
a header that could be sensibly included by this code.

There are some helpers in <linux/minmax.h> which would result in:

	period = min_t(u64, state->period, INT_MAX)

or

	period = min(state->period, (u64)INT_MAX);

. I don't feel strong here, my initial variant needs more vertical space
but might be a tad easier to understand. In retrospect I'd say that
adding a comment would be more imporant than how to actually calculate
the value, something like:

	/*
	 * The legacy callbacks use only (signed!) int for period and
	 * duty_cycle compared to u64 in struct pwm_state. So clamp the
	 * values to INT_MAX.
	 */

Sounds sensible?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] pwm: soften potential loss of precision in compat code

[Guru Das Srinagesh]

On Fri, Mar 12, 2021 at 08:12:33AM +0100, Uwe Kleine-König wrote:
> On Thu, Mar 11, 2021 at 01:06:39PM -0800, Guru Das Srinagesh wrote:
> > On Mon, Mar 08, 2021 at 10:23:22AM +0100, Uwe Kleine-König wrote:
> > >  		if (state->period != pwm->state.period ||
> > >  		    state->duty_cycle != pwm->state.duty_cycle) {
> > > +			int duty_cycle, period;
> > > +
> > > +			if (state->period < INT_MAX)
> > > +				period = state->period;
> > > +			else
> > > +				period = INT_MAX;
> > 
> > Using a MIN() macro here might improve readability:
> > 	period = MIN(state->period, INT_MAX);
> 
> Which MIN macro. There are 17 defined in the kernel and none of them in
> a header that could be sensibly included by this code.
> 
> There are some helpers in <linux/minmax.h> which would result in:
> 
> 	period = min_t(u64, state->period, INT_MAX)
> 
> or
> 
> 	period = min(state->period, (u64)INT_MAX);
> 
> . I don't feel strong here, my initial variant needs more vertical space
> but might be a tad easier to understand. In retrospect I'd say that
> adding a comment would be more imporant than how to actually calculate
> the value, something like:
> 
> 	/*
> 	 * The legacy callbacks use only (signed!) int for period and
> 	 * duty_cycle compared to u64 in struct pwm_state. So clamp the
> 	 * values to INT_MAX.
> 	 */
> 
> Sounds sensible?

Yes, I agree.

Guru Das.