| Message ID | 1519927170-11920-1-git-send-email-tamizhr@codeaurora.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Johannes Berg |
| Headers | show |
On 3/1/2018 6:59 PM, Tamizh chelvam wrote: > From: Tamizh chelvam <tamizhr@codeaurora.org> > > This patch introduces NL80211_CMD_SET_BTCOEX command and > NL80211_ATTR_BTCOEX_OP attribute to enable or disable btcoex. What kind of btcoex are we talking about here? Is it signalling between wlan and bt to get access to the shared RF. Why would it require user-space interaction? Are there no options for wlan to detect bt is in use, ie. bt hci is setup, and vice versa. Can it be indicated in platform data or device tree. Trying to understand the use-case here. > Signed-off-by: Tamizh chelvam <tamizhr@codeaurora.org> > --- > include/net/cfg80211.h | 4 ++++ > include/uapi/linux/nl80211.h | 10 ++++++++++ > net/wireless/nl80211.c | 28 ++++++++++++++++++++++++++++ > net/wireless/rdev-ops.h | 11 +++++++++++ > net/wireless/trace.h | 15 +++++++++++++++ > 5 files changed, 68 insertions(+) > > diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h > index fc40843..b0e8bf6 100644 > --- a/include/net/cfg80211.h > +++ b/include/net/cfg80211.h > @@ -2960,6 +2960,9 @@ struct cfg80211_external_auth_params { > * > * @external_auth: indicates result of offloaded authentication processing from > * user space > + * > + * @set_btcoex: This callback used to Enable/disable btcoex Bit strange to capitalize Enable here. > + * > */ > struct cfg80211_ops { > int (*suspend)(struct wiphy *wiphy, struct cfg80211_wowlan *wow); > @@ -3255,6 +3258,7 @@ struct cfg80211_ops { > const u8 *aa); > int (*external_auth)(struct wiphy *wiphy, struct net_device *dev, > struct cfg80211_external_auth_params *params); > + int (*set_btcoex)(struct wiphy *wiphy, bool enabled); > }; > > /* > diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h > index c13c843..3fb45b2 100644 > --- a/include/uapi/linux/nl80211.h > +++ b/include/uapi/linux/nl80211.h > @@ -1018,6 +1018,8 @@ > * &NL80211_ATTR_CHANNEL_WIDTH,&NL80211_ATTR_NSS attributes with its > * address(specified in &NL80211_ATTR_MAC). > * > + * @NL80211_CMD_SET_BTCOEX: Enable/Disable btcoex using %NL80211_ATTR_BTCOEX_OP. > + * > * @NL80211_CMD_MAX: highest used command number > * @__NL80211_CMD_AFTER_LAST: internal use > */ > @@ -1228,6 +1230,8 @@ enum nl80211_commands { > > NL80211_CMD_STA_OPMODE_CHANGED, > > + NL80211_CMD_SET_BTCOEX, > + > /* add new commands above here */ > > /* used to define NL80211_CMD_MAX below */ > @@ -2196,6 +2200,10 @@ enum nl80211_commands { > * @NL80211_ATTR_NSS: Station's New/updated RX_NSS value notified using this > * u8 attribute. This is used with %NL80211_CMD_STA_OPMODE_CHANGED. > * > + * @NL80211_ATTR_BTCOEX_OP: u8 attribute for driver supporting How is user-space supposed to know the driver is supporting btcoex. I do not see any advertisement in this patch. Should probably add ext_feature flag for this. > + * the btcoex feature. This will be used with %NL80211_CMD_SET_BTCOEX > + * to enable/disable btcoex. What does 'OP' stand for. Why not just call it 'ENABLE'. Also no need to make this u8. A flag attribute type seems sufficient here. > + * > * @NUM_NL80211_ATTR: total number of nl80211_attrs available > * @NL80211_ATTR_MAX: highest attribute number currently defined > * @__NL80211_ATTR_AFTER_LAST: internal use > @@ -2628,6 +2636,8 @@ enum nl80211_attrs { > NL80211_ATTR_NSS, > NL80211_ATTR_ACK_SIGNAL, > > + NL80211_ATTR_BTCOEX_OP, > + > /* add attributes here, update the policy in nl80211.c */ > > __NL80211_ATTR_AFTER_LAST, > diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c > index a910150..ebd119b 100644 > --- a/net/wireless/nl80211.c > +++ b/net/wireless/nl80211.c > @@ -422,6 +422,7 @@ enum nl80211_multicast_groups { > [NL80211_ATTR_PMK] = { .type = NLA_BINARY, .len = PMK_MAX_LEN }, > [NL80211_ATTR_SCHED_SCAN_MULTI] = { .type = NLA_FLAG }, > [NL80211_ATTR_EXTERNAL_AUTH_SUPPORT] = { .type = NLA_FLAG }, > + [NL80211_ATTR_BTCOEX_OP] = { .type = NLA_U8 }, so: [NL80211_ATTR_BTCOEX_ENABLE] = { .type = NLA_FLAG }, > }; > > /* policy for the key attributes */ > @@ -12517,6 +12518,25 @@ static int nl80211_external_auth(struct sk_buff *skb, struct genl_info *info) > return rdev_external_auth(rdev, dev, ¶ms); > } > > +static int nl80211_set_btcoex(struct sk_buff *skb, struct genl_info *info) > +{ > + struct cfg80211_registered_device *rdev = info->user_ptr[0]; > + u8 val = 0; > + > + if (!rdev->ops->set_btcoex) > + return -ENOTSUPP; For missing callback ops we generally use -EOPNOTSUPP. > + if (!info->attrs[NL80211_ATTR_BTCOEX_OP]) > + return -EINVAL; > + > + val = nla_get_u8(info->attrs[NL80211_ATTR_BTCOEX_OP]); > + > + if (val > 1) > + return -EINVAL; > + > + return rdev_set_btcoex(rdev, val); When using NLA_FLAG you can just say: + return rdev_set_btcoex(rdev, + info->attrs[NL80211_ATTR_BTCOEX_ENABLE]); and get rid of the EINVAL checks. Of course, you need to document the attribute needs to be added to enable and omitted to disable btcoex. > +} > + > #define NL80211_FLAG_NEED_WIPHY 0x01 > #define NL80211_FLAG_NEED_NETDEV 0x02 > #define NL80211_FLAG_NEED_RTNL 0x04 Regards, Arend
Arend van Spriel <arend.vanspriel@broadcom.com> writes: > On 3/1/2018 6:59 PM, Tamizh chelvam wrote: >> From: Tamizh chelvam <tamizhr@codeaurora.org> >> >> This patch introduces NL80211_CMD_SET_BTCOEX command and >> NL80211_ATTR_BTCOEX_OP attribute to enable or disable btcoex. > > What kind of btcoex are we talking about here? Is it signalling > between wlan and bt to get access to the shared RF. Yes, at least that's how I understand this. > Why would it require user-space interaction? Are there no options for > wlan to detect bt is in use, ie. bt hci is setup, and vice versa. Can > it be indicated in platform data or device tree. Trying to understand > the use-case here. One use case is being able to disable btcoex in case of problems or to test if it's btcoex related. I think during the last five years the need for this interface has come every once in a while.
+ Marcel On 3/2/2018 6:14 AM, Kalle Valo wrote: > Arend van Spriel <arend.vanspriel@broadcom.com> writes: > >> On 3/1/2018 6:59 PM, Tamizh chelvam wrote: >>> From: Tamizh chelvam <tamizhr@codeaurora.org> >>> >>> This patch introduces NL80211_CMD_SET_BTCOEX command and >>> NL80211_ATTR_BTCOEX_OP attribute to enable or disable btcoex. >> >> What kind of btcoex are we talking about here? Is it signalling >> between wlan and bt to get access to the shared RF. > > Yes, at least that's how I understand this. > >> Why would it require user-space interaction? Are there no options for >> wlan to detect bt is in use, ie. bt hci is setup, and vice versa. Can >> it be indicated in platform data or device tree. Trying to understand >> the use-case here. > > One use case is being able to disable btcoex in case of problems or to > test if it's btcoex related. I think during the last five years the need > for this interface has come every once in a while. Well, you would want to disable btcoex *and* bt to verify wlan is working properly on its own. And similarly disable btcoex *and* wlan to verify bt works properly. Now I do recall a thread between you and Marcel. Looked it up and it was this thread [1], but did not see a follow-up on it. I suspect it involves more than just an enable/disable state. That may be fine for devices in which BT and WLAN are integrated and coordination of RF use is done on the device. The "btcoex subsystem" thread seems to aim for more like providing the coordination logic so independent BT device and WLAN device can still use the same RF. So before adopting the api in nl80211 it would be good to revive that thread. Regards, Arend [1] https://www.spinics.net/lists/linux-wireless/msg133333.html
Hi Arend, >>>> This patch introduces NL80211_CMD_SET_BTCOEX command and >>>> NL80211_ATTR_BTCOEX_OP attribute to enable or disable btcoex. >>> >>> What kind of btcoex are we talking about here? Is it signalling >>> between wlan and bt to get access to the shared RF. >> >> Yes, at least that's how I understand this. >> >>> Why would it require user-space interaction? Are there no options for >>> wlan to detect bt is in use, ie. bt hci is setup, and vice versa. Can >>> it be indicated in platform data or device tree. Trying to understand >>> the use-case here. >> >> One use case is being able to disable btcoex in case of problems or to >> test if it's btcoex related. I think during the last five years the need >> for this interface has come every once in a while. > > Well, you would want to disable btcoex *and* bt to verify wlan is working properly on its own. And similarly disable btcoex *and* wlan to verify bt works properly. > > Now I do recall a thread between you and Marcel. Looked it up and it was this thread [1], but did not see a follow-up on it. I suspect it involves more than just an enable/disable state. That may be fine for devices in which BT and WLAN are integrated and coordination of RF use is done on the device. The "btcoex subsystem" thread seems to aim for more like providing the coordination logic so independent BT device and WLAN device can still use the same RF. So before adopting the api in nl80211 it would be good to revive that thread. actually we can solve any number of WiFi devices vs any number of Bluetooth devices on the same host. It goes mainly in the direction of WiFi having a notifier about its currently used channels and Bluetooth subsystem can subscribe to these and issue AFH channel map classification updates as needed. The other way around for the Bluetooth audio cases and a shared antenna, you need a tighter integration. If the antenna is not shared, then it makes no difference anyway since you are not sharing the transceiver. Normally these are just handled by the firmware internally and right priorities for RF scheduling are done. The Bluetooth controller normally then does a bit too much, but nobody wants to implement that in the host cleanly, and so they all hack it into the controller. I think that Realtek has some older chips where it would be really needed in the host. And Intel controllers can also let the host do it, but by default it is magically done by the controller. Regards Marcel
On 3/2/2018 10:59 AM, Marcel Holtmann wrote: > Hi Arend, > >>>>> This patch introduces NL80211_CMD_SET_BTCOEX command and >>>>> NL80211_ATTR_BTCOEX_OP attribute to enable or disable btcoex. >>>> >>>> What kind of btcoex are we talking about here? Is it signalling >>>> between wlan and bt to get access to the shared RF. >>> >>> Yes, at least that's how I understand this. >>> >>>> Why would it require user-space interaction? Are there no options for >>>> wlan to detect bt is in use, ie. bt hci is setup, and vice versa. Can >>>> it be indicated in platform data or device tree. Trying to understand >>>> the use-case here. >>> >>> One use case is being able to disable btcoex in case of problems or to >>> test if it's btcoex related. I think during the last five years the need >>> for this interface has come every once in a while. >> >> Well, you would want to disable btcoex *and* bt to verify wlan is working properly on its own. And similarly disable btcoex *and* wlan to verify bt works properly. >> >> Now I do recall a thread between you and Marcel. Looked it up and it was this thread [1], but did not see a follow-up on it. I suspect it involves more than just an enable/disable state. That may be fine for devices in which BT and WLAN are integrated and coordination of RF use is done on the device. The "btcoex subsystem" thread seems to aim for more like providing the coordination logic so independent BT device and WLAN device can still use the same RF. So before adopting the api in nl80211 it would be good to revive that thread. > > actually we can solve any number of WiFi devices vs any number of Bluetooth devices on the same host. It goes mainly in the direction of WiFi having a notifier about its currently used channels and Bluetooth subsystem can subscribe to these and issue AFH channel map classification updates as needed. I see. From this I would say the notifier functionality would sit nicely in cfg80211. > The other way around for the Bluetooth audio cases and a shared antenna, you need a tighter integration. If the antenna is not shared, then it makes no difference anyway since you are not sharing the transceiver. Normally these are just handled by the firmware internally and right priorities for RF scheduling are done. The Bluetooth controller normally then does a bit too much, but nobody wants to implement that in the host cleanly, and so they all hack it into the controller. I think that Realtek has some older chips where it would be really needed in the host. And Intel controllers can also let the host do it, but by default it is magically done by the controller. Not entirely sure what you mean by "does a bit too much", but this is what I considered as being btcoex. Indeed most do the whole coordination in the device and not on the host. Pretty sure about Broadcom ;-) Regards, Arend
Arend van Spriel <arend.vanspriel@broadcom.com> writes: > + Marcel > > On 3/2/2018 6:14 AM, Kalle Valo wrote: >> Arend van Spriel <arend.vanspriel@broadcom.com> writes: >> >>> On 3/1/2018 6:59 PM, Tamizh chelvam wrote: >>>> From: Tamizh chelvam <tamizhr@codeaurora.org> >>>> >>>> This patch introduces NL80211_CMD_SET_BTCOEX command and >>>> NL80211_ATTR_BTCOEX_OP attribute to enable or disable btcoex. >>> >>> What kind of btcoex are we talking about here? Is it signalling >>> between wlan and bt to get access to the shared RF. >> >> Yes, at least that's how I understand this. >> >>> Why would it require user-space interaction? Are there no options for >>> wlan to detect bt is in use, ie. bt hci is setup, and vice versa. Can >>> it be indicated in platform data or device tree. Trying to understand >>> the use-case here. >> >> One use case is being able to disable btcoex in case of problems or to >> test if it's btcoex related. I think during the last five years the need >> for this interface has come every once in a while. > > Well, you would want to disable btcoex *and* bt to verify wlan is > working properly on its own. And similarly disable btcoex *and* wlan > to verify bt works properly. Sure. But my main motiviation with this is to replace the module parameters we already have: drivers/net/wireless/ath/ath9k/htc_drv_init.c:module_param_named(btcoex_enable, ath9k_htc_btcoex_enable, int, 0444); drivers/net/wireless/ath/ath9k/htc_drv_init.c:MODULE_PARM_DESC(btcoex_enable, "Enable wifi-BT coexistence"); drivers/net/wireless/ath/ath9k/init.c:module_param_named(btcoex_enable, ath9k_btcoex_enable, int, 0444); drivers/net/wireless/ath/ath9k/init.c:MODULE_PARM_DESC(btcoex_enable, "Enable wifi-BT coexistence"); drivers/net/wireless/broadcom/b43/main.c:module_param_named(btcoex, modparam_btcoex, int, 0444); drivers/net/wireless/broadcom/b43/main.c:MODULE_PARM_DESC(btcoex, "Enable Bluetooth coexistence (default on)"); But looking closely in ath9k cases it doesn't actually sound doable as IIRC the ath9k module parameters are off by default. In b43 it's enabled by default, though. > Now I do recall a thread between you and Marcel. Looked it up and it > was this thread [1], but did not see a follow-up on it. I suspect it > involves more than just an enable/disable state. That may be fine for > devices in which BT and WLAN are integrated and coordination of RF use > is done on the device. The "btcoex subsystem" thread seems to aim for > more like providing the coordination logic so independent BT device > and WLAN device can still use the same RF. So before adopting the api > in nl80211 it would be good to revive that thread. I think that "btcoex subsystem" is a holy grail which will be never implemented :) But who knows, miracles can happen...
diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h index fc40843..b0e8bf6 100644 --- a/include/net/cfg80211.h +++ b/include/net/cfg80211.h @@ -2960,6 +2960,9 @@ struct cfg80211_external_auth_params { * * @external_auth: indicates result of offloaded authentication processing from * user space + * + * @set_btcoex: This callback used to Enable/disable btcoex + * */ struct cfg80211_ops { int (*suspend)(struct wiphy *wiphy, struct cfg80211_wowlan *wow); @@ -3255,6 +3258,7 @@ struct cfg80211_ops { const u8 *aa); int (*external_auth)(struct wiphy *wiphy, struct net_device *dev, struct cfg80211_external_auth_params *params); + int (*set_btcoex)(struct wiphy *wiphy, bool enabled); }; /* diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h index c13c843..3fb45b2 100644 --- a/include/uapi/linux/nl80211.h +++ b/include/uapi/linux/nl80211.h @@ -1018,6 +1018,8 @@ * &NL80211_ATTR_CHANNEL_WIDTH,&NL80211_ATTR_NSS attributes with its * address(specified in &NL80211_ATTR_MAC). * + * @NL80211_CMD_SET_BTCOEX: Enable/Disable btcoex using %NL80211_ATTR_BTCOEX_OP. + * * @NL80211_CMD_MAX: highest used command number * @__NL80211_CMD_AFTER_LAST: internal use */ @@ -1228,6 +1230,8 @@ enum nl80211_commands { NL80211_CMD_STA_OPMODE_CHANGED, + NL80211_CMD_SET_BTCOEX, + /* add new commands above here */ /* used to define NL80211_CMD_MAX below */ @@ -2196,6 +2200,10 @@ enum nl80211_commands { * @NL80211_ATTR_NSS: Station's New/updated RX_NSS value notified using this * u8 attribute. This is used with %NL80211_CMD_STA_OPMODE_CHANGED. * + * @NL80211_ATTR_BTCOEX_OP: u8 attribute for driver supporting + * the btcoex feature. This will be used with %NL80211_CMD_SET_BTCOEX + * to enable/disable btcoex. + * * @NUM_NL80211_ATTR: total number of nl80211_attrs available * @NL80211_ATTR_MAX: highest attribute number currently defined * @__NL80211_ATTR_AFTER_LAST: internal use @@ -2628,6 +2636,8 @@ enum nl80211_attrs { NL80211_ATTR_NSS, NL80211_ATTR_ACK_SIGNAL, + NL80211_ATTR_BTCOEX_OP, + /* add attributes here, update the policy in nl80211.c */ __NL80211_ATTR_AFTER_LAST, diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c index a910150..ebd119b 100644 --- a/net/wireless/nl80211.c +++ b/net/wireless/nl80211.c @@ -422,6 +422,7 @@ enum nl80211_multicast_groups { [NL80211_ATTR_PMK] = { .type = NLA_BINARY, .len = PMK_MAX_LEN }, [NL80211_ATTR_SCHED_SCAN_MULTI] = { .type = NLA_FLAG }, [NL80211_ATTR_EXTERNAL_AUTH_SUPPORT] = { .type = NLA_FLAG }, + [NL80211_ATTR_BTCOEX_OP] = { .type = NLA_U8 }, }; /* policy for the key attributes */ @@ -12517,6 +12518,25 @@ static int nl80211_external_auth(struct sk_buff *skb, struct genl_info *info) return rdev_external_auth(rdev, dev, ¶ms); } +static int nl80211_set_btcoex(struct sk_buff *skb, struct genl_info *info) +{ + struct cfg80211_registered_device *rdev = info->user_ptr[0]; + u8 val = 0; + + if (!rdev->ops->set_btcoex) + return -ENOTSUPP; + + if (!info->attrs[NL80211_ATTR_BTCOEX_OP]) + return -EINVAL; + + val = nla_get_u8(info->attrs[NL80211_ATTR_BTCOEX_OP]); + + if (val > 1) + return -EINVAL; + + return rdev_set_btcoex(rdev, val); +} + #define NL80211_FLAG_NEED_WIPHY 0x01 #define NL80211_FLAG_NEED_NETDEV 0x02 #define NL80211_FLAG_NEED_RTNL 0x04 @@ -13420,6 +13440,14 @@ static void nl80211_post_doit(const struct genl_ops *ops, struct sk_buff *skb, .internal_flags = NL80211_FLAG_NEED_NETDEV_UP | NL80211_FLAG_NEED_RTNL, }, + { + .cmd = NL80211_CMD_SET_BTCOEX, + .doit = nl80211_set_btcoex, + .policy = nl80211_policy, + .flags = GENL_UNS_ADMIN_PERM, + .internal_flags = NL80211_FLAG_NEED_WIPHY | + NL80211_FLAG_NEED_RTNL, + }, }; diff --git a/net/wireless/rdev-ops.h b/net/wireless/rdev-ops.h index 84f23ae..4002f2c 100644 --- a/net/wireless/rdev-ops.h +++ b/net/wireless/rdev-ops.h @@ -1205,4 +1205,15 @@ static inline int rdev_del_pmk(struct cfg80211_registered_device *rdev, return ret; } +static inline int +rdev_set_btcoex(struct cfg80211_registered_device *rdev, bool enabled) +{ + int ret; + + trace_rdev_set_btcoex(&rdev->wiphy, enabled); + ret = rdev->ops->set_btcoex(&rdev->wiphy, enabled); + trace_rdev_return_int(&rdev->wiphy, ret); + return ret; +} + #endif /* __CFG80211_RDEV_OPS */ diff --git a/net/wireless/trace.h b/net/wireless/trace.h index 5152938..838ec11 100644 --- a/net/wireless/trace.h +++ b/net/wireless/trace.h @@ -3173,6 +3173,21 @@ WIPHY_PR_ARG, __entry->n_rules) ); +TRACE_EVENT(rdev_set_btcoex, + TP_PROTO(struct wiphy *wiphy, bool enabled), + TP_ARGS(wiphy, enabled), + TP_STRUCT__entry( + WIPHY_ENTRY + __field(bool, enabled) + ), + TP_fast_assign( + WIPHY_ASSIGN; + __entry->enabled = enabled; + ), + TP_printk(WIPHY_PR_FMT ", enabled=%d ", + WIPHY_PR_ARG, __entry->enabled) +); + DEFINE_EVENT(wiphy_wdev_evt, rdev_abort_scan, TP_PROTO(struct wiphy *wiphy, struct wireless_dev *wdev), TP_ARGS(wiphy, wdev)