On 10 April 2014 08:59, Kalle Valo <kvalo@xxxxxxxxxxxxxxxx> wrote:
> Michal Kazior <michal.kazior@xxxxxxxxx> writes:
>
>> It was troublesome to iterate over peers and
>> perform sleepable calls. This will be necessary
>> for some upcomming changes to tx flushing.
>>
>> Make peer allocation and initial setup
>> protected by both ar->conf_mutex and
>> ar->data_lock. This way it's possible to iterate
>> over peers with conf_mutex and call sleepable
>> functions.
>>
>> Signed-off-by: Michal Kazior <michal.kazior@xxxxxxxxx>
>
> First comments, but I need to read this much more carefully still:
>
>> --- a/drivers/net/wireless/ath/ath10k/core.h
>> +++ b/drivers/net/wireless/ath/ath10k/core.h
>> @@ -199,9 +199,14 @@ struct ath10k_dfs_stats {
>> #define ATH10K_MAX_NUM_PEER_IDS (1 << 11) /* htt rx_desc limit */
>>
>> struct ath10k_peer {
>> + /* protected by conf_mutex + data_lock */
>> struct list_head list;
>
> This really needs a lot more documentation. And besides, don't we
> actually want to protect struct ath10k::peers, not this?
Parts of the structure needs to be locked differently.
I suppose we can say we just protect everything with data_lock except
`struct list_head list` which needs to be write-protected by both
conf_mutex and data_lock. But then again, addr and vdev_id don't need
any locking as they are immutable (and set before adding to the list).
>
>> +/* hold conf_mutex for simple iteration, or conf_mutex+data_lock for
>> + * modifications */
>> struct ath10k_peer *ath10k_peer_find(struct ath10k *ar, int vdev_id,
>> const u8 *addr)
>> {
>> struct ath10k_peer *peer;
>>
>> - lockdep_assert_held(&ar->data_lock);
>> -
>> list_for_each_entry(peer, &ar->peers, list) {
>> if (peer->vdev_id != vdev_id)
>> continue;
>
> The comment here makes me suspicious. How can we safely iterate the list
> if we don't take data_lock? Doesn't it mean that the list can change
> while we have conf_mutex?
The idea is you need BOTH locks to modify the list structure, but you
need only one of them to iterate over the list safely and
consistently. This means writer will not alter the list structure
until there are no readers.
Michał
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
