Message-ID: <CABicQ-V0gFyOXWZDCwve_yy6VvcLyZN8tQX2H6htJa2QyUo6Ow@mail.gmail.com>
Date:	Fri, 13 May 2016 11:22:25 +0800
From:	Wei-Ning Huang <wnhuang@...gle.com>
To:	Arend van Spriel <arend.vanspriel@...adcom.com>
Cc:	Dan Williams <dcbw@...hat.com>,
	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 Fri, May 13, 2016 at 6:02 AM, Arend van Spriel
<arend.vanspriel@...adcom.com> wrote:
> On 12-05-16 11:34, Wei-Ning Huang wrote:
>> On Thu, May 12, 2016 at 2:33 AM, Dan Williams <dcbw@...hat.com> 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 <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, 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@...gle.com | Cell: +886 910-380678
>>>>
>>>>
>>
>>
>>



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

Powered by blists - more mailing lists