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

[Wei-Ning Huang]

Hi Johannes,

Thanks for the reply.

You are right that the physical antenna does not change.
When I refer to 'calibration data', it actually corresponding to how
mwifiex adjust the per-band tx power. For mwifiex, the per-band tx
power is pre-calculated based on need, and stored in DT, a vendor
command or std nl80211 message is sent
to tell the driver to switch between two set of "calibration data".
I'm aware that iwl7000 is using a vendor command to do this as well,
but instead of pre-calculate required tx power info, the tx value can
be passed along with the vendor command message.

This patch was sent originally to standardize the requirement of
sending a vendor command to the driver (so it'll be a standard nl80211
message). However, we have decided to move along with vendor command
for both mwifiex and nl80211, so this patch is not needed anymore.
Thanks for the comments!

Wei-Ning

On Tue, Jun 28, 2016 at 6:57 PM, Johannes Berg
 wrote:
> On Thu, 2016-05-12 at 17:34 +0800, Wei-Ning Huang wrote:
>>
>> 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.
>>
>
> Sorry - I dropped this thread accidentally.
>
> I'm not really sure I understand the situation fully, but right now to
> me this seems very strange.
>
> The physical antennas probably don't really change between "clamshell"
> and "tablet" mode, do the physical radiation properties change enough
> to actually require different *calibration*? To me, that sounds very
> strange.
>
> Assuming they don't really change fundamentally, then I understand the
> need to set different power levels, per band/channel/whatever
> granularity. But that can be achieved in very different ways, and in
> fact if you look at Chrome then for our iwl7000 driver there we do have
> a command to do something similar (currently a vendor command, but that
> can be changed) without ever changing the *calibration*.
>
> So to me, the whole premise of the patch is confusing and/or wrong.
>
> johannes



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

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

[Johannes Berg]

On Thu, 2016-05-12 at 17:34 +0800, Wei-Ning Huang wrote:
> 
> 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.
> 

Sorry - I dropped this thread accidentally.

I'm not really sure I understand the situation fully, but right now to
me this seems very strange.

The physical antennas probably don't really change between "clamshell"
and "tablet" mode, do the physical radiation properties change enough
to actually require different *calibration*? To me, that sounds very
strange.

Assuming they don't really change fundamentally, then I understand the
need to set different power levels, per band/channel/whatever
granularity. But that can be achieved in very different ways, and in
fact if you look at Chrome then for our iwl7000 driver there we do have
a command to do something similar (currently a vendor command, but that
can be changed) without ever changing the *calibration*.

So to me, the whole premise of the patch is confusing and/or wrong.

johannes

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

[Wei-Ning Huang]

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
>>> + *   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
>> 

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

[Arend van Spriel]

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
>> + *   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 

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

[Wei-Ning Huang]

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
>> > > > + *   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 

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 
> 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
> > > > + *   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?
> > 

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

[Johannes Berg]

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.

I'm not a fan of this at all. Too much magic.

FWIW, we implemented something similar, currently with vendor commands,
that simply allows to set the TX power in a more controlled fashion,
i.e. allows setting TX power per band/channel range. For our device
it's only 2.4 GHz vs. 5.2 GHz, but I can imagine that you might want to
split it a bit further, so perhaps we need to advertise those ranges
somehow.

johannes

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

[Wei-Ning Huang]

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
>> > + *   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 = 
>>
>> 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 

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

[Wei-Ning Huang]

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
> > + *   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 = 
>
> 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 

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

[Dan Williams]

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
> + *   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.

Could this be already done by:
@NL80211_ATTR_WIPHY_TX_POWER_SETTING = NL80211_TX_POWER_LIMITED
@NL80211_ATTR_WIPHY_TX_POWER_LEVEL = 

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?

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