On Wed, Aug 30, 2023 at 06:14:02PM -0700, Stephen Boyd wrote: > It's possible for the completion in ipc_wait_for_interrupt() to timeout, > simply because the interrupt was delayed in being processed. A timeout > in itself is not an error. This driver should check the status register > upon a timeout to ensure that scheduling or interrupt processing delays > don't affect the outcome of the IPC return value. > > CPU0 SCU > ---- --- > ipc_wait_for_interrupt() > wait_for_completion_timeout(&scu->cmd_complete) > [TIMEOUT] status[IPC_BUSY]=0 > > Fix this problem by reading the status bit in all cases, regardless of > the timeout. If the completion times out, we'll assume the problem was > that the IPC_BUSY bit was still set, but if the status bit is cleared in > the meantime we know that we hit some scheduling delay and we should > just check the error bit. Makes sense, thanks for fixing this! Reviewed-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> Also see below. ... > /* Wait till ipc ioc interrupt is received or timeout in 10 HZ */ Not sure if this comment needs to be updated / amended. ... > status = ipc_read_status(scu); > - if (status & IPC_STATUS_ERR) > - return -EIO; > + if (!(status & IPC_STATUS_BUSY)) > + err = (status & IPC_STATUS_ERR) ? -EIO : 0; > > - return 0; > + return err; I would write it as: status = ipc_read_status(scu); if (status & IPC_STATUS_BUSY) return 0; if (status & IPC_STATUS_ERR) return -EIO; return 0; Also would be good, in case you are not doing it yet, to use --patience when formatting your patches. -- With Best Regards, Andy Shevchenko
