On Wednesday, 6 November 2024 10:04:39 CET Issam Hamdi wrote: [...] > This patch originally developed by "Simon Wunderlich <simon.wunderlich@xxxxxxxxxxxxx>" > and "Sven Eckelmann <sven.eckelmann@xxxxxxxxxxxxx>" Am I the only person which finds this style of adding information about "Co- authors" weird? [...] > Signed-off-by: Simon Wunderlich <sw@xxxxxxxxxxxxxxxxxx> > Signed-off-by: Sven Eckelmann <se@xxxxxxxxxxxxxxxxxx> > Signed-off-by: Issam Hamdi <ih@xxxxxxxxxxxxxxxxxx> > --- > v2: change the "Co-developed-by" to "Signed-off-by", remove the dependency I think Kalle meant that "Co-developed-by" should be followed by a "Signed-off-by" - not that "Co-developed-by" should be removed. I was not part of the delivery path for this version of the patch. But current Signed-off-by seem to suggest this. > on CONFIG_ATH9K_DEBUGFS and add more information in the commit description And please don't reply to the old thread when sending a new patchset - this becomes really unreadable after a while. You can simply use the method which b4 uses and just reference the old thread in your mail. Something like: Changes in v2: - change the "Co-developed-by" to "Signed-off-by" - remove the dependency on CONFIG_ATH9K_DEBUGFS - add more information in the commit description - Link to v1: https://lore.kernel.org/r/20241104171627.3789199-1-ih@xxxxxxxxxxxxxxxxxx [...] > +static bool ath_hw_hang_deaf(struct ath_softc *sc) > +{ > +#ifdef CONFIG_ATH9K_TX99 > + return false; > +#else > + struct ath_common *common = ath9k_hw_common(sc->sc_ah); > + u32 interrupts, interrupt_per_s; > + unsigned int interval; > + > + /* get historic data */ > + interval = jiffies_to_msecs(jiffies - sc->last_check_time); > + if (sc->sc_ah->caps.hw_caps & ATH9K_HW_CAP_EDMA) > + interrupts = sc->debug.stats.istats.rxlp; > + else > + interrupts = sc->debug.stats.istats.rxok; You can't simply access sc->debug.stats.istats. sc->debug is only available when building with CONFIG_ATH9K_DEBUGFS. See ath9k.c struct ath_softc { [...] #ifdef CONFIG_ATH9K_DEBUGFS struct ath9k_debug debug; #endif [...] } > + /* sanity check, should be 4 seconds */ > + if (interval > 10000 || interval < 1000) Here you have hardcoded values but the actual interval is hidden behind ATH_HANG_WORK_INTERVAL. Two things which now are rather disconnected and might cause problems in the future (when somebody fiddles around with ATH_HANG_WORK_INTERVAL). Overall, the proposal from Toke seems to be a lot better integrated in the HW check style which was introduced by Felix in the beginning of 2017 [1]. At the same time there was a proposal by Felix [2] - which diverged too much from our original patch (and as a result caused too many resets) [3]. I would therefore propose to check Toke's version and test handles the problem correctly. Kind regards, Sven [1] https://github.com/openwrt/openwrt/commit/b94177e10fc72f9309eae7459c3570e5c080e960 [2] https://patchwork.kernel.org/project/linux-wireless/patch/20170125163654.66431-3-nbd@xxxxxxxx/ [3] https://lore.kernel.org/all/2081606.z26xgMiW1A@prime/
