On Tue, May 15, 2018 at 4:45 PM, Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
> Not related to your patch...
>
> On Tue, May 15, 2018 at 02:14:02PM +0200, Sergio Paracuellos wrote:
>> static int
>> mediatek_gpio_bank_probe(struct platform_device *pdev, struct device_node *bank)
>> {
>> + struct mtk_data *gpio_data = dev_get_drvdata(&pdev->dev);
>> const __be32 *id = of_get_property(bank, "reg", NULL);
>> struct mtk_gc *rg = devm_kzalloc(&pdev->dev,
>> sizeof(struct mtk_gc), GFP_KERNEL);
>> + int ret;
>
>>
>> if (!rg || !id || be32_to_cpu(*id) > MTK_MAX_BANK)
>> return -ENOMEM;
>>
>> - gc_map[be32_to_cpu(*id)] = rg;
>> + gpio_data->gc_map[be32_to_cpu(*id)] = rg;
>>
>> memset(rg, 0, sizeof(struct mtk_gc));
>>
>
> I hate that devm_kzalloc() hidden in the allocations and now the
> allocation is even further from the NULL check. Allocations inside the
> declaration block are likelier to be leaked. We see that trend looking
> at static checker output. In this case we're using managed memory so
> it's not an issue, but it still makes me itche.
>
> If *id is MTK_MAX_BANK then we end up writing one element beyond the end
> of the gc_map[] array. Normally I would say just change > to >= but
> we're not using the first element of the gc_map[] array so perhaps we
> intended to subtract one? I don't know. Probably changing > to >= is
> correct.
>
> Also this the "memset(rg, 0, sizeof(struct mtk_gc));" line is not
> required since we allocated zeroed memory.
I'll clean this up a bit in next series.
>
> In other words it maybe should be:
>
> struct mtk_gc *rg;
> int ret;
>
> if (!id || be32_to_cpu(*id) >= MTK_MAX_BANK)
> return -EINVAL;
> rg = devm_kzalloc(&pdev->dev, sizeof(struct mtk_gc), GFP_KERNEL);
> if (!rg)
> return -ENOMEM;
>
> gc_map[be32_to_cpu(*id)] = rg;
>
>
> regards,
> dan carpenter
Best regards,
Sergio Paracuellos
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
