| Message ID | 20240130040303.370590-9-quic_kangyang@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | wifi: ath12k: P2P support for WCN7850 | expand |
Kang Yang <quic_kangyang@quicinc.com> writes: > In current code, the management frames must be sent after vdev is started. > But for P2P device, vdev won't start until P2P negotiation is done. So > this logic doesn't make sense for P2P device. > > Also use ar->conf_mutex to synchronize ar to avoid potential conflicts. Please do locking changes in a separate followup patch, I removed this in the pending branch: https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=d357dcb3cd0cd3bf57064dc673b5477d884454b3 I assume you are referring to ar->allocated_vdev_map and access to that indeed doesn't look consistent. Most of the places take conf->mutex but I see some places which it's accessed without the mutex, for example ath12k_mac_get_arvif_by_vdev_id() and ath12k_mac_get_ar_by_vdev_id(). I recommend in the followup patch checking all the access to ar->allocated_vdev_map, fixing that if needed and adding documentation to struct ath12k::allocated_vdev_map how it's supposed to be protected.
On 2/7/2024 12:24 AM, Kalle Valo wrote: > Kang Yang <quic_kangyang@quicinc.com> writes: > >> In current code, the management frames must be sent after vdev is started. >> But for P2P device, vdev won't start until P2P negotiation is done. So >> this logic doesn't make sense for P2P device. >> >> Also use ar->conf_mutex to synchronize ar to avoid potential conflicts. > > Please do locking changes in a separate followup patch, I removed this > in the pending branch: > > https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=d357dcb3cd0cd3bf57064dc673b5477d884454b3 > > I assume you are referring to ar->allocated_vdev_map and access to that > indeed doesn't look consistent. Most of the places take conf->mutex but > I see some places which it's accessed without the mutex, for example > ath12k_mac_get_arvif_by_vdev_id() and ath12k_mac_get_ar_by_vdev_id(). > > I recommend in the followup patch checking all the access to > ar->allocated_vdev_map, fixing that if needed and adding documentation > to struct ath12k::allocated_vdev_map how it's supposed to be protected. Will do it in the future. >
On 2/7/2024 12:24 AM, Kalle Valo wrote: > Kang Yang <quic_kangyang@quicinc.com> writes: > >> In current code, the management frames must be sent after vdev is started. >> But for P2P device, vdev won't start until P2P negotiation is done. So >> this logic doesn't make sense for P2P device. >> >> Also use ar->conf_mutex to synchronize ar to avoid potential conflicts. > > Please do locking changes in a separate followup patch, I removed this > in the pending branch: > > https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=d357dcb3cd0cd3bf57064dc673b5477d884454b3 > > I assume you are referring to ar->allocated_vdev_map and access to that > indeed doesn't look consistent. Most of the places take conf->mutex but > I see some places which it's accessed without the mutex, for example > ath12k_mac_get_arvif_by_vdev_id() and ath12k_mac_get_ar_by_vdev_id(). > Hi, kalle: I just take a look for ath12k_mac_get_arvif_by_vdev_id() and ath12k_mac_get_ar_by_vdev_id. Both of them use rcu_read_lock(), so we don't need mutex_lock() anymore, right? I also tried to add mutex_lock() for them, cannot work well: [ 7804.291286] BUG: sleeping function called from invalid context at kernel/locking/mutex.c:585 [ 7804.291349] in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 0, name: swapper/8 [ 7804.291358] preempt_count: 101, expected: 0 [ 7804.291366] RCU nest depth: 1, expected: 0 [ 7804.291374] 1 lock held by swapper/8/0: …… > I recommend in the followup patch checking all the access to > ar->allocated_vdev_map, fixing that if needed and adding documentation > to struct ath12k::allocated_vdev_map how it's supposed to be protected. > So i think the initial change is sufficient: just use mutex_lock() in ath12k_mgmt_over_wmi_tx_work().
diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c index ec90a21fcd7f..1377b710bdcb 100644 --- a/drivers/net/wireless/ath/ath12k/mac.c +++ b/drivers/net/wireless/ath/ath12k/mac.c @@ -5069,8 +5069,8 @@ static void ath12k_mgmt_over_wmi_tx_work(struct work_struct *work) } arvif = ath12k_vif_to_arvif(skb_cb->vif); - if (ar->allocated_vdev_map & (1LL << arvif->vdev_id) && - arvif->is_started) { + mutex_lock(&ar->conf_mutex); + if (ar->allocated_vdev_map & (1LL << arvif->vdev_id)) { ret = ath12k_mac_mgmt_tx_wmi(ar, arvif, skb); if (ret) { ath12k_warn(ar->ab, "failed to tx mgmt frame, vdev_id %d :%d\n", @@ -5084,6 +5084,7 @@ static void ath12k_mgmt_over_wmi_tx_work(struct work_struct *work) arvif->is_started); ath12k_mgmt_over_wmi_tx_drop(ar, skb); } + mutex_unlock(&ar->conf_mutex); } }
In current code, the management frames must be sent after vdev is started. But for P2P device, vdev won't start until P2P negotiation is done. So this logic doesn't make sense for P2P device. Also use ar->conf_mutex to synchronize ar to avoid potential conflicts. Tested-on: WCN7850 hw2.0 PCI WLAN.HMT.1.0.c5-00481-QCAHMTSWPL_V1.0_V2.0_SILICONZ-3 Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.0.1-00029-QCAHKSWPL_SILICONZ-1 Signed-off-by: Kang Yang <quic_kangyang@quicinc.com> --- v6: no change. v5: change commit log. v4: no change. v3: no change. v2: add Tested-on tag of QCN9274. --- drivers/net/wireless/ath/ath12k/mac.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)