From 7771c52368e03bb63fd49eb303b3152b557b897b Mon Sep 17 00:00:00 2001 From: Caveh Jalali Date: Tue, 20 Jun 2017 17:44:55 -0700 Subject: [PATCH] pd_suspend: coordinate with pd_task(). looks like we had a bit of a race condition: set_state() was effectively just an assignment opration to pd[port].task_state. it's called asynchronously from pd_set_suspend() in response to a PD_SUSPEND message from the AP as well as from pd_task() before it enters its main event loop. this can take a long time because tcpci_tcpm_init() has a 300ms timeout. last one wins. similarly, when pd_task() is running its main loop, pd_set_suspend() really needs to wait for pd_task() to actually enter the PD_STATE_SUSPENDED state before the caller can assume that the pd_task() has stopped accessing the TCPC. the particular failure case was when depthcharge would decide to do a TCPC firmware update. it starts by sending a PD_SUSPEND to the EC, then accessing the TCPC. unfortunately, the pd_task() hadn't gotten out of the way yet, thus causing TCPC access chaos. so, i'm adding a req_suspend_state flag to the pd_protocol struct so we can tell pd_task() to suspend itself in a controlled manner. when pd_task() is ready to do a state change - basically at the top of the main event loop - it'll change to PD_STATE_SUSPENDED and clear the req_suspend_state flag. in any case, pd_set_suspend() still needs to wait around for pd_task() to enter the suspended state as we don't have a fancy handshake mechanism between these tasks. TEST=in combination with some follow-on CLs, ps8751 firmware update works properly where previously it needed a ~2 second delay for the EC pd_task() to settle. the way to trigger the failure was to insert or remove the power brick. BRANCH=none BUG=b:62356808 Change-Id: I363803ff60db31ccf84d592f8c9d1610fbe0f9ce Signed-off-by: Caveh Jalali Reviewed-on: https://chromium-review.googlesource.com/544659 Reviewed-by: Shawn N --- common/usb_pd_protocol.c | 44 ++++++++++++++++++++++++++++++++-------- 1 file changed, 35 insertions(+), 9 deletions(-) diff --git a/common/usb_pd_protocol.c b/common/usb_pd_protocol.c index ee4ae31ab0..d3cfa0da73 100644 --- a/common/usb_pd_protocol.c +++ b/common/usb_pd_protocol.c @@ -118,6 +118,8 @@ static struct pd_protocol { enum pd_states task_state; /* PD state when we run state handler the last time */ enum pd_states last_state; + /* bool: request state change to SUSPENDED */ + uint8_t req_suspend_state; /* The state to go to after timeout */ enum pd_states timeout_state; /* Timeout for the current state. Set to 0 for no timeout. */ @@ -1803,6 +1805,10 @@ void pd_task(void) if (!tcpm_get_message(port, payload, &head)) handle_request(port, head, payload); } + + if (pd[port].req_suspend_state) + set_state(port, PD_STATE_SUSPENDED); + /* if nothing to do, verify the state of the world in 500ms */ this_state = pd[port].task_state; timeout = 500*MSEC; @@ -2299,6 +2305,7 @@ void pd_task(void) break; case PD_STATE_SUSPENDED: CPRINTS("TCPC p%d suspended!", port); + pd[port].req_suspend_state = 0; #ifdef CONFIG_USB_PD_TCPC pd_rx_disable_monitoring(port); pd_hw_release(port); @@ -3108,19 +3115,38 @@ DECLARE_HOOK(HOOK_CHIPSET_SHUTDOWN, dual_role_force_sink, HOOK_PRIO_DEFAULT); #endif /* CONFIG_USB_PD_DUAL_ROLE */ #ifdef CONFIG_COMMON_RUNTIME + +/* + * (enable=1) request pd_task transition to the suspended state. hang + * around for a while until we observe the state change. this can + * take a while (like 300ms) on startup when pd_task is sleeping in + * tcpci_tcpm_init. + * + * (enable=0) force pd_task out of the suspended state and into the + * port's default state. + */ + void pd_set_suspend(int port, int enable) { - int tries = 3; + int tries = 300; - do { - set_state(port, enable ? PD_STATE_SUSPENDED : - PD_DEFAULT_STATE(port)); + if (enable) { + pd[port].req_suspend_state = 1; + do { + task_wake(PD_PORT_TO_TASK_ID(port)); + if (pd[port].task_state == PD_STATE_SUSPENDED) + break; + msleep(1); + } while (--tries != 0); + if (!tries) + CPRINTS("TCPC p%d set_suspend failed!", port); + } else { + if (pd[port].task_state != PD_STATE_SUSPENDED) + CPRINTS("TCPC p%d suspend disable request " + "while not suspended!", port); + set_state(port, PD_DEFAULT_STATE(port)); task_wake(PD_PORT_TO_TASK_ID(port)); - } while (enable && pd[port].task_state != PD_STATE_SUSPENDED - && --tries); - - if (!tries) - CPRINTS("TCPC p%d set_suspend failed!", port); + } } #if defined(CONFIG_CMD_PD) && defined(CONFIG_CMD_PD_FLASH)