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

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

On 10/2/26 12:05 AM, Andy Shevchenko wrote:
> On Thu, Oct 01, 2026 at 04:03:50PM -0700, Vadillo, Miguel wrote:
>> On 10/1/26 11:32 AM, Andy Shevchenko wrote:
>>> On Wed, Sep 30, 2026 at 11:21:42AM -0700, Miguel Vadillo wrote:
> 
> ...
> 
>>>> +	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) {
> 
> Why scoped_guard()? If you need something to be outside of the regular
> guard()(), but double check that it's indeed the case, refactor to have to
> functions, one with guard()() in it and one that wraps it.

Fair question.
AFAICS, only the KOBJ_CHANGE uevent has to stay outside the lock: fwupd 
reacts to it by reading nvm_version / nvm_authenticate, and those take 
ctx->lock.

V2 can remove them and keep only the guard()(). cvs_nvm_authenticate() 
holds the lock with guard(), nvm_authenticate_store() wraps it and emits 
the uevent after it returns.
This also simplifies the error handling.

The other three scoped_guard() users (cvs_nvm_active_read(), 
nvm_version_show(), device_id_show()) only did a memcpy() or 
sysfs_emit() afterwards, so they had no reason either. All can be plain 
guard()() in V2.

> 
>> 		switch (val) {
>> 		...
>> 		}
>>
>> 		nvm->auth_status = -ret;
>> 	}
> 
>> 	if (ret)
>> 		return ret;
>>
>> 	if (do_uevent)
>> 		kobject_uevent(&dev->kobj, KOBJ_CHANGE);
>>
>> 	return count;
> 
> ...
> 
>>>>    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
>> ...
> 
> Better by `pahole` doesn't always mean better in all aspects. You have to also
> check it in conjunction with the output of `bloat-o-meter`. And in some
> (performance-critical) cases with the runtime performance tests.

Fair enough. Measured, the reorder is code-size neutral:

	add/remove: 0/0 grow/shrink: 0/0 up/down: 0/0 (0)
	Total: Before=14547, After=14547, chg +0.00%

text/data/bss are identical too. So it saves 16 bytes per device 
instance (1304 -> 1288) at no text cost, but struct icvs is 
devm_kzalloc()'d once per device and never touched on a hot path, so 
AFAICS there is nothing to measure at runtime. This could be a separate 
patch as cleanup (?)

--
regards,
Miguel
> 
>> 	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 (?)
>