Re: [PATCH RFC 2/7] wifi: cfg80211: reorg sinfo structure elements for MLO

[Sarika Sharma <quic_sarishar@quicinc.com>]

On 1/14/2025 4:49 PM, Johannes Berg wrote:
> On Sun, 2025-01-12 at 13:40 +0530, Sarika Sharma wrote:
>> On 1/10/2025 2:49 PM, Johannes Berg wrote:
>>> On Fri, 2025-01-10 at 09:54 +0530, Sarika Sharma wrote:
>>>>
>>>>    struct station_info {
>>>>
>>>> +	struct link_station_info deflink;
>>>
>>> Having a deflink here seems kind of questionable?
>>
>> ohh! why so? In other structures like vif, sdata, sta, we did the same
>> way. So why can't we have the same way here as well?
> 
> Well, (a) I tend to think in hindsight it was sort of a mistake, but (b)
> we couldn't really do it differently due to all drivers. But here you're
> already touching all the code anyway, and it's mostly cfg80211/mac80211,
> so why bother?

Sure, I will remove deflink and instead could use link pointer (link[0]) 
to fill statistics for non-ML stations.

> 
>>> Why not pass multiple pointers?
>>
>> Sorry, I didn't get you.
>> Did you mean keep the sinfo structure and call this for filling all link
>> level, non-mL, ML information?
> 
> I think I meant that we could have per link pointers passed to the
> method, but we might as well just have them in the sinfo struct as you
> introduce in later patches.

Sure, will introduce the link pointer here and use link[0] for non-ML 
stations and corresponding link pointer for ML stations.

For drivers that offload link decisions and do not provide per-link 
statistics, a flag could be used to indicate whether the per-link 
statistics is filled from driver or not, if not can use link[0] to fill 
the station information and if yes then can use the corresponding link 
pointers to fill link level information.

> 
>>>> -	if (!(sinfo->filled & BIT_ULL(NL80211_STA_INFO_TX_BITRATE)) &&
>>>> +	if (!(link_sinfo->filled & BIT_ULL(NL80211_STA_INFO_TX_BITRATE)) &&
>>>>
>>>
>>> A little less renaming would probably also make this easier to review.
>>
>> Sure, but this is corresponding to filling link level data, that's why
>> just renaming sinfo to link_sinfo.
> 
> Yeah, but does it matter so much? :)

Not really, this is just done to differentiate the structures 
used(station_info or link_station_info) and code readability purpose.

> 
> I guess the flipside is that this way it's easier to review that all
> values fall into the right place, but I think the data structure
> organisation you did mostly ensures that anyway? Not sure though.

Yes from data structure organization it is taken care that it falls at 
right place.
If it found to be difficult to review and required, I can rename to sinfo?

> 
> johannes