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

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 14 Aug 2026 12:07:03 +0300
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

> 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.
FWIW I think the runtime pm handling under the goto is unnecessary
anyway (as we don't need to grab a reference anyway) - so with that
gone this whole debate becomes moot. (Yes I did put moot in for my own amusement)


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