Re: [PATCH 1/2] cfg80211: Add support to notify station's opmode change to userspace

[Tamizh chelvam <tamizhr@codeaurora.org>]

Hi Johannes,

Thanks for the code review comments, I'll address these comments and 
send V2 patchset.

>> 
>>  /**
>> + * enum wiphy_opmode_info_flag - opmode information flags
>> + *
>> + * @MAX_BW_CHANGED: Max Bandwidth changed
>> + * @SMPS_MODE_CHANGED: SMPS mode changed
>> + * @N_SS_CHANGED: max N_SS (number of spatial streams) changed
>> + *
>> + */
>> +enum wiphy_opmode_info_flag {
>> +	MAX_BW_CHANGED		= BIT(0),
>> +	SMPS_MODE_CHANGED	= BIT(1),
>> +	N_SS_CHANGED		= BIT(2),
>> +};
> 
> These need to have some kind of common prefix, e.g.
> STA_OPMODE_{BW,SMPS,NSS}_CHANGED
> 
> 
>>  /**
>> + * cfg80211_sta_opmode_change_notify - Station's SMPS mode, rx_nss 
>> and
>> + *	max bandwidth change event
> 
> need to find a shorter description - must fit on one line
> 
>> + * @dev: network device
>> + * @mac: MAC address of a station which opmode got modified
>> + * @changed: contains value from &enum wiphy_opmode_info_flag
>> + * @smps_mode: New SMPS mode of a station
>> + * @bw: new max bandwidth value of a station
>> + * @rx_nss: new rx_nss value of a station
>> + * @gfp: context flags
>> + *
>> + * This function is called when station's opmode modified via action 
>> frame.
> 
> Rephrase, say "Drivers should call this function when ..."
> 
>> +void cfg80211_sta_opmode_change_notify(struct net_device *dev, const 
>> u8 *mac,
>> +				       enum wiphy_opmode_info_flag changed,
>> +				       u8 smps_mode, u8 bw, u8 rx_nss,
>> +				       gfp_t gfp);
> 
> We may not want to add more, but pulling out those parameters "changed,
> smps_mode, bw, rx_nss" into a small structure would IMHO be a good
> idea.
> 
>> + * @NL80211_CMD_STA_OPMODE_CHANGED: An event that notify station's
>> + *	ht opmode or vht opmode changes. This will use 
>> &NL80211_ATTR_SMPS_MODE,
>> + *	&NL80211_ATTR_CHANNEL_WIDTH, &NL80211_ATTR_NSS to send the event 
>> to
>> + *	userspace.
> 
> Should document that not all of those attributes may be included at all
> times.
> 
>> +void cfg80211_sta_opmode_change_notify(struct net_device *dev, const 
>> u8 *mac,
>> +				       enum wiphy_opmode_info_flag changed,
>> +				       u8 smps, u8 bw, u8 rx_nss, gfp_t gfp)
>> +{
>> +	struct sk_buff *msg;
>> +	struct wireless_dev *wdev = dev->ieee80211_ptr;
>> +	struct cfg80211_registered_device *rdev = 
>> wiphy_to_rdev(wdev->wiphy);
>> +	void *hdr;
>> +
>> +	if (!mac)
>> +		return;
> 
> WARN_ON(), perhaps
> 
>> +	if (mac && nla_put(msg, NL80211_ATTR_MAC, ETH_ALEN, mac))
>> +		goto nla_put_failure;
> 
> no need to check mac != NULL again
> 
>> +nla_put_failure:
>> +	genlmsg_cancel(msg, hdr);
> 
> no need for cancel when you're going to free the message
> 
> johannes