2022-03-07 15:27:31

by Lorenzo Bianconi

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

>
> Firmware initialization can take a while. Move mt7921_init_hw routine in
> a dedicated work in order to not slow down bootstrap process.
>
> Signed-off-by: Lorenzo Bianconi <[email protected]>

Hi Sean and Deren,

I tested this patch on mt7921e and mt7921u. Can you double check if it
works fine even on mt7921s? Thanks.

Regards,
Lorenzo

> ---
> .../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
>


2022-03-07 20:39:50

by Deren Wu

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

On Mon, 2022-03-07 at 13:00 +0100, Lorenzo Bianconi wrote:
> >
> > Firmware initialization can take a while. Move mt7921_init_hw
> > routine in
> > a dedicated work in order to not slow down bootstrap process.
> >
> > Signed-off-by: Lorenzo Bianconi <[email protected]>
>
> Hi Sean and Deren,
>
> I tested this patch on mt7921e and mt7921u. Can you double check if
> it
> works fine even on mt7921s? Thanks.
>
> Regards,
> Lorenzo

Hi Lore,

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.

I think the patch is still good now and we may need another patch to
cover the stress problem.

Tested-by: Deren Wu <[email protected]>


>
> > ---
> > .../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
> >
>
>

2022-03-07 20:40:08

by Lorenzo Bianconi

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

> On Mon, 2022-03-07 at 13:00 +0100, Lorenzo Bianconi wrote:
> > >
> > > Firmware initialization can take a while. Move mt7921_init_hw
> > > routine in
> > > a dedicated work in order to not slow down bootstrap process.
> > >
> > > Signed-off-by: Lorenzo Bianconi <[email protected]>
> >
> > Hi Sean and Deren,
> >
> > I tested this patch on mt7921e and mt7921u. Can you double check if
> > it
> > works fine even on mt7921s? Thanks.
> >
> > Regards,
> > Lorenzo
>
> Hi Lore,

Hi Deren,

>
> 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.
>
> I think the patch is still good now and we may need another patch to
> cover the stress problem.
>
> Tested-by: Deren Wu <[email protected]>

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

Regards,
Lorenzo

>
>
> >
> > > ---
> > > .../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
> > >
> >
> >
>


Attachments:
(No filename) (6.23 kB)
signature.asc (235.00 B)
Download all attachments