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

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

[Tamseel Shams]

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(-)

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;
-- 
2.17.1

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

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1586 bytes --]

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

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[m.shams]

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

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

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 4228 bytes --]

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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[m.shams]

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

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

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 5329 bytes --]

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

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 484 bytes --]

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

[m.shams]

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