Currently, upon virtual interface creation, wfx_add_interface() stores
a reference to the corresponding struct ieee80211_vif in private data,
for later usage. This is not needed when using the container_of
construct. This construct already has all the info it needs to retrieve
the reference to the corresponding struct from the offset that is
already available, inherent in container_of(), between its type and
member inputs (struct ieee80211_vif and drv_priv, respectively).
Remove vif (which was previously storing the reference to the struct
ieee80211_vif) from the struct wfx_vif, define a function
wvif_to_vif(wvif) for container_of(), and replace all wvif->vif with
the newly defined container_of construct.
Signed-off-by: Jaehee Park <[email protected]>
---
Changes from staging to wireless-next tree
- changed macro into function and named it back to wvif_to_vif
- fit all lines in patch to 80 columns
- decared a reference to vif at the beginning of the functions
NOTE: J?r?me is going to be testing this patch on his hardware
drivers/net/wireless/silabs/wfx/wfx.h | 6 +-
drivers/net/wireless/silabs/wfx/data_rx.c | 5 +-
drivers/net/wireless/silabs/wfx/data_tx.c | 3 +-
drivers/net/wireless/silabs/wfx/key.c | 4 +-
drivers/net/wireless/silabs/wfx/queue.c | 3 +-
drivers/net/wireless/silabs/wfx/scan.c | 9 ++-
drivers/net/wireless/silabs/wfx/sta.c | 69 ++++++++++++++---------
7 files changed, 63 insertions(+), 36 deletions(-)
diff --git a/drivers/net/wireless/silabs/wfx/wfx.h b/drivers/net/wireless/silabs/wfx/wfx.h
index 6594cc647c2f..718693a4273d 100644
--- a/drivers/net/wireless/silabs/wfx/wfx.h
+++ b/drivers/net/wireless/silabs/wfx/wfx.h
@@ -61,7 +61,6 @@ struct wfx_dev {
struct wfx_vif {
struct wfx_dev *wdev;
- struct ieee80211_vif *vif;
struct ieee80211_channel *channel;
int id;
@@ -91,6 +90,11 @@ struct wfx_vif {
struct completion set_pm_mode_complete;
};
+static inline struct ieee80211_vif *wvif_to_vif(struct wfx_vif *wvif)
+{
+ return container_of((void *)wvif, struct ieee80211_vif, drv_priv);
+}
+
static inline struct wfx_vif *wdev_to_wvif(struct wfx_dev *wdev, int vif_id)
{
if (vif_id >= ARRAY_SIZE(wdev->vif)) {
diff --git a/drivers/net/wireless/silabs/wfx/data_rx.c b/drivers/net/wireless/silabs/wfx/data_rx.c
index a4b5ffe158e4..342b9cd0e74c 100644
--- a/drivers/net/wireless/silabs/wfx/data_rx.c
+++ b/drivers/net/wireless/silabs/wfx/data_rx.c
@@ -16,6 +16,7 @@
static void wfx_rx_handle_ba(struct wfx_vif *wvif, struct ieee80211_mgmt *mgmt)
{
int params, tid;
+ struct ieee80211_vif *vif = wvif_to_vif(wvif);
if (wfx_api_older_than(wvif->wdev, 3, 6))
return;
@@ -24,12 +25,12 @@ static void wfx_rx_handle_ba(struct wfx_vif *wvif, struct ieee80211_mgmt *mgmt)
case WLAN_ACTION_ADDBA_REQ:
params = le16_to_cpu(mgmt->u.action.u.addba_req.capab);
tid = (params & IEEE80211_ADDBA_PARAM_TID_MASK) >> 2;
- ieee80211_start_rx_ba_session_offl(wvif->vif, mgmt->sa, tid);
+ ieee80211_start_rx_ba_session_offl(vif, mgmt->sa, tid);
break;
case WLAN_ACTION_DELBA:
params = le16_to_cpu(mgmt->u.action.u.delba.params);
tid = (params & IEEE80211_DELBA_PARAM_TID_MASK) >> 12;
- ieee80211_stop_rx_ba_session_offl(wvif->vif, mgmt->sa, tid);
+ ieee80211_stop_rx_ba_session_offl(vif, mgmt->sa, tid);
break;
}
}
diff --git a/drivers/net/wireless/silabs/wfx/data_tx.c b/drivers/net/wireless/silabs/wfx/data_tx.c
index e07381b2ff4d..db99142e8e8f 100644
--- a/drivers/net/wireless/silabs/wfx/data_tx.c
+++ b/drivers/net/wireless/silabs/wfx/data_tx.c
@@ -213,10 +213,11 @@ static u8 wfx_tx_get_link_id(struct wfx_vif *wvif, struct ieee80211_sta *sta,
{
struct wfx_sta_priv *sta_priv = sta ? (struct wfx_sta_priv *)&sta->drv_priv : NULL;
const u8 *da = ieee80211_get_DA(hdr);
+ struct ieee80211_vif *vif = wvif_to_vif(wvif);
if (sta_priv && sta_priv->link_id)
return sta_priv->link_id;
- if (wvif->vif->type != NL80211_IFTYPE_AP)
+ if (vif->type != NL80211_IFTYPE_AP)
return 0;
if (is_multicast_ether_addr(da))
return 0;
diff --git a/drivers/net/wireless/silabs/wfx/key.c b/drivers/net/wireless/silabs/wfx/key.c
index 8f23e8d42bd4..196d64ef68f3 100644
--- a/drivers/net/wireless/silabs/wfx/key.c
+++ b/drivers/net/wireless/silabs/wfx/key.c
@@ -156,6 +156,7 @@ static int wfx_add_key(struct wfx_vif *wvif, struct ieee80211_sta *sta,
struct wfx_dev *wdev = wvif->wdev;
int idx = wfx_alloc_key(wvif->wdev);
bool pairwise = key->flags & IEEE80211_KEY_FLAG_PAIRWISE;
+ struct ieee80211_vif *vif = wvif_to_vif(wvif);
WARN(key->flags & IEEE80211_KEY_FLAG_PAIRWISE && !sta, "inconsistent data");
ieee80211_get_key_rx_seq(key, 0, &seq);
@@ -174,7 +175,7 @@ static int wfx_add_key(struct wfx_vif *wvif, struct ieee80211_sta *sta,
k.type = fill_tkip_pair(&k.key.tkip_pairwise_key, key, sta->addr);
else
k.type = fill_tkip_group(&k.key.tkip_group_key, key, &seq,
- wvif->vif->type);
+ vif->type);
} else if (key->cipher == WLAN_CIPHER_SUITE_CCMP) {
if (pairwise)
k.type = fill_ccmp_pair(&k.key.aes_pairwise_key, key, sta->addr);
@@ -224,4 +225,3 @@ int wfx_set_key(struct ieee80211_hw *hw, enum set_key_cmd cmd, struct ieee80211_
mutex_unlock(&wvif->wdev->conf_mutex);
return ret;
}
-
diff --git a/drivers/net/wireless/silabs/wfx/queue.c b/drivers/net/wireless/silabs/wfx/queue.c
index 729825230db2..fa1837a00a74 100644
--- a/drivers/net/wireless/silabs/wfx/queue.c
+++ b/drivers/net/wireless/silabs/wfx/queue.c
@@ -206,8 +206,9 @@ unsigned int wfx_pending_get_pkt_us_delay(struct wfx_dev *wdev, struct sk_buff *
bool wfx_tx_queues_has_cab(struct wfx_vif *wvif)
{
int i;
+ struct ieee80211_vif *vif = wvif_to_vif(wvif);
- if (wvif->vif->type != NL80211_IFTYPE_AP)
+ if (vif->type != NL80211_IFTYPE_AP)
return false;
for (i = 0; i < IEEE80211_NUM_ACS; ++i)
/* Note: since only AP can have mcast frames in queue and only one vif can be AP,
diff --git a/drivers/net/wireless/silabs/wfx/scan.c b/drivers/net/wireless/silabs/wfx/scan.c
index 7f34f0d322f9..d2dd9829351a 100644
--- a/drivers/net/wireless/silabs/wfx/scan.c
+++ b/drivers/net/wireless/silabs/wfx/scan.c
@@ -24,8 +24,10 @@ static void wfx_ieee80211_scan_completed_compat(struct ieee80211_hw *hw, bool ab
static int update_probe_tmpl(struct wfx_vif *wvif, struct cfg80211_scan_request *req)
{
struct sk_buff *skb;
+ struct ieee80211_vif *vif = wvif_to_vif(wvif);
- skb = ieee80211_probereq_get(wvif->wdev->hw, wvif->vif->addr, NULL, 0, req->ie_len);
+ skb = ieee80211_probereq_get(wvif->wdev->hw, vif->addr, NULL, 0,
+ req->ie_len);
if (!skb)
return -ENOMEM;
@@ -39,6 +41,7 @@ static int send_scan_req(struct wfx_vif *wvif, struct cfg80211_scan_request *req
{
int i, ret;
struct ieee80211_channel *ch_start, *ch_cur;
+ struct ieee80211_vif *vif = wvif_to_vif(wvif);
for (i = start_idx; i < req->n_channels; i++) {
ch_start = req->channels[start_idx];
@@ -75,8 +78,8 @@ static int send_scan_req(struct wfx_vif *wvif, struct cfg80211_scan_request *req
} else {
ret = wvif->scan_nb_chan_done;
}
- if (req->channels[start_idx]->max_power != wvif->vif->bss_conf.txpower)
- wfx_hif_set_output_power(wvif, wvif->vif->bss_conf.txpower);
+ if (req->channels[start_idx]->max_power != vif->bss_conf.txpower)
+ wfx_hif_set_output_power(wvif, vif->bss_conf.txpower);
wfx_tx_unlock(wvif->wdev);
return ret;
}
diff --git a/drivers/net/wireless/silabs/wfx/sta.c b/drivers/net/wireless/silabs/wfx/sta.c
index 3297d73c327a..97fcbad23c94 100644
--- a/drivers/net/wireless/silabs/wfx/sta.c
+++ b/drivers/net/wireless/silabs/wfx/sta.c
@@ -101,6 +101,7 @@ void wfx_configure_filter(struct ieee80211_hw *hw, unsigned int changed_flags,
struct wfx_vif *wvif = NULL;
struct wfx_dev *wdev = hw->priv;
bool filter_bssid, filter_prbreq, filter_beacon;
+ struct ieee80211_vif *vif = wvif_to_vif(wvif);
/* Notes:
* - Probe responses (FIF_BCN_PRBRESP_PROMISC) are never filtered
@@ -132,7 +133,7 @@ void wfx_configure_filter(struct ieee80211_hw *hw, unsigned int changed_flags,
filter_bssid = true;
/* In AP mode, chip can reply to probe request itself */
- if (*total_flags & FIF_PROBE_REQ && wvif->vif->type == NL80211_IFTYPE_AP) {
+ if (*total_flags & FIF_PROBE_REQ && vif->type == NL80211_IFTYPE_AP) {
dev_dbg(wdev->dev, "do not forward probe request in AP mode\n");
*total_flags &= ~FIF_PROBE_REQ;
}
@@ -152,19 +153,28 @@ static int wfx_get_ps_timeout(struct wfx_vif *wvif, bool *enable_ps)
{
struct ieee80211_channel *chan0 = NULL, *chan1 = NULL;
struct ieee80211_conf *conf = &wvif->wdev->hw->conf;
+ struct ieee80211_vif *vif = wvif_to_vif(wvif);
- WARN(!wvif->vif->bss_conf.assoc && enable_ps,
+ WARN(!vif->bss_conf.assoc && enable_ps,
"enable_ps is reliable only if associated");
- if (wdev_to_wvif(wvif->wdev, 0))
- chan0 = wdev_to_wvif(wvif->wdev, 0)->vif->bss_conf.chandef.chan;
- if (wdev_to_wvif(wvif->wdev, 1))
- chan1 = wdev_to_wvif(wvif->wdev, 1)->vif->bss_conf.chandef.chan;
- if (chan0 && chan1 && wvif->vif->type != NL80211_IFTYPE_AP) {
+ if (wdev_to_wvif(wvif->wdev, 0)) {
+ struct wfx_vif *wvif_ch0 = wdev_to_wvif(wvif->wdev, 0);
+ struct ieee80211_vif *vif_ch0 = wvif_to_vif(wvif_ch0);
+
+ chan0 = vif_ch0->bss_conf.chandef.chan;
+ }
+ if (wdev_to_wvif(wvif->wdev, 1)) {
+ struct wfx_vif *wvif_ch1 = wdev_to_wvif(wvif->wdev, 1);
+ struct ieee80211_vif *vif_ch1 = wvif_to_vif(wvif_ch1);
+
+ chan1 = vif_ch1->bss_conf.chandef.chan;
+ }
+ if (chan0 && chan1 && vif->type != NL80211_IFTYPE_AP) {
if (chan0->hw_value == chan1->hw_value) {
/* It is useless to enable PS if channels are the same. */
if (enable_ps)
*enable_ps = false;
- if (wvif->vif->bss_conf.assoc && wvif->vif->bss_conf.ps)
+ if (vif->bss_conf.assoc && vif->bss_conf.ps)
dev_info(wvif->wdev->dev, "ignoring requested PS mode");
return -1;
}
@@ -177,8 +187,8 @@ static int wfx_get_ps_timeout(struct wfx_vif *wvif, bool *enable_ps)
return 30;
}
if (enable_ps)
- *enable_ps = wvif->vif->bss_conf.ps;
- if (wvif->vif->bss_conf.assoc && wvif->vif->bss_conf.ps)
+ *enable_ps = vif->bss_conf.ps;
+ if (vif->bss_conf.assoc && vif->bss_conf.ps)
return conf->dynamic_ps_timeout;
else
return -1;
@@ -188,8 +198,9 @@ int wfx_update_pm(struct wfx_vif *wvif)
{
int ps_timeout;
bool ps;
+ struct ieee80211_vif *vif = wvif_to_vif(wvif);
- if (!wvif->vif->bss_conf.assoc)
+ if (!vif->bss_conf.assoc)
return 0;
ps_timeout = wfx_get_ps_timeout(wvif, &ps);
if (!ps)
@@ -215,7 +226,8 @@ int wfx_conf_tx(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
mutex_lock(&wdev->conf_mutex);
assign_bit(queue, &wvif->uapsd_mask, params->uapsd);
wfx_hif_set_edca_queue_params(wvif, queue, params);
- if (wvif->vif->type == NL80211_IFTYPE_STATION && old_uapsd != wvif->uapsd_mask) {
+ if (vif->type == NL80211_IFTYPE_STATION &&
+ old_uapsd != wvif->uapsd_mask) {
wfx_hif_set_uapsd_info(wvif, wvif->uapsd_mask);
wfx_update_pm(wvif);
}
@@ -240,22 +252,24 @@ void wfx_event_report_rssi(struct wfx_vif *wvif, u8 raw_rcpi_rssi)
*/
int rcpi_rssi;
int cqm_evt;
+ struct ieee80211_vif *vif = wvif_to_vif(wvif);
rcpi_rssi = raw_rcpi_rssi / 2 - 110;
- if (rcpi_rssi <= wvif->vif->bss_conf.cqm_rssi_thold)
+ if (rcpi_rssi <= vif->bss_conf.cqm_rssi_thold)
cqm_evt = NL80211_CQM_RSSI_THRESHOLD_EVENT_LOW;
else
cqm_evt = NL80211_CQM_RSSI_THRESHOLD_EVENT_HIGH;
- ieee80211_cqm_rssi_notify(wvif->vif, cqm_evt, rcpi_rssi, GFP_KERNEL);
+ ieee80211_cqm_rssi_notify(vif, cqm_evt, rcpi_rssi, GFP_KERNEL);
}
static void wfx_beacon_loss_work(struct work_struct *work)
{
struct wfx_vif *wvif = container_of(to_delayed_work(work), struct wfx_vif,
beacon_loss_work);
- struct ieee80211_bss_conf *bss_conf = &wvif->vif->bss_conf;
+ struct ieee80211_vif *vif = wvif_to_vif(wvif);
+ struct ieee80211_bss_conf *bss_conf = &vif->bss_conf;
- ieee80211_beacon_loss(wvif->vif);
+ ieee80211_beacon_loss(vif);
schedule_delayed_work(to_delayed_work(work), msecs_to_jiffies(bss_conf->beacon_int));
}
@@ -322,14 +336,15 @@ int wfx_sta_remove(struct ieee80211_hw *hw, struct ieee80211_vif *vif, struct ie
static int wfx_upload_ap_templates(struct wfx_vif *wvif)
{
struct sk_buff *skb;
+ struct ieee80211_vif *vif = wvif_to_vif(wvif);
- skb = ieee80211_beacon_get(wvif->wdev->hw, wvif->vif);
+ skb = ieee80211_beacon_get(wvif->wdev->hw, vif);
if (!skb)
return -ENOMEM;
wfx_hif_set_template_frame(wvif, skb, HIF_TMPLT_BCN, API_RATE_INDEX_B_1MBPS);
dev_kfree_skb(skb);
- skb = ieee80211_proberesp_get(wvif->wdev->hw, wvif->vif);
+ skb = ieee80211_proberesp_get(wvif->wdev->hw, vif);
if (!skb)
return -ENOMEM;
wfx_hif_set_template_frame(wvif, skb, HIF_TMPLT_PRBRES, API_RATE_INDEX_B_1MBPS);
@@ -339,7 +354,8 @@ static int wfx_upload_ap_templates(struct wfx_vif *wvif)
static void wfx_set_mfp_ap(struct wfx_vif *wvif)
{
- struct sk_buff *skb = ieee80211_beacon_get(wvif->wdev->hw, wvif->vif);
+ struct ieee80211_vif *vif = wvif_to_vif(wvif);
+ struct sk_buff *skb = ieee80211_beacon_get(wvif->wdev->hw, vif);
const int ieoffset = offsetof(struct ieee80211_mgmt, u.beacon.variable);
const u16 *ptr = (u16 *)cfg80211_find_ie(WLAN_EID_RSN, skb->data + ieoffset,
skb->len - ieoffset);
@@ -389,7 +405,8 @@ void wfx_stop_ap(struct ieee80211_hw *hw, struct ieee80211_vif *vif)
static void wfx_join(struct wfx_vif *wvif)
{
int ret;
- struct ieee80211_bss_conf *conf = &wvif->vif->bss_conf;
+ struct ieee80211_vif *vif = wvif_to_vif(wvif);
+ struct ieee80211_bss_conf *conf = &vif->bss_conf;
struct cfg80211_bss *bss = NULL;
u8 ssid[IEEE80211_MAX_SSID_LEN];
const u8 *ssidie = NULL;
@@ -420,7 +437,7 @@ static void wfx_join(struct wfx_vif *wvif)
wvif->join_in_progress = true;
ret = wfx_hif_join(wvif, conf, wvif->channel, ssid, ssidlen);
if (ret) {
- ieee80211_connection_loss(wvif->vif);
+ ieee80211_connection_loss(vif);
wfx_reset(wvif);
} else {
/* Due to beacon filtering it is possible that the AP's beacon is not known for the
@@ -437,10 +454,11 @@ static void wfx_join_finalize(struct wfx_vif *wvif, struct ieee80211_bss_conf *i
struct ieee80211_sta *sta = NULL;
int ampdu_density = 0;
bool greenfield = false;
+ struct ieee80211_vif *vif = wvif_to_vif(wvif);
rcu_read_lock(); /* protect sta */
if (info->bssid && !info->ibss_joined)
- sta = ieee80211_find_sta(wvif->vif, info->bssid);
+ sta = ieee80211_find_sta(vif, info->bssid);
if (sta && sta->deflink.ht_cap.ht_supported)
ampdu_density = sta->deflink.ht_cap.ampdu_density;
if (sta && sta->deflink.ht_cap.ht_supported &&
@@ -564,8 +582,10 @@ static int wfx_update_tim(struct wfx_vif *wvif)
struct sk_buff *skb;
u16 tim_offset, tim_length;
u8 *tim_ptr;
+ struct ieee80211_vif *vif = wvif_to_vif(wvif);
- skb = ieee80211_beacon_get_tim(wvif->wdev->hw, wvif->vif, &tim_offset, &tim_length);
+ skb = ieee80211_beacon_get_tim(wvif->wdev->hw, vif, &tim_offset,
+ &tim_length);
if (!skb)
return -ENOENT;
tim_ptr = skb->data + tim_offset;
@@ -707,8 +727,6 @@ int wfx_add_interface(struct ieee80211_hw *hw, struct ieee80211_vif *vif)
return -EOPNOTSUPP;
}
- /* FIXME: prefer use of container_of() to get vif */
- wvif->vif = vif;
wvif->wdev = wdev;
wvif->link_id_map = 1; /* link-id 0 is reserved for multicast */
@@ -767,7 +785,6 @@ void wfx_remove_interface(struct ieee80211_hw *hw, struct ieee80211_vif *vif)
cancel_delayed_work_sync(&wvif->beacon_loss_work);
wdev->vif[wvif->id] = NULL;
- wvif->vif = NULL;
mutex_unlock(&wdev->conf_mutex);
--
2.25.1
On Monday 18 April 2022 05:51:10 CEST Jaehee Park wrote:
>
> Currently, upon virtual interface creation, wfx_add_interface() stores
> a reference to the corresponding struct ieee80211_vif in private data,
> for later usage. This is not needed when using the container_of
> construct. This construct already has all the info it needs to retrieve
> the reference to the corresponding struct from the offset that is
> already available, inherent in container_of(), between its type and
> member inputs (struct ieee80211_vif and drv_priv, respectively).
> Remove vif (which was previously storing the reference to the struct
> ieee80211_vif) from the struct wfx_vif, define a function
> wvif_to_vif(wvif) for container_of(), and replace all wvif->vif with
> the newly defined container_of construct.
>
> Signed-off-by: Jaehee Park <[email protected]>
> ---
>
> Changes from staging to wireless-next tree
> - changed macro into function and named it back to wvif_to_vif
> - fit all lines in patch to 80 columns
> - decared a reference to vif at the beginning of the functions
>
> NOTE: J?r?me is going to be testing this patch on his hardware
Don't forget to increment the version number of your submission (option
-v of git send-email).
> drivers/net/wireless/silabs/wfx/wfx.h | 6 +-
> drivers/net/wireless/silabs/wfx/data_rx.c | 5 +-
> drivers/net/wireless/silabs/wfx/data_tx.c | 3 +-
> drivers/net/wireless/silabs/wfx/key.c | 4 +-
> drivers/net/wireless/silabs/wfx/queue.c | 3 +-
> drivers/net/wireless/silabs/wfx/scan.c | 9 ++-
> drivers/net/wireless/silabs/wfx/sta.c | 69 ++++++++++++++---------
> 7 files changed, 63 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/net/wireless/silabs/wfx/wfx.h b/drivers/net/wireless/silabs/wfx/wfx.h
> index 6594cc647c2f..718693a4273d 100644
> --- a/drivers/net/wireless/silabs/wfx/wfx.h
> +++ b/drivers/net/wireless/silabs/wfx/wfx.h
> @@ -61,7 +61,6 @@ struct wfx_dev {
>
> struct wfx_vif {
> struct wfx_dev *wdev;
> - struct ieee80211_vif *vif;
> struct ieee80211_channel *channel;
> int id;
>
> @@ -91,6 +90,11 @@ struct wfx_vif {
> struct completion set_pm_mode_complete;
> };
>
> +static inline struct ieee80211_vif *wvif_to_vif(struct wfx_vif *wvif)
> +{
> + return container_of((void *)wvif, struct ieee80211_vif, drv_priv);
> +}
> +
> static inline struct wfx_vif *wdev_to_wvif(struct wfx_dev *wdev, int vif_id)
> {
> if (vif_id >= ARRAY_SIZE(wdev->vif)) {
> diff --git a/drivers/net/wireless/silabs/wfx/data_rx.c b/drivers/net/wireless/silabs/wfx/data_rx.c
> index a4b5ffe158e4..342b9cd0e74c 100644
> --- a/drivers/net/wireless/silabs/wfx/data_rx.c
> +++ b/drivers/net/wireless/silabs/wfx/data_rx.c
> @@ -16,6 +16,7 @@
> static void wfx_rx_handle_ba(struct wfx_vif *wvif, struct ieee80211_mgmt *mgmt)
> {
> int params, tid;
> + struct ieee80211_vif *vif = wvif_to_vif(wvif);
When you can, try to place the longest declaration first ("reverse
Christmas tree order").
[...]
> diff --git a/drivers/net/wireless/silabs/wfx/sta.c b/drivers/net/wireless/silabs/wfx/sta.c
> index 3297d73c327a..97fcbad23c94 100644
> --- a/drivers/net/wireless/silabs/wfx/sta.c
> +++ b/drivers/net/wireless/silabs/wfx/sta.c
[...]
> @@ -152,19 +153,28 @@ static int wfx_get_ps_timeout(struct wfx_vif *wvif, bool *enable_ps)
> {
> struct ieee80211_channel *chan0 = NULL, *chan1 = NULL;
> struct ieee80211_conf *conf = &wvif->wdev->hw->conf;
> + struct ieee80211_vif *vif = wvif_to_vif(wvif);
>
> - WARN(!wvif->vif->bss_conf.assoc && enable_ps,
> + WARN(!vif->bss_conf.assoc && enable_ps,
> "enable_ps is reliable only if associated");
> - if (wdev_to_wvif(wvif->wdev, 0))
> - chan0 = wdev_to_wvif(wvif->wdev, 0)->vif->bss_conf.chandef.chan;
> - if (wdev_to_wvif(wvif->wdev, 1))
> - chan1 = wdev_to_wvif(wvif->wdev, 1)->vif->bss_conf.chandef.chan;
> - if (chan0 && chan1 && wvif->vif->type != NL80211_IFTYPE_AP) {
> + if (wdev_to_wvif(wvif->wdev, 0)) {
> + struct wfx_vif *wvif_ch0 = wdev_to_wvif(wvif->wdev, 0);
> + struct ieee80211_vif *vif_ch0 = wvif_to_vif(wvif_ch0);
> +
> + chan0 = vif_ch0->bss_conf.chandef.chan;
> + }
> + if (wdev_to_wvif(wvif->wdev, 1)) {
> + struct wfx_vif *wvif_ch1 = wdev_to_wvif(wvif->wdev, 1);
> + struct ieee80211_vif *vif_ch1 = wvif_to_vif(wvif_ch1);
> +
> + chan1 = vif_ch1->bss_conf.chandef.chan;
> + }
I think this code could be simplified into:
if (wvif->wdev->vif[1])
chan1 = wvif->wdev->vif[1]->bss_conf.chandef.chan;
(If you choose this way, I suggest to place this change in a separate
patch)
[...]
--
J?r?me Pouiller
Jaehee Park <[email protected]> writes:
> Currently, upon virtual interface creation, wfx_add_interface() stores
> a reference to the corresponding struct ieee80211_vif in private data,
> for later usage. This is not needed when using the container_of
> construct. This construct already has all the info it needs to retrieve
> the reference to the corresponding struct from the offset that is
> already available, inherent in container_of(), between its type and
> member inputs (struct ieee80211_vif and drv_priv, respectively).
> Remove vif (which was previously storing the reference to the struct
> ieee80211_vif) from the struct wfx_vif, define a function
> wvif_to_vif(wvif) for container_of(), and replace all wvif->vif with
> the newly defined container_of construct.
>
> Signed-off-by: Jaehee Park <[email protected]>
[...]
> +static inline struct ieee80211_vif *wvif_to_vif(struct wfx_vif *wvif)
> +{
> + return container_of((void *)wvif, struct ieee80211_vif, drv_priv);
> +}
Why the void pointer cast? Avoid casts as much possible.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
On mercoledì 20 aprile 2022 13:57:57 CEST Kalle Valo wrote:
> Jaehee Park <[email protected]> writes:
>
> > Currently, upon virtual interface creation, wfx_add_interface() stores
> > a reference to the corresponding struct ieee80211_vif in private data,
> > for later usage. This is not needed when using the container_of
> > construct. This construct already has all the info it needs to retrieve
> > the reference to the corresponding struct from the offset that is
> > already available, inherent in container_of(), between its type and
> > member inputs (struct ieee80211_vif and drv_priv, respectively).
> > Remove vif (which was previously storing the reference to the struct
> > ieee80211_vif) from the struct wfx_vif, define a function
> > wvif_to_vif(wvif) for container_of(), and replace all wvif->vif with
> > the newly defined container_of construct.
> >
> > Signed-off-by: Jaehee Park <[email protected]>
>
> [...]
>
> > +static inline struct ieee80211_vif *wvif_to_vif(struct wfx_vif *wvif)
> > +{
> > + return container_of((void *)wvif, struct ieee80211_vif,
drv_priv);
> > +}
>
> Why the void pointer cast? Avoid casts as much possible.
In a previous email Jaehee wrote that she could compile her changes only by
using that "(void *)" cast.
I replied that probably this is a hint that something is broken, although
my argument is not necessarily a "proof". Might very well be that this cast
was needed in this particular situation but I cannot see why.
@Jaehee, please try to explain why this "(void *)" cast is actually
necessary and why your changes cannot avoid it.
Thanks,
Fabio M. De Francesco
On Wed, Apr 20, 2022 at 7:58 AM Kalle Valo <[email protected]> wrote:
>
> Jaehee Park <[email protected]> writes:
>
> > Currently, upon virtual interface creation, wfx_add_interface() stores
> > a reference to the corresponding struct ieee80211_vif in private data,
> > for later usage. This is not needed when using the container_of
> > construct. This construct already has all the info it needs to retrieve
> > the reference to the corresponding struct from the offset that is
> > already available, inherent in container_of(), between its type and
> > member inputs (struct ieee80211_vif and drv_priv, respectively).
> > Remove vif (which was previously storing the reference to the struct
> > ieee80211_vif) from the struct wfx_vif, define a function
> > wvif_to_vif(wvif) for container_of(), and replace all wvif->vif with
> > the newly defined container_of construct.
> >
> > Signed-off-by: Jaehee Park <[email protected]>
>
> [...]
>
> > +static inline struct ieee80211_vif *wvif_to_vif(struct wfx_vif *wvif)
> > +{
> > + return container_of((void *)wvif, struct ieee80211_vif, drv_priv);
> > +}
>
> Why the void pointer cast? Avoid casts as much possible.
>
Hi Kalle,
Sorry for the delay in getting back to you about why the void pointer
cast was used.
In essence, I'm taking private data with a driver-specific pointer
and that needs to be resolved back to a generic pointer.
The private data (drv_priv) is declared as a generic u8 array in struct
ieee80211_vif, but wvif is a more specific type.
I wanted to also point to existing, reasonable examples such as:
static void iwl_mvm_tcm_uapsd_nonagg_detected_wk(struct work_struct *wk)
{
struct iwl_mvm *mvm;
struct iwl_mvm_vif *mvmvif;
struct ieee80211_vif *vif;
mvmvif = container_of(wk, struct iwl_mvm_vif,
uapsd_nonagg_detected_wk.work);
vif = container_of((void *)mvmvif, struct ieee80211_vif, drv_priv);
in drivers/net/wireless$ less intel/iwlwifi/mvm/utils.c, which does the
same thing.
There are fifteen of them throughout:
wireless-next/drivers/net/wireless$ grep -rn "container_of(.* ieee80211_vif"
intel/iwlwifi/mvm/utils.c:794: vif = container_of((void *)mvmvif,
struct ieee80211_vif, drv_priv);
intel/iwlwifi/mvm/mac80211.c:1347: vif = container_of((void
*)mvmvif, struct ieee80211_vif, drv_priv);
mediatek/mt76/mt76x02_mmio.c:415: vif =
container_of(priv, struct ieee80211_vif, drv_priv);
mediatek/mt76/mt7615/mac.c:275: vif = container_of((void *)msta->vif,
struct ieee80211_vif, drv_priv);
mediatek/mt76/mt7915/mac.c:416: vif = container_of((void *)msta->vif,
struct ieee80211_vif, drv_priv);
mediatek/mt76/mt7915/mac.c:2327: vif =
container_of((void *)msta->vif, struct ieee80211_vif, drv_priv);
mediatek/mt76/mt7915/debugfs.c:1026: vif = container_of((void
*)msta->vif, struct ieee80211_vif, drv_priv);
mediatek/mt76/mt7921/mac.c:425: vif = container_of((void *)msta->vif,
struct ieee80211_vif, drv_priv);
ti/wlcore/wlcore_i.h:502: return container_of((void *)wlvif,
struct ieee80211_vif, drv_priv);
realtek/rtl818x/rtl8187/dev.c:1068: container_of((void
*)vif_priv, struct ieee80211_vif, drv_priv);
realtek/rtl818x/rtl8180/dev.c:1293: container_of((void
*)vif_priv, struct ieee80211_vif, drv_priv);
realtek/rtw88/main.h:2075: return container_of(p, struct
ieee80211_vif, drv_priv);
realtek/rtw89/core.h:3440: return container_of(p, struct
ieee80211_vif, drv_priv);
ath/carl9170/carl9170.h:641: return container_of((void *)priv,
struct ieee80211_vif, drv_priv);
ath/wcn36xx/wcn36xx.h:329: return container_of((void *) vif_priv,
struct ieee80211_vif, drv_priv);
Thanks,
Jaehee
> --
> https://patchwork.kernel.org/project/linux-wireless/list/
>
> https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
On Tue, Apr 19, 2022 at 03:39:19PM +0200, J?r?me Pouiller wrote:
> On Monday 18 April 2022 05:51:10 CEST Jaehee Park wrote:
> >
> > Currently, upon virtual interface creation, wfx_add_interface() stores
> > a reference to the corresponding struct ieee80211_vif in private data,
> > for later usage. This is not needed when using the container_of
> > construct. This construct already has all the info it needs to retrieve
> > the reference to the corresponding struct from the offset that is
> > already available, inherent in container_of(), between its type and
> > member inputs (struct ieee80211_vif and drv_priv, respectively).
> > Remove vif (which was previously storing the reference to the struct
> > ieee80211_vif) from the struct wfx_vif, define a function
> > wvif_to_vif(wvif) for container_of(), and replace all wvif->vif with
> > the newly defined container_of construct.
> >
> > Signed-off-by: Jaehee Park <[email protected]>
> > ---
> >
> > Changes from staging to wireless-next tree
> > - changed macro into function and named it back to wvif_to_vif
> > - fit all lines in patch to 80 columns
> > - decared a reference to vif at the beginning of the functions
> >
> > NOTE: J?r?me is going to be testing this patch on his hardware
>
> Don't forget to increment the version number of your submission (option
> -v of git send-email).
>
> > drivers/net/wireless/silabs/wfx/wfx.h | 6 +-
> > drivers/net/wireless/silabs/wfx/data_rx.c | 5 +-
> > drivers/net/wireless/silabs/wfx/data_tx.c | 3 +-
> > drivers/net/wireless/silabs/wfx/key.c | 4 +-
> > drivers/net/wireless/silabs/wfx/queue.c | 3 +-
> > drivers/net/wireless/silabs/wfx/scan.c | 9 ++-
> > drivers/net/wireless/silabs/wfx/sta.c | 69 ++++++++++++++---------
> > 7 files changed, 63 insertions(+), 36 deletions(-)
> >
> > diff --git a/drivers/net/wireless/silabs/wfx/wfx.h b/drivers/net/wireless/silabs/wfx/wfx.h
> > index 6594cc647c2f..718693a4273d 100644
> > --- a/drivers/net/wireless/silabs/wfx/wfx.h
> > +++ b/drivers/net/wireless/silabs/wfx/wfx.h
> > @@ -61,7 +61,6 @@ struct wfx_dev {
> >
> > struct wfx_vif {
> > struct wfx_dev *wdev;
> > - struct ieee80211_vif *vif;
> > struct ieee80211_channel *channel;
> > int id;
> >
> > @@ -91,6 +90,11 @@ struct wfx_vif {
> > struct completion set_pm_mode_complete;
> > };
> >
> > +static inline struct ieee80211_vif *wvif_to_vif(struct wfx_vif *wvif)
> > +{
> > + return container_of((void *)wvif, struct ieee80211_vif, drv_priv);
> > +}
> > +
> > static inline struct wfx_vif *wdev_to_wvif(struct wfx_dev *wdev, int vif_id)
> > {
> > if (vif_id >= ARRAY_SIZE(wdev->vif)) {
> > diff --git a/drivers/net/wireless/silabs/wfx/data_rx.c b/drivers/net/wireless/silabs/wfx/data_rx.c
> > index a4b5ffe158e4..342b9cd0e74c 100644
> > --- a/drivers/net/wireless/silabs/wfx/data_rx.c
> > +++ b/drivers/net/wireless/silabs/wfx/data_rx.c
> > @@ -16,6 +16,7 @@
> > static void wfx_rx_handle_ba(struct wfx_vif *wvif, struct ieee80211_mgmt *mgmt)
> > {
> > int params, tid;
> > + struct ieee80211_vif *vif = wvif_to_vif(wvif);
>
> When you can, try to place the longest declaration first ("reverse
> Christmas tree order").
Thanks Jerome, I have a new version of the patch I'm going to send today
with this edit.
>
> [...]
> > diff --git a/drivers/net/wireless/silabs/wfx/sta.c b/drivers/net/wireless/silabs/wfx/sta.c
> > index 3297d73c327a..97fcbad23c94 100644
> > --- a/drivers/net/wireless/silabs/wfx/sta.c
> > +++ b/drivers/net/wireless/silabs/wfx/sta.c
> [...]
> > @@ -152,19 +153,28 @@ static int wfx_get_ps_timeout(struct wfx_vif *wvif, bool *enable_ps)
> > {
> > struct ieee80211_channel *chan0 = NULL, *chan1 = NULL;
> > struct ieee80211_conf *conf = &wvif->wdev->hw->conf;
> > + struct ieee80211_vif *vif = wvif_to_vif(wvif);
> >
> > - WARN(!wvif->vif->bss_conf.assoc && enable_ps,
> > + WARN(!vif->bss_conf.assoc && enable_ps,
> > "enable_ps is reliable only if associated");
> > - if (wdev_to_wvif(wvif->wdev, 0))
> > - chan0 = wdev_to_wvif(wvif->wdev, 0)->vif->bss_conf.chandef.chan;
> > - if (wdev_to_wvif(wvif->wdev, 1))
> > - chan1 = wdev_to_wvif(wvif->wdev, 1)->vif->bss_conf.chandef.chan;
> > - if (chan0 && chan1 && wvif->vif->type != NL80211_IFTYPE_AP) {
> > + if (wdev_to_wvif(wvif->wdev, 0)) {
> > + struct wfx_vif *wvif_ch0 = wdev_to_wvif(wvif->wdev, 0);
> > + struct ieee80211_vif *vif_ch0 = wvif_to_vif(wvif_ch0);
> > +
> > + chan0 = vif_ch0->bss_conf.chandef.chan;
> > + }
> > + if (wdev_to_wvif(wvif->wdev, 1)) {
> > + struct wfx_vif *wvif_ch1 = wdev_to_wvif(wvif->wdev, 1);
> > + struct ieee80211_vif *vif_ch1 = wvif_to_vif(wvif_ch1);
> > +
> > + chan1 = vif_ch1->bss_conf.chandef.chan;
> > + }
>
> I think this code could be simplified into:
>
> if (wvif->wdev->vif[1])
> chan1 = wvif->wdev->vif[1]->bss_conf.chandef.chan;
>
> (If you choose this way, I suggest to place this change in a separate
> patch)
>
Ok I'll send your suggested edit in a separate patch after this patch
gets through.
> [...]
>
> --
> J?r?me Pouiller
>
>
On Mon, May 02, 2022 at 02:10:07PM -0400, Jaehee wrote:
> On Wed, Apr 20, 2022 at 7:58 AM Kalle Valo <[email protected]> wrote:
> >
> > Jaehee Park <[email protected]> writes:
> >
> > > Currently, upon virtual interface creation, wfx_add_interface() stores
> > > a reference to the corresponding struct ieee80211_vif in private data,
> > > for later usage. This is not needed when using the container_of
> > > construct. This construct already has all the info it needs to retrieve
> > > the reference to the corresponding struct from the offset that is
> > > already available, inherent in container_of(), between its type and
> > > member inputs (struct ieee80211_vif and drv_priv, respectively).
> > > Remove vif (which was previously storing the reference to the struct
> > > ieee80211_vif) from the struct wfx_vif, define a function
> > > wvif_to_vif(wvif) for container_of(), and replace all wvif->vif with
> > > the newly defined container_of construct.
> > >
> > > Signed-off-by: Jaehee Park <[email protected]>
> >
> > [...]
> >
> > > +static inline struct ieee80211_vif *wvif_to_vif(struct wfx_vif *wvif)
> > > +{
> > > + return container_of((void *)wvif, struct ieee80211_vif, drv_priv);
> > > +}
> >
> > Why the void pointer cast? Avoid casts as much possible.
> >
>
> Hi Kalle,
>
> Sorry for the delay in getting back to you about why the void pointer
> cast was used.
>
> In essence, I'm taking private data with a driver-specific pointer
> and that needs to be resolved back to a generic pointer.
>
> The private data (drv_priv) is declared as a generic u8 array in struct
> ieee80211_vif, but wvif is a more specific type.
>
> I wanted to also point to existing, reasonable examples such as:
> static void iwl_mvm_tcm_uapsd_nonagg_detected_wk(struct work_struct *wk)
> {
> struct iwl_mvm *mvm;
> struct iwl_mvm_vif *mvmvif;
> struct ieee80211_vif *vif;
>
> mvmvif = container_of(wk, struct iwl_mvm_vif,
> uapsd_nonagg_detected_wk.work);
> vif = container_of((void *)mvmvif, struct ieee80211_vif, drv_priv);
>
> in drivers/net/wireless$ less intel/iwlwifi/mvm/utils.c, which does the
> same thing.
>
> There are fifteen of them throughout:
The cast is fine, but this email is frustrating.
It sounds like you are saying that you copied it from other code and
that's not a good answer... :/ It's easiest if you just copy and paste
the build error and we can figure out why the cast is need for our
selves...
drivers/net/wireless/silabs/wfx/data_rx.c: In function ‘wvif_to_vif’:
./include/linux/build_bug.h:78:41: error: static assertion failed: "pointer type mismatch in container_of()"
78 | #define __static_assert(expr, msg, ...) _Static_assert(expr, msg)
| ^~~~~~~~~~~~~~
./include/linux/build_bug.h:77:34: note: in expansion of macro ‘__static_assert’
77 | #define static_assert(expr, ...) __static_assert(expr, ##__VA_ARGS__, #expr)
| ^~~~~~~~~~~~~~~
./include/linux/container_of.h:19:9: note: in expansion of macro ‘static_assert’
19 | static_assert(__same_type(*(ptr), ((type *)0)->member) || \
| ^~~~~~~~~~~~~
drivers/net/wireless/silabs/wfx/data_rx.c:20:16: note: in expansion of macro ‘container_of’
20 | return container_of(wvif, struct ieee80211_vif, drv_priv);
| ^~~~~~~~~~~~
regards,
dan carpenter
Hi Dan,
On Wed, 4 May 2022 12:33:48 +0300
Dan Carpenter <[email protected]> wrote:
> On Mon, May 02, 2022 at 02:10:07PM -0400, Jaehee wrote:
> > On Wed, Apr 20, 2022 at 7:58 AM Kalle Valo <[email protected]> wrote:
> > >
> > > Jaehee Park <[email protected]> writes:
> > >
> > > > Currently, upon virtual interface creation, wfx_add_interface() stores
> > > > a reference to the corresponding struct ieee80211_vif in private data,
> > > > for later usage. This is not needed when using the container_of
> > > > construct. This construct already has all the info it needs to retrieve
> > > > the reference to the corresponding struct from the offset that is
> > > > already available, inherent in container_of(), between its type and
> > > > member inputs (struct ieee80211_vif and drv_priv, respectively).
> > > > Remove vif (which was previously storing the reference to the struct
> > > > ieee80211_vif) from the struct wfx_vif, define a function
> > > > wvif_to_vif(wvif) for container_of(), and replace all wvif->vif with
> > > > the newly defined container_of construct.
> > > >
> > > > Signed-off-by: Jaehee Park <[email protected]>
> > >
> > > [...]
> > >
> > > > +static inline struct ieee80211_vif *wvif_to_vif(struct wfx_vif *wvif)
> > > > +{
> > > > + return container_of((void *)wvif, struct ieee80211_vif, drv_priv);
> > > > +}
> > >
> > > Why the void pointer cast? Avoid casts as much possible.
> > >
> >
> > Hi Kalle,
> >
> > Sorry for the delay in getting back to you about why the void pointer
> > cast was used.
> >
> > In essence, I'm taking private data with a driver-specific pointer
> > and that needs to be resolved back to a generic pointer.
> >
> > The private data (drv_priv) is declared as a generic u8 array in struct
> > ieee80211_vif, but wvif is a more specific type.
> >
> > I wanted to also point to existing, reasonable examples such as:
> > static void iwl_mvm_tcm_uapsd_nonagg_detected_wk(struct work_struct *wk)
> > {
> > struct iwl_mvm *mvm;
> > struct iwl_mvm_vif *mvmvif;
> > struct ieee80211_vif *vif;
> >
> > mvmvif = container_of(wk, struct iwl_mvm_vif,
> > uapsd_nonagg_detected_wk.work);
> > vif = container_of((void *)mvmvif, struct ieee80211_vif, drv_priv);
> >
> > in drivers/net/wireless$ less intel/iwlwifi/mvm/utils.c, which does the
> > same thing.
> >
> > There are fifteen of them throughout:
>
> The cast is fine, but this email is frustrating.
>
> It sounds like you are saying that you copied it from other code and
> that's not a good answer... :/ It's easiest if you just copy and paste
> the build error and we can figure out why the cast is need for our
> selves...
...my bad, then.
I suggested to Jaehee she would *also* point out that there are already
a pile of usages (which I grepped for myself, by the way).
And that it's *obvious* that container_of() would trigger warnings
otherwise. Well, obvious just for me, it seems.
--
Stefano
On Wed, May 04, 2022 at 01:50:59PM +0200, Stefano Brivio wrote:
> And that it's *obvious* that container_of() would trigger warnings
> otherwise. Well, obvious just for me, it seems.
:P
Apparently it wasn't obvious to Kalle and me. My guess is that you saw
the build error. Either that or Kalle and I are geezers who haven't
looked at container_of() since before the BUILD_BUG_ON() was added five
years ago.
regards,
dan carpenter
Dan Carpenter <[email protected]> writes:
> On Wed, May 04, 2022 at 01:50:59PM +0200, Stefano Brivio wrote:
>> And that it's *obvious* that container_of() would trigger warnings
>> otherwise. Well, obvious just for me, it seems.
>
> :P
>
> Apparently it wasn't obvious to Kalle and me. My guess is that you saw
> the build error. Either that or Kalle and I are geezers who haven't
> looked at container_of() since before the BUILD_BUG_ON() was added five
> years ago.
Exactly, I also had to duplicate the error myself before I was able to
understand the issue. So I'm officially a geezer now :D
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
On Wed, May 04, 2022 at 07:05:57PM +0300, Kalle Valo wrote:
> Dan Carpenter <[email protected]> writes:
>
> > On Wed, May 04, 2022 at 01:50:59PM +0200, Stefano Brivio wrote:
> >> And that it's *obvious* that container_of() would trigger warnings
> >> otherwise. Well, obvious just for me, it seems.
> >
> > :P
> >
> > Apparently it wasn't obvious to Kalle and me. My guess is that you saw
> > the build error. Either that or Kalle and I are geezers who haven't
> > looked at container_of() since before the BUILD_BUG_ON() was added five
> > years ago.
>
> Exactly, I also had to duplicate the error myself before I was able to
> understand the issue. So I'm officially a geezer now :D
>
Hi Dan and Kalle, thanks for your messages!
Understood -- I'll copy the error for reference next time. Sorry you
had to recreate the build error yourself!
> --
> https://patchwork.kernel.org/project/linux-wireless/list/
>
> https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches