| Message ID | 20240427091248.2013946-1-quic_tamizhr@quicinc.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | [PATCHv3] wifi: ath11k: skip status ring entry processing | expand |
On 4/27/2024 2:12 AM, Tamizh Chelvam Raja wrote: > From: Venkateswara Naralasetty <quic_vnaralas@quicinc.com> > > If STATUS_BUFFER_DONE is not set for a monitor status ring entry, > we don't process the status ring until STATUS_BUFFER_DONE set > for that status ring entry. > > During LMAC reset it may happen that hardware will not write > STATUS_BUFFER_DONE tlv in status buffer, in that case we end up > waiting for STATUS_BUFFER_DONE leading to backpressure on monitor > status ring. > > To fix the issue, when HP(Head Pointer) + 1 entry is peeked and if DMA > is not done and if HP + 2 entry's DMA done is set, > replenish HP + 1 entry and start processing in next interrupt. > If HP + 2 entry's DMA done is not set, poll onto HP + 1 entry DMA > done to be set. > > Also, during monitor attach HP points to the end of the ring and > TP(Tail Pointer) points to the start of the ring. > Using ath11k_hal_srng_src_peek() may result in processing invalid buffer > for the very first interrupt. Since, HW starts writing buffer from TP. > > To avoid this issue call ath11k_hal_srng_src_next_peek() instead of > calling ath11k_hal_srng_src_peek(). > > Tested-on: IPQ5018 hw1.0 AHB WLAN.HK.2.6.0.1-00861-QCAHKSWPL_SILICONZ-1 > > Reported-by: kernel test robot <lkp@intel.com> > Closes: https://lore.kernel.org/oe-kbuild-all/202303281719.CvnPkOiK-lkp@intel.com/ I believe these are misleading. LKP didn't find the problem you are fixing, it found a problem in the implementation of the patch. So I would move these below the "---" so the LKP knows the issue it found is fixed, but the git history isn't itself isn't misleading > Signed-off-by: Venkateswara Naralasetty <quic_vnaralas@quicinc.com> > Co-developed-by: Tamizh Chelvam Raja <quic_tamizhr@quicinc.com> > Signed-off-by: Tamizh Chelvam Raja <quic_tamizhr@quicinc.com> > --- > v3: > * Rebased on top of ToT > v2: > * Fixed compilation warning Reported-by: kernel test robot <lkp@intel.com> > > drivers/net/wireless/ath/ath11k/dp_rx.c | 88 ++++++++++++++++++++++--- > drivers/net/wireless/ath/ath11k/hal.c | 14 ++++ > drivers/net/wireless/ath/ath11k/hal.h | 2 + My Qualcomm Innovation Center copyright checker reports: drivers/net/wireless/ath/ath11k/dp_rx.c copyright missing 2024 drivers/net/wireless/ath/ath11k/hal.c copyright missing 2024
> On Apr 28, 2024, at 6:46 AM, Jeff Johnson <quic_jjohnson@quicinc.com> wrote: > > On 4/27/2024 2:12 AM, Tamizh Chelvam Raja wrote: >> From: Venkateswara Naralasetty <quic_vnaralas@quicinc.com> >> >> If STATUS_BUFFER_DONE is not set for a monitor status ring entry, >> we don't process the status ring until STATUS_BUFFER_DONE set >> for that status ring entry. >> >> During LMAC reset it may happen that hardware will not write >> STATUS_BUFFER_DONE tlv in status buffer, in that case we end up >> waiting for STATUS_BUFFER_DONE leading to backpressure on monitor >> status ring. >> Can you known the LMAC reset event, if you can known, you can set all the ring entry status to done after reset is done, and the logic of code will be more clear. >> To fix the issue, when HP(Head Pointer) + 1 entry is peeked and if DMA >> is not done and if HP + 2 entry's DMA done is set, >> replenish HP + 1 entry and start processing in next interrupt. >> If HP + 2 entry's DMA done is not set, poll onto HP + 1 entry DMA >> done to be set. >> >> Also, during monitor attach HP points to the end of the ring and >> TP(Tail Pointer) points to the start of the ring. >> Using ath11k_hal_srng_src_peek() may result in processing invalid buffer >> for the very first interrupt. Since, HW starts writing buffer from TP. >> >> To avoid this issue call ath11k_hal_srng_src_next_peek() instead of >> calling ath11k_hal_srng_src_peek(). >> >> Tested-on: IPQ5018 hw1.0 AHB WLAN.HK.2.6.0.1-00861-QCAHKSWPL_SILICONZ-1 >> >> Reported-by: kernel test robot <lkp@intel.com> >> Closes: https://lore.kernel.org/oe-kbuild-all/202303281719.CvnPkOiK-lkp@intel.com/ > > I believe these are misleading. LKP didn't find the problem you are fixing, it > found a problem in the implementation of the patch. > So I would move these below the "---" so the LKP knows the issue it found is > fixed, but the git history isn't itself isn't misleading > >> Signed-off-by: Venkateswara Naralasetty <quic_vnaralas@quicinc.com> >> Co-developed-by: Tamizh Chelvam Raja <quic_tamizhr@quicinc.com> >> Signed-off-by: Tamizh Chelvam Raja <quic_tamizhr@quicinc.com> >> --- >> v3: >> * Rebased on top of ToT >> v2: >> * Fixed compilation warning Reported-by: kernel test robot <lkp@intel.com> >> >> drivers/net/wireless/ath/ath11k/dp_rx.c | 88 ++++++++++++++++++++++--- >> drivers/net/wireless/ath/ath11k/hal.c | 14 ++++ >> drivers/net/wireless/ath/ath11k/hal.h | 2 + > > My Qualcomm Innovation Center copyright checker reports: > drivers/net/wireless/ath/ath11k/dp_rx.c copyright missing 2024 > drivers/net/wireless/ath/ath11k/hal.c copyright missing 2024 >
> On Apr 28, 2024, at 8:54 PM, Meiyong Yu <meiyong.yu@126.com> wrote: > > > >> On Apr 28, 2024, at 6:46 AM, Jeff Johnson <quic_jjohnson@quicinc.com> wrote: >> >> On 4/27/2024 2:12 AM, Tamizh Chelvam Raja wrote: >>> From: Venkateswara Naralasetty <quic_vnaralas@quicinc.com> >>> >>> If STATUS_BUFFER_DONE is not set for a monitor status ring entry, >>> we don't process the status ring until STATUS_BUFFER_DONE set >>> for that status ring entry. >>> >>> During LMAC reset it may happen that hardware will not write >>> STATUS_BUFFER_DONE tlv in status buffer, in that case we end up >>> waiting for STATUS_BUFFER_DONE leading to backpressure on monitor >>> status ring. >>> > > Can you known the LMAC reset event, if you can known, you can set all the ring entry > status to done after reset is done, and the logic of code will be more clear. > If sene of the LMAC reset event is asynchronous, You can do this: 1) when LMAC init than set a value to the new add global variabe lmac_life_cycle_id 2) before add tlv to ring, set lmac_life_cycle_id to tlv 3) when LMAC reset event is trigger, increase the value of lmac_life_cycle_id 4) when get the status of tlv in ring(must delay for same period to ensure the real send ring is already send), check the value lmac_life_cycle_id in tlv, if it smaller than the global one set tlv status to DONE >>> To fix the issue, when HP(Head Pointer) + 1 entry is peeked and if DMA >>> is not done and if HP + 2 entry's DMA done is set, >>> replenish HP + 1 entry and start processing in next interrupt. >>> If HP + 2 entry's DMA done is not set, poll onto HP + 1 entry DMA >>> done to be set. >>> >>> Also, during monitor attach HP points to the end of the ring and >>> TP(Tail Pointer) points to the start of the ring. >>> Using ath11k_hal_srng_src_peek() may result in processing invalid buffer >>> for the very first interrupt. Since, HW starts writing buffer from TP. >>> >>> To avoid this issue call ath11k_hal_srng_src_next_peek() instead of >>> calling ath11k_hal_srng_src_peek(). >>> >>> Tested-on: IPQ5018 hw1.0 AHB WLAN.HK.2.6.0.1-00861-QCAHKSWPL_SILICONZ-1 >>> >>> Reported-by: kernel test robot <lkp@intel.com> >>> Closes: https://lore.kernel.org/oe-kbuild-all/202303281719.CvnPkOiK-lkp@intel.com/ >> >> I believe these are misleading. LKP didn't find the problem you are fixing, it >> found a problem in the implementation of the patch. >> So I would move these below the "---" so the LKP knows the issue it found is >> fixed, but the git history isn't itself isn't misleading >> >>> Signed-off-by: Venkateswara Naralasetty <quic_vnaralas@quicinc.com> >>> Co-developed-by: Tamizh Chelvam Raja <quic_tamizhr@quicinc.com> >>> Signed-off-by: Tamizh Chelvam Raja <quic_tamizhr@quicinc.com> >>> --- >>> v3: >>> * Rebased on top of ToT >>> v2: >>> * Fixed compilation warning Reported-by: kernel test robot <lkp@intel.com> >>> >>> drivers/net/wireless/ath/ath11k/dp_rx.c | 88 ++++++++++++++++++++++--- >>> drivers/net/wireless/ath/ath11k/hal.c | 14 ++++ >>> drivers/net/wireless/ath/ath11k/hal.h | 2 + >> >> My Qualcomm Innovation Center copyright checker reports: >> drivers/net/wireless/ath/ath11k/dp_rx.c copyright missing 2024 >> drivers/net/wireless/ath/ath11k/hal.c copyright missing 2024 >> >
On 4/28/2024 6:32 PM, Meiyong Yu wrote: > > >> On Apr 28, 2024, at 8:54 PM, Meiyong Yu <meiyong.yu@126.com> wrote: >> >> >> >>> On Apr 28, 2024, at 6:46 AM, Jeff Johnson <quic_jjohnson@quicinc.com> wrote: >>> >>> On 4/27/2024 2:12 AM, Tamizh Chelvam Raja wrote: >>>> From: Venkateswara Naralasetty <quic_vnaralas@quicinc.com> >>>> >>>> If STATUS_BUFFER_DONE is not set for a monitor status ring entry, >>>> we don't process the status ring until STATUS_BUFFER_DONE set >>>> for that status ring entry. >>>> >>>> During LMAC reset it may happen that hardware will not write >>>> STATUS_BUFFER_DONE tlv in status buffer, in that case we end up >>>> waiting for STATUS_BUFFER_DONE leading to backpressure on monitor >>>> status ring. >>>> >> >> Can you known the LMAC reset event, if you can known, you can set all the ring entry >> status to done after reset is done, and the logic of code will be more clear. >> Host unaware of this event as this is internal to hardware. And as per the suggestion we are reaping next entry and proceeding further on this. > > If sene of the LMAC reset event is asynchronous, You can do this: > 1) when LMAC init than set a value to the new add global variabe lmac_life_cycle_id > 2) before add tlv to ring, set lmac_life_cycle_id to tlv > 3) when LMAC reset event is trigger, increase the value of lmac_life_cycle_id > 4) when get the status of tlv in ring(must delay for same period to ensure the real send ring is already send), > check the value lmac_life_cycle_id in tlv, if it smaller than the global one set tlv status to DONE > > >>>> To fix the issue, when HP(Head Pointer) + 1 entry is peeked and if DMA >>>> is not done and if HP + 2 entry's DMA done is set, >>>> replenish HP + 1 entry and start processing in next interrupt. >>>> If HP + 2 entry's DMA done is not set, poll onto HP + 1 entry DMA >>>> done to be set. >>>> >>>> Also, during monitor attach HP points to the end of the ring and >>>> TP(Tail Pointer) points to the start of the ring. >>>> Using ath11k_hal_srng_src_peek() may result in processing invalid buffer >>>> for the very first interrupt. Since, HW starts writing buffer from TP. >>>> >>>> To avoid this issue call ath11k_hal_srng_src_next_peek() instead of >>>> calling ath11k_hal_srng_src_peek(). >>>> >>>> Tested-on: IPQ5018 hw1.0 AHB WLAN.HK.2.6.0.1-00861-QCAHKSWPL_SILICONZ-1 >>>> >>>> Reported-by: kernel test robot <lkp@intel.com> >>>> Closes: https://lore.kernel.org/oe-kbuild-all/202303281719.CvnPkOiK-lkp@intel.com/ >>> >>> I believe these are misleading. LKP didn't find the problem you are fixing, it >>> found a problem in the implementation of the patch. >>> So I would move these below the "---" so the LKP knows the issue it found is >>> fixed, but the git history isn't itself isn't misleading >>> >>>> Signed-off-by: Venkateswara Naralasetty <quic_vnaralas@quicinc.com> >>>> Co-developed-by: Tamizh Chelvam Raja <quic_tamizhr@quicinc.com> >>>> Signed-off-by: Tamizh Chelvam Raja <quic_tamizhr@quicinc.com> >>>> --- >>>> v3: >>>> * Rebased on top of ToT >>>> v2: >>>> * Fixed compilation warning Reported-by: kernel test robot <lkp@intel.com> >>>> >>>> drivers/net/wireless/ath/ath11k/dp_rx.c | 88 ++++++++++++++++++++++--- >>>> drivers/net/wireless/ath/ath11k/hal.c | 14 ++++ >>>> drivers/net/wireless/ath/ath11k/hal.h | 2 + >>> >>> My Qualcomm Innovation Center copyright checker reports: >>> drivers/net/wireless/ath/ath11k/dp_rx.c copyright missing 2024 >>> drivers/net/wireless/ath/ath11k/hal.c copyright missing 2024 >>> >> >
diff --git a/drivers/net/wireless/ath/ath11k/dp_rx.c b/drivers/net/wireless/ath/ath11k/dp_rx.c index afd481f5858f..79b7f4996f5d 100644 --- a/drivers/net/wireless/ath/ath11k/dp_rx.c +++ b/drivers/net/wireless/ath/ath11k/dp_rx.c @@ -2990,11 +2990,52 @@ ath11k_dp_rx_mon_update_status_buf_state(struct ath11k_mon_data *pmon, } } +static enum dp_mon_status_buf_state +ath11k_dp_rx_mon_handle_status_buf_done(struct ath11k_base *ab, struct hal_srng *srng, + struct dp_rxdma_ring *rx_ring) +{ + struct ath11k_skb_rxcb *rxcb; + struct hal_tlv_hdr *tlv; + struct sk_buff *skb; + void *status_desc; + dma_addr_t paddr; + u32 cookie; + int buf_id; + u8 rbm; + + status_desc = ath11k_hal_srng_src_next_peek(ab, srng); + if (!status_desc) + return DP_MON_STATUS_NO_DMA; + + ath11k_hal_rx_buf_addr_info_get(status_desc, &paddr, &cookie, &rbm); + + buf_id = FIELD_GET(DP_RXDMA_BUF_COOKIE_BUF_ID, cookie); + + spin_lock_bh(&rx_ring->idr_lock); + skb = idr_find(&rx_ring->bufs_idr, buf_id); + spin_unlock_bh(&rx_ring->idr_lock); + + if (!skb) + return DP_MON_STATUS_NO_DMA; + + rxcb = ATH11K_SKB_RXCB(skb); + dma_sync_single_for_cpu(ab->dev, rxcb->paddr, + skb->len + skb_tailroom(skb), + DMA_FROM_DEVICE); + + tlv = (struct hal_tlv_hdr *)skb->data; + if (FIELD_GET(HAL_TLV_HDR_TAG, tlv->tl) != HAL_RX_STATUS_BUFFER_DONE) + return DP_MON_STATUS_NO_DMA; + + return DP_MON_STATUS_REPLINISH; +} + static int ath11k_dp_rx_reap_mon_status_ring(struct ath11k_base *ab, int mac_id, int *budget, struct sk_buff_head *skb_list) { struct ath11k *ar; const struct ath11k_hw_hal_params *hal_params; + enum dp_mon_status_buf_state reap_status; struct ath11k_pdev_dp *dp; struct dp_rxdma_ring *rx_ring; struct ath11k_mon_data *pmon; @@ -3022,8 +3063,7 @@ static int ath11k_dp_rx_reap_mon_status_ring(struct ath11k_base *ab, int mac_id, ath11k_hal_srng_access_begin(ab, srng); while (*budget) { *budget -= 1; - rx_mon_status_desc = - ath11k_hal_srng_src_peek(ab, srng); + rx_mon_status_desc = ath11k_hal_srng_src_peek(ab, srng); if (!rx_mon_status_desc) { pmon->buf_state = DP_MON_STATUS_REPLINISH; break; @@ -3057,15 +3097,43 @@ static int ath11k_dp_rx_reap_mon_status_ring(struct ath11k_base *ab, int mac_id, ath11k_warn(ab, "mon status DONE not set %lx, buf_id %d\n", FIELD_GET(HAL_TLV_HDR_TAG, tlv->tl), buf_id); - /* If done status is missing, hold onto status - * ring until status is done for this status - * ring buffer. - * Keep HP in mon_status_ring unchanged, - * and break from here. - * Check status for same buffer for next time + /* RxDMA status done bit might not be set even + * though tp is moved by HW. */ - pmon->buf_state = DP_MON_STATUS_NO_DMA; - break; + + /* If done status is missing: + * 1. As per MAC team's suggestion, + * when HP + 1 entry is peeked and if DMA + * is not done and if HP + 2 entry's DMA done + * is set. skip HP + 1 entry and + * start processing in next interrupt. + * 2. If HP + 2 entry's DMA done is not set, + * poll onto HP + 1 entry DMA done to be set. + * Check status for same buffer for next time + * dp_rx_mon_status_srng_process + */ + + reap_status = ath11k_dp_rx_mon_handle_status_buf_done(ab, srng, + rx_ring); + if (reap_status == DP_MON_STATUS_NO_DMA) { + continue; + } else if (reap_status == DP_MON_STATUS_REPLINISH) { + ath11k_warn(ab, "mon status DONE not set %lx, buf_id %d\n", + FIELD_GET(HAL_TLV_HDR_TAG, tlv->tl), + buf_id); + + spin_lock_bh(&rx_ring->idr_lock); + idr_remove(&rx_ring->bufs_idr, buf_id); + spin_unlock_bh(&rx_ring->idr_lock); + + dma_unmap_single(ab->dev, rxcb->paddr, + skb->len + skb_tailroom(skb), + DMA_FROM_DEVICE); + + dev_kfree_skb_any(skb); + pmon->buf_state = DP_MON_STATUS_REPLINISH; + goto move_next; + } } spin_lock_bh(&rx_ring->idr_lock); diff --git a/drivers/net/wireless/ath/ath11k/hal.c b/drivers/net/wireless/ath/ath11k/hal.c index f3d04568c221..dbba45c22dc6 100644 --- a/drivers/net/wireless/ath/ath11k/hal.c +++ b/drivers/net/wireless/ath/ath11k/hal.c @@ -796,6 +796,20 @@ u32 *ath11k_hal_srng_src_get_next_reaped(struct ath11k_base *ab, return desc; } +u32 *ath11k_hal_srng_src_next_peek(struct ath11k_base *ab, struct hal_srng *srng) +{ + u32 next_hp; + + lockdep_assert_held(&srng->lock); + + next_hp = (srng->u.src_ring.hp + srng->entry_size) % srng->ring_size; + + if (next_hp != srng->u.src_ring.cached_tp) + return srng->ring_base_vaddr + next_hp; + + return NULL; +} + u32 *ath11k_hal_srng_src_peek(struct ath11k_base *ab, struct hal_srng *srng) { lockdep_assert_held(&srng->lock); diff --git a/drivers/net/wireless/ath/ath11k/hal.h b/drivers/net/wireless/ath/ath11k/hal.h index e453c137385e..dc8bbe073017 100644 --- a/drivers/net/wireless/ath/ath11k/hal.h +++ b/drivers/net/wireless/ath/ath11k/hal.h @@ -947,6 +947,8 @@ u32 *ath11k_hal_srng_dst_peek(struct ath11k_base *ab, struct hal_srng *srng); int ath11k_hal_srng_dst_num_free(struct ath11k_base *ab, struct hal_srng *srng, bool sync_hw_ptr); u32 *ath11k_hal_srng_src_peek(struct ath11k_base *ab, struct hal_srng *srng); +u32 *ath11k_hal_srng_src_next_peek(struct ath11k_base *ab, + struct hal_srng *srng); u32 *ath11k_hal_srng_src_get_next_reaped(struct ath11k_base *ab, struct hal_srng *srng); u32 *ath11k_hal_srng_src_reap_next(struct ath11k_base *ab,