Re: [PATCH] cfg80211/nl80211: add wifi tx power mode switching support

[Dan Williams]

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@xxxxxxxxxx>
> wrote:
> > 
> > On Fri, May 6, 2016 at 12:07 AM, Dan Williams <dcbw@xxxxxxxxxx>
> > 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@xxxxxxxxxxxx>
> > > > ---
> > > > Â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@xxxxxxxxxx | Cell: +886 910-380678
> 
>