Hi!
I have a device with a "sound card" that has an amplifier that needs
an extra boost when high amplification is requested. This extra
boost is controlled with a pwm-regulator.
As of commit c73a3107624d ("pwm: Handle .get_state() failures") this
device no longer works. I have tracked the problem to an unfortunate
interaction between the underlying PWM driver and the PWM core.
The driver is drivers/pwm/pwm-atmel.c which has difficulties getting
the period and/or duty_cycle from the HW when the PWM is not enabled.
Because of this, I think, the driver does not fill in .period and
.duty_cycle at all in atmel_pwm_get_state() unless the PWM is enabled.
However, the PWM core is not expecting these fields to be left as-is,
at least not in pwm_adjust_config(), and its local state variable on
the stack ends up with whatever crap was on the stack on entry for
these fields. That fails spectacularly when the function continues to
do math on these uninitialized values.
In particular, I find this in the kernel log when a bad kernel runs:
pwm-regulator: probe of reg-ana failed with error -22
Before commit c73a3107624d this was a silent failure, and the situation
"repaired itself" when the PWM was later reprogrammed, at least for my
case. After that commit, the failure is fatal and the "sound card"
fails to come up at all.
I see a couple of adjustments that could be made.
1. Zero out some fields in the driver:
@@ -390,4 +390,6 @@ static int atmel_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
state->enabled = true;
} else {
+ state->period = 0;
+ state->duty_cycle = 0;
state->enabled = false;
}
2. Don't try to "adjust" a disabled PWM:
@@ -656,7 +656,7 @@ int pwm_adjust_config(struct pwm_device *pwm)
* In either case, we setup the new period and polarity, and assign a
* duty cycle of 0.
*/
- if (!state.period) {
+ if (!state.enabled || !state.period) {
state.duty_cycle = 0;
state.period = pargs.period;
state.polarity = pargs.polarity;
3. Zero out the state before calling pwm_get_state:
@@ -115,7 +115,7 @@ static int pwm_device_request(struct pwm_device *pwm, const char *label)
}
if (pwm->chip->ops->get_state) {
- struct pwm_state state;
+ struct pwm_state state = { .enabled = true };
err = pwm->chip->ops->get_state(pwm->chip, pwm, &state);
trace_pwm_get(pwm, &state, err);
@@ -448,7 +448,7 @@ static void pwm_apply_state_debug(struct pwm_device *pwm,
{
struct pwm_state *last = &pwm->last;
struct pwm_chip *chip = pwm->chip;
- struct pwm_state s1, s2;
+ struct pwm_state s1 = { .enabled = false }, s2;
int err;
if (!IS_ENABLED(CONFIG_PWM_DEBUG))
@@ -634,7 +634,7 @@ EXPORT_SYMBOL_GPL(pwm_capture);
*/
int pwm_adjust_config(struct pwm_device *pwm)
{
- struct pwm_state state;
+ struct pwm_state state = { .enabled = false };
struct pwm_args pargs;
pwm_get_args(pwm, &pargs);
@@ -1119,7 +1119,7 @@ static void pwm_dbg_show(struct pwm_chip *chip, struct seq_file *s)
for (i = 0; i < chip->npwm; i++) {
struct pwm_device *pwm = &chip->pwms[i];
- struct pwm_state state;
+ struct pwm_state state = { .enabled = false };
pwm_get_state(pwm, &state);
I have verified that any of the above approaches resolve the
regression but I don't know which approach is best? Maybe more
than one?
However:
Approach 1. will maybe clobber the saved pwm->state such that
it no longer works to get the period/duty_cycle if/when the
PWM is disabled? Maybe only for some corner case? But that might
be a significant corner case?
Approach 2. will maybe mess up some unrelated functionality?
Approach 3. is ugly, intrusive and is in all likelihood
incomplete. It also needs a rebase from the culprit commit.
Cheers,
Peter
#regzbot introduced c73a3107624d
On Mon, May 22, 2023 at 05:19:43PM +0200, Peter Rosin wrote:
> Hi!
>
> I have a device with a "sound card" that has an amplifier that needs
> an extra boost when high amplification is requested. This extra
> boost is controlled with a pwm-regulator.
>
> As of commit c73a3107624d ("pwm: Handle .get_state() failures") this
> device no longer works. I have tracked the problem to an unfortunate
> interaction between the underlying PWM driver and the PWM core.
>
> The driver is drivers/pwm/pwm-atmel.c which has difficulties getting
> the period and/or duty_cycle from the HW when the PWM is not enabled.
> Because of this, I think, the driver does not fill in .period and
> .duty_cycle at all in atmel_pwm_get_state() unless the PWM is enabled.
>
> However, the PWM core is not expecting these fields to be left as-is,
> at least not in pwm_adjust_config(), and its local state variable on
> the stack ends up with whatever crap was on the stack on entry for
> these fields. That fails spectacularly when the function continues to
> do math on these uninitialized values.
>
> In particular, I find this in the kernel log when a bad kernel runs:
> pwm-regulator: probe of reg-ana failed with error -22
>
> Before commit c73a3107624d this was a silent failure, and the situation
> "repaired itself" when the PWM was later reprogrammed, at least for my
> case. After that commit, the failure is fatal and the "sound card"
> fails to come up at all.
>
>
> I see a couple of adjustments that could be made.
>
> 1. Zero out some fields in the driver:
>
> @@ -390,4 +390,6 @@ static int atmel_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> state->enabled = true;
> } else {
> + state->period = 0;
> + state->duty_cycle = 0;
> state->enabled = false;
> }
I prefer this version. Drivers are supposed to set the state as
accurately as they can. If they can't say anything about the hardware
state because it's in an undetermined state, clearing out all the state
fields seems like the best option.
We could probably do this within the core to avoid any such bugs, but it
doesn't really hurt for drivers to be explicit either. Maybe Uwe has
additional thoughts on this.
Thierry
Hello Peter,
On Mon, May 22, 2023 at 05:19:43PM +0200, Peter Rosin wrote:
> I have a device with a "sound card" that has an amplifier that needs
> an extra boost when high amplification is requested. This extra
> boost is controlled with a pwm-regulator.
>
> As of commit c73a3107624d ("pwm: Handle .get_state() failures") this
> device no longer works. I have tracked the problem to an unfortunate
> interaction between the underlying PWM driver and the PWM core.
>
> The driver is drivers/pwm/pwm-atmel.c which has difficulties getting
> the period and/or duty_cycle from the HW when the PWM is not enabled.
> Because of this, I think, the driver does not fill in .period and
> .duty_cycle at all in atmel_pwm_get_state() unless the PWM is enabled.
>
> However, the PWM core is not expecting these fields to be left as-is,
> at least not in pwm_adjust_config(), and its local state variable on
> the stack ends up with whatever crap was on the stack on entry for
> these fields. That fails spectacularly when the function continues to
> do math on these uninitialized values.
>
> In particular, I find this in the kernel log when a bad kernel runs:
> pwm-regulator: probe of reg-ana failed with error -22
>
> Before commit c73a3107624d this was a silent failure, and the situation
> "repaired itself" when the PWM was later reprogrammed, at least for my
> case. After that commit, the failure is fatal and the "sound card"
> fails to come up at all.
>
>
> I see a couple of adjustments that could be made.
>
> 1. Zero out some fields in the driver:
>
> @@ -390,4 +390,6 @@ static int atmel_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> state->enabled = true;
> } else {
> + state->period = 0;
> + state->duty_cycle = 0;
> state->enabled = false;
> }
I don't particularily like that. While state->period is an invalid
value, IMHO enabled = false is enough information from the driver's POV.
> 2. Don't try to "adjust" a disabled PWM:
>
> @@ -656,7 +656,7 @@ int pwm_adjust_config(struct pwm_device *pwm)
> * In either case, we setup the new period and polarity, and assign a
> * duty cycle of 0.
> */
> - if (!state.period) {
> + if (!state.enabled || !state.period) {
> state.duty_cycle = 0;
> state.period = pargs.period;
> state.polarity = pargs.polarity;
In my book code that calls pwm_get_state() should consider .period
(and .duty_cycle) undefined if .enabled is false. So this hunk is an
improvement. Having said that, I think pwm_adjust_config() has a strange
semantic. I don't understand when you would want to call it, and it only
has one caller (i.e. pwm-regulator).
So another option would be
diff --git a/drivers/regulator/pwm-regulator.c b/drivers/regulator/pwm-regulator.c
index b64d99695b84..418aff0ddbed 100644
--- a/drivers/regulator/pwm-regulator.c
+++ b/drivers/regulator/pwm-regulator.c
@@ -368,10 +368,6 @@ static int pwm_regulator_probe(struct platform_device *pdev)
return ret;
}
- ret = pwm_adjust_config(drvdata->pwm);
- if (ret)
- return ret;
-
regulator = devm_regulator_register(&pdev->dev,
&drvdata->desc, &config);
if (IS_ERR(regulator)) {
and drop pwm_adjust_config() as a followup?!
> 3. Zero out the state before calling pwm_get_state:
>
> @@ -115,7 +115,7 @@ static int pwm_device_request(struct pwm_device *pwm, const char *label)
> }
>
> if (pwm->chip->ops->get_state) {
> - struct pwm_state state;
> + struct pwm_state state = { .enabled = true };
I wonder why you picked .enabled = true here but = false on all other
code locations.
Also you don't seem to have 1271a7b98e7989ba6bb978e14403fc84efe16e13
which has the same effect and is included in v6.3 and v6.2.13.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Hi!
2023-05-22 at 19:23, Uwe Kleine-König wrote:
> Hello Peter,
>
> On Mon, May 22, 2023 at 05:19:43PM +0200, Peter Rosin wrote:
>> I have a device with a "sound card" that has an amplifier that needs
>> an extra boost when high amplification is requested. This extra
>> boost is controlled with a pwm-regulator.
>>
>> As of commit c73a3107624d ("pwm: Handle .get_state() failures") this
>> device no longer works. I have tracked the problem to an unfortunate
>> interaction between the underlying PWM driver and the PWM core.
>>
>> The driver is drivers/pwm/pwm-atmel.c which has difficulties getting
>> the period and/or duty_cycle from the HW when the PWM is not enabled.
>> Because of this, I think, the driver does not fill in .period and
>> .duty_cycle at all in atmel_pwm_get_state() unless the PWM is enabled.
>>
>> However, the PWM core is not expecting these fields to be left as-is,
>> at least not in pwm_adjust_config(), and its local state variable on
>> the stack ends up with whatever crap was on the stack on entry for
>> these fields. That fails spectacularly when the function continues to
>> do math on these uninitialized values.
>>
>> In particular, I find this in the kernel log when a bad kernel runs:
>> pwm-regulator: probe of reg-ana failed with error -22
>>
>> Before commit c73a3107624d this was a silent failure, and the situation
>> "repaired itself" when the PWM was later reprogrammed, at least for my
>> case. After that commit, the failure is fatal and the "sound card"
>> fails to come up at all.
>>
>>
>> I see a couple of adjustments that could be made.
>>
>> 1. Zero out some fields in the driver:
>>
>> @@ -390,4 +390,6 @@ static int atmel_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
>> state->enabled = true;
>> } else {
>> + state->period = 0;
>> + state->duty_cycle = 0;
>> state->enabled = false;
>> }
>
> I don't particularily like that. While state->period is an invalid
> value, IMHO enabled = false is enough information from the driver's POV.
This was the preferred approach of Thierry, and given the number of
call sites for pwm_get_state with a local variable, I can sympathize
with that view. At the same time, fixing drivers one by one is not
a fun game, so I can certainly see that approach 3 also has an appeal.
>> 2. Don't try to "adjust" a disabled PWM:
>>
>> @@ -656,7 +656,7 @@ int pwm_adjust_config(struct pwm_device *pwm)
>> * In either case, we setup the new period and polarity, and assign a
>> * duty cycle of 0.
>> */
>> - if (!state.period) {
>> + if (!state.enabled || !state.period) {
>> state.duty_cycle = 0;
>> state.period = pargs.period;
>> state.polarity = pargs.polarity;
>
> In my book code that calls pwm_get_state() should consider .period
> (and .duty_cycle) undefined if .enabled is false. So this hunk is an
I agree fully. This feels like a good hunk.
> improvement. Having said that, I think pwm_adjust_config() has a strange
> semantic. I don't understand when you would want to call it, and it only
> has one caller (i.e. pwm-regulator).
I believe it's for a case where some bootstrap/bootloader has configured
a PWM in order to set up a regulator for some critical component, and the
kernel needs to cake over the responsibility seamlessly, or everything
will take a dive. I deduced that from the description of pwm_adjust_config:
* This function will adjust the PWM config to the PWM arguments provided
* by the DT or PWM lookup table. This is particularly useful to adapt
* the bootloader config to the Linux one.
But what do I know?
> So another option would be
>
> diff --git a/drivers/regulator/pwm-regulator.c b/drivers/regulator/pwm-regulator.c
> index b64d99695b84..418aff0ddbed 100644
> --- a/drivers/regulator/pwm-regulator.c
> +++ b/drivers/regulator/pwm-regulator.c
> @@ -368,10 +368,6 @@ static int pwm_regulator_probe(struct platform_device *pdev)
> return ret;
> }
>
> - ret = pwm_adjust_config(drvdata->pwm);
> - if (ret)
> - return ret;
> -
> regulator = devm_regulator_register(&pdev->dev,
> &drvdata->desc, &config);
> if (IS_ERR(regulator)) {
>
> and drop pwm_adjust_config() as a followup?!
As described above, I think that might be too drastic?
>> 3. Zero out the state before calling pwm_get_state:
>>
>> @@ -115,7 +115,7 @@ static int pwm_device_request(struct pwm_device *pwm, const char *label)
>> }
>>
>> if (pwm->chip->ops->get_state) {
>> - struct pwm_state state;
>> + struct pwm_state state = { .enabled = true };
>
> I wonder why you picked .enabled = true here but = false on all other
> code locations.
Silly typo, soory.
> Also you don't seem to have 1271a7b98e7989ba6bb978e14403fc84efe16e13
> which has the same effect and is included in v6.3 and v6.2.13.
Right, approach 3 felt so ugly and intrusive that I didn't bother to take
it beyond the problematic commit (and hinted at that further down: "It also
needs a rebase from the culprit commit"). Having said that, 1271a7b98e79 is
not enough, or else I would never have noticed the problem in the first
place. I don't think the hunk in pwm_dbg_show() makes any difference for my
case since I heven't enabled debugging, and a double-check confirms it:
After 1271a7b98e79, zeroing "state" in pwm_adjust_config() is enough to kill
this regression.
So, given that approach 3 is already half done, it no longer feels as ugly
and intrusive and more like just following the path forward.
Pros and cons, pros and cons. 1, 2, 3, don't know what to do. But hmm, it's
not some annoying "any two of three" crap, and it's actually possible and
not entirely unreasonable to do all three...
I'll let someonw else pick and choose. I have no strong preference.
Cheers,
Peter
Hello Peter,
On Mon, May 22, 2023 at 09:28:39PM +0200, Peter Rosin wrote:
> 2023-05-22 at 19:23, Uwe Kleine-K?nig wrote:
> > On Mon, May 22, 2023 at 05:19:43PM +0200, Peter Rosin wrote:
> >> I have a device with a "sound card" that has an amplifier that needs
> >> an extra boost when high amplification is requested. This extra
> >> boost is controlled with a pwm-regulator.
> >>
> >> As of commit c73a3107624d ("pwm: Handle .get_state() failures") this
> >> device no longer works. I have tracked the problem to an unfortunate
> >> interaction between the underlying PWM driver and the PWM core.
> >>
> >> The driver is drivers/pwm/pwm-atmel.c which has difficulties getting
> >> the period and/or duty_cycle from the HW when the PWM is not enabled.
> >> Because of this, I think, the driver does not fill in .period and
> >> .duty_cycle at all in atmel_pwm_get_state() unless the PWM is enabled.
> >>
> >> However, the PWM core is not expecting these fields to be left as-is,
> >> at least not in pwm_adjust_config(), and its local state variable on
> >> the stack ends up with whatever crap was on the stack on entry for
> >> these fields. That fails spectacularly when the function continues to
> >> do math on these uninitialized values.
After looking again, I don't understand that part. Note that
pwm_get_state() doesn't call .get_state() at all. Also pwmchip_add()
zero initializes .state, then pwm_get() calls .get_state() (via
pwm_device_request() which is called in .xlate()) which (if the HW is
disabled) doesn't touch .period, so it should continue to be zero?!
So I wonder why your approach 2 works at all. Do you see what I'm
missing?
> >> In particular, I find this in the kernel log when a bad kernel runs:
> >> pwm-regulator: probe of reg-ana failed with error -22
> >>
> >> Before commit c73a3107624d this was a silent failure, and the situation
> >> "repaired itself" when the PWM was later reprogrammed, at least for my
> >> case. After that commit, the failure is fatal and the "sound card"
> >> fails to come up at all.
> >>
> >>
> >> I see a couple of adjustments that could be made.
> >>
> >> 1. Zero out some fields in the driver:
> >>
> >> @@ -390,4 +390,6 @@ static int atmel_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> >> state->enabled = true;
> >> } else {
> >> + state->period = 0;
> >> + state->duty_cycle = 0;
> >> state->enabled = false;
> >> }
> >
> > I don't particularily like that. While state->period is an invalid
> > value, IMHO enabled = false is enough information from the driver's POV.
>
> This was the preferred approach of Thierry, and given the number of
> call sites for pwm_get_state with a local variable, I can sympathize
> with that view.
I looked a bit more into the issue and think that pwm_get_state() isn't
problematic. pwm_get_state() fully assigns *state.
> At the same time, fixing drivers one by one is not
> a fun game, so I can certainly see that approach 3 also has an appeal.
What I don't like about it is that the output of a disabled PWM doesn't
have a period. There might be one configured in hardware(, and the
.get_state() callback might or might not return that), but the emitted
signal has no well-defined period.
> >> 2. Don't try to "adjust" a disabled PWM:
> >>
> >> @@ -656,7 +656,7 @@ int pwm_adjust_config(struct pwm_device *pwm)
> >> * In either case, we setup the new period and polarity, and assign a
> >> * duty cycle of 0.
> >> */
> >> - if (!state.period) {
> >> + if (!state.enabled || !state.period) {
> >> state.duty_cycle = 0;
> >> state.period = pargs.period;
> >> state.polarity = pargs.polarity;
> >
> > In my book code that calls pwm_get_state() should consider .period
> > (and .duty_cycle) undefined if .enabled is false. So this hunk is an
>
> I agree fully. This feels like a good hunk.
>
> > improvement. Having said that, I think pwm_adjust_config() has a strange
> > semantic. I don't understand when you would want to call it, and it only
> > has one caller (i.e. pwm-regulator).
>
> I believe it's for a case where some bootstrap/bootloader has configured
> a PWM in order to set up a regulator for some critical component, and the
> kernel needs to cake over the responsibility seamlessly, or everything
> will take a dive. I deduced that from the description of pwm_adjust_config:
If it wants to take over seamlessly, it shouldn't call pwm_apply_state()
at all?!
> * This function will adjust the PWM config to the PWM arguments provided
> * by the DT or PWM lookup table. This is particularly useful to adapt
> * the bootloader config to the Linux one.
>
> But what do I know?
> > So another option would be
> >
> > diff --git a/drivers/regulator/pwm-regulator.c b/drivers/regulator/pwm-regulator.c
> > index b64d99695b84..418aff0ddbed 100644
> > --- a/drivers/regulator/pwm-regulator.c
> > +++ b/drivers/regulator/pwm-regulator.c
> > @@ -368,10 +368,6 @@ static int pwm_regulator_probe(struct platform_device *pdev)
> > return ret;
> > }
> >
> > - ret = pwm_adjust_config(drvdata->pwm);
> > - if (ret)
> > - return ret;
> > -
> > regulator = devm_regulator_register(&pdev->dev,
> > &drvdata->desc, &config);
> > if (IS_ERR(regulator)) {
> >
> > and drop pwm_adjust_config() as a followup?!
>
> As described above, I think that might be too drastic?
>
> >> 3. Zero out the state before calling pwm_get_state:
> >>
> >> @@ -115,7 +115,7 @@ static int pwm_device_request(struct pwm_device *pwm, const char *label)
> >> }
> >>
> >> if (pwm->chip->ops->get_state) {
> >> - struct pwm_state state;
> >> + struct pwm_state state = { .enabled = true };
> >
> > I wonder why you picked .enabled = true here but = false on all other
> > code locations.
>
> Silly typo, soory.
>
> > Also you don't seem to have 1271a7b98e7989ba6bb978e14403fc84efe16e13
> > which has the same effect and is included in v6.3 and v6.2.13.
>
> Right, approach 3 felt so ugly and intrusive that I didn't bother to take
> it beyond the problematic commit (and hinted at that further down: "It also
> needs a rebase from the culprit commit"). Having said that, 1271a7b98e79 is
> not enough, or else I would never have noticed the problem in the first
> place. I don't think the hunk in pwm_dbg_show() makes any difference for my
> case since I heven't enabled debugging, and a double-check confirms it:
> After 1271a7b98e79, zeroing "state" in pwm_adjust_config() is enough to kill
> this regression.
That really puzzles me because pwm_get_state() fully overwrites state.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Hi!
2023-05-22 at 22:43, Uwe Kleine-König wrote:
> Hello Peter,
>
> On Mon, May 22, 2023 at 09:28:39PM +0200, Peter Rosin wrote:
>> 2023-05-22 at 19:23, Uwe Kleine-König wrote:
>>> On Mon, May 22, 2023 at 05:19:43PM +0200, Peter Rosin wrote:
>>>> I have a device with a "sound card" that has an amplifier that needs
>>>> an extra boost when high amplification is requested. This extra
>>>> boost is controlled with a pwm-regulator.
>>>>
>>>> As of commit c73a3107624d ("pwm: Handle .get_state() failures") this
>>>> device no longer works. I have tracked the problem to an unfortunate
>>>> interaction between the underlying PWM driver and the PWM core.
>>>>
>>>> The driver is drivers/pwm/pwm-atmel.c which has difficulties getting
>>>> the period and/or duty_cycle from the HW when the PWM is not enabled.
>>>> Because of this, I think, the driver does not fill in .period and
>>>> .duty_cycle at all in atmel_pwm_get_state() unless the PWM is enabled.
>>>>
>>>> However, the PWM core is not expecting these fields to be left as-is,
>>>> at least not in pwm_adjust_config(), and its local state variable on
>>>> the stack ends up with whatever crap was on the stack on entry for
>>>> these fields. That fails spectacularly when the function continues to
>>>> do math on these uninitialized values.
>
> After looking again, I don't understand that part. Note that
> pwm_get_state() doesn't call .get_state() at all. Also pwmchip_add()
> zero initializes .state, then pwm_get() calls .get_state() (via
> pwm_device_request() which is called in .xlate()) which (if the HW is
> disabled) doesn't touch .period, so it should continue to be zero?!
>
> So I wonder why your approach 2 works at all. Do you see what I'm
> missing?
I tried various things on top of v6.3 and you are right. My hunks do
not make a (significant) difference there.
So, I took a step back and can only conclude that there must be some
another regression to find, and I was confused by that other regression.
In short, I was on 6.1.<foo> and everything was fine, and then I bumped
to 6.3 and a process crashed. I went to 6.2 and that same process also
crashed. I then totally focused on v6.1..v6.2 to figure out the problem.
I simply assumed v6.3 had the same problem because the symptom from
30.000ft was the same (that process died). I failed to go back to v6.3
to confirm that it was indeed the same problem as I had found in the
v6.1..v6.2 range.
My bad, it seems I have another day of bisections lined up.
>>>> In particular, I find this in the kernel log when a bad kernel runs:
>>>> pwm-regulator: probe of reg-ana failed with error -22
>>>>
>>>> Before commit c73a3107624d this was a silent failure, and the situation
>>>> "repaired itself" when the PWM was later reprogrammed, at least for my
>>>> case. After that commit, the failure is fatal and the "sound card"
>>>> fails to come up at all.
>>>>
>>>>
>>>> I see a couple of adjustments that could be made.
>>>>
>>>> 1. Zero out some fields in the driver:
>>>>
>>>> @@ -390,4 +390,6 @@ static int atmel_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
>>>> state->enabled = true;
>>>> } else {
>>>> + state->period = 0;
>>>> + state->duty_cycle = 0;
>>>> state->enabled = false;
>>>> }
>>>
>>> I don't particularily like that. While state->period is an invalid
>>> value, IMHO enabled = false is enough information from the driver's POV.
>>
>> This was the preferred approach of Thierry, and given the number of
>> call sites for pwm_get_state with a local variable, I can sympathize
>> with that view.
>
> I looked a bit more into the issue and think that pwm_get_state() isn't
> problematic. pwm_get_state() fully assigns *state.
Right, the only way pwm_get_state() can be problematic is if bogus values
have made their way into pwm->state earlier. Which is what happened for
v6.2.
>> At the same time, fixing drivers one by one is not
>> a fun game, so I can certainly see that approach 3 also has an appeal.
>
> What I don't like about it is that the output of a disabled PWM doesn't
> have a period. There might be one configured in hardware(, and the
> .get_state() callback might or might not return that), but the emitted
> signal has no well-defined period.
>
>>>> 2. Don't try to "adjust" a disabled PWM:
>>>>
>>>> @@ -656,7 +656,7 @@ int pwm_adjust_config(struct pwm_device *pwm)
>>>> * In either case, we setup the new period and polarity, and assign a
>>>> * duty cycle of 0.
>>>> */
>>>> - if (!state.period) {
>>>> + if (!state.enabled || !state.period) {
>>>> state.duty_cycle = 0;
>>>> state.period = pargs.period;
>>>> state.polarity = pargs.polarity;
>>>
>>> In my book code that calls pwm_get_state() should consider .period
>>> (and .duty_cycle) undefined if .enabled is false. So this hunk is an
>>
>> I agree fully. This feels like a good hunk.
>>
>>> improvement. Having said that, I think pwm_adjust_config() has a strange
>>> semantic. I don't understand when you would want to call it, and it only
>>> has one caller (i.e. pwm-regulator).
>>
>> I believe it's for a case where some bootstrap/bootloader has configured
>> a PWM in order to set up a regulator for some critical component, and the
>> kernel needs to cake over the responsibility seamlessly, or everything
>> will take a dive. I deduced that from the description of pwm_adjust_config:
>
> If it wants to take over seamlessly, it shouldn't call pwm_apply_state()
> at all?!
>
>> * This function will adjust the PWM config to the PWM arguments provided
>> * by the DT or PWM lookup table. This is particularly useful to adapt
>> * the bootloader config to the Linux one.
>>
>> But what do I know?
>
>>> So another option would be
>>>
>>> diff --git a/drivers/regulator/pwm-regulator.c b/drivers/regulator/pwm-regulator.c
>>> index b64d99695b84..418aff0ddbed 100644
>>> --- a/drivers/regulator/pwm-regulator.c
>>> +++ b/drivers/regulator/pwm-regulator.c
>>> @@ -368,10 +368,6 @@ static int pwm_regulator_probe(struct platform_device *pdev)
>>> return ret;
>>> }
>>>
>>> - ret = pwm_adjust_config(drvdata->pwm);
>>> - if (ret)
>>> - return ret;
>>> -
>>> regulator = devm_regulator_register(&pdev->dev,
>>> &drvdata->desc, &config);
>>> if (IS_ERR(regulator)) {
>>>
>>> and drop pwm_adjust_config() as a followup?!
>>
>> As described above, I think that might be too drastic?
>>
>>>> 3. Zero out the state before calling pwm_get_state:
>>>>
>>>> @@ -115,7 +115,7 @@ static int pwm_device_request(struct pwm_device *pwm, const char *label)
>>>> }
>>>>
>>>> if (pwm->chip->ops->get_state) {
>>>> - struct pwm_state state;
>>>> + struct pwm_state state = { .enabled = true };
>>>
>>> I wonder why you picked .enabled = true here but = false on all other
>>> code locations.
>>
>> Silly typo, soory.
>>
>>> Also you don't seem to have 1271a7b98e7989ba6bb978e14403fc84efe16e13
>>> which has the same effect and is included in v6.3 and v6.2.13.
>>
>> Right, approach 3 felt so ugly and intrusive that I didn't bother to take
>> it beyond the problematic commit (and hinted at that further down: "It also
>> needs a rebase from the culprit commit"). Having said that, 1271a7b98e79 is
>> not enough, or else I would never have noticed the problem in the first
>> place. I don't think the hunk in pwm_dbg_show() makes any difference for my
>> case since I heven't enabled debugging, and a double-check confirms it:
>> After 1271a7b98e79, zeroing "state" in pwm_adjust_config() is enough to kill
>> this regression.
>
> That really puzzles me because pwm_get_state() fully overwrites state.
I only verified that I had no problem /with/ my change. See above.
Sorry for wasting time.
Cheers,
Peter
>
> Best regards
> Uwe
>
2023-05-23 at 01:34, Peter Rosin wrote:
> So, I took a step back and can only conclude that there must be some
> another regression to find, and I was confused by that other regression.
> In short, I was on 6.1.<foo> and everything was fine, and then I bumped
> to 6.3 and a process crashed. I went to 6.2 and that same process also
> crashed. I then totally focused on v6.1..v6.2 to figure out the problem.
> I simply assumed v6.3 had the same problem because the symptom from
> 30.000ft was the same (that process died). I failed to go back to v6.3
> to confirm that it was indeed the same problem as I had found in the
> v6.1..v6.2 range.
>
> My bad, it seems I have another day of bisections lined up.
For closure, I ended up with this:
https://lore.kernel.org/lkml/[email protected]/
I.e. another v6.1..v6.2 regression that caused sound failures.
The two problems looked very similar to the suffering application.
Anyway, sorry again for the noise.
Cheers,
Peter
On Mon, May 22, 2023 at 10:43:46PM +0200, Uwe Kleine-König wrote:
> Hello Peter,
>
> On Mon, May 22, 2023 at 09:28:39PM +0200, Peter Rosin wrote:
> > 2023-05-22 at 19:23, Uwe Kleine-König wrote:
> > > On Mon, May 22, 2023 at 05:19:43PM +0200, Peter Rosin wrote:
> > >> I have a device with a "sound card" that has an amplifier that needs
> > >> an extra boost when high amplification is requested. This extra
> > >> boost is controlled with a pwm-regulator.
> > >>
> > >> As of commit c73a3107624d ("pwm: Handle .get_state() failures") this
> > >> device no longer works. I have tracked the problem to an unfortunate
> > >> interaction between the underlying PWM driver and the PWM core.
> > >>
> > >> The driver is drivers/pwm/pwm-atmel.c which has difficulties getting
> > >> the period and/or duty_cycle from the HW when the PWM is not enabled.
> > >> Because of this, I think, the driver does not fill in .period and
> > >> .duty_cycle at all in atmel_pwm_get_state() unless the PWM is enabled.
> > >>
> > >> However, the PWM core is not expecting these fields to be left as-is,
> > >> at least not in pwm_adjust_config(), and its local state variable on
> > >> the stack ends up with whatever crap was on the stack on entry for
> > >> these fields. That fails spectacularly when the function continues to
> > >> do math on these uninitialized values.
>
> After looking again, I don't understand that part. Note that
> pwm_get_state() doesn't call .get_state() at all. Also pwmchip_add()
> zero initializes .state, then pwm_get() calls .get_state() (via
> pwm_device_request() which is called in .xlate()) which (if the HW is
> disabled) doesn't touch .period, so it should continue to be zero?!
>
> So I wonder why your approach 2 works at all. Do you see what I'm
> missing?
>
> > >> In particular, I find this in the kernel log when a bad kernel runs:
> > >> pwm-regulator: probe of reg-ana failed with error -22
> > >>
> > >> Before commit c73a3107624d this was a silent failure, and the situation
> > >> "repaired itself" when the PWM was later reprogrammed, at least for my
> > >> case. After that commit, the failure is fatal and the "sound card"
> > >> fails to come up at all.
> > >>
> > >>
> > >> I see a couple of adjustments that could be made.
> > >>
> > >> 1. Zero out some fields in the driver:
> > >>
> > >> @@ -390,4 +390,6 @@ static int atmel_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> > >> state->enabled = true;
> > >> } else {
> > >> + state->period = 0;
> > >> + state->duty_cycle = 0;
> > >> state->enabled = false;
> > >> }
> > >
> > > I don't particularily like that. While state->period is an invalid
> > > value, IMHO enabled = false is enough information from the driver's POV.
> >
> > This was the preferred approach of Thierry, and given the number of
> > call sites for pwm_get_state with a local variable, I can sympathize
> > with that view.
>
> I looked a bit more into the issue and think that pwm_get_state() isn't
> problematic. pwm_get_state() fully assigns *state.
>
> > At the same time, fixing drivers one by one is not
> > a fun game, so I can certainly see that approach 3 also has an appeal.
>
> What I don't like about it is that the output of a disabled PWM doesn't
> have a period. There might be one configured in hardware(, and the
> .get_state() callback might or might not return that), but the emitted
> signal has no well-defined period.
Well, this is a bit of a gray area, admittedly. .get_state() was not
designed with regards to drivers that are unable to read the hardware
state. Essentially if you can't read the full hardware state, the
assumption was that you just shouldn't provide a .get_state() callback
at all.
In retrospect that's perhaps not ideal, and as you pointed out this is
all a bit moot as of v6.3 because the initial state is now effectively
zeroed out already.
But just to address your point about enabled = false being enough: it's
not. The reason is that consumers should be able to do something along
these lines:
pwm_get_state(pwm, &state);
state.enabled = true;
pwm_apply_state(pwm, &state);
And expect the device to do something reasonable. If the PWM isn't
properly configured, that pwm_apply_state() should return an error. If
->get_state() returned random values, that may not be guaranteed. With
v6.3, the above should return -EINVAL for pwm-atmel because period ends
up being 0 and we check for that explicitly. And that's really the only
sane behavior for drivers that can't read back the full hardware state.
Again, since we have this in the core that should be good enough. But it
is still something that drivers need to be aware about. If you can't
determine a real value from hardware readout, period and duty-cycle
should be zeroed out so that consumers don't end up applying garbage
values accidentally.
Thierry
On Tue, May 23, 2023 at 10:42:34PM +0200, Peter Rosin wrote:
> 2023-05-23 at 01:34, Peter Rosin wrote:
> > So, I took a step back and can only conclude that there must be some
> > another regression to find, and I was confused by that other regression.
> > In short, I was on 6.1.<foo> and everything was fine, and then I bumped
> > to 6.3 and a process crashed. I went to 6.2 and that same process also
> > crashed. I then totally focused on v6.1..v6.2 to figure out the problem.
> > I simply assumed v6.3 had the same problem because the symptom from
> > 30.000ft was the same (that process died). I failed to go back to v6.3
> > to confirm that it was indeed the same problem as I had found in the
> > v6.1..v6.2 range.
> >
> > My bad, it seems I have another day of bisections lined up.
>
> For closure, I ended up with this:
> https://lore.kernel.org/lkml/[email protected]/
>
> I.e. another v6.1..v6.2 regression that caused sound failures.
> The two problems looked very similar to the suffering application.
>
> Anyway, sorry again for the noise.
OK. After your first mail I had the impression there is another PWM
releated problem to be reported, but it seems this isn't the case.
Just to have that explicit: Did I understand you right?
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Hello Thierry,
On Tue, May 23, 2023 at 10:31:48PM +0200, Thierry Reding wrote:
> On Mon, May 22, 2023 at 10:43:46PM +0200, Uwe Kleine-K?nig wrote:
> > On Mon, May 22, 2023 at 09:28:39PM +0200, Peter Rosin wrote:
> > > 2023-05-22 at 19:23, Uwe Kleine-K?nig wrote:
> > > > On Mon, May 22, 2023 at 05:19:43PM +0200, Peter Rosin wrote:
> > > >> I have a device with a "sound card" that has an amplifier that needs
> > > >> an extra boost when high amplification is requested. This extra
> > > >> boost is controlled with a pwm-regulator.
> > > >>
> > > >> As of commit c73a3107624d ("pwm: Handle .get_state() failures") this
> > > >> device no longer works. I have tracked the problem to an unfortunate
> > > >> interaction between the underlying PWM driver and the PWM core.
> > > >>
> > > >> The driver is drivers/pwm/pwm-atmel.c which has difficulties getting
> > > >> the period and/or duty_cycle from the HW when the PWM is not enabled.
> > > >> Because of this, I think, the driver does not fill in .period and
> > > >> .duty_cycle at all in atmel_pwm_get_state() unless the PWM is enabled.
> > > >>
> > > >> However, the PWM core is not expecting these fields to be left as-is,
> > > >> at least not in pwm_adjust_config(), and its local state variable on
> > > >> the stack ends up with whatever crap was on the stack on entry for
> > > >> these fields. That fails spectacularly when the function continues to
> > > >> do math on these uninitialized values.
> >
> > After looking again, I don't understand that part. Note that
> > pwm_get_state() doesn't call .get_state() at all. Also pwmchip_add()
> > zero initializes .state, then pwm_get() calls .get_state() (via
> > pwm_device_request() which is called in .xlate()) which (if the HW is
> > disabled) doesn't touch .period, so it should continue to be zero?!
> >
> > So I wonder why your approach 2 works at all. Do you see what I'm
> > missing?
> >
> > > >> In particular, I find this in the kernel log when a bad kernel runs:
> > > >> pwm-regulator: probe of reg-ana failed with error -22
> > > >>
> > > >> Before commit c73a3107624d this was a silent failure, and the situation
> > > >> "repaired itself" when the PWM was later reprogrammed, at least for my
> > > >> case. After that commit, the failure is fatal and the "sound card"
> > > >> fails to come up at all.
> > > >>
> > > >>
> > > >> I see a couple of adjustments that could be made.
> > > >>
> > > >> 1. Zero out some fields in the driver:
> > > >>
> > > >> @@ -390,4 +390,6 @@ static int atmel_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> > > >> state->enabled = true;
> > > >> } else {
> > > >> + state->period = 0;
> > > >> + state->duty_cycle = 0;
> > > >> state->enabled = false;
> > > >> }
> > > >
> > > > I don't particularily like that. While state->period is an invalid
> > > > value, IMHO enabled = false is enough information from the driver's POV.
> > >
> > > This was the preferred approach of Thierry, and given the number of
> > > call sites for pwm_get_state with a local variable, I can sympathize
> > > with that view.
> >
> > I looked a bit more into the issue and think that pwm_get_state() isn't
> > problematic. pwm_get_state() fully assigns *state.
> >
> > > At the same time, fixing drivers one by one is not
> > > a fun game, so I can certainly see that approach 3 also has an appeal.
> >
> > What I don't like about it is that the output of a disabled PWM doesn't
> > have a period. There might be one configured in hardware(, and the
> > .get_state() callback might or might not return that), but the emitted
> > signal has no well-defined period.
>
> Well, this is a bit of a gray area, admittedly. .get_state() was not
> designed with regards to drivers that are unable to read the hardware
> state. Essentially if you can't read the full hardware state, the
> assumption was that you just shouldn't provide a .get_state() callback
> at all.
>
> In retrospect that's perhaps not ideal, and as you pointed out this is
> all a bit moot as of v6.3 because the initial state is now effectively
> zeroed out already.
>
> But just to address your point about enabled = false being enough: it's
> not. The reason is that consumers should be able to do something along
> these lines:
>
> pwm_get_state(pwm, &state);
> state.enabled = true;
> pwm_apply_state(pwm, &state);
>
> And expect the device to do something reasonable. If the PWM isn't
> properly configured, that pwm_apply_state() should return an error. If
> ->get_state() returned random values, that may not be guaranteed. With
> v6.3, the above should return -EINVAL for pwm-atmel because period ends
> up being 0 and we check for that explicitly. And that's really the only
> sane behavior for drivers that can't read back the full hardware state.
This is something that we don't agree about. IMHO it's better if
consumers specify the state they want to apply completely themselves
without using pwm_get_state(). I'm convinced that many people don't
understand pwm_get_state() and the resulting corner cases.
One such corner case is the one we hit here, that pwm_get_state() can
return a state that cannot be reapplied. Another common misunderstanding
is that (apart from directly after driver init (or was it after each
request?)) pwm_get_state() isn't about the actual HW state, but about
the state that was last applied successfully.
Do you think that I should be able to do:
pwm_apply(mypwm, &(struct pwm_state){ .period = 0, .duty_cycle = 0, .enabled = false, .polarity = PWM_POLARITY_NORMAL });
? This should be enough for a driver to know what to do. After that,
what is the "reasonable" thing that should happen if I follow up with
pwm_get_state(pwm, &state);
state.enabled = true;
pwm_apply_state(pwm, &state);
?
The current state is that the first pwm_apply() fails. I didn't check,
but I can imagine that there are other combinations of (state, lowlevel
driver) where pwm_apply() succeeds with .enabled = false but not with
.enabled = true. (For example consider a driver that can only do
inversed polarity, or polarity > 100000. What should happen when I call
pwm_apply(mypwm, &(struct pwm_state){ .period = 100, .duty_cycle = 20, .enabled = false, .polarity = PWM_POLARITY_NORMAL });
?)
If I was to design a PWM API today, I'd consider to not put .enabled
into pwm_state. With that enabled = true would be implied for
pwm_apply_state(), and there would be a separate pwm_disable().
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Hi!
2023-05-24 at 08:07, Uwe Kleine-König wrote:
> On Tue, May 23, 2023 at 10:42:34PM +0200, Peter Rosin wrote:
>> 2023-05-23 at 01:34, Peter Rosin wrote:
>>> So, I took a step back and can only conclude that there must be some
>>> another regression to find, and I was confused by that other regression.
>>> In short, I was on 6.1.<foo> and everything was fine, and then I bumped
>>> to 6.3 and a process crashed. I went to 6.2 and that same process also
>>> crashed. I then totally focused on v6.1..v6.2 to figure out the problem.
>>> I simply assumed v6.3 had the same problem because the symptom from
>>> 30.000ft was the same (that process died). I failed to go back to v6.3
>>> to confirm that it was indeed the same problem as I had found in the
>>> v6.1..v6.2 range.
>>>
>>> My bad, it seems I have another day of bisections lined up.
>>
>> For closure, I ended up with this:
>> https://lore.kernel.org/lkml/[email protected]/
>>
>> I.e. another v6.1..v6.2 regression that caused sound failures.
>> The two problems looked very similar to the suffering application.
>>
>> Anyway, sorry again for the noise.
>
> OK. After your first mail I had the impression there is another PWM
> releated problem to be reported, but it seems this isn't the case.
>
> Just to have that explicit: Did I understand you right?
The PWM regression was already solved by 1271a7b98e79 ("pwm: Zero-
initialize the pwm_state passed to driver's .get_state()") and I failed
to notice that. I was simply confused by that other totally unrelated
DMA problem that made me think the PWM problem persisted into the
present when in fact it didn't.
All is well.
Cheers,
Peter
#regzbot fixed-by: 1271a7b98e79
On 22.05.23 17:19, Peter Rosin wrote:
>
> I have a device with a "sound card" that has an amplifier that needs
> an extra boost when high amplification is requested. This extra
> boost is controlled with a pwm-regulator.
>
> As of commit c73a3107624d ("pwm: Handle .get_state() failures") this
> device no longer works. I have tracked the problem to an unfortunate
> interaction between the underlying PWM driver and the PWM core.
> [...]>
> Approach 1. will maybe clobber the saved pwm->state such that
> it no longer works to get the period/duty_cycle if/when the
> PWM is disabled? Maybe only for some corner case? But that might
> be a significant corner case?
>
> Approach 2. will maybe mess up some unrelated functionality?
>
> Approach 3. is ugly, intrusive and is in all likelihood
> incomplete. It also needs a rebase from the culprit commit.
>
> #regzbot introduced c73a3107624d
What happened to this? There was quite a bit of discussion, but then
nothing happened anymore.
Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
If I did something stupid, please tell me, as explained on that page.
#regzbot poke
Hi!
2023-06-20 at 16:24, Thorsten Leemhuis wrote:
> On 22.05.23 17:19, Peter Rosin wrote:
>>
>> I have a device with a "sound card" that has an amplifier that needs
>> an extra boost when high amplification is requested. This extra
>> boost is controlled with a pwm-regulator.
>>
>> As of commit c73a3107624d ("pwm: Handle .get_state() failures") this
>> device no longer works. I have tracked the problem to an unfortunate
>> interaction between the underlying PWM driver and the PWM core.
>> [...]>
>> Approach 1. will maybe clobber the saved pwm->state such that
>> it no longer works to get the period/duty_cycle if/when the
>> PWM is disabled? Maybe only for some corner case? But that might
>> be a significant corner case?
>>
>> Approach 2. will maybe mess up some unrelated functionality?
>>
>> Approach 3. is ugly, intrusive and is in all likelihood
>> incomplete. It also needs a rebase from the culprit commit.
>>
>> #regzbot introduced c73a3107624d
>
> What happened to this? There was quite a bit of discussion, but then
> nothing happened anymore.
I was seriously confused by another regression that caused my
user space to fail similarly (The sound driver didn't behave, but
for vastly different reasons). My bad, I should have made more
thorough checks before crying wolf.
So, this regression had already been fixed when I reported it.
For the curious, the other regression has also been fixed, merged
and picked up by stable. For the really curious, 2a6c7e8cc74e
("dmaengine: at_hdmac: Repair bitfield macros for peripheral ID
handling")
All in all, please close this one.
Cheers,
Peter
On 20.06.23 17:43, Peter Rosin wrote:
>
> 2023-06-20 at 16:24, Thorsten Leemhuis wrote:
>> On 22.05.23 17:19, Peter Rosin wrote:
>>>
>>> I have a device with a "sound card" that has an amplifier that needs
>>> an extra boost when high amplification is requested. This extra
>>> boost is controlled with a pwm-regulator.
>>>
>>> As of commit c73a3107624d ("pwm: Handle .get_state() failures") this
>>> device no longer works. I have tracked the problem to an unfortunate
>>> interaction between the underlying PWM driver and the PWM core.
>>> [...]>
>>> Approach 1. will maybe clobber the saved pwm->state such that
>>> it no longer works to get the period/duty_cycle if/when the
>>> PWM is disabled? Maybe only for some corner case? But that might
>>> be a significant corner case?
>>>
>>> Approach 2. will maybe mess up some unrelated functionality?
>>>
>>> Approach 3. is ugly, intrusive and is in all likelihood
>>> incomplete. It also needs a rebase from the culprit commit.
>>>
>>> #regzbot introduced c73a3107624d
>>
>> What happened to this? There was quite a bit of discussion, but then
>> nothing happened anymore.
>
> I was seriously confused by another regression that caused my
> user space to fail similarly (The sound driver didn't behave, but
> for vastly different reasons). My bad, I should have made more
> thorough checks before crying wolf.
Happens, no worries.
> So, this regression had already been fixed when I reported it.
> For the curious, the other regression has also been fixed, merged
> and picked up by stable. For the really curious, 2a6c7e8cc74e
> ("dmaengine: at_hdmac: Repair bitfield macros for peripheral ID
> handling")
>
> All in all, please close this one.
Okydo, thx for the update!
#regzbot resolve: "this regression had already been fixed when I
reported it"
Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
If I did something stupid, please tell me, as explained on that page.