linux-kernel - Re: [PATCH 2/3] platform/x86: intel_scu_ipc: Check status upon timeout in ipc_wait_for

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 31 Aug 2023 07:27:35 -0700
From:   Kuppuswamy Sathyanarayanan 
        <sathyanarayanan.kuppuswamy@...ux.intel.com>
To:     Stephen Boyd <swboyd@...omium.org>,
        Mika Westerberg <mika.westerberg@...ux.intel.com>,
        Hans de Goede <hdegoede@...hat.com>,
        Mark Gross <markgross@...nel.org>
Cc:     linux-kernel@...r.kernel.org, patches@...ts.linux.dev,
        platform-driver-x86@...r.kernel.org,
        Andy Shevchenko <andriy.shevchenko@...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 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@...omium.org>
> Cc: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@...ux.intel.com>
> Fixes: ed12f295bfd5 ("ipc: Added support for IPC interrupt mode")
> Signed-off-by: Stephen Boyd <swboyd@...omium.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@...ux.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