linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] staging: wfx: use container_of() to get vif
@ 2022-04-07 20:41 Jaehee Park
  2022-04-07 21:17 ` Jaehee Park
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Jaehee Park @ 2022-04-07 20:41 UTC (permalink / raw)
  To: Jérôme Pouiller, Greg Kroah-Hartman, linux-staging,
	linux-kernel, outreachy, Jaehee Park, Stefano Brivio

Use container_of() to get vif. This improves the code in two ways:
[1] it speeds up the compilation because container_of() saves steps to
retrieve vif (the representation of ieee80211_vif), and
[2] it eliminates the need to define multiple pointers. Previously,
after setting the pointer to drv_priv, another pointer had to be
defined by vif in struct wfx_vif to point back to ieee80211_vif.
Remove vif from the struct wfx_vif, define a macro wvif_to_vif(ptr)
for container_of(), and replace all wvif->vif with the newly defined
container_of() construct.

Signed-off-by: Jaehee Park <jhpark1013@gmail.com>
---
Questions:
- These changes built for me, but it would be great if someone can test it on the actual hardware.
- We noticed that the development of the wfx driver is happening in netdev instead of the staging directory. Do you suggest that I use the net-next tree for this change?
 
 drivers/staging/wfx/data_rx.c |  4 ++--
 drivers/staging/wfx/data_tx.c |  2 +-
 drivers/staging/wfx/key.c     |  3 +--
 drivers/staging/wfx/queue.c   |  2 +-
 drivers/staging/wfx/scan.c    |  6 ++---
 drivers/staging/wfx/sta.c     | 45 ++++++++++++++++-------------------
 drivers/staging/wfx/wfx.h     |  3 ++-
 7 files changed, 31 insertions(+), 34 deletions(-)

diff --git a/drivers/staging/wfx/data_rx.c b/drivers/staging/wfx/data_rx.c
index a4b5ffe158e4..98c2223089b8 100644
--- a/drivers/staging/wfx/data_rx.c
+++ b/drivers/staging/wfx/data_rx.c
@@ -24,12 +24,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(wvif_to_vif(wvif), 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(wvif_to_vif(wvif), mgmt->sa, tid);
 		break;
 	}
 }
diff --git a/drivers/staging/wfx/data_tx.c b/drivers/staging/wfx/data_tx.c
index e07381b2ff4d..fd79e03a08c5 100644
--- a/drivers/staging/wfx/data_tx.c
+++ b/drivers/staging/wfx/data_tx.c
@@ -216,7 +216,7 @@ static u8 wfx_tx_get_link_id(struct wfx_vif *wvif, struct ieee80211_sta *sta,
 
 	if (sta_priv && sta_priv->link_id)
 		return sta_priv->link_id;
-	if (wvif->vif->type != NL80211_IFTYPE_AP)
+	if (wvif_to_vif(wvif)->type != NL80211_IFTYPE_AP)
 		return 0;
 	if (is_multicast_ether_addr(da))
 		return 0;
diff --git a/drivers/staging/wfx/key.c b/drivers/staging/wfx/key.c
index 8f23e8d42bd4..71b1fca70248 100644
--- a/drivers/staging/wfx/key.c
+++ b/drivers/staging/wfx/key.c
@@ -174,7 +174,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);
+						 wvif_to_vif(wvif)->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 +224,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/staging/wfx/queue.c b/drivers/staging/wfx/queue.c
index 729825230db2..a2745d1024c6 100644
--- a/drivers/staging/wfx/queue.c
+++ b/drivers/staging/wfx/queue.c
@@ -207,7 +207,7 @@ bool wfx_tx_queues_has_cab(struct wfx_vif *wvif)
 {
 	int i;
 
-	if (wvif->vif->type != NL80211_IFTYPE_AP)
+	if (wvif_to_vif(wvif)->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/staging/wfx/scan.c b/drivers/staging/wfx/scan.c
index 7f34f0d322f9..5c25fde3fc41 100644
--- a/drivers/staging/wfx/scan.c
+++ b/drivers/staging/wfx/scan.c
@@ -25,7 +25,7 @@ static int update_probe_tmpl(struct wfx_vif *wvif, struct cfg80211_scan_request
 {
 	struct sk_buff *skb;
 
-	skb = ieee80211_probereq_get(wvif->wdev->hw, wvif->vif->addr, NULL, 0, req->ie_len);
+	skb = ieee80211_probereq_get(wvif->wdev->hw, wvif_to_vif(wvif)->addr, NULL, 0, req->ie_len);
 	if (!skb)
 		return -ENOMEM;
 
@@ -75,8 +75,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 != wvif_to_vif(wvif)->bss_conf.txpower)
+		wfx_hif_set_output_power(wvif, wvif_to_vif(wvif)->bss_conf.txpower);
 	wfx_tx_unlock(wvif->wdev);
 	return ret;
 }
diff --git a/drivers/staging/wfx/sta.c b/drivers/staging/wfx/sta.c
index 03025ef7f1be..b634c8e004db 100644
--- a/drivers/staging/wfx/sta.c
+++ b/drivers/staging/wfx/sta.c
@@ -132,7 +132,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 && wvif_to_vif(wvif)->type == NL80211_IFTYPE_AP) {
 			dev_dbg(wdev->dev, "do not forward probe request in AP mode\n");
 			*total_flags &= ~FIF_PROBE_REQ;
 		}
@@ -153,18 +153,18 @@ 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;
 
-	WARN(!wvif->vif->bss_conf.assoc && enable_ps,
+	WARN(!wvif_to_vif(wvif)->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;
+		chan0 = wvif_to_vif(wdev_to_wvif(wvif->wdev, 0))->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) {
+		chan1 = wvif_to_vif(wdev_to_wvif(wvif->wdev, 1))->bss_conf.chandef.chan;
+	if (chan0 && chan1 && wvif_to_vif(wvif)->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 (wvif_to_vif(wvif)->bss_conf.assoc && wvif_to_vif(wvif)->bss_conf.ps)
 				dev_info(wvif->wdev->dev, "ignoring requested PS mode");
 			return -1;
 		}
@@ -177,8 +177,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 = wvif_to_vif(wvif)->bss_conf.ps;
+	if (wvif_to_vif(wvif)->bss_conf.assoc && wvif_to_vif(wvif)->bss_conf.ps)
 		return conf->dynamic_ps_timeout;
 	else
 		return -1;
@@ -189,7 +189,7 @@ int wfx_update_pm(struct wfx_vif *wvif)
 	int ps_timeout;
 	bool ps;
 
-	if (!wvif->vif->bss_conf.assoc)
+	if (!wvif_to_vif(wvif)->bss_conf.assoc)
 		return 0;
 	ps_timeout = wfx_get_ps_timeout(wvif, &ps);
 	if (!ps)
@@ -215,7 +215,7 @@ 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 (wvif_to_vif(wvif)->type == NL80211_IFTYPE_STATION && old_uapsd != wvif->uapsd_mask) {
 		wfx_hif_set_uapsd_info(wvif, wvif->uapsd_mask);
 		wfx_update_pm(wvif);
 	}
@@ -242,20 +242,20 @@ void wfx_event_report_rssi(struct wfx_vif *wvif, u8 raw_rcpi_rssi)
 	int cqm_evt;
 
 	rcpi_rssi = raw_rcpi_rssi / 2 - 110;
-	if (rcpi_rssi <= wvif->vif->bss_conf.cqm_rssi_thold)
+	if (rcpi_rssi <= wvif_to_vif(wvif)->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(wvif_to_vif(wvif), 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_bss_conf *bss_conf = &wvif_to_vif(wvif)->bss_conf;
 
-	ieee80211_beacon_loss(wvif->vif);
+	ieee80211_beacon_loss(wvif_to_vif(wvif));
 	schedule_delayed_work(to_delayed_work(work), msecs_to_jiffies(bss_conf->beacon_int));
 }
 
@@ -323,13 +323,13 @@ static int wfx_upload_ap_templates(struct wfx_vif *wvif)
 {
 	struct sk_buff *skb;
 
-	skb = ieee80211_beacon_get(wvif->wdev->hw, wvif->vif);
+	skb = ieee80211_beacon_get(wvif->wdev->hw, wvif_to_vif(wvif));
 	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, wvif_to_vif(wvif));
 	if (!skb)
 		return -ENOMEM;
 	wfx_hif_set_template_frame(wvif, skb, HIF_TMPLT_PRBRES, API_RATE_INDEX_B_1MBPS);
@@ -339,7 +339,7 @@ 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 sk_buff *skb = ieee80211_beacon_get(wvif->wdev->hw, wvif_to_vif(wvif));
 	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 +389,7 @@ 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_bss_conf *conf = &wvif_to_vif(wvif)->bss_conf;
 	struct cfg80211_bss *bss = NULL;
 	u8 ssid[IEEE80211_MAX_SSID_LEN];
 	const u8 *ssid_ie = NULL;
@@ -420,7 +420,7 @@ static void wfx_join(struct wfx_vif *wvif)
 	wvif->join_in_progress = true;
 	ret = wfx_hif_join(wvif, conf, wvif->channel, ssid, ssid_len);
 	if (ret) {
-		ieee80211_connection_loss(wvif->vif);
+		ieee80211_connection_loss(wvif_to_vif(wvif));
 		wfx_reset(wvif);
 	} else {
 		/* Due to beacon filtering it is possible that the AP's beacon is not known for the
@@ -440,7 +440,7 @@ static void wfx_join_finalize(struct wfx_vif *wvif, struct ieee80211_bss_conf *i
 
 	rcu_read_lock(); /* protect sta */
 	if (info->bssid && !info->ibss_joined)
-		sta = ieee80211_find_sta(wvif->vif, info->bssid);
+		sta = ieee80211_find_sta(wvif_to_vif(wvif), info->bssid);
 	if (sta && sta->ht_cap.ht_supported)
 		ampdu_density = sta->ht_cap.ampdu_density;
 	if (sta && sta->ht_cap.ht_supported &&
@@ -565,7 +565,7 @@ static int wfx_update_tim(struct wfx_vif *wvif)
 	u16 tim_offset, tim_length;
 	u8 *tim_ptr;
 
-	skb = ieee80211_beacon_get_tim(wvif->wdev->hw, wvif->vif, &tim_offset, &tim_length);
+	skb = ieee80211_beacon_get_tim(wvif->wdev->hw, wvif_to_vif(wvif), &tim_offset, &tim_length);
 	if (!skb)
 		return -ENOENT;
 	tim_ptr = skb->data + tim_offset;
@@ -707,8 +707,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 +765,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);
 
diff --git a/drivers/staging/wfx/wfx.h b/drivers/staging/wfx/wfx.h
index 6594cc647c2f..78f2a416fe4f 100644
--- a/drivers/staging/wfx/wfx.h
+++ b/drivers/staging/wfx/wfx.h
@@ -25,6 +25,8 @@
 #define USEC_PER_TXOP 32 /* see struct ieee80211_tx_queue_params */
 #define USEC_PER_TU 1024
 
+#define wvif_to_vif(ptr)(container_of((void *)ptr, struct ieee80211_vif, drv_priv))
+
 struct wfx_hwbus_ops;
 
 struct wfx_dev {
@@ -61,7 +63,6 @@ struct wfx_dev {
 
 struct wfx_vif {
 	struct wfx_dev             *wdev;
-	struct ieee80211_vif       *vif;
 	struct ieee80211_channel   *channel;
 	int                        id;
 
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] staging: wfx: use container_of() to get vif
  2022-04-07 20:41 [PATCH] staging: wfx: use container_of() to get vif Jaehee Park
@ 2022-04-07 21:17 ` Jaehee Park
  2022-04-08  1:09 ` Alison Schofield
  2022-04-08  9:49 ` Julia Lawall
  2 siblings, 0 replies; 9+ messages in thread
From: Jaehee Park @ 2022-04-07 21:17 UTC (permalink / raw)
  To: Jérôme Pouiller, Greg Kroah-Hartman, linux-staging,
	linux-kernel, outreachy, Stefano Brivio

On Thu, Apr 07, 2022 at 04:41:28PM -0400, Jaehee Park wrote:
> Use container_of() to get vif. This improves the code in two ways:
> [1] it speeds up the compilation because container_of() saves steps to
> retrieve vif (the representation of ieee80211_vif), and
> [2] it eliminates the need to define multiple pointers. Previously,
> after setting the pointer to drv_priv, another pointer had to be
> defined by vif in struct wfx_vif to point back to ieee80211_vif.
> Remove vif from the struct wfx_vif, define a macro wvif_to_vif(ptr)
> for container_of(), and replace all wvif->vif with the newly defined
> container_of() construct.
> 
> Signed-off-by: Jaehee Park <jhpark1013@gmail.com>
> ---
> Questions:
> - These changes built for me, but it would be great if someone can test it on the actual hardware.
> - We noticed that the development of the wfx driver is happening in netdev instead of the staging directory. Do you suggest that I use the net-next tree for this change?

Sorry, to clarify my question regarding which tree to submit this patch to, I meant the netdev tree vs greg's tree:
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/log/drivers/staging/wfx
https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/log/drivers/staging/wfx

>  
>  drivers/staging/wfx/data_rx.c |  4 ++--
>  drivers/staging/wfx/data_tx.c |  2 +-
>  drivers/staging/wfx/key.c     |  3 +--
>  drivers/staging/wfx/queue.c   |  2 +-
>  drivers/staging/wfx/scan.c    |  6 ++---
>  drivers/staging/wfx/sta.c     | 45 ++++++++++++++++-------------------
>  drivers/staging/wfx/wfx.h     |  3 ++-
>  7 files changed, 31 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/staging/wfx/data_rx.c b/drivers/staging/wfx/data_rx.c
> index a4b5ffe158e4..98c2223089b8 100644
> --- a/drivers/staging/wfx/data_rx.c
> +++ b/drivers/staging/wfx/data_rx.c
> @@ -24,12 +24,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(wvif_to_vif(wvif), 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(wvif_to_vif(wvif), mgmt->sa, tid);
>  		break;
>  	}
>  }
> diff --git a/drivers/staging/wfx/data_tx.c b/drivers/staging/wfx/data_tx.c
> index e07381b2ff4d..fd79e03a08c5 100644
> --- a/drivers/staging/wfx/data_tx.c
> +++ b/drivers/staging/wfx/data_tx.c
> @@ -216,7 +216,7 @@ static u8 wfx_tx_get_link_id(struct wfx_vif *wvif, struct ieee80211_sta *sta,
>  
>  	if (sta_priv && sta_priv->link_id)
>  		return sta_priv->link_id;
> -	if (wvif->vif->type != NL80211_IFTYPE_AP)
> +	if (wvif_to_vif(wvif)->type != NL80211_IFTYPE_AP)
>  		return 0;
>  	if (is_multicast_ether_addr(da))
>  		return 0;
> diff --git a/drivers/staging/wfx/key.c b/drivers/staging/wfx/key.c
> index 8f23e8d42bd4..71b1fca70248 100644
> --- a/drivers/staging/wfx/key.c
> +++ b/drivers/staging/wfx/key.c
> @@ -174,7 +174,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);
> +						 wvif_to_vif(wvif)->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 +224,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/staging/wfx/queue.c b/drivers/staging/wfx/queue.c
> index 729825230db2..a2745d1024c6 100644
> --- a/drivers/staging/wfx/queue.c
> +++ b/drivers/staging/wfx/queue.c
> @@ -207,7 +207,7 @@ bool wfx_tx_queues_has_cab(struct wfx_vif *wvif)
>  {
>  	int i;
>  
> -	if (wvif->vif->type != NL80211_IFTYPE_AP)
> +	if (wvif_to_vif(wvif)->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/staging/wfx/scan.c b/drivers/staging/wfx/scan.c
> index 7f34f0d322f9..5c25fde3fc41 100644
> --- a/drivers/staging/wfx/scan.c
> +++ b/drivers/staging/wfx/scan.c
> @@ -25,7 +25,7 @@ static int update_probe_tmpl(struct wfx_vif *wvif, struct cfg80211_scan_request
>  {
>  	struct sk_buff *skb;
>  
> -	skb = ieee80211_probereq_get(wvif->wdev->hw, wvif->vif->addr, NULL, 0, req->ie_len);
> +	skb = ieee80211_probereq_get(wvif->wdev->hw, wvif_to_vif(wvif)->addr, NULL, 0, req->ie_len);
>  	if (!skb)
>  		return -ENOMEM;
>  
> @@ -75,8 +75,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 != wvif_to_vif(wvif)->bss_conf.txpower)
> +		wfx_hif_set_output_power(wvif, wvif_to_vif(wvif)->bss_conf.txpower);
>  	wfx_tx_unlock(wvif->wdev);
>  	return ret;
>  }
> diff --git a/drivers/staging/wfx/sta.c b/drivers/staging/wfx/sta.c
> index 03025ef7f1be..b634c8e004db 100644
> --- a/drivers/staging/wfx/sta.c
> +++ b/drivers/staging/wfx/sta.c
> @@ -132,7 +132,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 && wvif_to_vif(wvif)->type == NL80211_IFTYPE_AP) {
>  			dev_dbg(wdev->dev, "do not forward probe request in AP mode\n");
>  			*total_flags &= ~FIF_PROBE_REQ;
>  		}
> @@ -153,18 +153,18 @@ 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;
>  
> -	WARN(!wvif->vif->bss_conf.assoc && enable_ps,
> +	WARN(!wvif_to_vif(wvif)->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;
> +		chan0 = wvif_to_vif(wdev_to_wvif(wvif->wdev, 0))->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) {
> +		chan1 = wvif_to_vif(wdev_to_wvif(wvif->wdev, 1))->bss_conf.chandef.chan;
> +	if (chan0 && chan1 && wvif_to_vif(wvif)->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 (wvif_to_vif(wvif)->bss_conf.assoc && wvif_to_vif(wvif)->bss_conf.ps)
>  				dev_info(wvif->wdev->dev, "ignoring requested PS mode");
>  			return -1;
>  		}
> @@ -177,8 +177,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 = wvif_to_vif(wvif)->bss_conf.ps;
> +	if (wvif_to_vif(wvif)->bss_conf.assoc && wvif_to_vif(wvif)->bss_conf.ps)
>  		return conf->dynamic_ps_timeout;
>  	else
>  		return -1;
> @@ -189,7 +189,7 @@ int wfx_update_pm(struct wfx_vif *wvif)
>  	int ps_timeout;
>  	bool ps;
>  
> -	if (!wvif->vif->bss_conf.assoc)
> +	if (!wvif_to_vif(wvif)->bss_conf.assoc)
>  		return 0;
>  	ps_timeout = wfx_get_ps_timeout(wvif, &ps);
>  	if (!ps)
> @@ -215,7 +215,7 @@ 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 (wvif_to_vif(wvif)->type == NL80211_IFTYPE_STATION && old_uapsd != wvif->uapsd_mask) {
>  		wfx_hif_set_uapsd_info(wvif, wvif->uapsd_mask);
>  		wfx_update_pm(wvif);
>  	}
> @@ -242,20 +242,20 @@ void wfx_event_report_rssi(struct wfx_vif *wvif, u8 raw_rcpi_rssi)
>  	int cqm_evt;
>  
>  	rcpi_rssi = raw_rcpi_rssi / 2 - 110;
> -	if (rcpi_rssi <= wvif->vif->bss_conf.cqm_rssi_thold)
> +	if (rcpi_rssi <= wvif_to_vif(wvif)->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(wvif_to_vif(wvif), 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_bss_conf *bss_conf = &wvif_to_vif(wvif)->bss_conf;
>  
> -	ieee80211_beacon_loss(wvif->vif);
> +	ieee80211_beacon_loss(wvif_to_vif(wvif));
>  	schedule_delayed_work(to_delayed_work(work), msecs_to_jiffies(bss_conf->beacon_int));
>  }
>  
> @@ -323,13 +323,13 @@ static int wfx_upload_ap_templates(struct wfx_vif *wvif)
>  {
>  	struct sk_buff *skb;
>  
> -	skb = ieee80211_beacon_get(wvif->wdev->hw, wvif->vif);
> +	skb = ieee80211_beacon_get(wvif->wdev->hw, wvif_to_vif(wvif));
>  	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, wvif_to_vif(wvif));
>  	if (!skb)
>  		return -ENOMEM;
>  	wfx_hif_set_template_frame(wvif, skb, HIF_TMPLT_PRBRES, API_RATE_INDEX_B_1MBPS);
> @@ -339,7 +339,7 @@ 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 sk_buff *skb = ieee80211_beacon_get(wvif->wdev->hw, wvif_to_vif(wvif));
>  	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 +389,7 @@ 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_bss_conf *conf = &wvif_to_vif(wvif)->bss_conf;
>  	struct cfg80211_bss *bss = NULL;
>  	u8 ssid[IEEE80211_MAX_SSID_LEN];
>  	const u8 *ssid_ie = NULL;
> @@ -420,7 +420,7 @@ static void wfx_join(struct wfx_vif *wvif)
>  	wvif->join_in_progress = true;
>  	ret = wfx_hif_join(wvif, conf, wvif->channel, ssid, ssid_len);
>  	if (ret) {
> -		ieee80211_connection_loss(wvif->vif);
> +		ieee80211_connection_loss(wvif_to_vif(wvif));
>  		wfx_reset(wvif);
>  	} else {
>  		/* Due to beacon filtering it is possible that the AP's beacon is not known for the
> @@ -440,7 +440,7 @@ static void wfx_join_finalize(struct wfx_vif *wvif, struct ieee80211_bss_conf *i
>  
>  	rcu_read_lock(); /* protect sta */
>  	if (info->bssid && !info->ibss_joined)
> -		sta = ieee80211_find_sta(wvif->vif, info->bssid);
> +		sta = ieee80211_find_sta(wvif_to_vif(wvif), info->bssid);
>  	if (sta && sta->ht_cap.ht_supported)
>  		ampdu_density = sta->ht_cap.ampdu_density;
>  	if (sta && sta->ht_cap.ht_supported &&
> @@ -565,7 +565,7 @@ static int wfx_update_tim(struct wfx_vif *wvif)
>  	u16 tim_offset, tim_length;
>  	u8 *tim_ptr;
>  
> -	skb = ieee80211_beacon_get_tim(wvif->wdev->hw, wvif->vif, &tim_offset, &tim_length);
> +	skb = ieee80211_beacon_get_tim(wvif->wdev->hw, wvif_to_vif(wvif), &tim_offset, &tim_length);
>  	if (!skb)
>  		return -ENOENT;
>  	tim_ptr = skb->data + tim_offset;
> @@ -707,8 +707,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 +765,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);
>  
> diff --git a/drivers/staging/wfx/wfx.h b/drivers/staging/wfx/wfx.h
> index 6594cc647c2f..78f2a416fe4f 100644
> --- a/drivers/staging/wfx/wfx.h
> +++ b/drivers/staging/wfx/wfx.h
> @@ -25,6 +25,8 @@
>  #define USEC_PER_TXOP 32 /* see struct ieee80211_tx_queue_params */
>  #define USEC_PER_TU 1024
>  
> +#define wvif_to_vif(ptr)(container_of((void *)ptr, struct ieee80211_vif, drv_priv))
> +
>  struct wfx_hwbus_ops;
>  
>  struct wfx_dev {
> @@ -61,7 +63,6 @@ struct wfx_dev {
>  
>  struct wfx_vif {
>  	struct wfx_dev             *wdev;
> -	struct ieee80211_vif       *vif;
>  	struct ieee80211_channel   *channel;
>  	int                        id;
>  
> -- 
> 2.25.1
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] staging: wfx: use container_of() to get vif
  2022-04-07 20:41 [PATCH] staging: wfx: use container_of() to get vif Jaehee Park
  2022-04-07 21:17 ` Jaehee Park
@ 2022-04-08  1:09 ` Alison Schofield
  2022-04-08  1:50   ` Jaehee Park
  2022-04-08  9:49 ` Julia Lawall
  2 siblings, 1 reply; 9+ messages in thread
From: Alison Schofield @ 2022-04-08  1:09 UTC (permalink / raw)
  To: Jaehee Park
  Cc: Jérôme Pouiller, Greg Kroah-Hartman, linux-staging,
	linux-kernel, outreachy, Stefano Brivio

On Thu, Apr 07, 2022 at 04:41:28PM -0400, Jaehee Park wrote:
> Use container_of() to get vif. This improves the code in two ways:
> [1] it speeds up the compilation because container_of() saves steps to
> retrieve vif (the representation of ieee80211_vif), and
> [2] it eliminates the need to define multiple pointers. Previously,
> after setting the pointer to drv_priv, another pointer had to be
> defined by vif in struct wfx_vif to point back to ieee80211_vif.
> Remove vif from the struct wfx_vif, define a macro wvif_to_vif(ptr)
> for container_of(), and replace all wvif->vif with the newly defined
> container_of() construct.
> 
> Signed-off-by: Jaehee Park <jhpark1013@gmail.com>
> ---
> Questions:
> - These changes built for me, but it would be great if someone can test it on the actual hardware.
> - We noticed that the development of the wfx driver is happening in netdev instead of the staging directory. Do you suggest that I use the net-next tree for this change?

Hi Jaehee,

Yes - it looks like you missed staging by just a couple of days with this one.

Perhaps use .gitorderfile to order how the files appear in the diff.
Here, it would be helpful to see the new define first (in wfx.h)
followed by all usages.

Alison

>  
>  drivers/staging/wfx/data_rx.c |  4 ++--
>  drivers/staging/wfx/data_tx.c |  2 +-
>  drivers/staging/wfx/key.c     |  3 +--
>  drivers/staging/wfx/queue.c   |  2 +-
>  drivers/staging/wfx/scan.c    |  6 ++---
>  drivers/staging/wfx/sta.c     | 45 ++++++++++++++++-------------------
>  drivers/staging/wfx/wfx.h     |  3 ++-
>  7 files changed, 31 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/staging/wfx/data_rx.c b/drivers/staging/wfx/data_rx.c
> index a4b5ffe158e4..98c2223089b8 100644
> --- a/drivers/staging/wfx/data_rx.c
> +++ b/drivers/staging/wfx/data_rx.c
> @@ -24,12 +24,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(wvif_to_vif(wvif), 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(wvif_to_vif(wvif), mgmt->sa, tid);
>  		break;
>  	}
>  }
> diff --git a/drivers/staging/wfx/data_tx.c b/drivers/staging/wfx/data_tx.c
> index e07381b2ff4d..fd79e03a08c5 100644
> --- a/drivers/staging/wfx/data_tx.c
> +++ b/drivers/staging/wfx/data_tx.c
> @@ -216,7 +216,7 @@ static u8 wfx_tx_get_link_id(struct wfx_vif *wvif, struct ieee80211_sta *sta,
>  
>  	if (sta_priv && sta_priv->link_id)
>  		return sta_priv->link_id;
> -	if (wvif->vif->type != NL80211_IFTYPE_AP)
> +	if (wvif_to_vif(wvif)->type != NL80211_IFTYPE_AP)
>  		return 0;
>  	if (is_multicast_ether_addr(da))
>  		return 0;
> diff --git a/drivers/staging/wfx/key.c b/drivers/staging/wfx/key.c
> index 8f23e8d42bd4..71b1fca70248 100644
> --- a/drivers/staging/wfx/key.c
> +++ b/drivers/staging/wfx/key.c
> @@ -174,7 +174,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);
> +						 wvif_to_vif(wvif)->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 +224,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/staging/wfx/queue.c b/drivers/staging/wfx/queue.c
> index 729825230db2..a2745d1024c6 100644
> --- a/drivers/staging/wfx/queue.c
> +++ b/drivers/staging/wfx/queue.c
> @@ -207,7 +207,7 @@ bool wfx_tx_queues_has_cab(struct wfx_vif *wvif)
>  {
>  	int i;
>  
> -	if (wvif->vif->type != NL80211_IFTYPE_AP)
> +	if (wvif_to_vif(wvif)->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/staging/wfx/scan.c b/drivers/staging/wfx/scan.c
> index 7f34f0d322f9..5c25fde3fc41 100644
> --- a/drivers/staging/wfx/scan.c
> +++ b/drivers/staging/wfx/scan.c
> @@ -25,7 +25,7 @@ static int update_probe_tmpl(struct wfx_vif *wvif, struct cfg80211_scan_request
>  {
>  	struct sk_buff *skb;
>  
> -	skb = ieee80211_probereq_get(wvif->wdev->hw, wvif->vif->addr, NULL, 0, req->ie_len);
> +	skb = ieee80211_probereq_get(wvif->wdev->hw, wvif_to_vif(wvif)->addr, NULL, 0, req->ie_len);
>  	if (!skb)
>  		return -ENOMEM;
>  
> @@ -75,8 +75,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 != wvif_to_vif(wvif)->bss_conf.txpower)
> +		wfx_hif_set_output_power(wvif, wvif_to_vif(wvif)->bss_conf.txpower);
>  	wfx_tx_unlock(wvif->wdev);
>  	return ret;
>  }
> diff --git a/drivers/staging/wfx/sta.c b/drivers/staging/wfx/sta.c
> index 03025ef7f1be..b634c8e004db 100644
> --- a/drivers/staging/wfx/sta.c
> +++ b/drivers/staging/wfx/sta.c
> @@ -132,7 +132,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 && wvif_to_vif(wvif)->type == NL80211_IFTYPE_AP) {
>  			dev_dbg(wdev->dev, "do not forward probe request in AP mode\n");
>  			*total_flags &= ~FIF_PROBE_REQ;
>  		}
> @@ -153,18 +153,18 @@ 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;
>  
> -	WARN(!wvif->vif->bss_conf.assoc && enable_ps,
> +	WARN(!wvif_to_vif(wvif)->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;
> +		chan0 = wvif_to_vif(wdev_to_wvif(wvif->wdev, 0))->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) {
> +		chan1 = wvif_to_vif(wdev_to_wvif(wvif->wdev, 1))->bss_conf.chandef.chan;
> +	if (chan0 && chan1 && wvif_to_vif(wvif)->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 (wvif_to_vif(wvif)->bss_conf.assoc && wvif_to_vif(wvif)->bss_conf.ps)
>  				dev_info(wvif->wdev->dev, "ignoring requested PS mode");
>  			return -1;
>  		}
> @@ -177,8 +177,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 = wvif_to_vif(wvif)->bss_conf.ps;
> +	if (wvif_to_vif(wvif)->bss_conf.assoc && wvif_to_vif(wvif)->bss_conf.ps)
>  		return conf->dynamic_ps_timeout;
>  	else
>  		return -1;
> @@ -189,7 +189,7 @@ int wfx_update_pm(struct wfx_vif *wvif)
>  	int ps_timeout;
>  	bool ps;
>  
> -	if (!wvif->vif->bss_conf.assoc)
> +	if (!wvif_to_vif(wvif)->bss_conf.assoc)
>  		return 0;
>  	ps_timeout = wfx_get_ps_timeout(wvif, &ps);
>  	if (!ps)
> @@ -215,7 +215,7 @@ 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 (wvif_to_vif(wvif)->type == NL80211_IFTYPE_STATION && old_uapsd != wvif->uapsd_mask) {
>  		wfx_hif_set_uapsd_info(wvif, wvif->uapsd_mask);
>  		wfx_update_pm(wvif);
>  	}
> @@ -242,20 +242,20 @@ void wfx_event_report_rssi(struct wfx_vif *wvif, u8 raw_rcpi_rssi)
>  	int cqm_evt;
>  
>  	rcpi_rssi = raw_rcpi_rssi / 2 - 110;
> -	if (rcpi_rssi <= wvif->vif->bss_conf.cqm_rssi_thold)
> +	if (rcpi_rssi <= wvif_to_vif(wvif)->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(wvif_to_vif(wvif), 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_bss_conf *bss_conf = &wvif_to_vif(wvif)->bss_conf;
>  
> -	ieee80211_beacon_loss(wvif->vif);
> +	ieee80211_beacon_loss(wvif_to_vif(wvif));
>  	schedule_delayed_work(to_delayed_work(work), msecs_to_jiffies(bss_conf->beacon_int));
>  }
>  
> @@ -323,13 +323,13 @@ static int wfx_upload_ap_templates(struct wfx_vif *wvif)
>  {
>  	struct sk_buff *skb;
>  
> -	skb = ieee80211_beacon_get(wvif->wdev->hw, wvif->vif);
> +	skb = ieee80211_beacon_get(wvif->wdev->hw, wvif_to_vif(wvif));
>  	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, wvif_to_vif(wvif));
>  	if (!skb)
>  		return -ENOMEM;
>  	wfx_hif_set_template_frame(wvif, skb, HIF_TMPLT_PRBRES, API_RATE_INDEX_B_1MBPS);
> @@ -339,7 +339,7 @@ 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 sk_buff *skb = ieee80211_beacon_get(wvif->wdev->hw, wvif_to_vif(wvif));
>  	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 +389,7 @@ 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_bss_conf *conf = &wvif_to_vif(wvif)->bss_conf;
>  	struct cfg80211_bss *bss = NULL;
>  	u8 ssid[IEEE80211_MAX_SSID_LEN];
>  	const u8 *ssid_ie = NULL;
> @@ -420,7 +420,7 @@ static void wfx_join(struct wfx_vif *wvif)
>  	wvif->join_in_progress = true;
>  	ret = wfx_hif_join(wvif, conf, wvif->channel, ssid, ssid_len);
>  	if (ret) {
> -		ieee80211_connection_loss(wvif->vif);
> +		ieee80211_connection_loss(wvif_to_vif(wvif));
>  		wfx_reset(wvif);
>  	} else {
>  		/* Due to beacon filtering it is possible that the AP's beacon is not known for the
> @@ -440,7 +440,7 @@ static void wfx_join_finalize(struct wfx_vif *wvif, struct ieee80211_bss_conf *i
>  
>  	rcu_read_lock(); /* protect sta */
>  	if (info->bssid && !info->ibss_joined)
> -		sta = ieee80211_find_sta(wvif->vif, info->bssid);
> +		sta = ieee80211_find_sta(wvif_to_vif(wvif), info->bssid);
>  	if (sta && sta->ht_cap.ht_supported)
>  		ampdu_density = sta->ht_cap.ampdu_density;
>  	if (sta && sta->ht_cap.ht_supported &&
> @@ -565,7 +565,7 @@ static int wfx_update_tim(struct wfx_vif *wvif)
>  	u16 tim_offset, tim_length;
>  	u8 *tim_ptr;
>  
> -	skb = ieee80211_beacon_get_tim(wvif->wdev->hw, wvif->vif, &tim_offset, &tim_length);
> +	skb = ieee80211_beacon_get_tim(wvif->wdev->hw, wvif_to_vif(wvif), &tim_offset, &tim_length);
>  	if (!skb)
>  		return -ENOENT;
>  	tim_ptr = skb->data + tim_offset;
> @@ -707,8 +707,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 +765,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);
>  
> diff --git a/drivers/staging/wfx/wfx.h b/drivers/staging/wfx/wfx.h
> index 6594cc647c2f..78f2a416fe4f 100644
> --- a/drivers/staging/wfx/wfx.h
> +++ b/drivers/staging/wfx/wfx.h
> @@ -25,6 +25,8 @@
>  #define USEC_PER_TXOP 32 /* see struct ieee80211_tx_queue_params */
>  #define USEC_PER_TU 1024
>  
> +#define wvif_to_vif(ptr)(container_of((void *)ptr, struct ieee80211_vif, drv_priv))
> +
>  struct wfx_hwbus_ops;
>  
>  struct wfx_dev {
> @@ -61,7 +63,6 @@ struct wfx_dev {
>  
>  struct wfx_vif {
>  	struct wfx_dev             *wdev;
> -	struct ieee80211_vif       *vif;
>  	struct ieee80211_channel   *channel;
>  	int                        id;
>  
> -- 
> 2.25.1
> 
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] staging: wfx: use container_of() to get vif
  2022-04-08  1:09 ` Alison Schofield
@ 2022-04-08  1:50   ` Jaehee Park
  0 siblings, 0 replies; 9+ messages in thread
From: Jaehee Park @ 2022-04-08  1:50 UTC (permalink / raw)
  To: Alison Schofield
  Cc: Jérôme Pouiller, Greg Kroah-Hartman, linux-staging,
	linux-kernel, outreachy, Stefano Brivio

On Thu, Apr 07, 2022 at 06:09:30PM -0700, Alison Schofield wrote:
> On Thu, Apr 07, 2022 at 04:41:28PM -0400, Jaehee Park wrote:
> > Use container_of() to get vif. This improves the code in two ways:
> > [1] it speeds up the compilation because container_of() saves steps to
> > retrieve vif (the representation of ieee80211_vif), and
> > [2] it eliminates the need to define multiple pointers. Previously,
> > after setting the pointer to drv_priv, another pointer had to be
> > defined by vif in struct wfx_vif to point back to ieee80211_vif.
> > Remove vif from the struct wfx_vif, define a macro wvif_to_vif(ptr)
> > for container_of(), and replace all wvif->vif with the newly defined
> > container_of() construct.
> > 
> > Signed-off-by: Jaehee Park <jhpark1013@gmail.com>
> > ---
> > Questions:
> > - These changes built for me, but it would be great if someone can test it on the actual hardware.
> > - We noticed that the development of the wfx driver is happening in netdev instead of the staging directory. Do you suggest that I use the net-next tree for this change?
> 
> Hi Jaehee,
> 
> Yes - it looks like you missed staging by just a couple of days with this one.
> 
> Perhaps use .gitorderfile to order how the files appear in the diff.
> Here, it would be helpful to see the new define first (in wfx.h)
> followed by all usages.
> 
> Alison
> 

Hi Alison,
I didn't know about .gitorderfile -- thank you for the advice! Seeing the wfx.h change first would make the other changes much easier to comprehend. I'll do a patch revision now.
Thanks!
Jaehee
> >  
> >  drivers/staging/wfx/data_rx.c |  4 ++--
> >  drivers/staging/wfx/data_tx.c |  2 +-
> >  drivers/staging/wfx/key.c     |  3 +--
> >  drivers/staging/wfx/queue.c   |  2 +-
> >  drivers/staging/wfx/scan.c    |  6 ++---
> >  drivers/staging/wfx/sta.c     | 45 ++++++++++++++++-------------------
> >  drivers/staging/wfx/wfx.h     |  3 ++-
> >  7 files changed, 31 insertions(+), 34 deletions(-)
> > 
> > diff --git a/drivers/staging/wfx/data_rx.c b/drivers/staging/wfx/data_rx.c
> > index a4b5ffe158e4..98c2223089b8 100644
> > --- a/drivers/staging/wfx/data_rx.c
> > +++ b/drivers/staging/wfx/data_rx.c
> > @@ -24,12 +24,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(wvif_to_vif(wvif), 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(wvif_to_vif(wvif), mgmt->sa, tid);
> >  		break;
> >  	}
> >  }
> > diff --git a/drivers/staging/wfx/data_tx.c b/drivers/staging/wfx/data_tx.c
> > index e07381b2ff4d..fd79e03a08c5 100644
> > --- a/drivers/staging/wfx/data_tx.c
> > +++ b/drivers/staging/wfx/data_tx.c
> > @@ -216,7 +216,7 @@ static u8 wfx_tx_get_link_id(struct wfx_vif *wvif, struct ieee80211_sta *sta,
> >  
> >  	if (sta_priv && sta_priv->link_id)
> >  		return sta_priv->link_id;
> > -	if (wvif->vif->type != NL80211_IFTYPE_AP)
> > +	if (wvif_to_vif(wvif)->type != NL80211_IFTYPE_AP)
> >  		return 0;
> >  	if (is_multicast_ether_addr(da))
> >  		return 0;
> > diff --git a/drivers/staging/wfx/key.c b/drivers/staging/wfx/key.c
> > index 8f23e8d42bd4..71b1fca70248 100644
> > --- a/drivers/staging/wfx/key.c
> > +++ b/drivers/staging/wfx/key.c
> > @@ -174,7 +174,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);
> > +						 wvif_to_vif(wvif)->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 +224,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/staging/wfx/queue.c b/drivers/staging/wfx/queue.c
> > index 729825230db2..a2745d1024c6 100644
> > --- a/drivers/staging/wfx/queue.c
> > +++ b/drivers/staging/wfx/queue.c
> > @@ -207,7 +207,7 @@ bool wfx_tx_queues_has_cab(struct wfx_vif *wvif)
> >  {
> >  	int i;
> >  
> > -	if (wvif->vif->type != NL80211_IFTYPE_AP)
> > +	if (wvif_to_vif(wvif)->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/staging/wfx/scan.c b/drivers/staging/wfx/scan.c
> > index 7f34f0d322f9..5c25fde3fc41 100644
> > --- a/drivers/staging/wfx/scan.c
> > +++ b/drivers/staging/wfx/scan.c
> > @@ -25,7 +25,7 @@ static int update_probe_tmpl(struct wfx_vif *wvif, struct cfg80211_scan_request
> >  {
> >  	struct sk_buff *skb;
> >  
> > -	skb = ieee80211_probereq_get(wvif->wdev->hw, wvif->vif->addr, NULL, 0, req->ie_len);
> > +	skb = ieee80211_probereq_get(wvif->wdev->hw, wvif_to_vif(wvif)->addr, NULL, 0, req->ie_len);
> >  	if (!skb)
> >  		return -ENOMEM;
> >  
> > @@ -75,8 +75,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 != wvif_to_vif(wvif)->bss_conf.txpower)
> > +		wfx_hif_set_output_power(wvif, wvif_to_vif(wvif)->bss_conf.txpower);
> >  	wfx_tx_unlock(wvif->wdev);
> >  	return ret;
> >  }
> > diff --git a/drivers/staging/wfx/sta.c b/drivers/staging/wfx/sta.c
> > index 03025ef7f1be..b634c8e004db 100644
> > --- a/drivers/staging/wfx/sta.c
> > +++ b/drivers/staging/wfx/sta.c
> > @@ -132,7 +132,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 && wvif_to_vif(wvif)->type == NL80211_IFTYPE_AP) {
> >  			dev_dbg(wdev->dev, "do not forward probe request in AP mode\n");
> >  			*total_flags &= ~FIF_PROBE_REQ;
> >  		}
> > @@ -153,18 +153,18 @@ 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;
> >  
> > -	WARN(!wvif->vif->bss_conf.assoc && enable_ps,
> > +	WARN(!wvif_to_vif(wvif)->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;
> > +		chan0 = wvif_to_vif(wdev_to_wvif(wvif->wdev, 0))->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) {
> > +		chan1 = wvif_to_vif(wdev_to_wvif(wvif->wdev, 1))->bss_conf.chandef.chan;
> > +	if (chan0 && chan1 && wvif_to_vif(wvif)->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 (wvif_to_vif(wvif)->bss_conf.assoc && wvif_to_vif(wvif)->bss_conf.ps)
> >  				dev_info(wvif->wdev->dev, "ignoring requested PS mode");
> >  			return -1;
> >  		}
> > @@ -177,8 +177,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 = wvif_to_vif(wvif)->bss_conf.ps;
> > +	if (wvif_to_vif(wvif)->bss_conf.assoc && wvif_to_vif(wvif)->bss_conf.ps)
> >  		return conf->dynamic_ps_timeout;
> >  	else
> >  		return -1;
> > @@ -189,7 +189,7 @@ int wfx_update_pm(struct wfx_vif *wvif)
> >  	int ps_timeout;
> >  	bool ps;
> >  
> > -	if (!wvif->vif->bss_conf.assoc)
> > +	if (!wvif_to_vif(wvif)->bss_conf.assoc)
> >  		return 0;
> >  	ps_timeout = wfx_get_ps_timeout(wvif, &ps);
> >  	if (!ps)
> > @@ -215,7 +215,7 @@ 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 (wvif_to_vif(wvif)->type == NL80211_IFTYPE_STATION && old_uapsd != wvif->uapsd_mask) {
> >  		wfx_hif_set_uapsd_info(wvif, wvif->uapsd_mask);
> >  		wfx_update_pm(wvif);
> >  	}
> > @@ -242,20 +242,20 @@ void wfx_event_report_rssi(struct wfx_vif *wvif, u8 raw_rcpi_rssi)
> >  	int cqm_evt;
> >  
> >  	rcpi_rssi = raw_rcpi_rssi / 2 - 110;
> > -	if (rcpi_rssi <= wvif->vif->bss_conf.cqm_rssi_thold)
> > +	if (rcpi_rssi <= wvif_to_vif(wvif)->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(wvif_to_vif(wvif), 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_bss_conf *bss_conf = &wvif_to_vif(wvif)->bss_conf;
> >  
> > -	ieee80211_beacon_loss(wvif->vif);
> > +	ieee80211_beacon_loss(wvif_to_vif(wvif));
> >  	schedule_delayed_work(to_delayed_work(work), msecs_to_jiffies(bss_conf->beacon_int));
> >  }
> >  
> > @@ -323,13 +323,13 @@ static int wfx_upload_ap_templates(struct wfx_vif *wvif)
> >  {
> >  	struct sk_buff *skb;
> >  
> > -	skb = ieee80211_beacon_get(wvif->wdev->hw, wvif->vif);
> > +	skb = ieee80211_beacon_get(wvif->wdev->hw, wvif_to_vif(wvif));
> >  	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, wvif_to_vif(wvif));
> >  	if (!skb)
> >  		return -ENOMEM;
> >  	wfx_hif_set_template_frame(wvif, skb, HIF_TMPLT_PRBRES, API_RATE_INDEX_B_1MBPS);
> > @@ -339,7 +339,7 @@ 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 sk_buff *skb = ieee80211_beacon_get(wvif->wdev->hw, wvif_to_vif(wvif));
> >  	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 +389,7 @@ 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_bss_conf *conf = &wvif_to_vif(wvif)->bss_conf;
> >  	struct cfg80211_bss *bss = NULL;
> >  	u8 ssid[IEEE80211_MAX_SSID_LEN];
> >  	const u8 *ssid_ie = NULL;
> > @@ -420,7 +420,7 @@ static void wfx_join(struct wfx_vif *wvif)
> >  	wvif->join_in_progress = true;
> >  	ret = wfx_hif_join(wvif, conf, wvif->channel, ssid, ssid_len);
> >  	if (ret) {
> > -		ieee80211_connection_loss(wvif->vif);
> > +		ieee80211_connection_loss(wvif_to_vif(wvif));
> >  		wfx_reset(wvif);
> >  	} else {
> >  		/* Due to beacon filtering it is possible that the AP's beacon is not known for the
> > @@ -440,7 +440,7 @@ static void wfx_join_finalize(struct wfx_vif *wvif, struct ieee80211_bss_conf *i
> >  
> >  	rcu_read_lock(); /* protect sta */
> >  	if (info->bssid && !info->ibss_joined)
> > -		sta = ieee80211_find_sta(wvif->vif, info->bssid);
> > +		sta = ieee80211_find_sta(wvif_to_vif(wvif), info->bssid);
> >  	if (sta && sta->ht_cap.ht_supported)
> >  		ampdu_density = sta->ht_cap.ampdu_density;
> >  	if (sta && sta->ht_cap.ht_supported &&
> > @@ -565,7 +565,7 @@ static int wfx_update_tim(struct wfx_vif *wvif)
> >  	u16 tim_offset, tim_length;
> >  	u8 *tim_ptr;
> >  
> > -	skb = ieee80211_beacon_get_tim(wvif->wdev->hw, wvif->vif, &tim_offset, &tim_length);
> > +	skb = ieee80211_beacon_get_tim(wvif->wdev->hw, wvif_to_vif(wvif), &tim_offset, &tim_length);
> >  	if (!skb)
> >  		return -ENOENT;
> >  	tim_ptr = skb->data + tim_offset;
> > @@ -707,8 +707,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 +765,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);
> >  
> > diff --git a/drivers/staging/wfx/wfx.h b/drivers/staging/wfx/wfx.h
> > index 6594cc647c2f..78f2a416fe4f 100644
> > --- a/drivers/staging/wfx/wfx.h
> > +++ b/drivers/staging/wfx/wfx.h
> > @@ -25,6 +25,8 @@
> >  #define USEC_PER_TXOP 32 /* see struct ieee80211_tx_queue_params */
> >  #define USEC_PER_TU 1024
> >  
> > +#define wvif_to_vif(ptr)(container_of((void *)ptr, struct ieee80211_vif, drv_priv))
> > +
> >  struct wfx_hwbus_ops;
> >  
> >  struct wfx_dev {
> > @@ -61,7 +63,6 @@ struct wfx_dev {
> >  
> >  struct wfx_vif {
> >  	struct wfx_dev             *wdev;
> > -	struct ieee80211_vif       *vif;
> >  	struct ieee80211_channel   *channel;
> >  	int                        id;
> >  
> > -- 
> > 2.25.1
> > 
> > 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] staging: wfx: use container_of() to get vif
  2022-04-07 20:41 [PATCH] staging: wfx: use container_of() to get vif Jaehee Park
  2022-04-07 21:17 ` Jaehee Park
  2022-04-08  1:09 ` Alison Schofield
@ 2022-04-08  9:49 ` Julia Lawall
  2022-04-08 22:21   ` Jaehee Park
  2 siblings, 1 reply; 9+ messages in thread
From: Julia Lawall @ 2022-04-08  9:49 UTC (permalink / raw)
  To: Jaehee Park
  Cc: Jérôme Pouiller, Greg Kroah-Hartman, linux-staging,
	linux-kernel, outreachy, Stefano Brivio

[-- Attachment #1: Type: text/plain, Size: 13315 bytes --]



On Thu, 7 Apr 2022, Jaehee Park wrote:

> Use container_of() to get vif. This improves the code in two ways:
> [1] it speeds up the compilation because container_of() saves steps to
> retrieve vif (the representation of ieee80211_vif), and

I don't understand this part.

julia

> [2] it eliminates the need to define multiple pointers. Previously,
> after setting the pointer to drv_priv, another pointer had to be
> defined by vif in struct wfx_vif to point back to ieee80211_vif.
> Remove vif from the struct wfx_vif, define a macro wvif_to_vif(ptr)
> for container_of(), and replace all wvif->vif with the newly defined
> container_of() construct.
>
> Signed-off-by: Jaehee Park <jhpark1013@gmail.com>
> ---
> Questions:
> - These changes built for me, but it would be great if someone can test it on the actual hardware.
> - We noticed that the development of the wfx driver is happening in netdev instead of the staging directory. Do you suggest that I use the net-next tree for this change?
>  
>  drivers/staging/wfx/data_rx.c |  4 ++--
>  drivers/staging/wfx/data_tx.c |  2 +-
>  drivers/staging/wfx/key.c     |  3 +--
>  drivers/staging/wfx/queue.c   |  2 +-
>  drivers/staging/wfx/scan.c    |  6 ++---
>  drivers/staging/wfx/sta.c     | 45 ++++++++++++++++-------------------
>  drivers/staging/wfx/wfx.h     |  3 ++-
>  7 files changed, 31 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/staging/wfx/data_rx.c b/drivers/staging/wfx/data_rx.c
> index a4b5ffe158e4..98c2223089b8 100644
> --- a/drivers/staging/wfx/data_rx.c
> +++ b/drivers/staging/wfx/data_rx.c
> @@ -24,12 +24,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(wvif_to_vif(wvif), 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(wvif_to_vif(wvif), mgmt->sa, tid);
>  		break;
>  	}
>  }
> diff --git a/drivers/staging/wfx/data_tx.c b/drivers/staging/wfx/data_tx.c
> index e07381b2ff4d..fd79e03a08c5 100644
> --- a/drivers/staging/wfx/data_tx.c
> +++ b/drivers/staging/wfx/data_tx.c
> @@ -216,7 +216,7 @@ static u8 wfx_tx_get_link_id(struct wfx_vif *wvif, struct ieee80211_sta *sta,
>
>  	if (sta_priv && sta_priv->link_id)
>  		return sta_priv->link_id;
> -	if (wvif->vif->type != NL80211_IFTYPE_AP)
> +	if (wvif_to_vif(wvif)->type != NL80211_IFTYPE_AP)
>  		return 0;
>  	if (is_multicast_ether_addr(da))
>  		return 0;
> diff --git a/drivers/staging/wfx/key.c b/drivers/staging/wfx/key.c
> index 8f23e8d42bd4..71b1fca70248 100644
> --- a/drivers/staging/wfx/key.c
> +++ b/drivers/staging/wfx/key.c
> @@ -174,7 +174,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);
> +						 wvif_to_vif(wvif)->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 +224,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/staging/wfx/queue.c b/drivers/staging/wfx/queue.c
> index 729825230db2..a2745d1024c6 100644
> --- a/drivers/staging/wfx/queue.c
> +++ b/drivers/staging/wfx/queue.c
> @@ -207,7 +207,7 @@ bool wfx_tx_queues_has_cab(struct wfx_vif *wvif)
>  {
>  	int i;
>
> -	if (wvif->vif->type != NL80211_IFTYPE_AP)
> +	if (wvif_to_vif(wvif)->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/staging/wfx/scan.c b/drivers/staging/wfx/scan.c
> index 7f34f0d322f9..5c25fde3fc41 100644
> --- a/drivers/staging/wfx/scan.c
> +++ b/drivers/staging/wfx/scan.c
> @@ -25,7 +25,7 @@ static int update_probe_tmpl(struct wfx_vif *wvif, struct cfg80211_scan_request
>  {
>  	struct sk_buff *skb;
>
> -	skb = ieee80211_probereq_get(wvif->wdev->hw, wvif->vif->addr, NULL, 0, req->ie_len);
> +	skb = ieee80211_probereq_get(wvif->wdev->hw, wvif_to_vif(wvif)->addr, NULL, 0, req->ie_len);
>  	if (!skb)
>  		return -ENOMEM;
>
> @@ -75,8 +75,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 != wvif_to_vif(wvif)->bss_conf.txpower)
> +		wfx_hif_set_output_power(wvif, wvif_to_vif(wvif)->bss_conf.txpower);
>  	wfx_tx_unlock(wvif->wdev);
>  	return ret;
>  }
> diff --git a/drivers/staging/wfx/sta.c b/drivers/staging/wfx/sta.c
> index 03025ef7f1be..b634c8e004db 100644
> --- a/drivers/staging/wfx/sta.c
> +++ b/drivers/staging/wfx/sta.c
> @@ -132,7 +132,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 && wvif_to_vif(wvif)->type == NL80211_IFTYPE_AP) {
>  			dev_dbg(wdev->dev, "do not forward probe request in AP mode\n");
>  			*total_flags &= ~FIF_PROBE_REQ;
>  		}
> @@ -153,18 +153,18 @@ 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;
>
> -	WARN(!wvif->vif->bss_conf.assoc && enable_ps,
> +	WARN(!wvif_to_vif(wvif)->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;
> +		chan0 = wvif_to_vif(wdev_to_wvif(wvif->wdev, 0))->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) {
> +		chan1 = wvif_to_vif(wdev_to_wvif(wvif->wdev, 1))->bss_conf.chandef.chan;
> +	if (chan0 && chan1 && wvif_to_vif(wvif)->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 (wvif_to_vif(wvif)->bss_conf.assoc && wvif_to_vif(wvif)->bss_conf.ps)
>  				dev_info(wvif->wdev->dev, "ignoring requested PS mode");
>  			return -1;
>  		}
> @@ -177,8 +177,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 = wvif_to_vif(wvif)->bss_conf.ps;
> +	if (wvif_to_vif(wvif)->bss_conf.assoc && wvif_to_vif(wvif)->bss_conf.ps)
>  		return conf->dynamic_ps_timeout;
>  	else
>  		return -1;
> @@ -189,7 +189,7 @@ int wfx_update_pm(struct wfx_vif *wvif)
>  	int ps_timeout;
>  	bool ps;
>
> -	if (!wvif->vif->bss_conf.assoc)
> +	if (!wvif_to_vif(wvif)->bss_conf.assoc)
>  		return 0;
>  	ps_timeout = wfx_get_ps_timeout(wvif, &ps);
>  	if (!ps)
> @@ -215,7 +215,7 @@ 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 (wvif_to_vif(wvif)->type == NL80211_IFTYPE_STATION && old_uapsd != wvif->uapsd_mask) {
>  		wfx_hif_set_uapsd_info(wvif, wvif->uapsd_mask);
>  		wfx_update_pm(wvif);
>  	}
> @@ -242,20 +242,20 @@ void wfx_event_report_rssi(struct wfx_vif *wvif, u8 raw_rcpi_rssi)
>  	int cqm_evt;
>
>  	rcpi_rssi = raw_rcpi_rssi / 2 - 110;
> -	if (rcpi_rssi <= wvif->vif->bss_conf.cqm_rssi_thold)
> +	if (rcpi_rssi <= wvif_to_vif(wvif)->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(wvif_to_vif(wvif), 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_bss_conf *bss_conf = &wvif_to_vif(wvif)->bss_conf;
>
> -	ieee80211_beacon_loss(wvif->vif);
> +	ieee80211_beacon_loss(wvif_to_vif(wvif));
>  	schedule_delayed_work(to_delayed_work(work), msecs_to_jiffies(bss_conf->beacon_int));
>  }
>
> @@ -323,13 +323,13 @@ static int wfx_upload_ap_templates(struct wfx_vif *wvif)
>  {
>  	struct sk_buff *skb;
>
> -	skb = ieee80211_beacon_get(wvif->wdev->hw, wvif->vif);
> +	skb = ieee80211_beacon_get(wvif->wdev->hw, wvif_to_vif(wvif));
>  	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, wvif_to_vif(wvif));
>  	if (!skb)
>  		return -ENOMEM;
>  	wfx_hif_set_template_frame(wvif, skb, HIF_TMPLT_PRBRES, API_RATE_INDEX_B_1MBPS);
> @@ -339,7 +339,7 @@ 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 sk_buff *skb = ieee80211_beacon_get(wvif->wdev->hw, wvif_to_vif(wvif));
>  	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 +389,7 @@ 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_bss_conf *conf = &wvif_to_vif(wvif)->bss_conf;
>  	struct cfg80211_bss *bss = NULL;
>  	u8 ssid[IEEE80211_MAX_SSID_LEN];
>  	const u8 *ssid_ie = NULL;
> @@ -420,7 +420,7 @@ static void wfx_join(struct wfx_vif *wvif)
>  	wvif->join_in_progress = true;
>  	ret = wfx_hif_join(wvif, conf, wvif->channel, ssid, ssid_len);
>  	if (ret) {
> -		ieee80211_connection_loss(wvif->vif);
> +		ieee80211_connection_loss(wvif_to_vif(wvif));
>  		wfx_reset(wvif);
>  	} else {
>  		/* Due to beacon filtering it is possible that the AP's beacon is not known for the
> @@ -440,7 +440,7 @@ static void wfx_join_finalize(struct wfx_vif *wvif, struct ieee80211_bss_conf *i
>
>  	rcu_read_lock(); /* protect sta */
>  	if (info->bssid && !info->ibss_joined)
> -		sta = ieee80211_find_sta(wvif->vif, info->bssid);
> +		sta = ieee80211_find_sta(wvif_to_vif(wvif), info->bssid);
>  	if (sta && sta->ht_cap.ht_supported)
>  		ampdu_density = sta->ht_cap.ampdu_density;
>  	if (sta && sta->ht_cap.ht_supported &&
> @@ -565,7 +565,7 @@ static int wfx_update_tim(struct wfx_vif *wvif)
>  	u16 tim_offset, tim_length;
>  	u8 *tim_ptr;
>
> -	skb = ieee80211_beacon_get_tim(wvif->wdev->hw, wvif->vif, &tim_offset, &tim_length);
> +	skb = ieee80211_beacon_get_tim(wvif->wdev->hw, wvif_to_vif(wvif), &tim_offset, &tim_length);
>  	if (!skb)
>  		return -ENOENT;
>  	tim_ptr = skb->data + tim_offset;
> @@ -707,8 +707,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 +765,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);
>
> diff --git a/drivers/staging/wfx/wfx.h b/drivers/staging/wfx/wfx.h
> index 6594cc647c2f..78f2a416fe4f 100644
> --- a/drivers/staging/wfx/wfx.h
> +++ b/drivers/staging/wfx/wfx.h
> @@ -25,6 +25,8 @@
>  #define USEC_PER_TXOP 32 /* see struct ieee80211_tx_queue_params */
>  #define USEC_PER_TU 1024
>
> +#define wvif_to_vif(ptr)(container_of((void *)ptr, struct ieee80211_vif, drv_priv))
> +
>  struct wfx_hwbus_ops;
>
>  struct wfx_dev {
> @@ -61,7 +63,6 @@ struct wfx_dev {
>
>  struct wfx_vif {
>  	struct wfx_dev             *wdev;
> -	struct ieee80211_vif       *vif;
>  	struct ieee80211_channel   *channel;
>  	int                        id;
>
> --
> 2.25.1
>
>
>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] staging: wfx: use container_of() to get vif
  2022-04-08  9:49 ` Julia Lawall
@ 2022-04-08 22:21   ` Jaehee Park
  2022-04-09 13:54     ` Jérôme Pouiller
  0 siblings, 1 reply; 9+ messages in thread
From: Jaehee Park @ 2022-04-08 22:21 UTC (permalink / raw)
  To: Julia Lawall
  Cc: Jérôme Pouiller, Greg Kroah-Hartman, linux-staging,
	linux-kernel, outreachy, Stefano Brivio

On Fri, Apr 08, 2022 at 11:49:49AM +0200, Julia Lawall wrote:
> 
> 
> On Thu, 7 Apr 2022, Jaehee Park wrote:
> 
> > Use container_of() to get vif. This improves the code in two ways:
> > [1] it speeds up the compilation because container_of() saves steps to
> > retrieve vif (the representation of ieee80211_vif), and
> 
> I don't understand this part.
> 
> julia
> 

Thank you for your message. I revised the commit message (below)-- 
I hope it makes it clearer. I'm open to suggestions as well!

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 macro 
wvif_to_vif(wvif) for container_of(), and replace all wvif->vif with 
the newly defined container_of construct.

Sorry if I'm putting out more information than was asked for:
Diagram is worth a thousand words; this one is made by Stefano when 
he was explaining the container_of() construct:

.-----------------------------------.
|  .------------------------------. |
'->| struct ieee80211_vif         | |
   |------------------------------| |
   | 1                            | |
   | 2                            | |
   |  .--------------------------.| |
   | 3| struct wfx_vif drv_priv   | |
   |  |---------------------------| |
   |  | ...                       | |
   |  |                           | |
   |  | struct ieee80211_vif *vif---'
   '------------------------------'
In this example, the offset is 3.
What I want to convey here is the compiler already knows the offset
of drv_priv within struct ieee80211_vif by using the container_of() 
construct. So, it doesn't need to do extra work.

> > [2] it eliminates the need to define multiple pointers. Previously,
> > after setting the pointer to drv_priv, another pointer had to be
> > defined by vif in struct wfx_vif to point back to ieee80211_vif.
> > Remove vif from the struct wfx_vif, define a macro wvif_to_vif(ptr)
> > for container_of(), and replace all wvif->vif with the newly defined
> > container_of() construct.
> >
> > Signed-off-by: Jaehee Park <jhpark1013@gmail.com>
> > ---
> > Questions:
> > - These changes built for me, but it would be great if someone can test it on the actual hardware.
> > - We noticed that the development of the wfx driver is happening in netdev instead of the staging directory. Do you suggest that I use the net-next tree for this change?
> >  
> >  drivers/staging/wfx/data_rx.c |  4 ++--
> >  drivers/staging/wfx/data_tx.c |  2 +-
> >  drivers/staging/wfx/key.c     |  3 +--
> >  drivers/staging/wfx/queue.c   |  2 +-
> >  drivers/staging/wfx/scan.c    |  6 ++---
> >  drivers/staging/wfx/sta.c     | 45 ++++++++++++++++-------------------
> >  drivers/staging/wfx/wfx.h     |  3 ++-
> >  7 files changed, 31 insertions(+), 34 deletions(-)
> >
> > diff --git a/drivers/staging/wfx/data_rx.c b/drivers/staging/wfx/data_rx.c
> > index a4b5ffe158e4..98c2223089b8 100644
> > --- a/drivers/staging/wfx/data_rx.c
> > +++ b/drivers/staging/wfx/data_rx.c
> > @@ -24,12 +24,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(wvif_to_vif(wvif), 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(wvif_to_vif(wvif), mgmt->sa, tid);
> >  		break;
> >  	}
> >  }
> > diff --git a/drivers/staging/wfx/data_tx.c b/drivers/staging/wfx/data_tx.c
> > index e07381b2ff4d..fd79e03a08c5 100644
> > --- a/drivers/staging/wfx/data_tx.c
> > +++ b/drivers/staging/wfx/data_tx.c
> > @@ -216,7 +216,7 @@ static u8 wfx_tx_get_link_id(struct wfx_vif *wvif, struct ieee80211_sta *sta,
> >
> >  	if (sta_priv && sta_priv->link_id)
> >  		return sta_priv->link_id;
> > -	if (wvif->vif->type != NL80211_IFTYPE_AP)
> > +	if (wvif_to_vif(wvif)->type != NL80211_IFTYPE_AP)
> >  		return 0;
> >  	if (is_multicast_ether_addr(da))
> >  		return 0;
> > diff --git a/drivers/staging/wfx/key.c b/drivers/staging/wfx/key.c
> > index 8f23e8d42bd4..71b1fca70248 100644
> > --- a/drivers/staging/wfx/key.c
> > +++ b/drivers/staging/wfx/key.c
> > @@ -174,7 +174,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);
> > +						 wvif_to_vif(wvif)->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 +224,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/staging/wfx/queue.c b/drivers/staging/wfx/queue.c
> > index 729825230db2..a2745d1024c6 100644
> > --- a/drivers/staging/wfx/queue.c
> > +++ b/drivers/staging/wfx/queue.c
> > @@ -207,7 +207,7 @@ bool wfx_tx_queues_has_cab(struct wfx_vif *wvif)
> >  {
> >  	int i;
> >
> > -	if (wvif->vif->type != NL80211_IFTYPE_AP)
> > +	if (wvif_to_vif(wvif)->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/staging/wfx/scan.c b/drivers/staging/wfx/scan.c
> > index 7f34f0d322f9..5c25fde3fc41 100644
> > --- a/drivers/staging/wfx/scan.c
> > +++ b/drivers/staging/wfx/scan.c
> > @@ -25,7 +25,7 @@ static int update_probe_tmpl(struct wfx_vif *wvif, struct cfg80211_scan_request
> >  {
> >  	struct sk_buff *skb;
> >
> > -	skb = ieee80211_probereq_get(wvif->wdev->hw, wvif->vif->addr, NULL, 0, req->ie_len);
> > +	skb = ieee80211_probereq_get(wvif->wdev->hw, wvif_to_vif(wvif)->addr, NULL, 0, req->ie_len);
> >  	if (!skb)
> >  		return -ENOMEM;
> >
> > @@ -75,8 +75,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 != wvif_to_vif(wvif)->bss_conf.txpower)
> > +		wfx_hif_set_output_power(wvif, wvif_to_vif(wvif)->bss_conf.txpower);
> >  	wfx_tx_unlock(wvif->wdev);
> >  	return ret;
> >  }
> > diff --git a/drivers/staging/wfx/sta.c b/drivers/staging/wfx/sta.c
> > index 03025ef7f1be..b634c8e004db 100644
> > --- a/drivers/staging/wfx/sta.c
> > +++ b/drivers/staging/wfx/sta.c
> > @@ -132,7 +132,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 && wvif_to_vif(wvif)->type == NL80211_IFTYPE_AP) {
> >  			dev_dbg(wdev->dev, "do not forward probe request in AP mode\n");
> >  			*total_flags &= ~FIF_PROBE_REQ;
> >  		}
> > @@ -153,18 +153,18 @@ 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;
> >
> > -	WARN(!wvif->vif->bss_conf.assoc && enable_ps,
> > +	WARN(!wvif_to_vif(wvif)->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;
> > +		chan0 = wvif_to_vif(wdev_to_wvif(wvif->wdev, 0))->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) {
> > +		chan1 = wvif_to_vif(wdev_to_wvif(wvif->wdev, 1))->bss_conf.chandef.chan;
> > +	if (chan0 && chan1 && wvif_to_vif(wvif)->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 (wvif_to_vif(wvif)->bss_conf.assoc && wvif_to_vif(wvif)->bss_conf.ps)
> >  				dev_info(wvif->wdev->dev, "ignoring requested PS mode");
> >  			return -1;
> >  		}
> > @@ -177,8 +177,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 = wvif_to_vif(wvif)->bss_conf.ps;
> > +	if (wvif_to_vif(wvif)->bss_conf.assoc && wvif_to_vif(wvif)->bss_conf.ps)
> >  		return conf->dynamic_ps_timeout;
> >  	else
> >  		return -1;
> > @@ -189,7 +189,7 @@ int wfx_update_pm(struct wfx_vif *wvif)
> >  	int ps_timeout;
> >  	bool ps;
> >
> > -	if (!wvif->vif->bss_conf.assoc)
> > +	if (!wvif_to_vif(wvif)->bss_conf.assoc)
> >  		return 0;
> >  	ps_timeout = wfx_get_ps_timeout(wvif, &ps);
> >  	if (!ps)
> > @@ -215,7 +215,7 @@ 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 (wvif_to_vif(wvif)->type == NL80211_IFTYPE_STATION && old_uapsd != wvif->uapsd_mask) {
> >  		wfx_hif_set_uapsd_info(wvif, wvif->uapsd_mask);
> >  		wfx_update_pm(wvif);
> >  	}
> > @@ -242,20 +242,20 @@ void wfx_event_report_rssi(struct wfx_vif *wvif, u8 raw_rcpi_rssi)
> >  	int cqm_evt;
> >
> >  	rcpi_rssi = raw_rcpi_rssi / 2 - 110;
> > -	if (rcpi_rssi <= wvif->vif->bss_conf.cqm_rssi_thold)
> > +	if (rcpi_rssi <= wvif_to_vif(wvif)->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(wvif_to_vif(wvif), 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_bss_conf *bss_conf = &wvif_to_vif(wvif)->bss_conf;
> >
> > -	ieee80211_beacon_loss(wvif->vif);
> > +	ieee80211_beacon_loss(wvif_to_vif(wvif));
> >  	schedule_delayed_work(to_delayed_work(work), msecs_to_jiffies(bss_conf->beacon_int));
> >  }
> >
> > @@ -323,13 +323,13 @@ static int wfx_upload_ap_templates(struct wfx_vif *wvif)
> >  {
> >  	struct sk_buff *skb;
> >
> > -	skb = ieee80211_beacon_get(wvif->wdev->hw, wvif->vif);
> > +	skb = ieee80211_beacon_get(wvif->wdev->hw, wvif_to_vif(wvif));
> >  	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, wvif_to_vif(wvif));
> >  	if (!skb)
> >  		return -ENOMEM;
> >  	wfx_hif_set_template_frame(wvif, skb, HIF_TMPLT_PRBRES, API_RATE_INDEX_B_1MBPS);
> > @@ -339,7 +339,7 @@ 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 sk_buff *skb = ieee80211_beacon_get(wvif->wdev->hw, wvif_to_vif(wvif));
> >  	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 +389,7 @@ 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_bss_conf *conf = &wvif_to_vif(wvif)->bss_conf;
> >  	struct cfg80211_bss *bss = NULL;
> >  	u8 ssid[IEEE80211_MAX_SSID_LEN];
> >  	const u8 *ssid_ie = NULL;
> > @@ -420,7 +420,7 @@ static void wfx_join(struct wfx_vif *wvif)
> >  	wvif->join_in_progress = true;
> >  	ret = wfx_hif_join(wvif, conf, wvif->channel, ssid, ssid_len);
> >  	if (ret) {
> > -		ieee80211_connection_loss(wvif->vif);
> > +		ieee80211_connection_loss(wvif_to_vif(wvif));
> >  		wfx_reset(wvif);
> >  	} else {
> >  		/* Due to beacon filtering it is possible that the AP's beacon is not known for the
> > @@ -440,7 +440,7 @@ static void wfx_join_finalize(struct wfx_vif *wvif, struct ieee80211_bss_conf *i
> >
> >  	rcu_read_lock(); /* protect sta */
> >  	if (info->bssid && !info->ibss_joined)
> > -		sta = ieee80211_find_sta(wvif->vif, info->bssid);
> > +		sta = ieee80211_find_sta(wvif_to_vif(wvif), info->bssid);
> >  	if (sta && sta->ht_cap.ht_supported)
> >  		ampdu_density = sta->ht_cap.ampdu_density;
> >  	if (sta && sta->ht_cap.ht_supported &&
> > @@ -565,7 +565,7 @@ static int wfx_update_tim(struct wfx_vif *wvif)
> >  	u16 tim_offset, tim_length;
> >  	u8 *tim_ptr;
> >
> > -	skb = ieee80211_beacon_get_tim(wvif->wdev->hw, wvif->vif, &tim_offset, &tim_length);
> > +	skb = ieee80211_beacon_get_tim(wvif->wdev->hw, wvif_to_vif(wvif), &tim_offset, &tim_length);
> >  	if (!skb)
> >  		return -ENOENT;
> >  	tim_ptr = skb->data + tim_offset;
> > @@ -707,8 +707,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 +765,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);
> >
> > diff --git a/drivers/staging/wfx/wfx.h b/drivers/staging/wfx/wfx.h
> > index 6594cc647c2f..78f2a416fe4f 100644
> > --- a/drivers/staging/wfx/wfx.h
> > +++ b/drivers/staging/wfx/wfx.h
> > @@ -25,6 +25,8 @@
> >  #define USEC_PER_TXOP 32 /* see struct ieee80211_tx_queue_params */
> >  #define USEC_PER_TU 1024
> >
> > +#define wvif_to_vif(ptr)(container_of((void *)ptr, struct ieee80211_vif, drv_priv))
> > +
> >  struct wfx_hwbus_ops;
> >
> >  struct wfx_dev {
> > @@ -61,7 +63,6 @@ struct wfx_dev {
> >
> >  struct wfx_vif {
> >  	struct wfx_dev             *wdev;
> > -	struct ieee80211_vif       *vif;
> >  	struct ieee80211_channel   *channel;
> >  	int                        id;
> >
> > --
> > 2.25.1
> >
> >
> >


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] staging: wfx: use container_of() to get vif
  2022-04-08 22:21   ` Jaehee Park
@ 2022-04-09 13:54     ` Jérôme Pouiller
  2022-04-09 15:37       ` Julia Lawall
  0 siblings, 1 reply; 9+ messages in thread
From: Jérôme Pouiller @ 2022-04-09 13:54 UTC (permalink / raw)
  To: Julia Lawall, Jaehee Park
  Cc: Greg Kroah-Hartman, linux-staging, linux-kernel, outreachy,
	Stefano Brivio

On Saturday 9 April 2022 00:21:02 CEST Jaehee Park wrote:
> On Fri, Apr 08, 2022 at 11:49:49AM +0200, Julia Lawall wrote:
> > On Thu, 7 Apr 2022, Jaehee Park wrote:
> >
> > > Use container_of() to get vif. This improves the code in two ways:
> > > [1] it speeds up the compilation because container_of() saves steps to
> > > retrieve vif (the representation of ieee80211_vif), and
> >
> > I don't understand this part.
> >
> > julia
> >
> 
> Thank you for your message. I revised the commit message (below)--
> I hope it makes it clearer. I'm open to suggestions as well!
> 
> 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 macro
> wvif_to_vif(wvif) for container_of(), and replace all wvif->vif with
> the newly defined container_of construct.
> 
> Sorry if I'm putting out more information than was asked for:
> Diagram is worth a thousand words; this one is made by Stefano when
> he was explaining the container_of() construct:
> 
> .-----------------------------------.
> |  .------------------------------. |
> '->| struct ieee80211_vif         | |
>    |------------------------------| |
>    | 1                            | |
>    | 2                            | |
>    |  .--------------------------.| |
>    | 3| struct wfx_vif drv_priv   | |
>    |  |---------------------------| |
>    |  | ...                       | |
>    |  |                           | |
>    |  | struct ieee80211_vif *vif---'
>    '------------------------------'
> In this example, the offset is 3.
> What I want to convey here is the compiler already knows the offset
> of drv_priv within struct ieee80211_vif by using the container_of()
> construct. So, it doesn't need to do extra work.

Wow, nice ascii art :)

I think that Julia has not understand why you said it "speeds up the
compilation". I think you meant "speeds up the execution", don't you?



-- 
Jérôme Pouiller



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] staging: wfx: use container_of() to get vif
  2022-04-09 13:54     ` Jérôme Pouiller
@ 2022-04-09 15:37       ` Julia Lawall
  2022-04-12  4:01         ` Jaehee Park
  0 siblings, 1 reply; 9+ messages in thread
From: Julia Lawall @ 2022-04-09 15:37 UTC (permalink / raw)
  To: Jérôme Pouiller
  Cc: Jaehee Park, Greg Kroah-Hartman, linux-staging, linux-kernel,
	outreachy, Stefano Brivio

[-- Attachment #1: Type: text/plain, Size: 2589 bytes --]



On Sat, 9 Apr 2022, Jérôme Pouiller wrote:

> On Saturday 9 April 2022 00:21:02 CEST Jaehee Park wrote:
> > On Fri, Apr 08, 2022 at 11:49:49AM +0200, Julia Lawall wrote:
> > > On Thu, 7 Apr 2022, Jaehee Park wrote:
> > >
> > > > Use container_of() to get vif. This improves the code in two ways:
> > > > [1] it speeds up the compilation because container_of() saves steps to
> > > > retrieve vif (the representation of ieee80211_vif), and
> > >
> > > I don't understand this part.
> > >
> > > julia
> > >
> >
> > Thank you for your message. I revised the commit message (below)--
> > I hope it makes it clearer. I'm open to suggestions as well!
> >
> > 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 macro
> > wvif_to_vif(wvif) for container_of(), and replace all wvif->vif with
> > the newly defined container_of construct.
> >
> > Sorry if I'm putting out more information than was asked for:
> > Diagram is worth a thousand words; this one is made by Stefano when
> > he was explaining the container_of() construct:
> >
> > .-----------------------------------.
> > |  .------------------------------. |
> > '->| struct ieee80211_vif         | |
> >    |------------------------------| |
> >    | 1                            | |
> >    | 2                            | |
> >    |  .--------------------------.| |
> >    | 3| struct wfx_vif drv_priv   | |
> >    |  |---------------------------| |
> >    |  | ...                       | |
> >    |  |                           | |
> >    |  | struct ieee80211_vif *vif---'
> >    '------------------------------'
> > In this example, the offset is 3.
> > What I want to convey here is the compiler already knows the offset
> > of drv_priv within struct ieee80211_vif by using the container_of()
> > construct. So, it doesn't need to do extra work.
>
> Wow, nice ascii art :)
>
> I think that Julia has not understand why you said it "speeds up the
> compilation". I think you meant "speeds up the execution", don't you?

Yes, that was my concern.

julia

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] staging: wfx: use container_of() to get vif
  2022-04-09 15:37       ` Julia Lawall
@ 2022-04-12  4:01         ` Jaehee Park
  0 siblings, 0 replies; 9+ messages in thread
From: Jaehee Park @ 2022-04-12  4:01 UTC (permalink / raw)
  To: Julia Lawall
  Cc: Jérôme Pouiller, Greg Kroah-Hartman, linux-staging,
	linux-kernel, outreachy, Stefano Brivio

On Sat, Apr 09, 2022 at 05:37:32PM +0200, Julia Lawall wrote:
> 
> 
> On Sat, 9 Apr 2022, Jérôme Pouiller wrote:
> 
> > On Saturday 9 April 2022 00:21:02 CEST Jaehee Park wrote:
> > > On Fri, Apr 08, 2022 at 11:49:49AM +0200, Julia Lawall wrote:
> > > > On Thu, 7 Apr 2022, Jaehee Park wrote:
> > > >
> > > > > Use container_of() to get vif. This improves the code in two ways:
> > > > > [1] it speeds up the compilation because container_of() saves steps to
> > > > > retrieve vif (the representation of ieee80211_vif), and
> > > >
> > > > I don't understand this part.
> > > >
> > > > julia
> > > >
> > >
> > > Thank you for your message. I revised the commit message (below)--
> > > I hope it makes it clearer. I'm open to suggestions as well!
> > >
> > > 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 macro
> > > wvif_to_vif(wvif) for container_of(), and replace all wvif->vif with
> > > the newly defined container_of construct.
> > >
> > > Sorry if I'm putting out more information than was asked for:
> > > Diagram is worth a thousand words; this one is made by Stefano when
> > > he was explaining the container_of() construct:
> > >
> > > .-----------------------------------.
> > > |  .------------------------------. |
> > > '->| struct ieee80211_vif         | |
> > >    |------------------------------| |
> > >    | 1                            | |
> > >    | 2                            | |
> > >    |  .--------------------------.| |
> > >    | 3| struct wfx_vif drv_priv   | |
> > >    |  |---------------------------| |
> > >    |  | ...                       | |
> > >    |  |                           | |
> > >    |  | struct ieee80211_vif *vif---'
> > >    '------------------------------'
> > > In this example, the offset is 3.
> > > What I want to convey here is the compiler already knows the offset
> > > of drv_priv within struct ieee80211_vif by using the container_of()
> > > construct. So, it doesn't need to do extra work.
> >
> > Wow, nice ascii art :)
> >
> > I think that Julia has not understand why you said it "speeds up the
> > compilation". I think you meant "speeds up the execution", don't you?
> 
> Yes, that was my concern.
> 
> julia

Understood-- Thank you Julia and Jérôme for catching my mistake and clarifying! 

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2022-04-12  4:02 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-07 20:41 [PATCH] staging: wfx: use container_of() to get vif Jaehee Park
2022-04-07 21:17 ` Jaehee Park
2022-04-08  1:09 ` Alison Schofield
2022-04-08  1:50   ` Jaehee Park
2022-04-08  9:49 ` Julia Lawall
2022-04-08 22:21   ` Jaehee Park
2022-04-09 13:54     ` Jérôme Pouiller
2022-04-09 15:37       ` Julia Lawall
2022-04-12  4:01         ` Jaehee Park

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).