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: <20220805155509.edqwxcvyoqfic4pn@pengutronix.de>
Subject: RE: [PATCH] pwm: removes period check from pwm_apply_state()
Date:   Mon, 8 Aug 2022 19:47:03 +0530
Message-ID: <019701d8ab31$94c86d60$be594820$@samsung.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="iso-8859-1"
Content-Transfer-Encoding: 7bit
Content-Language: en-in
Thread-Index: AQE14N23Ecc1n6hvU/1TooO9Vl2YEgEF3maVAoBW2yquzm0akA==
CMS-TYPE: 105P
DLP-Filter: Pass
References: <CGME20220805102056epcas5p29f22d42c854bebe6d0301b56094cf3ea@epcas5p2.samsung.com>
        <20220805101125.47955-1-m.shams@samsung.com>
        <20220805155509.edqwxcvyoqfic4pn@pengutronix.de>
Precedence: bulk


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