linux-kernel - [PATCH CFT] usb: ucsi_ccg: Fix command completion handling

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-next>] [day] [month] [year] [list]

Date: Thu, 15 Feb 2024 11:10:24 +0100
From: "Christian A. Ehrhardt" <lk@...e.de>
To: linux-usb@...r.kernel.org,
	linux-kernel@...r.kernel.org
Cc: "Christian A. Ehrhardt" <lk@...e.de>,
	Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
	Uwe Kleine-König <u.kleine-koenig@...gutronix.de>,
	Sing-Han Chen <singhanc@...dia.com>,
	Haotien Hsu <haotienh@...dia.com>,
	Utkarsh Patel <utkarsh.h.patel@...el.com>,
	"Jonathan Hunter" <jonathanh@...dia.com>,
	"Wayne Chang" <waynec@...dia.com>,
	"WK Tsai" <wtsai@...dia.com>
Subject: [PATCH CFT] usb: ucsi_ccg: Fix command completion handling

In case of a spurious or otherwise delayed interrupt
it is possible that CCI still reports the previous completion.
For this reason the UCSI spec provides different completion
bits for normal commands and for UCSI_ACK_CC_CI.

Only complete a sync command if the correct completion bit
is set.

This should avoid the need to clear out CCI before starting
a command. Thus remove this code.

Signed-off-by: Christian A. Ehrhardt <lk@...e.de>
Fixes: e32fd989ac1c ("usb: typec: ucsi: ccg: Move to the new API")
---
Additional information:
A similar change for ucsi_acpi.c is here:
  https://lore.kernel.org/all/20240121204123.275441-3-lk@c--e.de/
This restores behaviour that ucsi.c had before moving to the new API.
I've seen timeouts with ucsi_acpi.c without that fix, often if there
were many port events (plug/unplug).

I do _not_ have CCG hardware to test this. So someone else will have to
provide a Tested-By tag or similar (hence the CFT in the subject).

But from looking at the code I think this change is needed for CCG,
too. Additionally, the recent change to CCG here
  https://lore.kernel.org/all/20240126030115.3791554-1-haotienh@nvidia.com/
seems to work around the same problem.

Clearing the cached CCI value should not be necessary with this
anymore and I suspect that it can potentially cause other problems.
However, I can send an update patch without this hunk if desired.


 drivers/usb/typec/ucsi/ucsi_ccg.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c b/drivers/usb/typec/ucsi/ucsi_ccg.c
index dda7c7c94e08..9442307e0abd 100644
--- a/drivers/usb/typec/ucsi/ucsi_ccg.c
+++ b/drivers/usb/typec/ucsi/ucsi_ccg.c
@@ -616,14 +616,6 @@ static int ucsi_ccg_async_write(struct ucsi *ucsi, unsigned int offset,
 	struct ucsi_ccg *uc = ucsi_get_drvdata(ucsi);
 	u16 reg = CCGX_RAB_UCSI_DATA_BLOCK(offset);
 
-	/*
-	 * UCSI may read CCI instantly after async_write,
-	 * clear CCI to avoid caller getting wrong data before we get CCI from ISR
-	 */
-	spin_lock(&uc->op_lock);
-	uc->op_data.cci = 0;
-	spin_unlock(&uc->op_lock);
-
 	return ccg_write(uc, reg, val, val_len);
 }
 
@@ -708,9 +700,14 @@ static irqreturn_t ccg_irq_handler(int irq, void *data)
 err_clear_irq:
 	ccg_write(uc, CCGX_RAB_INTR_REG, &intr_reg, sizeof(intr_reg));
 
-	if (!ret && test_bit(DEV_CMD_PENDING, &uc->flags) &&
-	    cci & (UCSI_CCI_ACK_COMPLETE | UCSI_CCI_COMMAND_COMPLETE))
-		complete(&uc->complete);
+	if (!ret && test_bit(DEV_CMD_PENDING, &uc->flags)) {
+		bool ack = UCSI_COMMAND(uc->last_cmd_sent) == UCSI_ACK_CC_CI;
+
+		if (ack && (cci & UCSI_CCI_ACK_COMPLETE))
+			complete(&uc->complete);
+		if (!ack && (cci & UCSI_CCI_COMMAND_COMPLETE))
+			complete(&uc->complete);
+	}
 
 	return IRQ_HANDLED;
 }
-- 
2.40.1