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;
Date:   Fri, 5 Aug 2022 17:55:09 +0200
From:   Uwe =?utf-8?Q?Kleine-K=C3=B6nig?= <u.kleine-koenig@pengutronix.de>
To:     Tamseel 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()
Message-ID: <20220805155509.edqwxcvyoqfic4pn@pengutronix.de>
References: <CGME20220805102056epcas5p29f22d42c854bebe6d0301b56094cf3ea@epcas5p2.samsung.com>
 <20220805101125.47955-1-m.shams@samsung.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="xs6xr6e4bpqmmt22"
Content-Disposition: inline
In-Reply-To: <20220805101125.47955-1-m.shams@samsung.com>
Precedence: bulk


--xs6xr6e4bpqmmt22
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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 struc=
t pwm_state *state)
 	 */
 	might_sleep();
=20
-	if (!pwm || !state || !state->period ||
-	    state->duty_cycle > state->period)
+	if (!pwm || !state || state->enabled && (!state->period ||
+	    state->duty_cycle > state->period))
 		return -EINVAL;
=20
 	chip =3D pwm->chip;

That is, don't refuse calling pwm_apply_state() for state->period =3D 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

--=20
Pengutronix e.K.                           | Uwe Kleine-K=F6nig            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

--xs6xr6e4bpqmmt22
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQEzBAABCgAdFiEEfnIqFpAYrP8+dKQLwfwUeK3K7AkFAmLtPVsACgkQwfwUeK3K
7AkyeggAlIF4i7X3hL+F6WL3ld2fObx/1FvrgJASGSAoNdEVn0srMhrNEhjZrugG
Bdy3IESIAp86OHLfMwyAbF+w1Ww/mXcTq8Me3BrRN5cN2D4mtm0J4Pm4q3A2K85x
JdTfOiUV6exqQZScwhvSym3YMPwZO2sRc2gY2idBZhO4PRrdpE/aiifN0IdGKIQx
kEA7TEQfPm2/ZQR0nHpIGte2TLX8YzyHkdeRbmmV3+JNO/ywi+YwXM+aA79g1qVL
MGCsBDB6HjCu4RMpCpIZbGTuy9Xv7zFomVdvSMd8xF5vB5Y63JxsW0/JGZ1eP7VJ
bNo40HqSbGyEDCgwAipnhdUV+mVW1g==
=56AU
-----END PGP SIGNATURE-----

--xs6xr6e4bpqmmt22--