* [PATCH] staging:iio:ad7606: update api calls to devm_* variants
@ 2018-09-13 9:51 Alexandru Ardelean
2018-09-16 11:16 ` Jonathan Cameron
0 siblings, 1 reply; 3+ messages in thread
From: Alexandru Ardelean @ 2018-09-13 9:51 UTC (permalink / raw)
To: linux-iio, Michael.Hennerich, lars, jic23
Cc: knaack.h, pmeerw, Alexandru Ardelean
No significant functional changes. This just replaces the code with devm_*
that reduce the driver code, and simplifies some error code paths in the
ad7606_probe() function.
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
drivers/staging/iio/adc/ad7606.c | 25 +++++++++----------------
1 file changed, 9 insertions(+), 16 deletions(-)
diff --git a/drivers/staging/iio/adc/ad7606.c b/drivers/staging/iio/adc/ad7606.c
index 87d5fb073c95..793de92f27ed 100644
--- a/drivers/staging/iio/adc/ad7606.c
+++ b/drivers/staging/iio/adc/ad7606.c
@@ -458,28 +458,25 @@ int ad7606_probe(struct device *dev, int irq, void __iomem *base_address,
if (ret)
dev_warn(st->dev, "failed to RESET: no RESET GPIO specified\n");
- ret = request_irq(irq, ad7606_interrupt, IRQF_TRIGGER_FALLING, name,
- indio_dev);
+ ret = devm_request_irq(dev, irq, ad7606_interrupt,
+ IRQF_TRIGGER_FALLING,
+ name, indio_dev);
if (ret)
goto error_disable_reg;
- ret = iio_triggered_buffer_setup(indio_dev, &ad7606_trigger_handler,
- NULL, NULL);
+ ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
+ &ad7606_trigger_handler,
+ NULL, NULL);
if (ret)
- goto error_free_irq;
+ goto error_disable_reg;
- ret = iio_device_register(indio_dev);
+ ret = devm_iio_device_register(dev, indio_dev);
if (ret)
- goto error_unregister_ring;
+ goto error_disable_reg;
dev_set_drvdata(dev, indio_dev);
return 0;
-error_unregister_ring:
- iio_triggered_buffer_cleanup(indio_dev);
-
-error_free_irq:
- free_irq(irq, indio_dev);
error_disable_reg:
regulator_disable(st->reg);
@@ -492,10 +489,6 @@ int ad7606_remove(struct device *dev, int irq)
struct iio_dev *indio_dev = dev_get_drvdata(dev);
struct ad7606_state *st = iio_priv(indio_dev);
- iio_device_unregister(indio_dev);
- iio_triggered_buffer_cleanup(indio_dev);
-
- free_irq(irq, indio_dev);
regulator_disable(st->reg);
return 0;
--
2.17.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] staging:iio:ad7606: update api calls to devm_* variants
2018-09-13 9:51 [PATCH] staging:iio:ad7606: update api calls to devm_* variants Alexandru Ardelean
@ 2018-09-16 11:16 ` Jonathan Cameron
2018-09-17 7:52 ` Ardelean, Alexandru
0 siblings, 1 reply; 3+ messages in thread
From: Jonathan Cameron @ 2018-09-16 11:16 UTC (permalink / raw)
To: Alexandru Ardelean; +Cc: linux-iio, Michael.Hennerich, lars, knaack.h, pmeerw
On Thu, 13 Sep 2018 12:51:38 +0300
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
> No significant functional changes. This just replaces the code with devm_*
> that reduce the driver code, and simplifies some error code paths in the
> ad7606_probe() function.
>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
Sometimes there are reasons why devm functions aren't used. They are almost
always about potential races in remove paths and that is definitely the case
here. Suggestions inline on how to avoid that problem.
Thanks,
Jonathan
> ---
> drivers/staging/iio/adc/ad7606.c | 25 +++++++++----------------
> 1 file changed, 9 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/staging/iio/adc/ad7606.c b/drivers/staging/iio/adc/ad7606.c
> index 87d5fb073c95..793de92f27ed 100644
> --- a/drivers/staging/iio/adc/ad7606.c
> +++ b/drivers/staging/iio/adc/ad7606.c
> @@ -458,28 +458,25 @@ int ad7606_probe(struct device *dev, int irq, void __iomem *base_address,
> if (ret)
> dev_warn(st->dev, "failed to RESET: no RESET GPIO specified\n");
>
> - ret = request_irq(irq, ad7606_interrupt, IRQF_TRIGGER_FALLING, name,
> - indio_dev);
> + ret = devm_request_irq(dev, irq, ad7606_interrupt,
> + IRQF_TRIGGER_FALLING,
> + name, indio_dev);
You cannot safely mix devm and non devm setup. In this case you end up
with an interesting race around when the regulator is turned off resulting
in the device being powered down before the interrupt is released.
Now, that might be fine, but it's really hard to be sure when reviewing so
as a rule of thumb we never allow non obvious ordering.
Now, one option for this is to use devm_add_action to automatically call
the cleanup in the right sequence when remove occurs.
> if (ret)
> goto error_disable_reg;
>
> - ret = iio_triggered_buffer_setup(indio_dev, &ad7606_trigger_handler,
> - NULL, NULL);
> + ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
> + &ad7606_trigger_handler,
> + NULL, NULL);
> if (ret)
> - goto error_free_irq;
> + goto error_disable_reg;
>
> - ret = iio_device_register(indio_dev);
> + ret = devm_iio_device_register(dev, indio_dev);
> if (ret)
> - goto error_unregister_ring;
> + goto error_disable_reg;
Definitely not on this one. The power will be cut before the interfaces
are removed. That one is definitely going to be a nasty race condition!
>
> dev_set_drvdata(dev, indio_dev);
>
> return 0;
> -error_unregister_ring:
> - iio_triggered_buffer_cleanup(indio_dev);
> -
> -error_free_irq:
> - free_irq(irq, indio_dev);
>
> error_disable_reg:
> regulator_disable(st->reg);
> @@ -492,10 +489,6 @@ int ad7606_remove(struct device *dev, int irq)
> struct iio_dev *indio_dev = dev_get_drvdata(dev);
> struct ad7606_state *st = iio_priv(indio_dev);
>
> - iio_device_unregister(indio_dev);
> - iio_triggered_buffer_cleanup(indio_dev);
> -
> - free_irq(irq, indio_dev);
> regulator_disable(st->reg);
>
> return 0;
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] staging:iio:ad7606: update api calls to devm_* variants
2018-09-16 11:16 ` Jonathan Cameron
@ 2018-09-17 7:52 ` Ardelean, Alexandru
0 siblings, 0 replies; 3+ messages in thread
From: Ardelean, Alexandru @ 2018-09-17 7:52 UTC (permalink / raw)
To: jic23@kernel.org
Cc: lars@metafoo.de, knaack.h@gmx.de, linux-iio@vger.kernel.org,
Hennerich, Michael, pmeerw@pmeerw.net
T24gU3VuLCAyMDE4LTA5LTE2IGF0IDEyOjE2ICswMTAwLCBKb25hdGhhbiBDYW1lcm9uIHdyb3Rl
Og0KPiBPbiBUaHUsIDEzIFNlcCAyMDE4IDEyOjUxOjM4ICswMzAwDQo+IEFsZXhhbmRydSBBcmRl
bGVhbiA8YWxleGFuZHJ1LmFyZGVsZWFuQGFuYWxvZy5jb20+IHdyb3RlOg0KPiANCj4gPiBObyBz
aWduaWZpY2FudCBmdW5jdGlvbmFsIGNoYW5nZXMuIFRoaXMganVzdCByZXBsYWNlcyB0aGUgY29k
ZSB3aXRoDQo+ID4gZGV2bV8qDQo+ID4gdGhhdCByZWR1Y2UgdGhlIGRyaXZlciBjb2RlLCBhbmQg
c2ltcGxpZmllcyBzb21lIGVycm9yIGNvZGUgcGF0aHMgaW4NCj4gPiB0aGUNCj4gPiBhZDc2MDZf
cHJvYmUoKSBmdW5jdGlvbi4NCj4gPiANCj4gPiBTaWduZWQtb2ZmLWJ5OiBBbGV4YW5kcnUgQXJk
ZWxlYW4gPGFsZXhhbmRydS5hcmRlbGVhbkBhbmFsb2cuY29tPg0KPiANCj4gU29tZXRpbWVzIHRo
ZXJlIGFyZSByZWFzb25zIHdoeSBkZXZtIGZ1bmN0aW9ucyBhcmVuJ3QgdXNlZC4gVGhleSBhcmUN
Cj4gYWxtb3N0DQo+IGFsd2F5cyBhYm91dCBwb3RlbnRpYWwgcmFjZXMgaW4gcmVtb3ZlIHBhdGhz
IGFuZCB0aGF0IGlzIGRlZmluaXRlbHkgdGhlDQo+IGNhc2UNCj4gaGVyZS4gIFN1Z2dlc3Rpb25z
IGlubGluZSBvbiBob3cgdG8gYXZvaWQgdGhhdCBwcm9ibGVtLg0KPiANCg0KSSdsbCBhZG1pdCB0
aGF0IEkgd2FzIGEgYml0IG5haXZlIGFib3V0IHRoaXMgb25lLg0KQnV0IEknbGwgdGFrZSBpdCBh
cyBhIHF1aWNrIGNyYXNoLWNvdXJzZSBvbiB0aGlzIHN1YmplY3QgYW5kIGFkYXB0IG15DQp0aGlu
a2luZyBmb3IgdGhlc2UgZHJpdmVycyArIGRldm1fIEFQSXMuDQoNCk5hdHVyYWxseSwgSSdkIGxp
a2UgdG8gZHJvcCB0aGlzIHBhdGNoOyBJJ2xsIHRha2UgYSBxdWljayBsb29rIGF0IHRoZQ0KaW5p
dGlhbGl6YXRpb24gc2VxdWVuY2UsIGFuZCBpZiBJIHNlZSBhbnl0aGluZyB3cm9uZywgSSdsbCBj
b21lIGJhY2sgd2l0aA0KcGF0Y2ggZm9yIHRoYXQgaXNzdWUuDQoNClRoYW5rcw0KQWxleA0KDQoN
Cj4gVGhhbmtzLA0KPiANCj4gSm9uYXRoYW4NCj4gDQo+ID4gLS0tDQo+ID4gIGRyaXZlcnMvc3Rh
Z2luZy9paW8vYWRjL2FkNzYwNi5jIHwgMjUgKysrKysrKysrLS0tLS0tLS0tLS0tLS0tLQ0KPiA+
ICAxIGZpbGUgY2hhbmdlZCwgOSBpbnNlcnRpb25zKCspLCAxNiBkZWxldGlvbnMoLSkNCj4gPiAN
Cj4gPiBkaWZmIC0tZ2l0IGEvZHJpdmVycy9zdGFnaW5nL2lpby9hZGMvYWQ3NjA2LmMNCj4gPiBi
L2RyaXZlcnMvc3RhZ2luZy9paW8vYWRjL2FkNzYwNi5jDQo+ID4gaW5kZXggODdkNWZiMDczYzk1
Li43OTNkZTkyZjI3ZWQgMTAwNjQ0DQo+ID4gLS0tIGEvZHJpdmVycy9zdGFnaW5nL2lpby9hZGMv
YWQ3NjA2LmMNCj4gPiArKysgYi9kcml2ZXJzL3N0YWdpbmcvaWlvL2FkYy9hZDc2MDYuYw0KPiA+
IEBAIC00NTgsMjggKzQ1OCwyNSBAQCBpbnQgYWQ3NjA2X3Byb2JlKHN0cnVjdCBkZXZpY2UgKmRl
diwgaW50IGlycSwNCj4gPiB2b2lkIF9faW9tZW0gKmJhc2VfYWRkcmVzcywNCj4gPiAgCWlmIChy
ZXQpDQo+ID4gIAkJZGV2X3dhcm4oc3QtPmRldiwgImZhaWxlZCB0byBSRVNFVDogbm8gUkVTRVQg
R1BJTw0KPiA+IHNwZWNpZmllZFxuIik7DQo+ID4gIA0KPiA+IC0JcmV0ID0gcmVxdWVzdF9pcnEo
aXJxLCBhZDc2MDZfaW50ZXJydXB0LCBJUlFGX1RSSUdHRVJfRkFMTElORywNCj4gPiBuYW1lLA0K
PiA+IC0JCQkgIGluZGlvX2Rldik7DQo+ID4gKwlyZXQgPSBkZXZtX3JlcXVlc3RfaXJxKGRldiwg
aXJxLCBhZDc2MDZfaW50ZXJydXB0LA0KPiA+ICsJCQkgICAgICAgSVJRRl9UUklHR0VSX0ZBTExJ
TkcsDQo+ID4gKwkJCSAgICAgICBuYW1lLCBpbmRpb19kZXYpOw0KPiANCj4gWW91IGNhbm5vdCBz
YWZlbHkgbWl4IGRldm0gYW5kIG5vbiBkZXZtIHNldHVwLiAgSW4gdGhpcyBjYXNlIHlvdSBlbmQg
dXANCj4gd2l0aCBhbiBpbnRlcmVzdGluZyByYWNlIGFyb3VuZCB3aGVuIHRoZSByZWd1bGF0b3Ig
aXMgdHVybmVkIG9mZg0KPiByZXN1bHRpbmcNCj4gaW4gdGhlIGRldmljZSBiZWluZyBwb3dlcmVk
IGRvd24gYmVmb3JlIHRoZSBpbnRlcnJ1cHQgaXMgcmVsZWFzZWQuDQo+IA0KPiBOb3csIHRoYXQg
bWlnaHQgYmUgZmluZSwgYnV0IGl0J3MgcmVhbGx5IGhhcmQgdG8gYmUgc3VyZSB3aGVuIHJldmll
d2luZw0KPiBzbw0KPiBhcyBhIHJ1bGUgb2YgdGh1bWIgd2UgbmV2ZXIgYWxsb3cgbm9uIG9idmlv
dXMgb3JkZXJpbmcuDQo+IA0KPiBOb3csIG9uZSBvcHRpb24gZm9yIHRoaXMgaXMgdG8gdXNlIGRl
dm1fYWRkX2FjdGlvbiB0byBhdXRvbWF0aWNhbGx5IGNhbGwNCj4gdGhlIGNsZWFudXAgaW4gdGhl
IHJpZ2h0IHNlcXVlbmNlIHdoZW4gcmVtb3ZlIG9jY3Vycy4NCj4gDQo+ID4gIAlpZiAocmV0KQ0K
PiA+ICAJCWdvdG8gZXJyb3JfZGlzYWJsZV9yZWc7DQo+ID4gIA0KPiA+IC0JcmV0ID0gaWlvX3Ry
aWdnZXJlZF9idWZmZXJfc2V0dXAoaW5kaW9fZGV2LA0KPiA+ICZhZDc2MDZfdHJpZ2dlcl9oYW5k
bGVyLA0KPiA+IC0JCQkJCSBOVUxMLCBOVUxMKTsNCj4gPiArCXJldCA9IGRldm1faWlvX3RyaWdn
ZXJlZF9idWZmZXJfc2V0dXAoZGV2LCBpbmRpb19kZXYsDQo+ID4gKwkJCQkJICAgICAgJmFkNzYw
Nl90cmlnZ2VyX2hhbmRsZXIsDQo+ID4gKwkJCQkJICAgICAgTlVMTCwgTlVMTCk7DQo+ID4gIAlp
ZiAocmV0KQ0KPiA+IC0JCWdvdG8gZXJyb3JfZnJlZV9pcnE7DQo+ID4gKwkJZ290byBlcnJvcl9k
aXNhYmxlX3JlZzsNCj4gPiAgDQo+ID4gLQlyZXQgPSBpaW9fZGV2aWNlX3JlZ2lzdGVyKGluZGlv
X2Rldik7DQo+ID4gKwlyZXQgPSBkZXZtX2lpb19kZXZpY2VfcmVnaXN0ZXIoZGV2LCBpbmRpb19k
ZXYpOw0KPiA+ICAJaWYgKHJldCkNCj4gPiAtCQlnb3RvIGVycm9yX3VucmVnaXN0ZXJfcmluZzsN
Cj4gPiArCQlnb3RvIGVycm9yX2Rpc2FibGVfcmVnOw0KPiANCj4gRGVmaW5pdGVseSBub3Qgb24g
dGhpcyBvbmUuICBUaGUgcG93ZXIgd2lsbCBiZSBjdXQgYmVmb3JlIHRoZSBpbnRlcmZhY2VzDQo+
IGFyZSByZW1vdmVkLiAgVGhhdCBvbmUgaXMgZGVmaW5pdGVseSBnb2luZyB0byBiZSBhIG5hc3R5
IHJhY2UgY29uZGl0aW9uIQ0KPiANCj4gPiAgDQo+ID4gIAlkZXZfc2V0X2RydmRhdGEoZGV2LCBp
bmRpb19kZXYpOw0KPiA+ICANCj4gPiAgCXJldHVybiAwOw0KPiA+IC1lcnJvcl91bnJlZ2lzdGVy
X3Jpbmc6DQo+ID4gLQlpaW9fdHJpZ2dlcmVkX2J1ZmZlcl9jbGVhbnVwKGluZGlvX2Rldik7DQo+
ID4gLQ0KPiA+IC1lcnJvcl9mcmVlX2lycToNCj4gPiAtCWZyZWVfaXJxKGlycSwgaW5kaW9fZGV2
KTsNCj4gPiAgDQo+ID4gIGVycm9yX2Rpc2FibGVfcmVnOg0KPiA+ICAJcmVndWxhdG9yX2Rpc2Fi
bGUoc3QtPnJlZyk7DQo+ID4gQEAgLTQ5MiwxMCArNDg5LDYgQEAgaW50IGFkNzYwNl9yZW1vdmUo
c3RydWN0IGRldmljZSAqZGV2LCBpbnQgaXJxKQ0KPiA+ICAJc3RydWN0IGlpb19kZXYgKmluZGlv
X2RldiA9IGRldl9nZXRfZHJ2ZGF0YShkZXYpOw0KPiA+ICAJc3RydWN0IGFkNzYwNl9zdGF0ZSAq
c3QgPSBpaW9fcHJpdihpbmRpb19kZXYpOw0KPiA+ICANCj4gPiAtCWlpb19kZXZpY2VfdW5yZWdp
c3RlcihpbmRpb19kZXYpOw0KPiA+IC0JaWlvX3RyaWdnZXJlZF9idWZmZXJfY2xlYW51cChpbmRp
b19kZXYpOw0KPiA+IC0NCj4gPiAtCWZyZWVfaXJxKGlycSwgaW5kaW9fZGV2KTsNCj4gPiAgCXJl
Z3VsYXRvcl9kaXNhYmxlKHN0LT5yZWcpOw0KPiA+ICANCj4gPiAgCXJldHVybiAwOw0KPiANCj4g
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-09-17 7:52 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-09-13 9:51 [PATCH] staging:iio:ad7606: update api calls to devm_* variants Alexandru Ardelean
2018-09-16 11:16 ` Jonathan Cameron
2018-09-17 7:52 ` Ardelean, Alexandru
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).