Re: [PATCH 2/2] usb: typec: Add support for Parade PS5169 USB Type-C redriver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Race condition due to early device registration before hardware and mutex initialization.
- [High] Type-C retimer device is leaked in the `ps5169_probe` error paths, leading to Use-After-Free.
- [High] Probe returns 0 (success) on invalid chip ID, causing double-frees during removal.
- [Medium] Software state cache is updated before hardware programming and validation, leading to state desynchronization.
- [Medium] Reading raw I2C bytes into a native `u16` using `regmap_raw_read` causes endianness mismatches on big-endian architectures.
- [Medium] The driver is missing `MODULE_DEVICE_TABLE` for device tree matching, preventing auto-loading.
--

--- Patch [2]: [PATCH 2/2] usb: typec: Add support for Parade PS5169 USB Type-C redriver ---
commit 09d82f46ea2293b7e9f9ce40ffd1668b1a047cd2
Author: Esteban Urrutia <esteuwu@proton.me>

usb: typec: Add support for Parade PS5169 USB Type-C redriver

Add driver for the Parade PS5169, an USB Type-C redriver capable of
handling 10Gb/s USB signals and 8.1Gb/s DisplayPort signals.

> diff --git a/drivers/usb/typec/mux/ps5169.c b/drivers/usb/typec/mux/ps5169.c
> index 0000000000000..3ee04843fa2dc
> --- /dev/null
> +++ b/drivers/usb/typec/mux/ps5169.c
[ ... ]
> @@ -242,28 +242,21 @@ static int ps5169_retimer_set(struct typec_retimer *retimer, struct typec_retimer_state *state)
>  	struct redriver *ps5169 = typec_retimer_get_drvdata(retimer);
>  	struct typec_mux_state mux_state;
>  	int ret = 0;
>  
>  	mutex_lock(&ps5169->lock);
>  
>  	if (ps5169->mode != state->mode) {
>  		ps5169->mode = state->mode;
>  
>  		if (state->alt)
>  			ps5169->svid = state->alt->svid;
>  		else
>  			ps5169->svid = 0; // No SVID
>  
>  		ret = ps5169_set(ps5169);
>  	}

[Severity: Medium]
Does this risk desynchronizing the software state from the hardware?

If ps5169_set() fails and returns an error, the internal state (ps5169->mode)
remains updated. If the caller subsequently requests the exact same mode
again, the equality check evaluates to false.

This causes the driver to bypass validation and falsely report success while
leaving the hardware unconfigured. Should the state update be rolled back on
error, or deferred until after a successful hardware update?

>  	mutex_unlock(&ps5169->lock);
[ ... ]
> @@ -361,54 +354,47 @@ static int ps5169_probe(struct i2c_client *client)
>  	ret = regulator_enable(ps5169->vcc);
>  	if (ret) {
>  		dev_err(dev, "Failed to enable vcc-supply\n");
>  		goto mux_put;
>  	}
>  
>  	/* Reset the retimer */
>  	gpiod_set_value(ps5169->reset_gpio, 1);
>  	usleep_range(1000, 1500);
>  	gpiod_set_value(ps5169->reset_gpio, 0);
>  	usleep_range(10000, 11000);
>  
>  	ret = regmap_raw_read(ps5169->regmap, PS5169_CHIP_ID_REG, &chip_id, sizeof(chip_id));

[Severity: Medium]
Does reading raw I2C bytes directly into a native u16 variable cause endianness
mismatches on big-endian architectures?

regmap_raw_read() performs a raw byte copy, so reading a 2-byte register
directly into chip_id produces a byte order mismatch on big-endian systems.
Could a standard regmap read or explicit endianness handling be used here?

>  	if (ret) {
>  		dev_err(dev, "Failed to read chip ID\n");
>  		goto vcc_disable;
>  	}
>  
>  	/* Compare chip IDs */
>  	if (chip_id != PS5169_CHIP_ID) {
>  		dev_err(dev, "Unexpected chip ID 0x%04x\n", chip_id);
>  		goto vcc_disable;
>  	}

[Severity: High]
Is it intended to return success when the chip ID is invalid?

The code jumps to vcc_disable without updating the ret variable. Since the
preceding regmap_raw_read() just succeeded, ret equals 0. The probe function
falsely returns success to the driver core while releasing resources, leaving
a crippled device object that might be double-freed on removal.

Should ret be set to -ENODEV before the goto?

>  	/* Read chip revision */
>  	ret = regmap_raw_read(ps5169->regmap, PS5169_CHIP_REVISION_REG, &chip_revision,
[ ... ]
> @@ -395,21 +381,14 @@ static int ps5169_probe(struct i2c_client *client)
>  
>  	/* orientation-switch */
>  	switch_desc.drvdata = ps5169;
>  	switch_desc.fwnode = dev_fwnode(dev);
>  	switch_desc.set = ps5169_switch_set;
>  
>  	ps5169->sw = typec_switch_register(dev, &switch_desc);
>  	if (IS_ERR(ps5169->sw)) {
>  		ret = dev_err_probe(dev, PTR_ERR(ps5169->sw), "Failed to register switch\n");
>  		goto vcc_disable;
>  	}
>  
>  	/* retimer-switch */
>  	retimer_desc.drvdata = ps5169;
>  	retimer_desc.fwnode = dev_fwnode(dev);
>  	retimer_desc.set = ps5169_retimer_set;
>  
>  	ps5169->retimer = typec_retimer_register(dev, &retimer_desc);
>  	if (IS_ERR(ps5169->retimer)) {
>  		ret = dev_err_probe(dev, PTR_ERR(ps5169->retimer), "Failed to register retimer\n");
>  		goto switch_unregister;
>  	}
[ ... ]
>  	ps5169_init(ps5169);
>  
>  	mutex_init(&ps5169->lock);
>  
>  	i2c_set_clientdata(client, ps5169);
>  
>  	return 0;

[Severity: High]
Does registering the typec_switch and typec_retimer before initializing the
hardware and mutex expose a race window?

The subsystem can immediately invoke the .set callbacks (ps5169_switch_set,
ps5169_retimer_set) before probe() finishes, causing the callbacks to attempt
to lock an uninitialized mutex and write to unconfigured hardware.

Should ps5169_init() and mutex_init() happen before device registration?

>  switch_unregister:
>  	typec_switch_unregister(ps5169->sw);
>  
>  vcc_disable:
>  	regulator_disable(ps5169->vcc);

[Severity: High]
Does this error path leak the retimer device?

If property parsing (like parade,dp-eq-level) fails after
typec_retimer_register() succeeds, the function jumps to the switch_unregister
label. The cleanup path unregisters the switch but omits calling
typec_retimer_unregister().

The retimer device object remains registered with the Type-C framework while
the parent memory block is freed, leading to a potential use-after-free.

>  mux_put:
>  	typec_mux_put(ps5169->typec_mux);
[ ... ]
> @@ -518,17 +490,10 @@ static void ps5169_remove(struct i2c_client *client)
>  	typec_mux_put(ps5169->typec_mux);
>  	typec_switch_put(ps5169->typec_switch);
>  }
>  
>  static const struct of_device_id ps5169_of_match_table[] = {
>  	{ .compatible = "parade,ps5169" },
>  	{ },
>  };

[Severity: Medium]
Is the driver missing MODULE_DEVICE_TABLE for device tree matching?

The kernel build system will not extract the device tree match information into
the module aliases without it, which might prevent user-space tools like udev
from auto-loading this driver when the hardware is discovered.

>  static struct i2c_driver ps5169_driver = {
>  	.driver = {
>  		.name = "ps5169",
>  		.of_match_table = ps5169_of_match_table,
>  	},

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260727-ps5169-bup-v1-0-c76a8ef3b239@proton.me?part=2