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

[Guru Das Srinagesh <gurus@codeaurora.org>]

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.