[v3,5/6] pwm: fsl-ftm: Don't update the state for the caller of pwm_apply_state()
diff mbox series

Message ID 20190824153707.13746-6-uwe@kleine-koenig.org
State New
Headers show
Series
  • pwm: ensure pwm_apply_state() doesn't modify the state argument
Related show

Commit Message

Uwe Kleine-König Aug. 24, 2019, 3:37 p.m. UTC
The pwm-fsl-ftm driver is one of only three PWM drivers which updates
the state for the caller of pwm_apply_state(). This might have
surprising results if the caller reuses the values expecting them to
still represent the same state.

Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
---
 drivers/pwm/pwm-fsl-ftm.c | 4 ----
 1 file changed, 4 deletions(-)

Comments

Doug Anderson Aug. 30, 2019, 5:39 p.m. UTC | #1
Hi,

On Sat, Aug 24, 2019 at 8:37 AM Uwe Kleine-König <uwe@kleine-koenig.org> wrote:
>
> The pwm-fsl-ftm driver is one of only three PWM drivers which updates
> the state for the caller of pwm_apply_state(). This might have
> surprising results if the caller reuses the values expecting them to
> still represent the same state.
>
> Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
> ---
>  drivers/pwm/pwm-fsl-ftm.c | 4 ----
>  1 file changed, 4 deletions(-)

Presumably this patch could break something since the pwm-fsl-ftm
driver doesn't appear to implement the get_state() function.  ...or
did I miss it?

-Doug
Uwe Kleine-König Sept. 2, 2019, 2:27 p.m. UTC | #2
On Fri, Aug 30, 2019 at 10:39:16AM -0700, Doug Anderson wrote:
> Hi,
> 
> On Sat, Aug 24, 2019 at 8:37 AM Uwe Kleine-König <uwe@kleine-koenig.org> wrote:
> >
> > The pwm-fsl-ftm driver is one of only three PWM drivers which updates
> > the state for the caller of pwm_apply_state(). This might have
> > surprising results if the caller reuses the values expecting them to
> > still represent the same state.
> >
> > Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
> > ---
> >  drivers/pwm/pwm-fsl-ftm.c | 4 ----
> >  1 file changed, 4 deletions(-)
> 
> Presumably this patch could break something since the pwm-fsl-ftm
> driver doesn't appear to implement the get_state() function.  ...or
> did I miss it?

I don't expect breakage. We have more than 50 pwm drivers and only three
of them made use of adapting the passed state. So unless you do
something special with the PWM (i.e. more than backlight, LED or fan
control) I don't think a consumer might care. But it might well be that
I miss something so feel free to prove me wrong.

Best regards
Uwe
Doug Anderson Sept. 3, 2019, 4:54 p.m. UTC | #3
Hi,

On Mon, Sep 2, 2019 at 7:27 AM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> On Fri, Aug 30, 2019 at 10:39:16AM -0700, Doug Anderson wrote:
> > Hi,
> >
> > On Sat, Aug 24, 2019 at 8:37 AM Uwe Kleine-König <uwe@kleine-koenig.org> wrote:
> > >
> > > The pwm-fsl-ftm driver is one of only three PWM drivers which updates
> > > the state for the caller of pwm_apply_state(). This might have
> > > surprising results if the caller reuses the values expecting them to
> > > still represent the same state.
> > >
> > > Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
> > > ---
> > >  drivers/pwm/pwm-fsl-ftm.c | 4 ----
> > >  1 file changed, 4 deletions(-)
> >
> > Presumably this patch could break something since the pwm-fsl-ftm
> > driver doesn't appear to implement the get_state() function.  ...or
> > did I miss it?
>
> I don't expect breakage. We have more than 50 pwm drivers and only three
> of them made use of adapting the passed state. So unless you do
> something special with the PWM (i.e. more than backlight, LED or fan
> control) I don't think a consumer might care. But it might well be that
> I miss something so feel free to prove me wrong.

I don't have this hardware so I can't prove you wrong.  ...but
presumably someone added the code to return the state on purpose?

Maybe you could implement get_state() for this driver in your series?

-Doug
Uwe Kleine-König Sept. 3, 2019, 6:48 p.m. UTC | #4
Hello,

On Tue, Sep 03, 2019 at 09:54:37AM -0700, Doug Anderson wrote:
> On Mon, Sep 2, 2019 at 7:27 AM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > On Fri, Aug 30, 2019 at 10:39:16AM -0700, Doug Anderson wrote:
> > > On Sat, Aug 24, 2019 at 8:37 AM Uwe Kleine-König <uwe@kleine-koenig.org> wrote:
> > > >
> > > > The pwm-fsl-ftm driver is one of only three PWM drivers which updates
> > > > the state for the caller of pwm_apply_state(). This might have
> > > > surprising results if the caller reuses the values expecting them to
> > > > still represent the same state.
> > > >
> > > > Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
> > > > ---
> > > >  drivers/pwm/pwm-fsl-ftm.c | 4 ----
> > > >  1 file changed, 4 deletions(-)
> > >
> > > Presumably this patch could break something since the pwm-fsl-ftm
> > > driver doesn't appear to implement the get_state() function.  ...or
> > > did I miss it?
> >
> > I don't expect breakage. We have more than 50 pwm drivers and only three
> > of them made use of adapting the passed state. So unless you do
> > something special with the PWM (i.e. more than backlight, LED or fan
> > control) I don't think a consumer might care. But it might well be that
> > I miss something so feel free to prove me wrong.
> 
> I don't have this hardware so I can't prove you wrong.  ...but
> presumably someone added the code to return the state on purpose?
> 
> Maybe you could implement get_state() for this driver in your series?

Sure, I could. But I don't have hardware either and so I'm not in a
better position than anybody else on this list.

I suggest to apply as is during the merge window, and let affected
user report problems (or patches) if there really is an issue.
Guessing what people might suffer from and trying to cure this with
untested patches won't help I think.

Best regards
Uwe
Doug Anderson Sept. 3, 2019, 7:35 p.m. UTC | #5
Hi,

On Tue, Sep 3, 2019 at 11:48 AM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Hello,
>
> On Tue, Sep 03, 2019 at 09:54:37AM -0700, Doug Anderson wrote:
> > On Mon, Sep 2, 2019 at 7:27 AM Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> wrote:
> > > On Fri, Aug 30, 2019 at 10:39:16AM -0700, Doug Anderson wrote:
> > > > On Sat, Aug 24, 2019 at 8:37 AM Uwe Kleine-König <uwe@kleine-koenig.org> wrote:
> > > > >
> > > > > The pwm-fsl-ftm driver is one of only three PWM drivers which updates
> > > > > the state for the caller of pwm_apply_state(). This might have
> > > > > surprising results if the caller reuses the values expecting them to
> > > > > still represent the same state.
> > > > >
> > > > > Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
> > > > > ---
> > > > >  drivers/pwm/pwm-fsl-ftm.c | 4 ----
> > > > >  1 file changed, 4 deletions(-)
> > > >
> > > > Presumably this patch could break something since the pwm-fsl-ftm
> > > > driver doesn't appear to implement the get_state() function.  ...or
> > > > did I miss it?
> > >
> > > I don't expect breakage. We have more than 50 pwm drivers and only three
> > > of them made use of adapting the passed state. So unless you do
> > > something special with the PWM (i.e. more than backlight, LED or fan
> > > control) I don't think a consumer might care. But it might well be that
> > > I miss something so feel free to prove me wrong.
> >
> > I don't have this hardware so I can't prove you wrong.  ...but
> > presumably someone added the code to return the state on purpose?
> >
> > Maybe you could implement get_state() for this driver in your series?
>
> Sure, I could. But I don't have hardware either and so I'm not in a
> better position than anybody else on this list.
>
> I suggest to apply as is during the merge window, and let affected
> user report problems (or patches) if there really is an issue.
> Guessing what people might suffer from and trying to cure this with
> untested patches won't help I think.

I suppose it's not up to me, but I would rather have a patch that
attempts to keep things working like they did before rather than one
that is known to change behavior.  Even worse is that your patch
description doesn't mention this functionality change at all.

I will also note that not everyone does a deep test of all
functionality during every kernel merge window.  ...so your change in
functionality certain has a pretty high chance of remaining broken for
a while.  In addition if a PWM is used for something like a PWM
regulator then subtle changes can cause totally non-obvious breakages,
maybe adjusting regulators by a very small percentage.  If you
implement the getter it seems (to me) more likely you'll either get it
right or it will totally break things.  That's actually a much better
failure mode...


-Doug
Uwe Kleine-König Sept. 3, 2019, 8:15 p.m. UTC | #6
On Tue, Sep 03, 2019 at 12:35:25PM -0700, Doug Anderson wrote:
> Hi,
> 
> On Tue, Sep 3, 2019 at 11:48 AM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> >
> > Hello,
> >
> > On Tue, Sep 03, 2019 at 09:54:37AM -0700, Doug Anderson wrote:
> > > On Mon, Sep 2, 2019 at 7:27 AM Uwe Kleine-König
> > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > On Fri, Aug 30, 2019 at 10:39:16AM -0700, Doug Anderson wrote:
> > > > > On Sat, Aug 24, 2019 at 8:37 AM Uwe Kleine-König <uwe@kleine-koenig.org> wrote:
> > > > > >
> > > > > > The pwm-fsl-ftm driver is one of only three PWM drivers which updates
> > > > > > the state for the caller of pwm_apply_state(). This might have
> > > > > > surprising results if the caller reuses the values expecting them to
> > > > > > still represent the same state.
> > > > > >
> > > > > > Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
> > > > > > ---
> > > > > >  drivers/pwm/pwm-fsl-ftm.c | 4 ----
> > > > > >  1 file changed, 4 deletions(-)
> > > > >
> > > > > Presumably this patch could break something since the pwm-fsl-ftm
> > > > > driver doesn't appear to implement the get_state() function.  ...or
> > > > > did I miss it?
> > > >
> > > > I don't expect breakage. We have more than 50 pwm drivers and only three
> > > > of them made use of adapting the passed state. So unless you do
> > > > something special with the PWM (i.e. more than backlight, LED or fan
> > > > control) I don't think a consumer might care. But it might well be that
> > > > I miss something so feel free to prove me wrong.
> > >
> > > I don't have this hardware so I can't prove you wrong.  ...but
> > > presumably someone added the code to return the state on purpose?
> > >
> > > Maybe you could implement get_state() for this driver in your series?
> >
> > Sure, I could. But I don't have hardware either and so I'm not in a
> > better position than anybody else on this list.
> >
> > I suggest to apply as is during the merge window, and let affected
> > user report problems (or patches) if there really is an issue.
> > Guessing what people might suffer from and trying to cure this with
> > untested patches won't help I think.
> 
> I suppose it's not up to me, but I would rather have a patch that
> attempts to keep things working like they did before rather than one
> that is known to change behavior.  Even worse is that your patch
> description doesn't mention this functionality change at all.

I suggest to add

	As the driver doesn't provide a .get_state() callback it is
	expected that this changes behaviour slightly as pwm_get_state()
	will yield the last set instead of the last implemented setting.

to the commit log to fix this.

> I will also note that not everyone does a deep test of all
> functionality during every kernel merge window.  ...so your change in
> functionality certain has a pretty high chance of remaining broken for
> a while.

I don't expect any real breakage. The changed behaviour only affects
users of pwm_get_state() that is called after pwm_apply_state(). 

> In addition if a PWM is used for something like a PWM
> regulator then subtle changes can cause totally non-obvious breakages,
> maybe adjusting regulators by a very small percentage.

So for drivers/regulator/pwm-regulator.c this affects the .get_voltage()
call only. Note that .set_voltage() does call pwm_get_state() but
doesn't use the result. I don't see how my change would affect the
configuration written to the PWM registers when the PWM regulator driver
is its user. So if you want to convince me that the PWM regulator is one
of the potentially affected consumers, you have to work a bit harder.
:-)

> If you implement the getter it seems (to me) more likely you'll either
> get it right or it will totally break things.  That's actually a much
> better failure mode...

I don't see how it would totally break even if the untested .get_state()
returns bogus values.

Best regards
Uwe
Doug Anderson Sept. 3, 2019, 8:50 p.m. UTC | #7
Hi,

On Tue, Sep 3, 2019 at 1:15 PM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> On Tue, Sep 03, 2019 at 12:35:25PM -0700, Doug Anderson wrote:
> > Hi,
> >
> > On Tue, Sep 3, 2019 at 11:48 AM Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> wrote:
> > >
> > > Hello,
> > >
> > > On Tue, Sep 03, 2019 at 09:54:37AM -0700, Doug Anderson wrote:
> > > > On Mon, Sep 2, 2019 at 7:27 AM Uwe Kleine-König
> > > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > > On Fri, Aug 30, 2019 at 10:39:16AM -0700, Doug Anderson wrote:
> > > > > > On Sat, Aug 24, 2019 at 8:37 AM Uwe Kleine-König <uwe@kleine-koenig.org> wrote:
> > > > > > >
> > > > > > > The pwm-fsl-ftm driver is one of only three PWM drivers which updates
> > > > > > > the state for the caller of pwm_apply_state(). This might have
> > > > > > > surprising results if the caller reuses the values expecting them to
> > > > > > > still represent the same state.
> > > > > > >
> > > > > > > Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
> > > > > > > ---
> > > > > > >  drivers/pwm/pwm-fsl-ftm.c | 4 ----
> > > > > > >  1 file changed, 4 deletions(-)
> > > > > >
> > > > > > Presumably this patch could break something since the pwm-fsl-ftm
> > > > > > driver doesn't appear to implement the get_state() function.  ...or
> > > > > > did I miss it?
> > > > >
> > > > > I don't expect breakage. We have more than 50 pwm drivers and only three
> > > > > of them made use of adapting the passed state. So unless you do
> > > > > something special with the PWM (i.e. more than backlight, LED or fan
> > > > > control) I don't think a consumer might care. But it might well be that
> > > > > I miss something so feel free to prove me wrong.
> > > >
> > > > I don't have this hardware so I can't prove you wrong.  ...but
> > > > presumably someone added the code to return the state on purpose?
> > > >
> > > > Maybe you could implement get_state() for this driver in your series?
> > >
> > > Sure, I could. But I don't have hardware either and so I'm not in a
> > > better position than anybody else on this list.
> > >
> > > I suggest to apply as is during the merge window, and let affected
> > > user report problems (or patches) if there really is an issue.
> > > Guessing what people might suffer from and trying to cure this with
> > > untested patches won't help I think.
> >
> > I suppose it's not up to me, but I would rather have a patch that
> > attempts to keep things working like they did before rather than one
> > that is known to change behavior.  Even worse is that your patch
> > description doesn't mention this functionality change at all.
>
> I suggest to add
>
>         As the driver doesn't provide a .get_state() callback it is
>         expected that this changes behaviour slightly as pwm_get_state()
>         will yield the last set instead of the last implemented setting.
>
> to the commit log to fix this.
>
> > I will also note that not everyone does a deep test of all
> > functionality during every kernel merge window.  ...so your change in
> > functionality certain has a pretty high chance of remaining broken for
> > a while.
>
> I don't expect any real breakage. The changed behaviour only affects
> users of pwm_get_state() that is called after pwm_apply_state().
>
> > In addition if a PWM is used for something like a PWM
> > regulator then subtle changes can cause totally non-obvious breakages,
> > maybe adjusting regulators by a very small percentage.
>
> So for drivers/regulator/pwm-regulator.c this affects the .get_voltage()
> call only. Note that .set_voltage() does call pwm_get_state() but
> doesn't use the result. I don't see how my change would affect the
> configuration written to the PWM registers when the PWM regulator driver
> is its user. So if you want to convince me that the PWM regulator is one
> of the potentially affected consumers, you have to work a bit harder.
> :-)

Prior to your patch, pwm_apply_state() would call the ->apply()
function, right?  That would modify the state.  Then pwm_apply_state()
would store the state (after it had been modified) into pwm->state.
All future calls to pwm_get_state() would return the modified state.

...this means that the call to pwm_get_state() in
pwm_regulator_get_voltage() would return the actual hardware state.

After your patch series pwm_get_state() will not return the actual
hardware state for "pwm-fsl-ftm.c", it will return the state that was
programmed.

While pwm_set_voltage() will not necessarily be affected, future calls
to pwm_regulator_get_voltage() could be affected.  Unless you are
asserting that 100% of the calls to pwm_get_voltage() cosmetic.


Please correct anything I got wrong there.

-Doug
Uwe Kleine-König Sept. 3, 2019, 9:07 p.m. UTC | #8
On Tue, Sep 03, 2019 at 01:50:27PM -0700, Doug Anderson wrote:
> Hi,
> 
> On Tue, Sep 3, 2019 at 1:15 PM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> >
> > On Tue, Sep 03, 2019 at 12:35:25PM -0700, Doug Anderson wrote:
> > > Hi,
> > >
> > > On Tue, Sep 3, 2019 at 11:48 AM Uwe Kleine-König
> > > <u.kleine-koenig@pengutronix.de> wrote:
> > > >
> > > > Hello,
> > > >
> > > > On Tue, Sep 03, 2019 at 09:54:37AM -0700, Doug Anderson wrote:
> > > > > On Mon, Sep 2, 2019 at 7:27 AM Uwe Kleine-König
> > > > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > > > On Fri, Aug 30, 2019 at 10:39:16AM -0700, Doug Anderson wrote:
> > > > > > > On Sat, Aug 24, 2019 at 8:37 AM Uwe Kleine-König <uwe@kleine-koenig.org> wrote:
> > > > > > > >
> > > > > > > > The pwm-fsl-ftm driver is one of only three PWM drivers which updates
> > > > > > > > the state for the caller of pwm_apply_state(). This might have
> > > > > > > > surprising results if the caller reuses the values expecting them to
> > > > > > > > still represent the same state.
> > > > > > > >
> > > > > > > > Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
> > > > > > > > ---
> > > > > > > >  drivers/pwm/pwm-fsl-ftm.c | 4 ----
> > > > > > > >  1 file changed, 4 deletions(-)
> > > > > > >
> > > > > > > Presumably this patch could break something since the pwm-fsl-ftm
> > > > > > > driver doesn't appear to implement the get_state() function.  ...or
> > > > > > > did I miss it?
> > > > > >
> > > > > > I don't expect breakage. We have more than 50 pwm drivers and only three
> > > > > > of them made use of adapting the passed state. So unless you do
> > > > > > something special with the PWM (i.e. more than backlight, LED or fan
> > > > > > control) I don't think a consumer might care. But it might well be that
> > > > > > I miss something so feel free to prove me wrong.
> > > > >
> > > > > I don't have this hardware so I can't prove you wrong.  ...but
> > > > > presumably someone added the code to return the state on purpose?
> > > > >
> > > > > Maybe you could implement get_state() for this driver in your series?
> > > >
> > > > Sure, I could. But I don't have hardware either and so I'm not in a
> > > > better position than anybody else on this list.
> > > >
> > > > I suggest to apply as is during the merge window, and let affected
> > > > user report problems (or patches) if there really is an issue.
> > > > Guessing what people might suffer from and trying to cure this with
> > > > untested patches won't help I think.
> > >
> > > I suppose it's not up to me, but I would rather have a patch that
> > > attempts to keep things working like they did before rather than one
> > > that is known to change behavior.  Even worse is that your patch
> > > description doesn't mention this functionality change at all.
> >
> > I suggest to add
> >
> >         As the driver doesn't provide a .get_state() callback it is
> >         expected that this changes behaviour slightly as pwm_get_state()
> >         will yield the last set instead of the last implemented setting.
> >
> > to the commit log to fix this.
> >
> > > I will also note that not everyone does a deep test of all
> > > functionality during every kernel merge window.  ...so your change in
> > > functionality certain has a pretty high chance of remaining broken for
> > > a while.
> >
> > I don't expect any real breakage. The changed behaviour only affects
> > users of pwm_get_state() that is called after pwm_apply_state().
> >
> > > In addition if a PWM is used for something like a PWM
> > > regulator then subtle changes can cause totally non-obvious breakages,
> > > maybe adjusting regulators by a very small percentage.
> >
> > So for drivers/regulator/pwm-regulator.c this affects the .get_voltage()
> > call only. Note that .set_voltage() does call pwm_get_state() but
> > doesn't use the result. I don't see how my change would affect the
> > configuration written to the PWM registers when the PWM regulator driver
> > is its user. So if you want to convince me that the PWM regulator is one
> > of the potentially affected consumers, you have to work a bit harder.
> > :-)
> 
> Prior to your patch, pwm_apply_state() would call the ->apply()
> function, right?  That would modify the state.  Then pwm_apply_state()
> would store the state (after it had been modified) into pwm->state.
> All future calls to pwm_get_state() would return the modified state.
> 
> ...this means that the call to pwm_get_state() in
> pwm_regulator_get_voltage() would return the actual hardware state.
> 
> After your patch series pwm_get_state() will not return the actual
> hardware state for "pwm-fsl-ftm.c", it will return the state that was
> programmed.
> 
> While pwm_set_voltage() will not necessarily be affected, future calls
> to pwm_regulator_get_voltage() could be affected.  Unless you are
> asserting that 100% of the calls to pwm_get_voltage() cosmetic.
>
> 
> Please correct anything I got wrong there.

I think this is all true. The key question here is then: Who calls the
.get_voltage() callback and cares about the result? Yes, it changes a
few files in sysfs but apart from that?

Best regards
Uwe
Doug Anderson Sept. 3, 2019, 9:48 p.m. UTC | #9
Hi,

On Tue, Sep 3, 2019 at 2:07 PM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> On Tue, Sep 03, 2019 at 01:50:27PM -0700, Doug Anderson wrote:
> > Hi,
> >
> > On Tue, Sep 3, 2019 at 1:15 PM Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> wrote:
> > >
> > > On Tue, Sep 03, 2019 at 12:35:25PM -0700, Doug Anderson wrote:
> > > > Hi,
> > > >
> > > > On Tue, Sep 3, 2019 at 11:48 AM Uwe Kleine-König
> > > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > >
> > > > > Hello,
> > > > >
> > > > > On Tue, Sep 03, 2019 at 09:54:37AM -0700, Doug Anderson wrote:
> > > > > > On Mon, Sep 2, 2019 at 7:27 AM Uwe Kleine-König
> > > > > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > > > > On Fri, Aug 30, 2019 at 10:39:16AM -0700, Doug Anderson wrote:
> > > > > > > > On Sat, Aug 24, 2019 at 8:37 AM Uwe Kleine-König <uwe@kleine-koenig.org> wrote:
> > > > > > > > >
> > > > > > > > > The pwm-fsl-ftm driver is one of only three PWM drivers which updates
> > > > > > > > > the state for the caller of pwm_apply_state(). This might have
> > > > > > > > > surprising results if the caller reuses the values expecting them to
> > > > > > > > > still represent the same state.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
> > > > > > > > > ---
> > > > > > > > >  drivers/pwm/pwm-fsl-ftm.c | 4 ----
> > > > > > > > >  1 file changed, 4 deletions(-)
> > > > > > > >
> > > > > > > > Presumably this patch could break something since the pwm-fsl-ftm
> > > > > > > > driver doesn't appear to implement the get_state() function.  ...or
> > > > > > > > did I miss it?
> > > > > > >
> > > > > > > I don't expect breakage. We have more than 50 pwm drivers and only three
> > > > > > > of them made use of adapting the passed state. So unless you do
> > > > > > > something special with the PWM (i.e. more than backlight, LED or fan
> > > > > > > control) I don't think a consumer might care. But it might well be that
> > > > > > > I miss something so feel free to prove me wrong.
> > > > > >
> > > > > > I don't have this hardware so I can't prove you wrong.  ...but
> > > > > > presumably someone added the code to return the state on purpose?
> > > > > >
> > > > > > Maybe you could implement get_state() for this driver in your series?
> > > > >
> > > > > Sure, I could. But I don't have hardware either and so I'm not in a
> > > > > better position than anybody else on this list.
> > > > >
> > > > > I suggest to apply as is during the merge window, and let affected
> > > > > user report problems (or patches) if there really is an issue.
> > > > > Guessing what people might suffer from and trying to cure this with
> > > > > untested patches won't help I think.
> > > >
> > > > I suppose it's not up to me, but I would rather have a patch that
> > > > attempts to keep things working like they did before rather than one
> > > > that is known to change behavior.  Even worse is that your patch
> > > > description doesn't mention this functionality change at all.
> > >
> > > I suggest to add
> > >
> > >         As the driver doesn't provide a .get_state() callback it is
> > >         expected that this changes behaviour slightly as pwm_get_state()
> > >         will yield the last set instead of the last implemented setting.
> > >
> > > to the commit log to fix this.
> > >
> > > > I will also note that not everyone does a deep test of all
> > > > functionality during every kernel merge window.  ...so your change in
> > > > functionality certain has a pretty high chance of remaining broken for
> > > > a while.
> > >
> > > I don't expect any real breakage. The changed behaviour only affects
> > > users of pwm_get_state() that is called after pwm_apply_state().
> > >
> > > > In addition if a PWM is used for something like a PWM
> > > > regulator then subtle changes can cause totally non-obvious breakages,
> > > > maybe adjusting regulators by a very small percentage.
> > >
> > > So for drivers/regulator/pwm-regulator.c this affects the .get_voltage()
> > > call only. Note that .set_voltage() does call pwm_get_state() but
> > > doesn't use the result. I don't see how my change would affect the
> > > configuration written to the PWM registers when the PWM regulator driver
> > > is its user. So if you want to convince me that the PWM regulator is one
> > > of the potentially affected consumers, you have to work a bit harder.
> > > :-)
> >
> > Prior to your patch, pwm_apply_state() would call the ->apply()
> > function, right?  That would modify the state.  Then pwm_apply_state()
> > would store the state (after it had been modified) into pwm->state.
> > All future calls to pwm_get_state() would return the modified state.
> >
> > ...this means that the call to pwm_get_state() in
> > pwm_regulator_get_voltage() would return the actual hardware state.
> >
> > After your patch series pwm_get_state() will not return the actual
> > hardware state for "pwm-fsl-ftm.c", it will return the state that was
> > programmed.
> >
> > While pwm_set_voltage() will not necessarily be affected, future calls
> > to pwm_regulator_get_voltage() could be affected.  Unless you are
> > asserting that 100% of the calls to pwm_get_voltage() cosmetic.
> >
> >
> > Please correct anything I got wrong there.
>
> I think this is all true. The key question here is then: Who calls the
> .get_voltage() callback and cares about the result? Yes, it changes a
> few files in sysfs but apart from that?

There are lots of drivers that call get_voltage() for things other
than sysfs, but without auditing each one I can't say if any of them
would change behavior in a way that would matter.

-Doug
Uwe Kleine-König Sept. 4, 2019, 8:21 a.m. UTC | #10
On Tue, Sep 03, 2019 at 02:48:54PM -0700, Doug Anderson wrote:
> Hi,
> 
> On Tue, Sep 3, 2019 at 2:07 PM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> >
> > On Tue, Sep 03, 2019 at 01:50:27PM -0700, Doug Anderson wrote:
> > > Hi,
> > >
> > > On Tue, Sep 3, 2019 at 1:15 PM Uwe Kleine-König
> > > <u.kleine-koenig@pengutronix.de> wrote:
> > > >
> > > > On Tue, Sep 03, 2019 at 12:35:25PM -0700, Doug Anderson wrote:
> > > > > Hi,
> > > > >
> > > > > On Tue, Sep 3, 2019 at 11:48 AM Uwe Kleine-König
> > > > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > > >
> > > > > > Hello,
> > > > > >
> > > > > > On Tue, Sep 03, 2019 at 09:54:37AM -0700, Doug Anderson wrote:
> > > > > > > On Mon, Sep 2, 2019 at 7:27 AM Uwe Kleine-König
> > > > > > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > > > > > On Fri, Aug 30, 2019 at 10:39:16AM -0700, Doug Anderson wrote:
> > > > > > > > > On Sat, Aug 24, 2019 at 8:37 AM Uwe Kleine-König <uwe@kleine-koenig.org> wrote:
> > > > > > > > > >
> > > > > > > > > > The pwm-fsl-ftm driver is one of only three PWM drivers which updates
> > > > > > > > > > the state for the caller of pwm_apply_state(). This might have
> > > > > > > > > > surprising results if the caller reuses the values expecting them to
> > > > > > > > > > still represent the same state.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
> > > > > > > > > > ---
> > > > > > > > > >  drivers/pwm/pwm-fsl-ftm.c | 4 ----
> > > > > > > > > >  1 file changed, 4 deletions(-)
> > > > > > > > >
> > > > > > > > > Presumably this patch could break something since the pwm-fsl-ftm
> > > > > > > > > driver doesn't appear to implement the get_state() function.  ...or
> > > > > > > > > did I miss it?
> > > > > > > >
> > > > > > > > I don't expect breakage. We have more than 50 pwm drivers and only three
> > > > > > > > of them made use of adapting the passed state. So unless you do
> > > > > > > > something special with the PWM (i.e. more than backlight, LED or fan
> > > > > > > > control) I don't think a consumer might care. But it might well be that
> > > > > > > > I miss something so feel free to prove me wrong.
> > > > > > >
> > > > > > > I don't have this hardware so I can't prove you wrong.  ...but
> > > > > > > presumably someone added the code to return the state on purpose?
> > > > > > >
> > > > > > > Maybe you could implement get_state() for this driver in your series?
> > > > > >
> > > > > > Sure, I could. But I don't have hardware either and so I'm not in a
> > > > > > better position than anybody else on this list.
> > > > > >
> > > > > > I suggest to apply as is during the merge window, and let affected
> > > > > > user report problems (or patches) if there really is an issue.
> > > > > > Guessing what people might suffer from and trying to cure this with
> > > > > > untested patches won't help I think.
> > > > >
> > > > > I suppose it's not up to me, but I would rather have a patch that
> > > > > attempts to keep things working like they did before rather than one
> > > > > that is known to change behavior.  Even worse is that your patch
> > > > > description doesn't mention this functionality change at all.
> > > >
> > > > I suggest to add
> > > >
> > > >         As the driver doesn't provide a .get_state() callback it is
> > > >         expected that this changes behaviour slightly as pwm_get_state()
> > > >         will yield the last set instead of the last implemented setting.
> > > >
> > > > to the commit log to fix this.
> > > >
> > > > > I will also note that not everyone does a deep test of all
> > > > > functionality during every kernel merge window.  ...so your change in
> > > > > functionality certain has a pretty high chance of remaining broken for
> > > > > a while.
> > > >
> > > > I don't expect any real breakage. The changed behaviour only affects
> > > > users of pwm_get_state() that is called after pwm_apply_state().
> > > >
> > > > > In addition if a PWM is used for something like a PWM
> > > > > regulator then subtle changes can cause totally non-obvious breakages,
> > > > > maybe adjusting regulators by a very small percentage.
> > > >
> > > > So for drivers/regulator/pwm-regulator.c this affects the .get_voltage()
> > > > call only. Note that .set_voltage() does call pwm_get_state() but
> > > > doesn't use the result. I don't see how my change would affect the
> > > > configuration written to the PWM registers when the PWM regulator driver
> > > > is its user. So if you want to convince me that the PWM regulator is one
> > > > of the potentially affected consumers, you have to work a bit harder.
> > > > :-)
> > >
> > > Prior to your patch, pwm_apply_state() would call the ->apply()
> > > function, right?  That would modify the state.  Then pwm_apply_state()
> > > would store the state (after it had been modified) into pwm->state.
> > > All future calls to pwm_get_state() would return the modified state.
> > >
> > > ...this means that the call to pwm_get_state() in
> > > pwm_regulator_get_voltage() would return the actual hardware state.
> > >
> > > After your patch series pwm_get_state() will not return the actual
> > > hardware state for "pwm-fsl-ftm.c", it will return the state that was
> > > programmed.
> > >
> > > While pwm_set_voltage() will not necessarily be affected, future calls
> > > to pwm_regulator_get_voltage() could be affected.  Unless you are
> > > asserting that 100% of the calls to pwm_get_voltage() cosmetic.
> > >
> > >
> > > Please correct anything I got wrong there.
> >
> > I think this is all true. The key question here is then: Who calls the
> > .get_voltage() callback and cares about the result? Yes, it changes a
> > few files in sysfs but apart from that?
> 
> There are lots of drivers that call get_voltage() for things other
> than sysfs, but without auditing each one I can't say if any of them
> would change behavior in a way that would matter.

In my book it is ok to do such a change. The driver continues to compile
just fine, it isn't knowingly broken. And if someone finds a regression
we can fix it then and have someone who cares for testing.

And even if a regulator changes its behaviour slightly and breaks in a
hardly detectable way, I would bet that in 95% of the cases it only
worked by chance before. And I hope for the remaining 5% who seem to
care about correctness, that the reverify on a kernel upgrade.

Yes, there might be some cases falling through the cracks, but if we
start to demand this kind of care from people who work on generic code
(here: PWM core) we will be stuck for a long time and scare people with
motivation away. So the lesser evil in my eyes is to accept that not all
corner cases might be handled because they are unknown.

Best regards
Uwe

Patch
diff mbox series

diff --git a/drivers/pwm/pwm-fsl-ftm.c b/drivers/pwm/pwm-fsl-ftm.c
index 9d31a217111d..3c9738617ceb 100644
--- a/drivers/pwm/pwm-fsl-ftm.c
+++ b/drivers/pwm/pwm-fsl-ftm.c
@@ -292,10 +292,6 @@  static int fsl_pwm_apply_config(struct fsl_pwm_chip *fpc,
 
 	regmap_update_bits(fpc->regmap, FTM_POL, BIT(pwm->hwpwm), reg_polarity);
 
-	newstate->period = fsl_pwm_ticks_to_ns(fpc,
-					       fpc->period.mod_period + 1);
-	newstate->duty_cycle = fsl_pwm_ticks_to_ns(fpc, duty);
-
 	ftm_set_write_protection(fpc);
 
 	return 0;