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

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] [day] [month] [year] [list]

Message-ID: <Z0fxrDTuCIeCDTiV@google.com>
Date: Thu, 28 Nov 2024 04:29:32 +0000
From: Tzung-Bi Shih <tzungbi@...nel.org>
To: Dawid Niedzwiecki <dawidn@...gle.com>
Cc: Benson Leung <bleung@...omium.org>, chrome-platform@...ts.linux.dev,
	linux-kernel@...r.kernel.org, chromeos-krk-upstreaming@...gle.com
Subject: Re: [PATCH 1/2] platform/chrome: cros_ec: jump to RW before probing

On Mon, Nov 25, 2024 at 06:44:45PM +0000, Dawid Niedzwiecki wrote:
> To avoid such problems, send the RWSIG continue command first, which
> skips waiting for the jump to RW. Send the command more times, to make
> sure EC is ready in RW before the start of the actual probing process. If
> a EC device doesn't support the RWSIG, it will respond with invalid
> command error code and probing will continue as usual.

I'm wondering should it only send RWSIG_ACTION_CONTINUE if EC_CMD_GET_VERSION
shows the FW is still in RO.

Curious about: who (in which use case) is responsible for sending
RWSIG_ACTION_ABORT if it wants the EC stays in RO?

> diff --git a/drivers/platform/chrome/cros_ec.c b/drivers/platform/chrome/cros_ec.c
[...]
> @@ -204,6 +204,11 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
>  	mutex_init(&ec_dev->lock);
>  	lockdep_set_class(&ec_dev->lock, &ec_dev->lockdep_key);
>  
> +	/* Send RWSIG continue to jump to RW for devices using RWSIG. */
> +	err = cros_ec_rwsig_continue(ec_dev);
> +	if (err)
> +		dev_warn(dev, "Failed to continue RWSIG: %d\n", err);

Too verbose, use dev_info() instead.

> diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
[...]
> +int cros_ec_rwsig_continue(struct cros_ec_device *ec_dev)
> +{
[...]
> +	for (int i = 0; i < RWSIG_CONTINUE_RETRIES; i++) {
> +		ret = cros_ec_send_command(ec_dev, msg);
> +
> +		if (ret < 0)
> +			error_count++;

Should it just return the error if the transmission fails?

> +		else if (msg->result == EC_RES_INVALID_COMMAND)
> +			/*
> +			 * If EC_RES_INVALID_COMMAND is retured, it means RWSIG
> +			 * is not supported or EC is already in RW, so there is
> +			 * nothing left to do.
> +			 */
> +			break;
> +		else if (msg->result != EC_RES_SUCCESS)
> +			/* Unexpected command error. */
> +			error_count++;

Same as `ret < 0`, should it just return if any unexpected errors?

> +		else
> +			/*
> +			 * The EC_CMD_RWSIG_ACTION succeed. Send the command
> +			 * more times, to make sure EC is in RW. A following
> +			 * command can timeout, because EC may need some time to
> +			 * initialize after jump to RW.
> +			 */
> +			error_count = 0;
> +
> +		if (error_count >= RWSIG_CONTINUE_MAX_ERRORS_IN_ROW)
> +			break;

It could return 0 if `error_count >= RWSIG_CONTINUE_MAX_ERRORS_IN_ROW`.

> +
> +		if (ret != -ETIMEDOUT)
> +			usleep_range(90000, 100000);
> +	}
> +
> +	kfree(msg);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(cros_ec_rwsig_continue);