Re: [PATCH] platform/x86: intel_scu_ipc: Keep polling IPC status if it reads IPC_STATUS_ERR

[Hans de Goede]

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@xxxxxxxxx> 
> Sent: Thursday, December 30, 2021 1:54 PM
> To: mika.westerberg@xxxxxxxxxxxxxxx
> Cc: linux-kernel@xxxxxxxxxxxxxxx; platform-driver-x86@xxxxxxxxxxxxxxx; Khandelwal, Rajat <rajat.khandelwal@xxxxxxxxx>; Westerberg, Mika <mika.westerberg@xxxxxxxxx>
> 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@xxxxxxxxx>
> ---
>  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
>