Skip to contentSkip to conversation

ia64: sn/pci/pcibr/pcibr_reg.c: check null pointer dereference

PATCH Superseded Cong Ding 2 participants 5 messages Jan 14, 2013

lore
Share & tools
lore

Cong Ding

author
#0 lore raw

PATCH 1/1 ia64: sn/pci/pcibr/pcibr_reg.c: check null pointer dereference

we should ensure the pointer is not null before the first use, rather than after it.

Signed-off-by Cong Ding <[email protected]>
---
 arch/ia64/sn/pci/pcibr/pcibr_reg.c |   29 +++++++++++------------------
 1 file changed, 11 insertions(+), 18 deletions(-)
arch/ia64/sn/pci/pcibr/pcibr_reg.c +9 -15
index 8b8bbd5..b33ee60 100644
@@ -25,9 +25,8 @@ union br_ptr {
  */
 void pcireg_control_bit_clr(struct pcibus_info *pcibus_info, u64 bits)
 {
-	union br_ptr __iomem *ptr = (union br_ptr __iomem *)pcibus_info->pbi_buscommon.bs_base;
-
 	if (pcibus_info) {
+		union br_ptr __iomem *ptr = (union br_ptr __iomem *)pcibus_info->pbi_buscommon.bs_base;
 		switch (pcibus_info->pbi_bridge_type) {
 		case PCIBR_BRIDGETYPE_TIOCP:
 			__sn_clrq_relaxed(&ptr->tio.cp_control, bits);

@@ -45,9 +44,8 @@ void pcireg_control_bit_clr(struct pcibus_info *pcibus_info, u64 bits)
 
 void pcireg_control_bit_set(struct pcibus_info *pcibus_info, u64 bits)
 {
-	union br_ptr __iomem *ptr = (union br_ptr __iomem *)pcibus_info->pbi_buscommon.bs_base;
-
 	if (pcibus_info) {
+		union br_ptr __iomem *ptr = (union br_ptr __iomem *)pcibus_info->pbi_buscommon.bs_base;
 		switch (pcibus_info->pbi_bridge_type) {
 		case PCIBR_BRIDGETYPE_TIOCP:
 			__sn_setq_relaxed(&ptr->tio.cp_control, bits);

@@ -68,10 +66,10 @@ void pcireg_control_bit_set(struct pcibus_info *pcibus_info, u64 bits)
  */
 u64 pcireg_tflush_get(struct pcibus_info *pcibus_info)
 {
-	union br_ptr __iomem *ptr = (union br_ptr __iomem *)pcibus_info->pbi_buscommon.bs_base;
 	u64 ret = 0;
 
 	if (pcibus_info) {
+		union br_ptr __iomem *ptr = (union br_ptr __iomem *)pcibus_info->pbi_buscommon.bs_base;
 		switch (pcibus_info->pbi_bridge_type) {
 		case PCIBR_BRIDGETYPE_TIOCP:
 			ret = __sn_readq_relaxed(&ptr->tio.cp_tflush);

@@ -98,10 +96,10 @@ u64 pcireg_tflush_get(struct pcibus_info *pcibus_info)
  */
 u64 pcireg_intr_status_get(struct pcibus_info * pcibus_info)
 {
-	union br_ptr __iomem *ptr = (union br_ptr __iomem *)pcibus_info->pbi_buscommon.bs_base;
 	u64 ret = 0;
 
 	if (pcibus_info) {
+		union br_ptr __iomem *ptr = (union br_ptr __iomem *)pcibus_info->pbi_buscommon.bs_base;
 		switch (pcibus_info->pbi_bridge_type) {
 		case PCIBR_BRIDGETYPE_TIOCP:
 			ret = __sn_readq_relaxed(&ptr->tio.cp_int_status);

@@ -123,9 +121,8 @@ u64 pcireg_intr_status_get(struct pcibus_info * pcibus_info)
  */
 void pcireg_intr_enable_bit_clr(struct pcibus_info *pcibus_info, u64 bits)
 {
-	union br_ptr __iomem *ptr = (union br_ptr __iomem *)pcibus_info->pbi_buscommon.bs_base;
-
 	if (pcibus_info) {
+		union br_ptr __iomem *ptr = (union br_ptr __iomem *)pcibus_info->pbi_buscommon.bs_base;
 		switch (pcibus_info->pbi_bridge_type) {
 		case PCIBR_BRIDGETYPE_TIOCP:
 			__sn_clrq_relaxed(&ptr->tio.cp_int_enable, bits);

@@ -143,9 +140,8 @@ void pcireg_intr_enable_bit_clr(struct pcibus_info *pcibus_info, u64 bits)
 
 void pcireg_intr_enable_bit_set(struct pcibus_info *pcibus_info, u64 bits)
 {
-	union br_ptr __iomem *ptr = (union br_ptr __iomem *)pcibus_info->pbi_buscommon.bs_base;
-
 	if (pcibus_info) {
+		union br_ptr __iomem *ptr = (union br_ptr __iomem *)pcibus_info->pbi_buscommon.bs_base;
 		switch (pcibus_info->pbi_bridge_type) {
 		case PCIBR_BRIDGETYPE_TIOCP:
 			__sn_setq_relaxed(&ptr->tio.cp_int_enable, bits);

@@ -167,9 +163,8 @@ void pcireg_intr_enable_bit_set(struct pcibus_info *pcibus_info, u64 bits)
 void pcireg_intr_addr_addr_set(struct pcibus_info *pcibus_info, int int_n,
 			       u64 addr)
 {
-	union br_ptr __iomem *ptr = (union br_ptr __iomem *)pcibus_info->pbi_buscommon.bs_base;
-
 	if (pcibus_info) {
+		union br_ptr __iomem *ptr = (union br_ptr __iomem *)pcibus_info->pbi_buscommon.bs_base;
 		switch (pcibus_info->pbi_bridge_type) {
 		case PCIBR_BRIDGETYPE_TIOCP:
 			__sn_clrq_relaxed(&ptr->tio.cp_int_addr[int_n],

@@ -196,9 +191,8 @@ void pcireg_intr_addr_addr_set(struct pcibus_info *pcibus_info, int int_n,
  */
 void pcireg_force_intr_set(struct pcibus_info *pcibus_info, int int_n)
 {
-	union br_ptr __iomem *ptr = (union br_ptr __iomem *)pcibus_info->pbi_buscommon.bs_base;
-
 	if (pcibus_info) {
+		union br_ptr __iomem *ptr = (union br_ptr __iomem *)pcibus_info->pbi_buscommon.bs_base;
 		switch (pcibus_info->pbi_bridge_type) {
 		case PCIBR_BRIDGETYPE_TIOCP:
 			writeq(1, &ptr->tio.cp_force_pin[int_n]);

@@ -219,10 +213,10 @@ void pcireg_force_intr_set(struct pcibus_info *pcibus_info, int int_n)
  */
 u64 pcireg_wrb_flush_get(struct pcibus_info *pcibus_info, int device)
 {
-	union br_ptr __iomem *ptr = (union br_ptr __iomem *)pcibus_info->pbi_buscommon.bs_base;
 	u64 ret = 0;
 
 	if (pcibus_info) {
+		union br_ptr __iomem *ptr = (union br_ptr __iomem *)pcibus_info->pbi_buscommon.bs_base;
 		switch (pcibus_info->pbi_bridge_type) {
 		case PCIBR_BRIDGETYPE_TIOCP:
 			ret @@ -244,9 +238,8 @@ u64 pcireg_wrb_flush_get(struct pcibus_info *pcibus_info, int device)

void pcireg_int_ate_set(struct pcibus_info *pcibus_info, int ate_index,
u64 val)
{
- union br_ptr __iomem *ptr = (union br_ptr __iomem *)pcibus_info->pbi_buscommon.bs_base;
-
if (pcibus_info) {
+ union br_ptr __iomem *ptr = (union br_ptr __iomem *)pcibus_info->pbi_buscommon.bs_base;
switch (pcibus_info->pbi_bridge_type) {
case PCIBR_BRIDGETYPE_TIOCP:
writeq(val, &ptr->tio.cp_int_ate_ram[ate_index]);
@@ -264,10 +257,10 @@ void pcireg_int_ate_set(struct pcibus_info *pcibus_info, int ate_index,

u64 __iomem *pcireg_int_ate_addr(struct pcibus_info *pcibus_info, int ate_index) {
- union br_ptr __iomem *ptr = (union br_ptr __iomem *)pcibus_info->pbi_buscommon.bs_base;
u64 __iomem *ret = NULL;

if (pcibus_info) {
+ union br_ptr __iomem *ptr = (union br_ptr __iomem *)pcibus_info->pbi_buscommon.bs_base;
switch (pcibus_info->pbi_bridge_type) { case PCIBR_BRIDGETYPE_TIOCP:
ret = &ptr->tio.cp_int_ate_ram[ate_index];

Signature · 1 line
1.7.9.5
#1 lore raw
Quoted 2 lines · Cong Ding

we should ensure the pointer is not null before the first use, rather than after it.

These changes look technically correct, but the whole pcibr_reg.c file is ridiculously defensive programming.

For example, the first two hunks are for pcireg_control_bit_clr() and pcireg_control_bit_set(). These functions are called only from pcibr_bus_fixup(), and it's impossible for it to pass a null pointer. It would be better to just remove the null pointer checks completely.

The panics in pcibr_reg.c are dubious, too. That sort of check belongs higher up, e.g., in pcibr_bus_fixup() where we set up pcibus_info->pbi_buscommon.bs_base in the first place.

Bjorn

Quoted 141 lines
Signed-off-by Cong Ding <[email protected]>
---
 arch/ia64/sn/pci/pcibr/pcibr_reg.c |   29 +++++++++++------------------
 1 file changed, 11 insertions(+), 18 deletions(-)
arch/ia64/sn/pci/pcibr/pcibr_reg.c +9 -15
index 8b8bbd5..b33ee60 100644
@@ -25,9 +25,8 @@ union br_ptr {
  */
 void pcireg_control_bit_clr(struct pcibus_info *pcibus_info, u64 bits)
 {
-       union br_ptr __iomem *ptr = (union br_ptr __iomem *)pcibus_info->pbi_buscommon.bs_base;
-
        if (pcibus_info) {
+               union br_ptr __iomem *ptr = (union br_ptr __iomem *)pcibus_info->pbi_buscommon.bs_base;
                switch (pcibus_info->pbi_bridge_type) {
                case PCIBR_BRIDGETYPE_TIOCP:
                        __sn_clrq_relaxed(&ptr->tio.cp_control, bits);

@@ -45,9 +44,8 @@ void pcireg_control_bit_clr(struct pcibus_info *pcibus_info, u64 bits)
 
 void pcireg_control_bit_set(struct pcibus_info *pcibus_info, u64 bits)
 {
-       union br_ptr __iomem *ptr = (union br_ptr __iomem *)pcibus_info->pbi_buscommon.bs_base;
-
        if (pcibus_info) {
+               union br_ptr __iomem *ptr = (union br_ptr __iomem *)pcibus_info->pbi_buscommon.bs_base;
                switch (pcibus_info->pbi_bridge_type) {
                case PCIBR_BRIDGETYPE_TIOCP:
                        __sn_setq_relaxed(&ptr->tio.cp_control, bits);

@@ -68,10 +66,10 @@ void pcireg_control_bit_set(struct pcibus_info *pcibus_info, u64 bits)
  */
 u64 pcireg_tflush_get(struct pcibus_info *pcibus_info)
 {
-       union br_ptr __iomem *ptr = (union br_ptr __iomem *)pcibus_info->pbi_buscommon.bs_base;
        u64 ret = 0;
 
        if (pcibus_info) {
+               union br_ptr __iomem *ptr = (union br_ptr __iomem *)pcibus_info->pbi_buscommon.bs_base;
                switch (pcibus_info->pbi_bridge_type) {
                case PCIBR_BRIDGETYPE_TIOCP:
                        ret = __sn_readq_relaxed(&ptr->tio.cp_tflush);

@@ -98,10 +96,10 @@ u64 pcireg_tflush_get(struct pcibus_info *pcibus_info)
  */
 u64 pcireg_intr_status_get(struct pcibus_info * pcibus_info)
 {
-       union br_ptr __iomem *ptr = (union br_ptr __iomem *)pcibus_info->pbi_buscommon.bs_base;
        u64 ret = 0;
 
        if (pcibus_info) {
+               union br_ptr __iomem *ptr = (union br_ptr __iomem *)pcibus_info->pbi_buscommon.bs_base;
                switch (pcibus_info->pbi_bridge_type) {
                case PCIBR_BRIDGETYPE_TIOCP:
                        ret = __sn_readq_relaxed(&ptr->tio.cp_int_status);

@@ -123,9 +121,8 @@ u64 pcireg_intr_status_get(struct pcibus_info * pcibus_info)
  */
 void pcireg_intr_enable_bit_clr(struct pcibus_info *pcibus_info, u64 bits)
 {
-       union br_ptr __iomem *ptr = (union br_ptr __iomem *)pcibus_info->pbi_buscommon.bs_base;
-
        if (pcibus_info) {
+               union br_ptr __iomem *ptr = (union br_ptr __iomem *)pcibus_info->pbi_buscommon.bs_base;
                switch (pcibus_info->pbi_bridge_type) {
                case PCIBR_BRIDGETYPE_TIOCP:
                        __sn_clrq_relaxed(&ptr->tio.cp_int_enable, bits);

@@ -143,9 +140,8 @@ void pcireg_intr_enable_bit_clr(struct pcibus_info *pcibus_info, u64 bits)
 
 void pcireg_intr_enable_bit_set(struct pcibus_info *pcibus_info, u64 bits)
 {
-       union br_ptr __iomem *ptr = (union br_ptr __iomem *)pcibus_info->pbi_buscommon.bs_base;
-
        if (pcibus_info) {
+               union br_ptr __iomem *ptr = (union br_ptr __iomem *)pcibus_info->pbi_buscommon.bs_base;
                switch (pcibus_info->pbi_bridge_type) {
                case PCIBR_BRIDGETYPE_TIOCP:
                        __sn_setq_relaxed(&ptr->tio.cp_int_enable, bits);

@@ -167,9 +163,8 @@ void pcireg_intr_enable_bit_set(struct pcibus_info *pcibus_info, u64 bits)
 void pcireg_intr_addr_addr_set(struct pcibus_info *pcibus_info, int int_n,
                               u64 addr)
 {
-       union br_ptr __iomem *ptr = (union br_ptr __iomem *)pcibus_info->pbi_buscommon.bs_base;
-
        if (pcibus_info) {
+               union br_ptr __iomem *ptr = (union br_ptr __iomem *)pcibus_info->pbi_buscommon.bs_base;
                switch (pcibus_info->pbi_bridge_type) {
                case PCIBR_BRIDGETYPE_TIOCP:
                        __sn_clrq_relaxed(&ptr->tio.cp_int_addr[int_n],

@@ -196,9 +191,8 @@ void pcireg_intr_addr_addr_set(struct pcibus_info *pcibus_info, int int_n,
  */
 void pcireg_force_intr_set(struct pcibus_info *pcibus_info, int int_n)
 {
-       union br_ptr __iomem *ptr = (union br_ptr __iomem *)pcibus_info->pbi_buscommon.bs_base;
-
        if (pcibus_info) {
+               union br_ptr __iomem *ptr = (union br_ptr __iomem *)pcibus_info->pbi_buscommon.bs_base;
                switch (pcibus_info->pbi_bridge_type) {
                case PCIBR_BRIDGETYPE_TIOCP:
                        writeq(1, &ptr->tio.cp_force_pin[int_n]);

@@ -219,10 +213,10 @@ void pcireg_force_intr_set(struct pcibus_info *pcibus_info, int int_n)
  */
 u64 pcireg_wrb_flush_get(struct pcibus_info *pcibus_info, int device)
 {
-       union br_ptr __iomem *ptr = (union br_ptr __iomem *)pcibus_info->pbi_buscommon.bs_base;
        u64 ret = 0;
 
        if (pcibus_info) {
+               union br_ptr __iomem *ptr = (union br_ptr __iomem *)pcibus_info->pbi_buscommon.bs_base;
                switch (pcibus_info->pbi_bridge_type) {
                case PCIBR_BRIDGETYPE_TIOCP:
                        ret > @@ -244,9 +238,8 @@ u64 pcireg_wrb_flush_get(struct pcibus_info *pcibus_info, int device)

void pcireg_int_ate_set(struct pcibus_info *pcibus_info, int ate_index,

                        u64 val)

{

arch/ia64/sn/pci/pcibr/pcibr_reg.c +2 -4
-       union br_ptr __iomem *ptr = (union br_ptr __iomem *)pcibus_info->pbi_buscommon.bs_base;
-
        if (pcibus_info) {
+               union br_ptr __iomem *ptr = (union br_ptr __iomem *)pcibus_info->pbi_buscommon.bs_base;
                switch (pcibus_info->pbi_bridge_type) {
                case PCIBR_BRIDGETYPE_TIOCP:
                        writeq(val, &ptr->tio.cp_int_ate_ram[ate_index]);

@@ -264,10 +257,10 @@ void pcireg_int_ate_set(struct pcibus_info *pcibus_info, int ate_index,
 
 u64 __iomem *pcireg_int_ate_addr(struct pcibus_info *pcibus_info, int ate_index)
 {
-       union br_ptr __iomem *ptr = (union br_ptr __iomem *)pcibus_info->pbi_buscommon.bs_base;
        u64 __iomem *ret = NULL;
 
        if (pcibus_info) {
+               union br_ptr __iomem *ptr = (union br_ptr __iomem *)pcibus_info->pbi_buscommon.bs_base;
                switch (pcibus_info->pbi_bridge_type) {
                case PCIBR_BRIDGETYPE_TIOCP:
                        ret = &ptr->tio.cp_int_ate_ram[ate_index];
--

1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

author
#2 lore raw
Quoted 7 lines · Bjorn Helgaas
Quoted 2 lines · Cong Ding

we should ensure the pointer is not null before the first use, rather than after it.

These changes look technically correct, but the whole pcibr_reg.c file is ridiculously defensive programming.

Quoted 9 lines

For example, the first two hunks are for pcireg_control_bit_clr() and pcireg_control_bit_set(). These functions are called only from pcibr_bus_fixup(), and it's impossible for it to pass a null pointer. It would be better to just remove the null pointer checks completely.

The panics in pcibr_reg.c are dubious, too. That sort of check belongs higher up, e.g., in pcibr_bus_fixup() where we set up pcibus_info->pbi_buscommon.bs_base in the first place.

Thanks Bjorn, so do you think a patch like the following would be better?

- cong

From 5c4798565320b2eeda23b68fdf950322f4ea66ff Mon Sep 17 00:00:00 2001
From: Cong Ding <[email protected]>
Date: Mon, 14 Jan 2013 18:41:24 +0000
Subject: [PATCH] ia64: sn/pci/pcibr/pcibr_reg.c: remove unnecessary null pointer check

These functions are called only from pcibr_bus_fixup(), and it's impossible for it to pass a null pointer. And these pointers are used before the checks.

Signed-off-by Cong Ding <[email protected]>
---
 arch/ia64/sn/pci/pcibr/pcibr_reg.c |  271 ++++++++++++++++--------------------
 1 files changed, 120 insertions(+), 151 deletions(-)
arch/ia64/sn/pci/pcibr/pcibr_reg.c +118 -149
index 8b8bbd5..ae16be3c 100644
@@ -27,19 +27,16 @@ void pcireg_control_bit_clr(struct pcibus_info *pcibus_info, u64 bits)
 {
 	union br_ptr __iomem *ptr = (union br_ptr __iomem *)pcibus_info->pbi_buscommon.bs_base;
 
-	if (pcibus_info) {
-		switch (pcibus_info->pbi_bridge_type) {
-		case PCIBR_BRIDGETYPE_TIOCP:
-			__sn_clrq_relaxed(&ptr->tio.cp_control, bits);
-			break;
-		case PCIBR_BRIDGETYPE_PIC:
-			__sn_clrq_relaxed(&ptr->pic.p_wid_control, bits);
-			break;
-		default:
-			panic
-			    ("pcireg_control_bit_clr: unknown bridgetype bridge 0x%p",
-			     ptr);
-		}
+	switch (pcibus_info->pbi_bridge_type) {
+	case PCIBR_BRIDGETYPE_TIOCP:
+		__sn_clrq_relaxed(&ptr->tio.cp_control, bits);
+		break;
+	case PCIBR_BRIDGETYPE_PIC:
+		__sn_clrq_relaxed(&ptr->pic.p_wid_control, bits);
+		break;
+	default:
+		panic("pcireg_control_bit_clr: unknown bridgetype bridge 0x%p",
+				ptr);
 	}
 }
 

@@ -47,19 +44,16 @@ void pcireg_control_bit_set(struct pcibus_info *pcibus_info, u64 bits)
 {
 	union br_ptr __iomem *ptr = (union br_ptr __iomem *)pcibus_info->pbi_buscommon.bs_base;
 
-	if (pcibus_info) {
-		switch (pcibus_info->pbi_bridge_type) {
-		case PCIBR_BRIDGETYPE_TIOCP:
-			__sn_setq_relaxed(&ptr->tio.cp_control, bits);
-			break;
-		case PCIBR_BRIDGETYPE_PIC:
-			__sn_setq_relaxed(&ptr->pic.p_wid_control, bits);
-			break;
-		default:
-			panic
-			    ("pcireg_control_bit_set: unknown bridgetype bridge 0x%p",
-			     ptr);
-		}
+	switch (pcibus_info->pbi_bridge_type) {
+	case PCIBR_BRIDGETYPE_TIOCP:
+		__sn_setq_relaxed(&ptr->tio.cp_control, bits);
+		break;
+	case PCIBR_BRIDGETYPE_PIC:
+		__sn_setq_relaxed(&ptr->pic.p_wid_control, bits);
+		break;
+	default:
+		panic("pcireg_control_bit_set: unknown bridgetype bridge 0x%p",
+				ptr);
 	}
 }
 

@@ -71,19 +65,16 @@ u64 pcireg_tflush_get(struct pcibus_info *pcibus_info)
 	union br_ptr __iomem *ptr = (union br_ptr __iomem *)pcibus_info->pbi_buscommon.bs_base;
 	u64 ret = 0;
 
-	if (pcibus_info) {
-		switch (pcibus_info->pbi_bridge_type) {
-		case PCIBR_BRIDGETYPE_TIOCP:
-			ret = __sn_readq_relaxed(&ptr->tio.cp_tflush);
-			break;
-		case PCIBR_BRIDGETYPE_PIC:
-			ret = __sn_readq_relaxed(&ptr->pic.p_wid_tflush);
-			break;
-		default:
-			panic
-			    ("pcireg_tflush_get: unknown bridgetype bridge 0x%p",
-			     ptr);
-		}
+	switch (pcibus_info->pbi_bridge_type) {
+	case PCIBR_BRIDGETYPE_TIOCP:
+		ret = __sn_readq_relaxed(&ptr->tio.cp_tflush);
+		break;
+	case PCIBR_BRIDGETYPE_PIC:
+		ret = __sn_readq_relaxed(&ptr->pic.p_wid_tflush);
+		break;
+	default:
+		panic("pcireg_tflush_get: unknown bridgetype bridge 0x%p",
+				ptr);
 	}
 
 	/* Read of the Target Flush should always return zero */

@@ -96,24 +87,21 @@ u64 pcireg_tflush_get(struct pcibus_info *pcibus_info)
 /*
  * Interrupt Status Register Access -- Read Only		    0000_0100
  */
-u64 pcireg_intr_status_get(struct pcibus_info * pcibus_info)
+u64 pcireg_intr_status_get(struct pcibus_info *pcibus_info)
 {
 	union br_ptr __iomem *ptr = (union br_ptr __iomem *)pcibus_info->pbi_buscommon.bs_base;
 	u64 ret = 0;
 
-	if (pcibus_info) {
-		switch (pcibus_info->pbi_bridge_type) {
-		case PCIBR_BRIDGETYPE_TIOCP:
-			ret = __sn_readq_relaxed(&ptr->tio.cp_int_status);
-			break;
-		case PCIBR_BRIDGETYPE_PIC:
-			ret = __sn_readq_relaxed(&ptr->pic.p_int_status);
-			break;
-		default:
-			panic
-			    ("pcireg_intr_status_get: unknown bridgetype bridge 0x%p",
-			     ptr);
-		}
+	switch (pcibus_info->pbi_bridge_type) {
+	case PCIBR_BRIDGETYPE_TIOCP:
+		ret = __sn_readq_relaxed(&ptr->tio.cp_int_status);
+		break;
+	case PCIBR_BRIDGETYPE_PIC:
+		ret = __sn_readq_relaxed(&ptr->pic.p_int_status);
+		break;
+	default:
+		panic("pcireg_intr_status_get: unknown bridgetype bridge 0x%p",
+				ptr);
 	}
 	return ret;
 }

@@ -125,19 +113,16 @@ void pcireg_intr_enable_bit_clr(struct pcibus_info *pcibus_info, u64 bits)
 {
 	union br_ptr __iomem *ptr = (union br_ptr __iomem *)pcibus_info->pbi_buscommon.bs_base;
 
-	if (pcibus_info) {
-		switch (pcibus_info->pbi_bridge_type) {
-		case PCIBR_BRIDGETYPE_TIOCP:
-			__sn_clrq_relaxed(&ptr->tio.cp_int_enable, bits);
-			break;
-		case PCIBR_BRIDGETYPE_PIC:
-			__sn_clrq_relaxed(&ptr->pic.p_int_enable, bits);
-			break;
-		default:
-			panic
-			    ("pcireg_intr_enable_bit_clr: unknown bridgetype bridge 0x%p",
-			     ptr);
-		}
+	switch (pcibus_info->pbi_bridge_type) {
+	case PCIBR_BRIDGETYPE_TIOCP:
+		__sn_clrq_relaxed(&ptr->tio.cp_int_enable, bits);
+		break;
+	case PCIBR_BRIDGETYPE_PIC:
+		__sn_clrq_relaxed(&ptr->pic.p_int_enable, bits);
+		break;
+	default:
+		panic("pcireg_intr_enable_bit_clr: unknown bridgetype bridge 0x%p",
+				ptr);
 	}
 }
 

@@ -145,19 +130,16 @@ void pcireg_intr_enable_bit_set(struct pcibus_info *pcibus_info, u64 bits)
 {
 	union br_ptr __iomem *ptr = (union br_ptr __iomem *)pcibus_info->pbi_buscommon.bs_base;
 
-	if (pcibus_info) {
-		switch (pcibus_info->pbi_bridge_type) {
-		case PCIBR_BRIDGETYPE_TIOCP:
-			__sn_setq_relaxed(&ptr->tio.cp_int_enable, bits);
-			break;
-		case PCIBR_BRIDGETYPE_PIC:
-			__sn_setq_relaxed(&ptr->pic.p_int_enable, bits);
-			break;
-		default:
-			panic
-			    ("pcireg_intr_enable_bit_set: unknown bridgetype bridge 0x%p",
-			     ptr);
-		}
+	switch (pcibus_info->pbi_bridge_type) {
+	case PCIBR_BRIDGETYPE_TIOCP:
+		__sn_setq_relaxed(&ptr->tio.cp_int_enable, bits);
+		break;
+	case PCIBR_BRIDGETYPE_PIC:
+		__sn_setq_relaxed(&ptr->pic.p_int_enable, bits);
+		break;
+	default:
+		panic("pcireg_intr_enable_bit_set: unknown bridgetype bridge 0x%p",
+				ptr);
 	}
 }
 

@@ -169,25 +151,22 @@ void pcireg_intr_addr_addr_set(struct pcibus_info *pcibus_info, int int_n,
 {
 	union br_ptr __iomem *ptr = (union br_ptr __iomem *)pcibus_info->pbi_buscommon.bs_base;
 
-	if (pcibus_info) {
-		switch (pcibus_info->pbi_bridge_type) {
-		case PCIBR_BRIDGETYPE_TIOCP:
-			__sn_clrq_relaxed(&ptr->tio.cp_int_addr[int_n],
-			    TIOCP_HOST_INTR_ADDR);
-			__sn_setq_relaxed(&ptr->tio.cp_int_addr[int_n],
-			    (addr & TIOCP_HOST_INTR_ADDR));
-			break;
-		case PCIBR_BRIDGETYPE_PIC:
-			__sn_clrq_relaxed(&ptr->pic.p_int_addr[int_n],
-			    PIC_HOST_INTR_ADDR);
-			__sn_setq_relaxed(&ptr->pic.p_int_addr[int_n],
-			    (addr & PIC_HOST_INTR_ADDR));
-			break;
-		default:
-			panic
-			    ("pcireg_intr_addr_addr_get: unknown bridgetype bridge 0x%p",
-			     ptr);
-		}
+	switch (pcibus_info->pbi_bridge_type) {
+	case PCIBR_BRIDGETYPE_TIOCP:
+		__sn_clrq_relaxed(&ptr->tio.cp_int_addr[int_n],
+		    TIOCP_HOST_INTR_ADDR);
+		__sn_setq_relaxed(&ptr->tio.cp_int_addr[int_n],
+		    (addr & TIOCP_HOST_INTR_ADDR));
+		break;
+	case PCIBR_BRIDGETYPE_PIC:
+		__sn_clrq_relaxed(&ptr->pic.p_int_addr[int_n],
+		    PIC_HOST_INTR_ADDR);
+		__sn_setq_relaxed(&ptr->pic.p_int_addr[int_n],
+		    (addr & PIC_HOST_INTR_ADDR));
+		break;
+	default:
+		panic("pcireg_intr_addr_addr_get: unknown bridgetype bridge 0x%p",
+				ptr);
 	}
 }
 

@@ -198,19 +177,16 @@ void pcireg_force_intr_set(struct pcibus_info *pcibus_info, int int_n)
 {
 	union br_ptr __iomem *ptr = (union br_ptr __iomem *)pcibus_info->pbi_buscommon.bs_base;
 
-	if (pcibus_info) {
-		switch (pcibus_info->pbi_bridge_type) {
-		case PCIBR_BRIDGETYPE_TIOCP:
-			writeq(1, &ptr->tio.cp_force_pin[int_n]);
-			break;
-		case PCIBR_BRIDGETYPE_PIC:
-			writeq(1, &ptr->pic.p_force_pin[int_n]);
-			break;
-		default:
-			panic
-			    ("pcireg_force_intr_set: unknown bridgetype bridge 0x%p",
-			     ptr);
-		}
+	switch (pcibus_info->pbi_bridge_type) {
+	case PCIBR_BRIDGETYPE_TIOCP:
+		writeq(1, &ptr->tio.cp_force_pin[int_n]);
+		break;
+	case PCIBR_BRIDGETYPE_PIC:
+		writeq(1, &ptr->pic.p_force_pin[int_n]);
+		break;
+	default:
+		panic("pcireg_force_intr_set: unknown bridgetype bridge 0x%p",
+				ptr);
 	}
 }
 

@@ -222,21 +198,20 @@ u64 pcireg_wrb_flush_get(struct pcibus_info *pcibus_info, int device)
 	union br_ptr __iomem *ptr = (union br_ptr __iomem *)pcibus_info->pbi_buscommon.bs_base;
 	u64 ret = 0;
 
-	if (pcibus_info) {
-		switch (pcibus_info->pbi_bridge_type) {
-		case PCIBR_BRIDGETYPE_TIOCP:
-			ret -			    __sn_readq_relaxed(&ptr->tio.cp_wr_req_buf[device]);
-			break;
-		case PCIBR_BRIDGETYPE_PIC:
-			ret -			    __sn_readq_relaxed(&ptr->pic.p_wr_req_buf[device]);
-			break;
-		default:
-		      panic("pcireg_wrb_flush_get: unknown bridgetype bridge 0x%p", ptr);
-		}
-
+	switch (pcibus_info->pbi_bridge_type) {
+	case PCIBR_BRIDGETYPE_TIOCP:
+		ret +		    __sn_readq_relaxed(&ptr->tio.cp_wr_req_buf[device]);
+		break;
+	case PCIBR_BRIDGETYPE_PIC:
+		ret +		    __sn_readq_relaxed(&ptr->pic.p_wr_req_buf[device]);
+		break;
+	default:
+	      panic("pcireg_wrb_flush_get: unknown bridgetype bridge 0x%p",
+			      ptr);
 	}
+
 	/* Read of the Write Buffer Flush should always return zero */
 	return ret;
 }

@@ -246,19 +221,16 @@ void pcireg_int_ate_set(struct pcibus_info *pcibus_info, int ate_index,
 {
 	union br_ptr __iomem *ptr = (union br_ptr __iomem *)pcibus_info->pbi_buscommon.bs_base;
 
-	if (pcibus_info) {
-		switch (pcibus_info->pbi_bridge_type) {
-		case PCIBR_BRIDGETYPE_TIOCP:
-			writeq(val, &ptr->tio.cp_int_ate_ram[ate_index]);
-			break;
-		case PCIBR_BRIDGETYPE_PIC:
-			writeq(val, &ptr->pic.p_int_ate_ram[ate_index]);
-			break;
-		default:
-			panic
-			    ("pcireg_int_ate_set: unknown bridgetype bridge 0x%p",
-			     ptr);
-		}
+	switch (pcibus_info->pbi_bridge_type) {
+	case PCIBR_BRIDGETYPE_TIOCP:
+		writeq(val, &ptr->tio.cp_int_ate_ram[ate_index]);
+		break;
+	case PCIBR_BRIDGETYPE_PIC:
+		writeq(val, &ptr->pic.p_int_ate_ram[ate_index]);
+		break;
+	default:
+		panic("pcireg_int_ate_set: unknown bridgetype bridge 0x%p",
+				ptr);
 	}
 }
 

@@ -267,19 +239,16 @@ u64 __iomem *pcireg_int_ate_addr(struct pcibus_info *pcibus_info, int ate_index)
 	union br_ptr __iomem *ptr = (union br_ptr __iomem *)pcibus_info->pbi_buscommon.bs_base;
 	u64 __iomem *ret = NULL;
 
-	if (pcibus_info) {
-		switch (pcibus_info->pbi_bridge_type) {
-		case PCIBR_BRIDGETYPE_TIOCP:
-			ret = &ptr->tio.cp_int_ate_ram[ate_index];
-			break;
-		case PCIBR_BRIDGETYPE_PIC:
-			ret = &ptr->pic.p_int_ate_ram[ate_index];
-			break;
-		default:
-			panic
-			    ("pcireg_int_ate_addr: unknown bridgetype bridge 0x%p",
-			     ptr);
-		}
+	switch (pcibus_info->pbi_bridge_type) {
+	case PCIBR_BRIDGETYPE_TIOCP:
+		ret = &ptr->tio.cp_int_ate_ram[ate_index];
+		break;
+	case PCIBR_BRIDGETYPE_PIC:
+		ret = &ptr->pic.p_int_ate_ram[ate_index];
+		break;
+	default:
+		panic("pcireg_int_ate_addr: unknown bridgetype bridge 0x%p",
+				ptr);
 	}
 	return ret;
 }
Signature · 1 line
1.7.4.5
#3 lore raw

[-cc [email protected] (stale)]

Quoted 13 lines · Cong Ding
Quoted 12 lines · Bjorn Helgaas
Quoted 2 lines · Cong Ding

we should ensure the pointer is not null before the first use, rather than after it.

These changes look technically correct, but the whole pcibr_reg.c file is ridiculously defensive programming.

For example, the first two hunks are for pcireg_control_bit_clr() and pcireg_control_bit_set(). These functions are called only from pcibr_bus_fixup(), and it's impossible for it to pass a null pointer. It would be better to just remove the null pointer checks completely.

Quoted 5 lines
Quoted 4 lines

The panics in pcibr_reg.c are dubious, too. That sort of check belongs higher up, e.g., in pcibr_bus_fixup() where we set up pcibus_info->pbi_buscommon.bs_base in the first place.

Thanks Bjorn, so do you think a patch like the following would be better?

Well, you have to do the rest of the analysis. I didn't look at *all* the functions in pcibr_reg.c, and I know some of them are called from places other than pcibr_bus_fixup(). I suspect the same idea applies to all, but you need to verify that and update the commit log accordingly.

Quoted 369 lines

From 5c4798565320b2eeda23b68fdf950322f4ea66ff Mon Sep 17 00:00:00 2001
From: Cong Ding <[email protected]>
Date: Mon, 14 Jan 2013 18:41:24 +0000
Subject: [PATCH] ia64: sn/pci/pcibr/pcibr_reg.c: remove unnecessary null pointer check

These functions are called only from pcibr_bus_fixup(), and it's impossible for it to pass a null pointer. And these pointers are used before the checks.

Signed-off-by Cong Ding <[email protected]>
---
 arch/ia64/sn/pci/pcibr/pcibr_reg.c |  271 ++++++++++++++++--------------------
 1 files changed, 120 insertions(+), 151 deletions(-)
arch/ia64/sn/pci/pcibr/pcibr_reg.c +118 -150
index 8b8bbd5..ae16be3c 100644
@@ -27,19 +27,16 @@ void pcireg_control_bit_clr(struct pcibus_info *pcibus_info, u64 bits)
 {
        union br_ptr __iomem *ptr = (union br_ptr __iomem *)pcibus_info->pbi_buscommon.bs_base;
 
-       if (pcibus_info) {
-               switch (pcibus_info->pbi_bridge_type) {
-               case PCIBR_BRIDGETYPE_TIOCP:
-                       __sn_clrq_relaxed(&ptr->tio.cp_control, bits);
-                       break;
-               case PCIBR_BRIDGETYPE_PIC:
-                       __sn_clrq_relaxed(&ptr->pic.p_wid_control, bits);
-                       break;
-               default:
-                       panic
-                           ("pcireg_control_bit_clr: unknown bridgetype bridge 0x%p",
-                            ptr);
-               }
+       switch (pcibus_info->pbi_bridge_type) {
+       case PCIBR_BRIDGETYPE_TIOCP:
+               __sn_clrq_relaxed(&ptr->tio.cp_control, bits);
+               break;
+       case PCIBR_BRIDGETYPE_PIC:
+               __sn_clrq_relaxed(&ptr->pic.p_wid_control, bits);
+               break;
+       default:
+               panic("pcireg_control_bit_clr: unknown bridgetype bridge 0x%p",
+                               ptr);
        }
 }
 

@@ -47,19 +44,16 @@ void pcireg_control_bit_set(struct pcibus_info *pcibus_info, u64 bits)
 {
        union br_ptr __iomem *ptr = (union br_ptr __iomem *)pcibus_info->pbi_buscommon.bs_base;
 
-       if (pcibus_info) {
-               switch (pcibus_info->pbi_bridge_type) {
-               case PCIBR_BRIDGETYPE_TIOCP:
-                       __sn_setq_relaxed(&ptr->tio.cp_control, bits);
-                       break;
-               case PCIBR_BRIDGETYPE_PIC:
-                       __sn_setq_relaxed(&ptr->pic.p_wid_control, bits);
-                       break;
-               default:
-                       panic
-                           ("pcireg_control_bit_set: unknown bridgetype bridge 0x%p",
-                            ptr);
-               }
+       switch (pcibus_info->pbi_bridge_type) {
+       case PCIBR_BRIDGETYPE_TIOCP:
+               __sn_setq_relaxed(&ptr->tio.cp_control, bits);
+               break;
+       case PCIBR_BRIDGETYPE_PIC:
+               __sn_setq_relaxed(&ptr->pic.p_wid_control, bits);
+               break;
+       default:
+               panic("pcireg_control_bit_set: unknown bridgetype bridge 0x%p",
+                               ptr);
        }
 }
 

@@ -71,19 +65,16 @@ u64 pcireg_tflush_get(struct pcibus_info *pcibus_info)
        union br_ptr __iomem *ptr = (union br_ptr __iomem *)pcibus_info->pbi_buscommon.bs_base;
        u64 ret = 0;
 
-       if (pcibus_info) {
-               switch (pcibus_info->pbi_bridge_type) {
-               case PCIBR_BRIDGETYPE_TIOCP:
-                       ret = __sn_readq_relaxed(&ptr->tio.cp_tflush);
-                       break;
-               case PCIBR_BRIDGETYPE_PIC:
-                       ret = __sn_readq_relaxed(&ptr->pic.p_wid_tflush);
-                       break;
-               default:
-                       panic
-                           ("pcireg_tflush_get: unknown bridgetype bridge 0x%p",
-                            ptr);
-               }
+       switch (pcibus_info->pbi_bridge_type) {
+       case PCIBR_BRIDGETYPE_TIOCP:
+               ret = __sn_readq_relaxed(&ptr->tio.cp_tflush);
+               break;
+       case PCIBR_BRIDGETYPE_PIC:
+               ret = __sn_readq_relaxed(&ptr->pic.p_wid_tflush);
+               break;
+       default:
+               panic("pcireg_tflush_get: unknown bridgetype bridge 0x%p",
+                               ptr);
        }
 
        /* Read of the Target Flush should always return zero */

@@ -96,24 +87,21 @@ u64 pcireg_tflush_get(struct pcibus_info *pcibus_info)
 /*
  * Interrupt Status Register Access -- Read Only                   0000_0100
  */
-u64 pcireg_intr_status_get(struct pcibus_info * pcibus_info)
+u64 pcireg_intr_status_get(struct pcibus_info *pcibus_info)
 {
        union br_ptr __iomem *ptr = (union br_ptr __iomem *)pcibus_info->pbi_buscommon.bs_base;
        u64 ret = 0;
 
-       if (pcibus_info) {
-               switch (pcibus_info->pbi_bridge_type) {
-               case PCIBR_BRIDGETYPE_TIOCP:
-                       ret = __sn_readq_relaxed(&ptr->tio.cp_int_status);
-                       break;
-               case PCIBR_BRIDGETYPE_PIC:
-                       ret = __sn_readq_relaxed(&ptr->pic.p_int_status);
-                       break;
-               default:
-                       panic
-                           ("pcireg_intr_status_get: unknown bridgetype bridge 0x%p",
-                            ptr);
-               }
+       switch (pcibus_info->pbi_bridge_type) {
+       case PCIBR_BRIDGETYPE_TIOCP:
+               ret = __sn_readq_relaxed(&ptr->tio.cp_int_status);
+               break;
+       case PCIBR_BRIDGETYPE_PIC:
+               ret = __sn_readq_relaxed(&ptr->pic.p_int_status);
+               break;
+       default:
+               panic("pcireg_intr_status_get: unknown bridgetype bridge 0x%p",
+                               ptr);
        }
        return ret;
 }

@@ -125,19 +113,16 @@ void pcireg_intr_enable_bit_clr(struct pcibus_info *pcibus_info, u64 bits)
 {
        union br_ptr __iomem *ptr = (union br_ptr __iomem *)pcibus_info->pbi_buscommon.bs_base;
 
-       if (pcibus_info) {
-               switch (pcibus_info->pbi_bridge_type) {
-               case PCIBR_BRIDGETYPE_TIOCP:
-                       __sn_clrq_relaxed(&ptr->tio.cp_int_enable, bits);
-                       break;
-               case PCIBR_BRIDGETYPE_PIC:
-                       __sn_clrq_relaxed(&ptr->pic.p_int_enable, bits);
-                       break;
-               default:
-                       panic
-                           ("pcireg_intr_enable_bit_clr: unknown bridgetype bridge 0x%p",
-                            ptr);
-               }
+       switch (pcibus_info->pbi_bridge_type) {
+       case PCIBR_BRIDGETYPE_TIOCP:
+               __sn_clrq_relaxed(&ptr->tio.cp_int_enable, bits);
+               break;
+       case PCIBR_BRIDGETYPE_PIC:
+               __sn_clrq_relaxed(&ptr->pic.p_int_enable, bits);
+               break;
+       default:
+               panic("pcireg_intr_enable_bit_clr: unknown bridgetype bridge 0x%p",
+                               ptr);
        }
 }
 

@@ -145,19 +130,16 @@ void pcireg_intr_enable_bit_set(struct pcibus_info *pcibus_info, u64 bits)
 {
        union br_ptr __iomem *ptr = (union br_ptr __iomem *)pcibus_info->pbi_buscommon.bs_base;
 
-       if (pcibus_info) {
-               switch (pcibus_info->pbi_bridge_type) {
-               case PCIBR_BRIDGETYPE_TIOCP:
-                       __sn_setq_relaxed(&ptr->tio.cp_int_enable, bits);
-                       break;
-               case PCIBR_BRIDGETYPE_PIC:
-                       __sn_setq_relaxed(&ptr->pic.p_int_enable, bits);
-                       break;
-               default:
-                       panic
-                           ("pcireg_intr_enable_bit_set: unknown bridgetype bridge 0x%p",
-                            ptr);
-               }
+       switch (pcibus_info->pbi_bridge_type) {
+       case PCIBR_BRIDGETYPE_TIOCP:
+               __sn_setq_relaxed(&ptr->tio.cp_int_enable, bits);
+               break;
+       case PCIBR_BRIDGETYPE_PIC:
+               __sn_setq_relaxed(&ptr->pic.p_int_enable, bits);
+               break;
+       default:
+               panic("pcireg_intr_enable_bit_set: unknown bridgetype bridge 0x%p",
+                               ptr);
        }
 }
 

@@ -169,25 +151,22 @@ void pcireg_intr_addr_addr_set(struct pcibus_info *pcibus_info, int int_n,
 {
        union br_ptr __iomem *ptr = (union br_ptr __iomem *)pcibus_info->pbi_buscommon.bs_base;
 
-       if (pcibus_info) {
-               switch (pcibus_info->pbi_bridge_type) {
-               case PCIBR_BRIDGETYPE_TIOCP:
-                       __sn_clrq_relaxed(&ptr->tio.cp_int_addr[int_n],
-                           TIOCP_HOST_INTR_ADDR);
-                       __sn_setq_relaxed(&ptr->tio.cp_int_addr[int_n],
-                           (addr & TIOCP_HOST_INTR_ADDR));
-                       break;
-               case PCIBR_BRIDGETYPE_PIC:
-                       __sn_clrq_relaxed(&ptr->pic.p_int_addr[int_n],
-                           PIC_HOST_INTR_ADDR);
-                       __sn_setq_relaxed(&ptr->pic.p_int_addr[int_n],
-                           (addr & PIC_HOST_INTR_ADDR));
-                       break;
-               default:
-                       panic
-                           ("pcireg_intr_addr_addr_get: unknown bridgetype bridge 0x%p",
-                            ptr);
-               }
+       switch (pcibus_info->pbi_bridge_type) {
+       case PCIBR_BRIDGETYPE_TIOCP:
+               __sn_clrq_relaxed(&ptr->tio.cp_int_addr[int_n],
+                   TIOCP_HOST_INTR_ADDR);
+               __sn_setq_relaxed(&ptr->tio.cp_int_addr[int_n],
+                   (addr & TIOCP_HOST_INTR_ADDR));
+               break;
+       case PCIBR_BRIDGETYPE_PIC:
+               __sn_clrq_relaxed(&ptr->pic.p_int_addr[int_n],
+                   PIC_HOST_INTR_ADDR);
+               __sn_setq_relaxed(&ptr->pic.p_int_addr[int_n],
+                   (addr & PIC_HOST_INTR_ADDR));
+               break;
+       default:
+               panic("pcireg_intr_addr_addr_get: unknown bridgetype bridge 0x%p",
+                               ptr);
        }
 }
 

@@ -198,19 +177,16 @@ void pcireg_force_intr_set(struct pcibus_info *pcibus_info, int int_n)
 {
        union br_ptr __iomem *ptr = (union br_ptr __iomem *)pcibus_info->pbi_buscommon.bs_base;
 
-       if (pcibus_info) {
-               switch (pcibus_info->pbi_bridge_type) {
-               case PCIBR_BRIDGETYPE_TIOCP:
-                       writeq(1, &ptr->tio.cp_force_pin[int_n]);
-                       break;
-               case PCIBR_BRIDGETYPE_PIC:
-                       writeq(1, &ptr->pic.p_force_pin[int_n]);
-                       break;
-               default:
-                       panic
-                           ("pcireg_force_intr_set: unknown bridgetype bridge 0x%p",
-                            ptr);
-               }
+       switch (pcibus_info->pbi_bridge_type) {
+       case PCIBR_BRIDGETYPE_TIOCP:
+               writeq(1, &ptr->tio.cp_force_pin[int_n]);
+               break;
+       case PCIBR_BRIDGETYPE_PIC:
+               writeq(1, &ptr->pic.p_force_pin[int_n]);
+               break;
+       default:
+               panic("pcireg_force_intr_set: unknown bridgetype bridge 0x%p",
+                               ptr);
        }
 }
 

@@ -222,21 +198,20 @@ u64 pcireg_wrb_flush_get(struct pcibus_info *pcibus_info, int device)
        union br_ptr __iomem *ptr = (union br_ptr __iomem *)pcibus_info->pbi_buscommon.bs_base;
        u64 ret = 0;
 
-       if (pcibus_info) {
-               switch (pcibus_info->pbi_bridge_type) {
-               case PCIBR_BRIDGETYPE_TIOCP:
-                       ret > -                           __sn_readq_relaxed(&ptr->tio.cp_wr_req_buf[device]);
-                       break;
-               case PCIBR_BRIDGETYPE_PIC:
-                       ret > -                           __sn_readq_relaxed(&ptr->pic.p_wr_req_buf[device]);
-                       break;
-               default:
-                     panic("pcireg_wrb_flush_get: unknown bridgetype bridge 0x%p", ptr);
-               }
-
+       switch (pcibus_info->pbi_bridge_type) {
+       case PCIBR_BRIDGETYPE_TIOCP:
+               ret > +                   __sn_readq_relaxed(&ptr->tio.cp_wr_req_buf[device]);
+               break;
+       case PCIBR_BRIDGETYPE_PIC:
+               ret > +                   __sn_readq_relaxed(&ptr->pic.p_wr_req_buf[device]);
+               break;
+       default:
+             panic("pcireg_wrb_flush_get: unknown bridgetype bridge 0x%p",
+                             ptr);
        }
+
        /* Read of the Write Buffer Flush should always return zero */
        return ret;
 }

@@ -246,19 +221,16 @@ void pcireg_int_ate_set(struct pcibus_info *pcibus_info, int ate_index,
 {
        union br_ptr __iomem *ptr = (union br_ptr __iomem *)pcibus_info->pbi_buscommon.bs_base;
 
-       if (pcibus_info) {
-               switch (pcibus_info->pbi_bridge_type) {
-               case PCIBR_BRIDGETYPE_TIOCP:
-                       writeq(val, &ptr->tio.cp_int_ate_ram[ate_index]);
-                       break;
-               case PCIBR_BRIDGETYPE_PIC:
-                       writeq(val, &ptr->pic.p_int_ate_ram[ate_index]);
-                       break;
-               default:
-                       panic
-                           ("pcireg_int_ate_set: unknown bridgetype bridge 0x%p",
-                            ptr);
-               }
+       switch (pcibus_info->pbi_bridge_type) {
+       case PCIBR_BRIDGETYPE_TIOCP:
+               writeq(val, &ptr->tio.cp_int_ate_ram[ate_index]);
+               break;
+       case PCIBR_BRIDGETYPE_PIC:
+               writeq(val, &ptr->pic.p_int_ate_ram[ate_index]);
+               break;
+       default:
+               panic("pcireg_int_ate_set: unknown bridgetype bridge 0x%p",
+                               ptr);
        }
 }
 

@@ -267,19 +239,16 @@ u64 __iomem *pcireg_int_ate_addr(struct pcibus_info *pcibus_info, int ate_index)
        union br_ptr __iomem *ptr = (union br_ptr __iomem *)pcibus_info->pbi_buscommon.bs_base;
        u64 __iomem *ret = NULL;
 
-       if (pcibus_info) {
-               switch (pcibus_info->pbi_bridge_type) {
-               case PCIBR_BRIDGETYPE_TIOCP:
-                       ret = &ptr->tio.cp_int_ate_ram[ate_index];
-                       break;
-               case PCIBR_BRIDGETYPE_PIC:
-                       ret = &ptr->pic.p_int_ate_ram[ate_index];
-                       break;
-               default:
-                       panic
-                           ("pcireg_int_ate_addr: unknown bridgetype bridge 0x%p",
-                            ptr);
-               }
+       switch (pcibus_info->pbi_bridge_type) {
+       case PCIBR_BRIDGETYPE_TIOCP:
+               ret = &ptr->tio.cp_int_ate_ram[ate_index];
+               break;
+       case PCIBR_BRIDGETYPE_PIC:
+               ret = &ptr->pic.p_int_ate_ram[ate_index];
+               break;
+       default:
+               panic("pcireg_int_ate_addr: unknown bridgetype bridge 0x%p",
+                               ptr);
        }
        return ret;
 }
--

1.7.4.5

author
#4 lore raw

PATCH v2 1/1 ia64: sn/pci/pcibr/pcibr_reg.c: remove unnecessary check null pointer

At first, all the null pointer checks are after the pointer is dereferenced in the same function. The followings are evidences of all the unnecessary null pointer checks.

These functions pcireg_control_bit_clr and pcireg_control_bit_set are called only from pcibr_bus_fixup() in pcibr_provider.c, and it's impossible for it to pass a null pointer due to null pointer check.

The function pcireg_tflush_get is called only from ate_write() (ate_write() is called by pcibr_ate_free()) in pcibr_ate.c, and it's impossible for it to pass a null pointer due to pointer dereference before the calling.

The functions pcireg_intr_status_get, pcireg_intr_enable_bit_clr and pcireg_force_intr_set get the parameter from pcidev_info->pdi_host_pcidev_info->pdi_pcibus_info, which is impossible to be null because the pcidev_info is from sn_irq_info->irq_pciioinfo and the irq_pciioinfo is checked in file irq.c and msi_sn.c.

The functions pcireg_intr_enable_bit_set and pcireg_intr_addr_addr_set are called after pcireg_intr_enable_bit_clr, so there are also impossible to be null.

The function pcireg_wrb_flush_get get the parameter from sn_dma_flush() in file pcibr_dma.c, where the parameter common->sfdl_pcibus_info is passed. The common->sfdl_pcibus_info is guaranteed to be not null by function pcibr_bus_fixup() in file pcibr_provider.c.

The functions pcireg_int_ate_set and pcireg_int_ate_addr are similar as pcireg_int_ate_set.

Signed-off-by Cong Ding <[email protected]>
---
 arch/ia64/sn/pci/pcibr/pcibr_reg.c |  271 ++++++++++++++++--------------------
 1 files changed, 120 insertions(+), 151 deletions(-)
arch/ia64/sn/pci/pcibr/pcibr_reg.c +118 -149
index 8b8bbd5..ae16be3c 100644
@@ -27,19 +27,16 @@ void pcireg_control_bit_clr(struct pcibus_info *pcibus_info, u64 bits)
 {
 	union br_ptr __iomem *ptr = (union br_ptr __iomem *)pcibus_info->pbi_buscommon.bs_base;
 
-	if (pcibus_info) {
-		switch (pcibus_info->pbi_bridge_type) {
-		case PCIBR_BRIDGETYPE_TIOCP:
-			__sn_clrq_relaxed(&ptr->tio.cp_control, bits);
-			break;
-		case PCIBR_BRIDGETYPE_PIC:
-			__sn_clrq_relaxed(&ptr->pic.p_wid_control, bits);
-			break;
-		default:
-			panic
-			    ("pcireg_control_bit_clr: unknown bridgetype bridge 0x%p",
-			     ptr);
-		}
+	switch (pcibus_info->pbi_bridge_type) {
+	case PCIBR_BRIDGETYPE_TIOCP:
+		__sn_clrq_relaxed(&ptr->tio.cp_control, bits);
+		break;
+	case PCIBR_BRIDGETYPE_PIC:
+		__sn_clrq_relaxed(&ptr->pic.p_wid_control, bits);
+		break;
+	default:
+		panic("pcireg_control_bit_clr: unknown bridgetype bridge 0x%p",
+				ptr);
 	}
 }
 

@@ -47,19 +44,16 @@ void pcireg_control_bit_set(struct pcibus_info *pcibus_info, u64 bits)
 {
 	union br_ptr __iomem *ptr = (union br_ptr __iomem *)pcibus_info->pbi_buscommon.bs_base;
 
-	if (pcibus_info) {
-		switch (pcibus_info->pbi_bridge_type) {
-		case PCIBR_BRIDGETYPE_TIOCP:
-			__sn_setq_relaxed(&ptr->tio.cp_control, bits);
-			break;
-		case PCIBR_BRIDGETYPE_PIC:
-			__sn_setq_relaxed(&ptr->pic.p_wid_control, bits);
-			break;
-		default:
-			panic
-			    ("pcireg_control_bit_set: unknown bridgetype bridge 0x%p",
-			     ptr);
-		}
+	switch (pcibus_info->pbi_bridge_type) {
+	case PCIBR_BRIDGETYPE_TIOCP:
+		__sn_setq_relaxed(&ptr->tio.cp_control, bits);
+		break;
+	case PCIBR_BRIDGETYPE_PIC:
+		__sn_setq_relaxed(&ptr->pic.p_wid_control, bits);
+		break;
+	default:
+		panic("pcireg_control_bit_set: unknown bridgetype bridge 0x%p",
+				ptr);
 	}
 }
 

@@ -71,19 +65,16 @@ u64 pcireg_tflush_get(struct pcibus_info *pcibus_info)
 	union br_ptr __iomem *ptr = (union br_ptr __iomem *)pcibus_info->pbi_buscommon.bs_base;
 	u64 ret = 0;
 
-	if (pcibus_info) {
-		switch (pcibus_info->pbi_bridge_type) {
-		case PCIBR_BRIDGETYPE_TIOCP:
-			ret = __sn_readq_relaxed(&ptr->tio.cp_tflush);
-			break;
-		case PCIBR_BRIDGETYPE_PIC:
-			ret = __sn_readq_relaxed(&ptr->pic.p_wid_tflush);
-			break;
-		default:
-			panic
-			    ("pcireg_tflush_get: unknown bridgetype bridge 0x%p",
-			     ptr);
-		}
+	switch (pcibus_info->pbi_bridge_type) {
+	case PCIBR_BRIDGETYPE_TIOCP:
+		ret = __sn_readq_relaxed(&ptr->tio.cp_tflush);
+		break;
+	case PCIBR_BRIDGETYPE_PIC:
+		ret = __sn_readq_relaxed(&ptr->pic.p_wid_tflush);
+		break;
+	default:
+		panic("pcireg_tflush_get: unknown bridgetype bridge 0x%p",
+				ptr);
 	}
 
 	/* Read of the Target Flush should always return zero */

@@ -96,24 +87,21 @@ u64 pcireg_tflush_get(struct pcibus_info *pcibus_info)
 /*
  * Interrupt Status Register Access -- Read Only		    0000_0100
  */
-u64 pcireg_intr_status_get(struct pcibus_info * pcibus_info)
+u64 pcireg_intr_status_get(struct pcibus_info *pcibus_info)
 {
 	union br_ptr __iomem *ptr = (union br_ptr __iomem *)pcibus_info->pbi_buscommon.bs_base;
 	u64 ret = 0;
 
-	if (pcibus_info) {
-		switch (pcibus_info->pbi_bridge_type) {
-		case PCIBR_BRIDGETYPE_TIOCP:
-			ret = __sn_readq_relaxed(&ptr->tio.cp_int_status);
-			break;
-		case PCIBR_BRIDGETYPE_PIC:
-			ret = __sn_readq_relaxed(&ptr->pic.p_int_status);
-			break;
-		default:
-			panic
-			    ("pcireg_intr_status_get: unknown bridgetype bridge 0x%p",
-			     ptr);
-		}
+	switch (pcibus_info->pbi_bridge_type) {
+	case PCIBR_BRIDGETYPE_TIOCP:
+		ret = __sn_readq_relaxed(&ptr->tio.cp_int_status);
+		break;
+	case PCIBR_BRIDGETYPE_PIC:
+		ret = __sn_readq_relaxed(&ptr->pic.p_int_status);
+		break;
+	default:
+		panic("pcireg_intr_status_get: unknown bridgetype bridge 0x%p",
+				ptr);
 	}
 	return ret;
 }

@@ -125,19 +113,16 @@ void pcireg_intr_enable_bit_clr(struct pcibus_info *pcibus_info, u64 bits)
 {
 	union br_ptr __iomem *ptr = (union br_ptr __iomem *)pcibus_info->pbi_buscommon.bs_base;
 
-	if (pcibus_info) {
-		switch (pcibus_info->pbi_bridge_type) {
-		case PCIBR_BRIDGETYPE_TIOCP:
-			__sn_clrq_relaxed(&ptr->tio.cp_int_enable, bits);
-			break;
-		case PCIBR_BRIDGETYPE_PIC:
-			__sn_clrq_relaxed(&ptr->pic.p_int_enable, bits);
-			break;
-		default:
-			panic
-			    ("pcireg_intr_enable_bit_clr: unknown bridgetype bridge 0x%p",
-			     ptr);
-		}
+	switch (pcibus_info->pbi_bridge_type) {
+	case PCIBR_BRIDGETYPE_TIOCP:
+		__sn_clrq_relaxed(&ptr->tio.cp_int_enable, bits);
+		break;
+	case PCIBR_BRIDGETYPE_PIC:
+		__sn_clrq_relaxed(&ptr->pic.p_int_enable, bits);
+		break;
+	default:
+		panic("pcireg_intr_enable_bit_clr: unknown bridgetype bridge 0x%p",
+				ptr);
 	}
 }
 

@@ -145,19 +130,16 @@ void pcireg_intr_enable_bit_set(struct pcibus_info *pcibus_info, u64 bits)
 {
 	union br_ptr __iomem *ptr = (union br_ptr __iomem *)pcibus_info->pbi_buscommon.bs_base;
 
-	if (pcibus_info) {
-		switch (pcibus_info->pbi_bridge_type) {
-		case PCIBR_BRIDGETYPE_TIOCP:
-			__sn_setq_relaxed(&ptr->tio.cp_int_enable, bits);
-			break;
-		case PCIBR_BRIDGETYPE_PIC:
-			__sn_setq_relaxed(&ptr->pic.p_int_enable, bits);
-			break;
-		default:
-			panic
-			    ("pcireg_intr_enable_bit_set: unknown bridgetype bridge 0x%p",
-			     ptr);
-		}
+	switch (pcibus_info->pbi_bridge_type) {
+	case PCIBR_BRIDGETYPE_TIOCP:
+		__sn_setq_relaxed(&ptr->tio.cp_int_enable, bits);
+		break;
+	case PCIBR_BRIDGETYPE_PIC:
+		__sn_setq_relaxed(&ptr->pic.p_int_enable, bits);
+		break;
+	default:
+		panic("pcireg_intr_enable_bit_set: unknown bridgetype bridge 0x%p",
+				ptr);
 	}
 }
 

@@ -169,25 +151,22 @@ void pcireg_intr_addr_addr_set(struct pcibus_info *pcibus_info, int int_n,
 {
 	union br_ptr __iomem *ptr = (union br_ptr __iomem *)pcibus_info->pbi_buscommon.bs_base;
 
-	if (pcibus_info) {
-		switch (pcibus_info->pbi_bridge_type) {
-		case PCIBR_BRIDGETYPE_TIOCP:
-			__sn_clrq_relaxed(&ptr->tio.cp_int_addr[int_n],
-			    TIOCP_HOST_INTR_ADDR);
-			__sn_setq_relaxed(&ptr->tio.cp_int_addr[int_n],
-			    (addr & TIOCP_HOST_INTR_ADDR));
-			break;
-		case PCIBR_BRIDGETYPE_PIC:
-			__sn_clrq_relaxed(&ptr->pic.p_int_addr[int_n],
-			    PIC_HOST_INTR_ADDR);
-			__sn_setq_relaxed(&ptr->pic.p_int_addr[int_n],
-			    (addr & PIC_HOST_INTR_ADDR));
-			break;
-		default:
-			panic
-			    ("pcireg_intr_addr_addr_get: unknown bridgetype bridge 0x%p",
-			     ptr);
-		}
+	switch (pcibus_info->pbi_bridge_type) {
+	case PCIBR_BRIDGETYPE_TIOCP:
+		__sn_clrq_relaxed(&ptr->tio.cp_int_addr[int_n],
+		    TIOCP_HOST_INTR_ADDR);
+		__sn_setq_relaxed(&ptr->tio.cp_int_addr[int_n],
+		    (addr & TIOCP_HOST_INTR_ADDR));
+		break;
+	case PCIBR_BRIDGETYPE_PIC:
+		__sn_clrq_relaxed(&ptr->pic.p_int_addr[int_n],
+		    PIC_HOST_INTR_ADDR);
+		__sn_setq_relaxed(&ptr->pic.p_int_addr[int_n],
+		    (addr & PIC_HOST_INTR_ADDR));
+		break;
+	default:
+		panic("pcireg_intr_addr_addr_get: unknown bridgetype bridge 0x%p",
+				ptr);
 	}
 }
 

@@ -198,19 +177,16 @@ void pcireg_force_intr_set(struct pcibus_info *pcibus_info, int int_n)
 {
 	union br_ptr __iomem *ptr = (union br_ptr __iomem *)pcibus_info->pbi_buscommon.bs_base;
 
-	if (pcibus_info) {
-		switch (pcibus_info->pbi_bridge_type) {
-		case PCIBR_BRIDGETYPE_TIOCP:
-			writeq(1, &ptr->tio.cp_force_pin[int_n]);
-			break;
-		case PCIBR_BRIDGETYPE_PIC:
-			writeq(1, &ptr->pic.p_force_pin[int_n]);
-			break;
-		default:
-			panic
-			    ("pcireg_force_intr_set: unknown bridgetype bridge 0x%p",
-			     ptr);
-		}
+	switch (pcibus_info->pbi_bridge_type) {
+	case PCIBR_BRIDGETYPE_TIOCP:
+		writeq(1, &ptr->tio.cp_force_pin[int_n]);
+		break;
+	case PCIBR_BRIDGETYPE_PIC:
+		writeq(1, &ptr->pic.p_force_pin[int_n]);
+		break;
+	default:
+		panic("pcireg_force_intr_set: unknown bridgetype bridge 0x%p",
+				ptr);
 	}
 }
 

@@ -222,21 +198,20 @@ u64 pcireg_wrb_flush_get(struct pcibus_info *pcibus_info, int device)
 	union br_ptr __iomem *ptr = (union br_ptr __iomem *)pcibus_info->pbi_buscommon.bs_base;
 	u64 ret = 0;
 
-	if (pcibus_info) {
-		switch (pcibus_info->pbi_bridge_type) {
-		case PCIBR_BRIDGETYPE_TIOCP:
-			ret -			    __sn_readq_relaxed(&ptr->tio.cp_wr_req_buf[device]);
-			break;
-		case PCIBR_BRIDGETYPE_PIC:
-			ret -			    __sn_readq_relaxed(&ptr->pic.p_wr_req_buf[device]);
-			break;
-		default:
-		      panic("pcireg_wrb_flush_get: unknown bridgetype bridge 0x%p", ptr);
-		}
-
+	switch (pcibus_info->pbi_bridge_type) {
+	case PCIBR_BRIDGETYPE_TIOCP:
+		ret +		    __sn_readq_relaxed(&ptr->tio.cp_wr_req_buf[device]);
+		break;
+	case PCIBR_BRIDGETYPE_PIC:
+		ret +		    __sn_readq_relaxed(&ptr->pic.p_wr_req_buf[device]);
+		break;
+	default:
+	      panic("pcireg_wrb_flush_get: unknown bridgetype bridge 0x%p",
+			      ptr);
 	}
+
 	/* Read of the Write Buffer Flush should always return zero */
 	return ret;
 }

@@ -246,19 +221,16 @@ void pcireg_int_ate_set(struct pcibus_info *pcibus_info, int ate_index,
 {
 	union br_ptr __iomem *ptr = (union br_ptr __iomem *)pcibus_info->pbi_buscommon.bs_base;
 
-	if (pcibus_info) {
-		switch (pcibus_info->pbi_bridge_type) {
-		case PCIBR_BRIDGETYPE_TIOCP:
-			writeq(val, &ptr->tio.cp_int_ate_ram[ate_index]);
-			break;
-		case PCIBR_BRIDGETYPE_PIC:
-			writeq(val, &ptr->pic.p_int_ate_ram[ate_index]);
-			break;
-		default:
-			panic
-			    ("pcireg_int_ate_set: unknown bridgetype bridge 0x%p",
-			     ptr);
-		}
+	switch (pcibus_info->pbi_bridge_type) {
+	case PCIBR_BRIDGETYPE_TIOCP:
+		writeq(val, &ptr->tio.cp_int_ate_ram[ate_index]);
+		break;
+	case PCIBR_BRIDGETYPE_PIC:
+		writeq(val, &ptr->pic.p_int_ate_ram[ate_index]);
+		break;
+	default:
+		panic("pcireg_int_ate_set: unknown bridgetype bridge 0x%p",
+				ptr);
 	}
 }
 

@@ -267,19 +239,16 @@ u64 __iomem *pcireg_int_ate_addr(struct pcibus_info *pcibus_info, int ate_index)
 	union br_ptr __iomem *ptr = (union br_ptr __iomem *)pcibus_info->pbi_buscommon.bs_base;
 	u64 __iomem *ret = NULL;
 
-	if (pcibus_info) {
-		switch (pcibus_info->pbi_bridge_type) {
-		case PCIBR_BRIDGETYPE_TIOCP:
-			ret = &ptr->tio.cp_int_ate_ram[ate_index];
-			break;
-		case PCIBR_BRIDGETYPE_PIC:
-			ret = &ptr->pic.p_int_ate_ram[ate_index];
-			break;
-		default:
-			panic
-			    ("pcireg_int_ate_addr: unknown bridgetype bridge 0x%p",
-			     ptr);
-		}
+	switch (pcibus_info->pbi_bridge_type) {
+	case PCIBR_BRIDGETYPE_TIOCP:
+		ret = &ptr->tio.cp_int_ate_ram[ate_index];
+		break;
+	case PCIBR_BRIDGETYPE_PIC:
+		ret = &ptr->pic.p_int_ate_ram[ate_index];
+		break;
+	default:
+		panic("pcireg_int_ate_addr: unknown bridgetype bridge 0x%p",
+				ptr);
 	}
 	return ret;
 }
Signature · 1 line
1.7.4.5