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: Tue, 18 Aug 2020 16:39:04 +0800 [thread overview] Message-ID: <b5a894f46ce01d89a0887d63296fd139@codeaurora.org> (raw) In-Reply-To: <87imdlkuw8.fsf@codeaurora.org> On 2020-08-15 01:19, Kalle Valo wrote: > Wen Gong <wgong@codeaurora.org> writes: > ... >> diff --git a/drivers/net/wireless/ath/ath10k/core.c >> b/drivers/net/wireless/ath/ath10k/core.c >> index 91f131b87efc..0e31846e6c89 100644 >> --- a/drivers/net/wireless/ath/ath10k/core.c >> +++ b/drivers/net/wireless/ath/ath10k/core.c >> @@ -2199,6 +2199,14 @@ static void ath10k_core_restart(struct >> work_struct *work) >> { >> struct ath10k *ar = container_of(work, struct ath10k, restart_work); >> int ret; >> + int restart_count; >> + >> + restart_count = atomic_add_return(1, &ar->restart_count); >> + if (restart_count > 1) { >> + ath10k_warn(ar, "can not restart, count: %d\n", restart_count); >> + atomic_dec(&ar->restart_count); >> + return; >> + } > > 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); > } > > That way restart_work queue would be called only one time. > This is not muti-thread-safe, for example, if 2 thread entered to the test_bit(flag) meanwhile and both check pass, then it will have 2 restart. atomic_add_return is muti-thread-safe, if 2 thread entered it, only 1 thread can pass the check, another will fail and return. The "payload length exceeds max htc length for sdio" happened many times in a very short time, so I add this check for it. > 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: Tue, 18 Aug 2020 16:39:04 +0800 [thread overview] Message-ID: <b5a894f46ce01d89a0887d63296fd139@codeaurora.org> (raw) In-Reply-To: <87imdlkuw8.fsf@codeaurora.org> On 2020-08-15 01:19, Kalle Valo wrote: > Wen Gong <wgong@codeaurora.org> writes: > ... >> diff --git a/drivers/net/wireless/ath/ath10k/core.c >> b/drivers/net/wireless/ath/ath10k/core.c >> index 91f131b87efc..0e31846e6c89 100644 >> --- a/drivers/net/wireless/ath/ath10k/core.c >> +++ b/drivers/net/wireless/ath/ath10k/core.c >> @@ -2199,6 +2199,14 @@ static void ath10k_core_restart(struct >> work_struct *work) >> { >> struct ath10k *ar = container_of(work, struct ath10k, restart_work); >> int ret; >> + int restart_count; >> + >> + restart_count = atomic_add_return(1, &ar->restart_count); >> + if (restart_count > 1) { >> + ath10k_warn(ar, "can not restart, count: %d\n", restart_count); >> + atomic_dec(&ar->restart_count); >> + return; >> + } > > 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); > } > > That way restart_work queue would be called only one time. > This is not muti-thread-safe, for example, if 2 thread entered to the test_bit(flag) meanwhile and both check pass, then it will have 2 restart. atomic_add_return is muti-thread-safe, if 2 thread entered it, only 1 thread can pass the check, another will fail and return. The "payload length exceeds max htc length for sdio" happened many times in a very short time, so I add this check for it. > 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-18 8:39 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 [this message] 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 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=b5a894f46ce01d89a0887d63296fd139@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.