x86/PCI: MMCONFIG: cleanup and add address warning to pci_mmconfig_insert

PATCH In review ethan zhao 4 participants 7 messages Jul 26, 2013 – Jul 27, 2013

Subsystems (2)

x86 PCI

lore

Share & tools

lore

ethan zhao

author

2013-07-26 09:10 UTC #0 lore raw

PATCH 1/1 x86/PCI: MMCONFIG: cleanup and add address warning to pci_mmconfig_insert

Cleanup the -EINVAL return value handling and add warning message for invalid start,end,addr parameters.

Signed-off-by ethan.zhao <[email protected]>

---
 arch/x86/pci/mmconfig-shared.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

arch/x86/pci/mmconfig-shared.c +6 -6

index 082e881..37f6c7f 100644
@@ -700,8 +700,13 @@ int pci_mmconfig_insert(struct device *dev, u16 seg, u8 start, u8 end,
     if (!(pci_probe & PCI_PROBE_MMCONF) || pci_mmcfg_arch_init_failed)
         return -ENODEV;
 
-    if (start > end)
+    if (start > end || !addr) {
+        dev_warn(dev, FW_WARN
+                                 "Invalid address to add MMCONFIG"
+                                 "start %02x end %02x addr %pR\n",
+                                  start, end, addr);
         return -EINVAL;
+    }
 
     mutex_lock(&pci_mmcfg_lock);
     cfg = pci_mmconfig_lookup(seg, start);

@@ -716,11 +721,6 @@ int pci_mmconfig_insert(struct device *dev, u16 seg, u8 start, u8 end,
         return -EEXIST;
     }
 
-    if (!addr) {
-        mutex_unlock(&pci_mmcfg_lock);
-        return -EINVAL;
-    }
-
     rc = -EBUSY;
     cfg = pci_mmconfig_alloc(seg, start, end, addr);
     if (cfg == NULL) {

Signature · 1 line

1.7.1

Quoted 1 line

From 4f7337340c8b3b03fcedfa02ca7b0c3ba4379711 Mon Sep 17 00:00:00 2001

From: ethan.zhao <[email protected]>
Date: Fri, 26 Jul 2013 16:25:07 -0400
Subject: [PATCH] x86/PCI: MMCONFIG: cleanup and add address warning to pci_mmconfig_insert

Cleanup the -EINVAL return value handling and add warning message for invalid start,end,addr parameters.

Signed-off-by ethan.zhao <[email protected]>

---
 arch/x86/pci/mmconfig-shared.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

arch/x86/pci/mmconfig-shared.c +6 -6

index 082e881..37f6c7f 100644
@@ -700,8 +700,13 @@ int pci_mmconfig_insert(struct device *dev, u16 seg, u8 start, u8 end,
 	if (!(pci_probe & PCI_PROBE_MMCONF) || pci_mmcfg_arch_init_failed)
 		return -ENODEV;
 
-	if (start > end)
+	if (start > end || !addr) {
+		dev_warn(dev, FW_WARN
+                                 "Invalid address to add MMCONFIG"
+                                 "start %02x end %02x addr %pR\n",
+                                  start, end, addr);
 		return -EINVAL;
+	}
 
 	mutex_lock(&pci_mmcfg_lock);
 	cfg = pci_mmconfig_lookup(seg, start);

@@ -716,11 +721,6 @@ int pci_mmconfig_insert(struct device *dev, u16 seg, u8 start, u8 end,
 		return -EEXIST;
 	}
 
-	if (!addr) {
-		mutex_unlock(&pci_mmcfg_lock);
-		return -EINVAL;
-	}
-
 	rc = -EBUSY;
 	cfg = pci_mmconfig_alloc(seg, start, end, addr);
 	if (cfg == NULL) {

Signature · 1 line

1.7.1

Bjorn Helgaas

maintainer Acked-by Yinghai Lu on v1 1/1

2013-07-26 17:40 UTC #2 lore raw

In reply to ethan zhao

[+cc Jiang, linux-pci, -cc [email protected] (dead address)]

Quoted 5 lines · ethan zhao

Cleanup the -EINVAL return value handling and add warning message for invalid
start,end,addr parameters.
Signed-off-by ethan.zhao <[email protected]>

This patch was corrupted, so I couldn't apply it directly. See Documentation/SubmittingPatches, sections 5-7.

Quoted 11 lines

---
 arch/x86/pci/mmconfig-shared.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

arch/x86/pci/mmconfig-shared.c +0 -0

index 082e881..37f6c7f 100644
@@ -700,8 +700,13 @@ int pci_mmconfig_insert(struct device *dev, u16
 seg, u8 start, u8 end,
     if (!(pci_probe & PCI_PROBE_MMCONF) || pci_mmcfg_arch_init_failed)

arch/x86/pci/mmconfig-shared.c int pci_mmconfig_insert(struct device *dev, u16 · line 710

Quoted 10 lines

arch/x86/pci/mmconfig-shared.c +6 -1

         return -ENODEV;
 
-    if (start > end)
+    if (start > end || !addr) {
+        dev_warn(dev, FW_WARN
+                                 "Invalid address to add MMCONFIG"
+                                 "start %02x end %02x addr %pR\n",
+                                  start, end, addr);
         return -EINVAL;
+    }

I like the "!addr" cleanup.

Did you actually see this problem on a machine?

I expect this would be a BIOS bug, and one that should be found early in development, long before a machine is released. Therefore, I doubt that it's worth adding another printk for it. If an end-user sees this problem, I think we'll already get a generic message from check_segment().

I propose the patch below; what do you think?

Quoted 15 lines

arch/x86/pci/mmconfig-shared.c +0 -5

     mutex_lock(&pci_mmcfg_lock);
     cfg = pci_mmconfig_lookup(seg, start);

@@ -716,11 +721,6 @@ int pci_mmconfig_insert(struct device *dev, u16
 seg, u8 start, u8 end,
         return -EEXIST;
     }
 
-    if (!addr) {
-        mutex_unlock(&pci_mmcfg_lock);
-        return -EINVAL;
-    }
-
     rc = -EBUSY;
     cfg = pci_mmconfig_alloc(seg, start, end, addr);

Quoted 1 line

     if (cfg == NULL) {

Author: ethan.zhao <[email protected]>
Date:   Fri Jul 26 11:21:24 2013 -0600

x86/PCI: MMCONFIG: Check earlier for MMCONFIG region at address zero

We can check for addr being zero earlier and thus avoid the mutex_unlock() cleanup path.

[bhelgaas: changelog, drop printk]

Signed-off-by ethan.zhao <[email protected]>

Signed-off-by Bjorn Helgaas <[email protected]>

Acked-by Yinghai Lu <[email protected]>

arch/x86/pci/mmconfig-shared.c +1 -6

index 082e881..5596c7b 100644
@@ -700,7 +700,7 @@ int pci_mmconfig_insert(struct device *dev, u16 seg, u8 start, u8 end,
 	if (!(pci_probe & PCI_PROBE_MMCONF) || pci_mmcfg_arch_init_failed)
 		return -ENODEV;
 
-	if (start > end)
+	if (start > end || !addr)
 		return -EINVAL;
 
 	mutex_lock(&pci_mmcfg_lock);

@@ -716,11 +716,6 @@ int pci_mmconfig_insert(struct device *dev, u16 seg, u8 start, u8 end,
 		return -EEXIST;
 	}
 
-	if (!addr) {
-		mutex_unlock(&pci_mmcfg_lock);
-		return -EINVAL;
-	}
-
 	rc = -EBUSY;
 	cfg = pci_mmconfig_alloc(seg, start, end, addr);
 	if (cfg == NULL) {

Yinghai Lu

Acked-by

2013-07-27 14:27 UTC #3 lore raw

Quoted 70 lines · Bjorn Helgaas

[+cc Jiang, linux-pci, -cc [email protected] (dead address)]
Quoted 5 lines · ethan zhao
Cleanup the -EINVAL return value handling and add warning message for invalid
start,end,addr parameters.
Signed-off-by ethan.zhao <[email protected]>
This patch was corrupted, so I couldn't apply it directly. See Documentation/SubmittingPatches, sections 5-7.
Quoted 11 lines
---
 arch/x86/pci/mmconfig-shared.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)
arch/x86/pci/mmconfig-shared.c +0 -0
index 082e881..37f6c7f 100644
@@ -700,8 +700,13 @@ int pci_mmconfig_insert(struct device *dev, u16
 seg, u8 start, u8 end,
     if (!(pci_probe & PCI_PROBE_MMCONF) || pci_mmcfg_arch_init_failed)
arch/x86/pci/mmconfig-shared.c int pci_mmconfig_insert(struct device *dev, u16 · line 710
Quoted 10 lines
arch/x86/pci/mmconfig-shared.c +6 -1
         return -ENODEV;
 
-    if (start > end)
+    if (start > end || !addr) {
+        dev_warn(dev, FW_WARN
+                                 "Invalid address to add MMCONFIG"
+                                 "start %02x end %02x addr %pR\n",
+                                  start, end, addr);
         return -EINVAL;
+    }
I like the "!addr" cleanup.
Did you actually see this problem on a machine?
I expect this would be a BIOS bug, and one that should be found early in development, long before a machine is released. Therefore, I doubt that it's worth adding another printk for it. If an end-user sees this problem, I think we'll already get a generic message from check_segment().
I propose the patch below; what do you think?
Quoted 15 lines
arch/x86/pci/mmconfig-shared.c +0 -5
     mutex_lock(&pci_mmcfg_lock);
     cfg = pci_mmconfig_lookup(seg, start);
@@ -716,11 +721,6 @@ int pci_mmconfig_insert(struct device *dev, u16
 seg, u8 start, u8 end,
         return -EEXIST;
     }
 
-    if (!addr) {
-        mutex_unlock(&pci_mmcfg_lock);
-        return -EINVAL;
-    }
-
     rc = -EBUSY;
     cfg = pci_mmconfig_alloc(seg, start, end, addr);
Quoted 1 line
     if (cfg == NULL) {
Author: ethan.zhao <[email protected]>
Date:   Fri Jul 26 11:21:24 2013 -0600
x86/PCI: MMCONFIG: Check earlier for MMCONFIG region at address zero

Quoted 7 lines

We can check for addr being zero earlier and thus avoid the mutex_unlock() cleanup path.
[bhelgaas: changelog, drop printk]
Signed-off-by ethan.zhao <[email protected]>
Signed-off-by Bjorn Helgaas <[email protected]>
Acked-by Yinghai Lu <[email protected]>

looks like I had typo in ack. please update to

Acked-by Yinghai Lu <[email protected]>

Quoted 26 lines

arch/x86/pci/mmconfig-shared.c +1 -6

index 082e881..5596c7b 100644
@@ -700,7 +700,7 @@ int pci_mmconfig_insert(struct device *dev, u16 seg, u8 start, u8 end,
        if (!(pci_probe & PCI_PROBE_MMCONF) || pci_mmcfg_arch_init_failed)
                return -ENODEV;
 
-       if (start > end)
+       if (start > end || !addr)
                return -EINVAL;
 
        mutex_lock(&pci_mmcfg_lock);

@@ -716,11 +716,6 @@ int pci_mmconfig_insert(struct device *dev, u16 seg, u8 start, u8 end,
                return -EEXIST;
        }
 
-       if (!addr) {
-               mutex_unlock(&pci_mmcfg_lock);
-               return -EINVAL;
-       }
-
        rc = -EBUSY;
        cfg = pci_mmconfig_alloc(seg, start, end, addr);
        if (cfg == NULL) {

Bjorn Helgaas

maintainer

2013-07-27 16:10 UTC #4 lore raw

Quoted 10 lines · Yinghai Lu

Quoted 4 lines · Bjorn Helgaas
[bhelgaas: changelog, drop printk]
Signed-off-by ethan.zhao <[email protected]>
Signed-off-by Bjorn Helgaas <[email protected]>
Acked-by Yinghai Lu <[email protected]>
looks like I had typo in ack. please update to
Acked-by Yinghai Lu <[email protected]>

Done, thanks, sorry I didn't notice that :)

Ethan Zhao

2013-07-27 23:50 UTC #5 lore raw

Seems there are code style issues etc after I pasted it in my mail client. I will correct it and resend v2.

Thanks.

Quoted 13 lines · Bjorn Helgaas

Quoted 10 lines · Yinghai Lu
Quoted 4 lines · Bjorn Helgaas
[bhelgaas: changelog, drop printk]
Signed-off-by ethan.zhao <[email protected]>
Signed-off-by Bjorn Helgaas <[email protected]>
Acked-by Yinghai Lu <[email protected]>
looks like I had typo in ack. please update to
Acked-by Yinghai Lu <[email protected]>
Done, thanks, sorry I didn't notice that :)

Ethan Zhao

2013-07-27 23:47 UTC #6 lore raw

In reply to Bjorn Helgaas

Quoted 36 lines · Bjorn Helgaas

[+cc Jiang, linux-pci, -cc [email protected] (dead address)]
Quoted 5 lines · ethan zhao
Cleanup the -EINVAL return value handling and add warning message for invalid
start,end,addr parameters.
Signed-off-by ethan.zhao <[email protected]>
This patch was corrupted, so I couldn't apply it directly. See Documentation/SubmittingPatches, sections 5-7.
Quoted 11 lines
---
 arch/x86/pci/mmconfig-shared.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)
arch/x86/pci/mmconfig-shared.c +0 -0
index 082e881..37f6c7f 100644
@@ -700,8 +700,13 @@ int pci_mmconfig_insert(struct device *dev, u16
 seg, u8 start, u8 end,
     if (!(pci_probe & PCI_PROBE_MMCONF) || pci_mmcfg_arch_init_failed)
arch/x86/pci/mmconfig-shared.c int pci_mmconfig_insert(struct device *dev, u16 · line 710
Quoted 10 lines
arch/x86/pci/mmconfig-shared.c +6 -1
         return -ENODEV;
 
-    if (start > end)
+    if (start > end || !addr) {
+        dev_warn(dev, FW_WARN
+                                 "Invalid address to add MMCONFIG"
+                                 "start %02x end %02x addr %pR\n",
+                                  start, end, addr);
         return -EINVAL;
+    }
I like the "!addr" cleanup.

Quoted 8 lines

Did you actually see this problem on a machine?
I expect this would be a BIOS bug, and one that should be found early in development, long before a machine is released. Therefore, I doubt that it's worth adding another printk for it. If an end-user sees this problem, I think we'll already get a generic message from check_segment().

Yes, Maybe I met a BIOS bug, but the mmconfig just yelled a failed message like this, no other information

"fail to add MMCONFIG information, xx--xx can't access extended PCI configuration space under this bridge"

While I look into the code, found there are detail messages for other return values in pci_mmconfig_insert(),
But no for -EINVAL, that is not fair and force me to add more code to debug the cause.

Quoted 59 lines

I propose the patch below; what do you think?

Quoted 15 lines

arch/x86/pci/mmconfig-shared.c +0 -5

     mutex_lock(&pci_mmcfg_lock);
     cfg = pci_mmconfig_lookup(seg, start);

@@ -716,11 +721,6 @@ int pci_mmconfig_insert(struct device *dev, u16
 seg, u8 start, u8 end,
         return -EEXIST;
     }
 
-    if (!addr) {
-        mutex_unlock(&pci_mmcfg_lock);
-        return -EINVAL;
-    }
-
     rc = -EBUSY;
     cfg = pci_mmconfig_alloc(seg, start, end, addr);

Quoted 1 line

     if (cfg == NULL) {

Author: ethan.zhao <[email protected]>
Date:   Fri Jul 26 11:21:24 2013 -0600

x86/PCI: MMCONFIG: Check earlier for MMCONFIG region at address zero

We can check for addr being zero earlier and thus avoid the mutex_unlock() cleanup path.

[bhelgaas: changelog, drop printk]

Signed-off-by ethan.zhao <[email protected]>

Signed-off-by Bjorn Helgaas <[email protected]>

Acked-by Yinghai Lu <[email protected]>

arch/x86/pci/mmconfig-shared.c +1 -6

index 082e881..5596c7b 100644
@@ -700,7 +700,7 @@ int pci_mmconfig_insert(struct device *dev, u16 seg, u8 start, u8 end,
        if (!(pci_probe & PCI_PROBE_MMCONF) || pci_mmcfg_arch_init_failed)
                return -ENODEV;
 
-       if (start > end)
+       if (start > end || !addr)
                return -EINVAL;
 
        mutex_lock(&pci_mmcfg_lock);

@@ -716,11 +716,6 @@ int pci_mmconfig_insert(struct device *dev, u16 seg, u8 start, u8 end,
                return -EEXIST;
        }
 
-       if (!addr) {
-               mutex_unlock(&pci_mmcfg_lock);
-               return -EINVAL;
-       }
-
        rc = -EBUSY;
        cfg = pci_mmconfig_alloc(seg, start, end, addr);
        if (cfg == NULL) {