Re: [PATCH 01/15] wifi: mac80211: add support for mld in ieee80211_chswitch_done

["Grumbach, Emmanuel" <emmanuel.grumbach@intel.com>]

On Sun, 2023-08-27 at 09:43 -0700, Jeff Johnson wrote:
> On 8/27/2023 4:05 AM, gregory.greenman@intel.com wrote:
> > From: Emmanuel Grumbach <emmanuel.grumbach@intel.com>
> > 
> > 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@intel.com>
> > Signed-off-by: Gregory Greenman <gregory.greenman@intel.com>
> > ---
> >   .../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?