2017-02-02 10:19:53

by Amadeusz Sławiński

[permalink] [raw]
Subject: [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 <[email protected]>
---
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,
struct ieee80211_vif *vif)
{
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_vht_cap vht_cap = {};
int ret;

@@ -2818,7 +2818,7 @@ static int ath10k_station_assoc(struct ath10k *ar,
struct ieee80211_sta *sta,
bool reassoc)
{
- struct ath10k_vif *arvif = ath10k_vif_to_arvif(vif);
+ struct ath10k_vif *arvif = (void *)vif->drv_priv;
struct wmi_peer_assoc_complete_arg peer_arg;
int ret = 0;

@@ -2885,7 +2885,7 @@ static int ath10k_station_disassoc(struct ath10k *ar,
struct ieee80211_vif *vif,
struct ieee80211_sta *sta)
{
- struct ath10k_vif *arvif = ath10k_vif_to_arvif(vif);
+ struct ath10k_vif *arvif = (void *)vif->drv_priv;
int ret = 0;

lockdep_assert_held(&ar->conf_mutex);
@@ -3111,7 +3111,7 @@ static void ath10k_mac_tx_unlock_iter(void *data, u8 *mac,
struct ieee80211_vif *vif)
{
struct ath10k *ar = data;
- struct ath10k_vif *arvif = ath10k_vif_to_arvif(vif);
+ struct ath10k_vif *arvif = (void *)vif->drv_priv;

if (arvif->tx_paused)
return;
@@ -3198,7 +3198,7 @@ struct ath10k_mac_tx_pause {
static void ath10k_mac_handle_tx_pause_iter(void *data, u8 *mac,
struct ieee80211_vif *vif)
{
- struct ath10k_vif *arvif = ath10k_vif_to_arvif(vif);
+ struct ath10k_vif *arvif = (void *)vif->drv_priv;
struct ath10k_mac_tx_pause *arg = data;

if (arvif->vdev_id != arg->vdev_id)
@@ -3294,7 +3294,7 @@ static bool ath10k_tx_h_use_hwcrypto(struct ieee80211_vif *vif,
return false;

if (vif)
- return !ath10k_vif_to_arvif(vif)->nohwcrypt;
+ return !((struct ath10k_vif *)vif->drv_priv)->nohwcrypt;

return true;
}
@@ -3359,7 +3359,7 @@ static void ath10k_tx_h_add_p2p_noa_ie(struct ath10k *ar,
struct sk_buff *skb)
{
struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb->data;
- struct ath10k_vif *arvif = ath10k_vif_to_arvif(vif);
+ struct ath10k_vif *arvif = (void *)vif->drv_priv;

/* This is case only for P2P_GO */
if (vif->type != NL80211_IFTYPE_AP || !vif->p2p)
@@ -4775,7 +4775,7 @@ static int ath10k_add_interface(struct ieee80211_hw *hw,
struct ieee80211_vif *vif)
{
struct ath10k *ar = hw->priv;
- struct ath10k_vif *arvif = ath10k_vif_to_arvif(vif);
+ struct ath10k_vif *arvif = (void *)vif->drv_priv;
struct ath10k_peer *peer;
enum wmi_sta_powersave_param param;
int ret = 0;
@@ -5111,7 +5111,7 @@ static void ath10k_remove_interface(struct ieee80211_hw *hw,
struct ieee80211_vif *vif)
{
struct ath10k *ar = hw->priv;
- struct ath10k_vif *arvif = ath10k_vif_to_arvif(vif);
+ struct ath10k_vif *arvif = (void *)vif->drv_priv;
struct ath10k_peer *peer;
int ret;
int i;
@@ -5242,7 +5242,7 @@ static void ath10k_bss_info_changed(struct ieee80211_hw *hw,
u32 changed)
{
struct ath10k *ar = hw->priv;
- struct ath10k_vif *arvif = ath10k_vif_to_arvif(vif);
+ struct ath10k_vif *arvif = (void *)vif->drv_priv;
int ret = 0;
u32 vdev_param, pdev_param, slottime, preamble;

@@ -5436,7 +5436,7 @@ static int ath10k_hw_scan(struct ieee80211_hw *hw,
struct ieee80211_scan_request *hw_req)
{
struct ath10k *ar = hw->priv;
- struct ath10k_vif *arvif = ath10k_vif_to_arvif(vif);
+ struct ath10k_vif *arvif = (void *)vif->drv_priv;
struct cfg80211_scan_request *req = &hw_req->req;
struct wmi_start_scan_arg arg;
int ret = 0;
@@ -5568,7 +5568,7 @@ static int ath10k_set_key(struct ieee80211_hw *hw, enum set_key_cmd cmd,
struct ieee80211_key_conf *key)
{
struct ath10k *ar = hw->priv;
- struct ath10k_vif *arvif = ath10k_vif_to_arvif(vif);
+ struct ath10k_vif *arvif = (void *)vif->drv_priv;
struct ath10k_peer *peer;
const u8 *peer_addr;
bool is_wep = key->cipher == WLAN_CIPHER_SUITE_WEP40 ||
@@ -5707,7 +5707,7 @@ static void ath10k_set_default_unicast_key(struct ieee80211_hw *hw,
int keyidx)
{
struct ath10k *ar = hw->priv;
- struct ath10k_vif *arvif = ath10k_vif_to_arvif(vif);
+ struct ath10k_vif *arvif = (void *)vif->drv_priv;
int ret;

mutex_lock(&arvif->ar->conf_mutex);
@@ -5888,7 +5888,7 @@ static int ath10k_mac_tdls_vif_stations_count(struct ieee80211_hw *hw,
static void ath10k_mac_tdls_vifs_count_iter(void *data, u8 *mac,
struct ieee80211_vif *vif)
{
- struct ath10k_vif *arvif = ath10k_vif_to_arvif(vif);
+ struct ath10k_vif *arvif = (void *)vif->drv_priv;
int *num_tdls_vifs = data;

if (vif->type != NL80211_IFTYPE_STATION)
@@ -5916,7 +5916,7 @@ static int ath10k_sta_state(struct ieee80211_hw *hw,
enum ieee80211_sta_state new_state)
{
struct ath10k *ar = hw->priv;
- struct ath10k_vif *arvif = ath10k_vif_to_arvif(vif);
+ struct ath10k_vif *arvif = (void *)vif->drv_priv;
struct ath10k_sta *arsta = (struct ath10k_sta *)sta->drv_priv;
struct ath10k_peer *peer;
int ret = 0;
@@ -6151,7 +6151,7 @@ static int ath10k_sta_state(struct ieee80211_hw *hw,
static int ath10k_conf_tx_uapsd(struct ath10k *ar, struct ieee80211_vif *vif,
u16 ac, bool enable)
{
- struct ath10k_vif *arvif = ath10k_vif_to_arvif(vif);
+ struct ath10k_vif *arvif = (void *)vif->drv_priv;
struct wmi_sta_uapsd_auto_trig_arg arg = {};
u32 prio = 0, acc = 0;
u32 value = 0;
@@ -6259,7 +6259,7 @@ static int ath10k_conf_tx(struct ieee80211_hw *hw,
const struct ieee80211_tx_queue_params *params)
{
struct ath10k *ar = hw->priv;
- struct ath10k_vif *arvif = ath10k_vif_to_arvif(vif);
+ struct ath10k_vif *arvif = (void *)vif->drv_priv;
struct wmi_wmm_params_arg *p = NULL;
int ret;

@@ -6333,7 +6333,7 @@ static int ath10k_remain_on_channel(struct ieee80211_hw *hw,
enum ieee80211_roc_type type)
{
struct ath10k *ar = hw->priv;
- struct ath10k_vif *arvif = ath10k_vif_to_arvif(vif);
+ struct ath10k_vif *arvif = (void *)vif->drv_priv;
struct wmi_start_scan_arg arg;
int ret = 0;
u32 scan_time_msec;
@@ -6833,7 +6833,7 @@ static int ath10k_mac_op_set_bitrate_mask(struct ieee80211_hw *hw,
struct ieee80211_vif *vif,
const struct cfg80211_bitrate_mask *mask)
{
- struct ath10k_vif *arvif = ath10k_vif_to_arvif(vif);
+ struct ath10k_vif *arvif = (void *)vif->drv_priv;
struct cfg80211_chan_def def;
struct ath10k *ar = arvif->ar;
enum nl80211_band band;
@@ -6981,7 +6981,7 @@ static void ath10k_offset_tsf(struct ieee80211_hw *hw,
struct ieee80211_vif *vif, s64 tsf_offset)
{
struct ath10k *ar = hw->priv;
- struct ath10k_vif *arvif = ath10k_vif_to_arvif(vif);
+ struct ath10k_vif *arvif = (void *)vif->drv_priv;
u32 offset, vdev_param;
int ret;

@@ -7006,7 +7006,7 @@ static int ath10k_ampdu_action(struct ieee80211_hw *hw,
struct ieee80211_ampdu_params *params)
{
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 *sta = params->sta;
enum ieee80211_ampdu_mlme_action action = params->action;
u16 tid = params->tid;
@@ -7104,7 +7104,7 @@ static int ath10k_ampdu_action(struct ieee80211_hw *hw,
ath10k_monitor_stop(ar);

for (i = 0; i < n_vifs; i++) {
- arvif = ath10k_vif_to_arvif(vifs[i].vif);
+ arvif = (void *)vifs[i].vif->drv_priv;

ath10k_dbg(ar, ATH10K_DBG_MAC,
"mac chanctx switch vdev_id %i freq %hu->%hu width %d->%d\n",
@@ -7137,7 +7137,7 @@ static int ath10k_ampdu_action(struct ieee80211_hw *hw,
spin_unlock_bh(&ar->data_lock);

for (i = 0; i < n_vifs; i++) {
- arvif = ath10k_vif_to_arvif(vifs[i].vif);
+ arvif = (void *)vifs[i].vif->drv_priv;

if (WARN_ON(!arvif->is_started))
continue;
@@ -7771,7 +7771,7 @@ static void ath10k_get_arvif_iter(void *data, u8 *mac,
struct ieee80211_vif *vif)
{
struct ath10k_vif_iter *arvif_iter = data;
- struct ath10k_vif *arvif = ath10k_vif_to_arvif(vif);
+ struct ath10k_vif *arvif = (void *)vif->drv_priv;

if (arvif->vdev_id == arvif_iter->vdev_id)
arvif_iter->arvif = arvif;
diff --git a/drivers/net/wireless/ath/ath10k/mac.h b/drivers/net/wireless/ath/ath10k/mac.h
index 1bd29ec..553747b 100644
--- a/drivers/net/wireless/ath/ath10k/mac.h
+++ b/drivers/net/wireless/ath/ath10k/mac.h
@@ -83,17 +83,12 @@ struct ieee80211_txq *ath10k_mac_txq_lookup(struct ath10k *ar,
u8 tid);
int ath10k_mac_ext_resource_config(struct ath10k *ar, u32 val);

-static inline struct ath10k_vif *ath10k_vif_to_arvif(struct ieee80211_vif *vif)
-{
- return (struct ath10k_vif *)vif->drv_priv;
-}
-
static inline void ath10k_tx_h_seq_no(struct ieee80211_vif *vif,
struct sk_buff *skb)
{
struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb->data;
- struct ath10k_vif *arvif = ath10k_vif_to_arvif(vif);
+ struct ath10k_vif *arvif = (void *)vif->drv_priv;

if (info->flags & IEEE80211_TX_CTL_ASSIGN_SEQ) {
if (arvif->tx_seq_no == 0)
diff --git a/drivers/net/wireless/ath/ath10k/p2p.c b/drivers/net/wireless/ath/ath10k/p2p.c
index c0b6ffa..7e621ee 100644
--- a/drivers/net/wireless/ath/ath10k/p2p.c
+++ b/drivers/net/wireless/ath/ath10k/p2p.c
@@ -132,7 +132,7 @@ struct ath10k_p2p_noa_arg {
static void ath10k_p2p_noa_update_vdev_iter(void *data, u8 *mac,
struct ieee80211_vif *vif)
{
- struct ath10k_vif *arvif = ath10k_vif_to_arvif(vif);
+ struct ath10k_vif *arvif = (void *)vif->drv_priv;
struct ath10k_p2p_noa_arg *arg = data;

if (arvif->vdev_id != arg->vdev_id)
diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
index 50d6ee6..f9fd9f0 100644
--- a/drivers/net/wireless/ath/ath10k/wmi.c
+++ b/drivers/net/wireless/ath/ath10k/wmi.c
@@ -1772,7 +1772,7 @@ static void ath10k_wmi_tx_beacon_nowait(struct ath10k_vif *arvif)
static void ath10k_wmi_tx_beacons_iter(void *data, u8 *mac,
struct ieee80211_vif *vif)
{
- struct ath10k_vif *arvif = ath10k_vif_to_arvif(vif);
+ struct ath10k_vif *arvif = (void *)vif->drv_priv;

ath10k_wmi_tx_beacon_nowait(arvif);
}
--
1.9.1


2017-02-10 07:14:47

by Adrian Chadd

[permalink] [raw]
Subject: Re: [PATCH 1/3] ath10k: remove ath10k_vif_to_arvif()

On 9 February 2017 at 23:03, Valo, Kalle <[email protected]> wrote:
> Ben Greear <[email protected]> writes:
>
>> On 02/07/2017 01:14 AM, Valo, Kalle wrote:
>>> Adrian Chadd <[email protected]> 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

2017-02-10 07:04:00

by Kalle Valo

[permalink] [raw]
Subject: Re: [PATCH 1/3] ath10k: remove ath10k_vif_to_arvif()

Ben Greear <[email protected]> writes:

> On 02/07/2017 01:14 AM, Valo, Kalle wrote:
>> Adrian Chadd <[email protected]> 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?

--=20
Kalle Valo=

2017-02-02 19:40:05

by Sebastian Gottschall

[permalink] [raw]
Subject: Re: [PATCH 2/3] ath10k: use size_t for len variables

consider that you changed the length of this datatype on 64 bit systems.
unsigned int is always 32 bit, but size_t may be 64 bit too


Am 02.02.2017 um 11:19 schrieb Amadeusz Sławiński:
> cleanup to consolidate type used for len variables
>
> Signed-off-by: Amadeusz Sławiński <[email protected]>
> ---
> drivers/net/wireless/ath/ath10k/core.c | 2 +-
> drivers/net/wireless/ath/ath10k/debug.c | 49 +++++++++++++++---------------
> drivers/net/wireless/ath/ath10k/spectral.c | 7 +++--
> 3 files changed, 29 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c
> index 749e381..2aa6bc7 100644
> --- a/drivers/net/wireless/ath/ath10k/core.c
> +++ b/drivers/net/wireless/ath/ath10k/core.c
> @@ -349,7 +349,7 @@ void ath10k_core_get_fw_features_str(struct ath10k *ar,
> char *buf,
> size_t buf_len)
> {
> - unsigned int len = 0;
> + size_t len = 0;
> int i;
>
> for (i = 0; i < ATH10K_FW_FEATURE_COUNT; i++) {
> diff --git a/drivers/net/wireless/ath/ath10k/debug.c b/drivers/net/wireless/ath/ath10k/debug.c
> index 82a4c67..f649236 100644
> --- a/drivers/net/wireless/ath/ath10k/debug.c
> +++ b/drivers/net/wireless/ath/ath10k/debug.c
> @@ -235,7 +235,7 @@ static ssize_t ath10k_read_wmi_services(struct file *file,
> {
> struct ath10k *ar = file->private_data;
> char *buf;
> - unsigned int len = 0, buf_len = 4096;
> + size_t len = 0, buf_len = 4096;
> const char *name;
> ssize_t ret_cnt;
> bool enabled;
> @@ -524,7 +524,7 @@ static ssize_t ath10k_fw_stats_read(struct file *file, char __user *user_buf,
> size_t count, loff_t *ppos)
> {
> const char *buf = file->private_data;
> - unsigned int len = strlen(buf);
> + size_t len = strlen(buf);
>
> return simple_read_from_buffer(user_buf, count, ppos, buf, len);
> }
> @@ -542,17 +542,16 @@ static ssize_t ath10k_debug_fw_reset_stats_read(struct file *file,
> size_t count, loff_t *ppos)
> {
> struct ath10k *ar = file->private_data;
> - int ret, len, buf_len;
> + int ret;
> + size_t len = 0, buf_len = 500;
> char *buf;
>
> - buf_len = 500;
> buf = kmalloc(buf_len, GFP_KERNEL);
> if (!buf)
> return -ENOMEM;
>
> spin_lock_bh(&ar->data_lock);
>
> - len = 0;
> len += scnprintf(buf + len, buf_len - len,
> "fw_crash_counter\t\t%d\n", ar->stats.fw_crash_counter);
> len += scnprintf(buf + len, buf_len - len,
> @@ -691,7 +690,7 @@ static ssize_t ath10k_read_chip_id(struct file *file, char __user *user_buf,
> size_t count, loff_t *ppos)
> {
> struct ath10k *ar = file->private_data;
> - unsigned int len;
> + size_t len;
> char buf[50];
>
> len = scnprintf(buf, sizeof(buf), "0x%08x\n", ar->chip_id);
> @@ -726,8 +725,8 @@ static struct ath10k_dump_file_data *ath10k_build_dump_file(struct ath10k *ar)
> struct ath10k_fw_crash_data *crash_data = ar->debug.fw_crash_data;
> struct ath10k_dump_file_data *dump_data;
> struct ath10k_tlv_dump_data *dump_tlv;
> - int hdr_len = sizeof(*dump_data);
> - unsigned int len, sofar = 0;
> + size_t hdr_len = sizeof(*dump_data);
> + size_t len, sofar = 0;
> unsigned char *buf;
>
> len = hdr_len;
> @@ -844,7 +843,7 @@ static ssize_t ath10k_reg_addr_read(struct file *file,
> {
> struct ath10k *ar = file->private_data;
> u8 buf[32];
> - unsigned int len = 0;
> + size_t len = 0;
> u32 reg_addr;
>
> mutex_lock(&ar->conf_mutex);
> @@ -892,7 +891,7 @@ static ssize_t ath10k_reg_value_read(struct file *file,
> {
> struct ath10k *ar = file->private_data;
> u8 buf[48];
> - unsigned int len;
> + size_t len;
> u32 reg_addr, reg_val;
> int ret;
>
> @@ -1115,7 +1114,7 @@ static ssize_t ath10k_read_htt_stats_mask(struct file *file,
> {
> struct ath10k *ar = file->private_data;
> char buf[32];
> - unsigned int len;
> + size_t len;
>
> len = scnprintf(buf, sizeof(buf), "%lu\n", ar->debug.htt_stats_mask);
>
> @@ -1169,7 +1168,7 @@ static ssize_t ath10k_read_htt_max_amsdu_ampdu(struct file *file,
> struct ath10k *ar = file->private_data;
> char buf[64];
> u8 amsdu, ampdu;
> - unsigned int len;
> + size_t len;
>
> mutex_lock(&ar->conf_mutex);
>
> @@ -1229,7 +1228,7 @@ static ssize_t ath10k_read_fw_dbglog(struct file *file,
> size_t count, loff_t *ppos)
> {
> struct ath10k *ar = file->private_data;
> - unsigned int len;
> + size_t len;
> char buf[96];
>
> len = scnprintf(buf, sizeof(buf), "0x%16llx %u\n",
> @@ -1555,11 +1554,10 @@ static ssize_t ath10k_read_ani_enable(struct file *file, char __user *user_buf,
> size_t count, loff_t *ppos)
> {
> struct ath10k *ar = file->private_data;
> - int len = 0;
> + size_t len;
> char buf[32];
>
> - len = scnprintf(buf, sizeof(buf) - len, "%d\n",
> - ar->ani_enabled);
> + len = scnprintf(buf, sizeof(buf), "%d\n", ar->ani_enabled);
>
> return simple_read_from_buffer(user_buf, count, ppos, buf, len);
> }
> @@ -1584,11 +1582,10 @@ static ssize_t ath10k_read_nf_cal_period(struct file *file,
> size_t count, loff_t *ppos)
> {
> struct ath10k *ar = file->private_data;
> - unsigned int len;
> + size_t len;
> char buf[32];
>
> - len = scnprintf(buf, sizeof(buf), "%d\n",
> - ar->debug.nf_cal_period);
> + len = scnprintf(buf, sizeof(buf), "%d\n", ar->debug.nf_cal_period);
>
> return simple_read_from_buffer(user_buf, count, ppos, buf, len);
> }
> @@ -1684,9 +1681,10 @@ void ath10k_debug_tpc_stats_process(struct ath10k *ar,
> }
>
> static void ath10k_tpc_stats_print(struct ath10k_tpc_stats *tpc_stats,
> - unsigned int j, char *buf, unsigned int *len)
> + unsigned int j, char *buf, size_t *len)
> {
> - unsigned int i, buf_len;
> + int i;
> + size_t buf_len;
> static const char table_str[][5] = { "CDD",
> "STBC",
> "TXBF" };
> @@ -1726,7 +1724,8 @@ static void ath10k_tpc_stats_fill(struct ath10k *ar,
> struct ath10k_tpc_stats *tpc_stats,
> char *buf)
> {
> - unsigned int len, j, buf_len;
> + int j;
> + size_t len, buf_len;
>
> len = 0;
> buf_len = ATH10K_TPC_CONFIG_BUF_SIZE;
> @@ -1860,7 +1859,7 @@ static ssize_t ath10k_tpc_stats_read(struct file *file, char __user *user_buf,
> size_t count, loff_t *ppos)
> {
> const char *buf = file->private_data;
> - unsigned int len = strlen(buf);
> + size_t len = strlen(buf);
>
> return simple_read_from_buffer(user_buf, count, ppos, buf, len);
> }
> @@ -2284,7 +2283,7 @@ static ssize_t ath10k_debug_fw_checksums_read(struct file *file,
> size_t count, loff_t *ppos)
> {
> struct ath10k *ar = file->private_data;
> - unsigned int len = 0, buf_len = 4096;
> + size_t len = 0, buf_len = 4096;
> ssize_t ret_cnt;
> char *buf;
>
> @@ -2500,7 +2499,7 @@ void ath10k_dbg_dump(struct ath10k *ar,
> const void *buf, size_t len)
> {
> char linebuf[256];
> - unsigned int linebuflen;
> + size_t linebuflen;
> const void *ptr;
>
> if (ath10k_debug_mask & mask) {
> diff --git a/drivers/net/wireless/ath/ath10k/spectral.c b/drivers/net/wireless/ath/ath10k/spectral.c
> index 2ffc1fe..c061d69 100644
> --- a/drivers/net/wireless/ath/ath10k/spectral.c
> +++ b/drivers/net/wireless/ath/ath10k/spectral.c
> @@ -278,7 +278,7 @@ static ssize_t read_file_spec_scan_ctl(struct file *file, char __user *user_buf,
> {
> struct ath10k *ar = file->private_data;
> char *mode = "";
> - unsigned int len;
> + size_t len;
> enum ath10k_spectral_mode spectral_mode;
>
> mutex_lock(&ar->conf_mutex);
> @@ -370,7 +370,7 @@ static ssize_t read_file_spectral_count(struct file *file,
> {
> struct ath10k *ar = file->private_data;
> char buf[32];
> - unsigned int len;
> + size_t len;
> u8 spectral_count;
>
> mutex_lock(&ar->conf_mutex);
> @@ -422,7 +422,8 @@ static ssize_t read_file_spectral_bins(struct file *file,
> {
> struct ath10k *ar = file->private_data;
> char buf[32];
> - unsigned int len, bins, fft_size, bin_scale;
> + unsigned int bins, fft_size, bin_scale;
> + size_t len;
>
> mutex_lock(&ar->conf_mutex);
>


--
Mit freundlichen Grüssen / Regards

Sebastian Gottschall / CTO

NewMedia-NET GmbH - DD-WRT
Firmensitz: Berliner Ring 101, 64625 Bensheim
Registergericht: Amtsgericht Darmstadt, HRB 25473
Geschäftsführer: Peter Steinhäuser, Christian Scheele
http://www.dd-wrt.com
email: [email protected]
Tel.: +496251-582650 / Fax: +496251-5826565

2017-02-10 07:37:12

by Joe Perches

[permalink] [raw]
Subject: 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.

2017-02-02 16:17:03

by Adrian Chadd

[permalink] [raw]
Subject: 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

2017-02-02 10:19:53

by Amadeusz Sławiński

[permalink] [raw]
Subject: [PATCH 2/3] ath10k: use size_t for len variables

cleanup to consolidate type used for len variables

Signed-off-by: Amadeusz Sławiński <[email protected]>
---
drivers/net/wireless/ath/ath10k/core.c | 2 +-
drivers/net/wireless/ath/ath10k/debug.c | 49 +++++++++++++++---------------
drivers/net/wireless/ath/ath10k/spectral.c | 7 +++--
3 files changed, 29 insertions(+), 29 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c
index 749e381..2aa6bc7 100644
--- a/drivers/net/wireless/ath/ath10k/core.c
+++ b/drivers/net/wireless/ath/ath10k/core.c
@@ -349,7 +349,7 @@ void ath10k_core_get_fw_features_str(struct ath10k *ar,
char *buf,
size_t buf_len)
{
- unsigned int len = 0;
+ size_t len = 0;
int i;

for (i = 0; i < ATH10K_FW_FEATURE_COUNT; i++) {
diff --git a/drivers/net/wireless/ath/ath10k/debug.c b/drivers/net/wireless/ath/ath10k/debug.c
index 82a4c67..f649236 100644
--- a/drivers/net/wireless/ath/ath10k/debug.c
+++ b/drivers/net/wireless/ath/ath10k/debug.c
@@ -235,7 +235,7 @@ static ssize_t ath10k_read_wmi_services(struct file *file,
{
struct ath10k *ar = file->private_data;
char *buf;
- unsigned int len = 0, buf_len = 4096;
+ size_t len = 0, buf_len = 4096;
const char *name;
ssize_t ret_cnt;
bool enabled;
@@ -524,7 +524,7 @@ static ssize_t ath10k_fw_stats_read(struct file *file, char __user *user_buf,
size_t count, loff_t *ppos)
{
const char *buf = file->private_data;
- unsigned int len = strlen(buf);
+ size_t len = strlen(buf);

return simple_read_from_buffer(user_buf, count, ppos, buf, len);
}
@@ -542,17 +542,16 @@ static ssize_t ath10k_debug_fw_reset_stats_read(struct file *file,
size_t count, loff_t *ppos)
{
struct ath10k *ar = file->private_data;
- int ret, len, buf_len;
+ int ret;
+ size_t len = 0, buf_len = 500;
char *buf;

- buf_len = 500;
buf = kmalloc(buf_len, GFP_KERNEL);
if (!buf)
return -ENOMEM;

spin_lock_bh(&ar->data_lock);

- len = 0;
len += scnprintf(buf + len, buf_len - len,
"fw_crash_counter\t\t%d\n", ar->stats.fw_crash_counter);
len += scnprintf(buf + len, buf_len - len,
@@ -691,7 +690,7 @@ static ssize_t ath10k_read_chip_id(struct file *file, char __user *user_buf,
size_t count, loff_t *ppos)
{
struct ath10k *ar = file->private_data;
- unsigned int len;
+ size_t len;
char buf[50];

len = scnprintf(buf, sizeof(buf), "0x%08x\n", ar->chip_id);
@@ -726,8 +725,8 @@ static struct ath10k_dump_file_data *ath10k_build_dump_file(struct ath10k *ar)
struct ath10k_fw_crash_data *crash_data = ar->debug.fw_crash_data;
struct ath10k_dump_file_data *dump_data;
struct ath10k_tlv_dump_data *dump_tlv;
- int hdr_len = sizeof(*dump_data);
- unsigned int len, sofar = 0;
+ size_t hdr_len = sizeof(*dump_data);
+ size_t len, sofar = 0;
unsigned char *buf;

len = hdr_len;
@@ -844,7 +843,7 @@ static ssize_t ath10k_reg_addr_read(struct file *file,
{
struct ath10k *ar = file->private_data;
u8 buf[32];
- unsigned int len = 0;
+ size_t len = 0;
u32 reg_addr;

mutex_lock(&ar->conf_mutex);
@@ -892,7 +891,7 @@ static ssize_t ath10k_reg_value_read(struct file *file,
{
struct ath10k *ar = file->private_data;
u8 buf[48];
- unsigned int len;
+ size_t len;
u32 reg_addr, reg_val;
int ret;

@@ -1115,7 +1114,7 @@ static ssize_t ath10k_read_htt_stats_mask(struct file *file,
{
struct ath10k *ar = file->private_data;
char buf[32];
- unsigned int len;
+ size_t len;

len = scnprintf(buf, sizeof(buf), "%lu\n", ar->debug.htt_stats_mask);

@@ -1169,7 +1168,7 @@ static ssize_t ath10k_read_htt_max_amsdu_ampdu(struct file *file,
struct ath10k *ar = file->private_data;
char buf[64];
u8 amsdu, ampdu;
- unsigned int len;
+ size_t len;

mutex_lock(&ar->conf_mutex);

@@ -1229,7 +1228,7 @@ static ssize_t ath10k_read_fw_dbglog(struct file *file,
size_t count, loff_t *ppos)
{
struct ath10k *ar = file->private_data;
- unsigned int len;
+ size_t len;
char buf[96];

len = scnprintf(buf, sizeof(buf), "0x%16llx %u\n",
@@ -1555,11 +1554,10 @@ static ssize_t ath10k_read_ani_enable(struct file *file, char __user *user_buf,
size_t count, loff_t *ppos)
{
struct ath10k *ar = file->private_data;
- int len = 0;
+ size_t len;
char buf[32];

- len = scnprintf(buf, sizeof(buf) - len, "%d\n",
- ar->ani_enabled);
+ len = scnprintf(buf, sizeof(buf), "%d\n", ar->ani_enabled);

return simple_read_from_buffer(user_buf, count, ppos, buf, len);
}
@@ -1584,11 +1582,10 @@ static ssize_t ath10k_read_nf_cal_period(struct file *file,
size_t count, loff_t *ppos)
{
struct ath10k *ar = file->private_data;
- unsigned int len;
+ size_t len;
char buf[32];

- len = scnprintf(buf, sizeof(buf), "%d\n",
- ar->debug.nf_cal_period);
+ len = scnprintf(buf, sizeof(buf), "%d\n", ar->debug.nf_cal_period);

return simple_read_from_buffer(user_buf, count, ppos, buf, len);
}
@@ -1684,9 +1681,10 @@ void ath10k_debug_tpc_stats_process(struct ath10k *ar,
}

static void ath10k_tpc_stats_print(struct ath10k_tpc_stats *tpc_stats,
- unsigned int j, char *buf, unsigned int *len)
+ unsigned int j, char *buf, size_t *len)
{
- unsigned int i, buf_len;
+ int i;
+ size_t buf_len;
static const char table_str[][5] = { "CDD",
"STBC",
"TXBF" };
@@ -1726,7 +1724,8 @@ static void ath10k_tpc_stats_fill(struct ath10k *ar,
struct ath10k_tpc_stats *tpc_stats,
char *buf)
{
- unsigned int len, j, buf_len;
+ int j;
+ size_t len, buf_len;

len = 0;
buf_len = ATH10K_TPC_CONFIG_BUF_SIZE;
@@ -1860,7 +1859,7 @@ static ssize_t ath10k_tpc_stats_read(struct file *file, char __user *user_buf,
size_t count, loff_t *ppos)
{
const char *buf = file->private_data;
- unsigned int len = strlen(buf);
+ size_t len = strlen(buf);

return simple_read_from_buffer(user_buf, count, ppos, buf, len);
}
@@ -2284,7 +2283,7 @@ static ssize_t ath10k_debug_fw_checksums_read(struct file *file,
size_t count, loff_t *ppos)
{
struct ath10k *ar = file->private_data;
- unsigned int len = 0, buf_len = 4096;
+ size_t len = 0, buf_len = 4096;
ssize_t ret_cnt;
char *buf;

@@ -2500,7 +2499,7 @@ void ath10k_dbg_dump(struct ath10k *ar,
const void *buf, size_t len)
{
char linebuf[256];
- unsigned int linebuflen;
+ size_t linebuflen;
const void *ptr;

if (ath10k_debug_mask & mask) {
diff --git a/drivers/net/wireless/ath/ath10k/spectral.c b/drivers/net/wireless/ath/ath10k/spectral.c
index 2ffc1fe..c061d69 100644
--- a/drivers/net/wireless/ath/ath10k/spectral.c
+++ b/drivers/net/wireless/ath/ath10k/spectral.c
@@ -278,7 +278,7 @@ static ssize_t read_file_spec_scan_ctl(struct file *file, char __user *user_buf,
{
struct ath10k *ar = file->private_data;
char *mode = "";
- unsigned int len;
+ size_t len;
enum ath10k_spectral_mode spectral_mode;

mutex_lock(&ar->conf_mutex);
@@ -370,7 +370,7 @@ static ssize_t read_file_spectral_count(struct file *file,
{
struct ath10k *ar = file->private_data;
char buf[32];
- unsigned int len;
+ size_t len;
u8 spectral_count;

mutex_lock(&ar->conf_mutex);
@@ -422,7 +422,8 @@ static ssize_t read_file_spectral_bins(struct file *file,
{
struct ath10k *ar = file->private_data;
char buf[32];
- unsigned int len, bins, fft_size, bin_scale;
+ unsigned int bins, fft_size, bin_scale;
+ size_t len;

mutex_lock(&ar->conf_mutex);

--
1.9.1

2017-02-02 10:19:54

by Amadeusz Sławiński

[permalink] [raw]
Subject: [PATCH 3/3] ath10k: fix comment

I wanted to take a look and it's apparently in other header

Signed-off-by: Amadeusz Sławiński <[email protected]>
---
drivers/net/wireless/ath/ath10k/wmi.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/ath10k/wmi.h b/drivers/net/wireless/ath/ath10k/wmi.h
index 5d3dff9..bf8dcbe 100644
--- a/drivers/net/wireless/ath/ath10k/wmi.h
+++ b/drivers/net/wireless/ath/ath10k/wmi.h
@@ -75,7 +75,7 @@ struct wmi_cmd_hdr {

/*
* There is no signed version of __le32, so for a temporary solution come
- * up with our own version. The idea is from fs/ntfs/types.h.
+ * up with our own version. The idea is from fs/ntfs/endian.h.
*
* Use a_ prefix so that it doesn't conflict if we get proper support to
* linux/types.h.
--
1.9.1

2017-02-07 09:14:45

by Kalle Valo

[permalink] [raw]
Subject: Re: [PATCH 1/3] ath10k: remove ath10k_vif_to_arvif()

Adrian Chadd <[email protected]> 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.

--=20
Kalle Valo=

2017-02-10 17:37:42

by Ben Greear

[permalink] [raw]
Subject: Re: [PATCH 1/3] ath10k: remove ath10k_vif_to_arvif()

On 02/09/2017 11:03 PM, Valo, Kalle wrote:
> Ben Greear <[email protected]> writes:
>
>> On 02/07/2017 01:14 AM, Valo, Kalle wrote:
>>> Adrian Chadd <[email protected]> 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 <[email protected]>
Candela Technologies Inc http://www.candelatech.com

2017-02-07 14:46:50

by Ben Greear

[permalink] [raw]
Subject: Re: [PATCH 1/3] ath10k: remove ath10k_vif_to_arvif()



On 02/07/2017 01:14 AM, Valo, Kalle wrote:
> Adrian Chadd <[email protected]> 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 <[email protected]>
Candela Technologies Inc http://www.candelatech.com

2017-02-10 15:33:58

by Adrian Chadd

[permalink] [raw]
Subject: Re: [PATCH 1/3] ath10k: remove ath10k_vif_to_arvif()

On 9 February 2017 at 23:37, Joe Perches <[email protected]> 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

2017-02-14 17:40:13

by Kalle Valo

[permalink] [raw]
Subject: Re: [1/3] ath10k: remove ath10k_vif_to_arvif()

Amadeusz Sławiński wrote:
> it adds unnecessary level of indirection, while we just access structure
> field
>
> Signed-off-by: Amadeusz Sławiński <[email protected]>

3 patches applied to ath-next branch of ath.git, thanks.

56ac13bfc703 ath10k: remove ath10k_vif_to_arvif()
182f1e5a626e ath10k: use size_t for len variables
6d2191135637 ath10k: fix comment

--
https://patchwork.kernel.org/patch/9551559/

Documentation about submitting wireless patches and checking status
from patchwork:

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches