Re: [PATCH] usb: typec: tcpm: allow external charger detection to set USB_TYPE

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] usb: typec: tcpm: tcpm_psy_set_prop() rejects USB_TYPE on
  non-PD ports
- [Medium] usb: typec: tcpm: unlocked usb_type write in
  tcpm_psy_set_prop()
- [Low] usb: typec: tcpm: inaccurate psy_desc.usb_types claim in commit
  message
--

commit 5e903d3047946eea57f9c3fe92ba30e6462da7b9
Author: Li Jun <jun.li@nxp.com>

usb: typec: tcpm: allow external charger detection to set USB_TYPE

This commit teaches tcpm_psy_set_prop() to accept POWER_SUPPLY_PROP_USB_TYPE
and adds the BC1.2 USB types to psy_desc.usb_types.

>     Teach tcpm_psy_set_prop() to accept writes of
>     POWER_SUPPLY_PROP_USB_TYPE and store the value in port->usb_type, and
>     advertise the BC1.2 USB types (SDP/DCP/CDP/ACA) in
>     psy_desc.usb_types so power_supply_set_property() callers are not
>     rejected by power_supply_has_property().

[Severity: Low]
Is the commit message accurate about how psy_desc.usb_types is checked?

power_supply_set_property() calls psy->desc->set_property() directly, and
power_supply_has_property() only checks psy->desc->properties.  Instead,
psy_desc.usb_types is used by power_supply_format_property() and
power_supply_show_enum_with_available() when formatting
POWER_SUPPLY_PROP_USB_TYPE for sysfs reads and uevents.

> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index d721baef52104..24f4e8854f37f 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -8766,6 +8766,10 @@ static int tcpm_psy_set_prop(struct power_supply *psy,
>  	case POWER_SUPPLY_PROP_CURRENT_NOW:
>  		ret = tcpm_aug_set_op_curr(port, val->intval / 1000);
>  		break;
> +	case POWER_SUPPLY_PROP_USB_TYPE:
> +		port->usb_type = val->intval;
> +		ret = 0;
> +		break;

[Severity: Medium]
Does tcpm_psy_set_prop() reject POWER_SUPPLY_PROP_USB_TYPE on non-PD ports?

At the start of tcpm_psy_set_prop(), the !port->pd_supported check still
runs unconditionally before the switch statement:

drivers/usb/typec/tcpm/tcpm.c:tcpm_psy_set_prop() {
	/*
	 * All the properties below are related to USB PD. The check needs to be
	 * property specific when a non-pd related property is added.
	 */
	if (!port->pd_supported)
		return -EOPNOTSUPP;
}

On a Type-C port with pd-disable set, power_supply_set_property() for
POWER_SUPPLY_PROP_USB_TYPE will always fail with -EOPNOTSUPP instead of
updating port->usb_type.

[Severity: Medium]
Can this unlocked write to port->usb_type race with the TCPM state machine
and overwrite a negotiated PD type or a disconnect reset?

All TCPM updates to port->usb_type in tcpm_pd_select_pdo() and
tcpm_reset_port() run under port->lock, while BC1.2 detection runs
asynchronously in PHY workqueues after tcpm_mux_set() sets USB_ROLE_DEVICE:

CPU0 (TCPM kthread)               CPU1 (PHY charger work)
tcpm_pd_select_pdo()
  mutex_lock(&port->lock)
  port->usb_type =
    POWER_SUPPLY_USB_TYPE_PD;
  mutex_unlock(&port->lock)
                                  power_supply_set_property()
                                    tcpm_psy_set_prop()
                                      port->usb_type = val->intval;
                                      <-- clobbers PD usb_type with BC1.2

If BC1.2 detection finishes after PD capability negotiation (or after a
Data Role Swap to device mode) or after unplugging the cable,
tcpm_psy_set_prop() overwrites POWER_SUPPLY_USB_TYPE_PD or
POWER_SUPPLY_USB_TYPE_C with a stale BC1.2 type because it neither holds
port->lock nor checks port->attached and port->pd_capable.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106115742.730704-1-daniel.baluta@nxp.com?part=1