Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 7 Mar 2022 23:47:51 +0100
From:   Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
To:     sean.wang@mediatek.com
Cc:     nbd@nbd.name, Deren.Wu@mediatek.com,
        linux-wireless@vger.kernel.org, linux-mediatek@lists.infradead.org
Subject: Re: [PATCH] mt76: mt7921: move mt7921_init_hw in a dedicated work
Message-ID: <YiaLl++oijKrByBA@lore-desk>
References: <0358e31e71481dec87f7ecb3b040db53076ffc4a.1646654230.git.lorenzo@kernel.org>
 <1646689596-21189-1-git-send-email-sean.wang@mediatek.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="G4rL/TNDZ89nVV1r"
Content-Disposition: inline
In-Reply-To: <1646689596-21189-1-git-send-email-sean.wang@mediatek.com>
Precedence: bulk


--G4rL/TNDZ89nVV1r
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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

Hi Sean,

>=20
> I don't think the patch is really needed and it creates the different sta=
te of the driver after mt7921_*_probe between without and with the patch
> we should be careful to handle it.
>=20
> For example.
>=20
> 1) It is possible that ieee80211_ops mt7921_ops is working while mt7921_i=
nit_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 mac80=
211
till init_work completes since we run mt76_register_device() in
mt7921_init_work().

>=20
> 2) mt7921[k,s,e].ko are always successful probed ( the .ko are always sho=
wn 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 visi=
ble
to the user.

>=20
> so I would prefer to wait a while in mt7921_*_proble until the hardware i=
s ready to be working to get rid of the extra synchronization to be added a=
s 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

>=20
>         Sean
>=20
> >
> >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 =3D 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 =3D container_of(work, struct mt7921_dev,
> >+					      init_work);
> >+	int ret;
> >+
> >+	ret =3D mt7921_init_hardware(dev);
> >+	if (ret)
> >+		return;
> >+
> >+	mt76_set_stream_caps(&dev->mphy, true);
> >+	mt7921_set_stream_he_caps(&dev->phy);
> >+
> >+	ret =3D 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 =3D mt7921_init_debugfs(dev);
> >+	if (ret) {
> >+		dev_err(dev->mt76.dev, "debugfs register failed\n");
> >+		goto error;
> >+	}
> >+
> >+	ret =3D mt76_connac_mcu_set_deep_sleep(&dev->mt76, dev->pm.ds_enable);
> >+	if (ret)
> >+		goto error;
> >+
> >+	dev->hw_init_done =3D true;
> >+	return;
> >+error:
> >+	mt76_unregister_device(&dev->mt76);
> >+}
> >+
> > int mt7921_register_device(struct mt7921_dev *dev)  {
> >	struct ieee80211_hw *hw =3D mt76_hw(dev); @@ -222,6 +265,7 @@ int mt792=
1_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 =3D MT7921_PM_TIMEOUT;
> >	dev->pm.stats.last_wake_event =3D jiffies; @@ -234,7 +278,7 @@ int mt79=
21_register_device(struct mt7921_dev *dev)
> >	if (mt76_is_sdio(&dev->mt76))
> >		hw->extra_tx_headroom +=3D MT_SDIO_TXD_SIZE + MT_SDIO_HDR_SIZE;
> >
> >-	ret =3D mt7921_init_hardware(dev);
> >+	ret =3D 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 =3D dev->mphy.chainmask;
> >	dev->mphy.hw->wiphy->available_antennas_tx =3D dev->mphy.chainmask;
> >
> >-	mt76_set_stream_caps(&dev->mphy, true);
> >-	mt7921_set_stream_he_caps(&dev->phy);
> >-
> >-	ret =3D mt76_register_device(&dev->mt76, true, mt76_rates,
> >-				   ARRAY_SIZE(mt76_rates));
> >-	if (ret)
> >-		return ret;
> >-
> >-	ret =3D mt7921_init_debugfs(dev);
> >-	if (ret)
> >-		return ret;
> >-
> >-	ret =3D mt76_connac_mcu_set_deep_sleep(&dev->mt76, dev->pm.ds_enable);
> >-	if (ret)
> >-		return ret;
> >-
> >-	dev->hw_init_done =3D true;
> >+	queue_work(system_wq, &dev->init_work);
> >
> >	return 0;
> > }
> >diff --git a/drivers/net/wireless/mediatek/mt76/mt7921/mt7921.h b/driver=
s/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
> >
> >
> >
>=20

--G4rL/TNDZ89nVV1r
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iHUEABYKAB0WIQTquNwa3Txd3rGGn7Y6cBh0uS2trAUCYiaLlwAKCRA6cBh0uS2t
rINRAPwMEqqJSqZrKkBMHnZCGKdl0qG2LgtbR+2tHRFZb/NW+gEA5LiyDV3v8BRa
zfjJ/6daLh/EcA8bJGgw2xEvs6FUBQw=
=aVQc
-----END PGP SIGNATURE-----

--G4rL/TNDZ89nVV1r--