Re: [PATCH 2/2] usb: typec: mux: gpio-sbu: enable when only used for orientation

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] The `mode_switch` property is initialized after the Type-C switch device is registered, creating a race window where concurrent callbacks use an uninitialized value.
- [Medium] The SBU lines are briefly enabled in the wrong orientation before being swapped, potentially causing a signal glitch or reversed polarity.

Pre-existing issues:
- [High] Concurrent callbacks may access freed memory if probe fails or during device removal because devres frees `sbu_mux` before active consumer references are revoked.
--

--- Patch [2]: [PATCH 2/2] usb: typec: mux: gpio-sbu: enable when only used for orientation ---
commit fb130c070f4886abd845b89bf6f060523ddd0aaf
Author: Fabrice Gasnier <fabrice.gasnier@foss.st.com>

usb: typec: mux: gpio-sbu: enable when only used for orientation

When used as orientation-switch only (no mode-switch, e.g. no altmode),
the optional enable gpio remains disabled.
Enable it from the switch_set() routine, in this case, when the
orientation is set and the enable-gpios property has been provided.

> diff --git a/drivers/usb/typec/mux/gpio-sbu-mux.c b/drivers/usb/typec/mux/gpio-sbu-mux.c
> index 1834f1a2dd9dd..4151d78e3b8d6 100644
> --- a/drivers/usb/typec/mux/gpio-sbu-mux.c
> +++ b/drivers/usb/typec/mux/gpio-sbu-mux.c

[ ... ]

> @@ -40,9 +41,13 @@ static int gpio_sbu_switch_set(struct typec_switch_dev *sw,
>  		enabled = false;
>  		break;
>  	case TYPEC_ORIENTATION_NORMAL:
> +		if (!sbu_mux->mode_switch)
> +			enabled = true;
>  		swapped = false;
>  		break;
>  	case TYPEC_ORIENTATION_REVERSE:
> +		if (!sbu_mux->mode_switch)
> +			enabled = true;
>  		swapped = true;
>  		break;
>  	}

[Severity: Medium]
Does this result in a signal glitch?

Later in gpio_sbu_switch_set(), the code updates the enable_gpio before
the select_gpio:

	if (enabled != sbu_mux->enabled)
		gpiod_set_value_cansleep(sbu_mux->enable_gpio, enabled);

	if (swapped != sbu_mux->swapped)
		gpiod_set_value_cansleep(sbu_mux->select_gpio, swapped);

If both enabled and swapped transition to true here, the switch will
briefly be enabled in the normal orientation before it is swapped to the
reverse orientation. 

Would it be safer to configure the swap GPIO before enabling the switch?

[ ... ]

> @@ -125,6 +130,8 @@ static int gpio_sbu_mux_probe(struct platform_device *pdev)
>  		return dev_err_probe(dev, PTR_ERR(sbu_mux->sw),
>  				     "failed to register typec switch\n");
>  
> +	sbu_mux->mode_switch = device_property_read_bool(dev, "mode-switch");
> +

[Severity: Medium]
Can this create a race window during initialization?

At this point, typec_switch_register() has already been called and the
device is exposed to consumers. 

If a concurrent consumer (like TCPM) invokes gpio_sbu_switch_set() right
after registration but before this property is read, it will see
sbu_mux->mode_switch as false.

Would it be better to read this property before registering the switch?

[Severity: High]
This is a pre-existing issue, but does the error path handling just below
this lead to a use-after-free?

If typec_mux_register() fails later in probe, the code unregisters the
switch but doesn't seem to synchronize with active consumers:

	sbu_mux->mux = typec_mux_register(dev, &mux_desc);
	if (IS_ERR(sbu_mux->mux)) {
		typec_switch_unregister(sbu_mux->sw);
		return dev_err_probe(...);
	}

If a concurrent consumer invokes gpio_sbu_switch_set() before the
reference is revoked, and devres synchronously frees sbu_mux after probe
returns the error, the callback will dereference the freed sbu_mux memory
when it attempts to acquire sbu_mux->lock.

A similar situation appears to happen during device removal in
gpio_sbu_mux_remove().

>  	mux_desc.drvdata = sbu_mux;
>  	mux_desc.fwnode = dev_fwnode(dev);
>  	mux_desc.set = gpio_sbu_mux_set;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260825-usb-fusb340-v1-v1-0-2191179b0661@foss.st.com?part=2