On Wed, Feb 09, 2011 at 10:11:03PM +0100, Arend van Spriel wrote:
> On Wed, 09 Feb 2011 20:45:12 +0100, Greg KH <greg@xxxxxxxxx> wrote:
>
> >On Wed, Feb 09, 2011 at 12:16:26AM +0300, Dan Carpenter wrote:
> >>On Tue, Feb 08, 2011 at 02:39:14PM +0100, Arend van Spriel wrote:
> >>> The implementation for bss_info_changed was not handling all
> >>> changes as provided by mac80211 module. These have been added
> >>> where needed.
> >>>
> >>
> >>The subject says clean up and the long description say implementing
> >>new features were implemented on an as needed basis.
> >>
>
> Your remark applies to the cover letter describing the patch series.
> Each individual patch in this series is targetting a separate issue
> imho.
>
No, I mean this patch specifically. Git history doesn't care about
patch sets, only about individual patches. (The cover letter is not
saved anywhere). I've commented on the patch below which I things I
view as behavior changes and which I view as cleanup.
> diff --git a/drivers/staging/brcm80211/brcmsmac/wl_mac80211.c b/drivers/staging/brcm80211/brcmsmac/wl_mac80211.c
> index 745c887..9ce10ef 100644
> --- a/drivers/staging/brcm80211/brcmsmac/wl_mac80211.c
> +++ b/drivers/staging/brcm80211/brcmsmac/wl_mac80211.c
> @@ -328,64 +328,99 @@ wl_ops_bss_info_changed(struct ieee80211_hw *hw,
> struct wl_info *wl = HW_TO_WL(hw);
> int val;
>
> -
> if (changed & BSS_CHANGED_ASSOC) {
> - WL_ERROR("Associated:\t%s\n", info->assoc ? "True" : "False");
> /* association status changed (associated/disassociated)
> * also implies a change in the AID.
> */
> + WL_NONE("Associated: %s\n", info->assoc ? "true" : "false");
Here you've changed the output level and the format. That's an
implementation change but it's still cleanup.
> + wlc_associate_upd(wl->wlc, info->assoc);
This is behavior change.
> }
> if (changed & BSS_CHANGED_ERP_CTS_PROT) {
> - WL_NONE("Use_cts_prot:\t%s Implement me\n",
> - info->use_cts_prot ? "True" : "False");
> /* CTS protection changed */
> + WL_NONE("Use_cts_prot: %s (implement)\n",
> + info->use_cts_prot ? "true" : "false");
Cleanup.
> }
> if (changed & BSS_CHANGED_ERP_PREAMBLE) {
> - WL_NONE("Short preamble:\t%s Implement me\n",
> - info->use_short_preamble ? "True" : "False");
> /* preamble changed */
> + WL_NONE("Short preamble: %s (implement)\n",
> + info->use_short_preamble ? "true" : "false");
Cleanup.
> }
> if (changed & BSS_CHANGED_ERP_SLOT) {
> - WL_NONE("Changing short slot:\t%s\n",
> - info->use_short_slot ? "True" : "False");
> + /* slot timing changed */
> + WL_NONE("Changing short slot: %s\n",
> + info->use_short_slot ? "true" : "false");
Cleanup.
> if (info->use_short_slot)
> val = 1;
> else
> val = 0;
> wlc_set(wl->wlc, WLC_SET_SHORTSLOT_OVERRIDE, val);
> - /* slot timing changed */
> }
>
> if (changed & BSS_CHANGED_HT) {
> - WL_NONE("%s: HT mode - Implement me\n", __func__);
> /* 802.11n parameters changed */
> + u16 mode = info->ht_operation_mode;
> + WL_NONE("%s: HT mode: 0x%04X (implement)\n", __func__, mode);
> + wlc_protection_upd(wl->wlc, WLC_PROT_N_CFG,
> + mode & IEEE80211_HT_OP_MODE_PROTECTION);
> + wlc_protection_upd(wl->wlc, WLC_PROT_N_NONGF,
> + mode & IEEE80211_HT_OP_MODE_NON_GF_STA_PRSNT);
> + wlc_protection_upd(wl->wlc, WLC_PROT_N_OBSS,
> + mode & IEEE80211_HT_OP_MODE_NON_HT_STA_PRSNT);
Behavior change.
> }
> if (changed & BSS_CHANGED_BASIC_RATES) {
> - WL_NONE("Need to change Basic Rates:\t0x%x! Implement me\n",
> - (u32) info->basic_rates);
> /* Basic rateset changed */
> + WL_NONE("Need to change Basic Rates: 0x%x (implement)\n",
> + (u32) info->basic_rates);
> }
> if (changed & BSS_CHANGED_BEACON_INT) {
> - WL_NONE("Beacon Interval:\t%d Implement me\n",
> - info->beacon_int);
> /* Beacon interval changed */
> + WL_NONE("Beacon Interval: %d\n",
> + info->beacon_int);
> + wlc_set(wl->wlc, WLC_SET_BCNPRD, info->beacon_int);
Behavior change.
> }
> if (changed & BSS_CHANGED_BSSID) {
> - WL_NONE("new BSSID:\taid %d bss:%pM\n",
> - info->aid, info->bssid);
> /* BSSID changed, for whatever reason (IBSS and managed mode) */
> - /* FIXME: need to store bssid in bsscfg */
> + WL_NONE("new BSSID: aid %d bss:%pM\n",
> + info->aid, info->bssid);
> wlc_set_addrmatch(wl->wlc, RCM_BSSID_OFFSET,
> info->bssid);
> }
> if (changed & BSS_CHANGED_BEACON) {
> - WL_ERROR("BSS_CHANGED_BEACON\n");
> /* Beacon data changed, retrieve new beacon (beaconing modes) */
> + WL_NONE("BSS_CHANGED_BEACON\n");
> }
> if (changed & BSS_CHANGED_BEACON_ENABLED) {
> - WL_ERROR("Beacon enabled:\t%s\n",
> - info->enable_beacon ? "True" : "False");
> /* Beaconing should be enabled/disabled (beaconing modes) */
> + WL_NONE("Beacon enabled: %s\n",
> + info->enable_beacon ? "true" : "false");
> + }
> + if (changed & BSS_CHANGED_CQM) {
> + /* Connection quality monitor config changed */
> + WL_NONE("BSS_CHANGED_CQM: threshold %d, hys %d\n",
> + info->cqm_rssi_thold, info->cqm_rssi_hyst);
> + }
> + if (changed & BSS_CHANGED_IBSS) {
> + /* IBSS join status changed */
> + WL_NONE("BSS_CHANGED_IBSS: %s\n",
> + info->ibss_joined ? "true" : "false");
> + }
> + if (changed & BSS_CHANGED_ARP_FILTER) {
> + /* Hardware ARP filter address list or state changed */
> + WL_NONE("BSS_CHANGED_ARP_FILTER: enabled %s, count %d\n",
> + info->arp_filter_enabled ? "true" : "false",
> + info->arp_addr_cnt);
> + }
> + if (changed & BSS_CHANGED_QOS) {
> + /*
> + * QoS for this association was enabled/disabled.
> + * Note that it is only ever disabled for station mode.
> + */
> + WL_NONE("BSS_CHANGED_QOS: %s\n", info->qos ? "true" : "false");
> + }
> + if (changed & BSS_CHANGED_IDLE) {
> + /* Idle changed for this BSS/interface */
> + WL_NONE("BSS_CHANGED_IDLE: %s\n",
> + info->idle ? "true" : "false");
> }
A whole bunch of debug code added. This should have been mentioned
in the commit message.
> return;
> }
> diff --git a/drivers/staging/brcm80211/brcmsmac/wlc_mac80211.c b/drivers/staging/brcm80211/brcmsmac/wlc_mac80211.c
> index 3f7f93e..41c8c60 100644
> --- a/drivers/staging/brcm80211/brcmsmac/wlc_mac80211.c
> +++ b/drivers/staging/brcm80211/brcmsmac/wlc_mac80211.c
> @@ -5742,7 +5742,7 @@ wlc_d11hdrs_mac80211(struct wlc_info *wlc, struct ieee80211_hw *hw,
>
> /* add Broadcom tx descriptor header */
> txh = (d11txh_t *) skb_push(p, D11_TXH_LEN);
> - memset((char *)txh, 0, D11_TXH_LEN);
> + memset(txh, 0, D11_TXH_LEN);
>
Remove an unneeded cast. Cleanup.
> /* setup frameid */
> if (tx_info->flags & IEEE80211_TX_CTL_ASSIGN_SEQ) {
> @@ -5938,10 +5938,9 @@ wlc_d11hdrs_mac80211(struct wlc_info *wlc, struct ieee80211_hw *hw,
>
> /* if SGI is selected, then forced mm for single stream */
> if ((rspec[k] & RSPEC_SHORT_GI)
> - && IS_SINGLE_STREAM(rspec[k] &
> - RSPEC_RATE_MASK)) {
> + && IS_SINGLE_STREAM(
> + rspec[k] & RSPEC_RATE_MASK))
> preamble_type[k] = WLC_MM_PREAMBLE;
> - }
> }
>
> /* mimo bw field MUST now be valid in the rspec (it affects duration calculations) */
> @@ -8255,6 +8254,8 @@ wlc_set_addrmatch(struct wlc_info *wlc, int match_reg_offset,
> const u8 *addr)
> {
> wlc_bmac_set_addrmatch(wlc->hw, match_reg_offset, addr);
> + if (match_reg_offset == RCM_BSSID_OFFSET)
> + bcopy(addr, wlc->cfg->BSSID, ETH_ALEN);
Behavior change. Btw. Don't use bcopy() in new code. I didn't mention
it the first time I reviewed this because I assume you plan on removing
the bcopy() macro eventually.
> }
>
> void wlc_set_rcmta(struct wlc_info *wlc, int idx, const u8 *addr)
> @@ -8503,3 +8504,9 @@ void wlc_scan_stop(struct wlc_info *wlc)
> {
> wlc_phy_hold_upd(wlc->band->pi, PHY_HOLD_FOR_SCAN, false);
> }
> +
> +void wlc_associate_upd(struct wlc_info *wlc, bool state)
> +{
> + wlc->pub->associated = state;
> + wlc->cfg->associated = state;
> +}
Behavior change.
> diff --git a/drivers/staging/brcm80211/brcmsmac/wlc_mac80211.h b/drivers/staging/brcm80211/brcmsmac/wlc_mac80211.h
> index 5817a49..0aeb9c6 100644
> --- a/drivers/staging/brcm80211/brcmsmac/wlc_mac80211.h
> +++ b/drivers/staging/brcm80211/brcmsmac/wlc_mac80211.h
> @@ -29,19 +29,6 @@
> #define MAXCOREREV 28 /* max # supported core revisions (0 .. MAXCOREREV - 1) */
> #define WLC_MAXMODULES 22 /* max # wlc_module_register() calls */
>
> -/* network protection config */
> -#define WLC_PROT_G_SPEC 1 /* SPEC g protection */
> -#define WLC_PROT_G_OVR 2 /* SPEC g prot override */
> -#define WLC_PROT_G_USER 3 /* gmode specified by user */
> -#define WLC_PROT_OVERLAP 4 /* overlap */
> -#define WLC_PROT_N_USER 10 /* nmode specified by user */
> -#define WLC_PROT_N_CFG 11 /* n protection */
> -#define WLC_PROT_N_CFG_OVR 12 /* n protection override */
> -#define WLC_PROT_N_NONGF 13 /* non-GF protection */
> -#define WLC_PROT_N_NONGF_OVR 14 /* non-GF protection override */
> -#define WLC_PROT_N_PAM_OVR 15 /* n preamble override */
> -#define WLC_PROT_N_OBSS 16 /* non-HT OBSS present */
> -
> #define WLC_BITSCNT(x) bcm_bitcount((u8 *)&(x), sizeof(u8))
>
> /* Maximum wait time for a MAC suspend */
> @@ -847,7 +834,6 @@ extern void wlc_set_cwmax(struct wlc_info *wlc, u16 newmax);
> extern void wlc_fifoerrors(struct wlc_info *wlc);
> extern void wlc_pllreq(struct wlc_info *wlc, bool set, mbool req_bit);
> extern void wlc_reset_bmac_done(struct wlc_info *wlc);
> -extern void wlc_protection_upd(struct wlc_info *wlc, uint idx, int val);
> extern void wlc_hwtimer_gptimer_set(struct wlc_info *wlc, uint us);
> extern void wlc_hwtimer_gptimer_abort(struct wlc_info *wlc);
>
> diff --git a/drivers/staging/brcm80211/brcmsmac/wlc_pub.h b/drivers/staging/brcm80211/brcmsmac/wlc_pub.h
> index e059ebf..ca9a5d8 100644
> --- a/drivers/staging/brcm80211/brcmsmac/wlc_pub.h
> +++ b/drivers/staging/brcm80211/brcmsmac/wlc_pub.h
> @@ -484,6 +484,19 @@ extern const u8 wme_fifo2ac[];
> #define WLCNTSET(a, value) /* No stats support */
> #define WLCNTVAL(a) 0 /* No stats support */
>
> +/* network protection config */
> +#define WLC_PROT_G_SPEC 1 /* SPEC g protection */
> +#define WLC_PROT_G_OVR 2 /* SPEC g prot override */
> +#define WLC_PROT_G_USER 3 /* gmode specified by user */
> +#define WLC_PROT_OVERLAP 4 /* overlap */
> +#define WLC_PROT_N_USER 10 /* nmode specified by user */
> +#define WLC_PROT_N_CFG 11 /* n protection */
> +#define WLC_PROT_N_CFG_OVR 12 /* n protection override */
> +#define WLC_PROT_N_NONGF 13 /* non-GF protection */
> +#define WLC_PROT_N_NONGF_OVR 14 /* non-GF protection override */
> +#define WLC_PROT_N_PAM_OVR 15 /* n preamble override */
> +#define WLC_PROT_N_OBSS 16 /* non-HT OBSS present */
> +
I guess we are exporting these to definitions to user space now? It
would help if you told us that.
regards,
dan carpenter
> /* common functions for every port */
> extern void *wlc_attach(void *wl, u16 vendor, u16 device, uint unit,
> bool piomode, struct osl_info *osh, void *regsva,
> @@ -517,6 +530,7 @@ extern int wlc_ioctl(struct wlc_info *wlc, int cmd, void *arg, int len,
> struct wlc_if *wlcif);
> /* helper functions */
> extern void wlc_statsupd(struct wlc_info *wlc);
> +extern void wlc_protection_upd(struct wlc_info *wlc, uint idx, int val);
> extern int wlc_get_header_len(void);
> extern void wlc_mac_bcn_promisc_change(struct wlc_info *wlc, bool promisc);
> extern void wlc_set_addrmatch(struct wlc_info *wlc, int match_reg_offset,
> @@ -568,6 +582,7 @@ extern u32 wlc_get_rspec_history(struct wlc_bsscfg *cfg);
> extern u32 wlc_get_current_highest_rate(struct wlc_bsscfg *cfg);
> extern void wlc_scan_start(struct wlc_info *wlc);
> extern void wlc_scan_stop(struct wlc_info *wlc);
> +extern void wlc_associate_upd(struct wlc_info *wlc, bool state);
>
> static inline int wlc_iovar_getuint(struct wlc_info *wlc, const char *name,
> uint *arg)
> --
> 1.7.1
>
>
> _______________________________________________
> devel mailing list
> devel@xxxxxxxxxxxxxxxxxxxxxx
> http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
