lkml.org 
[lkml]   [2012]   [Mar]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH] staging:iio:tsl2563 rewrite probe error handling
    From
    Looks good to me.

    Reviewed-by: Benson Leung <bleung@chromium.org>


    On Mon, Mar 5, 2012 at 9:15 AM, Grant Grundler <grundler@chromium.org> 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@cam.ac.uk>
    >    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@chromium.org>
    > ---
    >  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@chromium.org
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2012-03-05 22:31    [W:0.037 / U:0.788 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site