All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] mt76: mt7921: move mt7921_init_hw in a dedicated work
@ 2022-03-07 11:58 Lorenzo Bianconi
  2022-03-07 12:00 ` Lorenzo Bianconi
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Lorenzo Bianconi @ 2022-03-07 11:58 UTC (permalink / raw)
  To: nbd; +Cc: linux-wireless, lorenzo.bianconi, sean.wang, deren.wu

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


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] mt76: mt7921: move mt7921_init_hw in a dedicated work
  2022-03-07 11:58 [PATCH] mt76: mt7921: move mt7921_init_hw in a dedicated work Lorenzo Bianconi
@ 2022-03-07 12:00 ` Lorenzo Bianconi
  2022-03-07 13:35   ` Deren Wu
  2022-03-07 21:46   ` sean.wang
  2022-03-13 12:53 ` Lorenzo Bianconi
  2 siblings, 1 reply; 11+ messages in thread
From: Lorenzo Bianconi @ 2022-03-07 12:00 UTC (permalink / raw)
  To: Sean Wang, Deren Wu (武德仁)
  Cc: Felix Fietkau, linux-wireless

>
> 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 <lorenzo@kernel.org>

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
>


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] mt76: mt7921: move mt7921_init_hw in a dedicated work
  2022-03-07 12:00 ` Lorenzo Bianconi
@ 2022-03-07 13:35   ` Deren Wu
  2022-03-07 14:27     ` Lorenzo Bianconi
  0 siblings, 1 reply; 11+ messages in thread
From: Deren Wu @ 2022-03-07 13:35 UTC (permalink / raw)
  To: Lorenzo Bianconi, Sean Wang; +Cc: Felix Fietkau, linux-wireless

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 <lorenzo@kernel.org>
> 
> 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 <deren.wu@mediatek.com>


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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] mt76: mt7921: move mt7921_init_hw in a dedicated work
  2022-03-07 13:35   ` Deren Wu
@ 2022-03-07 14:27     ` Lorenzo Bianconi
  0 siblings, 0 replies; 11+ messages in thread
From: Lorenzo Bianconi @ 2022-03-07 14:27 UTC (permalink / raw)
  To: Deren Wu; +Cc: Sean Wang, Felix Fietkau, linux-wireless

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

> 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 <lorenzo@kernel.org>
> > 
> > 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 <deren.wu@mediatek.com>

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

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] mt76: mt7921: move mt7921_init_hw in a dedicated work
  2022-03-07 11:58 [PATCH] mt76: mt7921: move mt7921_init_hw in a dedicated work Lorenzo Bianconi
@ 2022-03-07 21:46   ` sean.wang
  2022-03-07 21:46   ` sean.wang
  2022-03-13 12:53 ` Lorenzo Bianconi
  2 siblings, 0 replies; 11+ messages in thread
From: sean.wang @ 2022-03-07 21:46 UTC (permalink / raw)
  To: nbd, lorenzo.bianconi; +Cc: Deren.Wu, linux-wireless, linux-mediatek, Sean Wang

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

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

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

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.

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] mt76: mt7921: move mt7921_init_hw in a dedicated work
@ 2022-03-07 21:46   ` sean.wang
  0 siblings, 0 replies; 11+ messages in thread
From: sean.wang @ 2022-03-07 21:46 UTC (permalink / raw)
  To: nbd, lorenzo.bianconi; +Cc: Deren.Wu, linux-wireless, linux-mediatek, Sean Wang

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

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

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

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.

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

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] mt76: mt7921: move mt7921_init_hw in a dedicated work
  2022-03-07 21:46   ` sean.wang
@ 2022-03-07 22:47     ` Lorenzo Bianconi
  -1 siblings, 0 replies; 11+ messages in thread
From: Lorenzo Bianconi @ 2022-03-07 22:47 UTC (permalink / raw)
  To: sean.wang; +Cc: nbd, Deren.Wu, linux-wireless, linux-mediatek

[-- 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 --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] mt76: mt7921: move mt7921_init_hw in a dedicated work
@ 2022-03-07 22:47     ` Lorenzo Bianconi
  0 siblings, 0 replies; 11+ messages in thread
From: Lorenzo Bianconi @ 2022-03-07 22:47 UTC (permalink / raw)
  To: sean.wang; +Cc: nbd, Deren.Wu, linux-wireless, linux-mediatek


[-- Attachment #1.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 #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 170 bytes --]

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] mt76: mt7921: move mt7921_init_hw in a dedicated work
  2022-03-07 22:47     ` Lorenzo Bianconi
@ 2022-03-08  2:27       ` sean.wang
  -1 siblings, 0 replies; 11+ messages in thread
From: sean.wang @ 2022-03-08  2:27 UTC (permalink / raw)
  To: nbd, lorenzo.bianconi; +Cc: Deren.Wu, linux-wireless, linux-mediatek, Sean Wang

From: Sean Wang <sean.wang@mediatek.com>

>> 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().

No, It is just my guess. I was not fully sure if there is definitely no racing issue after init work is created. If that never happens, it would be good news to me

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

Another question I had is Is there any problem with rmmod the module with mt7921_init_work failure? It seemed to me that mt76_unregister_device might be called twice in the condition.

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

It is good to see we can shorten the time in the module probe. I was just worried whether we have good handling in any case.

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] mt76: mt7921: move mt7921_init_hw in a dedicated work
@ 2022-03-08  2:27       ` sean.wang
  0 siblings, 0 replies; 11+ messages in thread
From: sean.wang @ 2022-03-08  2:27 UTC (permalink / raw)
  To: nbd, lorenzo.bianconi; +Cc: Deren.Wu, linux-wireless, linux-mediatek, Sean Wang

From: Sean Wang <sean.wang@mediatek.com>

>> 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().

No, It is just my guess. I was not fully sure if there is definitely no racing issue after init work is created. If that never happens, it would be good news to me

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

Another question I had is Is there any problem with rmmod the module with mt7921_init_work failure? It seemed to me that mt76_unregister_device might be called twice in the condition.

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

It is good to see we can shorten the time in the module probe. I was just worried whether we have good handling in any case.

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

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] mt76: mt7921: move mt7921_init_hw in a dedicated work
  2022-03-07 11:58 [PATCH] mt76: mt7921: move mt7921_init_hw in a dedicated work Lorenzo Bianconi
  2022-03-07 12:00 ` Lorenzo Bianconi
  2022-03-07 21:46   ` sean.wang
@ 2022-03-13 12:53 ` Lorenzo Bianconi
  2 siblings, 0 replies; 11+ messages in thread
From: Lorenzo Bianconi @ 2022-03-13 12:53 UTC (permalink / raw)
  To: nbd; +Cc: linux-wireless, lorenzo.bianconi, sean.wang, deren.wu

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

> 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 <lorenzo@kernel.org>

Hi Felix,

let's put this patch on hold for the moment and continue the upstream
discussion. 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
> 

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2022-03-13 12:53 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-07 11:58 [PATCH] mt76: mt7921: move mt7921_init_hw in a dedicated work Lorenzo Bianconi
2022-03-07 12:00 ` Lorenzo Bianconi
2022-03-07 13:35   ` Deren Wu
2022-03-07 14:27     ` Lorenzo Bianconi
2022-03-07 21:46 ` sean.wang
2022-03-07 21:46   ` sean.wang
2022-03-07 22:47   ` Lorenzo Bianconi
2022-03-07 22:47     ` Lorenzo Bianconi
2022-03-08  2:27     ` sean.wang
2022-03-08  2:27       ` sean.wang
2022-03-13 12:53 ` Lorenzo Bianconi

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.