[PATCH] wifi: mt76: mt7921: introduce mt7921_get_mac80211_ops utility routine

[PATCH] wifi: mt76: mt7921: introduce mt7921_get_mac80211_ops utility routine

[Lorenzo Bianconi]

Since the fw offload capability check is shared between pci,usb and sdio
devices, move it in common init code and reduce code duplication.

Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 .../net/wireless/mediatek/mt76/mt7921/init.c  | 29 +++++++++++++++++--
 .../wireless/mediatek/mt76/mt7921/mt7921.h    |  3 +-
 .../net/wireless/mediatek/mt76/mt7921/pci.c   | 19 ++----------
 .../net/wireless/mediatek/mt76/mt7921/sdio.c  | 20 ++-----------
 .../net/wireless/mediatek/mt76/mt7921/usb.c   | 19 ++----------
 5 files changed, 35 insertions(+), 55 deletions(-)

diff --git a/drivers/net/wireless/mediatek/mt76/mt7921/init.c b/drivers/net/wireless/mediatek/mt76/mt7921/init.c
index 80c71acfe159..33681fbb2fe8 100644
--- a/drivers/net/wireless/mediatek/mt76/mt7921/init.c
+++ b/drivers/net/wireless/mediatek/mt76/mt7921/init.c
@@ -169,7 +169,8 @@ mt7921_mac_init_band(struct mt7921_dev *dev, u8 band)
 	mt76_rmw(dev, MT_WTBLOFF_TOP_RSCR(band), mask, set);
 }
 
-u8 mt7921_check_offload_capability(struct device *dev, const char *fw_wm)
+static u8
+mt7921_get_offload_capability(struct device *dev, const char *fw_wm)
 {
 	struct mt7921_fw_features *features = NULL;
 	const struct mt76_connac2_fw_trailer *hdr;
@@ -220,7 +221,31 @@ u8 mt7921_check_offload_capability(struct device *dev, const char *fw_wm)
 
 	return features ? features->data : 0;
 }
-EXPORT_SYMBOL_GPL(mt7921_check_offload_capability);
+
+struct ieee80211_ops *
+mt7921_get_mac80211_ops(struct device *dev, void *drv_data, u8 *fw_features)
+{
+	struct ieee80211_ops *ops;
+
+	ops = devm_kmemdup(dev, &mt7921_ops, sizeof(mt7921_ops), GFP_KERNEL);
+	if (!ops)
+		return NULL;
+
+	*fw_features = mt7921_get_offload_capability(dev, drv_data);
+	if (!(*fw_features & MT7921_FW_CAP_CNM)) {
+		ops->remain_on_channel = NULL;
+		ops->cancel_remain_on_channel = NULL;
+		ops->add_chanctx = NULL;
+		ops->remove_chanctx = NULL;
+		ops->change_chanctx = NULL;
+		ops->assign_vif_chanctx = NULL;
+		ops->unassign_vif_chanctx = NULL;
+		ops->mgd_prepare_tx = NULL;
+		ops->mgd_complete_tx = NULL;
+	}
+	return ops;
+}
+EXPORT_SYMBOL_GPL(mt7921_get_mac80211_ops);
 
 int mt7921_mac_init(struct mt7921_dev *dev)
 {
diff --git a/drivers/net/wireless/mediatek/mt76/mt7921/mt7921.h b/drivers/net/wireless/mediatek/mt76/mt7921/mt7921.h
index 1af70dac723b..b1dceeafa54e 100644
--- a/drivers/net/wireless/mediatek/mt76/mt7921/mt7921.h
+++ b/drivers/net/wireless/mediatek/mt76/mt7921/mt7921.h
@@ -593,5 +593,6 @@ int mt7921_mcu_set_roc(struct mt7921_phy *phy, struct mt7921_vif *vif,
 		       enum mt7921_roc_req type, u8 token_id);
 int mt7921_mcu_abort_roc(struct mt7921_phy *phy, struct mt7921_vif *vif,
 			 u8 token_id);
-u8 mt7921_check_offload_capability(struct device *dev, const char *fw_wm);
+struct ieee80211_ops *mt7921_get_mac80211_ops(struct device *dev,
+					      void *drv_data, u8 *fw_features);
 #endif
diff --git a/drivers/net/wireless/mediatek/mt76/mt7921/pci.c b/drivers/net/wireless/mediatek/mt76/mt7921/pci.c
index 1a8a54a46dcc..b520cd258170 100644
--- a/drivers/net/wireless/mediatek/mt76/mt7921/pci.c
+++ b/drivers/net/wireless/mediatek/mt76/mt7921/pci.c
@@ -256,7 +256,6 @@ static int mt7921_pci_probe(struct pci_dev *pdev,
 		.drv_own = mt7921e_mcu_drv_pmctrl,
 		.fw_own = mt7921e_mcu_fw_pmctrl,
 	};
-
 	struct ieee80211_ops *ops;
 	struct mt76_bus_ops *bus_ops;
 	struct mt7921_dev *dev;
@@ -285,27 +284,13 @@ static int mt7921_pci_probe(struct pci_dev *pdev,
 	if (mt7921_disable_aspm)
 		mt76_pci_disable_aspm(pdev);
 
-	features = mt7921_check_offload_capability(&pdev->dev, (const char *)
-						   id->driver_data);
-	ops = devm_kmemdup(&pdev->dev, &mt7921_ops, sizeof(mt7921_ops),
-			   GFP_KERNEL);
+	ops = mt7921_get_mac80211_ops(&pdev->dev, (void *)id->driver_data,
+				      &features);
 	if (!ops) {
 		ret = -ENOMEM;
 		goto err_free_pci_vec;
 	}
 
-	if (!(features & MT7921_FW_CAP_CNM)) {
-		ops->remain_on_channel = NULL;
-		ops->cancel_remain_on_channel = NULL;
-		ops->add_chanctx = NULL;
-		ops->remove_chanctx = NULL;
-		ops->change_chanctx = NULL;
-		ops->assign_vif_chanctx = NULL;
-		ops->unassign_vif_chanctx = NULL;
-		ops->mgd_prepare_tx = NULL;
-		ops->mgd_complete_tx = NULL;
-	}
-
 	mdev = mt76_alloc_device(&pdev->dev, sizeof(*dev), ops, &drv_ops);
 	if (!mdev) {
 		ret = -ENOMEM;
diff --git a/drivers/net/wireless/mediatek/mt76/mt7921/sdio.c b/drivers/net/wireless/mediatek/mt76/mt7921/sdio.c
index 8ce4252b8ae7..584921fb25b5 100644
--- a/drivers/net/wireless/mediatek/mt76/mt7921/sdio.c
+++ b/drivers/net/wireless/mediatek/mt76/mt7921/sdio.c
@@ -122,33 +122,17 @@ static int mt7921s_probe(struct sdio_func *func,
 		.drv_own = mt7921s_mcu_drv_pmctrl,
 		.fw_own = mt7921s_mcu_fw_pmctrl,
 	};
-
 	struct ieee80211_ops *ops;
 	struct mt7921_dev *dev;
 	struct mt76_dev *mdev;
 	u8 features;
 	int ret;
 
-	features = mt7921_check_offload_capability(&func->dev, (const char *)
-						   id->driver_data);
-
-	ops = devm_kmemdup(&func->dev, &mt7921_ops, sizeof(mt7921_ops),
-			   GFP_KERNEL);
+	ops = mt7921_get_mac80211_ops(&func->dev, (void *)id->driver_data,
+				      &features);
 	if (!ops)
 		return -ENOMEM;
 
-	if (!(features & MT7921_FW_CAP_CNM)) {
-		ops->remain_on_channel = NULL;
-		ops->cancel_remain_on_channel = NULL;
-		ops->add_chanctx = NULL;
-		ops->remove_chanctx = NULL;
-		ops->change_chanctx = NULL;
-		ops->assign_vif_chanctx = NULL;
-		ops->unassign_vif_chanctx = NULL;
-		ops->mgd_prepare_tx = NULL;
-		ops->mgd_complete_tx = NULL;
-	}
-
 	mdev = mt76_alloc_device(&func->dev, sizeof(*dev), ops, &drv_ops);
 	if (!mdev)
 		return -ENOMEM;
diff --git a/drivers/net/wireless/mediatek/mt76/mt7921/usb.c b/drivers/net/wireless/mediatek/mt76/mt7921/usb.c
index 8fef09ed29c9..376bf89a70d8 100644
--- a/drivers/net/wireless/mediatek/mt76/mt7921/usb.c
+++ b/drivers/net/wireless/mediatek/mt76/mt7921/usb.c
@@ -210,27 +210,12 @@ static int mt7921u_probe(struct usb_interface *usb_intf,
 	u8 features;
 	int ret;
 
-	features = mt7921_check_offload_capability(&usb_intf->dev, (const char *)
-						   id->driver_info);
-	ops = devm_kmemdup(&usb_intf->dev, &mt7921_ops, sizeof(mt7921_ops),
-			   GFP_KERNEL);
+	ops = mt7921_get_mac80211_ops(&usb_intf->dev, (void *)id->driver_info,
+				      &features);
 	if (!ops)
 		return -ENOMEM;
 
-	if (!(features & MT7921_FW_CAP_CNM)) {
-		ops->remain_on_channel = NULL;
-		ops->cancel_remain_on_channel = NULL;
-		ops->add_chanctx = NULL;
-		ops->remove_chanctx = NULL;
-		ops->change_chanctx = NULL;
-		ops->assign_vif_chanctx = NULL;
-		ops->unassign_vif_chanctx = NULL;
-		ops->mgd_prepare_tx = NULL;
-		ops->mgd_complete_tx = NULL;
-	}
-
 	ops->stop = mt7921u_stop;
-
 	mdev = mt76_alloc_device(&usb_intf->dev, sizeof(*dev), ops, &drv_ops);
 	if (!mdev)
 		return -ENOMEM;
-- 
2.39.2

Re: [PATCH] wifi: mt76: mt7921: introduce mt7921_get_mac80211_ops utility routine

[Johannes Berg]

On Mon, 2023-03-06 at 18:50 +0100, Lorenzo Bianconi wrote:
> 
> +struct ieee80211_ops *
> +mt7921_get_mac80211_ops(struct device *dev, void *drv_data, u8 *fw_features)
> +{
> +	struct ieee80211_ops *ops;
> +
> +	ops = devm_kmemdup(dev, &mt7921_ops, sizeof(mt7921_ops), GFP_KERNEL);

It's kind of nice to have static const ops so they can't be a target for
overwriting and function pointer injection, maybe just declare two
copies with a macro or so, with and without chanctx, and return the
appropriate one? It won't even use more memory unless you never run a
device w/o chanctx ops.

johannes

Re: [PATCH] wifi: mt76: mt7921: introduce mt7921_get_mac80211_ops utility routine

[Lorenzo Bianconi]

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

> On Mon, 2023-03-06 at 18:50 +0100, Lorenzo Bianconi wrote:
> > 
> > +struct ieee80211_ops *
> > +mt7921_get_mac80211_ops(struct device *dev, void *drv_data, u8 *fw_features)
> > +{
> > +	struct ieee80211_ops *ops;
> > +
> > +	ops = devm_kmemdup(dev, &mt7921_ops, sizeof(mt7921_ops), GFP_KERNEL);
> 
> It's kind of nice to have static const ops so they can't be a target for
> overwriting and function pointer injection, maybe just declare two
> copies with a macro or so, with and without chanctx, and return the
> appropriate one? It won't even use more memory unless you never run a
> device w/o chanctx ops.

In the current status quo I think it is fine but it does not scale if in the
future we want to add some new fw features that require changing ieee80211_ops
pointer. Honestly I do not know if it is something we should consider nowadays.
What do you think?

Regards,
Lorenzo

> 
> johannes
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]