[PATCH] staging: mt7621-gpio: avoid use of globals and use a drvdata allocated structure

Sergio Paracuellos sergio.paracuellos at gmail.com
Tue May 15 17:02:30 UTC 2018


On Tue, May 15, 2018 at 4:45 PM, Dan Carpenter <dan.carpenter at oracle.com> 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


More information about the devel mailing list