linux-kernel - RE: [PATCH] platform/x86: intel_scu_ipc: Keep polling IPC status if it reads IPC_STATUS

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]

Message-ID: <CO1PR11MB483572AC606C04CBD061AC0896289@CO1PR11MB4835.namprd11.prod.outlook.com>
Date:   Thu, 3 Feb 2022 14:10:06 +0000
From:   "Khandelwal, Rajat" <rajat.khandelwal@...el.com>
To:     Hans de Goede <hdegoede@...hat.com>,
        "mika.westerberg@...ux.intel.com" <mika.westerberg@...ux.intel.com>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "platform-driver-x86@...r.kernel.org" 
        <platform-driver-x86@...r.kernel.org>,
        "Westerberg, Mika" <mika.westerberg@...el.com>
Subject: RE: [PATCH] platform/x86: intel_scu_ipc: Keep polling IPC status if
 it reads IPC_STATUS_ERR

Hi @Hans de Goede
Yes, when the ERR bit is set, it still polls until timeout expires. 
Also, IPC doesn’t clear the bit after msec as I have manually tested it and verified.
But also, in the current implementation, it returns ETIMEDOUT even when the status reflects BUSY. This leads to developers thinking that the communication didn’t go through because timeout occurred not because the SCU was busy. We had to write manual debug prints to differentiate between these two. 

Linux 5.17 has also come. Can you drive a closure to this patch?

Thanks
Rajat

-----Original Message-----
From: Hans de Goede <hdegoede@...hat.com> 
Sent: Monday, January 17, 2022 3:19 PM
To: Khandelwal, Rajat <rajat.khandelwal@...el.com>; mika.westerberg@...ux.intel.com
Cc: linux-kernel@...r.kernel.org; platform-driver-x86@...r.kernel.org; Westerberg, Mika <mika.westerberg@...el.com>
Subject: Re: [PATCH] platform/x86: intel_scu_ipc: Keep polling IPC status if it reads IPC_STATUS_ERR

Hi,

On 12/30/21 09:30, Khandelwal, Rajat wrote:
> Hi Mika
> I hope this annotation is correct? Sorry for the multiple errors! 
> 
> -----Original Message-----
> From: Khandelwal, Rajat <rajat.khandelwal@...el.com>
> Sent: Thursday, December 30, 2021 1:54 PM
> To: mika.westerberg@...ux.intel.com
> Cc: linux-kernel@...r.kernel.org; platform-driver-x86@...r.kernel.org; 
> Khandelwal, Rajat <rajat.khandelwal@...el.com>; Westerberg, Mika 
> <mika.westerberg@...el.com>
> Subject: [PATCH] platform/x86: intel_scu_ipc: Keep polling IPC status 
> if it reads IPC_STATUS_ERR
> 
> The current implementation returns -EIO if and when IPC_STATUS_ERR is returned and returns -ETIMEDOUT even if the status is busy.
> This patch polls the IPC status even if IPC_STATUS_ERR is returned until timeout expires and returns -EBUSY if the status shows busy.
> Observed in multiple scenarios, trying to fetch the status of IPC after it shows ERR sometimes eradicates the ERR status.

So what this is doing is continue to poll, even though the SCU says it is ready, when the ERR bit is set ?

Are we sure the IPC does not just simply clear the err bit after some time becuse it expects it to be "consumed" within X msec after dropping busy low?

IOW what guarantees are there that this new behavior of ipc_data_readl() is not actually causing us to ignore actual errors ?

Regards,

Hans





> 
> Signed-off-by: Rajat-Khandelwal <rajat.khandelwal@...el.com>
> ---
>  drivers/platform/x86/intel_scu_ipc.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel_scu_ipc.c 
> b/drivers/platform/x86/intel_scu_ipc.c
> index 7cc9089d1e14..1f90acaa7212 100644
> --- a/drivers/platform/x86/intel_scu_ipc.c
> +++ b/drivers/platform/x86/intel_scu_ipc.c
> @@ -233,17 +233,23 @@ static inline u32 ipc_data_readl(struct intel_scu_ipc_dev *scu, u32 offset)  static inline int busy_loop(struct intel_scu_ipc_dev *scu)  {
>  	unsigned long end = jiffies + IPC_TIMEOUT;
> +	u32 status;
>  
>  	do {
> -		u32 status;
> -
>  		status = ipc_read_status(scu);
> -		if (!(status & IPC_STATUS_BUSY))
> -			return (status & IPC_STATUS_ERR) ? -EIO : 0;
> +		if (!(status & IPC_STATUS_BUSY)) {
> +			if (!(status & IPC_STATUS_ERR))
> +				return 0;
> +		}
>  
>  		usleep_range(50, 100);
>  	} while (time_before(jiffies, end));
>  
> +	if (status & IPC_STATUS_BUSY)
> +		return -EBUSY;
> +	if (status & IPC_STATUS_ERR)
> +		return -EIO;
> +
>  	return -ETIMEDOUT;
>  }
>  
> --
> 2.17.1
>