Re: [PATCH] pwm: removes period check from pwm_apply_state()

[Uwe Kleine-König]

Hello,

I fixed up the quoting for you in this mail. Please fix your mailer to
not break quotes, this is quite annoying. (Looking at the headers of
your mail you're using Outlook. Then your only viable option is to
switch to a saner client.)

On Mon, Aug 08, 2022 at 07:47:03PM +0530, m.shams wrote:
> On Fri, Aug 05, 2022 at 03:41:25PM +0530, Tamseel Shams wrote:
> > > There may be situation when PWM is exported using sysfs, but at that 
> > > point PWM period is not set. At this situation if we issue a system 
> > > suspend, it calls pwm_class_suspend which in turn calls 
> > > pwm_apply_state, where PWM period value is checked which returns an 
> > > invalid argument error casuing Kernel to panic. So, check for PWM 
> > > period value is removed so as to fix the kernel panic observed during 
> > > suspend.
> >
> > This looks and sounds wrong. One thing I would accept is:
> >
> > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> > index 0e042410f6b9..075bbcdad6c1 100644
> > --- a/drivers/pwm/core.c
> > +++ b/drivers/pwm/core.c
> > @@ -557,8 +557,8 @@ int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state)
> > 	 */
> > 	might_sleep();
> > 
> > -	if (!pwm || !state || !state->period ||
> > -	    state->duty_cycle > state->period)
> > +	if (!pwm || !state || state->enabled && (!state->period ||
> > +	    state->duty_cycle > state->period))
> >  		return -EINVAL;
> > 
> > 	chip = pwm->chip;
> >
> > That is, don't refuse calling pwm_apply_state() for state->period = 0 and
> > even state->duty_cycle > state->period if the > > PWM is not enabled.
> 
> By this do you mean doing it following way?
> 
> if (!pwm || !state || (pwm && !state->period) ||
> 	    (pwm && state->duty_cycle > state->period))
> 		return -EINVAL;

No. Your expression is logically equivalent to what we already have. I
meant:

	if (!pwm || !state || state->enabled && (!state->period ||
	    state->duty_cycle > state->period))
		return -EINVAL;

Learning to read diffs (maybe Outlook scrambled the view for you, too?)
is a nice capability you should master.

> > But anyhow, even without that the kernel should not panic. So I ask you
> > to research and provide some more info about > > the problem. (Which
> > hardware does it affect? Where does it panic? ...)
> 
> Observing Kernel panic in exynos SoC when we issue system suspend. Following
> is the snippet of error:
> 
> # echo mem > /sys/power/state
> [   29.224784] 010: Kernel panic - not syncing: pwm pwmchip0:
> dpm_run_callback failure
> [   29.240134] 010: Call trace:
> [   29.242993] 010:  dump_backtrace+0x0/0x1b8
> [   29.247067] 010:  show_stack+0x24/0x30
> [   29.250793] 010:  dump_stack+0xb8/0x114
> [   29.254606] 010:  panic+0x180/0x398
> [   29.258073] 010:  dpm_run_callback+0x270/0x278
> [   29.262493] 010:  __device_suspend+0x15c/0x628
> [   29.266913] 010:  dpm_suspend+0x124/0x3b0
> [   29.270899] 010:  dpm_suspend_start+0xa0/0xa8
> [   29.275233] 010:  suspend_devices_and_enter+0x110/0x968
> [   29.280433] 010:  pm_suspend+0x308/0x3d8
> [   29.284333] 010:  state_store+0x8c/0x110
> [   29.288233] 010:  kobj_attr_store+0x14/0x28
> [   29.292393] 010:  sysfs_kf_write+0x5c/0x78
> [   29.296466] 010:  kernfs_fop_write+0x10c/0x220
> [   29.300886] 010:  __vfs_write+0x48/0x90
> [   29.304699] 010:  vfs_write+0xb8/0x1c0
> [   29.308426] 010:  ksys_write+0x74/0x100
> [   29.312240] 010:  __arm64_sys_write+0x24/0x30
> [   29.316573] 010:  el0_svc_handler+0x110/0x1b8
> [   29.320906] 010:  el0_svc+0x8/0x1bc
> [   29.324374] 010: SMP: stopping secondary CPUs
> [   29.328711] 010: Kernel Offset: disabled
> [   29.332607] 010: CPU features: 0x0002,00006008
> [   29.337026] 010: Memory Limit: none
> [   29.343949] 010: Rebooting in 1 seconds..
> [   30.344539] 010: Disabling non-boot CPUs ...

Just locking at that and starring at drivers/base/power/main.c for a
while doesn't make this clearer to me. Are you using a mainline kernel?
Which version?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Attachment: signature.asc
Description: PGP signature