| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <1462991620.22404.5.camel@redhat.com>
Date: Wed, 11 May 2016 13:33:40 -0500
From: Dan Williams <dcbw@...hat.com>
To: Wei-Ning Huang <wnhuang@...gle.com>
Cc: Linux-Wireless <linux-wireless@...r.kernel.org>,
LKML <linux-kernel@...r.kernel.org>,
Johannes Berg <johannes@...solutions.net>,
Sameer Nanda <snanda@...omium.org>,
Todd Broch <tbroch@...omium.org>, davem@...emloft.net,
netdev@...r.kernel.org
Subject: Re: [PATCH] cfg80211/nl80211: add wifi tx power mode switching
support
On Wed, 2016-05-11 at 13:03 +0800, Wei-Ning Huang wrote:
> On Fri, May 6, 2016 at 4:19 PM, Wei-Ning Huang <wnhuang@...gle.com>
> wrote:
> >
> > On Fri, May 6, 2016 at 12:07 AM, Dan Williams <dcbw@...hat.com>
> > wrote:
> > >
> > >
> > > On Thu, 2016-05-05 at 14:44 +0800, Wei-Ning Huang wrote:
> > > >
> > > > Recent new hardware has the ability to switch between tablet
> > > > mode and
> > > > clamshell mode. To optimize WiFi performance, we want to be
> > > > able to
> > > > use
> > > > different power table between modes. This patch adds a new
> > > > netlink
> > > > message type and cfg80211_ops function to allow userspace to
> > > > trigger
> > > > a
> > > > power mode switch for a given wireless interface.
> > > >
> > > > Signed-off-by: Wei-Ning Huang <wnhuang@...omium.org>
> > > > ---
> > > > include/net/cfg80211.h | 11 +++++++++++
> > > > include/uapi/linux/nl80211.h | 21 +++++++++++++++++++++
> > > > net/wireless/nl80211.c | 16 ++++++++++++++++
> > > > net/wireless/rdev-ops.h | 22 ++++++++++++++++++++++
> > > > net/wireless/trace.h | 20 ++++++++++++++++++++
> > > > 5 files changed, 90 insertions(+)
> > > >
> > > > diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
> > > > index 9e1b24c..aa77fa0 100644
> > > > --- a/include/net/cfg80211.h
> > > > +++ b/include/net/cfg80211.h
> > > > @@ -2370,6 +2370,12 @@ struct cfg80211_qos_map {
> > > > * @get_tx_power: store the current TX power into the dbm
> > > > variable;
> > > > * return 0 if successful
> > > > *
> > > > + * @set_tx_power_mode: set the transmit power mode. Some
> > > > device have
> > > > the ability
> > > > + * to transform between different mode such as clamshell and
> > > > tablet mode.
> > > > + * set_tx_power_mode allows setting of different TX power
> > > > mode at runtime.
> > > > + * @get_tx_power_mode: store the current TX power mode into
> > > > the mode
> > > > variable;
> > > > + * return 0 if successful
> > > > + *
> > > > * @set_wds_peer: set the WDS peer for a WDS interface
> > > > *
> > > > * @rfkill_poll: polls the hw rfkill line, use cfg80211
> > > > reporting
> > > > @@ -2631,6 +2637,11 @@ struct cfg80211_ops {
> > > > int (*get_tx_power)(struct wiphy *wiphy, struct
> > > > wireless_dev *wdev,
> > > > int *dbm);
> > > >
> > > > + int (*set_tx_power_mode)(struct wiphy *wiphy,
> > > > + enum nl80211_tx_power_mode
> > > > mode);
> > > > + int (*get_tx_power_mode)(struct wiphy *wiphy,
> > > > + enum nl80211_tx_power_mode
> > > > *mode);
> > > > +
> > > > int (*set_wds_peer)(struct wiphy *wiphy, struct
> > > > net_device *dev,
> > > > const u8 *addr);
> > > >
> > > > diff --git a/include/uapi/linux/nl80211.h
> > > > b/include/uapi/linux/nl80211.h
> > > > index 5a30a75..9b1888a 100644
> > > > --- a/include/uapi/linux/nl80211.h
> > > > +++ b/include/uapi/linux/nl80211.h
> > > > @@ -1796,6 +1796,9 @@ enum nl80211_commands {
> > > > * connecting to a PCP, and in %NL80211_CMD_START_AP to
> > > > start
> > > > * a PCP instead of AP. Relevant for DMG networks only.
> > > > *
> > > > + * @NL80211_ATTR_WIPHY_TX_POWER_MODE: Transmit power mode. See
> > > > + * &enum nl80211_tx_power_mode for possible values.
> > > > + *
> > > > * @NUM_NL80211_ATTR: total number of nl80211_attrs available
> > > > * @NL80211_ATTR_MAX: highest attribute number currently
> > > > defined
> > > > * @__NL80211_ATTR_AFTER_LAST: internal use
> > > > @@ -2172,6 +2175,8 @@ enum nl80211_attrs {
> > > >
> > > > NL80211_ATTR_PBSS,
> > > >
> > > > + NL80211_ATTR_WIPHY_TX_POWER_MODE,
> > > > +
> > > > /* add attributes here, update the policy in nl80211.c */
> > > >
> > > > __NL80211_ATTR_AFTER_LAST,
> > > > @@ -3703,6 +3708,22 @@ enum nl80211_tx_power_setting {
> > > > };
> > > >
> > > > /**
> > > > + * enum nl80211_tx_power_mode - TX power mode setting
> > > > + * @NL80211_TX_POWER_LOW: general low TX power mode
> > > > + * @NL80211_TX_POWER_MEDIUM: general medium TX power mode
> > > > + * @NL80211_TX_POWER_HIGH: general high TX power mode
> > > > + * @NL80211_TX_POWER_CLAMSHELL: clamshell mode TX power mode
> > > > + * @NL80211_TX_POWER_TABLET: tablet mode TX power mode
> > > > + */
> > > > +enum nl80211_tx_power_mode {
> > > > + NL80211_TX_POWER_LOW,
> > > > + NL80211_TX_POWER_MEDIUM,
> > > > + NL80211_TX_POWER_HIGH,
> > > > + NL80211_TX_POWER_CLAMSHELL,
> > > > + NL80211_TX_POWER_TABLET,
> > >
> > > "clamshell" and "tablet" probably mean many different things to
> > > many
> > > different people with respect to whether or not they should do
> > > anything
> > > with power saving or wifi. I feel like a more generic interface
> > > is
> > > needed here.
> > We could probably drop those two CLAMSHELL and TABLET constant or
> > describing what they mean
> > in more detail?
> >
> > >
> > >
> > > Could this be already done by:
> > > @NL80211_ATTR_WIPHY_TX_POWER_SETTING = NL80211_TX_POWER_LIMITED
> > > @NL80211_ATTR_WIPHY_TX_POWER_LEVEL = <limit for clamshell/tablet
> > > mode>
> > >
> > > and then the device would be able to change its TX power as it
> > > saw fit
> > > up to that limit set by your application which figures out
> > > whether it's
> > > in clamshell or tablet mode?
> > We usually want different power settings in different band/channel.
> > For example, we can have three different power settings
> > in 2.4Ghz, channels 36-64 & channels 100+ on 5Ghz. In this case, we
> > can not simply set a fixed number to the power level.
> > A power table is required to map channel/band to actual power
> > limit.
> > For most of the driver, changing power table requires loading
> > a new set of calibration data from either devicetree or ACPI. Due
> > to
> > this, a new message type is required to allow the driver to
> > switch between tables.
> >
> > Wei-Ning
> Hi Dan,
>
> Does this make sense to you? If so I'll send another patch to clarify
> the term clamshell / tablet mode.
> Thanks for reviewing.
Yes, you're right that NL80211_ATTR_WIPHY_TX_POWER_SETTING is probably
too limiting. But I feel that the constants that you've proposed are
too "magic" and will mean many different things to different driver
writers and userspace clients. I think it would be better implemented
as a request to simply load specific power calibration data or a new
power table, instead of attempting to classify power tables through
kernel constants that could mean something different to everyone.
Dan
> Wei-Ning
> >
> > >
> > >
> > >
> > > Dan
> > >
> > > >
> > > > +};
> > > > +
> > > > +/**
> > > > * enum nl80211_packet_pattern_attr - packet pattern attribute
> > > > * @__NL80211_PKTPAT_INVALID: invalid number for nested
> > > > attribute
> > > > * @NL80211_PKTPAT_PATTERN: the pattern, values where the mask
> > > > has
> > > > diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
> > > > index 056a730..61b474d 100644
> > > > --- a/net/wireless/nl80211.c
> > > > +++ b/net/wireless/nl80211.c
> > > > @@ -402,6 +402,7 @@ static const struct nla_policy
> > > > nl80211_policy[NUM_NL80211_ATTR] = {
> > > > [NL80211_ATTR_SCHED_SCAN_DELAY] = { .type = NLA_U32 },
> > > > [NL80211_ATTR_REG_INDOOR] = { .type = NLA_FLAG },
> > > > [NL80211_ATTR_PBSS] = { .type = NLA_FLAG },
> > > > + [NL80211_ATTR_WIPHY_TX_POWER_MODE] = { .type = NLA_U32 },
> > > > };
> > > >
> > > > /* policy for the key attributes */
> > > > @@ -2218,6 +2219,21 @@ static int nl80211_set_wiphy(struct
> > > > sk_buff
> > > > *skb, struct genl_info *info)
> > > > return result;
> > > > }
> > > >
> > > > + if (info->attrs[NL80211_ATTR_WIPHY_TX_POWER_MODE]) {
> > > > + enum nl80211_tx_power_mode mode;
> > > > + int idx = 0;
> > > > +
> > > > + if (!rdev->ops->set_tx_power_mode)
> > > > + return -EOPNOTSUPP;
> > > > +
> > > > + idx = NL80211_ATTR_WIPHY_TX_POWER_MODE;
> > > > + mode = nla_get_u32(info->attrs[idx]);
> > > > +
> > > > + result = rdev_set_tx_power_mode(rdev, mode);
> > > > + if (result)
> > > > + return result;
> > > > + }
> > > > +
> > > > if (info->attrs[NL80211_ATTR_WIPHY_ANTENNA_TX] &&
> > > > info->attrs[NL80211_ATTR_WIPHY_ANTENNA_RX]) {
> > > > u32 tx_ant, rx_ant;
> > > > diff --git a/net/wireless/rdev-ops.h b/net/wireless/rdev-ops.h
> > > > index 8ae0c04..c3a1035 100644
> > > > --- a/net/wireless/rdev-ops.h
> > > > +++ b/net/wireless/rdev-ops.h
> > > > @@ -552,6 +552,28 @@ static inline int rdev_get_tx_power(struct
> > > > cfg80211_registered_device *rdev,
> > > > return ret;
> > > > }
> > > >
> > > > +static inline int
> > > > +rdev_set_tx_power_mode(struct cfg80211_registered_device
> > > > *rdev,
> > > > + enum nl80211_tx_power_mode mode)
> > > > +{
> > > > + int ret;
> > > > + trace_rdev_set_tx_power_mode(&rdev->wiphy, mode);
> > > > + ret = rdev->ops->set_tx_power_mode(&rdev->wiphy, mode);
> > > > + trace_rdev_return_int(&rdev->wiphy, ret);
> > > > + return ret;
> > > > +}
> > > > +
> > > > +static inline int
> > > > +rdev_get_tx_power_mode(struct cfg80211_registered_device
> > > > *rdev,
> > > > + enum nl80211_tx_power_mode *mode)
> > > > +{
> > > > + int ret;
> > > > + trace_rdev_get_tx_power_mode(&rdev->wiphy);
> > > > + ret = rdev->ops->get_tx_power_mode(&rdev->wiphy, mode);
> > > > + trace_rdev_return_int_int(&rdev->wiphy, ret, *mode);
> > > > + return ret;
> > > > +}
> > > > +
> > > > static inline int rdev_set_wds_peer(struct
> > > > cfg80211_registered_device *rdev,
> > > > struct net_device *dev, const
> > > > u8
> > > > *addr)
> > > > {
> > > > diff --git a/net/wireless/trace.h b/net/wireless/trace.h
> > > > index 09b242b..132c8c2 100644
> > > > --- a/net/wireless/trace.h
> > > > +++ b/net/wireless/trace.h
> > > > @@ -1420,6 +1420,26 @@ TRACE_EVENT(rdev_set_tx_power,
> > > > WIPHY_PR_ARG, WDEV_PR_ARG,__entry->type,
> > > > __entry-
> > > > >
> > > > > mbm)
> > > > );
> > > >
> > > > +DEFINE_EVENT(wiphy_only_evt, rdev_get_tx_power_mode,
> > > > + TP_PROTO(struct wiphy *wiphy),
> > > > + TP_ARGS(wiphy)
> > > > +);
> > > > +
> > > > +TRACE_EVENT(rdev_set_tx_power_mode,
> > > > + TP_PROTO(struct wiphy *wiphy, enum nl80211_tx_power_mode
> > > > mode),
> > > > + TP_ARGS(wiphy, mode),
> > > > + TP_STRUCT__entry(
> > > > + WIPHY_ENTRY
> > > > + __field(enum nl80211_tx_power_mode, mode)
> > > > + ),
> > > > + TP_fast_assign(
> > > > + WIPHY_ASSIGN;
> > > > + __entry->mode = mode;
> > > > + ),
> > > > + TP_printk(WIPHY_PR_FMT ", mode: %d",
> > > > + WIPHY_PR_ARG, __entry->mode)
> > > > +);
> > > > +
> > > > TRACE_EVENT(rdev_return_int_int,
> > > > TP_PROTO(struct wiphy *wiphy, int func_ret, int
> > > > func_fill),
> > > > TP_ARGS(wiphy, func_ret, func_fill),
> >
> >
> >
> > --
> > Wei-Ning Huang, 黃偉寧 | Software Engineer, Google Inc., Taiwan |
> > wnhuang@...gle.com | Cell: +886 910-380678
>
>
Powered by blists - more mailing lists