| Message ID | 20220805101125.47955-1-m.shams@samsung.com |
|---|---|
| State | Rejected |
| Headers | show |
| Series | pwm: removes period check from pwm_apply_state() | expand |
Hello, 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. 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? ...) Best regards Uwe
Hi Uwe, 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; > 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 ... Thanks & Regards, Tamseel
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
Hi Uwe, > 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.) > Sorry for the inconvenience. I have fixed my mailer. > 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. Best Regards, Tamseel Shams
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. Best regards Uwe
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
diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c index c7552df32082..69bca7f82398 100644 --- a/drivers/pwm/core.c +++ b/drivers/pwm/core.c @@ -634,8 +634,7 @@ 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->duty_cycle > state->period) return -EINVAL; chip = pwm->chip;
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. Signed-off-by: Tamseel Shams <m.shams@samsung.com> --- drivers/pwm/core.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)