pwm: soften potential loss of precision in compat code

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(-)

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
>

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

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.