diff mbox

[2/2] qtnfmac: abort scans on wireless interface changes

Message ID 20170918080446.21763-3-sergey.matyukevich.os@quantenna.com
State Superseded
Headers show

Commit Message

Sergey Matyukevich Sept. 18, 2017, 8:04 a.m. UTC
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(-)

Comments

Johannes Berg Sept. 18, 2017, 8:41 a.m. UTC | #1
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
Sergey Matyukevich Sept. 18, 2017, 9:57 a.m. UTC | #2
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
Johannes Berg Sept. 18, 2017, 10:03 a.m. UTC | #3
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
Sergey Matyukevich Sept. 18, 2017, 10:16 a.m. UTC | #4
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
Johannes Berg Sept. 18, 2017, 10:20 a.m. UTC | #5
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
Sergey Matyukevich Sept. 18, 2017, 10:31 a.m. UTC | #6
> > > > > > -     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
Kalle Valo Sept. 18, 2017, 12:51 p.m. UTC | #7
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 mbox

Patch

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;