linux-kernel - Re: [PATCH v3 1/6] iio: adc: meson_saradc: Don't attach managed resource to IIO device object

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]
Date:   Fri, 3 Jun 2022 17:29:20 +0100
From:   Jonathan Cameron <jic23@...nel.org>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc:     linux-iio@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-amlogic@...ts.infradead.org, linux-kernel@...r.kernel.org,
        Lars-Peter Clausen <lars@...afoo.de>,
        Neil Armstrong <narmstrong@...libre.com>,
        Kevin Hilman <khilman@...libre.com>,
        Jerome Brunet <jbrunet@...libre.com>,
        Martin Blumenstingl <martin.blumenstingl@...glemail.com>
Subject: Re: [PATCH v3 1/6] iio: adc: meson_saradc: Don't attach managed
 resource to IIO device object

On Fri, 3 Jun 2022 17:23:07 +0100
Jonathan Cameron <jic23@...nel.org> wrote:

> On Fri, 3 Jun 2022 17:06:12 +0100
> Jonathan Cameron <jic23@...nel.org> wrote:
> 
> > On Fri,  3 Jun 2022 12:59:59 +0300
> > Andy Shevchenko <andriy.shevchenko@...ux.intel.com> wrote:
> >   
> > > It feels wrong and actually inconsistent to attach managed resources
> > > to the IIO device object, which is child of the physical device object.
> > > The rest of the ->probe() calls do that against physical device.
> > > 
> > > Resolve this by reassigning managed resources to the physical device object.
> > > 
> > > Fixes: 3adbf3427330 ("iio: adc: add a driver for the SAR ADC found in Amlogic Meson SoCs")
> > > Suggested-by: Lars-Peter Clausen <lars@...afoo.de>
> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>    
> > Hi Andy,
> > 
> > This has come up a few times in the past (and we elected to not clean it up
> > at the time, though it wasn't a decision to never do so!)
> > 
> > It would definitely be wrong if we had another driver binding against
> > the resulting created device (funnily enough I reported a bug on a driver
> > doing just that earlier this week), but in this case it's harmless because the
> > the tear down will occur with a put_device() ultimately calling device_release()
> > and devres_release_all()
> > 
> > https://elixir.bootlin.com/linux/latest/source/drivers/base/core.c#L2211
> > 
> > Has a comment that covers this case (more or less).
> > "
> > 	 * Some platform devices are driven without driver attached
> > 	 * and managed resources may have been acquired.  Make sure
> > 	 * all resources are released.
> > "
> > 
> > Now, I definitely agree with your statement that it's a bit inconsistent to
> > do this, just not the fixes tag.
> > 
> > One other suggestion below.

Andy, put a cover letter on these larger series - if nothing else it gives
somewhere convenient for people to give tags for the whole series, or 
maintainer to say what they are doing with it.

Anyhow, I'm fine with the series, but will leave it on list for a while
longer, particularly to get patch 6 some eyes + testing.

Currently I plan to drop the fixes tag from this first patch, but I'm prepared
to be convinced it's a bug fix rather than a consistency cleanup.

Jonathan

> > 
> >   
> > > ---
> > > v3: new fix-patch
> > >  drivers/iio/adc/meson_saradc.c | 12 +++++-------
> > >  1 file changed, 5 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/iio/adc/meson_saradc.c b/drivers/iio/adc/meson_saradc.c
> > > index 62cc6fb0ef85..4fe6b997cd03 100644
> > > --- a/drivers/iio/adc/meson_saradc.c
> > > +++ b/drivers/iio/adc/meson_saradc.c
> > > @@ -650,11 +650,11 @@ static int meson_sar_adc_clk_init(struct iio_dev *indio_dev,
> > >  				  void __iomem *base)
> > >  {
> > >  	struct meson_sar_adc_priv *priv = iio_priv(indio_dev);
> > > +	struct device *dev = indio_dev->dev.parent;    
> > 
> > I'd slightly prefer the device was passed in explicitly to this function rather
> > than using the parent assignment which feels a little fragile.   
> 
> Meh, ignore this. I see from one of the later patches, the driver is already
> making the assumption this is set in other calls, so we aren't making anything
> worse with this change.
> 
> Jonathan
> 
> > 
> >   
> > >  	struct clk_init_data init;
> > >  	const char *clk_parents[1];
> > >  
> > > -	init.name = devm_kasprintf(&indio_dev->dev, GFP_KERNEL, "%s#adc_div",
> > > -				   dev_name(indio_dev->dev.parent));
> > > +	init.name = devm_kasprintf(dev, GFP_KERNEL, "%s#adc_div", dev_name(dev));
> > >  	if (!init.name)
> > >  		return -ENOMEM;
> > >  
> > > @@ -670,13 +670,11 @@ static int meson_sar_adc_clk_init(struct iio_dev *indio_dev,
> > >  	priv->clk_div.hw.init = &init;
> > >  	priv->clk_div.flags = 0;
> > >  
> > > -	priv->adc_div_clk = devm_clk_register(&indio_dev->dev,
> > > -					      &priv->clk_div.hw);
> > > +	priv->adc_div_clk = devm_clk_register(dev, &priv->clk_div.hw);
> > >  	if (WARN_ON(IS_ERR(priv->adc_div_clk)))
> > >  		return PTR_ERR(priv->adc_div_clk);
> > >  
> > > -	init.name = devm_kasprintf(&indio_dev->dev, GFP_KERNEL, "%s#adc_en",
> > > -				   dev_name(indio_dev->dev.parent));
> > > +	init.name = devm_kasprintf(dev, GFP_KERNEL, "%s#adc_en", dev_name(dev));
> > >  	if (!init.name)
> > >  		return -ENOMEM;
> > >  
> > > @@ -690,7 +688,7 @@ static int meson_sar_adc_clk_init(struct iio_dev *indio_dev,
> > >  	priv->clk_gate.bit_idx = __ffs(MESON_SAR_ADC_REG3_CLK_EN);
> > >  	priv->clk_gate.hw.init = &init;
> > >  
> > > -	priv->adc_clk = devm_clk_register(&indio_dev->dev, &priv->clk_gate.hw);
> > > +	priv->adc_clk = devm_clk_register(dev, &priv->clk_gate.hw);
> > >  	if (WARN_ON(IS_ERR(priv->adc_clk)))
> > >  		return PTR_ERR(priv->adc_clk);
> > >      
> >   
>