2023-11-24 19:18:17

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH 4/6] wifi: cfg80211: Schedule regulatory check on BSS STA channel change

On Mon, 2023-11-13 at 11:35 +0200, [email protected] wrote:
>
> @@ -9302,6 +9302,17 @@ bool cfg80211_valid_disable_subchannel_bitmap(u16 *bitmap,
> * case disconnect instead.
> * Also note that the wdev mutex must be held.
> */
> +
> void cfg80211_links_removed(struct net_device *dev, u16 link_mask);

What happened there?

> +/**
> + * cfg80211_schedule_channels_check - schedule regulatory check if needed
> + * @netdev: the device to check
> + *
> + * In case the device supports NO_IR or DFS relaxations, schedule regulatory
> + * channels check, as previous concurrent operation conditions may not
> + * hold anymore.
> + */

...

> +void cfg80211_schedule_channels_check(struct net_device *netdev)
> +{
> + struct wireless_dev *wdev = netdev->ieee80211_ptr;
> + struct wiphy *wiphy = wdev->wiphy;
> +
> + /* Schedule channels check if NO_IR or DFS relaxations are supported */
> + if (wdev->iftype == NL80211_IFTYPE_STATION &&
> + (wiphy_ext_feature_isset(wiphy,
> + NL80211_EXT_FEATURE_DFS_CONCURRENT) ||
> + (IS_ENABLED(CONFIG_CFG80211_REG_RELAX_NO_IR) &&
> + wiphy->regulatory_flags & REGULATORY_ENABLE_RELAX_NO_IR)))
> + reg_check_channels();
> +}

That ... doesn't even use the netdev pointer, apart from going to the
wiphy? Why not have a wiphy argument instead?

johannes



2023-11-26 08:58:27

by Andrei Otcheretianski

[permalink] [raw]
Subject: RE: [PATCH 4/6] wifi: cfg80211: Schedule regulatory check on BSS STA channel change

> cfg80211_valid_disable_subchannel_bitmap(u16 *bitmap,
> > * case disconnect instead.
> > * Also note that the wdev mutex must be held.
> > */
> > +
> > void cfg80211_links_removed(struct net_device *dev, u16 link_mask);
>
> What happened there?

No idea, it wasn't in the original patch.. Will recheck with Gregory and fix it.

>
> > +void cfg80211_schedule_channels_check(struct net_device *netdev) {
> > + struct wireless_dev *wdev = netdev->ieee80211_ptr;
> > + struct wiphy *wiphy = wdev->wiphy;
> > +
> > + /* Schedule channels check if NO_IR or DFS relaxations are
> supported */
> > + if (wdev->iftype == NL80211_IFTYPE_STATION &&
> > + (wiphy_ext_feature_isset(wiphy,
> > +
> NL80211_EXT_FEATURE_DFS_CONCURRENT) ||
> > + (IS_ENABLED(CONFIG_CFG80211_REG_RELAX_NO_IR) &&
> > + wiphy->regulatory_flags &
> REGULATORY_ENABLE_RELAX_NO_IR)))
> > + reg_check_channels();
> > +}
>
> That ... doesn't even use the netdev pointer, apart from going to the wiphy?
> Why not have a wiphy argument instead?

We do need wdev here, I will change it to be wireless device instead

>
> johannes

2023-11-26 13:20:01

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH 4/6] wifi: cfg80211: Schedule regulatory check on BSS STA channel change

>
> >
> > > +void cfg80211_schedule_channels_check(struct net_device *netdev) {
> > > + struct wireless_dev *wdev = netdev->ieee80211_ptr;
> > > + struct wiphy *wiphy = wdev->wiphy;
> > > +
> > > + /* Schedule channels check if NO_IR or DFS relaxations are
> > supported */
> > > + if (wdev->iftype == NL80211_IFTYPE_STATION &&
> > > + (wiphy_ext_feature_isset(wiphy,
>
> We do need wdev here, 
>

Oh right, I missed that, sorry.

> I will change it to be wireless device instead
>

No need I guess, if we're only going to check for it being a station?
Would we ever have a reason to call this for a p2p/nan device? I guess
not, since that doesn't affect regulatory in the same way?

johannes

2023-11-28 08:26:47

by Andrei Otcheretianski

[permalink] [raw]
Subject: RE: [PATCH 4/6] wifi: cfg80211: Schedule regulatory check on BSS STA channel change

> > > > +void cfg80211_schedule_channels_check(struct net_device *netdev)
> {
> > > > + struct wireless_dev *wdev = netdev->ieee80211_ptr;
> > > > + struct wiphy *wiphy = wdev->wiphy;
> > > > +
> > > > + /* Schedule channels check if NO_IR or DFS relaxations are
> > > supported */
> > > > + if (wdev->iftype == NL80211_IFTYPE_STATION &&
> > > > + (wiphy_ext_feature_isset(wiphy,
> >
> > We do need wdev here,
> >
>
> Oh right, I missed that, sorry.
>
> > I will change it to be wireless device instead
> >
>
> No need I guess, if we're only going to check for it being a station?
> Would we ever have a reason to call this for a p2p/nan device? I guess
> not, since that doesn't affect regulatory in the same way?

No, I don't think P2P/NAN devices can anyhow affect regulatory, for sure not for concurrent DFS case.

>
> johannes