Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) client-ip=23.128.96.32;
Date:   Wed, 1 Nov 2023 16:56:55 +0100
From:   Stanislaw Gruszka <stf_xl@wp.pl>
To:     Shiji Yang <yangshiji66@outlook.com>
Cc:     linux-wireless@vger.kernel.org, Kalle Valo <kvalo@kernel.org>
Subject: Re: [PATCH 1/3] wifi: rt2x00: introduce DMA busy check watchdog for
 rt2800
Message-ID: <20231101155655.GA559463@wp.pl>
References: <TYAP286MB0315F9EFFA3D13AA1AD7AEF4BCA3A@TYAP286MB0315.JPNP286.PROD.OUTLOOK.COM>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <TYAP286MB0315F9EFFA3D13AA1AD7AEF4BCA3A@TYAP286MB0315.JPNP286.PROD.OUTLOOK.COM>
Precedence: bulk

On Sat, Oct 28, 2023 at 08:15:30PM +0800, Shiji Yang wrote:
> When I tried to fix the watchdog of rt2800, I found that sometimes
> the watchdog could not reset the hung device. This is because the
> queue did not completely stop, it just became very slow. The Mediatek
> vendor driver for the new chips (MT7603/MT7612) has a DMA busy
> watchdog to detect device hangs by checking DMA busy status. This
> implementation is something similar to it. To reduce unnecessary
> watchdog reset, we can check the INT_STATUS register together as I
> found that when the radio hung, the RX/TX coherent interrupt will
> always stuck at triggered state.
> 
> This patch also changes the watchdog module parameters to the new
> 'hang_watchdog' and 'dma_busy_watchdog' so that we can control them
> separately. That's because they may have different behavior on
> specific chip.
> 
> This watchdog function is a slight schedule and it won't affect the
> WiFi transmission speed. Watchdog can help the driver automatically
> recover from the abnormal state. So I think it should be default on.
> Anyway it can be disabled by module parameter 'dma_busy_watchdog=0'.
> 
> Tested on MT7620 and RT5350.

I think this will not work on USB as INT_SOURCE_CSR is mmio/pci
specific. Did you tested on USB ? Or this is disabled for USB by
default? 

> Signed-off-by: Shiji Yang <yangshiji66@outlook.com>
> ---
>  .../net/wireless/ralink/rt2x00/rt2800lib.c    | 81 ++++++++++++++++---
>  drivers/net/wireless/ralink/rt2x00/rt2x00.h   |  3 +
>  2 files changed, 72 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/wireless/ralink/rt2x00/rt2800lib.c b/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
> index 594dd3d9f..6ca2f2c23 100644
> --- a/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
> +++ b/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
> @@ -30,9 +30,15 @@
>  #include "rt2800lib.h"
>  #include "rt2800.h"
>  
> -static bool modparam_watchdog;
> -module_param_named(watchdog, modparam_watchdog, bool, S_IRUGO);
> -MODULE_PARM_DESC(watchdog, "Enable watchdog to detect tx/rx hangs and reset hardware if detected");
> +static bool modparam_dma_wdt = true;
> +module_param_named(dma_busy_watchdog, modparam_dma_wdt, bool, 0444);
> +MODULE_PARM_DESC(dma_busy_watchdog, "Enable watchdog to detect tx/rx"
> +		 " DMA busy and reset hardware if detected");
> +
> +static bool modparam_hang_wdt;
> +module_param_named(hang_watchdog, modparam_hang_wdt, bool, 0444);
> +MODULE_PARM_DESC(hang_watchdog, "Enable watchdog to detect tx/rx hangs"
> +		 " and reset hardware if detected");

Do not have strong opinion here. But please consider to keep old module
parameter name and make it bitmask, 1 - hang_wdt, 2 dma_wdt, 3 - both.
Such way, it will keep old meaning if someone is using the parameter in
script/config.  

I also wandering if we need two implementations. If dma 
hang detection is superior, it should replace the old one IMHO.
Or queue hang should be used for USB and dma hang for pci/mmio ? 

Otherwise code looks fine/correct to me. 

Regards
Stanislaw