| Message ID | 20201006213047.31308-2-elder@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | net: ipa: non-setup bug fixes | expand |
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-
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- >
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 --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);
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(+)