Re: [PATCH 2/4] usb: typec: add CIX Type-C PD controller driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 12 potential
issues to consider:
- [High] usb: typec: cix_pd: unbalanced disable_irq() on shared IRQ in
  cix_pd_remove()
- [Medium] usb: typec: cix_pd: NULL altmode passed to mux in
  cix_pd_typec_port_update()
- [Medium] usb: typec: cix_pd: invalid unconditional DP partner altmode
  registration
- [Medium] usb: typec: cix_pd: wrong USB 2.0 orientation and mux state
  in port update
- [Medium] usb: typec: cix_pd: role cached on switch failure and skipped
  on restore
- [Medium] usb: typec: Kconfig: missing USB_ROLE_SWITCH dependency for
  TYPEC_CIX_PD
- [Medium] usb: typec: cix_pd: missing .poweroff callback and broken PM
  state handling
- [Low] usb: typec: cix_pd: missing static qualifier on DEVICE_ATTR_RW()
  definition
- [Low] usb: typec: cix_pd: cix_pd_typec_parse_port_props() duplicates
  typec_get_fw_cap()
- [Low] usb: typec: cix_pd: deferred-probe churn and mux state in
  cix_pd_init_ports()
- [Low] usb: typec: cix_pd: non-static and undocumented
  disable_pd_interrupt sysfs attr
- [Low] usb: typec: cix_pd: positional struct i2c_device_id initializer
  in cix_pd_id
--

commit 580ec433cd2d8c9792fdacbbc473c28e5cbfb4c8
Author: Gary Yang <gary.yang@cixtech.com>

usb: typec: add CIX Type-C PD controller driver

This commit adds an I2C driver for the CIX Type-C PD controller (initially
supporting the Realtek RTS5453H) to report orientation, USB role, and
DisplayPort alternate mode state to the Type-C class, mux, and USB role
switch frameworks.

> diff --git a/drivers/usb/typec/Kconfig b/drivers/usb/typec/Kconfig
> index 2f80c2792dbda..b81a0d946d002 100644
> --- a/drivers/usb/typec/Kconfig
> +++ b/drivers/usb/typec/Kconfig
> @@ -110,6 +110,17 @@ config TYPEC_WUSB3801
>  	  If you choose to build this driver as a dynamically linked module, the
>  	  module will be called wusb3801.ko.
>  
> +config TYPEC_CIX_PD
> +	tristate "CIX Type-C PD controller driver"
> +	depends on I2C
> +	select REGMAP_I2C

[Severity: Medium]
Does TYPEC_CIX_PD need a dependency on USB_ROLE_SWITCH?  Building with
CONFIG_TYPEC_CIX_PD=y and CONFIG_USB_ROLE_SWITCH=m fails to link with
undefined references to fwnode_usb_role_switch_get(),
usb_role_switch_set_role(), and usb_role_switch_put(), while
CONFIG_USB_ROLE_SWITCH=n causes fwnode_usb_role_switch_get() to return
ERR_PTR(-ENODEV) and fail cix_pd_probe() unconditionally.

> diff --git a/drivers/usb/typec/cix_pd.c b/drivers/usb/typec/cix_pd.c
> new file mode 100644
> index 0000000000000..b65e1a4760b7d
> --- /dev/null
> +++ b/drivers/usb/typec/cix_pd.c
[ ... ]
> +static const struct typec_altmode_desc cix_pd_dp_altmode = {
> +	.svid = USB_TYPEC_DP_SID,
> +	.mode = USB_TYPEC_DP_MODE,
> +};
> +
> +static int cix_pd_typec_port_update(struct cix_pd *typec)
> +	struct typec_displayport_data *dp_data = &typec->dp_data;
> +	enum typec_orientation orientation;
> +	enum usb_role role;
> +	bool connected;
> +	int ret = 0;
> +
> +	if (!(typec->data_status[1] & USB3_2_CONNECTION) &&
> +	    !(typec->data_status[2] & DP_CONNECTION))
> +		orientation = TYPEC_ORIENTATION_NONE;
> +	else if (typec->data_status[1] & CONNECTION_ORIENTATION)
> +		orientation = TYPEC_ORIENTATION_REVERSE;
> +	else
> +		orientation = TYPEC_ORIENTATION_NORMAL;
> +
> +	ret = typec_set_orientation(typec->port, orientation);
> +	if (ret)
> +		return ret;
> +
> +	if (!(typec->data_status[1] & USB3_2_CONNECTION) &&
> +	    !(typec->data_status[1] & USB2_CONNECTION))
> +		role = USB_ROLE_NONE;
> +	else if (typec->data_status[1] & USB_DATA_ROLE)
> +		role = USB_ROLE_DEVICE;
> +	else
> +		role = USB_ROLE_HOST;
> +
> +	if (typec->data_status[2] & DEBUG_ACCESSORY_ATTACHED)
> +		dev_err(typec->dev,
> +			"debug accessory mode not supported, try another usb cable\n");
> +
> +	/*
> +	 * Update the partner before building the mux state so that the DP
> +	 * alternate mode handed to the mux is always a registered one.
> +	 * Debug accessories are treated as not connected, since they are
> +	 * not supported.
> +	 */
> +	connected = !(typec->data_status[2] & DEBUG_ACCESSORY_ATTACHED) &&
> +		    ((typec->data_status[1] & (USB2_CONNECTION | USB3_2_CONNECTION)) ||
> +		     (typec->data_status[2] & DP_CONNECTION));
> +	if (!connected) {
> +		if (typec->partner) {
> +			dev_info(typec->dev, "typec port(%d): unregister partner\n",
> +				 typec->id);
> +			typec_unregister_altmode(typec->partner_altmode);
> +			typec->partner_altmode = NULL;
> +			typec_unregister_partner(typec->partner);
> +			typec->partner = NULL;
> +		}
> +	} else {
> +		if (!typec->partner) {
> +			dev_info(typec->dev, "typec port(%d): register partner\n",
> +				 typec->id);
> +			typec->partner = typec_register_partner(typec->port,
> +								&typec->desc);
> +			if (IS_ERR(typec->partner)) {
> +				dev_err(typec->dev,
> +					"typec port(%d): failed to register partner: %ld\n",
> +					typec->id, PTR_ERR(typec->partner));
> +				typec->partner = NULL;
> +			}
> +		}
> +
> +		if (typec->partner && !typec->partner_altmode) {
> +			typec->partner_altmode =
> +				typec_partner_register_altmode(typec->partner,
> +							       &cix_pd_dp_altmode);
> +			if (IS_ERR(typec->partner_altmode)) {
> +				dev_err(typec->dev,
> +					"typec port(%d): failed to register DP altmode: %ld\n",
> +					typec->id,
> +					PTR_ERR(typec->partner_altmode));
> +				typec->partner_altmode = NULL;
> +			}
> +		}
> +	}

[Severity: Medium]
Should typec->partner_altmode only be registered when DP_CONNECTION is
set?

Calling typec_partner_register_altmode() for every connected partner
exposes a DisplayPort alternate mode in sysfs even for plain USB 2.0 or
USB 3.2 devices.  In addition, cix_pd_dp_altmode leaves .vdo = 0 and the
registration runs before typec_set_data_role(), so when
CONFIG_TYPEC_DP_ALTMODE is enabled, dp_altmode_probe() fails with -EPROTO
or -ENODEV on every attach.  typec_altmode_update_active() is also never
called on typec->partner_altmode, and cix_pd_typec_parse_port_props() does
not set cap->no_mode_control = true even though firmware manages alternate
modes autonomously.

> +	if ((typec->data_status[1] & USB3_2_CONNECTION) &&
> +	    (typec->data_status[2] & DP_CONNECTION)) {
> +		/* usb + dp */
> +		dp_data->status = DP_STATUS_ENABLED;
> +		if (typec->data_status[2] & HPD_STATE)
> +			dp_data->status |= DP_STATUS_HPD_STATE;
> +		if (typec->data_status[2] & HPD_IRQ)
> +			dp_data->status |= DP_STATUS_IRQ_HPD;
> +
> +		typec->state.data = dp_data;
> +		typec->state.mode = TYPEC_DP_STATE_D;
> +		typec->state.alt = typec->partner_altmode;
> +	} else if (typec->data_status[2] & DP_CONNECTION) {
> +		/* dp only */
> +		dp_data->status = DP_STATUS_ENABLED;
> +		if (typec->data_status[2] & HPD_STATE)
> +			dp_data->status |= DP_STATUS_HPD_STATE;
> +		if (typec->data_status[2] & HPD_IRQ)
> +			dp_data->status |= DP_STATUS_IRQ_HPD;
> +		typec->state.data = dp_data;
> +		typec->state.mode = TYPEC_DP_STATE_E;
> +		typec->state.alt = typec->partner_altmode;

[Severity: Medium]
Can this pass a NULL state.alt to typec_mux_set() when state.mode is
TYPEC_DP_STATE_D or TYPEC_DP_STATE_E?

If typec_register_partner() or typec_partner_register_altmode() fails, or
if DEBUG_ACCESSORY_ATTACHED is set alongside DP_CONNECTION,
typec->partner_altmode is NULL, but execution continues into the
DP_CONNECTION branches and calls typec_mux_set() with a modal state and
state.alt == NULL, which mux drivers dereference directly (e.g.
state->alt->svid).  DEBUG_ACCESSORY_ATTACHED is also ignored when
computing orientation, role, and mux state above, despite the comment
stating that debug accessories are treated as not connected.

> +	} else if (typec->data_status[1] & USB3_2_CONNECTION) {
> +		/* usb only */
> +		dp_data->status = 0;
> +		typec->state.mode = TYPEC_STATE_USB;
> +		typec->state.alt = NULL;
> +		typec->state.data = NULL;
> +	} else {
> +		/* disconnect */
> +		dp_data->status = 0;
> +		typec->state.mode = TYPEC_STATE_SAFE;
> +		typec->state.alt = NULL;
> +		typec->state.data = NULL;
> +	}

[Severity: Medium]
Does cix_pd_typec_port_update() mishandle USB 2.0-only connections in the
orientation and mux state checks?

When USB2_CONNECTION is set without USB3_2_CONNECTION or DP_CONNECTION,
connected is true and a partner is registered, but the orientation check
at the top of the function sets TYPEC_ORIENTATION_NONE and the mux check
here falls into the disconnect branch (TYPEC_STATE_SAFE) and logs
"disconnect state" on attach.

> +	ret = typec_mux_set(typec->mux, &typec->state);
> +	if (ret)
> +		return ret;
> +
> +	if ((typec->data_status[3] & POWER_MASK) == POWER_SOURCE) {
> +		typec_set_pwr_role(typec->port, TYPEC_SOURCE);
> +	} else if ((typec->data_status[3] & POWER_MASK) == POWER_SINK) {
> +		typec_set_pwr_role(typec->port, TYPEC_SINK);
> +	} else {
> +		dev_dbg(typec->dev, "typec port(%d): unknown power state %d\n",
> +			typec->id, typec->data_status[3]);
> +	}
> +
> +	if (typec->role != role) {
> +		typec->role = role;
> +		ret = usb_role_switch_set_role(typec->role_sw, role);
> +		if (role == USB_ROLE_DEVICE)
> +			typec_set_data_role(typec->port, TYPEC_DEVICE);
> +		else if (role == USB_ROLE_HOST)
> +			typec_set_data_role(typec->port, TYPEC_HOST);
> +	}

[Severity: Medium]
If usb_role_switch_set_role() fails, does caching typec->role = role first
prevent later status updates from retrying the role switch?  It also calls
typec_set_data_role() despite the failure, and during hibernation restore
(cix_pd_restore() -> cix_pd_event_handler()), usb_role_switch_set_role()
will be skipped if the post-hibernation role matches the pre-hibernation
typec->role.

[ ... ]

> +static int cix_pd_typec_parse_port_props(struct typec_capability *cap,
> +					 struct fwnode_handle *fwnode,
> +					 struct device *dev)
> +{
> +	const char *buf;
> +	int ret;
> +
> +	memset(cap, 0, sizeof(*cap));
> +	ret = fwnode_property_read_string(fwnode, "power-role", &buf);
> +	if (ret) {
> +		dev_err(dev, "power-role not found: %d\n", ret);
> +		return ret;
> +	}
> +	ret = typec_find_port_power_role(buf);
> +	if (ret < 0)
> +		return ret;
> +	cap->type = ret;
> +	ret = fwnode_property_read_string(fwnode, "data-role", &buf);
> +	if (ret) {
> +		dev_err(dev, "data-role not found: %d\n", ret);
> +		return ret;
> +	}
> +	ret = typec_find_port_data_role(buf);
> +	if (ret < 0)
> +		return ret;
> +	cap->data = ret;
> +	ret = fwnode_property_read_string(fwnode, "try-power-role", &buf);
> +	if (ret) {
> +		dev_err(dev, "try-power-role not found: %d\n", ret);
> +		return ret;
> +	}
> +	ret = typec_find_power_role(buf);
> +	if (ret < 0)
> +		return ret;
> +	cap->prefer_role = ret;
> +	cap->fwnode = fwnode_handle_get(fwnode);
> +	cap->orientation_aware = 1;
> +	return 0;
> +}

[Severity: Low]
Could this use the exported typec_get_fw_cap() helper instead of
open-coding the fwnode property parsing?  cix_pd_typec_parse_port_props()
also requires "try-power-role" and "data-role" unconditionally, whereas
typec_get_fw_cap() and usb-connector.yaml treat "data-role" as optional
and only parse "try-power-role" for DRP ports.

[ ... ]

> +static void cix_pd_unregister_ports(struct cix_pd *typec)
> +{
> +	if (!IS_ERR_OR_NULL(typec->port)) {
> +		typec->state.alt = NULL;
> +		typec->state.mode = TYPEC_STATE_USB;
> +		typec->state.data = NULL;
> +		usb_role_switch_set_role(typec->role_sw, USB_ROLE_NONE);
> +		typec_set_orientation(typec->port, TYPEC_ORIENTATION_NONE);
> +		typec_mux_set(typec->mux, &typec->state);
[ ... ]
> +static int cix_pd_init_ports(struct cix_pd *typec)
> +{
[ ... ]
> +		typec->port = typec_register_port(dev, &typec->caps);
> +		if (IS_ERR(typec->port)) {
> +			ret = PTR_ERR(typec->port);
> +			typec->port = NULL;
> +			dev_err(dev, "Failed to register port: %d\n", ret);
> +			goto unregister_ports;
> +		}
> +
> +		desc.roles = typec->caps.data;
> +		typec->port_altmode = typec_port_register_altmode(typec->port,
> +								  &desc);
> +		if (IS_ERR(typec->port_altmode)) {
> +			ret = PTR_ERR(typec->port_altmode);
> +			typec->port_altmode = NULL;
> +			dev_err(dev, "Failed to register DP altmode: %d\n", ret);
> +			goto unregister_ports;
> +		}
> +
> +		ret = cix_pd_typec_get_handles(typec, fwnode, dev);
> +		if (ret)
> +			goto unregister_ports;
> +	}

[Severity: Low]
Should cix_pd_typec_get_handles() be called before typec_register_port(),
and should the port and altmode registration errors use dev_err_probe()?

typec_register_port() and typec_port_register_altmode() can return
ERR_PTR(-EPROBE_DEFER), so dev_err() logs error messages on deferred
probe, and deferring in cix_pd_typec_get_handles() after registering the
port causes unnecessary sysfs registration and removal churn.  Also, in
cix_pd_unregister_ports(), should typec->state.mode be set to
TYPEC_STATE_SAFE rather than TYPEC_STATE_USB when disconnecting the mux?

[ ... ]

> +static void cix_pd_sync_irq(struct cix_pd *rts)
> +{
> +	bool disable = rts->pd_int_disabled || rts->suspended;
> +
> +	if (!rts->irq_requested || rts->irq_disabled == disable)
> +		return;
> +
> +	rts->irq_disabled = disable;
> +	if (disable)
> +		disable_irq(rts->irq);
> +	else
> +		enable_irq(rts->irq);
> +}
[ ... ]
> +DEVICE_ATTR_RW(disable_pd_interrupt);

[Severity: Low]
Should DEVICE_ATTR_RW(disable_pd_interrupt) be declared static so
dev_attr_disable_pd_interrupt does not pollute the global namespace or
trigger a -Wmissing-variable-declarations warning?

Also, this sysfs attribute is undocumented in Documentation/ABI/, is
created via device_create_file() after device registration instead of
.dev_groups (or debugfs for a compliance-test knob), and its show output
("enabled" / "disabled") is rejected with -EINVAL by kstrtobool() in
disable_pd_interrupt_store().

> +static int cix_pd_probe(struct i2c_client *client)
> +{
[ ... ]
> +	if (client->irq > 0) {
> +		irq_set_status_flags(client->irq, IRQ_DISABLE_UNLAZY);
> +		ret = request_threaded_irq(client->irq, NULL, cix_pd_irq_handle,
> +					   IRQF_SHARED | IRQF_ONESHOT,
> +					   dev_name(&client->dev), rts);
> +		if (ret) {
> +			dev_err(&client->dev, "failed to request irq: %d\n", ret);
> +			goto err_unregister_ports;
> +		}
> +		rts->irq_requested = true;
> +	}
[ ... ]
> +static void cix_pd_remove(struct i2c_client *client)
> +{
> +	struct cix_pd *rts = i2c_get_clientdata(client);
> +
> +	device_remove_file(&client->dev, &dev_attr_disable_pd_interrupt);
> +
> +	mutex_lock(&rts->irq_lock);
> +	if (rts->irq_requested) {
> +		rts->suspended = true;
> +		cix_pd_sync_irq(rts);
> +		if (rts->wake_enabled) {
> +			disable_irq_wake(rts->irq);
> +			rts->wake_enabled = false;
> +		}
> +		free_irq(rts->irq, rts);
> +		rts->irq_requested = false;
> +	}
> +	mutex_unlock(&rts->irq_lock);
> +
> +	cix_pd_unregister_ports(rts);
> +}

[Severity: High]
Does cix_pd_remove() leave the shared IRQ permanently disabled for all
other controller instances sharing client->irq?

Because the interrupt is requested with IRQF_SHARED, calling
cix_pd_sync_irq() in cix_pd_remove() (or earlier via
disable_pd_interrupt_store()) calls disable_irq(rts->irq), which
increments desc->depth on the shared irq_desc, and then free_irq() is
called without a matching enable_irq().  When another instance remains
registered on the shared line, __free_irq() does not call irq_shutdown(),
leaving desc->depth elevated and the hardware interrupt masked forever for
sibling ports.

More generally, calling disable_irq() in cix_pd_sync_irq() and setting
IRQ_DISABLE_UNLAZY on a shared line affects all sibling instances:
disabling interrupts on one port via sysfs masks the line for all ports,
and in cix_pd_suspend() calling disable_irq() with IRQ_DISABLE_UNLAZY
masks the hardware IRQ before enable_irq_wake() (and increments
desc->depth once per instance, so suspend_device_irq() cannot re-enable it
when two instances share the line).  Also, irq_clear_status_flags() is not
called if request_threaded_irq() fails in cix_pd_probe(), and
cix_pd_irq_handle() unconditionally acknowledges the interrupt and returns
IRQ_HANDLED without checking whether that instance actually asserted the
shared interrupt.

> +static const struct of_device_id cix_pd_of_match[] = {
> +	{ .compatible = "realtek,rts5453h", },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, cix_pd_of_match);
> +
> +static const struct i2c_device_id cix_pd_id[] = {
> +	{ "rts5453h" },
> +	{}
> +};

[Severity: Low]
This isn't a bug, but I2C subsystem conventions prefer named initializers
for new struct i2c_device_id entries (i.e. { .name = "rts5453h" }).

[ ... ]

> +static int __maybe_unused cix_pd_restore(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct cix_pd *rts = i2c_get_clientdata(client);
> +	int ret;
> +
> +	ret = cix_pd_resume(dev);
> +	if (ret)
> +		return ret;
> +
> +	/* Re-trigger PD event if PD event occurs during std */
> +	ret = cix_pd_event_handler(rts, 0);
> +	if (ret)
> +		dev_err(&client->dev, "typec event handling error %d\n", ret);
> +
> +	return ret;
> +}
[ ... ]
> +static const struct dev_pm_ops cix_pd_pm_ops = {
> +	.suspend = pm_sleep_ptr(cix_pd_suspend),
> +	.resume = pm_sleep_ptr(cix_pd_resume),
> +	.freeze = pm_sleep_ptr(cix_pd_suspend),
> +	.thaw = pm_sleep_ptr(cix_pd_resume),
> +	.restore = pm_sleep_ptr(cix_pd_restore),
> +};

[Severity: Medium]
Is cix_pd_pm_ops missing a .poweroff callback for hibernation?

During hibernation, the PM core runs .freeze, creates the image, runs
.thaw (cix_pd_resume(), which clears rts->suspended, disables wake, and
re-enables the IRQ), and then runs .poweroff; without .poweroff, the IRQ
is left enabled and wakeup is not armed when powering off.  In addition,
cix_pd_restore() returns the internal -ESHUTDOWN error code from
cix_pd_event_handler() directly to the PM core instead of translating it
to -EIO as cix_pd_probe() does, and cix_pd_resume() does not call
cix_pd_event_handler() to refresh port state after S3 suspend.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261107011012.3655873-1-gary.yang@cixtech.com?part=2