Re: [PATCH v2 2/2] wifi: ath9k: Reset chip on potential deaf state

[Sven Eckelmann <se@simonwunderlich.de>]

On Wednesday, 6 November 2024 10:04:39 CET Issam Hamdi wrote:
[...]
> This patch originally developed by "Simon Wunderlich <simon.wunderlich@open-mesh.com>"
> and "Sven Eckelmann <sven.eckelmann@open-mesh.com>"

Am I the only person which finds this style of adding information about "Co-
authors" weird?

[...]
> Signed-off-by: Simon Wunderlich <sw@simonwunderlich.de>
> Signed-off-by: Sven Eckelmann <se@simonwunderlich.de>
> Signed-off-by: Issam Hamdi <ih@simonwunderlich.de>
> ---
> 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@simonwunderlich.de

[...]
> +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@nbd.name/
[3] https://lore.kernel.org/all/2081606.z26xgMiW1A@prime/