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

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

[Khandelwal, Rajat]

Hi Mika
I hope this annotation is correct? Sorry for the multiple errors! 

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

Signed-off-by: Rajat-Khandelwal <rajat.khandelwal@intel.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

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

[Khandelwal, Rajat]

Adding ++Heikki

Thanks
Rajat
-----Original Message-----
From: Khandelwal, Rajat <rajat.khandelwal@intel.com> 
Sent: Thursday, December 30, 2021 1:54 PM
To: mika.westerberg@linux.intel.com
Cc: linux-kernel@vger.kernel.org; platform-driver-x86@vger.kernel.org; Khandelwal, Rajat <rajat.khandelwal@intel.com>; Westerberg, Mika <mika.westerberg@intel.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.

Signed-off-by: Rajat-Khandelwal <rajat.khandelwal@intel.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

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

[Hans de Goede]

Hi Rajat,

On 12/31/21 16:49, Khandelwal, Rajat wrote:
> Adding ++Heikki
> 
> Thanks
> Rajat

If you want me to apply this, please send this as a proper patch, not as an inline
forward to me, with the platform-driver-x86@vger.kernel.org list in the Cc.

Regards,

Hans


> -----Original Message-----
> From: Khandelwal, Rajat <rajat.khandelwal@intel.com> 
> Sent: Thursday, December 30, 2021 1:54 PM
> To: mika.westerberg@linux.intel.com
> Cc: linux-kernel@vger.kernel.org; platform-driver-x86@vger.kernel.org; Khandelwal, Rajat <rajat.khandelwal@intel.com>; Westerberg, Mika <mika.westerberg@intel.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.
> 
> Signed-off-by: Rajat-Khandelwal <rajat.khandelwal@intel.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
> 

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

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

[Khandelwal, Rajat]

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@redhat.com> 
Sent: Monday, January 17, 2022 3:19 PM
To: Khandelwal, Rajat <rajat.khandelwal@intel.com>; mika.westerberg@linux.intel.com
Cc: linux-kernel@vger.kernel.org; platform-driver-x86@vger.kernel.org; Westerberg, Mika <mika.westerberg@intel.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@intel.com>
> Sent: Thursday, December 30, 2021 1:54 PM
> To: mika.westerberg@linux.intel.com
> Cc: linux-kernel@vger.kernel.org; platform-driver-x86@vger.kernel.org; 
> Khandelwal, Rajat <rajat.khandelwal@intel.com>; Westerberg, Mika 
> <mika.westerberg@intel.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@intel.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
> 

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

[Hans de Goede]

Hi Rajat,

On 2/3/22 15:10, Khandelwal, Rajat wrote:
> 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?

I'm sorry, but in its current state the patch is no good at all:

1. You never properly submitted it, as Mika already responded here:
https://lore.kernel.org/platform-driver-x86/CO1PR11MB4835D217B78F17BA6AD79F0096449@CO1PR11MB4835.namprd11.prod.outlook.com/T/

"Please run checkpatch.pl and fix all the issues it reports, and change
the $subject line to match the style in that file and then re-send."

Still I decided to try and merge it manually from your "original message"
(which I nor the archive ever received) quote:

"""
--- a/drivers/platform/x86/intel_scu_ipc.c
+++ b/drivers/platform/x86/intel_scu_ipc.c
@@ -233,17 +233,19 @@ 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_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;
 }
 
"""

But this actually is a corrupt unified diff. Count the number of + and - lines,
there are 17 - lines, as indicated in the chunk header, but there are 23 +
lines, where as the chunk header: "@@ -233,17 +233,19" @@ claims there are only
19. So this smells like a manually edited diff and one which was not even
test applied after editing let alone it actually being compiled
and functionally tested.

I generally dislike it when other kernel subsystem maintainers use
strong language, but I have to say that the above
(never applied as submitted, thus never tested as submitted)
is utter garbage.

Please:

1. Create a new patch which actually applies and *thoroughly* test that
before submitting the new version

2. Directly submit it to me / the mailinglist the proper way as documented:
https://www.kernel.org/doc/html/latest/process/submitting-patches.html

3. Don't you ever ever again send an untested patch for upstream
inclusion!!

Regards,

Hans




> -----Original Message-----
> From: Hans de Goede <hdegoede@redhat.com> 
> Sent: Monday, January 17, 2022 3:19 PM
> To: Khandelwal, Rajat <rajat.khandelwal@intel.com>; mika.westerberg@linux.intel.com
> Cc: linux-kernel@vger.kernel.org; platform-driver-x86@vger.kernel.org; Westerberg, Mika <mika.westerberg@intel.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@intel.com>
>> Sent: Thursday, December 30, 2021 1:54 PM
>> To: mika.westerberg@linux.intel.com
>> Cc: linux-kernel@vger.kernel.org; platform-driver-x86@vger.kernel.org; 
>> Khandelwal, Rajat <rajat.khandelwal@intel.com>; Westerberg, Mika 
>> <mika.westerberg@intel.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@intel.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
>>
>