Return-path: Received: from mx1.redhat.com ([209.132.183.28]:51840 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932263AbcEKSdt (ORCPT ); Wed, 11 May 2016 14:33:49 -0400 Message-ID: <1462991620.22404.5.camel@redhat.com> (sfid-20160511_203418_417992_353DB203) Subject: Re: [PATCH] cfg80211/nl80211: add wifi tx power mode switching support From: Dan Williams To: Wei-Ning Huang Cc: Linux-Wireless , LKML , Johannes Berg , Sameer Nanda , Todd Broch , davem@davemloft.net, netdev@vger.kernel.org Date: Wed, 11 May 2016 13:33:40 -0500 In-Reply-To: References: <1462430663-9448-1-git-send-email-wnhuang@chromium.org> <1462464478.23962.12.camel@redhat.com> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Sender: linux-wireless-owner@vger.kernel.org List-ID: 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 > wrote: > > > > On Fri, May 6, 2016 at 12:07 AM, Dan Williams > > 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 > > > > --- > > > >  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 = > > 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@google.com | Cell: +886 910-380678 > >