Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   "m.shams" <m.shams@samsung.com>
To:     =?iso-8859-1?Q?'Uwe_Kleine-K=F6nig'?= 
        <u.kleine-koenig@pengutronix.de>
Cc:     <thierry.reding@gmail.com>, <lee.jones@linaro.org>,
        <linux-pwm@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <alim.akhtar@samsung.com>
In-Reply-To: <20220808174842.jiato34jzqstchdn@pengutronix.de>
Subject: RE: [PATCH] pwm: removes period check from pwm_apply_state()
Date:   Wed, 10 Aug 2022 17:09:30 +0530
Message-ID: <004301d8acad$e31fee70$a95fcb50$@samsung.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="iso-8859-1"
Content-Transfer-Encoding: 7bit
Thread-Index: AQE14N23Ecc1n6hvU/1TooO9Vl2YEgEF3maVAoBW2yoC+wqOdQK9h22TrqOiL9A=
Content-Language: en-in
CMS-TYPE: 105P
DLP-Filter: Pass
References: <CGME20220805102056epcas5p29f22d42c854bebe6d0301b56094cf3ea@epcas5p2.samsung.com>
        <20220805101125.47955-1-m.shams@samsung.com>
        <20220805155509.edqwxcvyoqfic4pn@pengutronix.de>
        <019701d8ab31$94c86d60$be594820$@samsung.com>
        <20220808174842.jiato34jzqstchdn@pengutronix.de>
Precedence: bulk

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