Re: [PATCH] mt76: mt7921: move mt7921_init_hw in a dedicated work

[Lorenzo Bianconi <lorenzo.bianconi@redhat.com>]

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

> From: Sean Wang <sean.wang@mediatek.com>
> 
> >Firmware initialization can take a while. Move mt7921_init_hw routine in a dedicated work in order to not slow down bootstrap process.
> 
> Hi, Lore

Hi Sean,

> 
> I don't think the patch is really needed and it creates the different state of the driver after mt7921_*_probe between without and with the patch
> we should be careful to handle it.
> 
> For example.
> 
> 1) It is possible that ieee80211_ops mt7921_ops is working while mt7921_init_work is not completed, so that creates the race issue between ieee80211_ops mt7921_ops and mt7921_init_work still in progress

Can you please elaborate on this? The device will not be "visible" to mac80211
till init_work completes since we run mt76_register_device() in
mt7921_init_work().

> 
> 2) mt7921[k,s,e].ko are always successful probed ( the .ko are always shown in `lsmod` ) that would confuse the users even when we actually got the failure of hardware initialization in mt7921_init_work

If mt7921_init_work fails, we will remove the wiphy, so it will not be visible
to the user.

> 
> so I would prefer to wait a while in mt7921_*_proble until the hardware is ready to be working to get rid of the extra synchronization to be added as well as keep the driver much simple.

In the current codebase the time needed for device probing is quite visible on
usb (afaiu this time is needed for fw initialization).

Regards,
Lorenzo

> 
>         Sean
> 
> >
> >Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> >---
> > .../net/wireless/mediatek/mt76/mt7921/init.c  | 66 +++++++++++++------
> > .../wireless/mediatek/mt76/mt7921/mt7921.h    |  2 +
> > 2 files changed, 49 insertions(+), 19 deletions(-)
> >
> >diff --git a/drivers/net/wireless/mediatek/mt76/mt7921/init.c b/drivers/net/wireless/mediatek/mt76/mt7921/init.c
> >index fa6af85bba7b..332af886b95a 100644
> >--- a/drivers/net/wireless/mediatek/mt76/mt7921/init.c
> >+++ b/drivers/net/wireless/mediatek/mt76/mt7921/init.c
> >@@ -165,7 +165,7 @@ static int __mt7921_init_hardware(struct mt7921_dev *dev)
> >
> > static int mt7921_init_hardware(struct mt7921_dev *dev)  {
> >-	int ret, idx, i;
> >+	int ret, i;
> >
> >	set_bit(MT76_STATE_INITIALIZED, &dev->mphy.state);
> >
> >@@ -182,6 +182,13 @@ static int mt7921_init_hardware(struct mt7921_dev *dev)
> >		return ret;
> >	}
> >
> >+	return 0;
> >+}
> >+
> >+static int mt7921_init_wcid(struct mt7921_dev *dev) {
> >+	int idx;
> >+
> >	/* Beacon and mgmt frames should occupy wcid 0 */
> >	idx = mt76_wcid_alloc(dev->mt76.wcid_mask, MT7921_WTBL_STA - 1);
> >	if (idx)
> >@@ -195,6 +202,42 @@ static int mt7921_init_hardware(struct mt7921_dev *dev)
> >	return 0;
> > }
> >
> >+static void mt7921_init_work(struct work_struct *work) {
> >+	struct mt7921_dev *dev = container_of(work, struct mt7921_dev,
> >+					      init_work);
> >+	int ret;
> >+
> >+	ret = mt7921_init_hardware(dev);
> >+	if (ret)
> >+		return;
> >+
> >+	mt76_set_stream_caps(&dev->mphy, true);
> >+	mt7921_set_stream_he_caps(&dev->phy);
> >+
> >+	ret = mt76_register_device(&dev->mt76, true, mt76_rates,
> >+				   ARRAY_SIZE(mt76_rates));
> >+	if (ret) {
> >+		dev_err(dev->mt76.dev, "register device failed\n");
> >+		return;
> >+	}
> >+
> >+	ret = mt7921_init_debugfs(dev);
> >+	if (ret) {
> >+		dev_err(dev->mt76.dev, "debugfs register failed\n");
> >+		goto error;
> >+	}
> >+
> >+	ret = mt76_connac_mcu_set_deep_sleep(&dev->mt76, dev->pm.ds_enable);
> >+	if (ret)
> >+		goto error;
> >+
> >+	dev->hw_init_done = true;
> >+	return;
> >+error:
> >+	mt76_unregister_device(&dev->mt76);
> >+}
> >+
> > int mt7921_register_device(struct mt7921_dev *dev)  {
> >	struct ieee80211_hw *hw = mt76_hw(dev); @@ -222,6 +265,7 @@ int mt7921_register_device(struct mt7921_dev *dev)
> >	spin_lock_init(&dev->sta_poll_lock);
> >
> >	INIT_WORK(&dev->reset_work, mt7921_mac_reset_work);
> >+	INIT_WORK(&dev->init_work, mt7921_init_work);
> >
> >	dev->pm.idle_timeout = MT7921_PM_TIMEOUT;
> >	dev->pm.stats.last_wake_event = jiffies; @@ -234,7 +278,7 @@ int mt7921_register_device(struct mt7921_dev *dev)
> >	if (mt76_is_sdio(&dev->mt76))
> >		hw->extra_tx_headroom += MT_SDIO_TXD_SIZE + MT_SDIO_HDR_SIZE;
> >
> >-	ret = mt7921_init_hardware(dev);
> >+	ret = mt7921_init_wcid(dev);
> >	if (ret)
> >		return ret;
> >
> >@@ -262,23 +306,7 @@ int mt7921_register_device(struct mt7921_dev *dev)
> >	dev->mphy.hw->wiphy->available_antennas_rx = dev->mphy.chainmask;
> >	dev->mphy.hw->wiphy->available_antennas_tx = dev->mphy.chainmask;
> >
> >-	mt76_set_stream_caps(&dev->mphy, true);
> >-	mt7921_set_stream_he_caps(&dev->phy);
> >-
> >-	ret = mt76_register_device(&dev->mt76, true, mt76_rates,
> >-				   ARRAY_SIZE(mt76_rates));
> >-	if (ret)
> >-		return ret;
> >-
> >-	ret = mt7921_init_debugfs(dev);
> >-	if (ret)
> >-		return ret;
> >-
> >-	ret = mt76_connac_mcu_set_deep_sleep(&dev->mt76, dev->pm.ds_enable);
> >-	if (ret)
> >-		return ret;
> >-
> >-	dev->hw_init_done = true;
> >+	queue_work(system_wq, &dev->init_work);
> >
> >	return 0;
> > }
> >diff --git a/drivers/net/wireless/mediatek/mt76/mt7921/mt7921.h b/drivers/net/wireless/mediatek/mt76/mt7921/mt7921.h
> >index 394a677140da..b6c8f84acb64 100644
> >--- a/drivers/net/wireless/mediatek/mt76/mt7921/mt7921.h
> >+++ b/drivers/net/wireless/mediatek/mt76/mt7921/mt7921.h
> >@@ -204,6 +204,8 @@ struct mt7921_dev {
> >	struct list_head sta_poll_list;
> >	spinlock_t sta_poll_lock;
> >
> >+	struct work_struct init_work;
> >+
> >	u8 fw_debug;
> >
> >	struct mt76_connac_pm pm;
> >--
> >2.35.1
> >
> >
> >
> 

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