From:   Kalle Valo <kvalo@kernel.org>
To:     Manikanta Pubbisetty <quic_mpubbise@quicinc.com>
Cc:     <ath11k@lists.infradead.org>, <linux-wireless@vger.kernel.org>
Subject: Re: [PATCH v4 3/3] ath11k: Enable low power mode when WLAN is not active
References: <20230203060128.19625-1-quic_mpubbise@quicinc.com>
        <20230203060128.19625-4-quic_mpubbise@quicinc.com>
Date:   Fri, 24 Feb 2023 16:46:31 +0200
In-Reply-To: <20230203060128.19625-4-quic_mpubbise@quicinc.com> (Manikanta
        Pubbisetty's message of "Fri, 3 Feb 2023 11:31:28 +0530")
Message-ID: <87h6vbruig.fsf@kernel.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

Manikanta Pubbisetty <quic_mpubbise@quicinc.com> writes:

> Currently, WLAN chip is powered once during driver probe and is kept
> ON (powered) always even when WLAN is not active; keeping the chip
> powered ON all the time will consume extra power which is not
> desirable for a battery operated device. Same is the case with non-WoW
> suspend, chip will never be put into low power mode when the system is
> suspended resulting in higher battery drain.
>
> As per the recommendation, sending a PDEV suspend WMI command followed
> by a QMI MODE OFF command will cease all WLAN activity and put the device
> in low power mode. When WLAN interfaces are brought up, sending a QMI
> MISSION MODE command would be sufficient to bring the chip out of low
> power. This is a better approach than doing hif_power_down()/hif_power_up()
> for every WiFi ON/OFF sequence since the turnaround time for entry/exit of
> low power mode is much less. Overhead is just the time taken for sending
> QMI MODE OFF & QMI MISSION MODE commands instead of going through the
> entire chip boot & QMI init sequence.
>
> Currently the changes are applicable only for WCN6750. This can be
> extended to other targets with a future patch.
>
> Tested-on: WCN6750 hw1.0 AHB WLAN.MSL.1.0.1-00887-QCAMSLSWPLZ-1
> Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3.6510.16
>
> Signed-off-by: Manikanta Pubbisetty <quic_mpubbise@quicinc.com>

This is still crashing for me every time with WCN6855 on a NUC x86
device when I rmmod ath11k. Interestingly enough QCA6390 on a Dell XPS
13 9310 does not crash.

I investigated the crash more, the crash happens in
ath11k_dp_process_rxdma_err() on this line:

	srng = &ab->hal.srng_list[err_ring->ring_id];

Here are the debug messages before the crash (first and last are my
own messages):

[  226.766111] rmmod ath11k_pci
[  227.003678] ath11k_pci 0000:06:00.0: txpower from firmware NaN, reported -2147483648 dBm
[  227.082283] ath11k_pci 0000:06:00.0: qmi wifi fw del server
[  227.195760] ath11k_pci 0000:06:00.0: cookie:0x0
[  227.195843] ath11k_pci 0000:06:00.0: WLAON_WARM_SW_ENTRY 0x15b894d
[  227.216022] ath11k_pci 0000:06:00.0: WLAON_WARM_SW_ENTRY 0x0
[  227.216086] ath11k_pci 0000:06:00.0: soc reset cause:0
[  227.236170] ath11k_pci 0000:06:00.0: MHISTATUS 0xff04
[  227.270816] ath11k_pci 0000:06:00.0: ext irq:167
[  227.271231] ath11k_dp_process_rxdma_err() 4187 ab ffff888145520000 err_ring 00000000000001d0

So we get irq 167 which is:

 167:          0          0          0          0          0          0          0          0  IR-PCI-MSI-0000:06:00.0   14-edge      DP_EXT_IRQ

But in ath11k_pcic_ext_interrupt_handler() ATH11K_FLAG_EXT_IRQ_ENABLED
is still enabled so the irq is processed:

	if (!test_bit(ATH11K_FLAG_EXT_IRQ_ENABLED, &ab->dev_flags))
		return IRQ_HANDLED;

It looks like that, after applying this patch 3, whenever
ath11k_pci_remove() is called we are not calling
ath11k_hif_irq_disable() anymore. I checked that without patch 3
ath11k_hif_irq_disable() is always called. So this patch is definitely
breaking something fundamental, but I ran out of time to invetigate
further. I hope this still helps.

Do note I have concerns about this patchset, it just changes quite a lot
of the driver logic and I'm worried what else this breaks. Also we
should definitely test with another AHB device like IPQ8074, this
patchset needs extensive testing.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches