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

["Vadillo, Miguel" <miguel.vadillo@intel.com>]

On 10/1/26 11:32 AM, Andy Shevchenko wrote:
> 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.
Thanks for your review Andy,

Yeah lets see how it goes with the feedback, if needed I'll bump...>
> ...
> 
>> +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);

acked

> 
>> +	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?

There is none this was a mistake on my side. There is only 0 or 
negative. I'll fix for next version, to if (ret) and return ret ? : 
end_ret>
>> +	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?

You are right scoped_guard() should be the case here and to get rid of 
the gotos, this could be done like:
...
	scoped_guard(mutex, &ctx->lock) {
		switch (val) {
		...
		}

		nvm->auth_status = -ret;
	}

	if (ret)
		return ret;

	if (do_uevent)
		kobject_uevent(&dev->kobj, KOBJ_CHANGE);

	return count;
...

done for v2

> 
>> +	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() ?

acked

> 
> ...
> 
>>   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?

Yes. struct icvs_nvm is itself hole-free and fits in one cacheline.
That being said, there seems to be other holes in the full struct from 
the existing implementation, this order could make it better
...
	struct media_pad pads[ICVS_CSI_NUM_PADS];
	struct device_link *ipu_link;
	unsigned long quirks;
	struct gpio_desc *rst;
	struct gpio_desc *req;
	struct gpio_desc *resp;
	wait_queue_head_t hostwake_event;
	struct icvs_nvm nvm;
	struct icvs_dev_capabilities caps;
	u32 nr_of_lanes;
	enum icvs_resources res;
	int irq;
	bool prefix;
	bool hostwake_event_arg;

but maybe send as a separate patch since it is not related to the patch 
intent (?)

--
regards,
Miguel
>