linux-kernel - [PATCH v2] pwm: improve state handling in ehrpwm driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-Id: <20241127222649.6394-1-rafael.v.volkmer@gmail.com>
Date: Wed, 27 Nov 2024 19:26:49 -0300
From: "Rafael V. Volkmer" <rafael.v.volkmer@...il.com>
To: ukleinek@...nel.org
Cc: linux-kernel@...r.kernel.org,
	linux-pwm@...r.kernel.org,
	rafael.v.volkmer@...il.com
Subject: [PATCH v2] pwm: improve state handling in ehrpwm driver

Introduce ehrpwm_is_enabled() to verify the module's enable/disable state
by checking the AQCSFRC and AQCTLA registers, instead of relying solely
on pwm->state.enabled. This ensures a more accurate representation of
the ePWM state in the kernel.

Previously, when performing fops operations directly in kernel space
after retrieving the platform device (pdev)—bypassing the sysfs interface—
pwm->state.enabled could incorrectly signal transitions between active
and inactive states, leading to inconsistent behavior.

Signed-off-by: Rafael V. Volkmer <rafael.v.volkmer@...il.com>
---
Hello Uwe,

Thank you very much for the feedback.

I understand your concern about the kernel layer structure, so I took 
a new approach that also fixes my problem, but this time without directly 
manipulating `pwm->state`, but rather through a double check of the registers
within `apply`.

I hope you can see the implementation and tell me, again, what you think.

Best regards

Changes in v2:
- Implemented `ehrpwm_is_enabled()` to check hardware registers instead of relying on `pwm->state.enabled`.
- Removed direct manipulation of `pwm->state` in `ehrpwm_pwm_apply()`.
- Addressed your feedback regarding kernel layer structure.

 drivers/pwm/pwm-tiehrpwm.c | 34 +++++++++++++++++++++++++++-------
 1 file changed, 27 insertions(+), 7 deletions(-)

diff --git a/drivers/pwm/pwm-tiehrpwm.c b/drivers/pwm/pwm-tiehrpwm.c
index 9f939d535440..fdcda0ffc9db 100644
--- a/drivers/pwm/pwm-tiehrpwm.c
+++ b/drivers/pwm/pwm-tiehrpwm.c
@@ -387,6 +387,25 @@ static void ehrpwm_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 	pm_runtime_put_sync(pwmchip_parent(chip));
 }
 
+static bool ehrpwm_is_enabled(struct pwm_chip *chip)
+{
+	struct ehrpwm_pwm_chip *pc = to_ehrpwm_pwm_chip(chip);
+	bool ret;
+	u16 aqcsfrc_reg;
+	u16 aqctla_reg;
+	u8 csfa_bits;
+	
+	aqcsfrc_reg = readw(pc->mmio_base + AQCSFRC);
+	csfa_bits = (u8)(aqcsfrc_reg & AQCSFRC_CSFA_MASK);
+
+	aqctla_reg= readw(pc->mmio_base + AQCTLA);
+
+	ret = (csfa_bits != 0u) ? false :
+			(aqctla_reg == 0u) ? false : true;
+
+	return ret;
+}
+
 static void ehrpwm_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
 {
 	struct ehrpwm_pwm_chip *pc = to_ehrpwm_pwm_chip(chip);
@@ -404,7 +423,9 @@ static int ehrpwm_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 			    const struct pwm_state *state)
 {
 	int err;
-	bool enabled = pwm->state.enabled;
+	bool enabled;
+
+	enabled = (ehrpwm_is_enabled(chip) | pwm->state.enabled);
 
 	if (state->polarity != pwm->state.polarity) {
 		if (enabled) {
@@ -417,10 +438,8 @@ static int ehrpwm_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 			return err;
 	}
 
-	if (!state->enabled) {
-		if (enabled)
-			ehrpwm_pwm_disable(chip, pwm);
+	if ((state->enabled != enabled) && (state->enabled == false)) {
+		ehrpwm_pwm_disable(chip, pwm);
 		return 0;
 	}
 
@@ -428,9 +447,10 @@ static int ehrpwm_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	if (err)
 		return err;
 
-	if (!enabled)
+	if ((state->enabled != enabled) && (state->enabled == true)) {
 		err = ehrpwm_pwm_enable(chip, pwm);
+		return err;
+	}
 
 	return err;
 }
-- 
2.25.1