* Re: [PATCH] mt76: mt7921: move mt7921_init_hw in a dedicated work [not found] <0358e31e71481dec87f7ecb3b040db53076ffc4a.1646654230.git.lorenzo@kernel.org> @ 2022-03-07 21:46 ` sean.wang 2022-03-07 22:47 ` Lorenzo Bianconi 0 siblings, 1 reply; 3+ 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] 3+ messages in thread
* Re: [PATCH] mt76: mt7921: move mt7921_init_hw in a dedicated work 2022-03-07 21:46 ` [PATCH] mt76: mt7921: move mt7921_init_hw in a dedicated work sean.wang @ 2022-03-07 22:47 ` Lorenzo Bianconi 2022-03-08 2:27 ` sean.wang 0 siblings, 1 reply; 3+ 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] 3+ 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 0 siblings, 0 replies; 3+ 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] 3+ messages in thread
end of thread, other threads:[~2022-03-08 2:30 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <0358e31e71481dec87f7ecb3b040db53076ffc4a.1646654230.git.lorenzo@kernel.org>
2022-03-07 21:46 ` [PATCH] mt76: mt7921: move mt7921_init_hw in a dedicated work sean.wang
2022-03-07 22:47 ` Lorenzo Bianconi
2022-03-08 2:27 ` sean.wang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox