Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <ad9a0e601a5e7dd2642be9ae8b17c1408a8bd1d1.camel@sipsolutions.net>
Subject: Re: [PATCH 1/1] cfg80211: Update Transition Disable policy during
 port authorization
From:   Johannes Berg <johannes@sipsolutions.net>
To:     Vinayak Yadawad <vinayak.yadawad@broadcom.com>
Cc:     linux-wireless@vger.kernel.org, jithu.jance@broadcom.com
Date:   Fri, 26 Aug 2022 12:07:08 +0200
In-Reply-To: <a408102ee0917de77ecb3a0b63cd7c650ba93750.1661506891.git.vinayak.yadawad@broadcom.com>
References: <a408102ee0917de77ecb3a0b63cd7c650ba93750.1661506891.git.vinayak.yadawad@broadcom.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.44.4 (3.44.4-1.fc36) 
MIME-Version: 1.0
Precedence: bulk

> In case of 4way handshake offload, transition disable policy
> updated by the AP during EAPOL 3/4 is not updated to the cfg80211
> layer. This results in mismatch between transition disable policy
> between the upper layer and the driver


I find this a bit confusingly worded - cfg80211 doesn't care, so it
doesn't need to be updated _in_ cfg80211. "To" cfg80211 sounds a bit
weird. Maybe just phrase that in terms of userspace?

> +++ b/include/net/cfg80211.h
> @@ -7669,7 +7669,7 @@ void cfg80211_roamed(struct net_device *dev, struct=
 cfg80211_roam_info *info,
>   * indicate the 802.11 association.
>   */
>  void cfg80211_port_authorized(struct net_device *dev, const u8 *bssid,
> -			      gfp_t gfp);
> +			      const u8 td_bitmap, gfp_t gfp);

Missing docs.

Also missing the corresponding driver update, even if it should just
pass 0 for now in this patch to not have a lot of new driver logic here?

Or maybe it should be valid to pass -1 (i.e. make it a s16) to avoid
passing it to userspace when the driver doesn't know?

Also no point in marking the parameter const.
=20
>=20
> +++ b/include/uapi/linux/nl80211.h
> @@ -3268,6 +3268,7 @@ enum nl80211_attrs {
> =20
>  	NL80211_ATTR_TX_HW_TIMESTAMP,
>  	NL80211_ATTR_RX_HW_TIMESTAMP,
> +	NL80211_ATTR_TD_BITMAP,

Also missing docs.

>  void nl80211_send_port_authorized(struct cfg80211_registered_device *rde=
v,
> -				  struct net_device *netdev, const u8 *bssid)
> +				  struct net_device *netdev, const u8 *bssid,
> +				  const u8 td_bitmap)


same here about const

>  	if (nla_put_u32(msg, NL80211_ATTR_WIPHY, rdev->wiphy_idx) ||
>  	    nla_put_u32(msg, NL80211_ATTR_IFINDEX, netdev->ifindex) ||
> -	    nla_put(msg, NL80211_ATTR_MAC, ETH_ALEN, bssid))
> +	    nla_put(msg, NL80211_ATTR_MAC, ETH_ALEN, bssid) ||
> +	    nla_put_u8(msg, NL80211_ATTR_TD_BITMAP, td_bitmap))

and yeah maybe here check for <0 or something if it should sometimes
result in a missing attribute?

johannes