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 15:27:56 +0100
From:   Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
To:     Deren Wu <deren.wu@mediatek.com>
Cc:     Sean Wang <sean.wang@mediatek.com>, Felix Fietkau <nbd@nbd.name>,
        linux-wireless <linux-wireless@vger.kernel.org>
Subject: Re: [PATCH] mt76: mt7921: move mt7921_init_hw in a dedicated work
Message-ID: <YiYWbFLhV7+Sw9zM@lore-desk>
References: <0358e31e71481dec87f7ecb3b040db53076ffc4a.1646654230.git.lorenzo@kernel.org>
 <CAJ0CqmWQxORW8+++DVW3ujYccfCq_DB5hNOMn+gMs8kgoTk=5w@mail.gmail.com>
 <700fc094dbca7406326dde1b169c2d2fe6a1bb24.camel@mediatek.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="bfAh3h8oI8IIWJTO"
Content-Disposition: inline
In-Reply-To: <700fc094dbca7406326dde1b169c2d2fe6a1bb24.camel@mediatek.com>
Precedence: bulk


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

> On Mon, 2022-03-07 at 13:00 +0100, Lorenzo Bianconi wrote:
> > >=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
> > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> >=20
> > Hi Sean and Deren,
> >=20
> > I tested this patch on mt7921e and mt7921u. Can you double check if
> > it
> > works fine even on mt7921s? Thanks.
> >=20
> > Regards,
> > Lorenzo
>=20
> Hi Lore,

Hi Deren,

>=20
> The patch is good on normal initial flow with mt7921s. However, if I
> run command "insmod mt7921s && rmmod mt7921s" quickly, there would be
> some problems in the remove process. The problem can be avoided if
> adding "cancel_work_sync(&dev->init_work)" in unregiser hook.
>=20
> I think the patch is still good now and we may need another patch to
> cover the stress problem.
>=20
> Tested-by: Deren Wu <deren.wu@mediatek.com>

ack, I will include it in v2 as soon as the other pending patches are appli=
ed.
Thanks.

Regards,
Lorenzo

>=20
>=20
> >=20
> > > ---
> > >  .../net/wireless/mediatek/mt76/mt7921/init.c  | 66 +++++++++++++
> > > ------
> > >  .../wireless/mediatek/mt76/mt7921/mt7921.h    |  2 +
> > >  2 files changed, 49 insertions(+), 19 deletions(-)
> > >=20
> > > 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)
> > >=20
> > >  static int mt7921_init_hardware(struct mt7921_dev *dev)
> > >  {
> > > -       int ret, idx, i;
> > > +       int ret, i;
> > >=20
> > >         set_bit(MT76_STATE_INITIALIZED, &dev->mphy.state);
> > >=20
> > > @@ -182,6 +182,13 @@ static int mt7921_init_hardware(struct
> > > mt7921_dev *dev)
> > >                 return ret;
> > >         }
> > >=20
> > > +       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;
> > >  }
> > >=20
> > > +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 mt7921_register_device(struct mt7921_dev
> > > *dev)
> > >         spin_lock_init(&dev->sta_poll_lock);
> > >=20
> > >         INIT_WORK(&dev->reset_work, mt7921_mac_reset_work);
> > > +       INIT_WORK(&dev->init_work, mt7921_init_work);
> > >=20
> > >         dev->pm.idle_timeout =3D MT7921_PM_TIMEOUT;
> > >         dev->pm.stats.last_wake_event =3D jiffies;
> > > @@ -234,7 +278,7 @@ int mt7921_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;
> > >=20
> > > -       ret =3D mt7921_init_hardware(dev);
> > > +       ret =3D mt7921_init_wcid(dev);
> > >         if (ret)
> > >                 return ret;
> > >=20
> > > @@ -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;
> > >=20
> > > -       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);
> > >=20
> > >         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;
> > >=20
> > > +       struct work_struct init_work;
> > > +
> > >         u8 fw_debug;
> > >=20
> > >         struct mt76_connac_pm pm;
> > > --
> > > 2.35.1
> > >=20
> >=20
> >=20
>=20

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

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

iHUEABYKAB0WIQTquNwa3Txd3rGGn7Y6cBh0uS2trAUCYiYWawAKCRA6cBh0uS2t
rDt/AQDGyBzMm0iYd8AaE4VnyZlhjz3tXsGR4zVFO5s1kIcK9gD/bBIJ8SN9yqwA
kQe+AfJCkXdWOZkDoPyufNevqLkGUAE=
=yglT
-----END PGP SIGNATURE-----

--bfAh3h8oI8IIWJTO--