linux-kernel - Re: [PATCH v4 2/2] iio: adc: Add the NXP SAR ADC support for the s32g2/3 platforms

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1bfa9a86-0a80-416a-b653-8d14f5ebd891@linaro.org>
Date: Fri, 17 Oct 2025 11:01:43 +0200
From: Daniel Lezcano <daniel.lezcano@...aro.org>
To: Jonathan Cameron <jic23@...nel.org>
Cc: dlechner@...libre.com, nuno.sa@...log.com, andy@...nel.org,
 robh@...nel.org, conor+dt@...nel.org, krzk+dt@...nel.org,
 linux-iio@...r.kernel.org, s32@....com, linux-kernel@...r.kernel.org,
 devicetree@...r.kernel.org, chester62515@...il.com, mbrugger@...e.com,
 ghennadi.procopciuc@....nxp.com
Subject: Re: [PATCH v4 2/2] iio: adc: Add the NXP SAR ADC support for the
 s32g2/3 platforms

On 9/20/25 11:27, Jonathan Cameron wrote:

[ ... ]

>> +static void nxp_sar_adc_remove(struct platform_device *pdev)
>> +{
>> +	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>> +	struct nxp_sar_adc *info = iio_priv(indio_dev);
>> +
>> +	nxp_sar_adc_stop_conversion(info);
> 
> I assume this is safe even if no start has happened and is here so
> that the driver remove can run whilst buffered capture is still going on?
> That should be done by the core as part of unwinding the register().
> So I'd not expect a need for it here. This may be a side effect of the
> ordering issue mixing devm and not causes.  The same is true of some
> of these other calls - I haven't checked them all.
> 
>> +	nxp_sar_adc_channels_disable(info, NXP_SAR_ADC_CH_MASK);
>> +	nxp_sar_adc_dma_channels_disable(info, NXP_SAR_ADC_CH_MASK);
>> +	nxp_sar_adc_dma_cfg(info, false);
>> +	nxp_sar_adc_disable(info);
>> +	dmaengine_terminate_sync(info->dma_chan);
> 
> Mixing devm and non devm is a never a good idea. Here one possible issue is that
> the userspace interfaces are only torn down when devm unwind gets to unwind
> devm_iio_device_register();  That happens well after this code has ripped down the
> dma engine that a channel read will try to use.  It might be safe to do that
> but it certainly makes the driver harder to reason about.
> 
> A simple 'rule' is once you first call a non devm function in probe that needs unwinding
> in remove, you cannot call any more devm functions.   Whilst there are lots of cases
> that are safe, this approach ensures none of the cases that aren't sneak in and makes
> review straight forward.
> 
> devm_add_action_or_reset() can often be used to keep the chain of devm calls running,
> sometimes covering everything such that we don't need a remove callback.
> 
>> +}

Actually I think these calls are not relevant. If we remove the 
nxp_sar_adc_remove() function, the iio core code will unregister the device.

All operations are doing on/off in the different callbacks (raw_read, 
postenable, predisable). When the device is unregistered it is not 
possible to have an ongoing conversion, a channel enabled or the adc 
enabled, as well as the DMA. IINW, we can just remove this block of code.

[ ... ]



-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog