Re: [PATCH 1/4] thunderbolt: add platform nhi driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] thunderbolt: duplicate module_init() across domain.c, pci.c, and plat.c
- [High] thunderbolt: invalid to_pci_dev() cast on platform device in icm_probe()
- [High] thunderbolt: missing nhi_shutdown() cleanup on nhi_probe() error paths
- [Medium] thunderbolt: double pm_runtime_disable() in nhi_plat_init() and exit
- [Low] thunderbolt: missing kernel-doc entry for irq member in struct tb_nhi
- [Low] thunderbolt: undocumented DT compatible and redundant calls in plat.c
- [Low] thunderbolt: nhi_plat_isr() in plat.c duplicates nhi_msi() in nhi.c
--

commit 27d62cb037ec7e4b78f736b27dff7bee6d842f3a
Author: Chunfeng Yun <chunfeng.yun@mediatek.com>

thunderbolt: add platform nhi driver

This commit adds a platform bus NHI driver in drivers/thunderbolt/plat.c and
converts domain.c and pci.c to use separate initcall and module_pci_driver
registrations.

> diff --git a/drivers/thunderbolt/Makefile b/drivers/thunderbolt/Makefile
> index beb054c3126b1..9c941682f3d6c 100644
> --- a/drivers/thunderbolt/Makefile
> +++ b/drivers/thunderbolt/Makefile
> @@ -3,7 +3,7 @@ ccflags-y := -I$(src)
>  obj-${CONFIG_USB4} := thunderbolt.o
>  thunderbolt-objs := nhi.o ctl.o tb.o switch.o cap.o pci.o path.o tunnel.o eeprom.o
>  thunderbolt-objs += domain.o dma_port.o icm.o property.o xdomain.o lc.o tmu.o usb4.o
> -thunderbolt-objs += usb4_port.o nvm.o retimer.o quirks.o clx.o
> +thunderbolt-objs += usb4_port.o nvm.o retimer.o quirks.o clx.o plat.o
>  
>  thunderbolt-${CONFIG_ACPI} += acpi.o
>  thunderbolt-$(CONFIG_DEBUG_FS) += debugfs.o
> diff --git a/drivers/thunderbolt/domain.c b/drivers/thunderbolt/domain.c
> index 24611f05b3cda..bec7e6ebc4a88 100644
> --- a/drivers/thunderbolt/domain.c
> +++ b/drivers/thunderbolt/domain.c
> @@ -881,7 +881,7 @@ int tb_domain_unregister_unplugged_xdomains(struct tb *tb)
>  	return ctx.n;
>  }
>  
> -int tb_domain_init(void)
> +static int __init tb_domain_init(void)
>  {
>  	int ret;
>  
> @@ -907,7 +907,7 @@ int tb_domain_init(void)
>  	return ret;
>  }
>  
> -void tb_domain_exit(void)
> +static void __exit tb_domain_exit(void)
>  {
>  	bus_unregister(&tb_bus_type);
>  	ida_destroy(&tb_domain_ida);
> @@ -917,3 +917,9 @@ void tb_domain_exit(void)
>  	tb_debugfs_exit();
>  	tb_configfs_exit();
>  }
> +
> +subsys_initcall(tb_domain_init);
> +module_exit(tb_domain_exit);
> +
> +MODULE_DESCRIPTION("Thunderbolt/USB4 domain driver");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/thunderbolt/nhi.c b/drivers/thunderbolt/nhi.c
> index e99a3fcc4a299..c0f9f1855e8f3 100644
> --- a/drivers/thunderbolt/nhi.c
> +++ b/drivers/thunderbolt/nhi.c
> @@ -1379,8 +1379,10 @@ int nhi_probe(struct tb_nhi *nhi)
>  	device_wakeup_enable(dev);
>  
>  	pm_runtime_allow(dev);
> +	device_enable_async_suspend(dev);
>  	pm_runtime_set_autosuspend_delay(dev, TB_AUTOSUSPEND_DELAY);
>  	pm_runtime_use_autosuspend(dev);
> +	pm_runtime_mark_last_busy(dev);
>  	pm_runtime_put_autosuspend(dev);
>  
>  	return 0;

[ ... ]

> diff --git a/drivers/thunderbolt/pci.c b/drivers/thunderbolt/pci.c
> index 4408229d8ac7b..91d2363007034 100644
> --- a/drivers/thunderbolt/pci.c
> +++ b/drivers/thunderbolt/pci.c
> @@ -715,7 +715,7 @@ static struct pci_device_id nhi_ids[] = {
>  };
>  
>  MODULE_DEVICE_TABLE(pci, nhi_ids);
> -MODULE_DESCRIPTION("Thunderbolt/USB4 core driver");
> +MODULE_DESCRIPTION("Thunderbolt/USB4 PCI host interface driver");
>  MODULE_LICENSE("GPL");
>  
>  static struct pci_driver nhi_driver = {
> @@ -727,26 +727,4 @@ static struct pci_driver nhi_driver = {
>  	.driver.pm = &nhi_pm_ops,
>  };
>  
> -static int __init nhi_init(void)
> -{
> -	int ret;
> -
> -	ret = tb_domain_init();
> -	if (ret)
> -		return ret;
> -
> -	ret = pci_register_driver(&nhi_driver);
> -	if (ret)
> -		tb_domain_exit();
> -
> -	return ret;
> -}
> -
> -static void __exit nhi_unload(void)
> -{
> -	pci_unregister_driver(&nhi_driver);
> -	tb_domain_exit();
> -}
> -
> -rootfs_initcall(nhi_init);
> -module_exit(nhi_unload);
> +module_pci_driver(nhi_driver);

[Severity: High]
Does this break modular builds when CONFIG_USB4=m?

In drivers/thunderbolt/Makefile, domain.o, pci.o, and plat.o are all linked
into the single thunderbolt.o module. When CONFIG_USB4=m,
subsys_initcall(tb_domain_init) and module_exit(tb_domain_exit) in domain.c,
module_pci_driver(nhi_driver) in pci.c, and
module_platform_driver(nhi_plat_driver) in plat.c all expand to definitions
of init_module and cleanup_module, causing multiple-definition linker errors
(along with duplicate MODULE_DESCRIPTION and MODULE_LICENSE entries).

Also, when CONFIG_USB4=y, decoupling tb_domain_init() from driver registration
means pci_register_driver(&nhi_driver) and
platform_driver_register(&nhi_plat_driver) will still run even if
tb_domain_init() fails, and replacing rootfs_initcall(nhi_init) with
module_pci_driver(nhi_driver) moves PCI NHI initialization back to
device_initcall, before rootfs_initcall IOMMU initialization (regressing
commit eafa717bc145 ("thunderbolt: Initialize after IOMMUs")).

Should thunderbolt.ko keep a single module init/exit pair that initializes
the domain first, registers both the PCI and platform drivers in order, and
unregisters them before calling tb_domain_exit()?

> diff --git a/drivers/thunderbolt/plat.c b/drivers/thunderbolt/plat.c
> new file mode 100644
> index 0000000000000..85a00cd391d4e
> --- /dev/null
> +++ b/drivers/thunderbolt/plat.c
> @@ -0,0 +1,155 @@

[ ... ]

> +static int nhi_plat_init(struct tb_nhi *nhi)
> +{
> +	/*
> +	 * avoid device link fail for pcie tunnel
> +	 */
> +	pm_runtime_set_active(nhi->dev);
> +	pm_runtime_get_noresume(nhi->dev);
> +	devm_pm_runtime_enable(nhi->dev);
> +
> +	return 0;
> +}
> +
> +static irqreturn_t nhi_plat_isr(int irq, void *data)
> +{
> +	struct tb_nhi *nhi = data;
> +	schedule_work(&nhi->interrupt_work);
> +	return IRQ_HANDLED;
> +}

[Severity: Low]
This isn't a bug, but nhi_plat_isr() is identical to nhi_msi() in
drivers/thunderbolt/nhi.c, which is already declared in nhi.h. Could
nhi_plat_init_irq() reuse nhi_msi() directly instead of duplicating it here?

> +
> +static int nhi_plat_init_irq(struct tb_nhi *nhi)
> +{
> +	int ret;
> +
> +	INIT_WORK(&nhi->interrupt_work, nhi_interrupt_work);
> +
> +	ret = devm_request_irq(nhi->dev, nhi->irq, nhi_plat_isr, IRQF_NO_SUSPEND,
> +			       dev_name(nhi->dev), nhi);
> +	if (ret)
> +		return dev_err_probe(nhi->dev, ret, "request irq failed\n");
> +
> +	return 0;
> +}
> +
> +static void nhi_plat_exit(struct tb_nhi *nhi)
> +{
> +	devm_free_irq(nhi->dev, nhi->irq, nhi);
> +	flush_work(&nhi->interrupt_work);
> +
> +	pm_runtime_put_noidle(nhi->dev);
> +	pm_runtime_disable(nhi->dev);
> +	pm_runtime_set_suspended(nhi->dev);
> +}

[Severity: Medium]
Won't calling devm_pm_runtime_enable(nhi->dev) in nhi_plat_init() and
pm_runtime_disable(nhi->dev) in nhi_plat_exit() disable runtime PM twice on
driver removal or probe failure?

When nhi_plat_init() calls devm_pm_runtime_enable(nhi->dev) (whose return
value is also unchecked), devres registers pm_runtime_disable_action() to
call pm_runtime_disable() automatically on detach. When nhi_plat_remove() ->
nhi_shutdown() -> nhi_plat_exit() calls pm_runtime_disable(nhi->dev)
manually, and devres then runs pm_runtime_disable_action(),
dev->power.disable_depth is incremented to 2, leaving runtime PM disabled if
the device is bound again.

[Severity: High]
What happens if nhi_probe() fails after nhi_plat_init_irq() or
nhi_plat_init(), but before tb_domain_add()?

In drivers/thunderbolt/nhi.c:

nhi_probe()
    nhi->ops->init_interrupts(nhi); -> nhi_plat_init_irq()
    dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
    nhi->ops->init(nhi); -> nhi_plat_init()
    tb = nhi_select_cm(nhi);
    if (!tb)
        return dev_err_probe(...);

If dma_set_mask_and_coherent(), nhi->ops->init(), or nhi_select_cm() fails,
nhi_probe() returns without calling nhi_shutdown(nhi). As a result,
nhi_plat_exit() is not called: the runtime PM reference acquired by
pm_runtime_get_noresume(nhi->dev) in nhi_plat_init() is leaked, and devres
will free the IRQ and the devm-allocated struct tb_nhi without calling
flush_work(&nhi->interrupt_work), which can leave nhi_interrupt_work running
on freed memory.

> +
> +static const struct tb_nhi_ops nhi_plat_default_ops = {
> +	.init = nhi_plat_init,
> +	.shutdown = nhi_plat_exit,
> +	.init_interrupts = nhi_plat_init_irq,
> +};
> +
> +static int nhi_plat_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct tb_nhi *nhi;
> +
> +	nhi = devm_kzalloc(dev, sizeof(*nhi), GFP_KERNEL);
> +	if (!nhi)
> +		return -ENOMEM;
> +
> +	nhi->dev = dev;
> +	nhi->ops = &nhi_plat_default_ops;
> +
> +	nhi->iobase = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(nhi->iobase))
> +		return PTR_ERR(nhi->iobase);
> +
> +	nhi->irq = platform_get_irq(pdev, 0);
> +	if (nhi->irq < 0)
> +		return dev_err_probe(dev, nhi->irq, "get irq failed\n");
> +
> +	return nhi_probe(nhi);
> +}

[Severity: High]
Can nhi_plat_probe() -> nhi_probe() -> nhi_select_cm() -> icm_probe() cause
an out-of-bounds read on nhi->dev?

In nhi_plat_probe(), nhi->dev is &pdev->dev (a struct platform_device).
nhi_probe() calls nhi_select_cm(nhi):

drivers/thunderbolt/nhi.c:nhi_select_cm() {
    ...
    if (!tb_acpi_is_native()) {
        tb = icm_probe(nhi);
        if (tb)
            return tb;
    }
    ...
}

When CONFIG_ACPI=y, tb_acpi_is_native() returns false on all Device Tree
boots (such as on MediaTek SoCs) as well as on ACPI boots where firmware has
not granted native USB4 control. icm_probe() then unconditionally casts
nhi->dev to struct pci_dev:

drivers/thunderbolt/icm.c:icm_probe() {
    struct pci_dev *pdev = to_pci_dev(nhi->dev);
    ...
    switch (pdev->device) {

Because nhi->dev is embedded in a struct platform_device rather than a
struct pci_dev, to_pci_dev(nhi->dev) computes an invalid container pointer
and reads pdev->device out of bounds before the start of the platform_device
allocation. Should icm_probe() check dev_is_pci(nhi->dev) first?

[ ... ]

> +#ifdef CONFIG_OF
> +static const struct of_device_id nhi_plat_of_match[] = {
> +	{ .compatible = "thunderbolt-platform" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, nhi_plat_of_match);
> +#endif
> +
> +#ifdef CONFIG_ACPI
> +static const struct acpi_device_id nhi_plat_acpi_match[] = {
> +	{ "ACPI0015", 0 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(acpi, nhi_plat_acpi_match);
> +#endif

[Severity: Low]
This isn't a runtime bug, but there are a few binding and cleanup issues here:

1. The commit message states this driver is for MediaTek mt8901/mt8992 SoCs,
   but nhi_plat_of_match uses a generic "thunderbolt-platform" compatible
   string that is not documented in Documentation/devicetree/bindings/, and
   nhi_plat_acpi_match adds "ACPI0015" without mention in the commit message.
   Should the DT compatible use a documented SoC-specific binding?

2. In nhi_plat_probe(), platform_get_irq() already calls dev_err_probe() on
   failure, so calling dev_err_probe() again on nhi->irq < 0 logs a duplicate
   message. Similarly, nhi_plat_init_irq() calls dev_err_probe() and its
   caller nhi_probe() calls dev_err_probe() again on the returned error.

3. In nhi_probe(), calling pm_runtime_mark_last_busy(dev) right before
   pm_runtime_put_autosuspend(dev) is redundant because
   pm_runtime_put_autosuspend() already calls pm_runtime_mark_last_busy().

[ ... ]

> diff --git a/include/linux/thunderbolt.h b/include/linux/thunderbolt.h
> index 57502da290805..212c8fea65711 100644
> --- a/include/linux/thunderbolt.h
> +++ b/include/linux/thunderbolt.h
> @@ -532,6 +532,7 @@ struct tb_nhi {
>  	u32 *interrupt_mask;
>  	bool going_away;
>  	bool iommu_dma_protection;
> +	int irq;
>  	struct work_struct interrupt_work;
>  	u32 hop_count;
>  	unsigned long quirks;

[Severity: Low]
struct tb_nhi has a kernel-doc comment block right above its definition.
Could @irq be added to that kernel-doc comment so W=1 builds do not warn
about an undescribed struct member?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030082539.1208733-1-chunfeng.yun@mediatek.com?part=1