Hi,
> Am 20.02.2019 um 17:18 schrieb Jonathan Cameron <jic23@xxxxxxxxxx>:
>
> On Wed, 20 Feb 2019 15:00:52 +0100
> "H. Nikolaus Schaller" <hns@xxxxxxxxxxxxx> 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 <hns@xxxxxxxxxxxxx>
>> ---
>> 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);
>
