Messages in this thread | | | From | "m.shams" <> | Subject | RE: [PATCH] pwm: removes period check from pwm_apply_state() | Date | Fri, 19 Aug 2022 10:09:18 +0530 |
| |
Hi Uwe,
> -----Original Message----- > From: Uwe Kleine-König [mailto:u.kleine-koenig@pengutronix.de] > Sent: 10 August 2022 22:41 > To: m.shams <m.shams@samsung.com> > Cc: thierry.reding@gmail.com; lee.jones@linaro.org; linux- > pwm@vger.kernel.org; linux-kernel@vger.kernel.org; > alim.akhtar@samsung.com > Subject: Re: [PATCH] pwm: removes period check from pwm_apply_state() > > Hello, > > On Wed, Aug 10, 2022 at 05:09:30PM +0530, m.shams wrote: > > > 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.) > > > > > > > Sorry for the inconvenience. I have fixed my mailer. > > No you didn't. > > > > 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? > > > > > > > Looks like I had some local patch which was causing the error to > > trigger Kernel Panic (sorry about that). > > On removing those local changes, I do not observe kernel panic, but > > observe following error and then suspend fails. > > > > [ 63.963063] pwm pwmchip0: PM: dpm_run_callback (): > > pwm_class_suspend+0x0/0xf8 returns -22 > > [ 63.963079] pwm pwmchip0: PM: failed to suspend: error -22 > > > > So, as to fix this issue I will post a new version of patch containing > > change suggested by you. > > Note that my suggestion only covers you problem, it doesn't solve it. >
Your suggestion looks like a permanent fix, as there should not be any use case where we check for "period" and "duty_cycle", without PWM being in enabled state.
Thanks & Regards, Tamseel Shams
| |