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

[Vadillo, Miguel]

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