Re: [PATCH v8 4/8] usb: misc: qcom_eud: add per-path role switch support

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

New issues:
- [Medium] The driver registers its USB role switch with the incorrect fwnode for legacy device trees, preventing USB connectors from finding the switch.
- [Medium] Dangling fwnode pointer in the registered USB role switch due to missing device node refcount increment.

Pre-existing issues:
- [High] The hardirq handler calls a function that sleeps, leading to a kernel panic.
--

commit 9414ff0b2c9f7c78a66f0bce1fe95af108a567fa
Author: Elson Serrao [off-list ref]

usb: misc: qcom_eud: add per-path role switch support

The EUD hardware can support multiple High-Speed USB paths, each connected
to different USB controllers. Restructure the driver to support per-path
role switches and remove the chip-level role switch. Additionally, remove
the unnecessary role switch call from enable_eud().

[ ... ]

[Severity: High]
This is a pre-existing issue and was not introduced by the patch under
review, but does handle_eud_irq() end up sleeping in a hardirq context?

Looking at the driver's interrupt handling, handle_eud_irq() is the
primary hardirq handler (top-half). When it receives an EUD_INT_SAFE_MODE
interrupt, it calls pet_eud():

drivers/usb/misc/qcom_eud.c:handle_eud_irq() {
    ...
	case EUD_INT_SAFE_MODE:
		pet_eud(chip);
		return IRQ_HANDLED;
    ...
}

However, pet_eud() invokes readl_poll_timeout() which internally calls
usleep_range() and attempts to sleep:

drivers/usb/misc/qcom_eud.c:pet_eud() {
    ...
		ret = readl_poll_timeout(chip->base + EUD_REG_SW_ATTACH_DET,
					reg, (reg == 0), 1, 100);
    ...
}

Can this lead to a kernel panic due to scheduling while atomic when the
hardware generates a safe mode interrupt?

 static irqreturn_t handle_eud_irq_thread(int irq, void *data)
 {

[ ... ]

[ ... ]

+	role_sw_desc.fwnode = of_fwnode_handle(np);

[Severity: Medium]
When processing a legacy device tree, does this assign the wrong fwnode
to the role switch?

In a legacy DT, np points to the ports node instead of the main eud
device node. By setting the fwnode to the ports node here, the role
switch is registered with that specific fwnode.

However, when a connector driver searches for its remote role switch,
of_fwnode_graph_get_port_parent() explicitly skips nodes named "ports" and
returns the parent device node instead. Does this mismatch cause the
connector framework to fail to find the EUD's role switch, breaking the
role-relay functionality on platforms using legacy DT bindings?

+	role_sw_desc.set = eud_role_switch_set;
+	role_sw_desc.driver_data = path;
+	snprintf(path->name, sizeof(path->name), "eud-path%u", path_num);
+	role_sw_desc.name = path->name;
+
+	path->eud_sw = usb_role_switch_register(chip->dev, &role_sw_desc);

[Severity: Medium]
Does this leave a dangling fwnode pointer in the registered USB role switch?

Looking at eud_probe(), it iterates over child nodes using:

for_each_child_of_node_scoped(np, child) {
    ret = eud_init_path(chip, child);
    ...
}

The child device node pointer is passed to eud_init_path() as np, where
its fwnode is directly passed to usb_role_switch_register().
usb_role_switch_register() stores the fwnode but does not increment its
reference count.

Since for_each_child_of_node_scoped() automatically drops the reference on
the child node when the loop advances or exits, does the registered
role switch retain a pointer to a device node with a decremented reference
count? For dynamic DT overlays, could this lead to a Use-After-Free upon
role switch removal or sysfs access?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260912225301.3396556-1-elson.serrao@oss.qualcomm.com?part=4