On Thu, May 12, 2016 at 2:33 AM, Dan Williams [off-list ref] wrote:

On Wed, 2016-05-11 at 13:03 +0800, Wei-Ning Huang wrote:

quoted

On Fri, May 6, 2016 at 4:19 PM, Wei-Ning Huang [off-list ref]
wrote:

quoted

On Fri, May 6, 2016 at 12:07 AM, Dan Williams [off-list ref]
wrote:

quoted

On Thu, 2016-05-05 at 14:44 +0800, Wei-Ning Huang wrote:

quoted

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 <redacted>
---
 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?

quoted

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, Thanks for the pointer. Passing the power table/calibration data
name instead of
constants sounds reasonable.

Johannes, I feel like being able to set calibration data at runtime is
something common
to all wireless drivers, so instead of using vendor commands what do
you think if I pass
the calibration data name instead of using those magic constants? This
way, userspace
does not need to know the details of what band/range power limit the
driver supports. It
allows for flexible driver side implementation and easier for
userspace to control.

Wei-Ning

Dan

quoted

Wei-Ning

quoted

quoted

Dan

quoted

+};
+
+/**
  * 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-

quoted

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@google.com | Cell: +886 910-380678

-- 
Wei-Ning Huang, 黃偉寧 | Software Engineer, Google Inc., Taiwan |
wnhuang@google.com | Cell: +886 910-380678