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: <20251018140225.014d858d@jic23-huawei>
Date: Sat, 18 Oct 2025 14:02:25 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Daniel Lezcano <daniel.lezcano@...aro.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 Fri, 17 Oct 2025 11:01:43 +0200
Daniel Lezcano <daniel.lezcano@...aro.org> wrote:

> 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.

Great.

> 
> [ ... ]
> 
> 
>