Re: [PATCH 2/4] thunderbolt: add support optional dedicated wakeup irq

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] thunderbolt: wake_irq never armed for suspend due to unset can_wakeup
- [Medium] thunderbolt: wake_irq leaked on nhi_probe() error after nhi_plat_init_irq()
- [Medium] thunderbolt: nhi_plat_probe() ignores -EPROBE_DEFER for optional wake_irq
- [Low] thunderbolt: missing kernel-doc entry for wake_irq in struct tb_nhi
--

commit 0039f17f466f2a19ba5fd45e19c6de293662f588
Author: Chunfeng Yun <chunfeng.yun@mediatek.com>

thunderbolt: add support optional dedicated wakeup irq

This commit adds support for an optional dedicated wakeup IRQ to the
platform Thunderbolt NHI driver for NVIDIA N1/N2 SoCs.

> diff --git a/drivers/thunderbolt/plat.c b/drivers/thunderbolt/plat.c
> index 85a00cd391d4e..8d83b484d942c 100644
> --- a/drivers/thunderbolt/plat.c
> +++ b/drivers/thunderbolt/plat.c
> @@ -11,6 +11,7 @@
>  #include <linux/of.h>
>  #include <linux/platform_device.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/pm_wakeirq.h>
>  #include <linux/slab.h>
>  
>  #include "nhi.h"
> @@ -46,6 +47,15 @@ static int nhi_plat_init_irq(struct tb_nhi *nhi)
>  	if (ret)
>  		return dev_err_probe(nhi->dev, ret, "request irq failed\n");
>  
> +	if (nhi->wake_irq) {
> +		ret = dev_pm_set_dedicated_wake_irq(nhi->dev, nhi->wake_irq);
> +		if (ret) {
> +			dev_err_probe(nhi->dev, ret, "set wake irq %d failed\n", nhi->wake_irq);
> +			return ret;
> +		}
> +		dev_info(nhi->dev, "wake irq %d\n", nhi->wake_irq);
> +	}

[Severity: Medium]
Will nhi->wake_irq ever be armed for system suspend when dev->power.can_wakeup
has not been set on the platform device?

In nhi_plat_probe() -> nhi_probe() -> nhi_plat_init_irq(),
dev_pm_set_dedicated_wake_irq(nhi->dev, nhi->wake_irq) attaches wirq to
dev->power.wakeirq while dev->power.wakeup is still NULL. Later at the end of
nhi_probe() in drivers/thunderbolt/nhi.c:

	device_wakeup_enable(dev);

In drivers/base/power/wakeup.c, device_wakeup_enable() checks:

	if (!dev || !dev->power.can_wakeup)
		return -EINVAL;

Unlike PCI devices (where pci_pm_init() marks the device wakeup-capable),
neither the platform bus nor nhi_plat_probe() / nhi_plat_init_irq() calls
device_set_wakeup_capable(nhi->dev, true) or device_init_wakeup(nhi->dev,
true) for DT or ACPI platform devices without _PRW. As a result,
device_wakeup_enable() returns -EINVAL, dev->power.wakeup remains NULL,
device_may_wakeup() returns false in nhi_suspend_noirq() and
nhi_poweroff_noirq(), and device_wakeup_arm_wake_irqs() never arms
nhi->wake_irq during system suspend.

Should device_set_wakeup_capable(nhi->dev, true) be called before
device_wakeup_enable(dev) is invoked?

> +
>  	return 0;
>  }
>  
> @@ -53,6 +63,7 @@ static void nhi_plat_exit(struct tb_nhi *nhi)
>  {
>  	devm_free_irq(nhi->dev, nhi->irq, nhi);
>  	flush_work(&nhi->interrupt_work);
> +	dev_pm_clear_wake_irq(nhi->dev);

[Severity: Medium]
Does this leak the dedicated wake_irq if nhi_probe() fails after
nhi_plat_init_irq() succeeds?

Unlike devm_request_irq() used for nhi->irq, dev_pm_set_dedicated_wake_irq()
is not devres-managed: it allocates struct wake_irq, requests a threaded IRQ,
and sets dev->power.wakeirq. However, dev_pm_clear_wake_irq(nhi->dev) is only
called in nhi_plat_exit() via nhi_shutdown().

In drivers/thunderbolt/nhi.c, nhi_probe() has several error paths after
nhi->ops->init_interrupts(nhi) that return directly without calling
nhi_shutdown():

	res = nhi->ops->init_interrupts(nhi);
	if (res)
		return dev_err_probe(dev, res, "cannot enable interrupts, aborting\n");

	spin_lock_init(&nhi->lock);

	res = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
	if (res)
		return dev_err_probe(dev, res, "failed to set DMA mask\n");

	if (nhi->ops->init) {
		res = nhi->ops->init(nhi);
		if (res)
			return dev_err_probe(dev, res, "NHI specific init failed\n");
	}

	init_completion(&nhi->domain_released);

	tb = nhi_select_cm(nhi);
	if (!tb)
		return dev_err_probe(dev, -ENODEV,
			"failed to determine connection manager, aborting\n");

If any of those steps fail, the requested threaded wake IRQ and struct
wake_irq remain allocated and dev->power.wakeirq stays set, causing any later
probe attempt to fail with -EEXIST or -EBUSY. Could devm_add_action_or_reset()
or proper error unwinding be used to ensure dev_pm_clear_wake_irq() is called
on probe failure?

>  
>  	pm_runtime_put_noidle(nhi->dev);
>  	pm_runtime_disable(nhi->dev);
> @@ -85,6 +96,11 @@ static int nhi_plat_probe(struct platform_device *pdev)
>  	if (nhi->irq < 0)
>  		return dev_err_probe(dev, nhi->irq, "get irq failed\n");
>  
> +	/* ignore error */
> +	nhi->wake_irq = platform_get_irq_optional(pdev, 1);
> +	if (nhi->wake_irq < 0)
> +		nhi->wake_irq = 0;

[Severity: Medium]
Can this silently drop -EPROBE_DEFER when the optional wake_irq provider is
not ready yet?

In nhi_plat_probe(), platform_get_irq_optional(pdev, 1) returns -ENXIO when
the optional interrupt is absent, but returns -EPROBE_DEFER (or another
negative error code) if the interrupt is specified in firmware and its
interrupt controller has not probed yet. Clamping all negative values to 0
causes probe to proceed without the wakeup IRQ instead of deferring.

Should this only ignore -ENXIO and return dev_err_probe() on other negative
errors?

> diff --git a/include/linux/thunderbolt.h b/include/linux/thunderbolt.h
> index 212c8fea65711..06b90d452ee24 100644
> --- a/include/linux/thunderbolt.h
> +++ b/include/linux/thunderbolt.h
> @@ -533,6 +533,7 @@ struct tb_nhi {
>  	bool going_away;
>  	bool iommu_dma_protection;
>  	int irq;
> +	int wake_irq;

[Severity: Low]
This isn't a bug, but struct tb_nhi in include/linux/thunderbolt.h has a
kernel-doc comment block above it; should @wake_irq be documented there to
avoid a kernel-doc warning under W=1 builds?

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