Re: [PATCH 15/50] wifi: ath12k: add dp_rx.c

Jeff Johnson
2022-10-13T06:50:36Z

On 10/12/2022 4:39 PM, Sriram R (QUIC) wrote:
>>> + elem = kzalloc(sizeof(*elem), GFP_ATOMIC);
>>> + if (!elem)
>>> + goto free_desc;
>>> +
>>> + elem->ts = jiffies;
>>> + memcpy(&elem->data, rx_tid, sizeof(*rx_tid));
>>> +
>>> + spin_lock_bh(&dp->reo_cmd_lock);
>>> + list_add_tail(&elem->list, &dp->reo_cmd_cache_flush_list);
>>> + dp->reo_cmd_cache_flush_count++;
>>> +
>>> + /* Flush and invalidate aged REO desc from HW cache */
>>> + list_for_each_entry_safe(elem, tmp, &dp->reo_cmd_cache_flush_list,
>>> + list) {
>>> + if (dp->reo_cmd_cache_flush_count >
>> ATH12K_DP_RX_REO_DESC_FREE_THRES ||
>>> + time_after(jiffies, elem->ts +
>>> +
>> msecs_to_jiffies(ATH12K_DP_RX_REO_DESC_FREE_TIMEOUT_MS))) {
>>> + list_del(&elem->list);
>>> + dp->reo_cmd_cache_flush_count--;
>>> + spin_unlock_bh(&dp->reo_cmd_lock);
>>> +
>>> + ath12k_dp_reo_cache_flush(ab, &elem->data);
>>> + kfree(elem);
>>> + spin_lock_bh(&dp->reo_cmd_lock);
>>
>> is this really a safe iteration if you unlock & lock in the middle?
>> what prevents the tmp node from being deleted during this window?
> The reo_cmd_cache_flush_list is used in only two contexts, one is this
> Function called from napi and the other in ath12k_dp_free during
> core destroy. Before dp_free, the irqs would be disabled and would wait
> synchronize. Hence there wouldn’t be any race against add or delete
> to this list. Please let me know if that’s fine.

please add that as a code comment since unlock/do something/lock is a
"code smell" so you should justify the smell