| Message ID | 20171125153510.25359-7-luca@coelho.fi (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Luca Coelho |
| Headers | show |
Luca Coelho <luca@coelho.fi> writes: > From: Sara Sharon <sara.sharon@intel.com> > > When getting HW rfkill we get stop_device being called from two paths. > One path is the IRQ calling stop device, and updating op mode and > stack. As a result, cfg80211 is running rfkill sync work that shuts > down all devices (second path). In the second path, we eventually get > to iwl_mvm_stop_device which calls > iwl_fw_dump_conf_clear->iwl_fw_dbg_stop_recording, that access > periphery registers. The device may be stopped at this point from the > first path, which will result with a failure to access those > registers. Simply checking for the trans status is insufficient, since > the race will still exist, only minimized. Instead, move the stop from > iwl_fw_dump_conf_clear (which is getting called only from stop path) > to the transport stop device function, where the access is always > safe. This has the added value, of actually stopping dbgc before > stopping device even when the stop is initiated from the transport. > > Fixes: 1efc3843a4ee ("iwlwifi: stop dbgc recording before stopping > DMA") No need to resend because of this, but Fixes should be in one line.
Kalle Valo <kvalo@codeaurora.org> writes: > Luca Coelho <luca@coelho.fi> writes: > >> From: Sara Sharon <sara.sharon@intel.com> >> >> When getting HW rfkill we get stop_device being called from two paths. >> One path is the IRQ calling stop device, and updating op mode and >> stack. As a result, cfg80211 is running rfkill sync work that shuts >> down all devices (second path). In the second path, we eventually get >> to iwl_mvm_stop_device which calls >> iwl_fw_dump_conf_clear->iwl_fw_dbg_stop_recording, that access >> periphery registers. The device may be stopped at this point from the >> first path, which will result with a failure to access those >> registers. Simply checking for the trans status is insufficient, since >> the race will still exist, only minimized. Instead, move the stop from >> iwl_fw_dump_conf_clear (which is getting called only from stop path) >> to the transport stop device function, where the access is always >> safe. This has the added value, of actually stopping dbgc before >> stopping device even when the stop is initiated from the transport. >> >> Fixes: 1efc3843a4ee ("iwlwifi: stop dbgc recording before stopping >> DMA") > > No need to resend because of this, but Fixes should be in one line. Please ignore, I was too clever and forgot that I had called gnus-article-fill-cited-article which word wraps the mail :)
Luca Coelho <luca@coelho.fi> writes: > From: Sara Sharon <sara.sharon@intel.com> > > When getting HW rfkill we get stop_device being called from > two paths. > One path is the IRQ calling stop device, and updating op > mode and stack. > As a result, cfg80211 is running rfkill sync work that shuts > down all devices (second path). > In the second path, we eventually get to iwl_mvm_stop_device > which calls iwl_fw_dump_conf_clear->iwl_fw_dbg_stop_recording, > that access periphery registers. > The device may be stopped at this point from the first path, > which will result with a failure to access those registers. > Simply checking for the trans status is insufficient, since > the race will still exist, only minimized. > Instead, move the stop from iwl_fw_dump_conf_clear (which is > getting called only from stop path) to the transport stop > device function, where the access is always safe. > This has the added value, of actually stopping dbgc before > stopping device even when the stop is initiated from the > transport. > > Fixes: 1efc3843a4ee ("iwlwifi: stop dbgc recording before stopping DMA") > Signed-off-by: Sara Sharon <sara.sharon@intel.com> > Signed-off-by: Luca Coelho <luciano.coelho@intel.com> But no wonder I called gnus-article-fill-cited-article, the commit log is just weirdly wrapped. Are you using outlook or how do you get it so ugly? :)
On Wed, 2017-11-29 at 12:23 +0200, Kalle Valo wrote: > Luca Coelho <luca@coelho.fi> writes: > > > From: Sara Sharon <sara.sharon@intel.com> > > > > When getting HW rfkill we get stop_device being called from > > two paths. > > One path is the IRQ calling stop device, and updating op > > mode and stack. > > As a result, cfg80211 is running rfkill sync work that shuts > > down all devices (second path). > > In the second path, we eventually get to iwl_mvm_stop_device > > which calls iwl_fw_dump_conf_clear->iwl_fw_dbg_stop_recording, > > that access periphery registers. > > The device may be stopped at this point from the first path, > > which will result with a failure to access those registers. > > Simply checking for the trans status is insufficient, since > > the race will still exist, only minimized. > > Instead, move the stop from iwl_fw_dump_conf_clear (which is > > getting called only from stop path) to the transport stop > > device function, where the access is always safe. > > This has the added value, of actually stopping dbgc before > > stopping device even when the stop is initiated from the > > transport. > > > > Fixes: 1efc3843a4ee ("iwlwifi: stop dbgc recording before stopping > > DMA") > > Signed-off-by: Sara Sharon <sara.sharon@intel.com> > > Signed-off-by: Luca Coelho <luciano.coelho@intel.com> > > But no wonder I called gnus-article-fill-cited-article, the commit > log > is just weirdly wrapped. Are you using outlook or how do you get it > so > ugly? :) Heh! I don't think it's wrapped weirdly, it's just that the paragraphs don't have spaces between them, right? ;) I noticed this but was probably too tired to nitpick back when I merged this in our internal tree. I'll make sure people space the commit messages better from now on. Sorry for the trouble. -- Cheers, Luca.
Luca Coelho <luca@coelho.fi> writes: > On Wed, 2017-11-29 at 12:23 +0200, Kalle Valo wrote: > >> But no wonder I called gnus-article-fill-cited-article, the commit >> log is just weirdly wrapped. Are you using outlook or how do you get >> it so ugly? :) > > Heh! I don't think it's wrapped weirdly, it's just that the paragraphs > don't have spaces between them, right? ;) Sorry, don't get what you mean with missing spaces. To me (and patchwork[1] and git[2] seem to agree) the word wrapping is just broken for this commit log, and I have seen it also with other iwlwifi commits. For example, there are just two words in the second line "two paths." The standard format for git commit logs is something like this: ---------------------------------------------------------------------- When getting HW rfkill we get stop_device being called from two paths. One path is the IRQ calling stop device, and updating op mode and stack. As a result, cfg80211 is running rfkill sync work that shuts down all devices (second path). In the second path, we eventually get to iwl_mvm_stop_device which calls iwl_fw_dump_conf_clear->iwl_fw_dbg_stop_recording, that access periphery registers. The device may be stopped at this point from the first path, which will result with a failure to access those registers. Simply checking for the trans status is insufficient, since the race will still exist, only minimized. Instead, move the stop from iwl_fw_dump_conf_clear (which is getting called only from stop path) to the transport stop device function, where the access is always safe. This has the added value, of actually stopping dbgc before stopping device even when the stop is initiated from the transport. ---------------------------------------------------------------------- [1] https://patchwork.kernel.org/patch/10074849/ [2] https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/wireless-drivers.git/commit/?id=0232d2cd7aa8e1b810fe84fb4059a0bd1eabe2ba
Kalle Valo <kvalo@codeaurora.org> writes: > Luca Coelho <luca@coelho.fi> writes: > >> On Wed, 2017-11-29 at 12:23 +0200, Kalle Valo wrote: >> >>> But no wonder I called gnus-article-fill-cited-article, the commit >>> log is just weirdly wrapped. Are you using outlook or how do you get >>> it so ugly? :) >> >> Heh! I don't think it's wrapped weirdly, it's just that the paragraphs >> don't have spaces between them, right? ;) > > Sorry, don't get what you mean with missing spaces. To me (and > patchwork[1] and git[2] seem to agree) the word wrapping is just broken > for this commit log, and I have seen it also with other iwlwifi commits. > For example, there are just two words in the second line "two paths." > > The standard format for git commit logs is something like this: > > ---------------------------------------------------------------------- > When getting HW rfkill we get stop_device being called from two paths. > One path is the IRQ calling stop device, and updating op mode and stack. > As a result, cfg80211 is running rfkill sync work that shuts down all > devices (second path). In the second path, we eventually get to > iwl_mvm_stop_device which calls iwl_fw_dump_conf_clear->iwl_fw_dbg_stop_recording, > that access periphery registers. > > The device may be stopped at this point from the first path, > which will result with a failure to access those registers. Simply > checking for the trans status is insufficient, since the race will still > exist, only minimized. Instead, move the stop from > iwl_fw_dump_conf_clear (which is getting called only from stop path) to > the transport stop device function, where the access is always safe. > This has the added value, of actually stopping dbgc before stopping > device even when the stop is initiated from the transport. > ---------------------------------------------------------------------- > > [1] https://patchwork.kernel.org/patch/10074849/ Instead of trying to get what I mean, check instead the patchwork page and compare there the original commit log with my reformatted version. I think that visualises pretty well what I'm trying to say :)
On Fri, 2017-12-08 at 14:22 +0200, Kalle Valo wrote: > Luca Coelho <luca@coelho.fi> writes: > > > On Wed, 2017-11-29 at 12:23 +0200, Kalle Valo wrote: > > > > > But no wonder I called gnus-article-fill-cited-article, the > > > commit > > > log is just weirdly wrapped. Are you using outlook or how do you > > > get > > > it so ugly? :) > > > > Heh! I don't think it's wrapped weirdly, it's just that the > > paragraphs > > don't have spaces between them, right? ;) > > Sorry, don't get what you mean with missing spaces. To me (and > patchwork[1] and git[2] seem to agree) the word wrapping is just > broken > for this commit log, and I have seen it also with other iwlwifi > commits. > For example, there are just two words in the second line "two paths." That depends on how many characters per line you want to include. "two paths." is the last part of the first paragraph, I guess Sari's editor is wrapping somewhere at <70 chars per line. Then that is accentuated a bit by the lack of an empty line between paragraphs. But with those lines added, besides being a bit narrow, it would look quite clear: -----8<----- When getting HW rfkill we get stop_device being called from two paths. One path is the IRQ calling stop device, and updating op mode and stack. As a result, cfg80211 is running rfkill sync work that shuts down all devices (second path). In the second path, we eventually get to iwl_mvm_stop_device which calls iwl_fw_dump_conf_clear->iwl_fw_dbg_stop_recording, that access periphery registers. The device may be stopped at this point from the first path, which will result with a failure to access those registers. Simply checking for the trans status is insufficient, since the race will still exist, only minimized. Instead, move the stop from iwl_fw_dump_conf_clear (which is getting called only from stop path) to the transport stop device function, where the access is always safe. This has the added value, of actually stopping dbgc before stopping device even when the stop is initiated from the transport. ----->8----- Does it make sense? I've already told Sari about your preference and I'll make sure the next patches will look cleaner. I can fix this patch if you want, but is it worth remaking my pull-request just for this? -- Cheers, Luca.
On Fri, 2017-12-08 at 14:26 +0200, Kalle Valo wrote: > Kalle Valo <kvalo@codeaurora.org> writes: > > > Luca Coelho <luca@coelho.fi> writes: > > > > > On Wed, 2017-11-29 at 12:23 +0200, Kalle Valo wrote: > > > > > > > But no wonder I called gnus-article-fill-cited-article, the > > > > commit > > > > log is just weirdly wrapped. Are you using outlook or how do > > > > you get > > > > it so ugly? :) > > > > > > Heh! I don't think it's wrapped weirdly, it's just that the > > > paragraphs > > > don't have spaces between them, right? ;) > > > > Sorry, don't get what you mean with missing spaces. To me (and > > patchwork[1] and git[2] seem to agree) the word wrapping is just > > broken > > for this commit log, and I have seen it also with other iwlwifi > > commits. > > For example, there are just two words in the second line "two > > paths." > > > > The standard format for git commit logs is something like this: > > > > ----------------------------------------------------------------- > > ----- > > When getting HW rfkill we get stop_device being called from two > > paths. > > One path is the IRQ calling stop device, and updating op mode and > > stack. > > As a result, cfg80211 is running rfkill sync work that shuts down > > all > > devices (second path). In the second path, we eventually get to > > iwl_mvm_stop_device which calls iwl_fw_dump_conf_clear- > > >iwl_fw_dbg_stop_recording, > > that access periphery registers. > > > > The device may be stopped at this point from the first path, > > which will result with a failure to access those registers. Simply > > checking for the trans status is insufficient, since the race will > > still > > exist, only minimized. Instead, move the stop from > > iwl_fw_dump_conf_clear (which is getting called only from stop > > path) to > > the transport stop device function, where the access is always > > safe. > > This has the added value, of actually stopping dbgc before stopping > > device even when the stop is initiated from the transport. > > ----------------------------------------------------------------- > > ----- > > > > [1] https://patchwork.kernel.org/patch/10074849/ > > Instead of trying to get what I mean, check instead the patchwork > page > and compare there the original commit log with my reformatted > version. I > think that visualises pretty well what I'm trying to say :) Okay, granted, but that's not really because of alignment or line- wrapping. It's because the paragraphs are broken too often. In any case, I'll pay more attention to this. So my questions remains, do you want me to remake my pull-req to fix this? -- Luca.
Luca Coelho <luca@coelho.fi> writes: > On Fri, 2017-12-08 at 14:26 +0200, Kalle Valo wrote: >> Kalle Valo <kvalo@codeaurora.org> writes: >> >> > Luca Coelho <luca@coelho.fi> writes: >> > >> > > On Wed, 2017-11-29 at 12:23 +0200, Kalle Valo wrote: >> > > >> > > > But no wonder I called gnus-article-fill-cited-article, the >> > > > commit >> > > > log is just weirdly wrapped. Are you using outlook or how do >> > > > you get >> > > > it so ugly? :) >> > > >> > > Heh! I don't think it's wrapped weirdly, it's just that the >> > > paragraphs >> > > don't have spaces between them, right? ;) >> > >> > Sorry, don't get what you mean with missing spaces. To me (and >> > patchwork[1] and git[2] seem to agree) the word wrapping is just >> > broken >> > for this commit log, and I have seen it also with other iwlwifi >> > commits. >> > For example, there are just two words in the second line "two >> > paths." >> > >> > The standard format for git commit logs is something like this: >> > >> > ----------------------------------------------------------------- >> > ----- >> > When getting HW rfkill we get stop_device being called from two >> > paths. >> > One path is the IRQ calling stop device, and updating op mode and >> > stack. >> > As a result, cfg80211 is running rfkill sync work that shuts down >> > all >> > devices (second path). In the second path, we eventually get to >> > iwl_mvm_stop_device which calls iwl_fw_dump_conf_clear- >> > >iwl_fw_dbg_stop_recording, >> > that access periphery registers. >> > >> > The device may be stopped at this point from the first path, >> > which will result with a failure to access those registers. Simply >> > checking for the trans status is insufficient, since the race will >> > still >> > exist, only minimized. Instead, move the stop from >> > iwl_fw_dump_conf_clear (which is getting called only from stop >> > path) to >> > the transport stop device function, where the access is always >> > safe. >> > This has the added value, of actually stopping dbgc before stopping >> > device even when the stop is initiated from the transport. >> > ----------------------------------------------------------------- >> > ----- >> > >> > [1] https://patchwork.kernel.org/patch/10074849/ >> >> Instead of trying to get what I mean, check instead the patchwork >> page >> and compare there the original commit log with my reformatted >> version. I >> think that visualises pretty well what I'm trying to say :) > > Okay, granted, but that's not really because of alignment or line- > wrapping. It's because the paragraphs are broken too often. > > In any case, I'll pay more attention to this. So my questions remains, > do you want me to remake my pull-req to fix this? This commit is already in wireless-drivers, so no need to resend. I was just trying to give an example of what I was trying to say.
diff --git a/drivers/net/wireless/intel/iwlwifi/fw/dbg.h b/drivers/net/wireless/intel/iwlwifi/fw/dbg.h index 9c889a32fe24..223fb77a3aa9 100644 --- a/drivers/net/wireless/intel/iwlwifi/fw/dbg.h +++ b/drivers/net/wireless/intel/iwlwifi/fw/dbg.h @@ -209,8 +209,6 @@ static inline void iwl_fw_dbg_stop_recording(struct iwl_fw_runtime *fwrt) static inline void iwl_fw_dump_conf_clear(struct iwl_fw_runtime *fwrt) { - iwl_fw_dbg_stop_recording(fwrt); - fwrt->dump.conf = FW_DBG_INVALID; } diff --git a/drivers/net/wireless/intel/iwlwifi/pcie/trans-gen2.c b/drivers/net/wireless/intel/iwlwifi/pcie/trans-gen2.c index c59f4581e972..ac05fd1e74c4 100644 --- a/drivers/net/wireless/intel/iwlwifi/pcie/trans-gen2.c +++ b/drivers/net/wireless/intel/iwlwifi/pcie/trans-gen2.c @@ -49,6 +49,7 @@ * *****************************************************************************/ #include "iwl-trans.h" +#include "iwl-prph.h" #include "iwl-context-info.h" #include "internal.h" @@ -156,6 +157,11 @@ void _iwl_trans_pcie_gen2_stop_device(struct iwl_trans *trans, bool low_power) trans_pcie->is_down = true; + /* Stop dbgc before stopping device */ + iwl_write_prph(trans, DBGC_IN_SAMPLE, 0); + udelay(100); + iwl_write_prph(trans, DBGC_OUT_CTRL, 0); + /* tell the device to stop sending interrupts */ iwl_disable_interrupts(trans); diff --git a/drivers/net/wireless/intel/iwlwifi/pcie/trans.c b/drivers/net/wireless/intel/iwlwifi/pcie/trans.c index 3dee95e6a475..4541c86881d6 100644 --- a/drivers/net/wireless/intel/iwlwifi/pcie/trans.c +++ b/drivers/net/wireless/intel/iwlwifi/pcie/trans.c @@ -1227,6 +1227,15 @@ static void _iwl_trans_pcie_stop_device(struct iwl_trans *trans, bool low_power) trans_pcie->is_down = true; + /* Stop dbgc before stopping device */ + if (trans->cfg->device_family == IWL_DEVICE_FAMILY_7000) { + iwl_set_bits_prph(trans, MON_BUFF_SAMPLE_CTL, 0x100); + } else { + iwl_write_prph(trans, DBGC_IN_SAMPLE, 0); + udelay(100); + iwl_write_prph(trans, DBGC_OUT_CTRL, 0); + } + /* tell the device to stop sending interrupts */ iwl_disable_interrupts(trans);