[PATCH net 0/2] net: ipa: non-setup bug fixes

[PATCH net 0/2] net: ipa: non-setup bug fixes

[Alex Elder]

This series fixes two bugs that occur if the IPA driver has not
completed its setup phase of initialization when an event occurs.

One event is the crash of the modem.  If the modem crashes,
ipa_modem_crashed() is called, and it performs various activities
intended to put the IPA hardware into a good state before the modem
reboots.  But if the IPA setup has not completed, resources used
for this cleanup will not be properly initialized.  So we must
skip doing this activity if we have not completed setup.

Similarly, if a system suspend is initiated but IPA setup has not
completed, the processing done in ipa_endpoint_suspend() should be
avoided.  And a subsequent system resume should also avoid resuming
endpoints that have not been initialized by IPA setup.

					-Alex

Alex Elder (2):
  net: ipa: only clear hardware state if setup has completed
  net: ipa: skip suspend/resume activities if not set up

 drivers/net/ipa/ipa_endpoint.c | 6 ++++++
 drivers/net/ipa/ipa_modem.c    | 3 +++
 2 files changed, 9 insertions(+)

-- 
2.20.1

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

[Alex Elder]

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(+)

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);
-- 
2.20.1

[PATCH net 2/2] net: ipa: skip suspend/resume activities if not set up

[Alex Elder]

When processing a system suspend request we suspend modem endpoints
if they are enabled, and call ipa_cmd_tag_process() (which issues
IPA commands) to ensure the IPA pipeline is cleared.  It is an error
to attempt to issue an IPA command before setup is complete, so this
is clearly a bug.  But we also shouldn't suspend or resume any
endpoints that have not been set up.

Have ipa_endpoint_suspend() and ipa_endpoint_resume() immediately
return if setup hasn't completed, to avoid any attempt to configure
endpoints or issue IPA commands in that case.

Fixes: 84f9bd12d46d ("soc: qcom: ipa: IPA endpoints")
Tested-by: Matthias Kaehlcke <mka@chromium.org>
Signed-off-by: Alex Elder <elder@linaro.org>
---
 drivers/net/ipa/ipa_endpoint.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/ipa/ipa_endpoint.c b/drivers/net/ipa/ipa_endpoint.c
index b7efd7c95e9c8..ed60fa5bcdaca 100644
--- a/drivers/net/ipa/ipa_endpoint.c
+++ b/drivers/net/ipa/ipa_endpoint.c
@@ -1471,6 +1471,9 @@ void ipa_endpoint_resume_one(struct ipa_endpoint *endpoint)
 
 void ipa_endpoint_suspend(struct ipa *ipa)
 {
+	if (!ipa->setup_complete)
+		return;
+
 	if (ipa->modem_netdev)
 		ipa_modem_suspend(ipa->modem_netdev);
 
@@ -1482,6 +1485,9 @@ void ipa_endpoint_suspend(struct ipa *ipa)
 
 void ipa_endpoint_resume(struct ipa *ipa)
 {
+	if (!ipa->setup_complete)
+		return;
+
 	ipa_endpoint_resume_one(ipa->name_map[IPA_ENDPOINT_AP_COMMAND_TX]);
 	ipa_endpoint_resume_one(ipa->name_map[IPA_ENDPOINT_AP_LAN_RX]);
 
-- 
2.20.1

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

[Jakub Kicinski]

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-

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

[Alex Elder]

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-
> 

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

[Jakub Kicinski]

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!