On Sun, 2023-08-27 at 09:43 -0700, Jeff Johnson wrote:
> On 8/27/2023 4:05 AM, gregory.greenman@xxxxxxxxx wrote:
> > From: Emmanuel Grumbach <emmanuel.grumbach@xxxxxxxxx>
> >
> > This allows to finalize the CSA per link.
> > In case the switch didn't work, tear down the MLD connection.
> > Also pass the ieee80211_bss_conf to post_channel_switch to let the
> > driver know which link completed the switch.
> >
> > Signed-off-by: Emmanuel Grumbach <emmanuel.grumbach@xxxxxxxxx>
> > Signed-off-by: Gregory Greenman <gregory.greenman@xxxxxxxxx>
> > ---
> > .../net/wireless/intel/iwlegacy/4965-mac.c | 2 +-
> > drivers/net/wireless/intel/iwlegacy/common.c | 2 +-
> > .../net/wireless/intel/iwlwifi/dvm/mac80211.c | 6 ++--
> > .../net/wireless/intel/iwlwifi/mvm/mac-ctxt.c | 2 +-
> > .../net/wireless/intel/iwlwifi/mvm/mac80211.c | 10 +++---
> > drivers/net/wireless/intel/iwlwifi/mvm/mvm.h | 3 +-
> > .../wireless/intel/iwlwifi/mvm/time-event.c | 2 +-
> > drivers/net/wireless/ti/wlcore/event.c | 2 +-
> > drivers/net/wireless/ti/wlcore/main.c | 6 ++--
> > include/net/mac80211.h | 8 +++--
> > net/mac80211/cfg.c | 35 ++++++++++---------
> > net/mac80211/driver-ops.h | 6 ++--
> > net/mac80211/mlme.c | 29 ++++++++++-----
> ...
> > diff --git a/include/net/mac80211.h b/include/net/mac80211.h
> > index 7c707358d15c..67f54825110f 100644
> > --- a/include/net/mac80211.h
> > +++ b/include/net/mac80211.h
> > @@ -4544,7 +4544,8 @@ struct ieee80211_ops {
> > struct ieee80211_channel_switch *ch_switch);
> >
> > int (*post_channel_switch)(struct ieee80211_hw *hw,
> > - struct ieee80211_vif *vif);
> > + struct ieee80211_vif *vif,
> > + struct ieee80211_bss_conf *link_conf);
> > void (*abort_channel_switch)(struct ieee80211_hw *hw,
> > struct ieee80211_vif *vif);
> > void (*channel_switch_rx_beacon)(struct ieee80211_hw *hw,
> > @@ -6542,11 +6543,14 @@ void ieee80211_radar_detected(struct ieee80211_hw *hw);
> > * ieee80211_chswitch_done - Complete channel switch process
> > * @vif: &struct ieee80211_vif pointer from the add_interface callback.
> > * @success: make the channel switch successful or not
> > + * @link_id: the link_id on which the switch was done. Ignored if success is
> > + * false.
>
> I would not call this being ignored:
> + link = rcu_dereference(sdata->link[link_id]);
> + if (WARN_ON(!link)) {
> + rcu_read_unlock();
> + return;
> + }
>
Yeah - you're right. We don't need the link pointer in case success is false and yet we return here,
I'll fix.
> > *
> > * Complete the channel switch post-process: set the new operational channel
> > * and wake up the suspended queues.
> > */
> > -void ieee80211_chswitch_done(struct ieee80211_vif *vif, bool success);
> > +void ieee80211_chswitch_done(struct ieee80211_vif *vif, bool success,
> > + unsigned int link_id);
> >
> > /**
> > * ieee80211_channel_switch_disconnect - disconnect due to channel switch error
> ...
> > diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
> > index f93eb38ae0b8..ca6045f56b4b 100644
> > --- a/net/mac80211/mlme.c
> > +++ b/net/mac80211/mlme.c
> > @@ -1773,7 +1773,7 @@ static void ieee80211_chswitch_post_beacon(struct ieee80211_link_data
> > *link)
> > */
> > link->u.mgd.beacon_crc_valid = false;
> >
> > - ret = drv_post_channel_switch(sdata);
> > + ret = drv_post_channel_switch(link);
> > if (ret) {
> > sdata_info(sdata,
> > "driver post channel switch failed, disconnecting\n");
> > @@ -1785,25 +1785,36 @@ static void ieee80211_chswitch_post_beacon(struct ieee80211_link_data
> > *link)
> > cfg80211_ch_switch_notify(sdata->dev, &link->reserved_chandef, 0, 0);
> > }
> >
> > -void ieee80211_chswitch_done(struct ieee80211_vif *vif, bool success)
> > +void ieee80211_chswitch_done(struct ieee80211_vif *vif, bool success,
> > + unsigned int link_id)
> > {
> > + struct ieee80211_link_data *link;
> > struct ieee80211_sub_if_data *sdata = vif_to_sdata(vif);
> > - struct ieee80211_if_managed *ifmgd = &sdata->u.mgd;
> > -
> > - if (WARN_ON(ieee80211_vif_is_mld(&sdata->vif)))
> > - success = false;
> > + struct ieee80211_link_data_managed *ifmgd;
> >
> > trace_api_chswitch_done(sdata, success);
>
> no value in tracing the link_id?
I can add it.
>
> > +
> > + rcu_read_lock();
> > +
> > + link = rcu_dereference(sdata->link[link_id]);
> > + if (WARN_ON(!link)) {
> > + rcu_read_unlock();
> > + return;
> > + }
> > +
> > + ifmgd = &link->u.mgd;
> > +
> > if (!success) {
> > sdata_info(sdata,
> > "driver channel switch failed, disconnecting\n");
> > wiphy_work_queue(sdata->local->hw.wiphy,
> > - &ifmgd->csa_connection_drop_work);
> > + &sdata->u.mgd.csa_connection_drop_work);
> > } else {
> > wiphy_delayed_work_queue(sdata->local->hw.wiphy,
> > - &sdata->deflink.u.mgd.chswitch_work,
> > - 0);
> > + &ifmgd->chswitch_work, 0);
> > }
> > +
> > + rcu_read_unlock();
> > }
> > EXPORT_SYMBOL(ieee80211_chswitch_done);
> >
>
Gregory / Johannes, I'll send a patch internally to fix the issues raised here and you'll squash
before resending?
