Messages in this thread | | | Date | Sat, 2 Aug 2014 13:30:01 +0200 | From | Wolfram Sang <> | Subject | Re: [PATCH v4] i2c: busses: i2c-pxa.c: Fix for possible null pointer dereference |
| |
Hi,
> res = platform_get_resource(dev, IORESOURCE_MEM, 0); > irq = platform_get_irq(dev, 0); > - if (res == NULL || irq < 0) { > - ret = -ENODEV; > - goto eclk; > - } > + if (res == NULL || irq < 0) > + return -ENODEV;
No need to check for valid 'res' here...
> > - if (!request_mem_region(res->start, resource_size(res), res->name)) { > - ret = -ENOMEM; > - goto eclk; > - } > + if (!devm_request_mem_region(&dev->dev, res->start, > + resource_size(res), res->name)) > + return -ENOMEM;
... and no need to use 'devm_request_mem_region' ...
> - i2c->reg_base = ioremap(res->start, resource_size(res)); > - if (!i2c->reg_base) { > - ret = -EIO; > - goto eremap; > - } > + i2c->reg_base = devm_ioremap_resource(&adev->dev, res)); > + if (IS_ERR(i2c->reg_base)) > + return -EIO;
... because devm_ioremap_resource does all that for you. Also, don't return -EIO but the PTR_ERR given from the function. Just check other drivers how they do it.
> > i2c->reg_ibmr = i2c->reg_base + pxa_reg_layout[i2c_type].ibmr; > i2c->reg_idbr = i2c->reg_base + pxa_reg_layout[i2c_type].idbr; > @@ -1227,10 +1218,12 @@ static int i2c_pxa_probe(struct platform_device *dev) > i2c->adap.algo = &i2c_pxa_pio_algorithm; > } else { > i2c->adap.algo = &i2c_pxa_algorithm; > - ret = request_irq(irq, i2c_pxa_handler, IRQF_SHARED, > - dev_name(&dev->dev), i2c); > - if (ret) > - goto ereqirq; > + ret = devm_request_irq(&dev->dev, irq, i2c_pxa_handler, > + IRQF_SHARED, dev_name(&dev->dev), i2c); > + if (ret) { > + clk_disable_unprepare(i2c->clk); > + return ret;
Maybe you should make a error path with 'goto' out of it...
> + } > } > > i2c_pxa_reset(i2c); > @@ -1244,7 +1237,7 @@ static int i2c_pxa_probe(struct platform_device *dev) > ret = i2c_add_numbered_adapter(&i2c->adap); > if (ret < 0) { > printk(KERN_INFO "I2C: Failed to add bus\n"); > - goto eadapt; > + return ret;
... since you forgot disable_unprepare here and could simply reuse it.
> } > > platform_set_drvdata(dev, i2c); > @@ -1257,20 +1250,6 @@ static int i2c_pxa_probe(struct platform_device *dev) > dev_name(&i2c->adap.dev)); > #endif > return 0; > - > -eadapt: > - if (!i2c->use_pio) > - free_irq(irq, i2c); > -ereqirq: > - clk_disable_unprepare(i2c->clk); > - iounmap(i2c->reg_base); > -eremap: > - clk_put(i2c->clk); > -eclk: > - kfree(i2c); > -emalloc: > - release_mem_region(res->start, resource_size(res)); > - return ret; > }
You forgot to cleanup the remove function.
BTW I forgot, can you test this patch on real hardware? If not, is there somebody who does?
All the best,
Wolfram
[unhandled content-type:application/pgp-signature] | |