Search Linux Wireless

Re: [PATCH 1/1] mac80211: adding bss_config to low driver ops

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 10/18/07, Johannes Berg <johannes@xxxxxxxxxxxxxxxx> wrote:
> On Thu, 2007-10-18 at 01:51 +0200, Tomas Winkler wrote:
>
> > +/* next structs enable the mac80211 to send association notification
> > + * to low-level driver, as well as data that may be changed through
> > + * the lifetime of the BSS, after it was parsed to meaningful structs */
> > +struct ieee80211_erp_info {
> > +     u8 changes;
> > +     int cts_protection;
> > +     int preamble;
> > +};
> > +
> > +struct ieee80211_wmm_info {
> > +     int queue;
> > +     struct ieee80211_tx_queue_params *params;
> > +};
>
> Please do kernel-doc style comments for both structures so I can
> incorporate this information easily into the mac80211 book. Also, some
> general guidelines in a DOC comment would be appreciated, like
>
> /**
>  * DOC: BSS Status changes
>  *
>  * Describe what is expected of driver here
>  */
>

We'll do

> > +struct ieee80211_bss_data {
> > +     u8 changed_map; /* use ASSOC_CHNAGED_.. to indicate changed element */
> > +     u8 assoc; /* 0 not assoc, 1 assoc */
> > +     u16 aid;
> > +     struct ieee80211_erp_info erp_info;
> > +     struct ieee80211_wmm_info wmm_info;
> > +};
>
> Again, kernel-doc; also why do we actually need the sub-structuring into
> erp_info and wmm_info (and if you manage convince me that we do :), can
> we please call them just 'erp' and 'wmm' to save typing)?
>
This was just to not break all the drivers but you are right these
structures won't be needed

> This seems pretty ad-hoc to me. I think it'd be better to have them all
> in one structure and use just a single 'changed' parameter instead of
> having an extra one in the erp info. I would also use a 'flags'
> parameter instead of 'assoc' and then roll CTS protect and short
> preamble into those flags.
>
The bits in flags are possible just sometimes you need to pass a value
such as assoc id
or queue num so it will be a little mess.

> Also, the ASSOC_CHANGED_* flags should be renamed to BSS_STATE_CHANGED_*
> or so, they don't really affect just the association.
>
Fair enough

> Hmm. And here's a thought: don't we need all this information within
> mac80211 as well? Would it maybe make sense to embed it into the sta
> sdata structure and have the 'changed_map' not be in the structure but
> as a second parameter to the callback function? Then we could always use
> a pointer to the embedded structure that keeps track of this information
> and build the changed value dynamically. This saves having to initialise
> the structure all the time when calling the function and makes sure that
> even the unchanged parameters are always valid should the driver need
> them. [also see at the very end of this mail]
>
Sounds good
> > @@ -1021,6 +1048,8 @@ struct ieee80211_ops {
> >       int (*config)(struct ieee80211_hw *hw, struct ieee80211_conf *conf);
> >       int (*config_interface)(struct ieee80211_hw *hw,
> >                               int if_id, struct ieee80211_if_conf *conf);
> > +     void (*bss_info_changed)(struct ieee80211_hw *hw,
> > +                              struct ieee80211_bss_data *bss_data);
>
> Missing doc updates, but see above. I'd prefer the docs in this struct
> to just be a short note what the handler is and what the context is when
> it is invoked (like "must be atomic" or "runs under XY lock") and then
> for more details we refer to a DOC: section as I said above. That makes
> it much easier to collate it into the mac80211 book.
>
> > diff --git a/net/mac80211/ieee80211_sta.c b/net/mac80211/ieee80211_sta.c
> > index 7c93f29..f38eb5a 100644
> > --- a/net/mac80211/ieee80211_sta.c
> > +++ b/net/mac80211/ieee80211_sta.c
> > @@ -408,56 +408,66 @@ static void ieee80211_sta_send_associnfo(struct net_device *dev,
> >
> >  static void ieee80211_set_associated(struct net_device *dev,
> >                                    struct ieee80211_if_sta *ifsta,
> > -                                  bool assoc)
> > +                                  struct ieee80211_bss_data *bss_data)
> >  {
> >       struct ieee80211_local *local = wdev_priv(dev->ieee80211_ptr);
> >       union iwreq_data wrqu;
> >
> > -     if (!!(ifsta->flags & IEEE80211_STA_ASSOCIATED) == assoc)
> > +     if (!bss_data->changed_map)
> >               return;
>
> If we did the embedding thing then all this would probably be simpler,
> we could keep the 'bool assoc' parameter and have this function update
> the [read on after the quote]
>
> > -     if (assoc) {
> > -             struct ieee80211_sub_if_data *sdata;
> > -             struct ieee80211_sta_bss *bss;
> > +     if (bss_data->changed_map & ASSOC_CHNAGED_STATE) {
> > +             if (bss_data->assoc) {
> > +                     struct ieee80211_sub_if_data *sdata;
> > +                     struct ieee80211_sta_bss *bss;
> >
> > -             ifsta->flags |= IEEE80211_STA_ASSOCIATED;
> > +                     ifsta->flags |= IEEE80211_STA_ASSOCIATED;
>
> flag within the embedded struct instead of having another flag that
> keeps track of the same information.
>
> > -             bss = ieee80211_rx_bss_get(dev, ifsta->bssid);
> > -             if (bss) {
> > -                     if (bss->has_erp_value)
> > -                             ieee80211_handle_erp_ie(dev, bss->erp_value);
> > -                     ieee80211_rx_bss_put(dev, bss);
> > -             }
> > +                     bss = ieee80211_rx_bss_get(dev, ifsta->bssid);
> > +                     if (bss) {
> > +                             if (bss->has_erp_value)
> > +                               ieee80211_handle_erp_ie(dev, bss->erp_value);
>
> [bad indenting]
>
> >  static void ieee80211_sta_tx(struct net_device *dev, struct sk_buff *skb,
> > @@ -1162,6 +1172,7 @@ static void ieee80211_rx_mgmt_assoc_resp(struct net_device *dev,
> >       u32 rates;
> >       u16 capab_info, status_code, aid;
> >       struct ieee802_11_elems elems;
> > +     struct ieee80211_bss_data bss_data;
> >       u8 *pos;
> >       int i, j;
> >
> > @@ -1249,7 +1260,11 @@ static void ieee80211_rx_mgmt_assoc_resp(struct net_device *dev,
> >       if (ifsta->assocresp_ies)
> >               memcpy(ifsta->assocresp_ies, pos, ifsta->assocresp_ies_len);
> >
> > -     ieee80211_set_associated(dev, ifsta, 1);
> > +     bss_data.assoc = 1;
> > +     bss_data.changed_map = ASSOC_CHNAGED_STATE;
> > +     bss_data.aid = aid;
> > +     bss_data.changed_map |= ASSOC_CHNAGED_AID;
> > +     ieee80211_set_associated(dev, ifsta, &bss_data);
>
> I think this is a bit dangerous. Above, you defined the structure member
> 'changed_map' which I personally would read as "take care, in this
> structure the fields X, Y and Z changed over what I gave you last time".
> However, the way it's used here is as a 'valid' bitmap, in "In this
> struct, only the fields X, Y and Z are valid". This is a huge difference
> if the driver happens to need multiple things at the same time, with the
> approach you're doing here you'd need to keep track of everything in the
> driver.
>
Yes we can have both valid and change bitmap to lower that burden from
form driver.

Thanks for your valuable comments

> johannes
>
>
-
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Index of Archives]     [Linux Host AP]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [Linux Kernel]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]
  Powered by Linux