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

[PATCH v3] 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.

Acked-by: Guru Das Srinagesh <gurus@codeaurora.org>
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
Changes since v2 (Message-Id: 20210312212119.1342666-1-u.kleine-koenig@pengutronix.de)

 - Fixed indention of comment (noticed by Guru Das)
 - Add Ack for Guru Das.

 drivers/pwm/core.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 4b3779d58c5a..b1adf3bb8508 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -605,9 +605,18 @@ 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;
+
+			/*
+			 * 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.
+			 */
+			period = min(state->period, (u64)INT_MAX);
+			duty_cycle = min(state->duty_cycle, (u64)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 v3] pwm: Soften potential loss of precision in compat code

[Thierry Reding]

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

On Mon, Mar 15, 2021 at 09:00:51AM +0100, Uwe Kleine-König wrote:
> 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.
> 
> Acked-by: Guru Das Srinagesh <gurus@codeaurora.org>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
> Changes since v2 (Message-Id: 20210312212119.1342666-1-u.kleine-koenig@pengutronix.de)
> 
>  - Fixed indention of comment (noticed by Guru Das)
>  - Add Ack for Guru Das.
> 
>  drivers/pwm/core.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index 4b3779d58c5a..b1adf3bb8508 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -605,9 +605,18 @@ 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;
> +
> +			/*
> +			 * 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.
> +			 */
> +			period = min(state->period, (u64)INT_MAX);
> +			duty_cycle = min(state->duty_cycle, (u64)INT_MAX);

Do we want to highlight this using a WARN()? It seems to me like doing
this would always be a programming error and easy to fix. Silently
truncating this to just INT_MAX may not give the desired effect and be
actively wrong most of the time.

Come to think of it: why not just refuse such requests with -EINVAL?
That's what drivers already do if they're faced with values that they
can't handle.

Thierry

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

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

[Uwe Kleine-König]

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

On Mon, Mar 22, 2021 at 12:12:56PM +0100, Thierry Reding wrote:
> On Mon, Mar 15, 2021 at 09:00:51AM +0100, Uwe Kleine-König wrote:
> > 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.
> > 
> > Acked-by: Guru Das Srinagesh <gurus@codeaurora.org>
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> > Changes since v2 (Message-Id: 20210312212119.1342666-1-u.kleine-koenig@pengutronix.de)
> > 
> >  - Fixed indention of comment (noticed by Guru Das)
> >  - Add Ack for Guru Das.
> > 
> >  drivers/pwm/core.c | 13 +++++++++++--
> >  1 file changed, 11 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> > index 4b3779d58c5a..b1adf3bb8508 100644
> > --- a/drivers/pwm/core.c
> > +++ b/drivers/pwm/core.c
> > @@ -605,9 +605,18 @@ 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;
> > +
> > +			/*
> > +			 * 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.
> > +			 */
> > +			period = min(state->period, (u64)INT_MAX);
> > +			duty_cycle = min(state->duty_cycle, (u64)INT_MAX);
> 
> Do we want to highlight this using a WARN()?

That would be fine for me, too. In the past you were not happy with
WARN I added, so I implemented this in a silent way.

> It seems to me like doing this would always be a programming error and
> easy to fix. Silently truncating this to just INT_MAX may not give the
> desired effect and be actively wrong most of the time.
> 
> Come to think of it: why not just refuse such requests with -EINVAL?
> That's what drivers already do if they're faced with values that they
> can't handle.

No, the strategy I ask authors of new drivers to implement is to program
the biggest possible period not bigger than the requested period. So if
a consumer requests INT_MAX+3 it must already today cope with the case
that it gets a smaller period.

The underlaying problem can only be solved with a way to query the
resulting configuration for a given request. I have a prototype I can
share if you're interested.

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 v3] pwm: Soften potential loss of precision in compat code

[Thierry Reding]

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

On Mon, Mar 22, 2021 at 12:29:47PM +0100, Uwe Kleine-König wrote:
> On Mon, Mar 22, 2021 at 12:12:56PM +0100, Thierry Reding wrote:
> > On Mon, Mar 15, 2021 at 09:00:51AM +0100, Uwe Kleine-König wrote:
> > > 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.
> > > 
> > > Acked-by: Guru Das Srinagesh <gurus@codeaurora.org>
> > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > ---
> > > Changes since v2 (Message-Id: 20210312212119.1342666-1-u.kleine-koenig@pengutronix.de)
> > > 
> > >  - Fixed indention of comment (noticed by Guru Das)
> > >  - Add Ack for Guru Das.
> > > 
> > >  drivers/pwm/core.c | 13 +++++++++++--
> > >  1 file changed, 11 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> > > index 4b3779d58c5a..b1adf3bb8508 100644
> > > --- a/drivers/pwm/core.c
> > > +++ b/drivers/pwm/core.c
> > > @@ -605,9 +605,18 @@ 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;
> > > +
> > > +			/*
> > > +			 * 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.
> > > +			 */
> > > +			period = min(state->period, (u64)INT_MAX);
> > > +			duty_cycle = min(state->duty_cycle, (u64)INT_MAX);
> > 
> > Do we want to highlight this using a WARN()?
> 
> That would be fine for me, too. In the past you were not happy with
> WARN I added, so I implemented this in a silent way.

The WARN that I remember was in a completely different context. There
are occasions when it's good to be loud. I think this is one of them.

> > It seems to me like doing this would always be a programming error and
> > easy to fix. Silently truncating this to just INT_MAX may not give the
> > desired effect and be actively wrong most of the time.
> > 
> > Come to think of it: why not just refuse such requests with -EINVAL?
> > That's what drivers already do if they're faced with values that they
> > can't handle.
> 
> No, the strategy I ask authors of new drivers to implement is to program
> the biggest possible period not bigger than the requested period. So if
> a consumer requests INT_MAX+3 it must already today cope with the case
> that it gets a smaller period.

That just seems wrong. I mean to a degree this might be sensible, but
the way you do this here it's completely out of hands. What if somebody
tries to configure this?

	duty-cycle: 0x0000000100000000
	period:     0x0000000200000000

Clearly what they were aiming for is a 50% duty-cycle, but with your
proposal, this will now result in a 100% duty-cycle, which is cleary not
what's requested here.

That's different, in my opinion, from a case where you may have to round
a little bit and get a deviation of, say, 1% in the resulting signal.

> The underlaying problem can only be solved with a way to query the
> resulting configuration for a given request. I have a prototype I can
> share if you're interested.

I don't think we need to go all the way for this legacy code. But I
think we need to make sure that people get a result that is reasonably
within what they asked for. And I think just flat out rejecting these
kinds of requests that are completely out of bounds of the hardware is
better than silently clamping the values. I'd be surprised if this even
had an impact at all on any existing drivers because it had, the
truncation that's currently happening would've likely already exposed
any of them.

Also, in most of these cases the period has been hand-picked and is part
of device tree (or some legacy PWM lookup table), and it was picked
because it is supported by the hardware. Since the duty-cycle always has
to be smaller than the period (we actively reject configurations if that
is not the case), I don't think it likely that we'd currently hit these
conditions at all.

Thierry

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

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

[Uwe Kleine-König]

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

Hello,

On Mon, Mar 22, 2021 at 01:15:31PM +0100, Thierry Reding wrote:
> On Mon, Mar 22, 2021 at 12:29:47PM +0100, Uwe Kleine-König wrote:
> > On Mon, Mar 22, 2021 at 12:12:56PM +0100, Thierry Reding wrote:
> > > On Mon, Mar 15, 2021 at 09:00:51AM +0100, Uwe Kleine-König wrote:
> > > > 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.
> > > > 
> > > > Acked-by: Guru Das Srinagesh <gurus@codeaurora.org>
> > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > > ---
> > > > Changes since v2 (Message-Id: 20210312212119.1342666-1-u.kleine-koenig@pengutronix.de)
> > > > 
> > > >  - Fixed indention of comment (noticed by Guru Das)
> > > >  - Add Ack for Guru Das.
> > > > 
> > > >  drivers/pwm/core.c | 13 +++++++++++--
> > > >  1 file changed, 11 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> > > > index 4b3779d58c5a..b1adf3bb8508 100644
> > > > --- a/drivers/pwm/core.c
> > > > +++ b/drivers/pwm/core.c
> > > > @@ -605,9 +605,18 @@ 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;
> > > > +
> > > > +			/*
> > > > +			 * 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.
> > > > +			 */
> > > > +			period = min(state->period, (u64)INT_MAX);
> > > > +			duty_cycle = min(state->duty_cycle, (u64)INT_MAX);
> > > 
> > > Do we want to highlight this using a WARN()?
> > 
> > That would be fine for me, too. In the past you were not happy with
> > WARN I added, so I implemented this in a silent way.
> 
> The WARN that I remember was in a completely different context. There
> are occasions when it's good to be loud. I think this is one of them.
> 
> > > It seems to me like doing this would always be a programming error and
> > > easy to fix. Silently truncating this to just INT_MAX may not give the
> > > desired effect and be actively wrong most of the time.
> > > 
> > > Come to think of it: why not just refuse such requests with -EINVAL?
> > > That's what drivers already do if they're faced with values that they
> > > can't handle.
> > 
> > No, the strategy I ask authors of new drivers to implement is to program
> > the biggest possible period not bigger than the requested period. So if
> > a consumer requests INT_MAX+3 it must already today cope with the case
> > that it gets a smaller period.
> 
> That just seems wrong. I mean to a degree this might be sensible, but
> the way you do this here it's completely out of hands. What if somebody
> tries to configure this?

The charm it has is that it is simple and doesn't cut somewhere
arbitrary.

> 	duty-cycle: 0x0000000100000000
> 	period:     0x0000000200000000
> 
> Clearly what they were aiming for is a 50% duty-cycle, but with your
> proposal, this will now result in a 100% duty-cycle, which is cleary not
> what's requested here.

OK, can you please formally describe how a hardware driver author should
detect these clear aims? Some document that for any given request and
any given hardware capability tells what setting to pick would be great.

Assume that the longest 50% setting some driver can implement is
duty_cycle = 0x7fffffff ns + period = 0xfffffffe ns. What is the biggest
duty_cycle + period where you want 0x7fffffff + 0xfffffffe to be
configured?

I expect that anything that targets to be practically better than my
approach is damned to be complicated, arbitrary and still doesn't catch
all clear aims. Still more because "clear aims" is something that is
subjective to each developer. And assume you really produce a recipe
that clearly dissects between "round a little bit" and "clearly not what
was requested", do you really want to implement that in each and every
low level driver? I don't, this would be a maintenance mess.

So that's why I think we need pwm_round_state with *easy* rules.

And given that this easy (and admittedly practically imperfect) rule
was "Use the biggest possible period not bigger than requested" and I
still consider this a good choice, clamping instead of returning an
error is still the right choice in my eyes.

> That's different, in my opinion, from a case where you may have to round
> a little bit and get a deviation of, say, 1% in the resulting signal.

Let's assume "1% deviation is ok" is to become the golden rule. If
the nearest possibility to satisfy the request

	.duty_cycle = 1000 ns
	.period = 2000 ns

was

	.duty_cycle = 1010 ns
	.period = 1980 ns

would this be within the 1% rule or wouldn't it? (The relative duty
cycle is > 51% and so it's off by > 2%, but the individual values differ
by just 1%.) Or should we prefer

	.duty_cycle = 1010 ns
	.period = 2030 ns

where the period is off by more than 1% but the relative duty_cycle is
actually nearer to the "clearly requested" 50%?

> > The underlaying problem can only be solved with a way to query the
> > resulting configuration for a given request. I have a prototype I can
> > share if you're interested.
> 
> I don't think we need to go all the way for this legacy code. But I
> think we need to make sure that people get a result that is reasonably
> within what they asked for. And I think just flat out rejecting these
> kinds of requests that are completely out of bounds of the hardware is
> better than silently clamping the values. I'd be surprised if this even
> had an impact at all on any existing drivers because it had, the
> truncation that's currently happening would've likely already exposed
> any of them.
> 
> Also, in most of these cases the period has been hand-picked and is part
> of device tree (or some legacy PWM lookup table), and it was picked
> because it is supported by the hardware. Since the duty-cycle always has
> to be smaller than the period (we actively reject configurations if that
> is not the case), I don't think it likely that we'd currently hit these
> conditions at all.

Does "I don't think we need to go all the way for this legacy code." and
"the period has been hand-picked" mean we shouldn't care at all about
legacy code and just assume nothing bad will happen until eventually all
legacy drivers are gone?

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 v3] pwm: Soften potential loss of precision in compat code

[Thierry Reding]

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

On Mon, Mar 22, 2021 at 04:46:59PM +0100, Uwe Kleine-König wrote:
> Hello,
> 
> On Mon, Mar 22, 2021 at 01:15:31PM +0100, Thierry Reding wrote:
> > On Mon, Mar 22, 2021 at 12:29:47PM +0100, Uwe Kleine-König wrote:
> > > On Mon, Mar 22, 2021 at 12:12:56PM +0100, Thierry Reding wrote:
> > > > On Mon, Mar 15, 2021 at 09:00:51AM +0100, Uwe Kleine-König wrote:
> > > > > 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.
> > > > > 
> > > > > Acked-by: Guru Das Srinagesh <gurus@codeaurora.org>
> > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > > > ---
> > > > > Changes since v2 (Message-Id: 20210312212119.1342666-1-u.kleine-koenig@pengutronix.de)
> > > > > 
> > > > >  - Fixed indention of comment (noticed by Guru Das)
> > > > >  - Add Ack for Guru Das.
> > > > > 
> > > > >  drivers/pwm/core.c | 13 +++++++++++--
> > > > >  1 file changed, 11 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> > > > > index 4b3779d58c5a..b1adf3bb8508 100644
> > > > > --- a/drivers/pwm/core.c
> > > > > +++ b/drivers/pwm/core.c
> > > > > @@ -605,9 +605,18 @@ 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;
> > > > > +
> > > > > +			/*
> > > > > +			 * 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.
> > > > > +			 */
> > > > > +			period = min(state->period, (u64)INT_MAX);
> > > > > +			duty_cycle = min(state->duty_cycle, (u64)INT_MAX);
> > > > 
> > > > Do we want to highlight this using a WARN()?
> > > 
> > > That would be fine for me, too. In the past you were not happy with
> > > WARN I added, so I implemented this in a silent way.
> > 
> > The WARN that I remember was in a completely different context. There
> > are occasions when it's good to be loud. I think this is one of them.
> > 
> > > > It seems to me like doing this would always be a programming error and
> > > > easy to fix. Silently truncating this to just INT_MAX may not give the
> > > > desired effect and be actively wrong most of the time.
> > > > 
> > > > Come to think of it: why not just refuse such requests with -EINVAL?
> > > > That's what drivers already do if they're faced with values that they
> > > > can't handle.
> > > 
> > > No, the strategy I ask authors of new drivers to implement is to program
> > > the biggest possible period not bigger than the requested period. So if
> > > a consumer requests INT_MAX+3 it must already today cope with the case
> > > that it gets a smaller period.
> > 
> > That just seems wrong. I mean to a degree this might be sensible, but
> > the way you do this here it's completely out of hands. What if somebody
> > tries to configure this?
> 
> The charm it has is that it is simple and doesn't cut somewhere
> arbitrary.
> 
> > 	duty-cycle: 0x0000000100000000
> > 	period:     0x0000000200000000
> > 
> > Clearly what they were aiming for is a 50% duty-cycle, but with your
> > proposal, this will now result in a 100% duty-cycle, which is cleary not
> > what's requested here.
> 
> OK, can you please formally describe how a hardware driver author should
> detect these clear aims? Some document that for any given request and
> any given hardware capability tells what setting to pick would be great.
> 
> Assume that the longest 50% setting some driver can implement is
> duty_cycle = 0x7fffffff ns + period = 0xfffffffe ns. What is the biggest
> duty_cycle + period where you want 0x7fffffff + 0xfffffffe to be
> configured?
> 
> I expect that anything that targets to be practically better than my
> approach is damned to be complicated, arbitrary and still doesn't catch
> all clear aims. Still more because "clear aims" is something that is
> subjective to each developer. And assume you really produce a recipe
> that clearly dissects between "round a little bit" and "clearly not what
> was requested", do you really want to implement that in each and every
> low level driver? I don't, this would be a maintenance mess.
> 
> So that's why I think we need pwm_round_state with *easy* rules.
> 
> And given that this easy (and admittedly practically imperfect) rule
> was "Use the biggest possible period not bigger than requested" and I
> still consider this a good choice, clamping instead of returning an
> error is still the right choice in my eyes.
> 
> > That's different, in my opinion, from a case where you may have to round
> > a little bit and get a deviation of, say, 1% in the resulting signal.
> 
> Let's assume "1% deviation is ok" is to become the golden rule. If
> the nearest possibility to satisfy the request
> 
> 	.duty_cycle = 1000 ns
> 	.period = 2000 ns
> 
> was
> 
> 	.duty_cycle = 1010 ns
> 	.period = 1980 ns
> 
> would this be within the 1% rule or wouldn't it? (The relative duty
> cycle is > 51% and so it's off by > 2%, but the individual values differ
> by just 1%.) Or should we prefer
> 
> 	.duty_cycle = 1010 ns
> 	.period = 2030 ns
> 
> where the period is off by more than 1% but the relative duty_cycle is
> actually nearer to the "clearly requested" 50%?

No matter where you draw the line, you're always going to be able to
find a case like that. But that's completely besides the point. I'm not
suggesting any solutions here to the problem of drivers guessing what's
right or of consumers detecting what drivers will set.

What I'm objecting to here is essentially ignoring what the user has
requested and doing something close to arbitrary instead. This might
yield a reasonable result in some cases but it can just as easily be
complete nonsense.

If the requested values are outside of the supported range we should
just reject the request and be done with it. Then there's no need to
even attempt guessing what might have been the intention. Instead we
let the user deal with the problem and provide values that are
supported.

Thierry

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

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

[Uwe Kleine-König]

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

Hello Thierry,

TL;DR: If you think my suggested algorithm to implement which setting to
implement for a given request is bad, please provide a better one.

On Tue, Mar 23, 2021 at 07:32:35PM +0100, Thierry Reding wrote:
> On Mon, Mar 22, 2021 at 04:46:59PM +0100, Uwe Kleine-König wrote:
> > On Mon, Mar 22, 2021 at 01:15:31PM +0100, Thierry Reding wrote:
> > > On Mon, Mar 22, 2021 at 12:29:47PM +0100, Uwe Kleine-König wrote:
> > > > No, the strategy I ask authors of new drivers to implement is to program
> > > > the biggest possible period not bigger than the requested period. So if
> > > > a consumer requests INT_MAX+3 it must already today cope with the case
> > > > that it gets a smaller period.
> > > 
> > > That just seems wrong. I mean to a degree this might be sensible, but
> > > the way you do this here it's completely out of hands. What if somebody
> > > tries to configure this?
> > 
> > The charm it has is that it is simple and doesn't cut somewhere
> > arbitrary.
> > 
> > > 	duty-cycle: 0x0000000100000000
> > > 	period:     0x0000000200000000
> > > 
> > > Clearly what they were aiming for is a 50% duty-cycle, but with your
> > > proposal, this will now result in a 100% duty-cycle, which is cleary not
> > > what's requested here.
> > 
> > OK, can you please formally describe how a hardware driver author should
> > detect these clear aims? Some document that for any given request and
> > any given hardware capability tells what setting to pick would be great.
> > 
> > Assume that the longest 50% setting some driver can implement is
> > duty_cycle = 0x7fffffff ns + period = 0xfffffffe ns. What is the biggest
> > duty_cycle + period where you want 0x7fffffff + 0xfffffffe to be
> > configured?
> > 
> > I expect that anything that targets to be practically better than my
> > approach is damned to be complicated, arbitrary and still doesn't catch
> > all clear aims. Still more because "clear aims" is something that is
> > subjective to each developer. And assume you really produce a recipe
> > that clearly dissects between "round a little bit" and "clearly not what
> > was requested", do you really want to implement that in each and every
> > low level driver? I don't, this would be a maintenance mess.
> > 
> > So that's why I think we need pwm_round_state with *easy* rules.
> > 
> > And given that this easy (and admittedly practically imperfect) rule
> > was "Use the biggest possible period not bigger than requested" and I
> > still consider this a good choice, clamping instead of returning an
> > error is still the right choice in my eyes.
> > 
> > > That's different, in my opinion, from a case where you may have to round
> > > a little bit and get a deviation of, say, 1% in the resulting signal.
> > 
> > Let's assume "1% deviation is ok" is to become the golden rule. If
> > the nearest possibility to satisfy the request
> > 
> > 	.duty_cycle = 1000 ns
> > 	.period = 2000 ns
> > 
> > was
> > 
> > 	.duty_cycle = 1010 ns
> > 	.period = 1980 ns
> > 
> > would this be within the 1% rule or wouldn't it? (The relative duty
> > cycle is > 51% and so it's off by > 2%, but the individual values differ
> > by just 1%.) Or should we prefer
> > 
> > 	.duty_cycle = 1010 ns
> > 	.period = 2030 ns
> > 
> > where the period is off by more than 1% but the relative duty_cycle is
> > actually nearer to the "clearly requested" 50%?
> 
> No matter where you draw the line, you're always going to be able to
> find a case like that. But that's completely besides the point.

IMHO it's not besides the point. You say yielding 0x7fffffff/0x7fffffff
when 0x100000000/0x200000000 was requested is bad and you want it to be
refused. To actually implement this refusal we have to draw a line
somewhere. Should we say for a legacy driver (that only supports ints as
values) that everything that doesn't fit into an int is invalid? So
period = 0x80000000 is refused independent of the question what is the
highest supportable period of the driver in use? What about out-of-range
for small values? If a consumer requests a period of 1ms but the driver
cannot provide that, should it refuse, too? How should a consumer know
if the driver can only do smaller or bigger periods? How can a consumer
find a request that the driver can fulfill in general?

Saying "This case is ridiculus" is easy (and I agree), but can you
please make a practical suggestion, how out-of-range requests should be
handled that gives enough information to actually implement it? Just
giving examples that are out of range is not enough.

With my suggested algorithm (i.e.: First pick the highest supported
period not bigger than the requested period, then pick the highest
supported duty_cycle not bigger than the requested duty_cycle) there are
results that are way off, yes. But together with a round_state function
consumers can know beforehand and there is an (more or less) effective
way for a consumer to pick the best configuration no matter what "best"
means for this particular consumer. Together with the fact that this
algorithm is simple for lowlevel drivers this convinces me that it is a
good strategy and I believe all other strategies are worse when
considering both sensibility and code complexity.

> I'm not suggesting any solutions here to the problem of drivers
> guessing what's right or of consumers detecting what drivers will set.
> 
> What I'm objecting to here is essentially ignoring what the user has
> requested and doing something close to arbitrary instead. This might
> yield a reasonable result in some cases but it can just as easily be
> complete nonsense.

As written above: These two things are essentially the same thing. To
let a driver judge how to respond to a given request that it cannot
fulfil exactly we need an algorithm that decides between "No, this is
too far off, return an error code" and "The configuration nearest to the
request is fine, configure that one."

(But note, this isn't even enough. Consider a driver that can provide
all combinations of period and duty_cycle where both values are a
multiple of 3ns (and duty_cycle <= period of course). What should be
chosen if period=32ns, duty_cycle=16ns is requested? Is it period=33ns +
duty_cycle=15ns because that's the nearest rounding? Or should it be
period=30ns+duty_cycle=15ns to hit the obviously wanted 50%? Or should
it be refused, because yielding 33 when 32 was requested is off by more
than 1%? So additionally we need a formal definition of "nearest".)

> If the requested values are outside of the supported range we should
> just reject the request and be done with it. Then there's no need to
> even attempt guessing what might have been the intention. Instead we
> let the user deal with the problem and provide values that are
> supported.

OK, so if a hardware supports say the following period lengths:

	
	1000/3 ns, 2000/3 ns, 4000/3 ns, 8000/3 ns,
	16000/3 ns, 32000/3 ns, 64000/3 ns, 128000/3 ns

you want to have:

	if requested_period <= 333 ns or requested_period >= 42667 ms:
	    return -EINVAL

, right? So when a consumer request 42667 they get -EINVAL (while the
driver can do 42666.666) and if they request 32000 they get 21333.333?

If you request a period of 3000 ns and your driver's only possible
period is 2800 ns, you get -EINVAL, right? If however the driver
supports 2800 ns and 5600 ns you get 2800 ns? If a driver only supports
a period of 6553.5 ns, what should I actually request to get this,
because mathematically both 6553 and 6554 are out of range? What if the
single supportable period is 6553.6 ns or 6553.4?

In sum I think whatever algorithm is picked, there are ridiculous cases.
So lets pick a strategy that is at least simple to implement, doesn't
require to adapt a driver if a consumer appears that has a different
metric of "best" and allows to find a best match no matter what best
means today for the consumer in question.

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 --]