> -----Original Message-----
> From: Johannes Berg <johannes@xxxxxxxxxxxxxxxx>
> Sent: Wednesday, April 30, 2025 3:53 PM
> To: Maharaja Kennadyrajan <maharaja.kennadyrajan@xxxxxxxxxxxxxxxx>
> Cc: linux-wireless@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH wireless-next RFC 2/2] wifi: mac80211: process group
> addressed Rx data and mgmt packets on intended interface
>
> WARNING: This email originated from outside of Qualcomm. Please be wary of
> any links or attachments, and do not enable macros.
>
> Hi,
>
> I suppose you're mostly looking for feedback from Mediatek etc. for their multi-
> radio architecture. Couple of comments on the code anyway:
>
>
> > + if (valid_links) {
> > + for_each_set_bit(link_id, &valid_links,
> > + IEEE80211_MLD_MAX_NUM_LINKS) {
>
> We just added some for_each_sdata_link() macro or so, so you don't need the
> distinction between MLO and non-MLO. I really don't like seeing that, if we do
> that all the time we have far too many places that would do it.
>
We have to loop over the links of sdata here. So shall we use this API for_each_link_data() instead of for_each_sdata_link?
> > + bss_conf = rcu_dereference(sdata->vif.link_conf[link_id]);
> > + if (!bss_conf)
> > + continue;
> > + conf = rcu_dereference(bss_conf->chanctx_conf);
> > + if (conf && conf->def.chan &&
> > + conf->def.chan->center_freq == freq) {
> > + is_freq_match = true;
>
> You could much easier just "return true" here, and "return false" later after all of
> it, without the variable.
>
> (Curious: do you have some coding style guidelines that this breaks?
> Like "single return from function" style? Because I see it often from Qualcomm for
> no real reason. Maybe I'm also just misremembering, but I don't see a good
> reason in cases like this to even want to write it with a variable. Seems more
> natural to just say "ok got it, return true".)
>
Yeah, we can return true here and avoid the variable.
> > if (prev) {
> > - rx.sdata = prev;
> > + if (!status->freq ||
> > + ieee80211_rx_is_sdata_match(prev, status->freq)) {
> > + rx.sdata = prev;
>
> nit: you can combine it all into one condition now:
>
> if (prev && (!status->freq || ...)) {
> }
>
> not sure that's really all that much better, but saves the reindent?
>
>
> Actually, it might be better overall to move the !freq into
> ieee80211_rx_is_sdata_match()? So just have
>
> ieee80211_rx_is_sdata_match(...)
> {
> if (!freq)
> return true;
> ...
> }
>
> and then you don't need the || in the caller, which simplifies that? And also gives
> you a more natural place to put the comment.
Thanks for this suggestion. I will rework on this and send the updated version.
>
> johannes
