| Message ID | 20210804153626.1549001-3-elder@linaro.org (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: ipa: more work toward runtime PM | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | success | CCed 4 of 4 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 32 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
On Wed, 4 Aug 2021 10:36:22 -0500 Alex Elder wrote: > Assign the ipa->modem_netdev and endpoint->netdev pointers *before* > registering the network device. As soon as the device is > registered it can be opened, and by that time we'll want those > pointers valid. > > Similarly, don't make those pointers NULL until *after* the modem > network device is unregistered in ipa_modem_stop(). > > Signed-off-by: Alex Elder <elder@linaro.org> This one seems like a pretty legit race, net would be better if you don't mind.
On Thu, 5 Aug 2021 18:27:12 -0700 Jakub Kicinski wrote: > On Wed, 4 Aug 2021 10:36:22 -0500 Alex Elder wrote: > > Assign the ipa->modem_netdev and endpoint->netdev pointers *before* > > registering the network device. As soon as the device is > > registered it can be opened, and by that time we'll want those > > pointers valid. > > > > Similarly, don't make those pointers NULL until *after* the modem > > network device is unregistered in ipa_modem_stop(). > > > > Signed-off-by: Alex Elder <elder@linaro.org> > > This one seems like a pretty legit race, net would be better if you > don't mind. Ah, this set was already applied, don't mind me :)
On 8/5/21 8:27 PM, Jakub Kicinski wrote: > On Wed, 4 Aug 2021 10:36:22 -0500 Alex Elder wrote: >> Assign the ipa->modem_netdev and endpoint->netdev pointers *before* >> registering the network device. As soon as the device is >> registered it can be opened, and by that time we'll want those >> pointers valid. >> >> Similarly, don't make those pointers NULL until *after* the modem >> network device is unregistered in ipa_modem_stop(). >> >> Signed-off-by: Alex Elder <elder@linaro.org> > > This one seems like a pretty legit race, net would be better if you > don't mind. I don't mind at all. But now that it's accepted, I'm not sure how to go about getting it back-ported. Mainly I don't want to interfere with any interaction between net/master and net-next/master... Maybe you're in a better position to do that. And if so: Fixes: 57f63faf0562 ("net: ipa: only set endpoint netdev pointer when in use") I'll happily do it if you can tell me the best way how. Thanks. -Alex
On 8/5/21 8:41 PM, Jakub Kicinski wrote: > On Thu, 5 Aug 2021 18:27:12 -0700 Jakub Kicinski wrote: >> On Wed, 4 Aug 2021 10:36:22 -0500 Alex Elder wrote: >>> Assign the ipa->modem_netdev and endpoint->netdev pointers *before* >>> registering the network device. As soon as the device is >>> registered it can be opened, and by that time we'll want those >>> pointers valid. >>> >>> Similarly, don't make those pointers NULL until *after* the modem >>> network device is unregistered in ipa_modem_stop(). >>> >>> Signed-off-by: Alex Elder <elder@linaro.org> >> >> This one seems like a pretty legit race, net would be better if you >> don't mind. > > Ah, this set was already applied, don't mind me :) I really appreciate your review and feedback. Applied or not, it's valuable to me. -Alex
diff --git a/drivers/net/ipa/ipa_modem.c b/drivers/net/ipa/ipa_modem.c index 663a610979e70..ad4019e8016ec 100644 --- a/drivers/net/ipa/ipa_modem.c +++ b/drivers/net/ipa/ipa_modem.c @@ -231,13 +231,15 @@ int ipa_modem_start(struct ipa *ipa) SET_NETDEV_DEV(netdev, &ipa->pdev->dev); priv = netdev_priv(netdev); priv->ipa = ipa; + ipa->name_map[IPA_ENDPOINT_AP_MODEM_TX]->netdev = netdev; + ipa->name_map[IPA_ENDPOINT_AP_MODEM_RX]->netdev = netdev; + ipa->modem_netdev = netdev; ret = register_netdev(netdev); - if (!ret) { - ipa->modem_netdev = netdev; - ipa->name_map[IPA_ENDPOINT_AP_MODEM_TX]->netdev = netdev; - ipa->name_map[IPA_ENDPOINT_AP_MODEM_RX]->netdev = netdev; - } else { + if (ret) { + ipa->modem_netdev = NULL; + ipa->name_map[IPA_ENDPOINT_AP_MODEM_RX]->netdev = NULL; + ipa->name_map[IPA_ENDPOINT_AP_MODEM_TX]->netdev = NULL; free_netdev(netdev); } @@ -276,10 +278,10 @@ int ipa_modem_stop(struct ipa *ipa) /* If it was opened, stop it first */ if (netdev->flags & IFF_UP) (void)ipa_stop(netdev); - ipa->name_map[IPA_ENDPOINT_AP_MODEM_RX]->netdev = NULL; - ipa->name_map[IPA_ENDPOINT_AP_MODEM_TX]->netdev = NULL; - ipa->modem_netdev = NULL; unregister_netdev(netdev); + ipa->modem_netdev = NULL; + ipa->name_map[IPA_ENDPOINT_AP_MODEM_RX]->netdev = NULL; + ipa->name_map[IPA_ENDPOINT_AP_MODEM_TX]->netdev = NULL; free_netdev(netdev); }
Assign the ipa->modem_netdev and endpoint->netdev pointers *before* registering the network device. As soon as the device is registered it can be opened, and by that time we'll want those pointers valid. Similarly, don't make those pointers NULL until *after* the modem network device is unregistered in ipa_modem_stop(). Signed-off-by: Alex Elder <elder@linaro.org> --- drivers/net/ipa/ipa_modem.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-)