Re: [PATCH v6 2/4] iio: light: add support for veml6031x00 ALS series

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

On Fri, Aug 14, 2026 at 10:58:10AM +0200, Javier Carrasco wrote:
> On Fri Aug 14, 2026 at 9:49 AM CEST, Andy Shevchenko wrote:
> > On Thu, Aug 13, 2026 at 11:46:32AM +0200, Javier Carrasco wrote:
> >> Hello Andy, thank you again for your thorough review.
> >> On Thu Aug 13, 2026 at 8:59 AM CEST, Andy Shevchenko wrote:
> >> > On Wed, Aug 12, 2026 at 10:27:41PM +0200, Javier Carrasco wrote:

...

> >> >> +	data->regmap = devm_regmap_init_i2c(i2c, &veml6031x00_regmap_config);
> >> >> +	if (IS_ERR(data->regmap))
> >> >> +		return dev_err_probe(dev, PTR_ERR(data->regmap),
> >> >> +				     "Failed to set regmap\n");
> >> >
> >> > Is debugfs access already enabled for regmap after this call? Perhaps you want
> >> > mutex to be initialised before that?
> >>
> >> Could you please explain what you are trying to avoid? Even if the
> >> debugfs is already enabled for regmap at this point, what is the
> >> possible race condition?
> >
> > That's my question to you. If you think there is none, then we are all good.
> > Basically the Q is if that mutex (that is initialised later in the probe) is
> > required for the device IO which might be possible immediately when regmap
> > is registered.
> >
> >> The IIO device is still not registered at this
> >> point, and the registers are set to their right values in _hw_init()
> >> after the mutexes were initialized. Moving the mutex initialization a
> >> couple of lines towards the top is not an issue, but I would like to
> >> understand the reasoning behind.
> >
> > User may read (and depending on the driver case writing) the register map.
> > Would it be a problem?
> 
> That is not a problem, as no assumptions about the original values are
> made.

It's not about the values, it's about the *access* HW during that time when
there is no mutex available yet.

> >> >> +	ret = devm_mutex_init(dev, &data->scale_lock);
> >> >> +	if (ret)
> >> >> +		return ret;

...

> >> >> +err_pm_put:
> >> >> +	pm_runtime_put_noidle(dev);
> >> >
> >> > Hmm... This is usually a red flag to see a goto after devm_*() calls.
> >>
> >> Could you please elaborate on this? I am aware of the dangers of a goto
> >> after the cleanup attribute, but devm_*() calls work on a different
> >> scope, not local to the function. What could go wrong? This goto is just
> >> a way not to repeat pm_runtime_put_noidle(); return ret; and I am not
> >> strongly against repeating these two lines wherever there is a goto, but
> >> I am not sure what we are avoiding in this case.
> >
> > No, the goto here may (or may not) lead to the wrong ordering on the cleanup
> > and/or error paths. It's usually recommended to avoid such a style even if
> > it works without races in that case. That's why when switching to managed
> > resources it's highly recommended to wrap everything to be managed (in case
> > there is no native APIs available) and avoid any flow disruption (goto:s).
> 
> In this case I can't see any issue with the goto. It only leads to a
> call to pm_runtime_put_noidle(), and after returning 'ret', the
> device-managed resources are released. If the goto is removed and a
> call to pm_runtime_put_noidle() is carried out in every error path, the
> result is exactly the same as there are no operations within the probe
> execution path that affect their behavior.

Right, but again "It's usually recommended to avoid such a style...".

> I can imagine that it might be problematic in some special cases, but
> the use of devm_*() and goto jumps within a function is allowed and used
> in the official documentation [1], and in several upstream drivers in IIO
> [2][3] and other subsystems [4]. Of course, all those examples might be bad
> examples, but I could not find any reference discouraging the use of
> goto with devm_*().

> [1] https://docs.kernel.org/driver-api/usb/writing_musb_glue_layer.html

It's not kernel-wide documentation. At a brief glance it has more issues than
just that one. I would not recommend to refer and use that piece of the
documentation without fixing it first.

> [2] drivers/iio/accel/kxcjk-1013.c
> [3] drivers/iio/pressure/hid-sensor-press.c
> [4] drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c

-- 
With Best Regards,
Andy Shevchenko