Re: [PATCH v2 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 4 potential issue(s) to consider:
- [High] The Type-C switch and retimer are registered before their dependencies are initialized, leading to potential crashes or lockdep splats.
- [High] Failed hardware configurations corrupt the cached software state, preventing future validation and retries.
- [Medium] Silent failures due to ignored I2C/regmap error returns during device configuration.
- [Low] Incorrect endianness macro used for memory-to-CPU conversion.
--

--- Patch [2]: [PATCH v2 2/2] usb: typec: Add support for Parade PS5169 USB Type-C redriver ---
commit 63274a3d684029a0da1c1e47cb9856c0a759ba57
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..74cc1504dd357
> --- /dev/null
> +++ b/drivers/usb/typec/mux/ps5169.c
[ ... ]
> +static void ps5169_write(struct redriver *ps5169, unsigned int reg, unsigned int val)
> +{
> +	regmap_write(ps5169->regmap, reg, val);

[Severity: Medium]
Does this function need to handle errors from regmap_write? The error codes
are currently discarded, which might cause the hardware state to fall out of
sync with the software state if an I2C transaction fails.

> +
> +	if (val & PS5169_CONFIG_DP_EN) {
> +		/* Clearing this bit will enable the AUX channel */
> +		regmap_clear_bits(ps5169->regmap, PS5169_AUX_MONITORING_REG,
> +				  PS5169_AUX_MONITORING_BIT);
[ ... ]
> +static int ps5169_set(struct redriver *ps5169)
> +{
> +	bool reverse = (ps5169->orientation == TYPEC_ORIENTATION_REVERSE);
> +	unsigned int val = PS5169_CONFIG_BASE;
> +
> +	switch (ps5169->mode) {
> +	case TYPEC_STATE_SAFE:
> +		ps5169_write(ps5169, PS5169_CONFIG_REG, val);
> +
> +		return 0;

[Severity: Medium]
Is it intentional to return success unconditionally here even if the underlying
I2C writes in ps5169_write() failed?

[ ... ]
> +static int ps5169_switch_set(struct typec_switch_dev *sw, enum typec_orientation orientation)
> +{
> +	struct redriver *ps5169 = typec_switch_get_drvdata(sw);
> +	int ret;
> +
> +	ret = typec_switch_set(ps5169->typec_switch, orientation);
> +	if (ret)
> +		return ret;
> +
> +	mutex_lock(&ps5169->lock);
> +
> +	if (ps5169->orientation != orientation) {
> +		ps5169->orientation = orientation;
> +
> +		ret = ps5169_set(ps5169);
> +	}

[Severity: High]
If ps5169_set() fails here, ps5169->orientation is already updated. Will this
prevent future validation and retries since the driver might skip hardware
configuration on a subsequent retry with the same orientation?

> +
> +	mutex_unlock(&ps5169->lock);
> +
> +	return ret;
> +}
> +
> +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: High]
Similar to ps5169_switch_set(), if ps5169_set() fails here, ps5169->mode and
ps5169->svid are already updated. Does this need to be reverted on failure to
prevent desynchronization?

Also, is it intentional that SVID updates are completely ignored if the mode
does not change?

> +
> +	mutex_unlock(&ps5169->lock);
[ ... ]
> +static int ps5169_probe(struct i2c_client *client)
> +{
[ ... ]
> +	ret = regmap_raw_read(ps5169->regmap, PS5169_CHIP_ID_REG, &chip_id, sizeof(chip_id));
> +	if (ret) {
> +		dev_err(dev, "Failed to read chip ID\n");
> +		goto vcc_disable;
> +	}
> +	chip_id = cpu_to_le16(chip_id);

[Severity: Low]
Could this be le16_to_cpu() instead of cpu_to_le16()? While mathematically
identical, cpu_to_le16() assigns a little-endian type (__le16) back to a
CPU-native u16 variable, which might cause sparse warnings and semantic
confusion.

> +
> +	/* Compare chip IDs */
> +	if (chip_id != PS5169_CHIP_ID) {
> +		dev_err(dev, "Unexpected chip ID 0x%04x\n", chip_id);
> +		ret = -EINVAL;
> +		goto vcc_disable;
> +	}
> +
> +	/* Read chip revision */
> +	ret = regmap_raw_read(ps5169->regmap, PS5169_CHIP_REVISION_REG, &chip_revision,
> +			      sizeof(chip_revision));
> +	if (ret) {
> +		dev_err(dev, "Failed to read chip revision\n");
> +		goto vcc_disable;
> +	}
> +	dev_dbg(dev, "Found PS5169 with chip revision 0x%04x\n", cpu_to_le16(chip_revision));

[Severity: Low]
Similar to the chip_id reading above, should this use le16_to_cpu() when
passing the value to the string format expecting a CPU-native unsigned int?

> +
> +	ret = drm_aux_bridge_register(dev);
[ ... ]
> +	/* 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;
> +	}
> +
> +	/* Read properties from DT */
> +	ps5169->aux_rx_data_reverse = device_property_read_bool(dev, "parade,aux-rx-data-reverse");
> +	ps5169->tune_lfps = device_property_read_bool(dev, "parade,fine-tune-lfps-swing");
> +	ps5169->dp_set_gain = device_property_read_bool(dev, "parade,dp-set-gain");
> +	ps5169->usb_rx_set_gain = device_property_read_bool(dev, "parade,usb-rx-set-gain");
> +	ps5169->usb_tx_set_gain = device_property_read_bool(dev, "parade,usb-tx-set-gain");
> +
> +	ps5169_init(ps5169);
> +
> +	mutex_init(&ps5169->lock);

[Severity: High]
Can the subsystem invoke a state change callback (like ps5169_switch_set or
ps5169_retimer_set) as soon as the switch and retimer are registered? If so,
could those callbacks execute before mutex_init(&ps5169->lock) is called here,
leading to a crash or lockdep splat since they attempt to lock this uninitialized
mutex?

> +
> +	i2c_set_clientdata(client, ps5169);
> +
> +	return 0;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260729-ps5169-bup-v2-0-203b0a49881a@proton.me?part=2