| Message ID | 20220603100004.70336-1-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v3,1/6] iio: adc: meson_saradc: Don't attach managed resource to IIO device object | expand |
On Fri, 3 Jun 2022 12:59:59 +0300 Andy Shevchenko <andriy.shevchenko@linux.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@metafoo.de> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.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. > --- > 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. > 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); >
On Fri, 3 Jun 2022 17:06:12 +0100 Jonathan Cameron <jic23@kernel.org> wrote: > On Fri, 3 Jun 2022 12:59:59 +0300 > Andy Shevchenko <andriy.shevchenko@linux.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@metafoo.de> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.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. > > > > --- > > 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); > > >
On Fri, 3 Jun 2022 17:23:07 +0100 Jonathan Cameron <jic23@kernel.org> wrote: > On Fri, 3 Jun 2022 17:06:12 +0100 > Jonathan Cameron <jic23@kernel.org> wrote: > > > On Fri, 3 Jun 2022 12:59:59 +0300 > > Andy Shevchenko <andriy.shevchenko@linux.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@metafoo.de> > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.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); > > > > > >
On Fri, Jun 03, 2022 at 05:29:20PM +0100, Jonathan Cameron wrote: > On Fri, 3 Jun 2022 17:23:07 +0100 > Jonathan Cameron <jic23@kernel.org> wrote: ... > 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. I'll try to remember this. The series was started from only a couple of patches and grew to this big, and I forgot to add a cover letter when it seems not anymore obvious what has been done. > 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. Fine with me, thanks!
On Fri, Jun 3, 2022 at 12:00 PM Andy Shevchenko <andriy.shevchenko@linux.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@metafoo.de> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Reviewed-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> I am also fine if the Fixes tag is being dropped - please keep my Reviewed-by in that case.
On Fri, 3 Jun 2022 19:54:43 +0300 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Fri, Jun 03, 2022 at 05:29:20PM +0100, Jonathan Cameron wrote: > > On Fri, 3 Jun 2022 17:23:07 +0100 > > Jonathan Cameron <jic23@kernel.org> wrote: > > ... > > > 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. > > I'll try to remember this. The series was started from only a couple of patches > and grew to this big, and I forgot to add a cover letter when it seems not > anymore obvious what has been done. > > > 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. > > Fine with me, thanks! > Applied to the togreg branch of iio.git and pushed out as testing for all the normal reasons. Thanks, Jonathan
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; 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);
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@metafoo.de> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- v3: new fix-patch drivers/iio/adc/meson_saradc.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-)