[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZPCcevAt4CwCADe2@smile.fi.intel.com>
Date: Thu, 31 Aug 2023 16:58:18 +0300
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Stephen Boyd <swboyd@...omium.org>
Cc: Mika Westerberg <mika.westerberg@...ux.intel.com>,
Hans de Goede <hdegoede@...hat.com>,
Mark Gross <markgross@...nel.org>,
linux-kernel@...r.kernel.org, patches@...ts.linux.dev,
platform-driver-x86@...r.kernel.org,
Kuppuswamy Sathyanarayanan
<sathyanarayanan.kuppuswamy@...ux.intel.com>,
Prashant Malani <pmalani@...omium.org>
Subject: Re: [PATCH 2/3] platform/x86: intel_scu_ipc: Check status upon
timeout in ipc_wait_for_interrupt()
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@...ux.intel.com>
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
Powered by blists - more mailing lists