Re: [PATCH 1/3] ath10k: remove ath10k_vif_to_arvif()
On 02/09/2017 11:03 PM, Valo, Kalle wrote: Ben Greear writes: On 02/07/2017 01:14 AM, Valo, Kalle wrote: Adrian Chadd writes: Removing this method makes the diff to FreeBSD larger, as "vif" in FreeBSD is a different pointer. (Yes, I have ath10k on freebsd working and I'd like to find a way to reduce the diff moving forward.) I don't like this "(void *) vif->drv_priv" style that much either but apparently it's commonly used in Linux wireless code and already parts of ath10k. So this patch just unifies the coding style. Surely the code compiles to the same thing, so why add a patch that makes it more difficult for Adrian and makes the code no easier to read for the rest of us? Because that's the coding style used already in Linux. It's great to see that parts of ath10k can be used also in other systems but in principle I'm not very fond of the idea starting to reject valid upstream patches because of driver forks. There are lots of people trying to maintain out-of-tree or backported patches to ath10k, and every time there is a meaningless style change, that just makes us waste more time on useless work instead of having time to work on more important matters. Thanks, Ben I think backports project is doing it right, it's not limiting upstream development in any way and handles all the API changes internally. Maybe FreeBSD could do something similar? -- Ben Greear Candela Technologies Inc http://www.candelatech.com
Re: [PATCH 1/3] ath10k: remove ath10k_vif_to_arvif()
On 9 February 2017 at 23:37, Joe Perches wrote: > On Thu, 2017-02-09 at 23:14 -0800, Adrian Chadd wrote: > >> If there >> were accessors for the skb data / len fields (like we do for mbufs) >> then porting the code would've involved about 5,000 less changed >> lines. > > What generic mechanisms would you suggest to make > porting easier between bsd and linux and what in > your opinion are the best naming schemes to make > these functions easiest to read and implement > without resorting to excessive identifier lengths? > > If you have some, please provide examples. (Why not, it's pre-coffee o'clock.) The biggest barriers are direct struct accessors. Most of the time the kernels have similar enough semantics that I can just implement a linux shim layer (like we do for graphics layer porting from Linux.) Eg, having skb_data(skb) (and skb_data_const(skb)) + skb_len(skb) instead of skb->data and skb->len would remove a lot of churn. Having say, a vif_to_drvpriv() method analogous to ath10k_vif_to_arvif() would also simplify the changes. For the rest of it we can just use a linux-like shim layer to get everything else working pretty darn well. But the biggest thing that helps is a quasi HAL code structure. I know HAL is a dirty word, so think of it more as "how would one separate out the OS interface layer from the rest of the driver." A good example in ath10k is the difference between say, wmi.c, the pci / copyengine code and mac.c. * the pci / copyengine code is almost 100% compilable on other platforms, save the differences in little things (malloc, free, KVA versus physical memory allocation, bounce buffering, sync'ing, etc.) A sufficiently refactored driver like ath10k where almost all of that stuff happens in the pci/copyengine code made porting that much less painful. * the wmi code is almost exclusively portable - besides the malloc/free, etc mechanical changes which honestly can be stubbed, it uses the lower layers (pci/ce, hif, htc, etc) for doing actual work, and the upper layer uses a well-defined API + callback mechanism for getting work done. Porting that was mechanical but reasonably easy. * however, the mac.c code contains both code which sends commands to the firmware (vif create/destroy, pdev commands, station associate/update/destroy, crypto key handling, peer rate control, etc) /and/ very linux mac80211/cfg80211 specific bits. If mac.c were split into mac-mac80211.c (which was /just/ mac80211, cfg80211, etc bits) and mac-utils.c (the bits that actually /sent/ the commands, responses, all the support code, etc) then my port would just implement mac-net80211.c as a completely new file, and the rest would just be modified as required by porting. A lot of the ath10k headers too mix linux specific things (eg struct device, dependencies) with hardware specific definitions for say, register accesses. I split out the register and firmware command / structures into separate header files that didn't mingle OS and driver specific structures to make it much easier to reuse that code. I find that good driver writing hygiene in any case. I'm not expecting an intel ethernet driver style HAL separation, although that'd certainly make life easier in porting over drivers. But just having inlined accessor functions for most things and some stricter driver structure for OS touch points (dma setup/teardown, bounce buffer stuff, mac80211/cfg80211, ethtool, etc APIs) would make porting and testing things a lot easier. :-) 2c, and I'll do the porting/reimplementing work anyway regardless of how much coffee it requires, -adrian
Re: [PATCH 1/3] ath10k: remove ath10k_vif_to_arvif()
On Thu, 2017-02-09 at 23:14 -0800, Adrian Chadd wrote: > If there > were accessors for the skb data / len fields (like we do for mbufs) > then porting the code would've involved about 5,000 less changed > lines. What generic mechanisms would you suggest to make porting easier between bsd and linux and what in your opinion are the best naming schemes to make these functions easiest to read and implement without resorting to excessive identifier lengths? If you have some, please provide examples.
Re: [PATCH 1/3] ath10k: remove ath10k_vif_to_arvif()
On 9 February 2017 at 23:03, Valo, Kalle wrote: > Ben Greear writes: > >> On 02/07/2017 01:14 AM, Valo, Kalle wrote: >>> Adrian Chadd writes: >>> Removing this method makes the diff to FreeBSD larger, as "vif" in FreeBSD is a different pointer. (Yes, I have ath10k on freebsd working and I'd like to find a way to reduce the diff moving forward.) >>> >>> I don't like this "(void *) vif->drv_priv" style that much either but >>> apparently it's commonly used in Linux wireless code and already parts >>> of ath10k. So this patch just unifies the coding style. >> >> Surely the code compiles to the same thing, so why add a patch that >> makes it more difficult for Adrian and makes the code no easier to read >> for the rest of us? > > Because that's the coding style used already in Linux. It's great to see > that parts of ath10k can be used also in other systems but in principle > I'm not very fond of the idea starting to reject valid upstream patches > because of driver forks. > > I think backports project is doing it right, it's not limiting upstream > development in any way and handles all the API changes internally. Maybe > FreeBSD could do something similar? I tried, but ... well, imagine renaming vif->drv_priv to something else. That's what you're suggesting. :-) You can do it with coccinelle, but not via just backports API implementations. I'm a big fan of light weight accessor APIs for the same reason. (Since FreeBSD doesn't have that pointer in ieee80211vap, it's done a different way.) If you could convert other direct uses over to ath10k_vif_to_arvif() then that'd make me happier. If not, it's fine, when I push this into freebsd and fast-forward commits, I'll have to just maintain it. For what it's worth - the linux skb accessors are the same. If there were accessors for the skb data / len fields (like we do for mbufs) then porting the code would've involved about 5,000 less changed lines. -adrian
Re: [PATCH 1/3] ath10k: remove ath10k_vif_to_arvif()
Ben Greear writes: > On 02/07/2017 01:14 AM, Valo, Kalle wrote: >> Adrian Chadd writes: >> >>> Removing this method makes the diff to FreeBSD larger, as "vif" in >>> FreeBSD is a different pointer. >>> >>> (Yes, I have ath10k on freebsd working and I'd like to find a way to >>> reduce the diff moving forward.) >> >> I don't like this "(void *) vif->drv_priv" style that much either but >> apparently it's commonly used in Linux wireless code and already parts >> of ath10k. So this patch just unifies the coding style. > > Surely the code compiles to the same thing, so why add a patch that > makes it more difficult for Adrian and makes the code no easier to read > for the rest of us? Because that's the coding style used already in Linux. It's great to see that parts of ath10k can be used also in other systems but in principle I'm not very fond of the idea starting to reject valid upstream patches because of driver forks. I think backports project is doing it right, it's not limiting upstream development in any way and handles all the API changes internally. Maybe FreeBSD could do something similar? -- Kalle Valo
Re: [PATCH 1/3] ath10k: remove ath10k_vif_to_arvif()
On 02/07/2017 01:14 AM, Valo, Kalle wrote: Adrian Chadd writes: Removing this method makes the diff to FreeBSD larger, as "vif" in FreeBSD is a different pointer. (Yes, I have ath10k on freebsd working and I'd like to find a way to reduce the diff moving forward.) I don't like this "(void *) vif->drv_priv" style that much either but apparently it's commonly used in Linux wireless code and already parts of ath10k. So this patch just unifies the coding style. Surely the code compiles to the same thing, so why add a patch that makes it more difficult for Adrian and makes the code no easier to read for the rest of us? Thanks, Ben -- Ben Greear Candela Technologies Inc http://www.candelatech.com
Re: [PATCH 1/3] ath10k: remove ath10k_vif_to_arvif()
Adrian Chadd writes: > Removing this method makes the diff to FreeBSD larger, as "vif" in > FreeBSD is a different pointer. > > (Yes, I have ath10k on freebsd working and I'd like to find a way to > reduce the diff moving forward.) I don't like this "(void *) vif->drv_priv" style that much either but apparently it's commonly used in Linux wireless code and already parts of ath10k. So this patch just unifies the coding style. -- Kalle Valo
Re: [PATCH 1/3] ath10k: remove ath10k_vif_to_arvif()
hiya, Removing this method makes the diff to FreeBSD larger, as "vif" in FreeBSD is a different pointer. (Yes, I have ath10k on freebsd working and I'd like to find a way to reduce the diff moving forward.) -adrian
[PATCH 1/3] ath10k: remove ath10k_vif_to_arvif()
it adds unnecessary level of indirection, while we just access structure field Signed-off-by: Amadeusz Sławiński --- drivers/net/wireless/ath/ath10k/mac.c | 68 +-- drivers/net/wireless/ath/ath10k/mac.h | 7 +--- drivers/net/wireless/ath/ath10k/p2p.c | 2 +- drivers/net/wireless/ath/ath10k/wmi.c | 2 +- 4 files changed, 37 insertions(+), 42 deletions(-) diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c index aa545a1..9cb96f6 100644 --- a/drivers/net/wireless/ath/ath10k/mac.c +++ b/drivers/net/wireless/ath/ath10k/mac.c @@ -1954,7 +1954,7 @@ static void ath10k_mac_handle_beacon_iter(void *data, u8 *mac, { struct sk_buff *skb = data; struct ieee80211_mgmt *mgmt = (void *)skb->data; - struct ath10k_vif *arvif = ath10k_vif_to_arvif(vif); + struct ath10k_vif *arvif = (void *)vif->drv_priv; if (vif->type != NL80211_IFTYPE_STATION) return; @@ -1977,7 +1977,7 @@ static void ath10k_mac_handle_beacon_miss_iter(void *data, u8 *mac, struct ieee80211_vif *vif) { u32 *vdev_id = data; - struct ath10k_vif *arvif = ath10k_vif_to_arvif(vif); + struct ath10k_vif *arvif = (void *)vif->drv_priv; struct ath10k *ar = arvif->ar; struct ieee80211_hw *hw = ar->hw; @@ -2044,7 +2044,7 @@ static void ath10k_peer_assoc_h_basic(struct ath10k *ar, struct ieee80211_sta *sta, struct wmi_peer_assoc_complete_arg *arg) { - struct ath10k_vif *arvif = ath10k_vif_to_arvif(vif); + struct ath10k_vif *arvif = (void *)vif->drv_priv; u32 aid; lockdep_assert_held(&ar->conf_mutex); @@ -2120,7 +2120,7 @@ static void ath10k_peer_assoc_h_rates(struct ath10k *ar, struct ieee80211_sta *sta, struct wmi_peer_assoc_complete_arg *arg) { - struct ath10k_vif *arvif = ath10k_vif_to_arvif(vif); + struct ath10k_vif *arvif = (void *)vif->drv_priv; struct wmi_rate_set_arg *rateset = &arg->peer_legacy_rates; struct cfg80211_chan_def def; const struct ieee80211_supported_band *sband; @@ -2183,7 +2183,7 @@ static void ath10k_peer_assoc_h_ht(struct ath10k *ar, struct wmi_peer_assoc_complete_arg *arg) { const struct ieee80211_sta_ht_cap *ht_cap = &sta->ht_cap; - struct ath10k_vif *arvif = ath10k_vif_to_arvif(vif); + struct ath10k_vif *arvif = (void *)vif->drv_priv; struct cfg80211_chan_def def; enum nl80211_band band; const u8 *ht_mcs_mask; @@ -2407,7 +2407,7 @@ static void ath10k_peer_assoc_h_vht(struct ath10k *ar, struct wmi_peer_assoc_complete_arg *arg) { const struct ieee80211_sta_vht_cap *vht_cap = &sta->vht_cap; - struct ath10k_vif *arvif = ath10k_vif_to_arvif(vif); + struct ath10k_vif *arvif = (void *)vif->drv_priv; struct cfg80211_chan_def def; enum nl80211_band band; const u16 *vht_mcs_mask; @@ -2465,7 +2465,7 @@ static void ath10k_peer_assoc_h_qos(struct ath10k *ar, struct ieee80211_sta *sta, struct wmi_peer_assoc_complete_arg *arg) { - struct ath10k_vif *arvif = ath10k_vif_to_arvif(vif); + struct ath10k_vif *arvif = (void *)vif->drv_priv; switch (arvif->vdev_type) { case WMI_VDEV_TYPE_AP: @@ -2505,7 +2505,7 @@ static void ath10k_peer_assoc_h_phymode(struct ath10k *ar, struct ieee80211_sta *sta, struct wmi_peer_assoc_complete_arg *arg) { - struct ath10k_vif *arvif = ath10k_vif_to_arvif(vif); + struct ath10k_vif *arvif = (void *)vif->drv_priv; struct cfg80211_chan_def def; enum nl80211_band band; const u8 *ht_mcs_mask; @@ -2625,7 +2625,7 @@ static int ath10k_mac_vif_recalc_txbf(struct ath10k *ar, struct ieee80211_vif *vif, struct ieee80211_sta_vht_cap vht_cap) { - struct ath10k_vif *arvif = ath10k_vif_to_arvif(vif); + struct ath10k_vif *arvif = (void *)vif->drv_priv; int ret; u32 param; u32 value; @@ -2692,7 +2692,7 @@ static void ath10k_bss_assoc(struct ieee80211_hw *hw, struct ieee80211_bss_conf *bss_conf) { struct ath10k *ar = hw->priv; - struct ath10k_vif *arvif = ath10k_vif_to_arvif(vif); + struct ath10k_vif *arvif = (void *)vif->drv_priv; struct ieee80211_sta_ht_cap ht_cap; struct ieee80211_sta_vht_cap vht_cap; struct wmi_peer_assoc_complete_arg peer_arg; @@ -2785,7 +2785,7 @@ static void ath10k_bss_disassoc(struct ieee80211_hw *hw, st