| Message ID | 0358e31e71481dec87f7ecb3b040db53076ffc4a.1646654230.git.lorenzo@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Felix Fietkau |
| Headers | show |
| Series | mt76: mt7921: move mt7921_init_hw in a dedicated work | expand |
> > 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 >
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 > > > >
> 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 > > > > > > > >
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 > > >
> 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 > > > > > > >
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 >> > >> > >> > >>
> 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 >
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;
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(-)