From: Wen Gong <wgong@codeaurora.org> To: Kalle Valo <kvalo@codeaurora.org> Cc: ath10k@lists.infradead.org, linux-wireless@vger.kernel.org Subject: Re: [PATCH v4 1/2] ath10k: add refcount for ath10k_core_restart Date: Wed, 19 Aug 2020 20:01:40 +0800 [thread overview] Message-ID: <35a24f0d901be76266f6c9ab020d2acc@codeaurora.org> (raw) In-Reply-To: <87imdlkuw8.fsf@codeaurora.org> On 2020-08-15 01:19, Kalle Valo wrote: ... > > I have been thinking a different approach for this. I think another > option is to have a function like this: > > ath10k_core_firmware_crashed() > { > queue_work(ar->workqueue, &ar->restart_work); > } > > In patch 1 we would convert all existing callers to call that > function instead of queue_work() directly. > > In patch 2 we would add a new flag to enum ath10k_dev_flags, or maybe > should actually use existing ATH10K_FLAG_CRASH_FLUSH? Don't know yet > which one is better. Now the function would do: > > ath10k_core_firmware_crashed() > { > if (test_bit(flag)) > return > > set_bit(flag) > queue_work(ar->workqueue, &ar->restart_work); > } > It is better to clear_bit ATH10K_FLAG_CRASH_FLUSH/new flag in ath10k_reconfig_complete instead of ath10k_core_start because ieee80211_reconfig(called by ieee80211_restart_work) of mac80211 do many things and drv_start is 1st thing and drv_reconfig_complete is last thing. > That way restart_work queue would be called only one time. > > Though I'm not sure how ATH10K_STATE_WEDGED would behave after this > change, it might get broken. Ah, actually I think even this patch > breaks > the WEDGED state. This firmware restart is tricky, difficult to say > what > is the best approach. Michal, are you reading? :) Any ideas? > > And after looking more about this patch I don't see the need for the > new > ar->restart_count atomic variable. Checking for ATH10K_FLAG_CRASH_FLUSH > would do the same thing AFAICS. > > And related to this, (in a separate patch) I think we should utilise > ATH10K_FLAG_CRASH_FLUSH more. For example in ath10k_wmi_cmd_send() to > not even try to send a WMI command if the flag is set. Basically all > hardware access should be disabled except what is needed to restart the > firmware.
WARNING: multiple messages have this Message-ID (diff)
From: Wen Gong <wgong@codeaurora.org> To: Kalle Valo <kvalo@codeaurora.org> Cc: linux-wireless@vger.kernel.org, ath10k@lists.infradead.org Subject: Re: [PATCH v4 1/2] ath10k: add refcount for ath10k_core_restart Date: Wed, 19 Aug 2020 20:01:40 +0800 [thread overview] Message-ID: <35a24f0d901be76266f6c9ab020d2acc@codeaurora.org> (raw) In-Reply-To: <87imdlkuw8.fsf@codeaurora.org> On 2020-08-15 01:19, Kalle Valo wrote: ... > > I have been thinking a different approach for this. I think another > option is to have a function like this: > > ath10k_core_firmware_crashed() > { > queue_work(ar->workqueue, &ar->restart_work); > } > > In patch 1 we would convert all existing callers to call that > function instead of queue_work() directly. > > In patch 2 we would add a new flag to enum ath10k_dev_flags, or maybe > should actually use existing ATH10K_FLAG_CRASH_FLUSH? Don't know yet > which one is better. Now the function would do: > > ath10k_core_firmware_crashed() > { > if (test_bit(flag)) > return > > set_bit(flag) > queue_work(ar->workqueue, &ar->restart_work); > } > It is better to clear_bit ATH10K_FLAG_CRASH_FLUSH/new flag in ath10k_reconfig_complete instead of ath10k_core_start because ieee80211_reconfig(called by ieee80211_restart_work) of mac80211 do many things and drv_start is 1st thing and drv_reconfig_complete is last thing. > That way restart_work queue would be called only one time. > > Though I'm not sure how ATH10K_STATE_WEDGED would behave after this > change, it might get broken. Ah, actually I think even this patch > breaks > the WEDGED state. This firmware restart is tricky, difficult to say > what > is the best approach. Michal, are you reading? :) Any ideas? > > And after looking more about this patch I don't see the need for the > new > ar->restart_count atomic variable. Checking for ATH10K_FLAG_CRASH_FLUSH > would do the same thing AFAICS. > > And related to this, (in a separate patch) I think we should utilise > ATH10K_FLAG_CRASH_FLUSH more. For example in ath10k_wmi_cmd_send() to > not even try to send a WMI command if the flag is set. Basically all > hardware access should be disabled except what is needed to restart the > firmware. _______________________________________________ ath10k mailing list ath10k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath10k
next prev parent reply other threads:[~2020-08-19 12:03 UTC|newest] Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-01-08 3:19 [PATCH v4 0/2] start recovery process when payload length overflow for sdio Wen Gong 2020-01-08 3:19 ` Wen Gong 2020-01-08 3:19 ` [PATCH v4 1/2] ath10k: add refcount for ath10k_core_restart Wen Gong 2020-01-08 3:19 ` Wen Gong 2020-01-08 12:02 ` Justin Capella 2020-01-08 12:02 ` Justin Capella 2020-01-10 10:29 ` Wen Gong 2020-01-10 10:29 ` Wen Gong 2020-01-17 7:19 ` Wen Gong 2020-01-17 7:19 ` Wen Gong 2020-01-20 9:38 ` Justin Capella 2020-01-20 9:38 ` Justin Capella 2020-01-20 13:34 ` Wen Gong 2020-01-20 13:34 ` Wen Gong 2020-01-20 15:37 ` Justin Capella 2020-01-20 15:37 ` Justin Capella 2020-08-14 17:19 ` Kalle Valo 2020-08-14 17:19 ` Kalle Valo 2020-08-18 8:39 ` Wen Gong 2020-08-18 8:39 ` Wen Gong 2020-09-07 15:52 ` Kalle Valo 2020-09-07 15:52 ` Kalle Valo 2020-08-19 12:01 ` Wen Gong [this message] 2020-08-19 12:01 ` Wen Gong 2020-08-20 9:18 ` Wen Gong 2020-08-20 9:18 ` Wen Gong 2020-08-24 4:36 ` Wen Gong 2020-08-24 4:36 ` Wen Gong 2020-09-07 15:55 ` Kalle Valo 2020-09-07 15:55 ` Kalle Valo [not found] ` <871rjd37kz.fsf@codeaurora.org> 2020-09-08 3:47 ` Wen Gong 2020-09-08 3:47 ` Wen Gong 2020-01-08 3:19 ` [PATCH v4 2/2] ath10k: start recovery process when payload length exceeds max htc length for sdio Wen Gong 2020-01-08 3:19 ` Wen Gong 2020-08-14 15:37 ` Kalle Valo 2020-08-14 15:37 ` Kalle Valo
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=35a24f0d901be76266f6c9ab020d2acc@codeaurora.org \ --to=wgong@codeaurora.org \ --cc=ath10k@lists.infradead.org \ --cc=kvalo@codeaurora.org \ --cc=linux-wireless@vger.kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
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.