Re: [PATCH] media: i2c: cvs: Add NVMem-based firmware update support

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Wed, Sep 30, 2026 at 11:21:42AM -0700, Miguel Vadillo wrote:
> Add firmware update support for the Intel CVS device using the kernel
> NVMem provider framework.
> 
> Two NVMem devices are registered per CVS device:
>  - nvm_active: read-only, exposes the active firmware version by
>    querying the device over I2C.
>  - nvm_non_active: write-only, root-only, accepts an incoming firmware
>    image staged by userspace (e.g. fwupd).
> 
> Firmware update is triggered via the nvm_authenticate sysfs attribute,
> which supports the following write values:
>  1 - Validate staged image, stream to device, and request reset
>  2 - Validate and stream image only (no reset request)
>  3 - Request reset for a previously streamed image
>  0 - Clear update state and reset_pending flag
> 
> On a successful write of 1 or 3, a KOBJ_CHANGE uevent is emitted and
> nvm_reset_pending is set to signal that a device reset is required to
> activate the new firmware.
> 
> The nvm_version attribute exposes the running firmware version in
> major.minor decimal format. The device_id attribute exposes the device
> VID:PID for identification by userspace tools.
> 
> Firmware images are streamed to the device in 256-byte or 1KB chunks
> over I2C depending on device quirks. The staging buffer is vmalloc'd
> on first write and released once the image has been streamed to the
> device, or on driver remove if no update was performed.
> 
> ABI documentation for all new sysfs attributes is added under
> Documentation/ABI/testing/sysfs-bus-i2c-devices-cvs.
> 
> The kernel does not inspect the firmware image contents. Signature
> verification and anti-rollback enforcement are performed by the CVS
> device firmware, which rejects images that fail either check.

...

> +Date:		January 2027
> +KernelVersion:	7.4

Tough deadline, but if there are nothing to address, you have a chance to land
it as expected.

...

> +static int cvs_do_fw_download(struct icvs *ctx, const u8 *buf, size_t size)
> +{
> +	struct icvs_cmd cmd = { };
> +	size_t chunk_max, chunk, pos;
> +	int ret, end_ret;
> +
> +	if (ctx->quirks & ICVS_FW_BUF_SIZE_256)
> +		chunk_max = SZ_256;
> +	else
> +		chunk_max = SZ_1K;
> +
> +	void *fw_buf __free(kfree) = kmalloc(chunk_max + sizeof(__be16),
> +					     GFP_KERNEL);

Slightly better to read in a form of

	void *fw_buf __free(kfree) =
		kmalloc(chunk_max + sizeof(__be16), GFP_KERNEL);

> +	if (!fw_buf)
> +		return -ENOMEM;
> +
> +	cmd.cmd_id = cpu_to_be16(ICVS_FW_LOADER_START);
> +	ret = cvs_send(ctx, &cmd, sizeof(cmd.cmd_id), ICVS_CMD_TIMEOUT);
> +	if (ret < 0)
> +		return ret;

What is the meaning of the positive returned value?

> +	for (pos = 0; pos < size; pos += chunk) {
> +		chunk = min(chunk_max, size - pos);
> +		put_unaligned_be16(ICVS_FW_LOADER_DATA, fw_buf);
> +		memcpy(fw_buf + sizeof(__be16), buf + pos, chunk);
> +
> +		ret = cvs_send(ctx, fw_buf, sizeof(__be16) + chunk,
> +			       ICVS_CMD_TIMEOUT);
> +		if (ret < 0) {
> +			dev_err(cvs_dev(ctx),
> +				"FW data chunk send failed: %d\n", ret);
> +			break;
> +		}
> +	}
> +
> +	/* Always send FW_LOADER_END, but keep any earlier DATA error. */
> +	cmd.cmd_id = cpu_to_be16(ICVS_FW_LOADER_END);
> +	end_ret = cvs_send(ctx, &cmd, sizeof(cmd.cmd_id), FW_END_TIMEOUT);
> +
> +	return ret < 0 ? ret : end_ret;
> +}

...

> +	mutex_lock(&ctx->lock);

Why not guard()()? Also how ACQUIRE() macros are co-habit with goto:s?

> +	ctx->nvm.auth_status = 0;
> +
> +	switch (val) {
> +	case ICVS_NVM_AUTH_CLEAR:
> +		ctx->nvm.flushed = false;
> +		break;
> +
> +	case ICVS_NVM_AUTH_WRITE_ONLY:
> +	case ICVS_NVM_AUTH_WRITE_AND_AUTH:
> +		ret = cvs_nvm_validate(ctx);
> +		if (ret)
> +			goto err_status;
> +
> +		ret = cvs_do_fw_download(ctx, ctx->nvm.buf_data_start,
> +					 ctx->nvm.buf_data_size);
> +		if (ret)
> +			goto err_status;
> +
> +		ctx->nvm.flushed = true;
> +		cvs_nvm_release_buf(&ctx->nvm);
> +
> +		if (val == ICVS_NVM_AUTH_WRITE_ONLY)
> +			break;
> +
> +		fallthrough;
> +
> +	case ICVS_NVM_AUTH_AUTH_ONLY:
> +		if (!ctx->nvm.flushed) {
> +			ret = -ENODATA;
> +			goto err_status;
> +		}
> +
> +		do_uevent = true;
> +		break;
> +	}
> +
> +	mutex_unlock(&ctx->lock);
> +
> +	if (do_uevent)
> +		kobject_uevent(&dev->kobj, KOBJ_CHANGE);
> +
> +	return count;
> +
> +err_status:
> +	ctx->nvm.auth_status = -ret;
> +	mutex_unlock(&ctx->lock);
> +
> +	return ret;

...

> +static const struct attribute_group *cvs_fw_groups[] = {
> +	&cvs_fw_group,
> +	NULL
> +};

__ATTRIBUTE_GROUPS() ?

...

>  struct icvs {
>  	struct i2c_client *i2c_client;

>  	int irq;
>  	wait_queue_head_t hostwake_event;
>  	bool hostwake_event_arg;
> +	struct icvs_nvm nvm;
>  };

Is `pahole` happy with the layout?

-- 
With Best Regards,
Andy Shevchenko