Re: [PATCH 5/9] iio: accel: bma180: convert to devm

[Jonathan Cameron]

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.

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);