Re: [PATCH 1/3] mtd/ftl: Use kmalloc_array() in build_maps()

[Cyrille Pitchen <cyrille.pitchen@atmel.com>]

Le 12/01/2017 à 11:35, SF Markus Elfring a écrit :
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Thu, 12 Jan 2017 10:42:25 +0100
> 
> * Multiplications for the size determination of memory allocations
>   indicated that array data structures should be processed.
>   Thus use the corresponding function "kmalloc_array".
> 
>   This issue was detected by using the Coccinelle software.
> 
> * Replace the specification of data types by pointer dereferences
>   to make the corresponding size determination a bit safer according to
>   the Linux coding style convention.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/mtd/ftl.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/mtd/ftl.c b/drivers/mtd/ftl.c
> index 9fb3b0dcdac2..ef2f38b6a837 100644
> --- a/drivers/mtd/ftl.c
> +++ b/drivers/mtd/ftl.c
> @@ -207,15 +207,14 @@ static int build_maps(partition_t *part)
>      /* Set up erase unit maps */
>      part->DataUnits = le16_to_cpu(part->header.NumEraseUnits) -
>  	part->header.NumTransferUnits;
> -    part->EUNInfo = kmalloc(part->DataUnits * sizeof(struct eun_info_t),
> -			    GFP_KERNEL);
> +	part->EUNInfo = kmalloc_array(part->DataUnits, sizeof(*part->EUNInfo),
> +				      GFP_KERNEL);

The indentation has been changed and the new one looks wrong...

I understand the original indentation, with spaces, doesn't follow the
Linux coding style but at least it's consistent and readable.

Your patch uses a different indentation with tabs, which now mixes two
different indentations: IMHO, this is worst than before.

If you want to fix the indentation to make it compliant with the Linux
coding style, do it on the whole file so every thing is uniform.

Reviewing such dummy/automatic patches is a pure waste of time, so
personally I think we should just ignore them.

>      if (!part->EUNInfo)
>  	    goto out;
>      for (i = 0; i < part->DataUnits; i++)
>  	part->EUNInfo[i].Offset = 0xffffffff;
> -    part->XferInfo > -	kmalloc(part->header.NumTransferUnits * sizeof(struct xfer_info_t),
> -		GFP_KERNEL);
> +	part->XferInfo = kmalloc_array(part->header.NumTransferUnits,
> +				       sizeof(*part->XferInfo), GFP_KERNEL);

Another indentation issue is introduced here too...

>      if (!part->XferInfo)
>  	    goto out_EUNInfo;
>  
> @@ -275,8 +274,8 @@ static int build_maps(partition_t *part)
>      memset(part->VirtualBlockMap, 0xff, blocks * sizeof(uint32_t));
>      part->BlocksPerUnit = (1 << header.EraseUnitSize) >> header.BlockSize;
>  
> -    part->bam_cache = kmalloc(part->BlocksPerUnit * sizeof(uint32_t),
> -			      GFP_KERNEL);
> +	part->bam_cache = kmalloc_array(part->BlocksPerUnit,
> +					sizeof(*part->bam_cache), GFP_KERNEL);

+1
>      if (!part->bam_cache)
>  	    goto out_VirtualBlockMap;
>  
>