diff mbox

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

Message ID b379ba57-d926-2572-17ec-5bbe166b0e37@users.sourceforge.net
State Rejected
Headers show

Commit Message

SF Markus Elfring Jan. 12, 2017, 10:35 a.m. UTC 
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(-)

Comments

Cyrille Pitchen Jan. 12, 2017, 1 p.m. UTC | #1 
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;
>  
>
SF Markus Elfring Jan. 12, 2017, 4:50 p.m. UTC | #2 
> The indentation has been changed and the new one looks wrong...

The source code formatting contained various open issues before already.


> 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.

Such a development task is too much for me today.


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

How much do you care for the usage of a function like “kmalloc_array”?

Regards,
Markus
Marek Vasut Jan. 12, 2017, 4:58 p.m. UTC | #3 
On 01/12/2017 05:50 PM, SF Markus Elfring wrote:
>> The indentation has been changed and the new one looks wrong...
> 
> The source code formatting contained various open issues before already.
> 
> 
>> 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.
> 
> Such a development task is too much for me today.

Let me officially NAK this patch.

>> Reviewing such dummy/automatic patches is a pure waste of time,
>> so personally I think we should just ignore them.
> 
> How much do you care for the usage of a function like “kmalloc_array”?

Not at all, there are more pressing issues.
kernel test robot Jan. 13, 2017, 9:32 a.m. UTC | #4 
Hi Markus,

[auto build test WARNING on mtd/master]
[also build test WARNING on v4.10-rc3 next-20170113]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/SF-Markus-Elfring/MTD-FTL-Fine-tuning-for-two-function-implementations/20170113-163618
base:   git://git.infradead.org/linux-mtd.git master
config: sparc64-allmodconfig (attached as .config)
compiler: sparc64-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=sparc64 

All warnings (new ones prefixed by >>):

   drivers/mtd/ftl.c: In function 'build_maps':
>> drivers/mtd/ftl.c:214:5: warning: this 'for' clause does not guard... [-Wmisleading-indentation]
        for (i = 0; i < part->DataUnits; i++)
        ^~~
   drivers/mtd/ftl.c:216:2: note: ...this statement, but the latter is misleadingly indented as if it is guarded by the 'for'
     part->XferInfo = kmalloc_array(part->header.NumTransferUnits,
     ^~~~

vim +/for +214 drivers/mtd/ftl.c

^1da177e Linus Torvalds  2005-04-16  198  static int build_maps(partition_t *part)
^1da177e Linus Torvalds  2005-04-16  199  {
^1da177e Linus Torvalds  2005-04-16  200      erase_unit_header_t header;
3854be77 David Woodhouse 2008-12-10  201      uint16_t xvalid, xtrans, i;
3854be77 David Woodhouse 2008-12-10  202      unsigned blocks, j;
^1da177e Linus Torvalds  2005-04-16  203      int hdr_ok, ret = -1;
^1da177e Linus Torvalds  2005-04-16  204      ssize_t retval;
^1da177e Linus Torvalds  2005-04-16  205      loff_t offset;
^1da177e Linus Torvalds  2005-04-16  206  
^1da177e Linus Torvalds  2005-04-16  207      /* Set up erase unit maps */
^1da177e Linus Torvalds  2005-04-16  208      part->DataUnits = le16_to_cpu(part->header.NumEraseUnits) -
^1da177e Linus Torvalds  2005-04-16  209  	part->header.NumTransferUnits;
b6c411b0 Markus Elfring  2017-01-12  210  	part->EUNInfo = kmalloc_array(part->DataUnits, sizeof(*part->EUNInfo),
^1da177e Linus Torvalds  2005-04-16  211  				      GFP_KERNEL);
^1da177e Linus Torvalds  2005-04-16  212      if (!part->EUNInfo)
^1da177e Linus Torvalds  2005-04-16  213  	    goto out;
^1da177e Linus Torvalds  2005-04-16 @214      for (i = 0; i < part->DataUnits; i++)
^1da177e Linus Torvalds  2005-04-16  215  	part->EUNInfo[i].Offset = 0xffffffff;
b6c411b0 Markus Elfring  2017-01-12  216  	part->XferInfo = kmalloc_array(part->header.NumTransferUnits,
b6c411b0 Markus Elfring  2017-01-12  217  				       sizeof(*part->XferInfo), GFP_KERNEL);
^1da177e Linus Torvalds  2005-04-16  218      if (!part->XferInfo)
^1da177e Linus Torvalds  2005-04-16  219  	    goto out_EUNInfo;
^1da177e Linus Torvalds  2005-04-16  220  
^1da177e Linus Torvalds  2005-04-16  221      xvalid = xtrans = 0;
^1da177e Linus Torvalds  2005-04-16  222      for (i = 0; i < le16_to_cpu(part->header.NumEraseUnits); i++) {

:::::: The code at line 214 was first introduced by commit
:::::: 1da177e4c3f41524e886b7f1b8a0c1fc7321cac2 Linux-2.6.12-rc2

:::::: TO: Linus Torvalds <torvalds@ppc970.osdl.org>
:::::: CC: Linus Torvalds <torvalds@ppc970.osdl.org>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
diff mbox

Patch

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);
     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);
     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);
     if (!part->bam_cache)
 	    goto out_VirtualBlockMap;