> -----Original Message-----
> From: Jeff Johnson <quic_jjohnson@xxxxxxxxxxx>
> Sent: Friday, August 19, 2022 5:04 AM
> To: Kalle Valo <kvalo@xxxxxxxxxx>; linux-wireless@xxxxxxxxxxxxxxx
> Cc: ath12k@xxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH 37/50] wifi: ath12k: add peer.c
>
> WARNING: This email originated from outside of Qualcomm. Please be wary
> of any links or attachments, and do not enable macros.
>
> On 8/12/2022 9:09 AM, Kalle Valo wrote:
> > From: Kalle Valo <quic_kvalo@xxxxxxxxxxx>
> >
> > (Patches split into one patch per file for easier review, but the
> > final commit will be one big patch. See the cover letter for more
> > info.)
> >
> > Signed-off-by: Kalle Valo <quic_kvalo@xxxxxxxxxxx>
> > ---
> > drivers/net/wireless/ath/ath12k/peer.c | 343
> +++++++++++++++++++++++++++++++++
> > 1 file changed, 343 insertions(+)
> >
> > diff --git a/drivers/net/wireless/ath/ath12k/peer.c
> > b/drivers/net/wireless/ath/ath12k/peer.c
> > new file mode 100644
> > index 000000000000..30ffdbbdcc7f
> > --- /dev/null
> > +++ b/drivers/net/wireless/ath/ath12k/peer.c
> > @@ -0,0 +1,343 @@
> > +// SPDX-License-Identifier: BSD-3-Clause-Clear
> > +/*
> > + * Copyright (c) 2018-2021 The Linux Foundation. All rights reserved.
> > + * Copyright (c) 2021-2022 Qualcomm Innovation Center, Inc. All rights
> reserved.
> > + */
> > +
> > +#include "core.h"
> > +#include "peer.h"
> > +#include "debug.h"
> > +
> > +struct ath12k_peer *ath12k_peer_find(struct ath12k_base *ab, int
> vdev_id,
> > + const u8 *addr) {
> > + struct ath12k_peer *peer;
> > +
> > + lockdep_assert_held(&ab->base_lock);
> > +
> > + list_for_each_entry(peer, &ab->peers, list) {
> > + if (peer->vdev_id != vdev_id)
> > + continue;
> > + if (!ether_addr_equal(peer->addr, addr))
> > + continue;
> > +
> > + return peer;
> > + }
> > +
> > + return NULL;
> > +}
> > +
> > +static struct ath12k_peer *ath12k_peer_find_by_pdev_idx(struct
> ath12k_base *ab,
> > + u8 pdev_idx,
> > +const u8 *addr) {
> > + struct ath12k_peer *peer;
> > +
> > + lockdep_assert_held(&ab->base_lock);
> > +
> > + list_for_each_entry(peer, &ab->peers, list) {
> > + if (peer->pdev_idx != pdev_idx)
> > + continue;
> > + if (!ether_addr_equal(peer->addr, addr))
> > + continue;
> > +
> > + return peer;
> > + }
> > +
> > + return NULL;
> > +}
> > +
> > +struct ath12k_peer *ath12k_peer_find_by_addr(struct ath12k_base *ab,
> > + const u8 *addr) {
> > + struct ath12k_peer *peer;
> > +
> > + lockdep_assert_held(&ab->base_lock);
> > +
> > + list_for_each_entry(peer, &ab->peers, list) {
> > + if (!ether_addr_equal(peer->addr, addr))
> > + continue;
> > +
> > + return peer;
> > + }
> > +
> > + return NULL;
> > +}
> > +
> > +struct ath12k_peer *ath12k_peer_find_by_id(struct ath12k_base *ab,
> > + int peer_id) {
> > + struct ath12k_peer *peer;
> > +
> > + lockdep_assert_held(&ab->base_lock);
> > +
> > + list_for_each_entry(peer, &ab->peers, list)
> > + if (peer_id == peer->peer_id)
> > + return peer;
> > +
> > + return NULL;
> > +}
> > +
> > +struct ath12k_peer *ath12k_peer_find_by_vdev_id(struct ath12k_base
> *ab,
> > + int vdev_id) {
> > + struct ath12k_peer *peer;
> > +
> > + spin_lock_bh(&ab->base_lock);
>
> the other ath12k_peer_find_*() functions expect the caller to take the lock,
> but in this one the function itself takes the lock. why the discrepancy? should
> these have a consistent interface? note that the returned peer won't be
> protected by the lock.
>
> note i see this is only used in one place, and there it doesn't use the peer, it
> just checks if it exists.
>
> perhaps rename this to bool ath12k_peer_exists_by_vdev() to justify the
> difference in locking?
>
Sure will address this comment in the next version of the patch
>
> > +
> > + list_for_each_entry(peer, &ab->peers, list) {
> > + if (vdev_id == peer->vdev_id) {
> > + spin_unlock_bh(&ab->base_lock);
> > + return peer;
> > + }
> > + }
> > + spin_unlock_bh(&ab->base_lock);
> > + return NULL;
> > +}
>
> [...]
Thanks
Karthikeyan
