lkml.org 
[lkml]   [2023]   [Aug]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH 2/3] platform/x86: intel_scu_ipc: Check status upon timeout in ipc_wait_for_interrupt()
From


On 8/30/2023 6:14 PM, 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.
>
> Cc: Prashant Malani <pmalani@chromium.org>
> Cc: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> Fixes: ed12f295bfd5 ("ipc: Added support for IPC interrupt mode")
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---
> drivers/platform/x86/intel_scu_ipc.c | 15 +++++++++------
> 1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/platform/x86/intel_scu_ipc.c b/drivers/platform/x86/intel_scu_ipc.c
> index 5a37becc65aa..2a21153e3bf3 100644
> --- a/drivers/platform/x86/intel_scu_ipc.c
> +++ b/drivers/platform/x86/intel_scu_ipc.c
> @@ -246,16 +246,19 @@ static inline int busy_loop(struct intel_scu_ipc_dev *scu)
> /* Wait till ipc ioc interrupt is received or timeout in 10 HZ */
> static inline int ipc_wait_for_interrupt(struct intel_scu_ipc_dev *scu)
> {
> - int status;
> + unsigned long time_left;
> + u8 status;
> + int err = 0;
>
> - if (!wait_for_completion_timeout(&scu->cmd_complete, IPC_TIMEOUT))
> - return -ETIMEDOUT;
> + time_left = wait_for_completion_timeout(&scu->cmd_complete, IPC_TIMEOUT);
> + if (!time_left)
> + err = -ETIMEDOUT;

Since you are not using the return value, I would not use time_left. I think the
following version is easy to read. But it is up to you.

wait_for_completion_timeout(&scu->cmd_complete, IPC_TIMEOUT)

status = ipc_read_status(scu);

if (status & IPC_STATUS_BUSY)
return -ETIMEDOUT;

if (status & IPC_STATUS_ERR)
return -EIO;

return 0;

With above fixed, you can add

Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>


>
> 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;
> }
>
> static int intel_scu_ipc_check_status(struct intel_scu_ipc_dev *scu)

--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

\
 
 \ /
  Last update: 2023-08-31 16:30    [W:0.148 / U:0.120 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site