Re: [PATCH v2 3/3] tcpm: Implement sink support for PD SPR AVS negotiation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] SPR AVS current requests are rounded UP and mutated in-place, which causes valid maximum boundary values to be rejected and permanently corrupts the port state.
- [High] The requested operating current and voltage for AVS are never validated against the sink's minimum operating power requirement (`operating_snk_mw`).
- [High] Activating SPR AVS fails deterministically if the current Fixed PDO voltage is below 9V, rendering the feature unusable.
- [High] Unprotected lockless checks of `active` flags in sysfs setters cause TOCTOU races, potentially activating both PPS and AVS simultaneously.
- [Medium] Incorrect VBUS discharge threshold configuration for SPR AVS due to false `pps_active` flag and hardcoded 0V minimum.
- [Medium] `port->spr_avs_data.port_snk_status` is not reset to `SPR_AVS_UNKNOWN` when capabilities are updated, leading to the use of stale SPR AVS capabilities.
--

--- Patch [3]: [PATCH v2 3/3] tcpm: Implement sink support for PD SPR AVS negotiation ---
Note: The format of this review has been altered to a free-form summary
without direct patch quoting due to code recitation restrictions.

commit dc9649cdbdf898247e40b664f584f8ccfa0a601b
Author: Badhri Jagan Sridharan <badhri@google.com>
Subject: tcpm: Implement sink support for PD SPR AVS negotiation

This patch adds support to enable TCPM to negotiate with USB PD Standard
Power Range Adjustable Voltage Supply (SPR AVS) when acting as a power sink.

[Severity: Medium]
In drivers/usb/typec/tcpm/tcpm.c:run_state_machine() during the
SNK_TRANSITION_SINK state, the code calls
tcpm_set_auto_vbus_discharge_threshold() passing port->pps_data.active as
the pps_active argument.

Since pps_data.active is false during an SPR AVS contract, doesn't this
result in applying a fixed voltage threshold instead of an adjustable one?
Also, in tcpm_set_auto_vbus_discharge_threshold(), the minimum voltage is
hardcoded to port->pps_data.min_volt (0V) instead of the AVS Tier 1 minimum
of 9V.

[Severity: High]
In drivers/usb/typec/tcpm/tcpm.c:tcpm_pd_select_spr_avs_apdo(), the
requested current req_op_curr_ma is rounded up to the nearest 50mA step
and written directly back to port->spr_avs_data.req_op_curr_ma.

Immediately after, this rounded value is checked against src_max_curr_ma
and snk_max_curr_ma. If a valid max current request (like 2260mA) is
rounded up (to 2300mA), it will exceed the maximum and return -EINVAL,
effectively rejecting valid boundary values.

Since it overwrites the state, does this also permanently corrupt the port
state for subsequent AVS requests?

[Severity: High]
In drivers/usb/typec/tcpm/tcpm.c, both tcpm_aug_set_op_curr() and
tcpm_aug_set_out_volt() perform a validation check ensuring target_mw is
not less than port->operating_snk_mw.

However, this check is placed entirely inside the PPS branch, leaving the
SPR AVS branch unchecked. Similarly, tcpm_pd_build_spr_avs_request() only
sets the RDO_CAP_MISMATCH flag but does not adjust the requested voltage or
current to meet the sink's minimum power like tcpm_pd_build_pps_request()
does.

Could this allow an AVS contract that provides less power than the system
needs to operate?

[Severity: High]
In drivers/usb/typec/tcpm/tcpm.c:tcpm_spr_avs_activate(), when activate is
true, port->spr_avs_data.req_out_volt_mv is initialized to
port->supply_voltage.

If the active fixed PDO is 5V, the request is set to 5000mV. The state
machine then calls tcpm_pd_select_spr_avs_apdo(), which strictly bounds
checks against SPR_AVS_TIER1_MIN_VOLT_MV (9000mV).

Will this cause SPR AVS activation to deterministically fail if the current
supply voltage is below 9V?

[Severity: Medium]
In drivers/usb/typec/tcpm/tcpm.c:tcpm_pd_select_pdo(),
port->pps_data.supported is reset to false before evaluating new
capabilities.

However, port->spr_avs_data.port_snk_status is not reset to
SPR_AVS_UNKNOWN.

If sink capabilities are updated and the new capabilities lack an AVS APDO,
wouldn't port_snk_status remain SPR_AVS_SUPPORTED with a stale
port_snk_pdo_index, causing tcpm_pd_select_spr_avs_apdo() to read invalid
capability data later?

[Severity: High]
In drivers/usb/typec/tcpm/tcpm.c:tcpm_psy_set_online() and
tcpm_disable_pps_avs(), the flags port->spr_avs_data.active and
port->pps_data.active are evaluated without holding port->lock.

If a sysfs write races with a state machine update that sets one of these
flags, could these lockless checks read stale data and allow both PPS and
AVS to be incorrectly activated simultaneously?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260316150301.3892223-1-badhri@google.com?part=3