Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <0e4a896d-1d57-166b-9cef-fd551d22464b@quicinc.com>
Date:   Wed, 12 Oct 2022 23:50:25 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.3.3
Subject: Re: [PATCH 15/50] wifi: ath12k: add dp_rx.c
Content-Language: en-US
To:     "Sriram R (QUIC)" <quic_srirrama@quicinc.com>,
        Kalle Valo <kvalo@kernel.org>,
        "linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>
CC:     "ath12k@lists.infradead.org" <ath12k@lists.infradead.org>
References: <20220812161003.27279-1-kvalo@kernel.org>
 <20220812161003.27279-16-kvalo@kernel.org>
 <b2cd5b02-60ca-c788-af71-9f36f6a8ede3@quicinc.com>
 <SN6PR02MB433498DC221CF70BE3B6CE63F7229@SN6PR02MB4334.namprd02.prod.outlook.com>
From:   Jeff Johnson <quic_jjohnson@quicinc.com>
In-Reply-To: <SN6PR02MB433498DC221CF70BE3B6CE63F7229@SN6PR02MB4334.namprd02.prod.outlook.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

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