diff mbox series

[net,1/2] net: ipa: only clear hardware state if setup has completed

Message ID 20201006213047.31308-2-elder@linaro.org
State New
Headers show
Series net: ipa: non-setup bug fixes | expand

Commit Message

Alex Elder Oct. 6, 2020, 9:30 p.m. UTC
In the setup phase of initialization, GSI firmware gets loaded
and initialized, and the AP command TX and default RX endpoints
get set up.  Until that happens, IPA commands must not be issued
to the hardware.

If the modem crashes, we stop endpoint channels and perform a
number of other activities to clear modem-related IPA state,
including zeroing filter and routing tables (using IPA commands).
This is a bug if setup is not complete.  We should also not be
performing the other cleanup activity in that case either.

Fix this by returning immediately when handling a modem crash if we
haven't completed setup.

Fixes: a646d6ec9098 ("soc: qcom: ipa: modem and microcontroller")
Tested-by: Matthias Kaehlcke <mka@chromium.org>
Signed-off-by: Alex Elder <elder@linaro.org>
---
 drivers/net/ipa/ipa_modem.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Jakub Kicinski Oct. 9, 2020, 7:25 p.m. UTC | #1
On Tue,  6 Oct 2020 16:30:46 -0500 Alex Elder wrote:
> In the setup phase of initialization, GSI firmware gets loaded

> and initialized, and the AP command TX and default RX endpoints

> get set up.  Until that happens, IPA commands must not be issued

> to the hardware.

> 

> If the modem crashes, we stop endpoint channels and perform a

> number of other activities to clear modem-related IPA state,

> including zeroing filter and routing tables (using IPA commands).

> This is a bug if setup is not complete.  We should also not be

> performing the other cleanup activity in that case either.

> 

> Fix this by returning immediately when handling a modem crash if we

> haven't completed setup.

> 

> Fixes: a646d6ec9098 ("soc: qcom: ipa: modem and microcontroller")

> Tested-by: Matthias Kaehlcke <mka@chromium.org>

> Signed-off-by: Alex Elder <elder@linaro.org>


> diff --git a/drivers/net/ipa/ipa_modem.c b/drivers/net/ipa/ipa_modem.c

> index e34fe2d77324e..dd5b89c5cb2d4 100644

> --- a/drivers/net/ipa/ipa_modem.c

> +++ b/drivers/net/ipa/ipa_modem.c

> @@ -285,6 +285,9 @@ static void ipa_modem_crashed(struct ipa *ipa)

>  	struct device *dev = &ipa->pdev->dev;

>  	int ret;

>  

> +	if (!ipa->setup_complete)

> +		return;

> +

>  	ipa_endpoint_modem_pause_all(ipa, true);

>  

>  	ipa_endpoint_modem_hol_block_clear_all(ipa);


The only call site already checks setup_complete, so this is not needed,
no?

$ git grep -C2 ipa_modem_crashed\(
drivers/net/ipa/ipa_modem.c-
drivers/net/ipa/ipa_modem.c-/* Treat a "clean" modem stop the same as a crash */
drivers/net/ipa/ipa_modem.c:static void ipa_modem_crashed(struct ipa *ipa)
drivers/net/ipa/ipa_modem.c-{
drivers/net/ipa/ipa_modem.c-    struct device *dev = &ipa->pdev->dev;
--
drivers/net/ipa/ipa_modem.c-                     notify_data->crashed ? "crashed" : "stopping");
drivers/net/ipa/ipa_modem.c-            if (ipa->setup_complete)
drivers/net/ipa/ipa_modem.c:                    ipa_modem_crashed(ipa);
drivers/net/ipa/ipa_modem.c-            break;
drivers/net/ipa/ipa_modem.c-
Alex Elder Oct. 9, 2020, 7:39 p.m. UTC | #2
On 10/9/20 2:25 PM, Jakub Kicinski wrote:
> On Tue,  6 Oct 2020 16:30:46 -0500 Alex Elder wrote:

>> In the setup phase of initialization, GSI firmware gets loaded

>> and initialized, and the AP command TX and default RX endpoints

>> get set up.  Until that happens, IPA commands must not be issued

>> to the hardware.

>>

>> If the modem crashes, we stop endpoint channels and perform a

>> number of other activities to clear modem-related IPA state,

>> including zeroing filter and routing tables (using IPA commands).

>> This is a bug if setup is not complete.  We should also not be

>> performing the other cleanup activity in that case either.

>>

>> Fix this by returning immediately when handling a modem crash if we

>> haven't completed setup.

>>

>> Fixes: a646d6ec9098 ("soc: qcom: ipa: modem and microcontroller")

>> Tested-by: Matthias Kaehlcke <mka@chromium.org>

>> Signed-off-by: Alex Elder <elder@linaro.org>

> 

>> diff --git a/drivers/net/ipa/ipa_modem.c b/drivers/net/ipa/ipa_modem.c

>> index e34fe2d77324e..dd5b89c5cb2d4 100644

>> --- a/drivers/net/ipa/ipa_modem.c

>> +++ b/drivers/net/ipa/ipa_modem.c

>> @@ -285,6 +285,9 @@ static void ipa_modem_crashed(struct ipa *ipa)

>>   	struct device *dev = &ipa->pdev->dev;

>>   	int ret;

>>   

>> +	if (!ipa->setup_complete)

>> +		return;

>> +

>>   	ipa_endpoint_modem_pause_all(ipa, true);

>>   

>>   	ipa_endpoint_modem_hol_block_clear_all(ipa);

> 

> The only call site already checks setup_complete, so this is not needed,

> no?


Wow, you are correct.

I was mainly focused on the fact that the ipa_modem_crashed()
call could happen asynchronously, and that it called
ipa_table_reset() which requires IPA immediate commands.
I should have followed it back to its callers.

I agree with you, this is not needed.  The other patch
is definitely needed though.

Would you like me to re-post that single patch, or is
it acceptable as-is?

Thank you for reviewing this and catching my mistake.

					-Alex

> $ git grep -C2 ipa_modem_crashed\(

> drivers/net/ipa/ipa_modem.c-

> drivers/net/ipa/ipa_modem.c-/* Treat a "clean" modem stop the same as a crash */

> drivers/net/ipa/ipa_modem.c:static void ipa_modem_crashed(struct ipa *ipa)

> drivers/net/ipa/ipa_modem.c-{

> drivers/net/ipa/ipa_modem.c-    struct device *dev = &ipa->pdev->dev;

> --

> drivers/net/ipa/ipa_modem.c-                     notify_data->crashed ? "crashed" : "stopping");

> drivers/net/ipa/ipa_modem.c-            if (ipa->setup_complete)

> drivers/net/ipa/ipa_modem.c:                    ipa_modem_crashed(ipa);

> drivers/net/ipa/ipa_modem.c-            break;

> drivers/net/ipa/ipa_modem.c-

>
Jakub Kicinski Oct. 9, 2020, 7:41 p.m. UTC | #3
On Fri, 9 Oct 2020 14:39:45 -0500 Alex Elder wrote:
> > The only call site already checks setup_complete, so this is not needed,

> > no?  

> 

> Wow, you are correct.

> 

> I was mainly focused on the fact that the ipa_modem_crashed()

> call could happen asynchronously, and that it called

> ipa_table_reset() which requires IPA immediate commands.

> I should have followed it back to its callers.

> 

> I agree with you, this is not needed.  The other patch

> is definitely needed though.

> 

> Would you like me to re-post that single patch, or is

> it acceptable as-is?


Please repost, thank you!
diff mbox series

Patch

diff --git a/drivers/net/ipa/ipa_modem.c b/drivers/net/ipa/ipa_modem.c
index e34fe2d77324e..dd5b89c5cb2d4 100644
--- a/drivers/net/ipa/ipa_modem.c
+++ b/drivers/net/ipa/ipa_modem.c
@@ -285,6 +285,9 @@  static void ipa_modem_crashed(struct ipa *ipa)
 	struct device *dev = &ipa->pdev->dev;
 	int ret;
 
+	if (!ipa->setup_complete)
+		return;
+
 	ipa_endpoint_modem_pause_all(ipa, true);
 
 	ipa_endpoint_modem_hol_block_clear_all(ipa);