| Message ID | 20170918080446.21763-3-sergey.matyukevich.os@quantenna.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Mon, 2017-09-18 at 11:04 +0300, Sergey Matyukevich wrote: > > */ > static int qtnf_netdev_close(struct net_device *ndev) > { > - netif_carrier_off(ndev); > qtnf_virtual_intf_cleanup(ndev); > qtnf_netdev_updown(ndev, 0); > + netif_carrier_off(ndev); > return 0; > } This seems unrelated? > - if (timer_pending(&mac->scan_timeout)) > - del_timer_sync(&mac->scan_timeout); > qtnf_scan_done(mac, le32_to_cpu(status->flags) & > QLINK_SCAN_ABORTED); and that's related perhaps but not really explained in the changelog, not sure? johannes
On Mon, Sep 18, 2017 at 10:41:08AM +0200, Johannes Berg wrote: Hello Johannes, > On Mon, 2017-09-18 at 11:04 +0300, Sergey Matyukevich wrote: > > > > */ > > static int qtnf_netdev_close(struct net_device *ndev) > > { > > - netif_carrier_off(ndev); > > qtnf_virtual_intf_cleanup(ndev); > > qtnf_netdev_updown(ndev, 0); > > + netif_carrier_off(ndev); > > return 0; > > } > > This seems unrelated? Hmm... The idea was to make sure that scan is canceled before cfg80211_netdev_notifier_call throws WARN when state is changed to NETDEV_DOWN. However this is not needed if scans are properly canceled in cfg80211_ops handlers. Thanks for catching, will remove. > > - if (timer_pending(&mac->scan_timeout)) > > - del_timer_sync(&mac->scan_timeout); > > qtnf_scan_done(mac, le32_to_cpu(status->flags) & > > QLINK_SCAN_ABORTED); > > and that's related perhaps but not really explained in the changelog, > not sure? That was minor optimization: to remove pedning timer whenever scan is canceled. Sure, it worth mentioning in changelog, will do. By the way, is it ok to send corrected single patch in reply to this discussion ? Or the appropriate way is to resend the whole patch set ? Regards, Sergey
Hi, > > > static int qtnf_netdev_close(struct net_device *ndev) > > > { > > > - netif_carrier_off(ndev); > > > qtnf_virtual_intf_cleanup(ndev); > > > qtnf_netdev_updown(ndev, 0); > > > + netif_carrier_off(ndev); > > > return 0; > > > } > > > > This seems unrelated? > > Hmm... The idea was to make sure that scan is canceled before > cfg80211_netdev_notifier_call throws WARN when state is changed > to NETDEV_DOWN. However this is not needed if scans are properly > canceled in cfg80211_ops handlers. Thanks for catching, will remove. Not sure how the carrier change is relevant though - that doesn't even notify cfg80211? Anyway, I don't really know, it just didn't seem related :) > > > - if (timer_pending(&mac->scan_timeout)) > > > - del_timer_sync(&mac->scan_timeout); > > > qtnf_scan_done(mac, le32_to_cpu(status->flags) & > > > QLINK_SCAN_ABORTED); > > > > and that's related perhaps but not really explained in the > > changelog, > > not sure? > > That was minor optimization: to remove pedning timer whenever scan > is canceled. Sure, it worth mentioning in changelog, will do. I realized (after sending my email) that it actually made a lot of sense, so yeah - and in fact I'm not sure it's just an optimisation, depends on where else you cancel the timer and if that could leave the timer running until after the interface is removed or something. Btw, I don't really see why you check pending first? > By the way, is it ok to send corrected single patch in reply to this > discussion ? Or the appropriate way is to resend the whole patch set > ? Good question. I think it's up to the specific maintainer, I know davem wants a full resend, but Kalle might have his own preference. I for one am willing to deal with both, though a full resend is somewhat easier. johannes
On Mon, Sep 18, 2017 at 12:03:07PM +0200, Johannes Berg wrote: > > > > - if (timer_pending(&mac->scan_timeout)) > > > > - del_timer_sync(&mac->scan_timeout); > > > > qtnf_scan_done(mac, le32_to_cpu(status->flags) & > > > > QLINK_SCAN_ABORTED); > > > > > > and that's related perhaps but not really explained in the > > > changelog, > > > not sure? > > > > That was minor optimization: to remove pedning timer whenever scan > > is canceled. Sure, it worth mentioning in changelog, will do. > > I realized (after sending my email) that it actually made a lot of > sense, so yeah - and in fact I'm not sure it's just an optimisation, > depends on where else you cancel the timer and if that could leave the > timer running until after the interface is removed or something. > > Btw, I don't really see why you check pending first? I don't quite understand the question. Do you mean it makes sense to call cfg80211_scan_done first and then cancel timer ? > > By the way, is it ok to send corrected single patch in reply to this > > discussion ? Or the appropriate way is to resend the whole patch set > > ? > > Good question. I think it's up to the specific maintainer, I know davem > wants a full resend, but Kalle might have his own preference. I for one > am willing to deal with both, though a full resend is somewhat easier. Ok, understood. Regards, Sergey
On Mon, 2017-09-18 at 13:16 +0300, Sergey Matyukevich wrote: > > > > > > - if (timer_pending(&mac->scan_timeout)) > > > > > - del_timer_sync(&mac->scan_timeout); > I don't quite understand the question. Do you mean it makes sense to > call cfg80211_scan_done first and then cancel timer ? No, I'm just not sure why you call timer_pending() first. johannes
> > > > > > - if (timer_pending(&mac->scan_timeout)) > > > > > > - del_timer_sync(&mac->scan_timeout); > > > I don't quite understand the question. Do you mean it makes sense to > > call cfg80211_scan_done first and then cancel timer ? > > No, I'm just not sure why you call timer_pending() first. Well, it looks like before this change it was redundant. But now it makes sense to avoid deadlock when this function is called from the timer itself. Regards, Sergey
Sergey Matyukevich <sergey.matyukevich.os@quantenna.com> writes: > By the way, is it ok to send corrected single patch in reply to this > discussion ? Or the appropriate way is to resend the whole patch set ? This has been a common question so I documented it: https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches#resubmit_the_whole_patchset I leave it as an exercise to the reader to find the answer, even though the URL even spoils it ;)
diff --git a/drivers/net/wireless/quantenna/qtnfmac/cfg80211.c b/drivers/net/wireless/quantenna/qtnfmac/cfg80211.c index 856fa6e8327e..a450bc6bc774 100644 --- a/drivers/net/wireless/quantenna/qtnfmac/cfg80211.c +++ b/drivers/net/wireless/quantenna/qtnfmac/cfg80211.c @@ -115,6 +115,8 @@ int qtnf_del_virtual_intf(struct wiphy *wiphy, struct wireless_dev *wdev) vif = qtnf_netdev_get_priv(wdev->netdev); + qtnf_scan_done(vif->mac, true); + if (qtnf_cmd_send_del_intf(vif)) pr_err("VIF%u.%u: failed to delete VIF\n", vif->mac->macid, vif->vifid); @@ -335,6 +337,8 @@ static int qtnf_stop_ap(struct wiphy *wiphy, struct net_device *dev) struct qtnf_vif *vif = qtnf_netdev_get_priv(dev); int ret; + qtnf_scan_done(vif->mac, true); + ret = qtnf_cmd_send_stop_ap(vif); if (ret) { pr_err("VIF%u.%u: failed to stop AP operation in FW\n", @@ -570,8 +574,6 @@ qtnf_del_station(struct wiphy *wiphy, struct net_device *dev, !qtnf_sta_list_lookup(&vif->sta_list, params->mac)) return 0; - qtnf_scan_done(vif->mac, true); - ret = qtnf_cmd_send_del_sta(vif, params); if (ret) pr_err("VIF%u.%u: failed to delete STA %pM\n", @@ -1134,8 +1136,9 @@ void qtnf_virtual_intf_cleanup(struct net_device *ndev) } vif->sta_state = QTNF_STA_DISCONNECTED; - qtnf_scan_done(mac, true); } + + qtnf_scan_done(mac, true); } void qtnf_cfg80211_vif_reset(struct qtnf_vif *vif) diff --git a/drivers/net/wireless/quantenna/qtnfmac/cfg80211.h b/drivers/net/wireless/quantenna/qtnfmac/cfg80211.h index 6a4af52522b8..66db26613b1f 100644 --- a/drivers/net/wireless/quantenna/qtnfmac/cfg80211.h +++ b/drivers/net/wireless/quantenna/qtnfmac/cfg80211.h @@ -34,6 +34,9 @@ static inline void qtnf_scan_done(struct qtnf_wmac *mac, bool aborted) .aborted = aborted, }; + if (timer_pending(&mac->scan_timeout)) + del_timer_sync(&mac->scan_timeout); + mutex_lock(&mac->mac_lock); if (mac->scan_req) { diff --git a/drivers/net/wireless/quantenna/qtnfmac/core.c b/drivers/net/wireless/quantenna/qtnfmac/core.c index 5e60180482d1..7f83bedc0b88 100644 --- a/drivers/net/wireless/quantenna/qtnfmac/core.c +++ b/drivers/net/wireless/quantenna/qtnfmac/core.c @@ -68,9 +68,9 @@ static int qtnf_netdev_open(struct net_device *ndev) */ static int qtnf_netdev_close(struct net_device *ndev) { - netif_carrier_off(ndev); qtnf_virtual_intf_cleanup(ndev); qtnf_netdev_updown(ndev, 0); + netif_carrier_off(ndev); return 0; } diff --git a/drivers/net/wireless/quantenna/qtnfmac/event.c b/drivers/net/wireless/quantenna/qtnfmac/event.c index 0fc2814eafad..43d2e7fd6e02 100644 --- a/drivers/net/wireless/quantenna/qtnfmac/event.c +++ b/drivers/net/wireless/quantenna/qtnfmac/event.c @@ -345,8 +345,6 @@ qtnf_event_handle_scan_complete(struct qtnf_wmac *mac, return -EINVAL; } - if (timer_pending(&mac->scan_timeout)) - del_timer_sync(&mac->scan_timeout); qtnf_scan_done(mac, le32_to_cpu(status->flags) & QLINK_SCAN_ABORTED); return 0;
Abort active scans when wireless interface configuration is changed. The usecases include wireless interface mode change, interface down, AP stop, virtual interface removal. Signed-off-by: Sergey Matyukevich <sergey.matyukevich.os@quantenna.com> --- drivers/net/wireless/quantenna/qtnfmac/cfg80211.c | 9 ++++++--- drivers/net/wireless/quantenna/qtnfmac/cfg80211.h | 3 +++ drivers/net/wireless/quantenna/qtnfmac/core.c | 2 +- drivers/net/wireless/quantenna/qtnfmac/event.c | 2 -- 4 files changed, 10 insertions(+), 6 deletions(-)