Re: [RFC PATCH v2 2/2] drivers/watchdog: Add RISC-V SBI watchdog driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] watchdog: riscv_sbi_wdt: missing return on dev_err_probe() in
  sbiwdt_probe()
- [Medium] watchdog: riscv_sbi_wdt: missing select WATCHDOG_CORE in
  Kconfig
--

commit 492be02d4c062c56b2b12bb8aae92692793bfd6b
Author: Prajwal N <prajwal.n@oss.qualcomm.com>

drivers/watchdog: Add RISC-V SBI watchdog driver

This commit adds a watchdog driver for the RISC-V SBI watchdog extension
with support for MSI-based pretimeout notifications.

> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index c884f86be4e37..164f4b426240e 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -2157,6 +2157,15 @@ config STARFIVE_WATCHDOG
>  	  Say Y here to support the watchdog of StarFive JH7100 and JH7110
>  	  SoC. This driver can also be built as a module if choose M.
>  
> +config RISCV_SBI_WATCHDOG
> +	tristate "RISC-V SBI based Watchdog support"
> +	depends on RISCV_SBI
> +	default RISCV
> +	help
> +	  Say Y here to include support for a watchdog timer
> +	  implemented by the Supervisor Binary Interface on RISCV platforms.
> +	  Requires firmware support.
> +

[Severity: Medium]
Should RISCV_SBI_WATCHDOG select WATCHDOG_CORE?

sbiwdt_probe() calls devm_watchdog_register_device(), so enabling
CONFIG_RISCV_SBI_WATCHDOG without CONFIG_WATCHDOG_CORE (or with
CONFIG_RISCV_SBI_WATCHDOG=y and CONFIG_WATCHDOG_CORE=m) fails to link.

> diff --git a/drivers/watchdog/riscv_sbi_wdt.c b/drivers/watchdog/riscv_sbi_wdt.c
> new file mode 100644
> index 0000000000000..577756f152ce8
> --- /dev/null
> +++ b/drivers/watchdog/riscv_sbi_wdt.c

[ ... ]

> +static int sbiwdt_probe(struct platform_device *pdev)
> +{
> +	struct sbiwdt *wdt;
> +	const struct imsic_global_config *imsic_global;
> +	struct irq_domain *msi_domain;
> +	struct device *dev = &pdev->dev;
> +	int rc;
> +
> +	/* Allocate memory for the global watchdog structure */
> +	wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL);
> +	if (!wdt)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, wdt);
> +	watchdog_set_drvdata(&wdt->wdd, wdt);
> +
> +	/* Initialiaze watchdog attributes */
> +	rc = sbiwdt_init_attributes(wdt);
> +	if (rc)
> +		dev_err_probe(dev, rc, "Failed to read SBI watchdog attributes\n");
> +
> +	wdt->wdd.parent = dev;
> +	wdt->wdd.info = &sbiwdt_info;
> +	wdt->wdd.ops = &sbiwdt_ops;
> +
> +	watchdog_stop_on_unregister(&wdt->wdd);
> +	watchdog_stop_on_reboot(&wdt->wdd);
> +
> +	/* Setup MSI if the capability is available */
> +	if (wdt->attrs.wdt_capability & SBI_WATCHDOG_CAP_MSI_SUPPORT_MASK) {
> +		imsic_global = imsic_get_global_config();
> +		if (!imsic_global)
> +			dev_err_probe(dev, -ENODEV, "IMSIC firmware node is unavailable\n");
> +
> +		msi_domain = irq_find_matching_fwnode(imsic_global->fwnode,
> +							DOMAIN_BUS_PLATFORM_MSI);
> +
> +		if (!msi_domain)
> +			dev_err_probe(dev, -ENODEV, "IMSIC platform MSI domain is unavailable\n");
> +
> +		dev_set_msi_domain(&pdev->dev, msi_domain);
> +
> +		/* MSI setup */
> +		rc = platform_device_msi_init_and_alloc_irqs(dev, 1, sbiwdt_msi_write);
> +		if (rc)
> +			dev_err_probe(dev, rc, "Failed to allocate MSIs\n");
> +
> +		/* Register with devm the MSI cleanup callback */
> +		rc = devm_add_action_or_reset(dev, sbiwdt_msi_free_irqs, dev);
> +		if (rc)
> +			return dev_err_probe(dev, rc, "Failed to add sbiwdt MSI cleanup\n");
> +
> +		/* Enable pretimeout only when the MSI is allocated */
> +		wdt->wdd.info = &sbiwdt_pretimeout_info;
> +
> +		wdt->virq = msi_get_virq(dev, 0);
> +		if (!wdt->virq)
> +			dev_err_probe(dev, -ENODEV, "Failed to get MSI virtual IRQ\n");
> +
> +		rc = devm_request_irq(dev, wdt->virq,
> +					sbiwdt_pretimeout_handler,
> +					0, "sbiwdt-pretimeout", wdt);
> +		if (rc)
> +			dev_err_probe(dev, rc, "Failed to request pretimeout IRQ\n");
> +	}
> +
> +	rc = devm_watchdog_register_device(dev, &wdt->wdd);
> +	if (rc)
> +		dev_err_probe(dev, rc, "Failed to register RISC-V SBI watchdog device\n");
> +
> +	dev_info(&pdev->dev, "RISC-V SBI Watchdog registered\n");
> +
> +	return 0;
> +}

[Severity: High]
Are seven of the eight dev_err_probe() calls in sbiwdt_probe() missing a
return statement?

Without returning, a NULL imsic_global causes an immediate NULL pointer
dereference at imsic_global->fwnode, and earlier or later failures allow
sbiwdt_probe() to continue with uninitialized attributes, broken MSI/IRQ
state, or a false 0 return that skips devres cleanup when
devm_watchdog_register_device() fails.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113121857.1640918-1-prajwal.n@oss.qualcomm.com?part=2