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

[Dan Carpenter]

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.

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