Re: [PATCH v5 4/4] wifi: ath12k: implement custom wake_tx_queue with flow control

[Jose Ignacio Tornos Martinez]

Thanks for the review, Jeff.

> Tested-on missing

Ok, I will add it in v6

> NAK, must be EXPORT_SYMBOL()
> The ath12k code was contributed under the license:
> // SPDX-License-Identifier: BSD-3-Clause-Clear

I did it in an automatic way, I will fix it in v6

>> +	rcu_read_lock();
> 
> does this lock really need to be held for the entire function including the tx
> loop?
> 
>> +
>> +	arvif = rcu_dereference(ahvif->link[link_id]);
>> +	if (!arvif || !arvif->ar) {
>> +		rcu_read_unlock();
>> +		return;
>> +	}
>> +
>> +	ar = arvif->ar;
>> +	dp = ar->ab->dp;
>
> or is it sufficient to cache these values and then rcu_read_unlock() here?
> my LLM reviewer seems to think that would be sufficient
>
> note the spinlock in the while loop will cause the RCU read lock to be held
> when invoking mac_op_tx(), satisfying the RCU lock requirement there.

Keeping rcu_read_lock() for the entire function is intentional. 
ath12k_wifi7_mac_op_tx() calls rcu_dereference() internally (on ahvif->link[]
and ahsta->link[]), and with CONFIG_DEBUG_LOCK_ALLOC, rcu_dereference() checks
lock_is_held(&rcu_lock_map) via rcu_read_lock_held(). 
Only an explicit rcu_read_lock() sets rcu_lock_map and spin_lock_bh() does not,
so releasing early would trigger lockdep warnings.

> using guard(spinlock_bh)(&tx_ring->wake_tx_lock) would eliminate the need for
> all the spin_unlock_bh(&tx_ring->wake_tx_lock) calls

Better, I will use it in v6

>> +
>> +		spin_lock(&tcl_ring->lock);
>
> seems this should be spin_lock_bh() to align with all other callers in the driver.

The outer spin_lock_bh(&tx_ring->wake_tx_lock); already disables BH, so
spin_lock() for the inner tcl_ring->lock is correct and sufficient. Using
spin_lock_bh() would redundantly disable BH again.

Best regards
Jose Ignacio