[PATCH CFT] usb: ucsi_ccg: Fix command completion handling

[PATCH CFT] usb: ucsi_ccg: Fix command completion handling

[Christian A. Ehrhardt]

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

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

Re: [PATCH CFT] usb: ucsi_ccg: Fix command completion handling

[Greg Kroah-Hartman]

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

Re: [PATCH CFT] usb: ucsi_ccg: Fix command completion handling

[Christian A. Ehrhardt]

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

Re: [PATCH CFT] usb: ucsi_ccg: Fix command completion handling

[HaoTien Hsu]

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

Re: [PATCH CFT] usb: ucsi_ccg: Fix command completion handling

[Christian A. Ehrhardt]

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