| Message ID | 20200526171821.934581-1-Jerome.Pouiller@silabs.com |
|---|---|
| Headers | show |
| Series | staging: wfx: introduce nl80211 vendor extensions | expand |
On Tue, May 26, 2020 at 07:18:11PM +0200, Jerome Pouiller wrote: > From: Jérôme Pouiller <jerome.pouiller@silabs.com> > > Hello, > > This series introduces some nl80211 vendor extensions to the wfx driver. > > This series may lead to some discussions: I've applied the first 6 patches here, until you get some answers from the wifi developers about what to do with the rest. Once you do, please fix up / change them to meet their requirements, and then resend. thanks, greg k-h
Jerome Pouiller <Jerome.Pouiller@silabs.com> writes: > This series introduces some nl80211 vendor extensions to the wfx driver. > > This series may lead to some discussions: > > 1. Patch 7 allows to change the dynamic PS timeout. I have found > an API in wext (cfg80211_wext_siwpower()) that do more or less the > same thing. However, I have not found any equivalent in nl80211. Is it > expected or this API should be ported to nl80211? struct wireless_dev::ps_timeout doesn't work for you? > > 2. The device The device allows to do Packet Traffic Arbitration (PTA or > also Coex). This feature allows the device to communicate with another > RF device in order to share the access to the RF. The patch 9 provides > a way to configure that. However, I think that this chip is not the > only one to provide this feature. Maybe a standard way to change > these parameters should be provided? > > 3. For these vendor extensions, I have used the new policy introduced by > the commit 901bb989185516 ("nl80211: require and validate vendor > command policy"). However, it seems that my version of 'iw' is not > able to follow this new policy (it does not pack the netlink > attributes into a NLA_NESTED). I could develop a tool specifically for > that API, but it is not very handy. So, in patch 10, I have also > introduced an API for compatibility with iw. Any comments about this? If you want the driver out of staging I recommend not adding any vendor commands until the driver is moved to drivers/net/wireless. Also do note that we have special rules for nl80211 vendor commands: https://wireless.wiki.kernel.org/en/developers/documentation/nl80211#vendor-specific_api
On Wednesday 27 May 2020 14:34:37 CEST Kalle Valo wrote: > Jerome Pouiller <Jerome.Pouiller@silabs.com> writes: > > > This series introduces some nl80211 vendor extensions to the wfx driver. > > > > This series may lead to some discussions: > > > > 1. Patch 7 allows to change the dynamic PS timeout. I have found > > an API in wext (cfg80211_wext_siwpower()) that do more or less the > > same thing. However, I have not found any equivalent in nl80211. Is it > > expected or this API should be ported to nl80211? > > struct wireless_dev::ps_timeout doesn't work for you? Indeed, cfg80211_wext_siwpower() modify wireless_dev::ps_timeout, but there is no equivalent in nl80211, no? Else, I choose to not directly change wireless_dev::ps_timeout because I worried about interactions with other parts of cfg80211/mac80211. > > > > 2. The device The device allows to do Packet Traffic Arbitration (PTA or > > also Coex). This feature allows the device to communicate with another > > RF device in order to share the access to the RF. The patch 9 provides > > a way to configure that. However, I think that this chip is not the > > only one to provide this feature. Maybe a standard way to change > > these parameters should be provided? > > > > 3. For these vendor extensions, I have used the new policy introduced by > > the commit 901bb989185516 ("nl80211: require and validate vendor > > command policy"). However, it seems that my version of 'iw' is not > > able to follow this new policy (it does not pack the netlink > > attributes into a NLA_NESTED). I could develop a tool specifically for > > that API, but it is not very handy. So, in patch 10, I have also > > introduced an API for compatibility with iw. Any comments about this? > > If you want the driver out of staging I recommend not adding any vendor > commands until the driver is moved to drivers/net/wireless. Also do note > that we have special rules for nl80211 vendor commands: > > https://wireless.wiki.kernel.org/en/developers/documentation/nl80211#vendor-specific_api I hoped to suggest the move of this driver outside of staging in some weeks (the last items in TODO list are either non-essential or easy to fix). So, you suggest me to resend these patches after that change?
Jérôme Pouiller <jerome.pouiller@silabs.com> writes: > On Wednesday 27 May 2020 14:34:37 CEST Kalle Valo wrote: >> Jerome Pouiller <Jerome.Pouiller@silabs.com> writes: >> >> > This series introduces some nl80211 vendor extensions to the wfx driver. >> > >> > This series may lead to some discussions: >> > >> > 1. Patch 7 allows to change the dynamic PS timeout. I have found >> > an API in wext (cfg80211_wext_siwpower()) that do more or less the >> > same thing. However, I have not found any equivalent in nl80211. Is it >> > expected or this API should be ported to nl80211? >> >> struct wireless_dev::ps_timeout doesn't work for you? > > Indeed, cfg80211_wext_siwpower() modify wireless_dev::ps_timeout, but > there is no equivalent in nl80211, no? Ah, I remember now. Something like 10 years ago there was a discussion about using qos-pm framework for modifying the timeout (or something like that, can't remember the details anymore) but no recollection what was the end result. > Else, I choose to not directly change wireless_dev::ps_timeout because I > worried about interactions with other parts of cfg80211/mac80211. This is exactly why we have strict rules for nl80211 vendor commands. We want to have generic interfaces as much as possible, not each driver coming up with their own interfaces. >> > 2. The device The device allows to do Packet Traffic Arbitration (PTA or >> > also Coex). This feature allows the device to communicate with another >> > RF device in order to share the access to the RF. The patch 9 provides >> > a way to configure that. However, I think that this chip is not the >> > only one to provide this feature. Maybe a standard way to change >> > these parameters should be provided? >> > >> > 3. For these vendor extensions, I have used the new policy introduced by >> > the commit 901bb989185516 ("nl80211: require and validate vendor >> > command policy"). However, it seems that my version of 'iw' is not >> > able to follow this new policy (it does not pack the netlink >> > attributes into a NLA_NESTED). I could develop a tool specifically for >> > that API, but it is not very handy. So, in patch 10, I have also >> > introduced an API for compatibility with iw. Any comments about this? >> >> If you want the driver out of staging I recommend not adding any vendor >> commands until the driver is moved to drivers/net/wireless. Also do note >> that we have special rules for nl80211 vendor commands: >> >> https://wireless.wiki.kernel.org/en/developers/documentation/nl80211#vendor-specific_api > > I hoped to suggest the move of this driver outside of staging in some > weeks (the last items in TODO list are either non-essential or easy to > fix). So, you suggest me to resend these patches after that change? It makes a lot easier for the review if there are no nl80211 vendor commands in the driver, most likely you would need to remove them. So yes, don't add anything unless absolutely essential until the driver is accepted upstream. The smaller the driver the faster the review.
From: Jérôme Pouiller <jerome.pouiller@silabs.com> Hello, This series introduces some nl80211 vendor extensions to the wfx driver. This series may lead to some discussions: 1. Patch 7 allows to change the dynamic PS timeout. I have found an API in wext (cfg80211_wext_siwpower()) that do more or less the same thing. However, I have not found any equivalent in nl80211. Is it expected or this API should be ported to nl80211? 2. The device The device allows to do Packet Traffic Arbitration (PTA or also Coex). This feature allows the device to communicate with another RF device in order to share the access to the RF. The patch 9 provides a way to configure that. However, I think that this chip is not the only one to provide this feature. Maybe a standard way to change these parameters should be provided? 3. For these vendor extensions, I have used the new policy introduced by the commit 901bb989185516 ("nl80211: require and validate vendor command policy"). However, it seems that my version of 'iw' is not able to follow this new policy (it does not pack the netlink attributes into a NLA_NESTED). I could develop a tool specifically for that API, but it is not very handy. So, in patch 10, I have also introduced an API for compatibility with iw. Any comments about this? Jérôme Pouiller (10): staging: wfx: drop unused variable staging: wfx: do not declare variables inside loops staging: wfx: drop unused function wfx_pending_requeue() staging: wfx: add support for tx_power_loop staging: wfx: retrieve the PS status from the vif staging: wfx: split wfx_get_ps_timeout() from wfx_update_pm() staging: wfx: add support for set/get ps_timeout staging: wfx: allow to burn prevent rollback bit staging: wfx: allow to set PTA settings staging: wfx: allow to run nl80211 vendor commands with 'iw' drivers/staging/wfx/Makefile | 3 +- drivers/staging/wfx/data_tx.c | 11 +- drivers/staging/wfx/debug.c | 26 +++++ drivers/staging/wfx/hif_api_general.h | 67 +++++++++++- drivers/staging/wfx/hif_rx.c | 7 ++ drivers/staging/wfx/hif_tx.c | 64 ++++++++++++ drivers/staging/wfx/hif_tx.h | 6 ++ drivers/staging/wfx/main.c | 6 ++ drivers/staging/wfx/nl80211_vendor.c | 143 ++++++++++++++++++++++++++ drivers/staging/wfx/nl80211_vendor.h | 93 +++++++++++++++++ drivers/staging/wfx/queue.c | 13 --- drivers/staging/wfx/queue.h | 1 - drivers/staging/wfx/sta.c | 56 ++++++---- drivers/staging/wfx/sta.h | 2 + drivers/staging/wfx/wfx.h | 7 ++ 15 files changed, 459 insertions(+), 46 deletions(-) create mode 100644 drivers/staging/wfx/nl80211_vendor.c create mode 100644 drivers/staging/wfx/nl80211_vendor.h