2024-02-15 20:16:20

by Johannes Berg

Subject: Re: [PATCH 1/2] wifi: mac80211: check beacon countdown is complete on per link basis

> -bool ieee80211_beacon_cntdwn_is_complete(struct ieee80211_vif *vif)
> +bool ieee80211_beacon_cntdwn_is_complete(struct ieee80211_vif *vif,
> + unsigned int link_id)
> {
> struct ieee80211_sub_if_data *sdata = vif_to_sdata(vif);
> + struct ieee80211_link_data *link;
> struct beacon_data *beacon = NULL;
> u8 *beacon_data;
> size_t beacon_data_len;
> @@ -5106,9 +5108,17 @@ bool ieee80211_beacon_cntdwn_is_complete(struct ieee80211_vif *vif)
> if (!ieee80211_sdata_running(sdata))
> return false;
>
> + if (WARN_ON(link_id >= IEEE80211_MLD_MAX_NUM_LINKS))
> + return 0;
> +
> rcu_read_lock();
> +
> + link = rcu_dereference(sdata->link[link_id]);
> + if (!link)
> + goto out;
>

Maybe that should be a warning too? Not sure I see any case where the
driver can/should call it with a link that's not even there?

Oh ... and maybe it should check if the link is active? We had the
sdata_running() check before, but that doesn't mean much for MLO?

Though then again we have the check below anyway:

> if (vif->type == NL80211_IFTYPE_AP) {
> - beacon = rcu_dereference(sdata->deflink.u.ap.beacon);
> + beacon = rcu_dereference(link->u.ap.beacon);
> if (WARN_ON(!beacon || !beacon->tail))
> goto out;
>

So that will just be NULL if it's not active... so I guess that's fine.

johannes

2024-02-16 03:04:52

by Aditya Kumar Singh

Subject: Re: [PATCH 1/2] wifi: mac80211: check beacon countdown is complete on per link basis

On 2/16/24 01:18, Johannes Berg wrote:
>
>> -bool ieee80211_beacon_cntdwn_is_complete(struct ieee80211_vif *vif)
>> +bool ieee80211_beacon_cntdwn_is_complete(struct ieee80211_vif *vif,
>> + unsigned int link_id)
>> {
>> struct ieee80211_sub_if_data *sdata = vif_to_sdata(vif);
>> + struct ieee80211_link_data *link;
>> struct beacon_data *beacon = NULL;
>> u8 *beacon_data;
>> size_t beacon_data_len;
>> @@ -5106,9 +5108,17 @@ bool ieee80211_beacon_cntdwn_is_complete(struct ieee80211_vif *vif)
>> if (!ieee80211_sdata_running(sdata))
>> return false;
>>
>> + if (WARN_ON(link_id >= IEEE80211_MLD_MAX_NUM_LINKS))
>> + return 0;
>> +
>> rcu_read_lock();
>> +
>> + link = rcu_dereference(sdata->link[link_id]);
>> + if (!link)
>> + goto out;
>>
>
> Maybe that should be a warning too? Not sure I see any case where the
> driver can/should call it with a link that's not even there?
>

Yes even I don't see any case like that.

> Oh ... and maybe it should check if the link is active? We had the
> sdata_running() check before, but that doesn't mean much for MLO?
>

Correct. But at least in this function I don't see much use of checking
if link is active.

> Though then again we have the check below anyway:
>
>> if (vif->type == NL80211_IFTYPE_AP) {
>> - beacon = rcu_dereference(sdata->deflink.u.ap.beacon);
>> + beacon = rcu_dereference(link->u.ap.beacon);
>> if (WARN_ON(!beacon || !beacon->tail))
>> goto out;
>>
>
> So that will just be NULL if it's not active... so I guess that's fine.
>
Yep. That's the intention here. Ultimately beacon would be NULL if link
is not active so eventually a WARN_ON() will trigger.