Re: [PATCH wireless-next v4 14/21] wifi: mac80211: add driver_op for SMD substate changes

[Pooventhiran G <pooventhiran.g@oss.qualcomm.com>]

On 11/11/2026 02:08 am, Johannes Berg wrote:
> On Fri, 2026-11-06 at 13:28 +0530, Pooventhiran G wrote:
>>
>> Valid transition order:
>>   NONE  -> PREP  (ST preparation completed at the target AP MLD)
>>   NONE  -> EXEC  (ST execution in progress at the current AP MLD)
>>   EXEC  -> DRAIN (DL drain period started at the current AP MLD)
> 
> So ...
> 
>> + * @sta_smd_state: Notifies low level driver about IEEE 802.11bn SMD substate
>> + *	transition of a station. It must not fail while transitioning down.
> 
> What does "down" then mean?
> 

Down is just resetting to NONE but I will capture that in the commit to avoid
this ambiguity.

> And again also - "SMD state" not really, more like "ST state"? If you
> want to take things literally, then the state with the SMD is always 4
> anyway.
> 

I will change this to st_state.

>>  	if (params->smd_params.smd_sta)
>>  		sta->sta.smd_params = params->smd_params;
>>  
>> -	/*
>> -	 * SMD BSS Transition sub-states (IEEE P802.11bn/D2.0, Aug 2026,
>> -	 * subclause 11.3.1).
>> -	 * Only meaningful for SMD-capable stations (WLAN_STA_SMD set).
>> -	 */
>> -	if (test_sta_flag(sta, WLAN_STA_SMD)) {
>> -		ret = sta_apply_smd_state_flags(sta, mask, set);
>> -		if (ret)
>> -			return ret;
>> -	}
>> -
>>  	/* mark TDLS channel switch support, if the AP allows it */
>>  	if (test_sta_flag(sta, WLAN_STA_TDLS_PEER) &&
>>  	    !sdata->deflink.u.mgd.tdls_chan_switch_prohibited &&
>> @@ -2954,6 +2963,17 @@ static int sta_apply_parameters(struct ieee80211_local *local,
>>  			return ret;
>>  	}
>>  
>> +	/*
>> +	 * SMD BSS Transition sub-states (IEEE P802.11bn/D2.0, Aug 2026,
>> +	 * subclause 11.3.1).
>> +	 * Only meaningful for SMD-capable stations (WLAN_STA_SMD set).
>> +	 */
>> +	if (test_sta_flag(sta, WLAN_STA_SMD)) {
>> +		ret = sta_apply_smd_state_flags(sta, mask, set);
>> +		if (ret)
>> +			return ret;
>> +	}
> 
> That's crazy, don't add it in the wrong place first?
> 

I will fix this.

>> @@ -3038,6 +3058,19 @@ static int ieee80211_add_station(struct wiphy *wiphy, struct wireless_dev *wdev,
>>  	 */
>>  	if (!err && sdata->vif.type == NL80211_IFTYPE_NAN_DATA)
>>  		ieee80211_nan_update_ndi_carrier(sta->sdata);
>> +
>> +	/*
>> +	 * SMD sub-state was set in sta_apply_parameters() before the station
>> +	 * was inserted so drv_sta_smd_state() was skipped (WLAN_STA_INSERTED
>> +	 * was not yet set). Walk the driver step-by-step to the target state.
>> +	 */
> 
> What does that actually explain that's not in the three lines of code
> below it? Did an LLM write it?
> 

I thought it would be clear to have it captured in the comment but it already
uses !err from sta_info_insert (which I missed). I will clean up this comment.

>> +void sta_info_reset_smd_state(struct sta_info *sta, bool notify)
>> +{
>> +	if (sta->smd_state == IEEE80211_STA_SMD_NONE)
>> +		return;
>> +
>> +	/* DRAIN -> NONE, EXEC -> NONE or PREP -> NONE */
>> +	WARN_ON_ONCE(sta_info_move_smd_state(sta, IEEE80211_STA_SMD_NONE,
>> +					     notify));
> 
> How is that even useful? If we're talking about a sub-state while in
> state 4, then surely moving out of state 4 simply invalidates that, why
> bother?
> 

True, thought I could clean up the roam states before that exact invalidation
happens. I will remove it from state 4->3 transition.

>> +	clear_sta_flag(sta, WLAN_STA_SMD_PREP_TARGET);
>> +	clear_sta_flag(sta, WLAN_STA_SMD_EXEC_CURRENT);
>> +	clear_sta_flag(sta, WLAN_STA_SMD_DL_DRAIN);
> 
> Understand that you need to clear the stale flags, but for the driver
> that just seems strange?
> 

So, this is also called from state 4->3 transition so clearing the stale flags
seems necessary. For the drv_sta_smd_state op as well, if we don't clear the
state in mac80211, there will be inconsistency between driver and mac80211.

>> +	case IEEE80211_STA_SMD_EXEC:
>> +	case IEEE80211_STA_SMD_DRAIN:
>> +		err = drv_sta_smd_state(local, sta->sdata, &sta->sta,
>> +					IEEE80211_STA_SMD_NONE,
>> +					IEEE80211_STA_SMD_EXEC);
>> +		if (err)
>> +			goto rollback;
>> +
>> +		if (new_ss == IEEE80211_STA_SMD_DRAIN)
>> +			err = drv_sta_smd_state(local, sta->sdata, &sta->sta,
>> +						IEEE80211_STA_SMD_EXEC,
>> +						IEEE80211_STA_SMD_DRAIN);
>> +		break;
> 
> you do realize "fallthrough" exists?
> 

I am afraid I am not able to understand how fallthrough helps here. Using that
will return failure even for a successful driver transition. Breaking from
here, based on err, will either return success or rollback flags/states.

>> +	default:
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (!err)
>> +		return 0;
>> +
>> +	/* if EXEC -> DRAIN failed in the driver, notify EXEC -> NONE */
>> +	WARN_ON_ONCE(drv_sta_smd_state(local, sta->sdata, &sta->sta,
>> +				       IEEE80211_STA_SMD_EXEC,
>> +				       IEEE80211_STA_SMD_NONE));
> 
> I'm not a fan of sticking whole blocks of code into a WARN, but I'm also
> not sure this whole "model what should be a one-way sub-state based on
> sta_state" was a good idea in this patch.
>
Could you please kindly clarify "model what should be a one-way sub-state
based on sta_state"? Would you prefer if the driver was able to directly
update the state as well?

> If this _fails_ in the driver, how do we even handle it? Should it even
> be allowed to fail? What's the point? Worst case we _technically_ break
> the spec and continue accepting frames from the STA in the wrong
> substate, but otherwise nothing much happens?
> 

There is no handling for the failure as it is not supposed to fail, similar to
sta_state. But if at all, driver returns a failure, can we simply ignore that
err unlike how a sta_state WARNs?

>> + * @smd_state: Sub-state for STAs in SMD BSS Transition
> 
> st_state, or so?
> 

Yes, I will change it to st_state.

> johannes