Return-path: Received: from mail-oi0-f50.google.com ([209.85.218.50]:34055 "EHLO mail-oi0-f50.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751502AbcEMDW0 convert rfc822-to-8bit (ORCPT ); Thu, 12 May 2016 23:22:26 -0400 Received: by mail-oi0-f50.google.com with SMTP id k142so151703330oib.1 for ; Thu, 12 May 2016 20:22:26 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <5734FD7E.7010307@broadcom.com> References: <1462430663-9448-1-git-send-email-wnhuang@chromium.org> <1462464478.23962.12.camel@redhat.com> <1462991620.22404.5.camel@redhat.com> <5734FD7E.7010307@broadcom.com> Date: Fri, 13 May 2016 11:22:25 +0800 Message-ID: (sfid-20160513_052251_668330_519E8E6D) Subject: Re: [PATCH] cfg80211/nl80211: add wifi tx power mode switching support From: Wei-Ning Huang To: Arend van Spriel Cc: Dan Williams , Linux-Wireless , LKML , Johannes Berg , Sameer Nanda , Todd Broch , davem@davemloft.net, netdev@vger.kernel.org Content-Type: text/plain; charset=UTF-8 Sender: linux-wireless-owner@vger.kernel.org List-ID: On Fri, May 13, 2016 at 6:02 AM, Arend van Spriel wrote: > On 12-05-16 11:34, Wei-Ning Huang wrote: >> On Thu, May 12, 2016 at 2:33 AM, Dan Williams wrote: >>> 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, 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 > > Not really. Only implicitly, eg. when changing to another frequency band > etc. > >> 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. > > user-space still need to have knowledge about what calibration data name > to pick for different scenarios. Also this means the driver will use > request_firmware() api to obtain the actual calibration data? I think > you then want that signed like the crda database. We usually store the calibration data in device tree, so it's only a matter of selecting the right name. Wei-Ning > > Regards, > Arend > >> It allows for flexible driver side implementation and easier for >> userspace to control. >> >> Wei-Ning >> >>> >>> 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 >>>> >>>> >> >> >> -- Wei-Ning Huang, 黃偉寧 | Software Engineer, Google Inc., Taiwan | wnhuang@google.com | Cell: +886 910-380678