| Message ID | 20240215101024.764444-1-lk@c--e.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [CFT] usb: ucsi_ccg: Fix command completion handling | expand |
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
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
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
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 --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; }
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(-)