Re: [patch] mtd/docg3: fix error handling in docg3_probe()

[Robert Jarzmik <robert.jarzmik@free.fr>]

Dan Carpenter <dan.carpenter@oracle.com> writes:

> There was a kfree(docg3_floors); missing from the error handling
> here.  Also we set docg3_floors[floor] = mtd; when mtd was an ERR_PTR
> and then we call doc_release_device() on it.
Hi Dan,

The missing kfree was dealt with by a later patch amending the probe path :
"mtd/docg3: add ECC correction code" submited in [1]. Your patch is in conflict
with this later one.
The doc_release_device() is an excellent catch. I wonder how you found it.

> diff --git a/drivers/mtd/devices/docg3.c b/drivers/mtd/devices/docg3.c
> index 27c4fea..bfc1ea1 100644
> --- a/drivers/mtd/devices/docg3.c
> +++ b/drivers/mtd/devices/docg3.c
> @@ -1110,21 +1110,24 @@ static int __init docg3_probe(struct platform_device *pdev)
>  	if (!docg3_floors)
>  		goto nomem;
>  
> -	ret = 0;
>  	for (floor = 0; floor < DOC_MAX_NBFLOORS; floor++) {
>  		mtd = doc_probe_device(base, floor, dev);
> -		if (floor = 0 && !mtd)
> -			goto notfound;
> -		if (!IS_ERR_OR_NULL(mtd))
> -			ret = mtd_device_parse_register(mtd, part_probes,
> -							NULL, NULL, 0);
> -		else
> +		if (IS_ERR(mtd)) {
>  			ret = PTR_ERR(mtd);
> +			goto err_probe;
> +		}
> +		if (!mtd) {
> +			if (floor = 0)
> +				goto notfound;
> +			else
> +				continue;
> +		}
>  		docg3_floors[floor] = mtd;
> +		ret = mtd_device_parse_register(mtd, part_probes, NULL, NULL,
> +						0);
>  		if (ret)
>  			goto err_probe;
> -		if (mtd)
> -			found++;
> +		found++;
>  	}
Okay, this looks better that the original code.

>  
>  	if (!found)
> @@ -1138,9 +1141,11 @@ notfound:
>  	ret = -ENODEV;
>  	dev_info(dev, "No supported DiskOnChip found\n");
>  err_probe:
> -	for (floor = 0; floor < DOC_MAX_NBFLOORS; floor++)
> +	for (floor = 0; floor < DOC_MAX_NBFLOORS; floor++) {
>  		if (docg3_floors[floor])
>  			doc_release_device(docg3_floors[floor]);
> +	}
> +	kfree(docg3_floors);
This is in conflict. Could you drop that hunk and wait for the other patch to go
upstream ? Or alternatively use the whole serie in [2] as your base ?
I think some patches of the serie didn't make it in the tree you're
using.

Could you have a look at the tree with the whole serie, and rebase your patch on
top of it ?


Cheers.

-- 
Robert

[1]: http://lists.infradead.org/pipermail/linux-mtd/2011-November/038496.html
[2]: http://lists.infradead.org/pipermail/linux-mtd/2011-November/038483.html