Re: [PATCH] staging:iio:tsl2563 rewrite probe error handling

[Grant Grundler]

On Mon, Mar 5, 2012 at 1:27 PM, Benson Leung <bleung@xxxxxxxxxxxx> wrote:
> Looks good to me.

Thanks Benson!

cheers,
grant

>
> 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-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/