Re: [PATCH] brcmfmac: support AP isolation to restrict reachability between stations

[Arend van Spriel <arend.vanspriel@broadcom.com>]

On 4/24/2025 12:15 AM, Johannes Berg wrote:
> On Wed, 2025-04-23 at 23:21 +0530, Gokul Sivakumar wrote:
>>
>> +static int brcmf_cfg80211_change_bss(struct wiphy *wiphy, struct net_device *dev,
>> +				     struct bss_parameters *params)
>> +{
>> +	struct brcmf_if *ifp = netdev_priv(dev);
>> +	int ret = 0;
>> +
>> +	/* In AP mode, the "ap_isolate" value represents
>> +	 *  0 = allow low-level bridging of frames between associated stations
>> +	 *  1 = restrict low-level bridging of frames to isolate associated stations
>> +	 * -1 = do not change existing setting
>> +	 */
>> +	if (params->ap_isolate >= 0) {
>> +		ret = brcmf_fil_iovar_int_set(ifp, "ap_isolate", params->ap_isolate);
>> +		if (ret < 0)
>> +			brcmf_err("ap_isolate iovar failed: ret=%d\n", ret);
>> +	}
>> +
>> +	return ret;
>> +}
> 
> Seems like a terrible idea to accept any other changes silently without
> doing anything at all.

Hi Johannes,

Agree. That would indeed give the wrong impression to user-space. 
However, what if the firmware does not support some of them that 
user-space actually want to change. Seems like we are missing a feedback 
mechanism here to inform user-space about partial failure to apply the 
requested parameters?

Looked at other drivers implementing this callback and here are the results:

[wil6210] wil_cfg80211_change_bss(): does exactly the same thing.
[wilc1000] change_bss(): worse! it accepts everything and does nothing.
[rtl8723bs] cfg80211_rtw_change_bss(): same. just an empty callback.
[mac80211] ieee80211_change_bss(): not surprising this looks pretty good

The mac80211 implementation fills a changed bitmask, but that is to 
inform the mac80211 driver what configuration changes to look for.

> Also, please pay attention to the linux-wireless list. Like, at all. We
> started using tree tags months ago, we started using a different subject
> prefix _years_ ago.

If this patch means Infineon is (mildly) regaining interest in upstream 
wifi development let's not discourage them. I do watch the 
linux-wireless list on occasion but I am a bit lost on your remark. What 
do you mean by tree tags. You mean the "wifi:" prefix? But then I am 
confused about the "subject prefix" remark.

Digging a bit further maybe you are referring to the "Tree labels" 
section [1]? I always considered a patch with only [PATCH] as being for 
-next implicitly. If it makes maintainer life easier I am happy to 
comply and add it explicit ;-)

Regards,
Arend

[1] 
https://wireless.docs.kernel.org/en/latest/en/developers/documentation/submittingpatches.html#tree-labels