Re: [PATCH v6 6/9] usb: misc: qcom_eud: add role-based EUD control

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Incorrect devres ordering causes a use-after-unmap vulnerability during driver removal.
- [Medium] Missing rollback on downstream USB role switch failure causes permanent state desynchronization and PHY power leak.
--

--- Patch [6]: [PATCH v6 6/9] usb: misc: qcom_eud: add role-based EUD control ---
commit bc0de19142343d10fe6a60512025d0e334d88fb3
Author: Elson Serrao <elson.serrao@oss.qualcomm.com>

usb: misc: qcom_eud: add role-based EUD control

EUD functions by presenting itself as a USB device to the host PC and is
therefore only applicable when the port operates in device role. This patch
updates the driver to treat EUD as usable only in device role, modifying the
role-switch callback to enable or disable EUD accordingly.

> diff --git a/drivers/usb/misc/qcom_eud.c b/drivers/usb/misc/qcom_eud.c
> index e83a4402e4b84..538e070cc140a 100644
> --- a/drivers/usb/misc/qcom_eud.c
> +++ b/drivers/usb/misc/qcom_eud.c
[ ... ]
> @@ -304,7 +323,30 @@ static irqreturn_t handle_eud_irq_thread(int irq, void *data)
>  static int eud_role_switch_set(struct usb_role_switch *sw, enum usb_role role)
>  {
>  	struct eud_path *path = usb_role_switch_get_drvdata(sw);
> -	int ret;
> +	struct eud_chip *chip = path->chip;
> +	int ret = 0;
> +
> +	guard(mutex)(&chip->state_lock);
> +
> +	/*
> +	 * EUD is usable only in device role. Power it down for every other
> +	 * role to avoid keeping an unusable module 'ON'. chip->enabled
> +	 * preserves user's sysfs configuration and is not modified across
> +	 * role transitions.
> +	 */
> +	if (chip->enabled && path->num == chip->port_idx && role != path->curr_role) {
> +		if (role == USB_ROLE_DEVICE)
> +			ret = enable_eud(chip);
> +		else if (path->curr_role == USB_ROLE_DEVICE)
> +			ret = disable_eud(chip);
> +
> +		if (ret) {
> +			dev_err(chip->dev, "failed to %s EUD for role %s: %d\n",
> +				str_enable_disable(role == USB_ROLE_DEVICE),
> +				usb_role_string(role), ret);
> +			return ret;
> +		}
> +	}
>  
>  	/* Forward the role request to the USB controller */
>  	ret = usb_role_switch_set_role(path->controller_sw, role);

[Severity: Medium]
If the downstream controller rejects the role switch and returns an error,
does this leave the EUD hardware state out of sync?

Looking at eud_role_switch_set(), if we were transitioning to USB_ROLE_DEVICE,
we already called enable_eud(). If usb_role_switch_set_role() then fails,
the function returns without updating path->curr_role or rolling back the
physical enable:

    ret = usb_role_switch_set_role(path->controller_sw, role);
    if (ret) {
        dev_err(...);
        return ret;
    }

Since path->curr_role remains out of sync with the physical state, will future
transitions to USB_ROLE_NONE evaluate (role != path->curr_role) as false and
permanently skip calling disable_eud(), leaving the PHY powered on?

[ ... ]
> @@ -431,6 +473,8 @@ static int eud_probe(struct platform_device *pdev)
>  
>  	chip->dev = &pdev->dev;
>  
> +	mutex_init(&chip->state_lock);
> +
>  	for_each_child_of_node_scoped(np, child) {
>  		ret = eud_init_path(chip, child);
>  		if (ret)

[Severity: High]
Does registering the role switch here create a devres ordering issue with the
I/O memory mapping?

In eud_probe(), the USB role switch is registered via eud_init_path() before
chip->base is mapped via devm_platform_ioremap_resource() later in the probe
function.

Since devres callbacks run in strictly reverse order, during driver teardown
chip->base will be unmapped before the role switch is unregistered. If a
concurrent role switch event arrives during this window, eud_role_switch_set()
could see chip->enabled == true and call enable_eud(), which writes to the
already unmapped chip->base.

Could this lead to a panic or page fault during driver removal?

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