[PATCH 0/5] PowerPC-pSeries: Adjustments for seven function implementations

[PATCH 0/5] PowerPC-pSeries: Adjustments for seven function implementations

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 18 Oct 2017 21:11:23 +0200

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

Markus Elfring (5):
  Delete five error messages for a failed memory allocation
  Improve nine size determinations
  Delete an unnecessary variable initialisation in iommu_pseries_alloc_group()
  Return directly after a failed kzalloc_node() in iommu_pseries_alloc_group()
  Less function calls in iommu_pseries_alloc_group() after error detection

 arch/powerpc/platforms/pseries/dlpar.c     |  5 ++--
 arch/powerpc/platforms/pseries/dtl.c       |  5 +---
 arch/powerpc/platforms/pseries/hvcserver.c |  7 ++----
 arch/powerpc/platforms/pseries/iommu.c     | 40 ++++++++++++------------------
 arch/powerpc/platforms/pseries/vio.c       |  9 +++----
 5 files changed, 24 insertions(+), 42 deletions(-)

-- 
2.14.2

[PATCH 1/5] powerpc-pseries: Delete five error messages for a failed memory allocation

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 18 Oct 2017 16:39:01 +0200

Omit extra messages for a memory allocation failure in these functions.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 arch/powerpc/platforms/pseries/dtl.c       |  5 +----
 arch/powerpc/platforms/pseries/hvcserver.c |  2 --
 arch/powerpc/platforms/pseries/iommu.c     | 11 +++--------
 arch/powerpc/platforms/pseries/vio.c       |  4 +---
 4 files changed, 5 insertions(+), 17 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/dtl.c b/arch/powerpc/platforms/pseries/dtl.c
index 18014cdeb590..467b9f578a7d 100644
--- a/arch/powerpc/platforms/pseries/dtl.c
+++ b/arch/powerpc/platforms/pseries/dtl.c
@@ -203,11 +203,8 @@ static int dtl_enable(struct dtl *dtl)
 
 	n_entries = dtl_buf_entries;
 	buf = kmem_cache_alloc_node(dtl_cache, GFP_KERNEL, cpu_to_node(dtl->cpu));
-	if (!buf) {
-		printk(KERN_WARNING "%s: buffer alloc failed for cpu %d\n",
-				__func__, dtl->cpu);
+	if (!buf)
 		return -ENOMEM;
-	}
 
 	spin_lock(&dtl->lock);
 	rc = -EBUSY;
diff --git a/arch/powerpc/platforms/pseries/hvcserver.c b/arch/powerpc/platforms/pseries/hvcserver.c
index 94a6e5612b0d..db2c20e65a58 100644
--- a/arch/powerpc/platforms/pseries/hvcserver.c
+++ b/arch/powerpc/platforms/pseries/hvcserver.c
@@ -177,8 +177,6 @@ int hvcs_get_partner_info(uint32_t unit_address, struct list_head *head,
 				GFP_ATOMIC);
 
 		if (!next_partner_info) {
-			printk(KERN_WARNING "HVCONSOLE: kmalloc() failed to"
-				" allocate partner info struct.\n");
 			hvcs_free_partner_info(head);
 			return -ENOMEM;
 		}
diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c
index 7c181467d0ad..c92822fdf269 100644
--- a/arch/powerpc/platforms/pseries/iommu.c
+++ b/arch/powerpc/platforms/pseries/iommu.c
@@ -1063,19 +1063,14 @@ static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn)
 	}
 	len = order_base_2(max_addr);
 	win64 = kzalloc(sizeof(struct property), GFP_KERNEL);
-	if (!win64) {
-		dev_info(&dev->dev,
-			"couldn't allocate property for 64bit dma window\n");
+	if (!win64)
 		goto out_failed;
-	}
+
 	win64->name = kstrdup(DIRECT64_PROPNAME, GFP_KERNEL);
 	win64->value = ddwprop = kmalloc(sizeof(*ddwprop), GFP_KERNEL);
 	win64->length = sizeof(*ddwprop);
-	if (!win64->name || !win64->value) {
-		dev_info(&dev->dev,
-			"couldn't allocate property name and value\n");
+	if (!win64->name || !win64->value)
 		goto out_free_prop;
-	}
 
 	ret = create_ddw(dev, ddw_avail, &create, page_shift, len);
 	if (ret != 0)
diff --git a/arch/powerpc/platforms/pseries/vio.c b/arch/powerpc/platforms/pseries/vio.c
index 12277bc9fd9e..74b919040e68 100644
--- a/arch/powerpc/platforms/pseries/vio.c
+++ b/arch/powerpc/platforms/pseries/vio.c
@@ -1386,10 +1386,8 @@ struct vio_dev *vio_register_device_node(struct device_node *of_node)
 
 	/* allocate a vio_dev for this node */
 	viodev = kzalloc(sizeof(struct vio_dev), GFP_KERNEL);
-	if (viodev == NULL) {
-		pr_warn("%s: allocation failure for VIO device.\n", __func__);
+	if (!viodev)
 		return NULL;
-	}
 
 	/* we need the 'device_type' property, in order to match with drivers */
 	viodev->family = family;
-- 
2.14.2

[PATCH 2/5] powerpc-pseries: Improve nine size determinations

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 18 Oct 2017 18:18:11 +0200

Replace the specification of data structures by pointer dereferences
as the parameter for the operator "sizeof" to make the corresponding size
determination a bit safer according to the Linux coding style convention.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 arch/powerpc/platforms/pseries/dlpar.c     |  5 ++---
 arch/powerpc/platforms/pseries/hvcserver.c |  5 ++---
 arch/powerpc/platforms/pseries/iommu.c     | 10 ++++------
 arch/powerpc/platforms/pseries/vio.c       |  5 ++---
 4 files changed, 10 insertions(+), 15 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/dlpar.c b/arch/powerpc/platforms/pseries/dlpar.c
index 6e35780c5962..dca88997cb4b 100644
--- a/arch/powerpc/platforms/pseries/dlpar.c
+++ b/arch/powerpc/platforms/pseries/dlpar.c
@@ -389,11 +389,10 @@ void queue_hotplug_event(struct pseries_hp_errorlog *hp_errlog,
 	struct pseries_hp_work *work;
 	struct pseries_hp_errorlog *hp_errlog_copy;
 
-	hp_errlog_copy = kmalloc(sizeof(struct pseries_hp_errorlog),
-				 GFP_KERNEL);
+	hp_errlog_copy = kmalloc(sizeof(*hp_errlog_copy), GFP_KERNEL);
 	memcpy(hp_errlog_copy, hp_errlog, sizeof(struct pseries_hp_errorlog));
 
-	work = kmalloc(sizeof(struct pseries_hp_work), GFP_KERNEL);
+	work = kmalloc(sizeof(*work), GFP_KERNEL);
 	if (work) {
 		INIT_WORK((struct work_struct *)work, pseries_hp_work_fn);
 		work->errlog = hp_errlog_copy;
diff --git a/arch/powerpc/platforms/pseries/hvcserver.c b/arch/powerpc/platforms/pseries/hvcserver.c
index db2c20e65a58..b85cca04dd7d 100644
--- a/arch/powerpc/platforms/pseries/hvcserver.c
+++ b/arch/powerpc/platforms/pseries/hvcserver.c
@@ -173,9 +173,8 @@ int hvcs_get_partner_info(uint32_t unit_address, struct list_head *head,
 
 		/* This is a very small struct and will be freed soon in
 		 * hvcs_free_partner_info(). */
-		next_partner_info = kmalloc(sizeof(struct hvcs_partner_info),
-				GFP_ATOMIC);
-
+		next_partner_info = kmalloc(sizeof(*next_partner_info),
+					    GFP_ATOMIC);
 		if (!next_partner_info) {
 			hvcs_free_partner_info(head);
 			return -ENOMEM;
diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c
index c92822fdf269..b37d4fb20d1c 100644
--- a/arch/powerpc/platforms/pseries/iommu.c
+++ b/arch/powerpc/platforms/pseries/iommu.c
@@ -59,17 +59,15 @@ static struct iommu_table_group *iommu_pseries_alloc_group(int node)
 	struct iommu_table *tbl = NULL;
 	struct iommu_table_group_link *tgl = NULL;
 
-	table_group = kzalloc_node(sizeof(struct iommu_table_group), GFP_KERNEL,
-			   node);
+	table_group = kzalloc_node(sizeof(*table_group), GFP_KERNEL, node);
 	if (!table_group)
 		goto fail_exit;
 
-	tbl = kzalloc_node(sizeof(struct iommu_table), GFP_KERNEL, node);
+	tbl = kzalloc_node(sizeof(*tbl), GFP_KERNEL, node);
 	if (!tbl)
 		goto fail_exit;
 
-	tgl = kzalloc_node(sizeof(struct iommu_table_group_link), GFP_KERNEL,
-			node);
+	tgl = kzalloc_node(sizeof(*tgl), GFP_KERNEL, node);
 	if (!tgl)
 		goto fail_exit;
 
@@ -1062,7 +1060,7 @@ static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn)
 		goto out_failed;
 	}
 	len = order_base_2(max_addr);
-	win64 = kzalloc(sizeof(struct property), GFP_KERNEL);
+	win64 = kzalloc(sizeof(*win64), GFP_KERNEL);
 	if (!win64)
 		goto out_failed;
 
diff --git a/arch/powerpc/platforms/pseries/vio.c b/arch/powerpc/platforms/pseries/vio.c
index 74b919040e68..cf0939eb3aee 100644
--- a/arch/powerpc/platforms/pseries/vio.c
+++ b/arch/powerpc/platforms/pseries/vio.c
@@ -754,8 +754,7 @@ static int vio_cmo_bus_probe(struct vio_dev *viodev)
 			viodev->cmo.desired = VIO_CMO_MIN_ENT;
 		size = VIO_CMO_MIN_ENT;
 
-		dev_ent = kmalloc(sizeof(struct vio_cmo_dev_entry),
-		                  GFP_KERNEL);
+		dev_ent = kmalloc(sizeof(*dev_ent), GFP_KERNEL);
 		if (!dev_ent)
 			return -ENOMEM;
 
@@ -1385,7 +1384,7 @@ struct vio_dev *vio_register_device_node(struct device_node *of_node)
 	}
 
 	/* allocate a vio_dev for this node */
-	viodev = kzalloc(sizeof(struct vio_dev), GFP_KERNEL);
+	viodev = kzalloc(sizeof(*viodev), GFP_KERNEL);
 	if (!viodev)
 		return NULL;
 
-- 
2.14.2

[PATCH 3/5] powerpc-pseries: Delete an unnecessary variable initialisation in iommu_pseries_alloc_group()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 18 Oct 2017 19:14:39 +0200

The variable "table_group" will be set to an appropriate pointer.
Thus omit the explicit initialisation at the beginning.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 arch/powerpc/platforms/pseries/iommu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c
index b37d4fb20d1c..b6c12b8e3ace 100644
--- a/arch/powerpc/platforms/pseries/iommu.c
+++ b/arch/powerpc/platforms/pseries/iommu.c
@@ -55,7 +55,7 @@
 
 static struct iommu_table_group *iommu_pseries_alloc_group(int node)
 {
-	struct iommu_table_group *table_group = NULL;
+	struct iommu_table_group *table_group;
 	struct iommu_table *tbl = NULL;
 	struct iommu_table_group_link *tgl = NULL;
 
-- 
2.14.2

[PATCH 4/5] powerpc-pseries: Return directly after a failed kzalloc_node() in iommu_pseries_alloc_group()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 18 Oct 2017 20:15:32 +0200

Return directly after a call of the function "kzalloc_node" failed
at the beginning.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 arch/powerpc/platforms/pseries/iommu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c
index b6c12b8e3ace..207ff8351af1 100644
--- a/arch/powerpc/platforms/pseries/iommu.c
+++ b/arch/powerpc/platforms/pseries/iommu.c
@@ -61,7 +61,7 @@ static struct iommu_table_group *iommu_pseries_alloc_group(int node)
 
 	table_group = kzalloc_node(sizeof(*table_group), GFP_KERNEL, node);
 	if (!table_group)
-		goto fail_exit;
+		return NULL;
 
 	tbl = kzalloc_node(sizeof(*tbl), GFP_KERNEL, node);
 	if (!tbl)
-- 
2.14.2

[PATCH 5/5] powerpc-pseries: Less function calls in iommu_pseries_alloc_group() after error detection

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 18 Oct 2017 20:48:52 +0200

The kfree() function was called in up to two cases by the
iommu_pseries_alloc_group() function during error handling
even if the passed variable contained a null pointer.

* Adjust jump targets according to the Linux coding style convention.

* Delete initialisations for the variables "tbl" and "tgl"
  which became unnecessary with this refactoring.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 arch/powerpc/platforms/pseries/iommu.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c
index 207ff8351af1..13b424f34039 100644
--- a/arch/powerpc/platforms/pseries/iommu.c
+++ b/arch/powerpc/platforms/pseries/iommu.c
@@ -56,8 +56,8 @@
 static struct iommu_table_group *iommu_pseries_alloc_group(int node)
 {
 	struct iommu_table_group *table_group;
-	struct iommu_table *tbl = NULL;
-	struct iommu_table_group_link *tgl = NULL;
+	struct iommu_table *tbl;
+	struct iommu_table_group_link *tgl;
 
 	table_group = kzalloc_node(sizeof(*table_group), GFP_KERNEL, node);
 	if (!table_group)
@@ -65,11 +65,11 @@ static struct iommu_table_group *iommu_pseries_alloc_group(int node)
 
 	tbl = kzalloc_node(sizeof(*tbl), GFP_KERNEL, node);
 	if (!tbl)
-		goto fail_exit;
+		goto free_group;
 
 	tgl = kzalloc_node(sizeof(*tgl), GFP_KERNEL, node);
 	if (!tgl)
-		goto fail_exit;
+		goto free_table;
 
 	INIT_LIST_HEAD_RCU(&tbl->it_group_list);
 	kref_init(&tbl->it_kref);
@@ -80,11 +80,10 @@ static struct iommu_table_group *iommu_pseries_alloc_group(int node)
 
 	return table_group;
 
-fail_exit:
-	kfree(tgl);
-	kfree(table_group);
+free_table:
 	kfree(tbl);
-
+free_group:
+	kfree(table_group);
 	return NULL;
 }
 
-- 
2.14.2

Re: [PATCH 3/5] powerpc-pseries: Delete an unnecessary variable initialisation in iommu_pseries_alloc_group()

[Michal Suchánek]

Hello,

On Wed, 18 Oct 2017 21:24:25 +0200
SF Markus Elfring <elfring@users.sourceforge.net> wrote:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Wed, 18 Oct 2017 19:14:39 +0200
> 
> The variable "table_group" will be set to an appropriate pointer.
> Thus omit the explicit initialisation at the beginning.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  arch/powerpc/platforms/pseries/iommu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/platforms/pseries/iommu.c
> b/arch/powerpc/platforms/pseries/iommu.c index
> b37d4fb20d1c..b6c12b8e3ace 100644 ---
> a/arch/powerpc/platforms/pseries/iommu.c +++
> b/arch/powerpc/platforms/pseries/iommu.c @@ -55,7 +55,7 @@
>  
>  static struct iommu_table_group *iommu_pseries_alloc_group(int node)
>  {
> -	struct iommu_table_group *table_group = NULL;
> +	struct iommu_table_group *table_group;
>  	struct iommu_table *tbl = NULL;
>  	struct iommu_table_group_link *tgl = NULL;
>  

I think initializing pointers to NULL is generally a good idea.

If there is no use of the variable before it is reinitialized by
allocation gcc is free to optimize out the variable and its initial
value.

On the other hand, if the code is changed later and use of the variable
becomes possible you may crash (and get a gcc warning, too).

Removing these initializers adds no value, to the contrary.

Thanks

Michal

Re: [PATCH 4/5] powerpc-pseries: Return directly after a failed kzalloc_node() in iommu_pseries_alloc_group()

[Michal Suchánek]

Hello,

On Wed, 18 Oct 2017 21:26:10 +0200
SF Markus Elfring <elfring@users.sourceforge.net> wrote:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Wed, 18 Oct 2017 20:15:32 +0200
> 
> Return directly after a call of the function "kzalloc_node" failed
> at the beginning.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  arch/powerpc/platforms/pseries/iommu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/platforms/pseries/iommu.c
> b/arch/powerpc/platforms/pseries/iommu.c index
> b6c12b8e3ace..207ff8351af1 100644 ---
> a/arch/powerpc/platforms/pseries/iommu.c +++
> b/arch/powerpc/platforms/pseries/iommu.c @@ -61,7 +61,7 @@ static
> struct iommu_table_group *iommu_pseries_alloc_group(int node) 
>  	table_group = kzalloc_node(sizeof(*table_group), GFP_KERNEL,
> node); if (!table_group)
> -		goto fail_exit;
> +		return NULL;
>  
>  	tbl = kzalloc_node(sizeof(*tbl), GFP_KERNEL, node);
>  	if (!tbl)

I have seen quite a few fixes that do inverse of this patch after a
piece of code allocating some extra piece of memory was added before
code that just returns on fail because it is the first allocation in
the function.

This is not useful.

A final fail_exit that frees everything that could have been allocated
is much better. That applies to 5/5 as well.

Thanks

Michal

Re: [PATCH 3/5] powerpc-pseries: Delete an unnecessary variable initialisation in iommu_pseries_alloc_group()

[SF Markus Elfring]

>>  static struct iommu_table_group *iommu_pseries_alloc_group(int node)
>>  {
>> -	struct iommu_table_group *table_group = NULL;
>> +	struct iommu_table_group *table_group;
>>  	struct iommu_table *tbl = NULL;
>>  	struct iommu_table_group_link *tgl = NULL;
>>  
> 
> I think initializing pointers to NULL is generally a good idea.

This one would also not be needed if the call of the function “kzalloc_node”
could be specified in the same statement.


> Removing these initializers adds no value, to the contrary.

This small update step is just a “preparation” for the subsequent two suggestions
in this patch series.

Regards,
Markus

Re: [PATCH 4/5] powerpc-pseries: Return directly after a failed kzalloc_node() in iommu_pseries_alloc_group()

[SF Markus Elfring]

>> @@ -61,7 +61,7 @@ static struct iommu_table_group *iommu_pseries_alloc_group(int node) 
>>  	table_group = kzalloc_node(sizeof(*table_group), GFP_KERNEL,
>> node); if (!table_group)
>> -		goto fail_exit;
>> +		return NULL;
>>  
>>  	tbl = kzalloc_node(sizeof(*tbl), GFP_KERNEL, node);
>>  	if (!tbl)
> 
> I have seen quite a few fixes that do inverse of this patch after a
> piece of code allocating some extra piece of memory was added before
> code that just returns on fail because it is the first allocation in
> the function.
> 
> This is not useful.

How do you think about an information from the section “7) Centralized exiting
of functions” in the document “coding-style.rst” then?

“…
If there is no cleanup needed then just return directly.
…”


> A final fail_exit that frees everything that could have been allocated
> is much better.

I got an other software development opinion for such use cases.
I prefer only required function calls there.

Regards,
Markus

Re: [PATCH 4/5] powerpc-pseries: Return directly after a failed kzalloc_node() in iommu_pseries_alloc_group()

[Michal Suchánek]

On Thu, 19 Oct 2017 14:04:43 +0200
SF Markus Elfring <elfring@users.sourceforge.net> wrote:

> >> @@ -61,7 +61,7 @@ static struct iommu_table_group
> >> *iommu_pseries_alloc_group(int node) table_group =
> >> kzalloc_node(sizeof(*table_group), GFP_KERNEL, node); if
> >> (!table_group)
> >> -		goto fail_exit;
> >> +		return NULL;
> >>  
> >>  	tbl = kzalloc_node(sizeof(*tbl), GFP_KERNEL, node);
> >>  	if (!tbl)  
> > 
> > I have seen quite a few fixes that do inverse of this patch after a
> > piece of code allocating some extra piece of memory was added before
> > code that just returns on fail because it is the first allocation in
> > the function.
> > 
> > This is not useful.  
> 
> How do you think about an information from the section “7)
> Centralized exiting of functions” in the document “coding-style.rst”
> then?
> 
> “…
> If there is no cleanup needed then just return directly.
> …”

There is also stated benefit

"
- errors by not updating individual exit points when making
  modifications are prevented
"

which is furthered by using the common cleanup even in case no cleanup
is required but running the cleanup does not cause any harm.

Thanks

Michal

Re: [PATCH 3/5] powerpc-pseries: Delete an unnecessary variable initialisation in iommu_pseries_alloc_group()

[Dan Carpenter]

On Thu, Oct 19, 2017 at 01:37:18PM +0200, Michal Suchánek wrote:
> Hello,
> 
> On Wed, 18 Oct 2017 21:24:25 +0200
> SF Markus Elfring <elfring@users.sourceforge.net> wrote:
> 
> > From: Markus Elfring <elfring@users.sourceforge.net>
> > Date: Wed, 18 Oct 2017 19:14:39 +0200
> > 
> > The variable "table_group" will be set to an appropriate pointer.
> > Thus omit the explicit initialisation at the beginning.
> > 
> > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> > ---
> >  arch/powerpc/platforms/pseries/iommu.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/arch/powerpc/platforms/pseries/iommu.c
> > b/arch/powerpc/platforms/pseries/iommu.c index
> > b37d4fb20d1c..b6c12b8e3ace 100644 ---
> > a/arch/powerpc/platforms/pseries/iommu.c +++
> > b/arch/powerpc/platforms/pseries/iommu.c @@ -55,7 +55,7 @@
> >  
> >  static struct iommu_table_group *iommu_pseries_alloc_group(int node)
> >  {
> > -	struct iommu_table_group *table_group = NULL;
> > +	struct iommu_table_group *table_group;
> >  	struct iommu_table *tbl = NULL;
> >  	struct iommu_table_group_link *tgl = NULL;
> >  
> 
> I think initializing pointers to NULL is generally a good idea.
> 
> If there is no use of the variable before it is reinitialized by
> allocation gcc is free to optimize out the variable and its initial
> value.
> 
> On the other hand, if the code is changed later and use of the variable
> becomes possible you may crash (and get a gcc warning, too).

No, it's the opposite. GCC doesn't warn about potential NULL
dereferences, it warns about uninitialized variables.  By initializing
it to a bogus value, you're deliberately disabling static analysis.
We do see bugs where, if only people didn't initialize stuff to bogus
values, then the bug would have been caught before it was merged.

You might imagine that static analysis tools would catch NULL
dereferences but it's actually really really hard.  We used to have
an __uninitialized_var() macro which was used to silence GCC false
positives, but now we initialize the pointers to NULL instead.  So
most of the code that you're dealing with is stuff that was marked as
too hard for GCC to understand.  It's tricky.

regards,
dan carpenter

Re: [PATCH 3/5] powerpc-pseries: Delete an unnecessary variable initialisation in iommu_pseries_alloc_group()

[Michal Suchánek]

On Thu, 19 Oct 2017 15:55:59 +0300
Dan Carpenter <dan.carpenter@oracle.com> wrote:

> On Thu, Oct 19, 2017 at 01:37:18PM +0200, Michal Suchánek wrote:
> > Hello,
> > 
> > On Wed, 18 Oct 2017 21:24:25 +0200
> > SF Markus Elfring <elfring@users.sourceforge.net> wrote:
> >   
> > > From: Markus Elfring <elfring@users.sourceforge.net>
> > > Date: Wed, 18 Oct 2017 19:14:39 +0200
> > > 
> > > The variable "table_group" will be set to an appropriate pointer.
> > > Thus omit the explicit initialisation at the beginning.
> > > 
> > > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> > > ---
> > >  arch/powerpc/platforms/pseries/iommu.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/arch/powerpc/platforms/pseries/iommu.c
> > > b/arch/powerpc/platforms/pseries/iommu.c index
> > > b37d4fb20d1c..b6c12b8e3ace 100644 ---
> > > a/arch/powerpc/platforms/pseries/iommu.c +++
> > > b/arch/powerpc/platforms/pseries/iommu.c @@ -55,7 +55,7 @@
> > >  
> > >  static struct iommu_table_group *iommu_pseries_alloc_group(int
> > > node) {
> > > -	struct iommu_table_group *table_group = NULL;
> > > +	struct iommu_table_group *table_group;
> > >  	struct iommu_table *tbl = NULL;
> > >  	struct iommu_table_group_link *tgl = NULL;
> > >    
> > 
> > I think initializing pointers to NULL is generally a good idea.
> > 
> > If there is no use of the variable before it is reinitialized by
> > allocation gcc is free to optimize out the variable and its initial
> > value.
> > 
> > On the other hand, if the code is changed later and use of the
> > variable becomes possible you may crash (and get a gcc warning,
> > too).  
> 
> No, it's the opposite. GCC doesn't warn about potential NULL
> dereferences, 

However, kernel produces runtime errors on actual NULL dereference. So
you will learn about the issue quite fast.

> it warns about uninitialized variables.  By initializing
> it to a bogus value, you're deliberately disabling static analysis.
> We do see bugs where, if only people didn't initialize stuff to bogus
> values, then the bug would have been caught before it was merged.

There are recently merged changes that cause new warnings as well.

How that can be? Perhaps #ifdefs depending on kernel configuration and
0-day not testing all possible combinations? Perhaps the compiler
getting confused by the code and only later compiler update finding the
issue?

Whatever the reason you cannot rely on compiler warnings to correct
your code. They may help you to point out issues but writing the code
in such a way that the issues are less likely to happen in the first
place is better than fixing warnings after the fact.

> 
> You might imagine that static analysis tools would catch NULL
> dereferences but it's actually really really hard.  We used to have
> an __uninitialized_var() macro which was used to silence GCC false
> positives, but now we initialize the pointers to NULL instead.  So
> most of the code that you're dealing with is stuff that was marked as
> too hard for GCC to understand.  It's tricky.
> 

Then it should be made easy to understand and maintain for humans since
the compilers have failed at maintaining it for us.

Thanks

Michal

Re: [PATCH 3/5] powerpc-pseries: Delete an unnecessary variable initialisation in iommu_pseries_alloc_group()

[David Gibson]

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

On Thu, Oct 19, 2017 at 03:55:59PM +0300, Dan Carpenter wrote:
> On Thu, Oct 19, 2017 at 01:37:18PM +0200, Michal Suchánek wrote:
> > Hello,
> > 
> > On Wed, 18 Oct 2017 21:24:25 +0200
> > SF Markus Elfring <elfring@users.sourceforge.net> wrote:
> > 
> > > From: Markus Elfring <elfring@users.sourceforge.net>
> > > Date: Wed, 18 Oct 2017 19:14:39 +0200
> > > 
> > > The variable "table_group" will be set to an appropriate pointer.
> > > Thus omit the explicit initialisation at the beginning.
> > > 
> > > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> > > ---
> > >  arch/powerpc/platforms/pseries/iommu.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/arch/powerpc/platforms/pseries/iommu.c
> > > b/arch/powerpc/platforms/pseries/iommu.c index
> > > b37d4fb20d1c..b6c12b8e3ace 100644 ---
> > > a/arch/powerpc/platforms/pseries/iommu.c +++
> > > b/arch/powerpc/platforms/pseries/iommu.c @@ -55,7 +55,7 @@
> > >  
> > >  static struct iommu_table_group *iommu_pseries_alloc_group(int node)
> > >  {
> > > -	struct iommu_table_group *table_group = NULL;
> > > +	struct iommu_table_group *table_group;
> > >  	struct iommu_table *tbl = NULL;
> > >  	struct iommu_table_group_link *tgl = NULL;
> > >  
> > 
> > I think initializing pointers to NULL is generally a good idea.
> > 
> > If there is no use of the variable before it is reinitialized by
> > allocation gcc is free to optimize out the variable and its initial
> > value.
> > 
> > On the other hand, if the code is changed later and use of the variable
> > becomes possible you may crash (and get a gcc warning, too).
> 
> No, it's the opposite. GCC doesn't warn about potential NULL
> dereferences, it warns about uninitialized variables.  By initializing
> it to a bogus value, you're deliberately disabling static analysis.
> We do see bugs where, if only people didn't initialize stuff to bogus
> values, then the bug would have been caught before it was merged.

Seconded, I've seen this a number of times.  I think this alone is a
reason not to initiaize locals if they don't require it.
 
> You might imagine that static analysis tools would catch NULL
> dereferences but it's actually really really hard.  We used to have
> an __uninitialized_var() macro which was used to silence GCC false
> positives, but now we initialize the pointers to NULL instead.  So
> most of the code that you're dealing with is stuff that was marked as
> too hard for GCC to understand.  It's tricky.
> 
> regards,
> dan carpenter
> 
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [4/5] powerpc-pseries: Return directly after a failed kzalloc_node() in iommu_pseries_alloc_group()

[Michael Ellerman]

On Wed, 2017-10-18 at 19:26:10 UTC, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Wed, 18 Oct 2017 20:15:32 +0200
> 
> Return directly after a call of the function "kzalloc_node" failed
> at the beginning.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/4dd9eab39c71628d113168a01473ee

cheers