| Message ID | 20200805091141.2.If847565ed7568448c67804f3735d5f8a61eda560@changeid (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] soc: qcom: aoss: Don't wait for IRQ if we might be in suspend/resume noirq | expand |
Hi, On Wed, Aug 5, 2020 at 10:28 AM Stephen Boyd <swboyd@chromium.org> wrote: > > Quoting Douglas Anderson (2020-08-05 09:16:11) > > The function qmp_send() called wait_event_interruptible_timeout() to > > wait for its interrupt. However, this function did not check for > > -ERESTARTSYS and assumed that any non-zero return value meant that the > > event happened. > > > > While we could try to figure out how to handle interruptions by > > figuring out how to cancel and/or undo our transfer in a race-free way > > and then communicating this status back to all of our callers, that > > seems like a whole lot of complexity. As I understand it the transfer > > should happen rather quickly and if we're really hitting the 1 second > > timeout we're in deep doggy doodoo anyway. Let's just use the > > non-interruptible version of the function and call it good enough. > > > > Found by code inspection. No known test cases expose the problem > > described here. > > > > Fixes: 2209481409b7 ("soc: qcom: Add AOSS QMP driver") > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > > --- > > I would put this first in the series as it's an obvious bug fix. I guess I thought of it the other way. This is a less serious problem (no known way to tickle it) and so deserved to be 2nd. I'm happy to flip it as needed, though. It would also be trivially easy to flip when applying. > Reviewed-by: Stephen Boyd <swboyd@chromium.org> Thanks! -Doug
diff --git a/drivers/soc/qcom/qcom_aoss.c b/drivers/soc/qcom/qcom_aoss.c index 818cdf74a267..897f9f1c33ba 100644 --- a/drivers/soc/qcom/qcom_aoss.c +++ b/drivers/soc/qcom/qcom_aoss.c @@ -257,8 +257,8 @@ static int qmp_send(struct qmp *qmp, const void *data, size_t len, bool noirq) time_left = readx_poll_timeout_atomic(qmp_message_empty, qmp, is_empty, is_empty, 1U, 1000000U); else - time_left = wait_event_interruptible_timeout(qmp->event, - qmp_message_empty(qmp), HZ); + time_left = wait_event_timeout(qmp->event, qmp_message_empty(qmp), HZ); + if (!time_left) { dev_err(qmp->dev, "ucore did not ack channel\n"); ret = -ETIMEDOUT;
The function qmp_send() called wait_event_interruptible_timeout() to wait for its interrupt. However, this function did not check for -ERESTARTSYS and assumed that any non-zero return value meant that the event happened. While we could try to figure out how to handle interruptions by figuring out how to cancel and/or undo our transfer in a race-free way and then communicating this status back to all of our callers, that seems like a whole lot of complexity. As I understand it the transfer should happen rather quickly and if we're really hitting the 1 second timeout we're in deep doggy doodoo anyway. Let's just use the non-interruptible version of the function and call it good enough. Found by code inspection. No known test cases expose the problem described here. Fixes: 2209481409b7 ("soc: qcom: Add AOSS QMP driver") Signed-off-by: Douglas Anderson <dianders@chromium.org> --- drivers/soc/qcom/qcom_aoss.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)