Hello Kalle, Igor, and all
Here are two patches intended for 4.14. The first patch fixes tx path
regression. Lock should be held when queuing packets to h/w fifos in order to
properly handle configurations with two enabled interfaces or multiple enabled
mbss. The second patch fixes scan issues addressing a review comment
from K.Valo regarding pending active scans upon device removal.
Regards,
Sergey
Sergey Matyukevich (2):
qtnfmac: lock access to h/w in tx path
qtnfmac: abort scans on wireless interface changes
cfg80211.c | 9 ++++++---
cfg80211.h | 3 +++
core.c | 2 +-
event.c | 2 --
pearl/pcie.c | 9 ++++++++-
pearl/pcie_bus_priv.h | 2 ++
6 files changed, 20 insertions(+), 7 deletions(-)
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 <[email protected]>
---
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(-)
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;
--
2.11.0
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, 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 <[email protected]> writes:
> Here are two patches intended for 4.14. The first patch fixes tx path
> regression. Lock should be held when queuing packets to h/w fifos in order to
> properly handle configurations with two enabled interfaces or multiple enabled
> mbss. The second patch fixes scan issues addressing a review comment
> from K.Valo regarding pending active scans upon device removal.
>
> Regards,
> Sergey
>
> Sergey Matyukevich (2):
>
> qtnfmac: lock access to h/w in tx path
> qtnfmac: abort scans on wireless interface changes
There was a problem patchwork.kernel.org over the weekend, but it should
be fixed now. Unfortunately your patches didn't make it to patchwork, so
please resend.
https://patchwork.kernel.org/project/linux-wireless/list/?state=*
--
Kalle Valo
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
Fix tx path regression. Lock should be held when queuing packets
to h/w fifos in order to properly handle configurations with
multiple enabled interfaces.
Signed-off-by: Sergey Matyukevich <[email protected]>
---
drivers/net/wireless/quantenna/qtnfmac/pearl/pcie.c | 9 ++++++++-
drivers/net/wireless/quantenna/qtnfmac/pearl/pcie_bus_priv.h | 2 ++
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/drivers/net/wireless/quantenna/qtnfmac/pearl/pcie.c b/drivers/net/wireless/quantenna/qtnfmac/pearl/pcie.c
index 502e72b7cdcc..69131965a298 100644
--- a/drivers/net/wireless/quantenna/qtnfmac/pearl/pcie.c
+++ b/drivers/net/wireless/quantenna/qtnfmac/pearl/pcie.c
@@ -661,14 +661,18 @@ static int qtnf_pcie_data_tx(struct qtnf_bus *bus, struct sk_buff *skb)
struct qtnf_pcie_bus_priv *priv = (void *)get_bus_priv(bus);
dma_addr_t txbd_paddr, skb_paddr;
struct qtnf_tx_bd *txbd;
+ unsigned long flags;
int len, i;
u32 info;
int ret = 0;
+ spin_lock_irqsave(&priv->tx0_lock, flags);
+
if (!qtnf_tx_queue_ready(priv)) {
if (skb->dev)
netif_stop_queue(skb->dev);
+ spin_unlock_irqrestore(&priv->tx0_lock, flags);
return NETDEV_TX_BUSY;
}
@@ -717,8 +721,10 @@ static int qtnf_pcie_data_tx(struct qtnf_bus *bus, struct sk_buff *skb)
dev_kfree_skb_any(skb);
}
- qtnf_pcie_data_tx_reclaim(priv);
priv->tx_done_count++;
+ spin_unlock_irqrestore(&priv->tx0_lock, flags);
+
+ qtnf_pcie_data_tx_reclaim(priv);
return NETDEV_TX_OK;
}
@@ -1247,6 +1253,7 @@ static int qtnf_pcie_probe(struct pci_dev *pdev, const struct pci_device_id *id)
strcpy(bus->fwname, QTN_PCI_PEARL_FW_NAME);
init_completion(&bus->request_firmware_complete);
mutex_init(&bus->bus_lock);
+ spin_lock_init(&pcie_priv->tx0_lock);
spin_lock_init(&pcie_priv->irq_lock);
spin_lock_init(&pcie_priv->tx_reclaim_lock);
diff --git a/drivers/net/wireless/quantenna/qtnfmac/pearl/pcie_bus_priv.h b/drivers/net/wireless/quantenna/qtnfmac/pearl/pcie_bus_priv.h
index e76a23716ee0..86ac1ccedb52 100644
--- a/drivers/net/wireless/quantenna/qtnfmac/pearl/pcie_bus_priv.h
+++ b/drivers/net/wireless/quantenna/qtnfmac/pearl/pcie_bus_priv.h
@@ -34,6 +34,8 @@ struct qtnf_pcie_bus_priv {
/* lock for tx reclaim operations */
spinlock_t tx_reclaim_lock;
+ /* lock for tx0 operations */
+ spinlock_t tx0_lock;
u8 msi_enabled;
int mps;
--
2.11.0
Sergey Matyukevich <[email protected]> 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 ;)
--
Kalle Valo
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