Re: [PATCH v2 2/2] PCI: qcom: Add support for Hawi

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Medium] The compatible string `qcom,hawi-pcie` violates the established Qualcomm PCIe naming convention and fails to differentiate between distinct controller instances.

Pre-existing issues:
- [High] Missing hardware readback before disabling clocks in qcom_pcie_deinit_2_7_0() can cause bus lockups.
- [High] Out-of-bounds memory read when parsing a malformed 'iommu-map' property in qcom_pcie_config_sid_1_9_0().
--

commit d9d16f2083b5ca557a88fe9cb78cb5c885342df2
Author: Matthew Leung <matthew.leung@oss.qualcomm.com>

PCI: qcom: Add support for Hawi

Add support for the Hawi platform which has two PCIe controllers: one
capable of Gen3 x2 operation and one capable of Gen4 x1 operation.

> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 11fc604898926..5fe8667fdb8f5 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -2280,6 +2280,7 @@ static int qcom_pcie_resume_noirq(struct device *dev)
>  }
>  
>  static const struct of_device_id qcom_pcie_match[] = {
> +	{ .compatible = "qcom,hawi-pcie", .data = &cfg_1_9_0 },

[Severity: Medium]
Since the commit message notes the platform has two controllers with differing
capabilities, is it intentional to use a single compatible string rather than
instance-specific ones?

Modern Qualcomm platforms typically use instance-specific compatibles like
qcom,pcie-sm8450-pcie0 and qcom,pcie-sm8450-pcie1 to manage differences in
interconnects, link speeds, and quirks. Lumping them under a single
compatible might prevent the driver from differentiating the controllers.

Additionally, the new compatible string reverses the established
qcom,pcie-<platform> naming pattern.

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a potential bus lockup
risk here:

drivers/pci/controller/dwc/pcie-qcom.c:qcom_pcie_deinit_2_7_0() {
    ...
	val = readl(pcie->parf + PARF_PHY_CTRL);
	val |= PHY_TEST_PWR_DOWN;
	writel(val, pcie->parf + PARF_PHY_CTRL);

	clk_bulk_disable_unprepare(res->num_clks, res->clks);
    ...
}

Because writel() performs a posted write, there is no guarantee that the
transaction has reached the peripheral before the clocks are removed.
Disabling the clock while a write is still propagating over the interconnect
can result in a lost write or a fatal bus lockup.

Does this need a hardware readback after the writel() to ensure completion
before disabling the clocks?

[Severity: High]
This is a pre-existing issue, but could this code cause an out-of-bounds
memory read when parsing a malformed iommu-map property?

drivers/pci/controller/dwc/pcie-qcom.c:qcom_pcie_config_sid_1_9_0() {
    ...
	of_get_property(dev->of_node, "iommu-map", &size);
	if (!size)
		return 0;
    ...
	map = kzalloc(size, GFP_KERNEL);
	if (!map)
		return -ENOMEM;

	of_property_read_u32_array(dev->of_node, "iommu-map", (u32 *)map,
				   size / sizeof(u32));
    ...
	smmu_sid_base = map[0].smmu_sid;
    ...
}

If the iommu-map property is present but smaller than the size of one entry,
kzalloc() allocates only that small size. The code then unconditionally
accesses map[0].smmu_sid, which resides at offset 8.

Would this cause an out-of-bounds memory read from the heap, potentially
triggering a kernel panic if the buffer is at the end of a slab page?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260529-hawi-pcie-v2-0-de87c6cc230c@oss.qualcomm.com?part=2