diff mbox series

[CFT] usb: ucsi_ccg: Fix command completion handling

Message ID	20240215101024.764444-1-lk@c--e.de
State	New
Headers	Received-SPF: pass (google.com: domain of linux-kernel+bounces-66617-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3; From: "Christian A. Ehrhardt" <lk@c--e.de> To: linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org Cc: "Christian A. Ehrhardt" <lk@c--e.de>, Heikki Krogerus <heikki.krogerus@linux.intel.com>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Andy Shevchenko <andriy.shevchenko@linux.intel.com>, =?utf-8?q?Uwe_Kleine-K?= =?utf-8?q?=C3=B6nig?= <u.kleine-koenig@pengutronix.de>, Sing-Han Chen <singhanc@nvidia.com>, Haotien Hsu <haotienh@nvidia.com>, Utkarsh Patel <utkarsh.h.patel@intel.com>, "Jonathan Hunter" <jonathanh@nvidia.com>, "Wayne Chang" <waynec@nvidia.com>, "WK Tsai" <wtsai@nvidia.com> Subject: [PATCH CFT] usb: ucsi_ccg: Fix command completion handling Date: Thu, 15 Feb 2024 11:10:24 +0100 Message-Id: <20240215101024.764444-1-lk@c--e.de> Precedence: bulk MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-getmail-retrieved-from-mailbox: INBOX
Series	[CFT] usb: ucsi_ccg: Fix command completion handling \| [CFT] usb: ucsi_ccg: Fix command completion handling

Commit Message

Christian A. Ehrhardt Feb. 15, 2024, 10:10 a.m. UTC

  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@c--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(-)

Comments

Greg KH Feb. 15, 2024, 11:07 a.m. UTC | #1

On Thu, Feb 15, 2024 at 11:10:24AM +0100, Christian A. Ehrhardt wrote:
> 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@c--e.de>
> Fixes: e32fd989ac1c ("usb: typec: ucsi: ccg: Move to the new API")

What does "CFT" in your subject line mean?

thanks,

greg k-h

Christian A. Ehrhardt Feb. 15, 2024, 12:03 p.m. UTC | #2

Hi Greg,

On Thu, Feb 15, 2024 at 12:07:20PM +0100, Greg Kroah-Hartman wrote:
> On Thu, Feb 15, 2024 at 11:10:24AM +0100, Christian A. Ehrhardt wrote:
> > 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@c--e.de>
> > Fixes: e32fd989ac1c ("usb: typec: ucsi: ccg: Move to the new API")
> 
> What does "CFT" in your subject line mean?

It's supposed to mean "Call For Testers". More info in the
"Additional Information" section of the original mail.

I think the change is necessary and good but I do not have the HW
to test it.

I did test a similar change for ucsi_acpi.c that got merged and this
is the ping for ucsi_ccg.c people that they probably need this, too.

   regards   Christian

HaoTien Hsu Feb. 29, 2024, 7:18 a.m. UTC | #3

On 2/15/24 20:03, Christian A. Ehrhardt wrote:
> External email: Use caution opening links or attachments
> 
> 
> Hi Greg,
> 
> On Thu, Feb 15, 2024 at 12:07:20PM +0100, Greg Kroah-Hartman wrote:
>> On Thu, Feb 15, 2024 at 11:10:24AM +0100, Christian A. Ehrhardt wrote:
>>> 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@c--e.de>
>>> Fixes: e32fd989ac1c ("usb: typec: ucsi: ccg: Move to the new API")
>>
>> What does "CFT" in your subject line mean?
> 
> It's supposed to mean "Call For Testers". More info in the
> "Additional Information" section of the original mail.
> 
> I think the change is necessary and good but I do not have the HW
> to test it.
> 
> I did test a similar change for ucsi_acpi.c that got merged and this
> is the ping for ucsi_ccg.c people that they probably need this, too.
> 
>     regards   Christian
> 
> 

Hi Christian,

If we don't clean the CCI cache in ucsi_ccg_async_write(), there might 
be a potential problem when the driver is polling the results.

In ucsi_init(), we may get EPROBE_DEFER from ucsi_register_port().
Then it does ucsi_reset_ppm() before returning the error code, and we 
will get  UCSI_CCI_RESET_COMPLETE and store it in the CCI cache.
If we don't clean the cache, when the UCSI driver calls ucsi_init() 
again, then in ucsi_reset_ppm(), it will get UCSI_CCI_RESET_COMPLETE 
from the CCI cache instantly.
Then the driver will run the next UCSI commands when the HW is not 
completely reset.

Regards,
Haotien

Christian A. Ehrhardt Feb. 29, 2024, 8:13 p.m. UTC | #4

Hi Haotien,

On Thu, Feb 29, 2024 at 07:18:44AM +0000, HaoTien Hsu wrote:
> On 2/15/24 20:03, Christian A. Ehrhardt wrote:
> > External email: Use caution opening links or attachments
> > 
> > 
> > Hi Greg,
> > 
> > On Thu, Feb 15, 2024 at 12:07:20PM +0100, Greg Kroah-Hartman wrote:
> >> On Thu, Feb 15, 2024 at 11:10:24AM +0100, Christian A. Ehrhardt wrote:
> >>> 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@c--e.de>
> >>> Fixes: e32fd989ac1c ("usb: typec: ucsi: ccg: Move to the new API")
> >>
> >> What does "CFT" in your subject line mean?
> > 
> > It's supposed to mean "Call For Testers". More info in the
> > "Additional Information" section of the original mail.
> > 
> > I think the change is necessary and good but I do not have the HW
> > to test it.
> > 
> > I did test a similar change for ucsi_acpi.c that got merged and this
> > is the ping for ucsi_ccg.c people that they probably need this, too.
> > 
> >     regards   Christian
> > 
> > 
> 
> Hi Christian,
> 
> If we don't clean the CCI cache in ucsi_ccg_async_write(), there might 
> be a potential problem when the driver is polling the results.
> 
> In ucsi_init(), we may get EPROBE_DEFER from ucsi_register_port().
> Then it does ucsi_reset_ppm() before returning the error code, and we 
> will get  UCSI_CCI_RESET_COMPLETE and store it in the CCI cache.
> If we don't clean the cache, when the UCSI driver calls ucsi_init() 
> again, then in ucsi_reset_ppm(), it will get UCSI_CCI_RESET_COMPLETE 
> from the CCI cache instantly.
> Then the driver will run the next UCSI commands when the HW is not 
> completely reset.

Thanks, I indeed did not think the reset case completely through.
However, the real bugfix is in the other hunk of the diff and this
is a genuine bugfix on its own. I found that the corresponding
diff was neccessary for ucsi_acpi.c. Should I resend without the
CCI cleaning?

Thanks   Christian

diff mbox series

Patch

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;
 }