[PATCH 0/3] MTD-FTL: Fine-tuning for two function implementations

[PATCH 0/3] MTD-FTL: Fine-tuning for two function implementations

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 12 Jan 2017 11:24:12 +0100

A few update suggestions were taken into account
from static source code analysis.

Markus Elfring (3):
  Use kmalloc_array() in build_maps()
  Delete an error message for a failed memory allocation in ftl_add_mtd()
  Improve another size determination in ftl_add_mtd()

 drivers/mtd/ftl.c | 21 ++++++++-------------
 1 file changed, 8 insertions(+), 13 deletions(-)

-- 
2.11.0

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

[SF Markus Elfring]

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);
     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;
 
-- 
2.11.0

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

[Cyrille Pitchen]

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

Re: mtd/ftl: Use kmalloc_array() in build_maps()

[SF Markus Elfring]

> 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

Re: mtd/ftl: Use kmalloc_array() in build_maps()

[Marek Vasut]

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.

-- 
Best regards,
Marek Vasut

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

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3485 bytes --]

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 48625 bytes --]

[PATCH 2/3] mtd/ftl: Delete an error message for a failed memory allocation in ftl_add_mtd()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 12 Jan 2017 11:11:47 +0100

The script "checkpatch.pl" pointed information out like the following.

WARNING: Possible unnecessary 'out of memory' message

Thus fix the affected source code place.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/mtd/ftl.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/mtd/ftl.c b/drivers/mtd/ftl.c
index ef2f38b6a837..7af23110be6e 100644
--- a/drivers/mtd/ftl.c
+++ b/drivers/mtd/ftl.c
@@ -1042,12 +1042,8 @@ static void ftl_add_mtd(struct mtd_blktrans_ops *tr, struct mtd_info *mtd)
 	partition_t *partition;
 
 	partition = kzalloc(sizeof(partition_t), GFP_KERNEL);
-
-	if (!partition) {
-		printk(KERN_WARNING "No memory to scan for FTL on %s\n",
-		       mtd->name);
+	if (!partition)
 		return;
-	}
 
 	partition->mbd.mtd = mtd;
 
-- 
2.11.0

[PATCH 3/3] mtd/ftl: Improve another size determination in ftl_add_mtd()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 12 Jan 2017 11:18:59 +0100

Replace the specification of a data type by a pointer dereference
as the parameter for the operator "sizeof" 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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/ftl.c b/drivers/mtd/ftl.c
index 7af23110be6e..70d583aa44ad 100644
--- a/drivers/mtd/ftl.c
+++ b/drivers/mtd/ftl.c
@@ -1041,7 +1041,7 @@ static void ftl_add_mtd(struct mtd_blktrans_ops *tr, struct mtd_info *mtd)
 {
 	partition_t *partition;
 
-	partition = kzalloc(sizeof(partition_t), GFP_KERNEL);
+	partition = kzalloc(sizeof(*partition), GFP_KERNEL);
 	if (!partition)
 		return;
 
-- 
2.11.0