On 6/12/2025 12:40 AM, balsam.chihi@xxxxxxxxxxx wrote:
> From: Balsam CHIHI <balsam.chihi@xxxxxxxxxxx>
>
> During system reboot or module removal (rmmod), a crash was observed
> due to a synchronous external abort caused by invalid register access
> in ath11k_pci_clear_dbg_registers(). This happens when the device is
> already powered off and the driver attempts to read debug registers.
What does 'powered off' mean exactly here? is WLAN powered by an internal/external supply
that will be removed during rmmod/reboot?
>
> The crash log shows:
> root@OpenWrt:~# reboot
> [ 343.663492] Internal error: synchronous external abort:
> 0000000096000210 [#1] SMP
> [ 343.670992] Modules linked in: [...]
> [ 343.842432] CPU: 7 PID: 9435 Comm: procd Tainted: G O 6.6.86 #0
> [ 343.849746] Hardware name: LS1088A
> [ 343.856969] pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> [ 343.863933] pc : ath11k_pci_get_msi_irq+0x18a0/0x1900 [ath11k_pci]
I don't see any relationship between ath11k_pci_clear_dbg_registers() and this PC pointer.
So how did you locate the problematic register access then?
> [ 343.870125] lr : ath11k_pcic_init_msi_config+0x98/0xc4 [ath11k]
> [...]
> [ 343.950800] Call trace:
> [ 343.953241] ath11k_pci_get_msi_irq+0x18a0/0x1900 [ath11k_pci]
> [ 343.959080] ath11k_pcic_init_msi_config+0x98/0xc4 [ath11k]
> [ 343.964671] ath11k_pcic_read32+0x30/0xb4 [ath11k]
> [ 343.969477] ath11k_pci_get_msi_irq+0x528/0x1900 [ath11k_pci]
> [ 343.975230] ath11k_pci_get_msi_irq+0x1460/0x1900 [ath11k_pci]
> [ 343.981068] ath11k_pci_get_msi_irq+0x1750/0x1900 [ath11k_pci]
> [ 343.986906] pci_device_shutdown+0x34/0x44
> [ 343.991004] device_shutdown+0x160/0x268
> [ 343.994928] kernel_restart+0x40/0xc0
> [ 343.998594] __do_sys_reboot+0x104/0x23c
> [ 344.002518] __arm64_sys_reboot+0x24/0x30
> [ 344.006529] do_el0_svc+0x6c/0x100
> [ 344.009931] el0_svc+0x28/0x9c
> [ 344.012986] el0t_64_sync_handler+0x120/0x12c
> [ 344.017344] el0t_64_sync+0x178/0x17c
> [ 344.021009] Code: f94e0a80 92404a73 91420273 8b130013 (b9400273)
> [ 344.027102] ---[ end trace 0000000000000000 ]---
>
> This patch adds a `power_on` flag to conditionally skip debug register
> reads when the device is not powered, preventing invalid memory access.
why is only 'read' concerned here?
You are skipping several 'read' in this change, could you help narrow down which
individual read instance causes this?
>
> With this change, the system no longer crashes on reboot or rmmod,
> and the driver continues to function correctly when reloaded.
>
> Tested-on: QCN9074 hw2.0 PCIe on LS1088A
> Tested-on: OpenWrt 24.10.1
> Fixes: crash on reboot/rmmod due to invalid register access
> Signed-off-by: Balsam CHIHI <balsam.chihi@xxxxxxxxxxx>
> ---
> drivers/net/wireless/ath/ath11k/pci.c | 36 ++++++++++++++++-----------
> 1 file changed, 21 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath11k/pci.c b/drivers/net/wireless/ath/ath11k/pci.c
> index 78444f8ea153..0c1b55957557 100644
> --- a/drivers/net/wireless/ath/ath11k/pci.c
> +++ b/drivers/net/wireless/ath/ath11k/pci.c
> @@ -203,16 +203,18 @@ static void ath11k_pci_soc_global_reset(struct ath11k_base *ab)
> ath11k_warn(ab, "link down error during global reset\n");
> }
>
> -static void ath11k_pci_clear_dbg_registers(struct ath11k_base *ab)
> +static void ath11k_pci_clear_dbg_registers(struct ath11k_base *ab, bool power_on)
> {
> - u32 val;
> + if (power_on) {
> + u32 val;
>
> - /* read cookie */
> - val = ath11k_pcic_read32(ab, PCIE_Q6_COOKIE_ADDR);
> - ath11k_dbg(ab, ATH11K_DBG_PCI, "pcie_q6_cookie_addr 0x%x\n", val);
> + /* read cookie */
> + val = ath11k_pcic_read32(ab, PCIE_Q6_COOKIE_ADDR);
> + ath11k_dbg(ab, ATH11K_DBG_PCI, "pcie_q6_cookie_addr 0x%x\n", val);
>
> - val = ath11k_pcic_read32(ab, WLAON_WARM_SW_ENTRY);
> - ath11k_dbg(ab, ATH11K_DBG_PCI, "wlaon_warm_sw_entry 0x%x\n", val);
> + val = ath11k_pcic_read32(ab, WLAON_WARM_SW_ENTRY);
> + ath11k_dbg(ab, ATH11K_DBG_PCI, "wlaon_warm_sw_entry 0x%x\n", val);
> + }
>
> /* TODO: exact time to sleep is uncertain */
> mdelay(10);
> @@ -223,14 +225,18 @@ static void ath11k_pci_clear_dbg_registers(struct ath11k_base *ab)
> ath11k_pcic_write32(ab, WLAON_WARM_SW_ENTRY, 0);
> mdelay(10);
>
> - val = ath11k_pcic_read32(ab, WLAON_WARM_SW_ENTRY);
> - ath11k_dbg(ab, ATH11K_DBG_PCI, "wlaon_warm_sw_entry 0x%x\n", val);
> + if (power_on) {
> + u32 val;
>
> - /* A read clear register. clear the register to prevent
> - * Q6 from entering wrong code path.
> - */
> - val = ath11k_pcic_read32(ab, WLAON_SOC_RESET_CAUSE_REG);
> - ath11k_dbg(ab, ATH11K_DBG_PCI, "soc reset cause %d\n", val);
> + val = ath11k_pcic_read32(ab, WLAON_WARM_SW_ENTRY);
> + ath11k_dbg(ab, ATH11K_DBG_PCI, "wlaon_warm_sw_entry 0x%x\n", val);
> +
> + /* A read clear register. clear the register to prevent
> + * Q6 from entering wrong code path.
> + */
> + val = ath11k_pcic_read32(ab, WLAON_SOC_RESET_CAUSE_REG);
> + ath11k_dbg(ab, ATH11K_DBG_PCI, "soc reset cause %d\n", val);
> + }
> }
>
> static int ath11k_pci_set_link_reg(struct ath11k_base *ab,
> @@ -368,7 +374,7 @@ static void ath11k_pci_sw_reset(struct ath11k_base *ab, bool power_on)
> }
>
> ath11k_mhi_clear_vector(ab);
> - ath11k_pci_clear_dbg_registers(ab);
> + ath11k_pci_clear_dbg_registers(ab, power_on);
> ath11k_pci_soc_global_reset(ab);
> ath11k_mhi_set_mhictrl_reset(ab);
> }
