[v2] wfx: implement Remain On Channel

[PATCH v2 5/9] wifi: wfx: add placeholders for remain_on_channel feature

Posted by Jérôme Pouiller 2 years, 4 months ago

First step to implement remain_on_channel.

Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
---
 drivers/net/wireless/silabs/wfx/main.c |  3 +++
 drivers/net/wireless/silabs/wfx/scan.c | 12 ++++++++++++
 drivers/net/wireless/silabs/wfx/scan.h |  5 +++++
 3 files changed, 20 insertions(+)

diff --git a/drivers/net/wireless/silabs/wfx/main.c b/drivers/net/wireless/silabs/wfx/main.c
index ede822d771aaf..31f6e0d3dc089 100644
--- a/drivers/net/wireless/silabs/wfx/main.c
+++ b/drivers/net/wireless/silabs/wfx/main.c
@@ -151,6 +151,8 @@ static const struct ieee80211_ops wfx_ops = {
 	.change_chanctx          = wfx_change_chanctx,
 	.assign_vif_chanctx      = wfx_assign_vif_chanctx,
 	.unassign_vif_chanctx    = wfx_unassign_vif_chanctx,
+	.remain_on_channel       = wfx_remain_on_channel,
+	.cancel_remain_on_channel = wfx_cancel_remain_on_channel,
 };
 
 bool wfx_api_older_than(struct wfx_dev *wdev, int major, int minor)
@@ -288,6 +290,7 @@ struct wfx_dev *wfx_init_common(struct device *dev, const struct wfx_platform_da
 	hw->wiphy->features |= NL80211_FEATURE_AP_SCAN;
 	hw->wiphy->flags |= WIPHY_FLAG_AP_PROBE_RESP_OFFLOAD;
 	hw->wiphy->flags |= WIPHY_FLAG_AP_UAPSD;
+	hw->wiphy->max_remain_on_channel_duration = 5000;
 	hw->wiphy->max_ap_assoc_sta = HIF_LINK_ID_MAX;
 	hw->wiphy->max_scan_ssids = 2;
 	hw->wiphy->max_scan_ie_len = IEEE80211_MAX_DATA_LEN;
diff --git a/drivers/net/wireless/silabs/wfx/scan.c b/drivers/net/wireless/silabs/wfx/scan.c
index 16f619ed22e00..51338fd43ae4f 100644
--- a/drivers/net/wireless/silabs/wfx/scan.c
+++ b/drivers/net/wireless/silabs/wfx/scan.c
@@ -145,3 +145,15 @@ void wfx_scan_complete(struct wfx_vif *wvif, int nb_chan_done)
 	wvif->scan_nb_chan_done = nb_chan_done;
 	complete(&wvif->scan_complete);
 }
+
+int wfx_remain_on_channel(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
+			  struct ieee80211_channel *chan, int duration,
+			  enum ieee80211_roc_type type)
+{
+	return 0;
+}
+
+int wfx_cancel_remain_on_channel(struct ieee80211_hw *hw, struct ieee80211_vif *vif)
+{
+	return 0;
+}
diff --git a/drivers/net/wireless/silabs/wfx/scan.h b/drivers/net/wireless/silabs/wfx/scan.h
index 78e3b984f375c..2f8361769303e 100644
--- a/drivers/net/wireless/silabs/wfx/scan.h
+++ b/drivers/net/wireless/silabs/wfx/scan.h
@@ -19,4 +19,9 @@ int wfx_hw_scan(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
 void wfx_cancel_hw_scan(struct ieee80211_hw *hw, struct ieee80211_vif *vif);
 void wfx_scan_complete(struct wfx_vif *wvif, int nb_chan_done);
 
+int wfx_remain_on_channel(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
+			  struct ieee80211_channel *chan, int duration,
+			  enum ieee80211_roc_type type);
+int wfx_cancel_remain_on_channel(struct ieee80211_hw *hw, struct ieee80211_vif *vif);
+
 #endif
-- 
2.39.2

Re: [PATCH v2 5/9] wifi: wfx: add placeholders for remain_on_channel feature

Posted by Kalle Valo 2 years, 4 months ago

Jérôme Pouiller <jerome.pouiller@silabs.com> writes:

> First step to implement remain_on_channel.
>
> Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
> ---
>  drivers/net/wireless/silabs/wfx/main.c |  3 +++
>  drivers/net/wireless/silabs/wfx/scan.c | 12 ++++++++++++
>  drivers/net/wireless/silabs/wfx/scan.h |  5 +++++
>  3 files changed, 20 insertions(+)
>
> diff --git a/drivers/net/wireless/silabs/wfx/main.c b/drivers/net/wireless/silabs/wfx/main.c
> index ede822d771aaf..31f6e0d3dc089 100644
> --- a/drivers/net/wireless/silabs/wfx/main.c
> +++ b/drivers/net/wireless/silabs/wfx/main.c
> @@ -151,6 +151,8 @@ static const struct ieee80211_ops wfx_ops = {
>  	.change_chanctx          = wfx_change_chanctx,
>  	.assign_vif_chanctx      = wfx_assign_vif_chanctx,
>  	.unassign_vif_chanctx    = wfx_unassign_vif_chanctx,
> +	.remain_on_channel       = wfx_remain_on_channel,
> +	.cancel_remain_on_channel = wfx_cancel_remain_on_channel,
>  };
>  
>  bool wfx_api_older_than(struct wfx_dev *wdev, int major, int minor)
> @@ -288,6 +290,7 @@ struct wfx_dev *wfx_init_common(struct device *dev, const struct wfx_platform_da
>  	hw->wiphy->features |= NL80211_FEATURE_AP_SCAN;
>  	hw->wiphy->flags |= WIPHY_FLAG_AP_PROBE_RESP_OFFLOAD;
>  	hw->wiphy->flags |= WIPHY_FLAG_AP_UAPSD;
> +	hw->wiphy->max_remain_on_channel_duration = 5000;
>  	hw->wiphy->max_ap_assoc_sta = HIF_LINK_ID_MAX;
>  	hw->wiphy->max_scan_ssids = 2;
>  	hw->wiphy->max_scan_ie_len = IEEE80211_MAX_DATA_LEN;
> diff --git a/drivers/net/wireless/silabs/wfx/scan.c b/drivers/net/wireless/silabs/wfx/scan.c
> index 16f619ed22e00..51338fd43ae4f 100644
> --- a/drivers/net/wireless/silabs/wfx/scan.c
> +++ b/drivers/net/wireless/silabs/wfx/scan.c
> @@ -145,3 +145,15 @@ void wfx_scan_complete(struct wfx_vif *wvif, int nb_chan_done)
>  	wvif->scan_nb_chan_done = nb_chan_done;
>  	complete(&wvif->scan_complete);
>  }
> +
> +int wfx_remain_on_channel(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
> +			  struct ieee80211_channel *chan, int duration,
> +			  enum ieee80211_roc_type type)
> +{
> +	return 0;
> +}
> +
> +int wfx_cancel_remain_on_channel(struct ieee80211_hw *hw, struct ieee80211_vif *vif)
> +{
> +	return 0;
> +}
> diff --git a/drivers/net/wireless/silabs/wfx/scan.h b/drivers/net/wireless/silabs/wfx/scan.h
> index 78e3b984f375c..2f8361769303e 100644
> --- a/drivers/net/wireless/silabs/wfx/scan.h
> +++ b/drivers/net/wireless/silabs/wfx/scan.h
> @@ -19,4 +19,9 @@ int wfx_hw_scan(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
>  void wfx_cancel_hw_scan(struct ieee80211_hw *hw, struct ieee80211_vif *vif);
>  void wfx_scan_complete(struct wfx_vif *wvif, int nb_chan_done);
>  
> +int wfx_remain_on_channel(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
> +			  struct ieee80211_channel *chan, int duration,
> +			  enum ieee80211_roc_type type);
> +int wfx_cancel_remain_on_channel(struct ieee80211_hw *hw, struct ieee80211_vif *vif);
> +
>  #endif

I'm not really seeing the point of this patch. I would expect that once
.remain_on_channel is assign the feature will work without issues, for
example otherwise git bisect will not work correctly.

What about folding patches 5 and 6 into one patch? And then moving that
patch as the last to make sure that the feature is enabled on the driver
only after it works correctly?

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH v2 5/9] wifi: wfx: add placeholders for remain_on_channel feature

Posted by Jérôme Pouiller 2 years, 4 months ago

On Wednesday 4 October 2023 12:23:30 CEST Kalle Valo wrote:
> Jérôme Pouiller <jerome.pouiller@silabs.com> writes:
> 
> > First step to implement remain_on_channel.
> >
> > Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
> > ---
> >  drivers/net/wireless/silabs/wfx/main.c |  3 +++
> >  drivers/net/wireless/silabs/wfx/scan.c | 12 ++++++++++++
> >  drivers/net/wireless/silabs/wfx/scan.h |  5 +++++
> >  3 files changed, 20 insertions(+)
> >
> > diff --git a/drivers/net/wireless/silabs/wfx/main.c b/drivers/net/wireless/silabs/wfx/main.c
> > index ede822d771aaf..31f6e0d3dc089 100644
> > --- a/drivers/net/wireless/silabs/wfx/main.c
> > +++ b/drivers/net/wireless/silabs/wfx/main.c
> > @@ -151,6 +151,8 @@ static const struct ieee80211_ops wfx_ops = {
> >       .change_chanctx          = wfx_change_chanctx,
> >       .assign_vif_chanctx      = wfx_assign_vif_chanctx,
> >       .unassign_vif_chanctx    = wfx_unassign_vif_chanctx,
> > +     .remain_on_channel       = wfx_remain_on_channel,
> > +     .cancel_remain_on_channel = wfx_cancel_remain_on_channel,
> >  };
> >
> >  bool wfx_api_older_than(struct wfx_dev *wdev, int major, int minor)
> > @@ -288,6 +290,7 @@ struct wfx_dev *wfx_init_common(struct device *dev, const struct wfx_platform_da
> >       hw->wiphy->features |= NL80211_FEATURE_AP_SCAN;
> >       hw->wiphy->flags |= WIPHY_FLAG_AP_PROBE_RESP_OFFLOAD;
> >       hw->wiphy->flags |= WIPHY_FLAG_AP_UAPSD;
> > +     hw->wiphy->max_remain_on_channel_duration = 5000;
> >       hw->wiphy->max_ap_assoc_sta = HIF_LINK_ID_MAX;
> >       hw->wiphy->max_scan_ssids = 2;
> >       hw->wiphy->max_scan_ie_len = IEEE80211_MAX_DATA_LEN;
> > diff --git a/drivers/net/wireless/silabs/wfx/scan.c b/drivers/net/wireless/silabs/wfx/scan.c
> > index 16f619ed22e00..51338fd43ae4f 100644
> > --- a/drivers/net/wireless/silabs/wfx/scan.c
> > +++ b/drivers/net/wireless/silabs/wfx/scan.c
> > @@ -145,3 +145,15 @@ void wfx_scan_complete(struct wfx_vif *wvif, int nb_chan_done)
> >       wvif->scan_nb_chan_done = nb_chan_done;
> >       complete(&wvif->scan_complete);
> >  }
> > +
> > +int wfx_remain_on_channel(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
> > +                       struct ieee80211_channel *chan, int duration,
> > +                       enum ieee80211_roc_type type)
> > +{
> > +     return 0;
> > +}
> > +
> > +int wfx_cancel_remain_on_channel(struct ieee80211_hw *hw, struct ieee80211_vif *vif)
> > +{
> > +     return 0;
> > +}
> > diff --git a/drivers/net/wireless/silabs/wfx/scan.h b/drivers/net/wireless/silabs/wfx/scan.h
> > index 78e3b984f375c..2f8361769303e 100644
> > --- a/drivers/net/wireless/silabs/wfx/scan.h
> > +++ b/drivers/net/wireless/silabs/wfx/scan.h
> > @@ -19,4 +19,9 @@ int wfx_hw_scan(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
> >  void wfx_cancel_hw_scan(struct ieee80211_hw *hw, struct ieee80211_vif *vif);
> >  void wfx_scan_complete(struct wfx_vif *wvif, int nb_chan_done);
> >
> > +int wfx_remain_on_channel(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
> > +                       struct ieee80211_channel *chan, int duration,
> > +                       enum ieee80211_roc_type type);
> > +int wfx_cancel_remain_on_channel(struct ieee80211_hw *hw, struct ieee80211_vif *vif);
> > +
> >  #endif
> 
> I'm not really seeing the point of this patch. I would expect that once
> .remain_on_channel is assign the feature will work without issues, for
> example otherwise git bisect will not work correctly.
> 
> What about folding patches 5 and 6 into one patch? And then moving that
> patch as the last to make sure that the feature is enabled on the driver
> only after it works correctly?

Ok.

I think I will have to reword a bit the commit logs but the reordering should
not be difficult.


-- 
Jérôme Pouiller

[PATCH v2 1/9] wifi: wfx: fix power_save setting when AP is stopped
[PATCH v2 2/9] wifi: wfx: relocate wfx_rate_mask_to_hw()
[PATCH v2 3/9] wifi: wfx: move wfx_skb_*() out of the header file
[PATCH v2 4/9] wifi: wfx: introduce hif_scan_uniq()
[PATCH v2 5/9] wifi: wfx: add placeholders for remain_on_channel feature
[PATCH v2 6/9] wifi: wfx: implement wfx_remain_on_channel()
[PATCH v2 7/9] wifi: wfx: allow to send frames during ROC
[PATCH v2 8/9] wifi: wfx: scan_lock is global to the device
[PATCH v2 9/9] wifi: wfx: fix possible lock-up between scan and Rx filters