Re: [PATCH v1 05/15] net/xsc: add event queues, interrupts and the

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] net/xsc: deadlock in xsc_cmd_use_polling() if xsc_cmd_enable_msix() times out
- [Low] net/xsc: missing select AUXILIARY_BUS in Kconfig for xsc_adev_add()
--

commit bbb498d789473438951b00cc57ce44b8544a92ad
Author: Xin Tian <tianx@yunsilicon.com>

net/xsc: add event queues, interrupts and the

This commit adds event queue support, MSI-X interrupt allocation and event
dispatch, DMA buffer allocation helpers, and auxiliary bus device registration
for the ethernet and RDMA function drivers.

> diff --git a/drivers/net/ethernet/yunsilicon/xsc/pci/adev.c b/drivers/net/ethernet/yunsilicon/xsc/pci/adev.c
> new file mode 100644
> index 0000000000000..c8e8f1a9ff5e3
> --- /dev/null
> +++ b/drivers/net/ethernet/yunsilicon/xsc/pci/adev.c
> @@ -0,0 +1,142 @@
[ ... ]
> +int xsc_adev_add(struct xsc_core_device *xdev, int idx)
> +{
[ ... ]
> +	ret = auxiliary_device_init(adev);
> +	if (ret) {
> +		kfree(xsc_adev);
> +		return ret;
> +	}
> +
> +	ret = auxiliary_device_add(adev);

[Severity: Low]
In drivers/net/ethernet/yunsilicon/xsc/pci/Makefile, adev.o is unconditionally
linked into xsc_pci.o when CONFIG_YUNSILICON_XSC_PCI is enabled, and
xsc_adev_add() calls auxiliary_device_init() and auxiliary_device_add().

In drivers/net/ethernet/yunsilicon/xsc/pci/Kconfig, YUNSILICON_XSC_PCI only
specifies:

config YUNSILICON_XSC_PCI
	tristate "Yunsilicon XSC PCI driver"
	depends on PCI

Since AUXILIARY_BUS is a hidden symbol without fallback stubs when disabled,
will building with CONFIG_YUNSILICON_XSC_PCI enabled and CONFIG_AUXILIARY_BUS
unselected fail at link time with undefined references to
auxiliary_device_init() and __auxiliary_device_add()? Should
YUNSILICON_XSC_PCI select AUXILIARY_BUS in Kconfig?

[ ... ]
> diff --git a/drivers/net/ethernet/yunsilicon/xsc/pci/pci_irq.c b/drivers/net/ethernet/yunsilicon/xsc/pci/pci_irq.c
> new file mode 100644
> index 0000000000000..d83f13f38e9b8
> --- /dev/null
> +++ b/drivers/net/ethernet/yunsilicon/xsc/pci/pci_irq.c
> @@ -0,0 +1,494 @@
[ ... ]
> +int xsc_irq_eq_create(struct xsc_core_device *xdev)
> +{
[ ... ]
> +	xsc_cmd_use_events(xdev);
> +	err = xsc_cmd_enable_msix(xdev);
> +	if (err) {
> +		pci_err(xdev->pdev, "xsc_cmd_enable_msix failed %d.\n", err);
> +		xsc_cmd_use_polling(xdev);
> +		goto err_clear_hints;
> +	}

[Severity: High]
Can this sequence deadlock in xsc_cmd_use_polling() if xsc_cmd_enable_msix()
times out?

In xsc_irq_eq_create(), xsc_cmd_use_events() stops the command CQ polling
kthread (cmd->cq_task) and switches cmd->mode to XSC_CMD_MODE_EVENTS before
xsc_cmd_enable_msix() issues XSC_CMD_OP_ENABLE_MSIX to the firmware.

When xsc_cmd_enable_msix() calls xsc_cmd_exec() -> _xsc_cmd_exec() ->
xsc_cmd_invoke(), cmd_work_handler() acquires one permit of cmd->sem via
down(&cmd->sem). Because polling is already stopped and MSI-X is not yet
enabled in firmware (or if MSI-X delivery fails), no completion interrupt may
arrive on cmd->irqn. When xsc_wait_func() times out, xsc_cmd_invoke() leaves
the cmd->sem permit held for the completion handler:

drivers/net/ethernet/yunsilicon/xsc/pci/cmdq.c:xsc_cmd_invoke() {
	...
	err = xsc_wait_func(xdev, ent);
	if (err == -ETIMEDOUT)
		/* leave the entry for the completion handler; the driver
		 * is marked timed out and rejects all further commands
		 */
		return err;
	...
}

The error path in xsc_irq_eq_create() then calls xsc_cmd_use_polling(), which
disables cmd->irqn and waits to acquire all cmd->max_reg_cmds permits of
cmd->sem before recreating cmd->cq_task:

drivers/net/ethernet/yunsilicon/xsc/pci/cmdq.c:xsc_cmd_use_polling() {
	...
	flush_workqueue(cmd->wq);

	disable_irq(cmd->irqn);

	for (i = 0; i < cmd->max_reg_cmds; i++)
		down(&cmd->sem);
	...
}

With one permit still held by the timed-out command and both the IRQ disabled
and the polling kthread stopped, wouldn't down(&cmd->sem) block forever? Should
xsc_cmd_enable_msix() be called while the command interface is still in
polling mode before switching to events with xsc_cmd_use_events()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104155650.37053-1-tianx@yunsilicon.com?part=5