On 04/24/2016 10:48 AM, Jonathan Cameron wrote:
> On 18/04/16 15:05, Marek Vasut wrote:
>> Add support for HopeRF pressure and temperature sensor.
>>
>> This device uses two fixed I2C addresses, one for storing
>> calibration coefficients and another for accessing the ADC.
>>
>> Signed-off-by: Marek Vasut <marex@xxxxxxx>
>> Cc: Matt Ranostay <mranostay@xxxxxxxxx>
>> Cc: Jonathan Cameron <jic23@xxxxxxxxxx>
> Not sure how I missed this earlier (sorry!) but you have some ordering issues
> in the probe vs remove functions that need sorting out.
I will comment on that below. Thanks for spotting this.
> As they were easily fixed, I've done it and applied the result to the
> togreg branch of iio.git. Please check I didn't mess anything up.
You mean 'testing' branch :)
> The binding is straight forward enough that I don't feel I need a devicetree
> ack, though of course one is always welcome!
>
> Thanks,
[...]
>> +static int hp03_probe(struct i2c_client *client,
>> + const struct i2c_device_id *id)
>> +{
>> + struct device *dev = &client->dev;
>> + struct iio_dev *indio_dev;
>> + struct hp03_priv *priv;
>> + int ret;
>> +
>> + indio_dev = devm_iio_device_alloc(dev, sizeof(*priv));
>> + if (!indio_dev)
>> + return -ENOMEM;
>> +
>> + priv = iio_priv(indio_dev);
>> + priv->client = client;
>> + mutex_init(&priv->lock);
>> +
>> + indio_dev->dev.parent = dev;
>> + indio_dev->name = id->name;
>> + indio_dev->channels = hp03_channels;
>> + indio_dev->num_channels = ARRAY_SIZE(hp03_channels);
>> + indio_dev->info = &hp03_info;
>> + indio_dev->modes = INDIO_DIRECT_MODE;
>> +
>> + /*
>> + * Allocate another device for the on-sensor EEPROM,
>> + * which has it's dedicated I2C address and contains
>> + * the calibration constants for the sensor.
>> + */
>> + priv->eeprom_client = i2c_new_dummy(client->adapter, HP03_EEPROM_ADDR);
>> + if (!priv->eeprom_client) {
>> + dev_err(dev, "New EEPROM I2C device failed\n");
>> + return -ENODEV;
>> + }
>> +
>> + priv->eeprom_regmap = devm_regmap_init_i2c(priv->eeprom_client,
>> + &hp03_regmap_config);
>> + if (IS_ERR(priv->eeprom_regmap)) {
>> + dev_err(dev, "Failed to allocate EEPROM regmap\n");
>> + ret = PTR_ERR(priv->eeprom_regmap);
>> + goto err;
>> + }
>> +
>> + priv->xclr_gpio = devm_gpiod_get_index(dev, "xclr", 0, GPIOD_OUT_HIGH);
>> + if (IS_ERR(priv->xclr_gpio)) {
>> + dev_err(dev, "Failed to claim XCLR GPIO\n");
>> + ret = PTR_ERR(priv->xclr_gpio);
>> + goto err;
>> + }
>> +
>> + ret = devm_iio_device_register(dev, indio_dev);
>> + if (ret) {
>> + dev_err(dev, "Failed to register IIO device\n");
>> + goto err;
>> + }
>> +
>> + i2c_set_clientdata(client, indio_dev);
>> +
>> + return 0;
>> +
>> +err:
>> + i2c_unregister_device(priv->eeprom_client);
>> + return ret;
>> +}
>> +
>> +static int hp03_remove(struct i2c_client *client)
>> +{
>> + struct iio_dev *indio_dev = i2c_get_clientdata(client);
>> + struct hp03_priv *priv = iio_priv(indio_dev);
>> +
>> + i2c_unregister_device(priv->eeprom_client);
> There is a race here unfortunately.
After reading the explanation, I agree. It's subtle and quite nasty.
> As you have used devm_iio_device_register the usespace interaces are still
> exposed at this point. So can they cause any use of the eeprom i2c device?
> Yes, they can so if we happen to get a userspace read between here and
> the devm initiallised unregister it will crash...
Yes, true.
> It is less obvious whether we can even use the devm_ regmap call for the
> eeprom i2c device. It can't be accessed as long as the iio_device
> has been deregistered. However, this isn't immediately obvious when
> reviewing so on that basis alone it should be unwound here rather than
> relying on devm...
>
> You will also want to reorganise the probe function to move the registration
> of this bus past the gpio stuff so as to avoid needing to unwind that by hand
> as well.
>
> Anyhow, rule of thumb is that probe order must be reversed in remove
> including devm related cleanup.
Right. I went through the updated HP03 code and it does make sense.
Thanks for catching this race and explaining it in this much detail,
I enjoyed reading this breakdown. And sorry for the hassle, I'll be
more careful next time.
>> +
> > + return 0;
>> +}
>> +
>> +static const struct i2c_device_id hp03_id[] = {
>> + { "hp03", 0 },
>> + { },
>> +};
>> +MODULE_DEVICE_TABLE(i2c, hp03_id);
>> +
>> +static struct i2c_driver hp03_driver = {
>> + .driver = {
>> + .name = "hp03",
>> + },
>> + .probe = hp03_probe,
>> + .remove = hp03_remove,
>> + .id_table = hp03_id,
>> +};
>> +module_i2c_driver(hp03_driver);
>> +
>> +MODULE_AUTHOR("Marek Vasut <marex@xxxxxxx>");
>> +MODULE_DESCRIPTION("Driver for Hope RF HP03 pressure and temperature sensor");
>> +MODULE_LICENSE("GPL v2");
>>
>
--
Best regards,
Marek Vasut
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
