remove global tsinfo debugfs variables
Signed-off-by: Zhu Yi <[email protected]>
---
net/mac80211/debugfs_netdev.c | 33 +++++++++++++++------------------
net/mac80211/ieee80211_i.h | 5 +++++
net/mac80211/ieee80211_iface.c | 8 ++++++++
3 files changed, 28 insertions(+), 18 deletions(-)
diff --git a/net/mac80211/debugfs_netdev.c b/net/mac80211/debugfs_netdev.c
index 7be0d90..a1c5fcc 100644
--- a/net/mac80211/debugfs_netdev.c
+++ b/net/mac80211/debugfs_netdev.c
@@ -87,16 +87,6 @@ static const struct file_operations name##_ops = { \
IEEE80211_IF_FMT_##format(name, field) \
__IEEE80211_IF_FILE(name)
-static struct ieee80211_elem_tspec _tspec = {
- .nominal_msdu_size = 200,
- .inactivity_interval = 40,
- .mean_data_rate = 40000,
- .min_phy_rate = 6000000,
- .surplus_band_allow = 8192,
- .medium_time = 30,
-};
-static u8 _dls_mac[ETH_ALEN];
-
#define DEBUGFS_QOS_FILE(name, f) \
static ssize_t qos_ ##name## _write(struct file *file, \
const char __user *userbuf, \
@@ -104,7 +94,7 @@ static ssize_t qos_ ##name## _write(struct file *file, \
{ \
struct ieee80211_sub_if_data *sdata = file->private_data; \
\
- f(sdata->dev, &sdata->u.sta, &_tspec); \
+ f(sdata->dev, &sdata->u.sta, &sdata->u.sta.tspec); \
\
return count; \
} \
@@ -132,7 +122,8 @@ DEBUGFS_QOS_FILE(delts_wmm, wmm_send_delts);
static ssize_t qos_if_dls_mac(const struct ieee80211_sub_if_data *sdata,
char *buf, int buflen)
{
- return scnprintf(buf, buflen, MAC_FMT "\n", MAC_ARG(_dls_mac));
+ return scnprintf(buf, buflen, MAC_FMT "\n",
+ MAC_ARG(sdata->u.sta.dls_mac));
}
static ssize_t qos_dls_mac_read(struct file *file,
@@ -163,7 +154,7 @@ static ssize_t qos_dls_mac_write(struct file *file, const char __user *userbuf,
printk(KERN_ERR "%s: sscanf input error\n", sdata->dev->name);
return -EINVAL;
}
- memcpy(_dls_mac, m, ETH_ALEN);
+ memcpy(sdata->u.sta.dls_mac, m, ETH_ALEN);
return count;
}
@@ -207,10 +198,12 @@ static ssize_t qos_dls_op_write(struct file *file, const char __user *userbuf,
}
switch (opt) {
case 1:
- ieee80211_send_dls_req(sdata->dev, &sdata->u.sta, _dls_mac, 0);
+ ieee80211_send_dls_req(sdata->dev, &sdata->u.sta,
+ sdata->u.sta.dls_mac, 0);
break;
case 2:
- ieee80211_send_dls_teardown(sdata->dev, &sdata->u.sta, _dls_mac,
+ ieee80211_send_dls_teardown(sdata->dev, &sdata->u.sta,
+ sdata->u.sta.dls_mac,
WLAN_REASON_QSTA_NOT_USE);
break;
default:
@@ -232,8 +225,9 @@ static ssize_t tsinfo_ ##_name## _read(struct file *file, \
size_t count, loff_t *ppos) \
{ \
char buf[20]; \
+ struct ieee80211_sub_if_data *sdata = file->private_data; \
int res = scnprintf(buf, count, "%u\n", \
- IEEE80211_TSINFO_## _name (_tspec.ts_info)); \
+ IEEE80211_TSINFO_## _name (sdata->u.sta.tspec.ts_info));\
return simple_read_from_buffer(userbuf, count, ppos, buf, res); \
} \
\
@@ -244,6 +238,7 @@ static ssize_t tsinfo_ ##_name## _write(struct file *file, \
char buf[20]; \
size_t size; \
int val; \
+ struct ieee80211_sub_if_data *sdata = file->private_data; \
\
size = min(sizeof(buf) - 1, count); \
buf[size] = '\0'; \
@@ -298,7 +293,8 @@ static ssize_t tspec_ ##name## _read(struct file *file, \
size_t count, loff_t *ppos) \
{ \
char buf[20]; \
- int res = scnprintf(buf, count, "%u\n", _tspec.name); \
+ struct ieee80211_sub_if_data *sdata = file->private_data; \
+ int res = scnprintf(buf, count, "%u\n", sdata->u.sta.tspec.name);\
return simple_read_from_buffer(userbuf, count, ppos, buf, res); \
} \
\
@@ -308,13 +304,14 @@ static ssize_t tspec_ ##name## _write(struct file *file, \
{ \
char buf[20]; \
size_t size; \
+ struct ieee80211_sub_if_data *sdata = file->private_data; \
\
size = min(sizeof(buf) - 1, count); \
buf[size] = '\0'; \
if (copy_from_user(buf, userbuf, size)) \
return -EFAULT; \
\
- _tspec.name = simple_strtoul(buf, NULL, 0); \
+ sdata->u.sta.tspec.name = simple_strtoul(buf, NULL, 0); \
return count; \
} \
\
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 4968723..52a85c9 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -21,6 +21,7 @@
#include <linux/workqueue.h>
#include <linux/types.h>
#include <linux/spinlock.h>
+//#include <linux/ieee80211.h>
#include <net/wireless.h>
#include "ieee80211_key.h"
#include "sta_info.h"
@@ -296,6 +297,10 @@ struct ieee80211_if_sta {
#define STA_TSDIR_NUM 2
/* EDCA: 0~7, HCCA: 8~15 */
struct sta_ts_data ts_data[STA_TSID_NUM][STA_TSDIR_NUM];
+#ifdef CONFIG_MAC80211_DEBUGFS
+ struct ieee80211_elem_tspec tspec;
+ u8 dls_mac[ETH_ALEN];
+#endif
};
diff --git a/net/mac80211/ieee80211_iface.c b/net/mac80211/ieee80211_iface.c
index c5c8bab..f4a6500 100644
--- a/net/mac80211/ieee80211_iface.c
+++ b/net/mac80211/ieee80211_iface.c
@@ -198,6 +198,14 @@ void ieee80211_if_set_type(struct net_device *dev, int type)
/* Initialize non-AP QSTA QoS Params */
ifsta->dot11EDCAAveragingPeriod = 5;
ifsta->MPDUExchangeTime = 0;
+#ifdef CONFIG_MAC80211_DEBUGFS
+ ifsta->tspec.nominal_msdu_size = 200,
+ ifsta->tspec.inactivity_interval = 40,
+ ifsta->tspec.mean_data_rate = 40000,
+ ifsta->tspec.min_phy_rate = 6000000,
+ ifsta->tspec.surplus_band_allow = 8192,
+ ifsta->tspec.medium_time = 30,
+#endif
msdata = IEEE80211_DEV_TO_SUB_IF(sdata->local->mdev);
sdata->bss = &msdata->u.ap;
--
1.5.0.rc2.g73a2
On Mon, 11 Jun 2007 10:57:32 +0800, Zhu Yi wrote:
> > Actually, now
> > that I understand it, don't you think it'd be easier to understand if
> > you had two write-only (!) files
> > * dls/teardown
> > * dls/request
> > and writing a mac address to each one would trigger the operation for
> > that mac address?
I like this idea. It should be also used for send_addts and similar - would
be better than the "write anything here to trigger the event" concept.
> > That way, you have it atomically and no problem with
> > two processes stomping each other (since now you write the mac address
> > and then the operation). That would be much closer to the nl80211
> > interface where I'd probably just have two commands NL80211_REQUEST_DLS
> > and NL80211_TEARDOWN_DLS that both take an ATTR_MAC_ADDRESS (in addition
> > to the virtual interface).
> >
> > Same for tspec, even though I haven't seen where it's used, why not have
> > a single file that accepts the whole tspec information element that
> > userspace has pieced together,
>
> When I sent the patch the first time which used sysfs, the comment was
> one value per file (Is this still true for debugfs?). So I split them
> up. The drawback for all values in one file is the user has to remember
> all the values and their orders.
Johannes probably meant a binary blob prepared by user space. That still
complies to the one value per file rule.
> The DLS is easier because it only has one parameter (peer mac address)
> now. I programed it the same way as tspec. So when we find to need more
> parameters for DLS setup, we can add another debugfs file for the new
> parameter instead of combining multiple parameters in one file.
>
> I'd agree I didn't pay a lot of attentions to the debugfs interface
> design since I thought it was used for occasional debug only. Please
> tell me what which do you prefer: one value per file or multiple values
> per file so that we can do one shot parameter passing? So I don't need
> to switch them back and forth.
Depends on how long you want to keep this debugfs interface. If it's just a
few months matter and it's never going to hit vanilla, it's probably not
worth the effort to rewrite it.
Thanks,
Jiri
--
Jiri Benc
SUSE Labs
On Fri, 2007-06-08 at 21:42 +0200, Johannes Berg wrote:
> On Sat, 2007-06-09 at 01:03 +0800, Zhu Yi wrote:
>
> > --- a/net/mac80211/ieee80211_i.h
> > +++ b/net/mac80211/ieee80211_i.h
> > @@ -21,6 +21,7 @@
> > #include <linux/workqueue.h>
> > #include <linux/types.h>
> > #include <linux/spinlock.h>
> > +//#include <linux/ieee80211.h>
>
> Is that accidentally in that patch or does that serve a purpose?
This should be removed.
> > +#ifdef CONFIG_MAC80211_DEBUGFS
> > + struct ieee80211_elem_tspec tspec;
> > + u8 dls_mac[ETH_ALEN];
> > +#endif
>
> Not sure I understand this. What's the point of the tspec element here?
> I just tried to find where it's actually used but couldn't, I guess I'm
> blind.
It is used in the debugfs_netdev.c in the same patch. Search
sdata->u.sta.tspec.
> However, why do we even need a whole bunch of these files? For the QoS
> stuff I can see what you're doing, you have the DLS MAC address in
> debugfs and then a debugfs operations file that calls
> ieee80211_send_dls_req or ieee80211_send_dls_teardown. Actually, now
> that I understand it, don't you think it'd be easier to understand if
> you had two write-only (!) files
> * dls/teardown
> * dls/request
> and writing a mac address to each one would trigger the operation for
> that mac address? That way, you have it atomically and no problem with
> two processes stomping each other (since now you write the mac address
> and then the operation). That would be much closer to the nl80211
> interface where I'd probably just have two commands NL80211_REQUEST_DLS
> and NL80211_TEARDOWN_DLS that both take an ATTR_MAC_ADDRESS (in addition
> to the virtual interface).
>
> Same for tspec, even though I haven't seen where it's used, why not have
> a single file that accepts the whole tspec information element that
> userspace has pieced together,
When I sent the patch the first time which used sysfs, the comment was
one value per file (Is this still true for debugfs?). So I split them
up. The drawback for all values in one file is the user has to remember
all the values and their orders.
The DLS is easier because it only has one parameter (peer mac address)
now. I programed it the same way as tspec. So when we find to need more
parameters for DLS setup, we can add another debugfs file for the new
parameter instead of combining multiple parameters in one file.
I'd agree I didn't pay a lot of attentions to the debugfs interface
design since I thought it was used for occasional debug only. Please
tell me what which do you prefer: one value per file or multiple values
per file so that we can do one shot parameter passing? So I don't need
to switch them back and forth.
> this is the same thing we're doing with
> the information element for inclusion in the association request (wext's
> GENIE request, which IMHO should be called ASSOC_EXTRA_IE or something,
> nl80211 has it too already). Then, this tspec information element could
> be read in one go and then used in whatever way necessary when it's
> written (after suitable sanity checks on it)
>
> johannes
On Mon, 2007-06-11 at 10:57 +0800, Zhu Yi wrote:
> When I sent the patch the first time which used sysfs, the comment was
> one value per file (Is this still true for debugfs?).
Hah, yeah, that's the regular rule for sysfs, but debugfs basically has
no rules ;)
> The DLS is easier because it only has one parameter (peer mac address)
> now. I programed it the same way as tspec. So when we find to need more
> parameters for DLS setup, we can add another debugfs file for the new
> parameter instead of combining multiple parameters in one file.
Alright. I don't think it matters much anyway.
> I'd agree I didn't pay a lot of attentions to the debugfs interface
> design since I thought it was used for occasional debug only. Please
> tell me what which do you prefer: one value per file or multiple values
> per file so that we can do one shot parameter passing? So I don't need
> to switch them back and forth.
No, don't bother, too much work for too little gain. Maybe gather some
thoughts on the nl80211 interface instead :) I personally prefer
one-shot parameter passing because it's closer to what we'll be doing
with nl80211 so we can model the interface like nl80211, however, the
interface between the debugfs and the regular mac80211 code could still
be modelled after nl80211 (it could even call some cfg80211 stuff when
that's added)
johannes
On Tue, 2007-06-12 at 13:36 +0200, Jiri Benc wrote:
> Johannes probably meant a binary blob prepared by user space. That still
> complies to the one value per file rule.
Right, but that is not easy to use (i.e with `echo` or `cat`) without
writting a user space program.
> > The DLS is easier because it only has one parameter (peer mac address)
> > now. I programed it the same way as tspec. So when we find to need more
> > parameters for DLS setup, we can add another debugfs file for the new
> > parameter instead of combining multiple parameters in one file.
> >
> > I'd agree I didn't pay a lot of attentions to the debugfs interface
> > design since I thought it was used for occasional debug only. Please
> > tell me what which do you prefer: one value per file or multiple values
> > per file so that we can do one shot parameter passing? So I don't need
> > to switch them back and forth.
>
> Depends on how long you want to keep this debugfs interface. If it's just a
> few months matter and it's never going to hit vanilla, it's probably not
> worth the effort to rewrite it.
I'm working with Johannes with the API define. When this is done,
hopfully we can remove the debugfs stuff.
Thanks,
-yi
On Sat, 2007-06-09 at 01:03 +0800, Zhu Yi wrote:
> --- a/net/mac80211/ieee80211_i.h
> +++ b/net/mac80211/ieee80211_i.h
> @@ -21,6 +21,7 @@
> #include <linux/workqueue.h>
> #include <linux/types.h>
> #include <linux/spinlock.h>
> +//#include <linux/ieee80211.h>
Is that accidentally in that patch or does that serve a purpose?
> +#ifdef CONFIG_MAC80211_DEBUGFS
> + struct ieee80211_elem_tspec tspec;
> + u8 dls_mac[ETH_ALEN];
> +#endif
Not sure I understand this. What's the point of the tspec element here?
I just tried to find where it's actually used but couldn't, I guess I'm
blind.
However, why do we even need a whole bunch of these files? For the QoS
stuff I can see what you're doing, you have the DLS MAC address in
debugfs and then a debugfs operations file that calls
ieee80211_send_dls_req or ieee80211_send_dls_teardown. Actually, now
that I understand it, don't you think it'd be easier to understand if
you had two write-only (!) files
* dls/teardown
* dls/request
and writing a mac address to each one would trigger the operation for
that mac address? That way, you have it atomically and no problem with
two processes stomping each other (since now you write the mac address
and then the operation). That would be much closer to the nl80211
interface where I'd probably just have two commands NL80211_REQUEST_DLS
and NL80211_TEARDOWN_DLS that both take an ATTR_MAC_ADDRESS (in addition
to the virtual interface).
Same for tspec, even though I haven't seen where it's used, why not have
a single file that accepts the whole tspec information element that
userspace has pieced together, this is the same thing we're doing with
the information element for inclusion in the association request (wext's
GENIE request, which IMHO should be called ASSOC_EXTRA_IE or something,
nl80211 has it too already). Then, this tspec information element could
be read in one go and then used in whatever way necessary when it's
written (after suitable sanity checks on it)
johannes