* [PATCH net 0/2] net: ipa: non-setup bug fixes
@ 2020-10-06 21:30 Alex Elder
2020-10-06 21:30 ` [PATCH net 1/2] net: ipa: only clear hardware state if setup has completed Alex Elder
2020-10-06 21:30 ` [PATCH net 2/2] net: ipa: skip suspend/resume activities if not set up Alex Elder
0 siblings, 2 replies; 6+ messages in thread
From: Alex Elder @ 2020-10-06 21:30 UTC (permalink / raw)
To: davem, kuba
Cc: evgreen, subashab, cpratapa, bjorn.andersson, mka, netdev, linux-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH net 1/2] net: ipa: only clear hardware state if setup has completed
2020-10-06 21:30 [PATCH net 0/2] net: ipa: non-setup bug fixes Alex Elder
@ 2020-10-06 21:30 ` Alex Elder
2020-10-09 19:25 ` Jakub Kicinski
2020-10-06 21:30 ` [PATCH net 2/2] net: ipa: skip suspend/resume activities if not set up Alex Elder
1 sibling, 1 reply; 6+ messages in thread
From: Alex Elder @ 2020-10-06 21:30 UTC (permalink / raw)
To: davem, kuba
Cc: evgreen, subashab, cpratapa, bjorn.andersson, mka, netdev, linux-kernel
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH net 1/2] net: ipa: only clear hardware state if setup has completed
2020-10-06 21:30 ` [PATCH net 1/2] net: ipa: only clear hardware state if setup has completed Alex Elder
@ 2020-10-09 19:25 ` Jakub Kicinski
2020-10-09 19:39 ` Alex Elder
0 siblings, 1 reply; 6+ messages in thread
From: Jakub Kicinski @ 2020-10-09 19:25 UTC (permalink / raw)
To: Alex Elder
Cc: davem, evgreen, subashab, cpratapa, bjorn.andersson, mka, netdev,
linux-kernel
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-
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH net 1/2] net: ipa: only clear hardware state if setup has completed
2020-10-09 19:25 ` Jakub Kicinski
@ 2020-10-09 19:39 ` Alex Elder
2020-10-09 19:41 ` Jakub Kicinski
0 siblings, 1 reply; 6+ messages in thread
From: Alex Elder @ 2020-10-09 19:39 UTC (permalink / raw)
To: Jakub Kicinski
Cc: davem, evgreen, subashab, cpratapa, bjorn.andersson, mka, netdev,
linux-kernel
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-
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH net 1/2] net: ipa: only clear hardware state if setup has completed
2020-10-09 19:39 ` Alex Elder
@ 2020-10-09 19:41 ` Jakub Kicinski
0 siblings, 0 replies; 6+ messages in thread
From: Jakub Kicinski @ 2020-10-09 19:41 UTC (permalink / raw)
To: Alex Elder
Cc: davem, evgreen, subashab, cpratapa, bjorn.andersson, mka, netdev,
linux-kernel
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!
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH net 2/2] net: ipa: skip suspend/resume activities if not set up
2020-10-06 21:30 [PATCH net 0/2] net: ipa: non-setup bug fixes Alex Elder
2020-10-06 21:30 ` [PATCH net 1/2] net: ipa: only clear hardware state if setup has completed Alex Elder
@ 2020-10-06 21:30 ` Alex Elder
1 sibling, 0 replies; 6+ messages in thread
From: Alex Elder @ 2020-10-06 21:30 UTC (permalink / raw)
To: davem, kuba
Cc: evgreen, subashab, cpratapa, bjorn.andersson, mka, netdev, linux-kernel
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-10-09 19:41 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-06 21:30 [PATCH net 0/2] net: ipa: non-setup bug fixes Alex Elder
2020-10-06 21:30 ` [PATCH net 1/2] net: ipa: only clear hardware state if setup has completed Alex Elder
2020-10-09 19:25 ` Jakub Kicinski
2020-10-09 19:39 ` Alex Elder
2020-10-09 19:41 ` Jakub Kicinski
2020-10-06 21:30 ` [PATCH net 2/2] net: ipa: skip suspend/resume activities if not set up Alex Elder
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).