Re: [PATCH ath-next 11/17] wifi: ath12k: Add platform MSI support for CE

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Resource leak of non-devm MSIs and IRQs on ath12k_ahb_probe() error paths
--

commit ad41050e8b81bf8d1dd6e4fe6253604d235fd347
Author: Aaradhana Sahu <aaradhana.sahu@oss.qualcomm.com>

wifi: ath12k: Add platform MSI support for CE

This commit adds support for configuring Copy Engine (CE) interrupts
through platform MSI on hybrid AHB devices like the QCN6432. It introduces
an IPCI MSI configuration, allocates MSI vectors from the platform MSI
domain, and registers CE interrupt handlers using the allocated vectors.

> diff --git a/drivers/net/wireless/ath/ath12k/pcic.c b/drivers/net/wireless/ath/ath12k/pcic.c
> --- a/drivers/net/wireless/ath/ath12k/pcic.c
> +++ b/drivers/net/wireless/ath/ath12k/pcic.c
[ ... ]
> @@ -154,3 +155,157 @@ void ath12k_pcic_write32(struct ath12k_base *ab, u32 offset, u32 value)
[ ... ]
> +static int ath12k_pcic_ce_irq_config(struct ath12k_base *ab,
> +				     struct msi_desc **msi_desc, int num_msi)
> +{
[ ... ]
> +		scnprintf(ce_pipe->irq_name, sizeof(ce_pipe->irq_name),
> +			  "pcic%u_wlan_ce_%u", userpd_id, i);
> +		ret = request_irq(desc->irq, ath12k_pcic_ce_interrupt_handler,
> +				  IRQF_SHARED, ce_pipe->irq_name, ce_pipe);

[Severity: High]
Does this leak the requested IRQ on the probe error path?

Since request_irq() is used here instead of devm_request_irq(), it requires
explicit cleanup. This function is called by ath12k_ahb_config_irq() during
ath12k_ahb_probe(). If a subsequent initialization step in probe fails
(such as arch_init or core_init), the error path jumps to labels like
err_deinit_arch or err_core_free.

However, the error path in ath12k_ahb_probe() does not appear to call
ath12k_ahb_disable_irq() or ath12k_pcic_free_irq() to free these IRQs.
If the device context (ab) is subsequently freed by ath12k_core_free()
while the interrupt handlers remain registered, could this lead to a
use-after-free panic if an interrupt fires?

> +		if (ret) {
> +			ath12k_warn(ab, "failed to request irq %d: %d\n", irq_idx, ret);
> +			ath12k_pci_free_ce_irq(ab, i);
> +			return ret;
> +		}
[ ... ]
> +int ath12k_pcic_config_irq(struct ath12k_base *ab)
> +{
> +	int user_base_data, base_vector, num_vectors;
> +	struct platform_device *pdev = ab->pdev;
> +	int ret;
> +
> +	ab->msi.config = &ath12k_msi_config[ATH12K_MSI_CONFIG_IPCI];
> +
> +	ret = platform_device_msi_init_and_alloc_irqs(&pdev->dev,
> +						      ab->msi.config->total_vectors,
> +						      ath12k_msi_msg_handler);

[Severity: High]
Similarly, does this platform MSI domain allocation get leaked on the
ath12k_ahb_probe() error path?

platform_device_msi_init_and_alloc_irqs() is not devres managed, so the
domain must be explicitly destroyed via platform_device_msi_free_irqs_all().
Because ath12k_ahb_probe() does not call the corresponding free routine when
it encounters an error after this point, are these MSI vectors leaked?

> +	if (ret)
> +		return ret;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008-mpp_bringup-v1-0-2c5797b5e4ac@oss.qualcomm.com?part=11