Looks good to me.
Reviewed-by: Benson Leung <bleung@xxxxxxxxxxxx>
On Mon, Mar 5, 2012 at 9:15 AM, Grant Grundler <grundler@xxxxxxxxxxxx> wrote:
>
> tsl2563 probe function has two minor issues with it's error handling
> paths:
> 1) it is silent (did not report errors to dmesg)
> 2) did not return failure code (mixed up use of ret and err)
>
> and two major issues:
> 3) goto fail2 would corrupt a free memory pool ("double free")
> 4) device registration failure did NOT cancel/flush delayed work.
> (and thus dereference a freed data structure later)
>
> The "double free" is subtle and was introduced with this change:
> Author: Jonathan Cameron <jic23@xxxxxxxxx>
> Date: Mon Apr 18 12:58:55 2011 +0100
> staging:iio:tsl2563 take advantage of new iio_device_allocate private
> data.
>
> Originally, chip was allocated seperately. Now it's appended to the
> indio_dev by iio_allocate_device(sizeof(*chip)). So we only need one
> kfree call as well (in iio_free_device()).
>
> Signed-off-by: Grant Grundler <grundler@xxxxxxxxxxxx>
> ---
> Gory details of tracking this down are here:
> http://crosbug.com/26819
>
> Despite iio_device_registration failing, system can at least now boot.
> Will follow up with a fix to "double register : in_intensity_both_raw"
> error that is included in the bug report.
>
> drivers/staging/iio/light/tsl2563.c | 39
> +++++++++++++++++++++++-----------
> 1 files changed, 26 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/staging/iio/light/tsl2563.c
> b/drivers/staging/iio/light/tsl2563.c
> index 7e984bc..bf6498b 100644
> --- a/drivers/staging/iio/light/tsl2563.c
> +++ b/drivers/staging/iio/light/tsl2563.c
> @@ -705,7 +705,6 @@ static int __devinit tsl2563_probe(struct i2c_client
> *client,
> struct tsl2563_chip *chip;
> struct tsl2563_platform_data *pdata = client->dev.platform_data;
> int err = 0;
> - int ret;
> u8 id = 0;
>
> indio_dev = iio_allocate_device(sizeof(*chip));
> @@ -719,13 +718,15 @@ static int __devinit tsl2563_probe(struct i2c_client
> *client,
>
> err = tsl2563_detect(chip);
> if (err) {
> - dev_err(&client->dev, "device not found, error %d\n",
> -err);
> + dev_err(&client->dev, "detect error %d\n", -err);
> goto fail1;
> }
>
> err = tsl2563_read_id(chip, &id);
> - if (err)
> + if (err) {
> + dev_err(&client->dev, "read id error %d\n", -err);
> goto fail1;
> + }
>
> mutex_init(&chip->lock);
>
> @@ -748,40 +749,52 @@ static int __devinit tsl2563_probe(struct i2c_client
> *client,
> indio_dev->num_channels = ARRAY_SIZE(tsl2563_channels);
> indio_dev->dev.parent = &client->dev;
> indio_dev->modes = INDIO_DIRECT_MODE;
> +
> if (client->irq)
> indio_dev->info = &tsl2563_info;
> else
> indio_dev->info = &tsl2563_info_no_irq;
> +
> if (client->irq) {
> - ret = request_threaded_irq(client->irq,
> + err = request_threaded_irq(client->irq,
> NULL,
> &tsl2563_event_handler,
> IRQF_TRIGGER_RISING |
> IRQF_ONESHOT,
> "tsl2563_event",
> indio_dev);
> - if (ret)
> - goto fail2;
> + if (err) {
> + dev_err(&client->dev, "irq request error %d\n",
> -err);
> + goto fail1;
> + }
> }
> +
> err = tsl2563_configure(chip);
> - if (err)
> - goto fail3;
> + if (err) {
> + dev_err(&client->dev, "configure error %d\n", -err);
> + goto fail2;
> + }
>
> INIT_DELAYED_WORK(&chip->poweroff_work, tsl2563_poweroff_work);
> +
> /* The interrupt cannot yet be enabled so this is fine without lock
> */
> schedule_delayed_work(&chip->poweroff_work, 5 * HZ);
>
> - ret = iio_device_register(indio_dev);
> - if (ret)
> + err = iio_device_register(indio_dev);
> + if (err) {
> + dev_err(&client->dev, "iio registration error %d\n",
> -err);
> goto fail3;
> + }
>
> return 0;
> +
> fail3:
> + cancel_delayed_work(&chip->poweroff_work);
> + flush_scheduled_work();
> +fail2:
> if (client->irq)
> free_irq(client->irq, indio_dev);
> -fail2:
> - iio_free_device(indio_dev);
> fail1:
> - kfree(chip);
> + iio_free_device(indio_dev);
> return err;
> }
>
> --
> 1.7.3.4
>
--
Benson Leung
Software Engineer, Chrom* OS
bleung@xxxxxxxxxxxx
--
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
