On Sat, Oct 16, 2010 at 11:13:27AM -0700, nohee ko wrote:
> Add debugfs in brcmfmac
Please detail why you are doing this and what it is for in the
changelog.
Lots of comments below:
> Signed-off-by: Nohee Ko <noheek@xxxxxxxxxxxx>
> ---
> drivers/staging/brcm80211/Kconfig | 9 ++-
> drivers/staging/brcm80211/brcmfmac/Makefile | 2 +-
> drivers/staging/brcm80211/brcmfmac/debugfs.c | 75 ++++++++++++
> drivers/staging/brcm80211/brcmfmac/debugfs.h | 13 ++
> drivers/staging/brcm80211/brcmfmac/wl_cfg80211.c | 132 +++++++++++++++------
> drivers/staging/brcm80211/brcmfmac/wl_cfg80211.h | 24 ++++-
> 6 files changed, 214 insertions(+), 41 deletions(-)
> create mode 100644 drivers/staging/brcm80211/brcmfmac/debugfs.c
> create mode 100644 drivers/staging/brcm80211/brcmfmac/debugfs.h
>
> diff --git a/drivers/staging/brcm80211/Kconfig b/drivers/staging/brcm80211/Kconfig
> index 57d2d1b..fdc3cb0 100644
> --- a/drivers/staging/brcm80211/Kconfig
> +++ b/drivers/staging/brcm80211/Kconfig
> @@ -1,4 +1,4 @@
> -menuconfig BRCM80211
> +config BRCM80211
Are you sure this should be changed? Doesn't it then break the other
option?
> tristate "Broadcom IEEE802.11n WLAN drivers"
> depends on WLAN
>
> @@ -30,4 +30,11 @@ config BRCMFMAC
> Broadcom IEEE802.11n FullMAC chipsets. This driver uses the kernel's
> wireless extensions subsystem. If you choose to build a module,
> it'll be called brcmfmac.ko.
> +
> +config BRCMFMAC_DEBUGFS
> + bool "Enable debugfs in brcmfmac"
Why would someone want this? Where is it going to show up in debugfs?
What is going to be there when it does?
And finally, why make this an option at all? Why not just always use
this code and if debugfs is not enabled, it should compile away
properly, right?
> + depends on BRCMFMAC && DEBUG_FS
> + ---help---
> + This option will enable to access statistics for Broadcom fullmac
> + Chipsets
> endchoice
> diff --git a/drivers/staging/brcm80211/brcmfmac/Makefile b/drivers/staging/brcm80211/brcmfmac/Makefile
> index 76f2d8b..494d46e 100644
> --- a/drivers/staging/brcm80211/brcmfmac/Makefile
> +++ b/drivers/staging/brcm80211/brcmfmac/Makefile
> @@ -44,4 +44,4 @@ DHDOFILES = dhd_linux.o ../util/linux_osl.o ../util/bcmutils.o dhd_common.o dhd_
>
> obj-m += brcmfmac.o
> brcmfmac-objs += $(DHDOFILES)
> -
> +brcmfmac-$(CONFIG_BRCMFMAC_DEBUGFS) += debugfs.o
> diff --git a/drivers/staging/brcm80211/brcmfmac/debugfs.c b/drivers/staging/brcm80211/brcmfmac/debugfs.c
> new file mode 100644
> index 0000000..26123fc
> --- /dev/null
> +++ b/drivers/staging/brcm80211/brcmfmac/debugfs.c
> @@ -0,0 +1,75 @@
> +/*
> + * Copyright (c) 2010 Broadcom Corporation
> + *
> + * Permission to use, copy, modify, and/or distribute this software for any
> + * purpose with or without fee is hereby granted, provided that the above
> + * copyright notice and this permission notice appear in all copies.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
> + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
> + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY
> + * SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
> + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION
> + * OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN
> + * CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
> + */
> +
> +#include <linux/slab.h>
> +#include <linux/kernel.h>
> +#include <linux/debugfs.h>
> +#include "wl_cfg80211.h"
> +#include "debugfs.h"
Please don't duplicate .h file names with a core kernel one. Also, why
does it have to be a separate .h file at all? I'm trying to merge the
many .h files together for this driver, not create new ones.
> +
> +static int wl_open_file_generic(struct inode *inode, struct file *file)
> +{
> + file->private_data = inode->i_private;
> + return 0;
> +}
> +
> +#define DEBUGFS_READONLY_FILE(name, buflen, fmt, value...) \
> +static ssize_t name## _read(struct file *file, char __user *userbuf, \
> + size_t count, loff_t *ppos) \
> +{ \
> + struct wl_priv *wl = file->private_data; \
> + char buf[buflen]; \
> + int res; \
> + \
> + res = scnprintf(buf, buflen, fmt "\n", ##value); \
> + return simple_read_from_buffer(userbuf, count, ppos, buf, res); \
> +} \
> + \
> +static const struct file_operations name## _ops = { \
> + .read = name## _read, \
> + .open = wl_open_file_generic, \
> +};
> +
> +DEBUGFS_READONLY_FILE(beacon_int, 20, "%d", wl->profile->beacon_interval)
> +DEBUGFS_READONLY_FILE(dtim_period, 20, "%d", wl->profile->dtim_period)
Can't you use one of the debugfs helper functions for these values?
> +
> +#define DEBUGFS_ADD(name) \
> + debugfs_create_file(#name, S_IRUGO, phyd, wl, &name## _ops);
This should not be a macro, come on, spell it out in the code.
> +
> +void wl_debugfs_add_params(struct wl_priv *wl)
> +{
> + struct dentry *phyd = wl->debugfs.dir;
> +
> + DEBUGFS_ADD(beacon_int);
> + DEBUGFS_ADD(dtim_period);
> +}
> +
> +void wl_debugfs_add_netdev(struct wl_priv *wl)
> +{
> + char buf[10+IFNAMSIZ];
> +
> + sprintf(buf, "netdev:%s", wl_to_ndev(wl)->name);
> + wl->debugfs.dir = debugfs_create_dir(buf, wl_to_wiphy(wl)->debugfsdir);
Do you really need a whole structure for the inode for debugfs? Please,
just use a single pointer, as that's all you are really doing in the
end.
> +}
> +
> +void wl_debugfs_remove_netdev(struct wl_priv *wl)
> +{
> + if (!wl->debugfs.dir)
> + return;
Why would you need to check this?
> +
> + debugfs_remove_recursive(wl->debugfs.dir);
> + wl->debugfs.dir = NULL;
> +}
> diff --git a/drivers/staging/brcm80211/brcmfmac/debugfs.h b/drivers/staging/brcm80211/brcmfmac/debugfs.h
> new file mode 100644
> index 0000000..aca0fa2
> --- /dev/null
> +++ b/drivers/staging/brcm80211/brcmfmac/debugfs.h
> @@ -0,0 +1,13 @@
> +#ifndef _wl_debugfs_h_
> +#define _wl_debugfs_h_
So you call the file debugfs.h yet your internal name for it is
different? Don't do that.
Also, two '_' characters please.
> +
> +#ifdef CONFIG_BRCMFMAC_DEBUGFS
> +void wl_debugfs_add_params(struct wl_priv *wl);
> +void wl_debugfs_add_netdev(struct wl_priv *wl);
> +void wl_debugfs_remove_netdev(struct wl_priv *wl);
> +#else
> +static inline void wl_debugfs_add_params(struct wl_priv *wl) {}
> +static inline void wl_debugfs_add_netdev(struct wl_priv *wl) {}
> +static inline void wl_debugfs_remove_netdev(struct wl_priv *wl) {}
> +#endif
> +#endif /* __wl_debugfs_h */
> diff --git a/drivers/staging/brcm80211/brcmfmac/wl_cfg80211.c b/drivers/staging/brcm80211/brcmfmac/wl_cfg80211.c
> index 20367a0..c3d5b41 100644
> --- a/drivers/staging/brcm80211/brcmfmac/wl_cfg80211.c
> +++ b/drivers/staging/brcm80211/brcmfmac/wl_cfg80211.c
> @@ -47,7 +47,8 @@
> #include <net/rtnetlink.h>
> #include <linux/mmc/sdio_func.h>
> #include <linux/firmware.h>
> -#include <wl_cfg80211.h>
> +#include "wl_cfg80211.h"
> +#include "debugfs.h"
>
> static struct sdio_func *cfg80211_sdio_func;
> static struct wl_dev *wl_cfg80211_dev;
> @@ -332,6 +333,7 @@ static __used s32 wl_update_pmklist(struct net_device *dev,
> struct wl_pmk_list *pmk_list, s32 err);
>
> static void wl_set_mpc(struct net_device *ndev, int mpc);
> +static u8 *wl_get_information_element(u8 *buf, s32 buf_len, enum wl_ie_id);
>
> #define WL_PRIV_GET() \
> ({ \
> @@ -732,6 +734,7 @@ static s32 wl_do_iscan(struct wl_priv *wl)
> struct wl_iscan_ctrl *iscan = wl_to_iscan(wl);
> struct net_device *ndev = wl_to_ndev(wl);
> struct wlc_ssid ssid;
> + s32 passive_scan;
> s32 err = 0;
>
> /* Broadcast scan by default */
> @@ -739,15 +742,12 @@ static s32 wl_do_iscan(struct wl_priv *wl)
>
> iscan->state = WL_ISCAN_STATE_SCANING;
>
> - if (wl->active_scan) {
> - s32 passive_scan = 0;
> - /* make it active scan */
> - err = wl_dev_ioctl(wl_to_ndev(wl), WLC_SET_PASSIVE_SCAN,
> - &passive_scan, sizeof(passive_scan));
> - if (unlikely(err)) {
> - WL_DBG(("error (%d)\n", err));
> - return err;
> - }
> + passive_scan = wl->active_scan ? 0 : 1;
> + err = wl_dev_ioctl(wl_to_ndev(wl), WLC_SET_PASSIVE_SCAN,
> + &passive_scan, sizeof(passive_scan));
> + if (unlikely(err)) {
> + WL_DBG(("error (%d)\n", err));
> + return err;
> }
> wl_set_mpc(ndev, 0);
> wl->iscan_kickstart = true;
What do these two chunks have to to with debugfs?
> @@ -766,6 +766,7 @@ __wl_cfg80211_scan(struct wiphy *wiphy, struct net_device *ndev,
> struct wl_priv *wl = ndev_to_wl(ndev);
> struct cfg80211_ssid *ssids;
> struct wl_scan_req *sr = wl_to_sr(wl);
> + s32 passive_scan;
> bool iscan_req;
> bool spec_scan;
> s32 err = 0;
> @@ -823,16 +824,12 @@ __wl_cfg80211_scan(struct wiphy *wiphy, struct net_device *ndev,
> WL_DBG(("Broadcast scan\n"));
> }
> WL_DBG(("sr->ssid.SSID_len (%d)\n", sr->ssid.SSID_len));
> - if (wl->active_scan) {
> - s32 pssive_scan = 0;
> - /* make it active scan */
> - err = wl_dev_ioctl(ndev, WLC_SET_PASSIVE_SCAN,
> - &pssive_scan, sizeof(pssive_scan));
> - if (unlikely(err)) {
> - WL_ERR(("WLC_SET_PASSIVE_SCAN error (%d)\n",
> - err));
> - goto scan_out;
> - }
> + passive_scan = wl->active_scan ? 0 : 1;
> + err = wl_dev_ioctl(ndev, WLC_SET_PASSIVE_SCAN,
> + &passive_scan, sizeof(passive_scan));
> + if (unlikely(err)) {
> + WL_ERR(("WLC_SET_PASSIVE_SCAN error (%d)\n", err));
> + goto scan_out;
> }
> wl_set_mpc(ndev, 0);
> err = wl_dev_ioctl(ndev, WLC_SCAN, &sr->ssid,
Same here, why do they have anything to do with the debugfs file?
> @@ -2264,6 +2261,8 @@ static s32 wl_inform_single_bss(struct wl_priv *wl, struct wl_bss_info *bi)
> struct ieee80211_supported_band *band;
> struct wl_cfg80211_bss_info *notif_bss_info;
> struct wl_scan_req *sr = wl_to_sr(wl);
> + struct beacon_proberesp *beacon_proberesp;
> + s32 mgmt_type;
> u32 signal;
> u32 freq;
> s32 err = 0;
> @@ -2289,13 +2288,18 @@ static s32 wl_inform_single_bss(struct wl_priv *wl, struct wl_bss_info *bi)
> band = wiphy->bands[IEEE80211_BAND_5GHZ];
> notif_bss_info->rssi = bi->RSSI;
> memcpy(mgmt->bssid, &bi->BSSID, ETHER_ADDR_LEN);
> + mgmt_type = wl->active_scan ?
> + IEEE80211_STYPE_PROBE_RESP : IEEE80211_STYPE_BEACON;
> if (!memcmp(bi->SSID, sr->ssid.SSID, bi->SSID_len)) {
> mgmt->frame_control = cpu_to_le16(IEEE80211_FTYPE_MGMT |
> - IEEE80211_STYPE_PROBE_RESP);
> - }
> - mgmt->u.probe_resp.timestamp = 0;
> - mgmt->u.probe_resp.beacon_int = cpu_to_le16(bi->beacon_period);
> - mgmt->u.probe_resp.capab_info = cpu_to_le16(bi->capability);
> + mgmt_type);
> + }
> + beacon_proberesp = wl->active_scan ?
> + (struct beacon_proberesp *)&mgmt->u.probe_resp :
> + (struct beacon_proberesp *)&mgmt->u.beacon;
> + beacon_proberesp->timestamp = 0;
> + beacon_proberesp->beacon_int = cpu_to_le16(bi->beacon_period);
> + beacon_proberesp->capab_info = cpu_to_le16(bi->capability);
> wl_rst_ie(wl);
> /*
> * wl_add_ie is not necessary because it can only add duplicated
> @@ -2307,18 +2311,18 @@ static s32 wl_inform_single_bss(struct wl_priv *wl, struct wl_bss_info *bi)
> * bi->rateset.rates);
> */
> wl_mrg_ie(wl, ((u8 *) bi) + bi->ie_offset, bi->ie_length);
> - wl_cp_ie(wl, mgmt->u.probe_resp.variable, WL_BSS_INFO_MAX -
> + wl_cp_ie(wl, beacon_proberesp->variable, WL_BSS_INFO_MAX -
> offsetof(struct wl_cfg80211_bss_info, frame_buf));
> notif_bss_info->frame_len =
> offsetof(struct ieee80211_mgmt,
> - u.probe_resp.variable) + wl_get_ielen(wl);
> + u.beacon.variable) + wl_get_ielen(wl);
> freq = ieee80211_channel_to_frequency(notif_bss_info->channel);
> channel = ieee80211_get_channel(wiphy, freq);
>
> WL_DBG(("SSID : \"%s\", rssi %d, channel %d, capability : 0x04%x, bssid %pM\n",
> bi->SSID,
> notif_bss_info->rssi, notif_bss_info->channel,
> - mgmt->u.probe_resp.capab_info, &bi->BSSID));
> + mgmt->u.beacon.capab_info, &bi->BSSID));
>
> signal = notif_bss_info->rssi * 100;
> if (unlikely(!cfg80211_inform_bss_frame(wiphy, channel, mgmt,
> @@ -2546,11 +2550,33 @@ static void wl_ch_to_chanspec(int ch, struct wl_join_params *join_params,
> }
> }
>
> +static u8 *wl_get_information_element(u8 *buf, s32 buf_len, enum wl_ie_id ie_id)
> +{
> + s32 ie_len;
> +
> + for (; buf_len > 0; buf_len -= ie_len) {
> + if (*buf == ie_id) {
> + WL_DBG(("found Information Element ID - %d\n", ie_id));
> + return buf;
> + } else {
> + ie_len = 2 + *(buf + 1);
> + buf += ie_len;
> + }
> + }
> +
> + return NULL;
> +}
What is this function for?
> +
> static s32 wl_update_bss_info(struct wl_priv *wl)
> {
> struct cfg80211_bss *bss;
> struct wl_bss_info *bi;
> struct wlc_ssid *ssid;
> + u16 beacon_interval;
> + u8 dtim_period;
> + size_t ie_len;
> + u8 *ie;
> + u8 *tim;
> s32 err = 0;
>
> if (wl_is_ibssmode(wl))
> @@ -2580,11 +2606,38 @@ static s32 wl_update_bss_info(struct wl_priv *wl)
> err = wl_inform_single_bss(wl, bi);
> if (unlikely(err))
> goto update_bss_info_out;
> +
> + ie = ((u8 *)bi) + bi->ie_offset;
> + ie_len = bi->ie_length;
> + beacon_interval = cpu_to_le16(bi->beacon_period);
> } else {
> WL_DBG(("Found the AP in the list - BSSID %pM\n", bss->bssid));
> + ie = bss->information_elements;
> + ie_len = bss->len_information_elements;
> + beacon_interval = bss->beacon_interval;
> cfg80211_put_bss(bss);
> }
How do these changes pertain to debugfs files?
>
> + tim = wl_get_information_element(ie, ie_len, WL_TIM);
> + if (tim) {
> + dtim_period = tim[3];
> + } else {
> + /*
> + * active scan was done so we could not get dtim
> + * information out of probe response.
> + * so we speficially query dtim information to dongle.
> + */
> + err = wl_dev_ioctl(wl_to_ndev(wl), WLC_GET_DTIMPRD,
> + &dtim_period, sizeof(dtim_period));
> + if (unlikely(err)) {
> + WL_ERR(("WLC_GET_DTIMPRD error (%d)\n", err));
> + goto update_bss_info_out;
> + }
> + }
> +
> + wl_update_prof(wl, NULL, &beacon_interval, WL_PROF_BEACONINT);
> + wl_update_prof(wl, NULL, &dtim_period, WL_PROF_DTIMPERIOD);
> +
> update_bss_info_out:
> rtnl_unlock();
> return err;
> @@ -3108,18 +3161,10 @@ static s32 wl_init_priv(struct wl_priv *wl)
>
> wl->scan_request = NULL;
> wl->pwr_save = !!(wiphy->flags & WIPHY_FLAG_PS_ON_BY_DEFAULT);
> -#ifndef WL_ISCAN_DISABLED
> wl->iscan_on = true; /* iscan on & off switch.
> we enable iscan per default */
> -#else
> - wl->iscan_on = false;
> -#endif /* WL_ISCAN_DISABLED */
> -#ifndef WL_ROAM_DISABLED
> - wl->roam_on = true; /* roam on & off switch.
> + wl->roam_on = false; /* roam on & off switch.
> we enable roam per default */
> -#else
> - wl->roam_on = false;
> -#endif /* WL_ROAM_DISABLED */
>
> wl->iscan_kickstart = false;
> wl->active_scan = true; /* we do active scan for
> @@ -3788,6 +3833,9 @@ static s32 __wl_cfg80211_up(struct wl_priv *wl)
> {
> s32 err = 0;
>
> + wl_debugfs_add_netdev(wl);
> + wl_debugfs_add_params(wl);
If you always do both things, why not just do it in one single function?
> +
> err = wl_config_dongle(wl, false);
> if (unlikely(err))
> return err;
> @@ -3825,6 +3873,8 @@ static s32 __wl_cfg80211_down(struct wl_priv *wl)
> clear_bit(WL_STATUS_SCAN_ABORTING, &wl->status);
> clear_bit(WL_STATUS_CONNECTED, &wl->status);
>
> + wl_debugfs_remove_netdev(wl);
> +
> return err;
> }
>
> @@ -3906,7 +3956,13 @@ wl_update_prof(struct wl_priv *wl, const wl_event_msg_t *e, void *data,
> memcpy(&wl->profile->sec, data, sizeof(wl->profile->sec));
> break;
> case WL_PROF_ACT:
> - wl->profile->active = *(bool *) data;
> + wl->profile->active = *(bool *)data;
> + break;
> + case WL_PROF_BEACONINT:
> + wl->profile->beacon_interval = *(u16 *)data;
> + break;
> + case WL_PROF_DTIMPERIOD:
> + wl->profile->dtim_period = *(u8 *)data;
> break;
> default:
> WL_ERR(("unsupported item (%d)\n", item));
What does this have to do with debugfs?
> diff --git a/drivers/staging/brcm80211/brcmfmac/wl_cfg80211.h b/drivers/staging/brcm80211/brcmfmac/wl_cfg80211.h
> index d125d90..956feba 100644
> --- a/drivers/staging/brcm80211/brcmfmac/wl_cfg80211.h
> +++ b/drivers/staging/brcm80211/brcmfmac/wl_cfg80211.h
> @@ -112,6 +112,11 @@ do { \
> */
> #define WL_FILE_NAME_MAX 256
>
> +/* information elements */
> +enum wl_ie_id {
> + WL_TIM = 5
> +};
> +
> /* dongle status */
> enum wl_status {
> WL_STATUS_READY,
> @@ -136,7 +141,9 @@ enum wl_prof_list {
> WL_PROF_IBSS,
> WL_PROF_BAND,
> WL_PROF_BSSID,
> - WL_PROF_ACT
> + WL_PROF_ACT,
> + WL_PROF_BEACONINT,
> + WL_PROF_DTIMPERIOD
> };
>
> /* dongle iscan state */
> @@ -151,6 +158,14 @@ enum wl_fw_status {
> WL_NVRAM_LOADING_DONE
> };
>
> +/* beacon / probe_response */
> +struct beacon_proberesp {
> + __le64 timestamp;
> + __le16 beacon_int;
> + __le16 capab_info;
> + u8 variable[0];
> +} __attribute__ ((packed));
> +
> /* dongle configuration */
> struct wl_conf {
> u32 mode; /* adhoc , infrastructure or ap */
> @@ -229,6 +244,8 @@ struct wl_profile {
> u32 mode;
> struct wlc_ssid ssid;
> u8 bssid[ETHER_ADDR_LEN];
> + u16 beacon_interval;
> + u8 dtim_period;
> struct wl_security sec;
> struct wl_ibss ibss;
> s32 band;
> @@ -329,6 +346,11 @@ struct wl_priv {
> bool scan_tried; /* indicates if first scan attempted */
> u8 *ioctl_buf; /* ioctl buffer */
> u8 *extra_buf; /* maily to grab assoc information */
> +#ifdef CONFIG_BRCMFMAC_DEBUGFS
> + struct {
> + struct dentry *dir;
> + } debugfs;
> +#endif
again, no need for a whole structure, just declare a single dentry
pointer always.
In short, I think you mixed a few different things in this patch.
Please break it up into logical steps, one perhaps adding some new
infrastructure that you will then, in a later patch, expose using
debugfs.
It should be two patches at the very least, possibly three, right?
thanks,
greg k-h
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
