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

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

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 18 Apr 2017 21:54:32 +0200

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

Markus Elfring (3):
  Use kcalloc() in ahci_platform_get_resources()
  Use devm_kcalloc() in ahci_platform_get_resources()
  Use devm_kcalloc() in mv_platform_probe()

 drivers/ata/libahci_platform.c | 11 ++++++-----
 drivers/ata/sata_mv.c          | 10 ++++++----
 2 files changed, 12 insertions(+), 9 deletions(-)

-- 
2.12.2

[PATCH 1/3] ata: libahci: Use kcalloc() in ahci_platform_get_resources()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 18 Apr 2017 21:07:34 +0200

A multiplication for the size determination of a memory allocation
indicated that an array data structure should be processed.
Thus use the corresponding function "kcalloc".

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/ata/libahci_platform.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
index aaa761b9081c..c68ea903b5bf 100644
--- a/drivers/ata/libahci_platform.c
+++ b/drivers/ata/libahci_platform.c
@@ -408,8 +408,9 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev)
 		rc = -ENOMEM;
 		goto err_out;
 	}
-	sz = hpriv->nports * sizeof(*hpriv->target_pwrs);
-	hpriv->target_pwrs = kzalloc(sz, GFP_KERNEL);
+	hpriv->target_pwrs = kcalloc(hpriv->nports,
+				     sizeof(*hpriv->target_pwrs),
+				     GFP_KERNEL);
 	if (!hpriv->target_pwrs) {
 		rc = -ENOMEM;
 		goto err_out;
-- 
2.12.2

[PATCH 2/3] ata: libahci: Use devm_kcalloc() in ahci_platform_get_resources()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 18 Apr 2017 21:15:48 +0200

* A multiplication for the size determination of a memory allocation
  indicated that an array data structure should be processed.
  Thus use the corresponding function "devm_kcalloc".

  This issue was detected by using the Coccinelle software.

* Delete the local variable "sz" which became unnecessary
  with this refactoring.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/ata/libahci_platform.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
index c68ea903b5bf..2bce61f7482f 100644
--- a/drivers/ata/libahci_platform.c
+++ b/drivers/ata/libahci_platform.c
@@ -350,7 +350,7 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev)
 	struct ahci_host_priv *hpriv;
 	struct clk *clk;
 	struct device_node *child;
-	int i, sz, enabled_ports = 0, rc = -ENOMEM, child_nodes;
+	int i, enabled_ports = 0, rc = -ENOMEM, child_nodes;
 	u32 mask_port_map = 0;
 
 	if (!devres_open_group(dev, NULL, GFP_KERNEL))
@@ -402,8 +402,8 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev)
 	if (!child_nodes)
 		hpriv->nports = 1;
 
-	sz = hpriv->nports * sizeof(*hpriv->phys);
-	hpriv->phys = devm_kzalloc(dev, sz, GFP_KERNEL);
+	hpriv->phys = devm_kcalloc(dev, hpriv->nports, sizeof(*hpriv->phys),
+				   GFP_KERNEL);
 	if (!hpriv->phys) {
 		rc = -ENOMEM;
 		goto err_out;
-- 
2.12.2

[PATCH 3/3] sata_mv: Use devm_kcalloc() in mv_platform_probe()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 18 Apr 2017 21:36:43 +0200

* Multiplications for the size determination of memory allocations
  indicated that array data structures should be processed.
  Thus use the corresponding function "devm_kcalloc".

  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/ata/sata_mv.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
index b66bcda88320..49abe386c7f6 100644
--- a/drivers/ata/sata_mv.c
+++ b/drivers/ata/sata_mv.c
@@ -4118,13 +4118,15 @@ static int mv_platform_probe(struct platform_device *pdev)
 
 	if (!host || !hpriv)
 		return -ENOMEM;
-	hpriv->port_clks = devm_kzalloc(&pdev->dev,
-					sizeof(struct clk *) * n_ports,
+	hpriv->port_clks = devm_kcalloc(&pdev->dev,
+					n_ports,
+					sizeof(*hpriv->port_clks),
 					GFP_KERNEL);
 	if (!hpriv->port_clks)
 		return -ENOMEM;
-	hpriv->port_phys = devm_kzalloc(&pdev->dev,
-					sizeof(struct phy *) * n_ports,
+	hpriv->port_phys = devm_kcalloc(&pdev->dev,
+					n_ports,
+					sizeof(*hpriv->port_phys),
 					GFP_KERNEL);
 	if (!hpriv->port_phys)
 		return -ENOMEM;
-- 
2.12.2

Re: [PATCH 0/3] SATA: Fine-tuning for two function implementations

[Tejun Heo]

Hello,

On Tue, Apr 18, 2017 at 10:00:37PM +0200, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Tue, 18 Apr 2017 21:54:32 +0200
> 
> A few update suggestions were taken into account
> from static source code analysis.

Hmmm, allocs -> callocs.  Are these actually beneficial?  If so, why?
Because one multiplication is rolled into the call?

Thanks.

-- 
tejun

Re: SATA: Fine-tuning for two function implementations

[SF Markus Elfring]

> Hmmm, allocs -> callocs.  Are these actually beneficial?  If so, why?
> Because one multiplication is rolled into the call?

Did the previous size calculations contain the general possibility for
integer overflows?
https://cwe.mitre.org/data/definitions/190.html

* Will the computed values usually stay within the limits of the used
  data types so far?

* How much do you care for corresponding checks and source code annotations
  by functions like “devm_kcalloc”?

Regards,
Markus

Re: [PATCH 0/3] SATA: Fine-tuning for two function implementations

[Bartlomiej Zolnierkiewicz]

Hi,

On Friday, April 28, 2017 05:53:34 PM Tejun Heo wrote:
> Hello,
> 
> On Tue, Apr 18, 2017 at 10:00:37PM +0200, SF Markus Elfring wrote:
> > From: Markus Elfring <elfring@users.sourceforge.net>
> > Date: Tue, 18 Apr 2017 21:54:32 +0200
> > 
> > A few update suggestions were taken into account
> > from static source code analysis.
> 
> Hmmm, allocs -> callocs.  Are these actually beneficial?  If so, why?
> Because one multiplication is rolled into the call?

Each conversion (i.e. I tried the one from patch #1) seems to add
an extra 24 bytes to the resulting code size (using gcc 4.8.4 for
ARM32 cross-compilation) so I don't see much point in the automatic
conversions. Only instances containing size calculations with
real possibility for integer overflows should be converted and
the patchset under discussion contains no such instances.

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics