[PATCH] wfx: use container_of() to get vif

[PATCH] wfx: use container_of() to get vif

[Jaehee Park]

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 <jhpark1013@gmail.com>
---

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

Re: [PATCH] wfx: use container_of() to get vif

[Jérôme Pouiller]

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 <jhpark1013@gmail.com>
> ---
> 
> 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

Re: [PATCH] wfx: use container_of() to get vif

[Kalle Valo]

Jaehee Park <jhpark1013@gmail.com> 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 <jhpark1013@gmail.com>

[...]

> +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

Re: [PATCH] wfx: use container_of() to get vif

[Fabio M. De Francesco]

On mercoledì 20 aprile 2022 13:57:57 CEST Kalle Valo wrote:
> Jaehee Park <jhpark1013@gmail.com> 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 <jhpark1013@gmail.com>
> 
> [...]
> 
> > +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

Re: [PATCH] wfx: use container_of() to get vif

[Jaehee]

On Wed, Apr 20, 2022 at 7:58 AM Kalle Valo <kvalo@kernel.org> wrote:
>
> Jaehee Park <jhpark1013@gmail.com> 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 <jhpark1013@gmail.com>
>
> [...]
>
> > +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

Re: [PATCH] wfx: use container_of() to get vif

[Jaehee Park]

On Mon, May 02, 2022 at 02:10:07PM -0400, Jaehee wrote:
> On Wed, Apr 20, 2022 at 7:58 AM Kalle Valo <kvalo@kernel.org> wrote:
> >
> > Jaehee Park <jhpark1013@gmail.com> 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 <jhpark1013@gmail.com>
> >
> > [...]
> >
> > > +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);
> 

Sorry -- here's the last portion of the email without wrapping.

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

Re: [PATCH] wfx: use container_of() to get vif

[Jaehee Park]

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 <jhpark1013@gmail.com>
> > ---
> > 
> > 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
> 
> 

Re: [PATCH] wfx: use container_of() to get vif

[Dan Carpenter]

On Mon, May 02, 2022 at 02:10:07PM -0400, Jaehee wrote:
> On Wed, Apr 20, 2022 at 7:58 AM Kalle Valo <kvalo@kernel.org> wrote:
> >
> > Jaehee Park <jhpark1013@gmail.com> 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 <jhpark1013@gmail.com>
> >
> > [...]
> >
> > > +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

Re: [PATCH] wfx: use container_of() to get vif

[Stefano Brivio]

Hi Dan,

On Wed, 4 May 2022 12:33:48 +0300
Dan Carpenter <dan.carpenter@oracle.com> wrote:

> On Mon, May 02, 2022 at 02:10:07PM -0400, Jaehee wrote:
> > On Wed, Apr 20, 2022 at 7:58 AM Kalle Valo <kvalo@kernel.org> wrote:  
> > >
> > > Jaehee Park <jhpark1013@gmail.com> 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 <jhpark1013@gmail.com>  
> > >
> > > [...]
> > >  
> > > > +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

Re: [PATCH] wfx: use container_of() to get vif

[Dan Carpenter]

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

Re: [PATCH] wfx: use container_of() to get vif

[Kalle Valo]

Dan Carpenter <dan.carpenter@oracle.com> 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

Re: [PATCH] wfx: use container_of() to get vif

[Jaehee Park]

On Wed, May 04, 2022 at 07:05:57PM +0300, Kalle Valo wrote:
> Dan Carpenter <dan.carpenter@oracle.com> 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