Hi, > Am 20.02.2019 um 17:18 schrieb Jonathan Cameron <[email protected]>: > > On Wed, 20 Feb 2019 15:00:52 +0100 > "H. Nikolaus Schaller" <[email protected]> wrote: > >> This simplifies the code a little. > It does, but at the cost of introducing potential race conditions. > Please don't do this. See below for why and a suggestion on how > to resolve things if you want to make this change safely.
Ok, I see. I just was working on that driver and thought that introducing devm is a good idea here. But since it does not improve any function (contrary to the mount-matrix patches), let's drop it for the moment. BR and thanks, Nikolaus > > Jonathan >> >> Signed-off-by: H. Nikolaus Schaller <[email protected]> >> --- >> drivers/iio/accel/bma180.c | 56 ++++++++++++++------------------------ >> 1 file changed, 21 insertions(+), 35 deletions(-) >> >> diff --git a/drivers/iio/accel/bma180.c b/drivers/iio/accel/bma180.c >> index 248be67e4f41..3f5ee2d495d3 100644 >> --- a/drivers/iio/accel/bma180.c >> +++ b/drivers/iio/accel/bma180.c >> @@ -738,7 +738,7 @@ static int bma180_probe(struct i2c_client *client, >> >> ret = data->part_info->chip_config(data); >> if (ret < 0) >> - goto err_chip_disable; >> + goto err; >> >> mutex_init(&data->mutex); >> indio_dev->dev.parent = &client->dev; >> @@ -748,12 +748,25 @@ static int bma180_probe(struct i2c_client *client, >> indio_dev->modes = INDIO_DIRECT_MODE; >> indio_dev->info = &bma180_info; >> >> + ret = devm_iio_triggered_buffer_setup(&client->dev, indio_dev, NULL, >> + bma180_trigger_handler, NULL); >> + if (ret < 0) { >> + dev_err(&client->dev, "unable to setup iio triggered buffer\n"); >> + goto err; >> + } >> + >> + ret = devm_iio_device_register(&client->dev, indio_dev); >> + if (ret < 0) { >> + dev_err(&client->dev, "unable to register iio device\n"); >> + goto err; >> + } >> + >> if (client->irq > 0) { >> - data->trig = iio_trigger_alloc("%s-dev%d", indio_dev->name, >> - indio_dev->id); >> + data->trig = devm_iio_trigger_alloc(&client->dev, "%s-dev%d", >> + indio_dev->name, indio_dev->id); >> if (!data->trig) { >> ret = -ENOMEM; >> - goto err_chip_disable; >> + goto err; >> } >> >> ret = devm_request_irq(&client->dev, client->irq, >> @@ -761,7 +774,7 @@ static int bma180_probe(struct i2c_client *client, >> "bma180_event", data->trig); >> if (ret) { >> dev_err(&client->dev, "unable to request IRQ\n"); >> - goto err_trigger_free; >> + goto err; >> } >> >> data->trig->dev.parent = &client->dev; >> @@ -769,34 +782,14 @@ static int bma180_probe(struct i2c_client *client, >> iio_trigger_set_drvdata(data->trig, indio_dev); >> indio_dev->trig = iio_trigger_get(data->trig); >> >> - ret = iio_trigger_register(data->trig); >> + ret = devm_iio_trigger_register(&client->dev, data->trig); >> if (ret) >> - goto err_trigger_free; >> - } >> - >> - ret = iio_triggered_buffer_setup(indio_dev, NULL, >> - bma180_trigger_handler, NULL); >> - if (ret < 0) { >> - dev_err(&client->dev, "unable to setup iio triggered buffer\n"); >> - goto err_trigger_unregister; >> - } >> - >> - ret = iio_device_register(indio_dev); >> - if (ret < 0) { >> - dev_err(&client->dev, "unable to register iio device\n"); >> - goto err_buffer_cleanup; >> + goto err; >> } >> >> return 0; >> >> -err_buffer_cleanup: >> - iio_triggered_buffer_cleanup(indio_dev); >> -err_trigger_unregister: >> - if (data->trig) >> - iio_trigger_unregister(data->trig); >> -err_trigger_free: >> - iio_trigger_free(data->trig); >> -err_chip_disable: >> +err: >> data->part_info->chip_disable(data); >> >> return ret; >> @@ -807,13 +800,6 @@ static int bma180_remove(struct i2c_client *client) >> struct iio_dev *indio_dev = i2c_get_clientdata(client); >> struct bma180_data *data = iio_priv(indio_dev); >> >> - iio_device_unregister(indio_dev); >> - iio_triggered_buffer_cleanup(indio_dev); >> - if (data->trig) { >> - iio_trigger_unregister(data->trig); >> - iio_trigger_free(data->trig); >> - } >> - > Now we disable the device before removing it's userspace interface. > Not a good idea. > > Generally I will insist on the remove order always being the precise > opposite of probe simply because it is obviously correct. > That includes cases which can be simply argued to be fine (not > this one which isn't). The reason is readability trumps saving > a few lines of code and it's a lot easier to check a probe and > remove function against each other if the order is maintained. > > Of course, there are ways to do this by making all the unwind > managed, but you haven't done this here. > devm_add_action_or_reset is handy for this if you want to do it. > >> mutex_lock(&data->mutex); >> data->part_info->chip_disable(data); >> mutex_unlock(&data->mutex); >
