linux-kernel - Re: [PATCH v1 1/2] platform/chrome: cros_ec

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aG88L7QXa5IU7zPE@google.com>
Date: Thu, 10 Jul 2025 04:06:07 +0000
From: Tzung-Bi Shih <tzungbi@...nel.org>
To: Radu Vele <raduvele@...gle.com>
Cc: Benson Leung <bleung@...omium.org>,
	Abhishek Pandit-Subedi <abhishekpandit@...omium.org>,
	Jameson Thies <jthies@...gle.com>,
	Andrei Kuchynski <akuchynski@...omium.org>,
	chrome-platform@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 1/2] platform/chrome: cros_ec_typec: Add role swap ops

On Wed, Jul 09, 2025 at 01:22:31PM +0000, Radu Vele wrote:
> +static int cros_typec_perform_role_swap(struct typec_port *tc_port, int target_role, u8 swap_type)
> +{
> [...]
> +	switch (swap_type) {
> +	case USB_PD_CTRL_SWAP_DATA:
> +		role = (resp.role & PD_CTRL_RESP_ROLE_DATA) ? TYPEC_HOST :
> +						TYPEC_DEVICE;
> +		break;
> +	case USB_PD_CTRL_SWAP_POWER:
> +		role = (resp.role & PD_CTRL_RESP_ROLE_POWER) ? TYPEC_SOURCE :
> +						TYPEC_SINK;
> +		break;
> +	default:
> +		dev_warn(data->dev, "Unsupported role swap type %d", swap_type);

Append a newline at the end of message?

> +		return -EOPNOTSUPP;
> +	}
> +
> +	if (role == target_role)
> +		return 0;
> +
> +	req.swap = swap_type;
> +	ret = cros_ec_cmd(data->ec, data->pd_ctrl_ver, EC_CMD_USB_PD_CONTROL,
> +				&req, sizeof(req), &resp, sizeof(resp));
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	switch (swap_type) {
> +	case USB_PD_CTRL_SWAP_DATA:
> +		typec_set_data_role(tc_port, resp.role & PD_CTRL_RESP_ROLE_DATA ?
> +									TYPEC_HOST :
> +									TYPEC_DEVICE);

To improve the readability a bit, how about re-use `role` variable here:

        role = resp.role & ... ? ... : ...;

Also I'm wondering doesn't it need to assert
(resp.role & ... ? ... : ...) == target_role here?  Otherwise, could it just
use `target_role` as the parameter for typec_set_data_role()?


> +		break;
> +	case USB_PD_CTRL_SWAP_POWER:
> +		typec_set_pwr_role(tc_port, resp.role & PD_CTRL_RESP_ROLE_POWER ?
> +									TYPEC_SOURCE :
> +									TYPEC_SINK);

Similar here.

> +		break;
> +	}

It already checks in previous code block.  Not sure if some checkers would
find the swtich-case block doesn't have a default clause.  How about still
add a default case, and have a comment to remind that should never execute?

>  static const struct typec_operations cros_typec_usb_mode_ops = {
> -	.enter_usb_mode = cros_typec_enter_usb_mode
> +	.enter_usb_mode = cros_typec_enter_usb_mode,
> +	.dr_set = cros_typec_dr_swap,
> +	.pr_set = cros_typec_pr_swap

Leave a comma "," at the end of line?