Re: [PATCH] x86/PCI: MMCONFIG: cleanup and add address warning to pci_mmconfig_insert
Seems there are code style issues etc after I pasted it in my mail client. I will correct it and resend v2. Thanks. On Sun, Jul 28, 2013 at 12:09 AM, Bjorn Helgaas wrote: > On Sat, Jul 27, 2013 at 8:27 AM, Yinghai Lu wrote: >> On Fri, Jul 26, 2013 at 10:39 AM, Bjorn Helgaas wrote: > >>> [bhelgaas: changelog, drop printk] >>> Signed-off-by: ethan.zhao >>> Signed-off-by: Bjorn Helgaas >>> Acked-by: Yinghai Lu >> >> looks like I had typo in ack. please update to >> >> Acked-by: Yinghai Lu > > Done, thanks, sorry I didn't notice that :) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] x86/PCI: MMCONFIG: cleanup and add address warning to pci_mmconfig_insert
On Sat, Jul 27, 2013 at 1:39 AM, Bjorn Helgaas wrote: > [+cc Jiang, linux-pci, -cc bjorn.helg...@hp.com (dead address)] > > On Fri, Jul 26, 2013 at 05:10:39PM +0800, ethan zhao wrote: >> Cleanup the -EINVAL return value handling and add warning message >> for invalid >> start,end,addr parameters. >> >> Signed-off-by: ethan.zhao > > This patch was corrupted, so I couldn't apply it directly. See > Documentation/SubmittingPatches, sections 5-7. > >> --- >> arch/x86/pci/mmconfig-shared.c | 12 ++-- >> 1 files changed, 6 insertions(+), 6 deletions(-) >> >> diff --git a/arch/x86/pci/mmconfig-shared.c b/arch/x86/pci/mmconfig-shared.c >> index 082e881..37f6c7f 100644 >> --- a/arch/x86/pci/mmconfig-shared.c >> +++ b/arch/x86/pci/mmconfig-shared.c >> @@ -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; >> +} > > 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(). > 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. > I propose the patch below; what do you think? > >> >> mutex_lock(_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(_mmcfg_lock); >> -return -EINVAL; >> -} >> - >> rc = -EBUSY; >> cfg = pci_mmconfig_alloc(seg, start, end, addr); >> if (cfg == NULL) { > > > > Author: ethan.zhao > 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 > Signed-off-by: Bjorn Helgaas > Acked-by: Yinghai Lu > > diff --git a/arch/x86/pci/mmconfig-shared.c b/arch/x86/pci/mmconfig-shared.c > index 082e881..5596c7b 100644 > --- a/arch/x86/pci/mmconfig-shared.c > +++ b/arch/x86/pci/mmconfig-shared.c > @@ -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(_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(_mmcfg_lock); > - return -EINVAL; > - } > - > rc = -EBUSY; > cfg = pci_mmconfig_alloc(seg, start, end, addr); > if (cfg == NULL) { -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] x86/PCI: MMCONFIG: cleanup and add address warning to pci_mmconfig_insert
On Sat, Jul 27, 2013 at 8:27 AM, Yinghai Lu wrote: > On Fri, Jul 26, 2013 at 10:39 AM, Bjorn Helgaas wrote: >> [bhelgaas: changelog, drop printk] >> Signed-off-by: ethan.zhao >> Signed-off-by: Bjorn Helgaas >> Acked-by: Yinghai Lu > > looks like I had typo in ack. please update to > > Acked-by: Yinghai Lu Done, thanks, sorry I didn't notice that :) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] x86/PCI: MMCONFIG: cleanup and add address warning to pci_mmconfig_insert
On Fri, Jul 26, 2013 at 10:39 AM, Bjorn Helgaas wrote: > [+cc Jiang, linux-pci, -cc bjorn.helg...@hp.com (dead address)] > > On Fri, Jul 26, 2013 at 05:10:39PM +0800, ethan zhao wrote: >> Cleanup the -EINVAL return value handling and add warning message >> for invalid >> start,end,addr parameters. >> >> Signed-off-by: ethan.zhao > > This patch was corrupted, so I couldn't apply it directly. See > Documentation/SubmittingPatches, sections 5-7. > >> --- >> arch/x86/pci/mmconfig-shared.c | 12 ++-- >> 1 files changed, 6 insertions(+), 6 deletions(-) >> >> diff --git a/arch/x86/pci/mmconfig-shared.c b/arch/x86/pci/mmconfig-shared.c >> index 082e881..37f6c7f 100644 >> --- a/arch/x86/pci/mmconfig-shared.c >> +++ b/arch/x86/pci/mmconfig-shared.c >> @@ -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; >> +} > > 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? > >> >> mutex_lock(_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(_mmcfg_lock); >> -return -EINVAL; >> -} >> - >> rc = -EBUSY; >> cfg = pci_mmconfig_alloc(seg, start, end, addr); >> if (cfg == NULL) { > > > > Author: ethan.zhao > 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 > Signed-off-by: Bjorn Helgaas > Acked-by: Yinghai Lu looks like I had typo in ack. please update to Acked-by: Yinghai Lu > > diff --git a/arch/x86/pci/mmconfig-shared.c b/arch/x86/pci/mmconfig-shared.c > index 082e881..5596c7b 100644 > --- a/arch/x86/pci/mmconfig-shared.c > +++ b/arch/x86/pci/mmconfig-shared.c > @@ -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(_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(_mmcfg_lock); > - return -EINVAL; > - } > - > rc = -EBUSY; > cfg = pci_mmconfig_alloc(seg, start, end, addr); > if (cfg == NULL) { -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] x86/PCI: MMCONFIG: cleanup and add address warning to pci_mmconfig_insert
On Fri, Jul 26, 2013 at 10:39 AM, Bjorn Helgaas bhelg...@google.com wrote: [+cc Jiang, linux-pci, -cc bjorn.helg...@hp.com (dead address)] On Fri, Jul 26, 2013 at 05:10:39PM +0800, ethan zhao wrote: Cleanup the -EINVAL return value handling and add warning message for invalid start,end,addr parameters. Signed-off-by: ethan.zhao ethan.z...@oracle.com This patch was corrupted, so I couldn't apply it directly. See Documentation/SubmittingPatches, sections 5-7. --- arch/x86/pci/mmconfig-shared.c | 12 ++-- 1 files changed, 6 insertions(+), 6 deletions(-) diff --git a/arch/x86/pci/mmconfig-shared.c b/arch/x86/pci/mmconfig-shared.c index 082e881..37f6c7f 100644 --- a/arch/x86/pci/mmconfig-shared.c +++ b/arch/x86/pci/mmconfig-shared.c @@ -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; +} 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? 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) { Author: ethan.zhao ethan.z...@oracle.com 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 ethan.z...@oracle.com Signed-off-by: Bjorn Helgaas bhelg...@google.com Acked-by: Yinghai Lu ying...@kerne.org looks like I had typo in ack. please update to Acked-by: Yinghai Lu ying...@kernel.org diff --git a/arch/x86/pci/mmconfig-shared.c b/arch/x86/pci/mmconfig-shared.c index 082e881..5596c7b 100644 --- a/arch/x86/pci/mmconfig-shared.c +++ b/arch/x86/pci/mmconfig-shared.c @@ -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) { -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] x86/PCI: MMCONFIG: cleanup and add address warning to pci_mmconfig_insert
On Sat, Jul 27, 2013 at 8:27 AM, Yinghai Lu ying...@kernel.org wrote: On Fri, Jul 26, 2013 at 10:39 AM, Bjorn Helgaas bhelg...@google.com wrote: [bhelgaas: changelog, drop printk] Signed-off-by: ethan.zhao ethan.z...@oracle.com Signed-off-by: Bjorn Helgaas bhelg...@google.com Acked-by: Yinghai Lu ying...@kerne.org looks like I had typo in ack. please update to Acked-by: Yinghai Lu ying...@kernel.org Done, thanks, sorry I didn't notice that :) -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] x86/PCI: MMCONFIG: cleanup and add address warning to pci_mmconfig_insert
On Sat, Jul 27, 2013 at 1:39 AM, Bjorn Helgaas bhelg...@google.com wrote: [+cc Jiang, linux-pci, -cc bjorn.helg...@hp.com (dead address)] On Fri, Jul 26, 2013 at 05:10:39PM +0800, ethan zhao wrote: Cleanup the -EINVAL return value handling and add warning message for invalid start,end,addr parameters. Signed-off-by: ethan.zhao ethan.z...@oracle.com This patch was corrupted, so I couldn't apply it directly. See Documentation/SubmittingPatches, sections 5-7. --- arch/x86/pci/mmconfig-shared.c | 12 ++-- 1 files changed, 6 insertions(+), 6 deletions(-) diff --git a/arch/x86/pci/mmconfig-shared.c b/arch/x86/pci/mmconfig-shared.c index 082e881..37f6c7f 100644 --- a/arch/x86/pci/mmconfig-shared.c +++ b/arch/x86/pci/mmconfig-shared.c @@ -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; +} 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(). 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. I propose the patch below; what do you think? 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) { Author: ethan.zhao ethan.z...@oracle.com 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 ethan.z...@oracle.com Signed-off-by: Bjorn Helgaas bhelg...@google.com Acked-by: Yinghai Lu ying...@kerne.org diff --git a/arch/x86/pci/mmconfig-shared.c b/arch/x86/pci/mmconfig-shared.c index 082e881..5596c7b 100644 --- a/arch/x86/pci/mmconfig-shared.c +++ b/arch/x86/pci/mmconfig-shared.c @@ -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) { -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] x86/PCI: MMCONFIG: cleanup and add address warning to pci_mmconfig_insert
Seems there are code style issues etc after I pasted it in my mail client. I will correct it and resend v2. Thanks. On Sun, Jul 28, 2013 at 12:09 AM, Bjorn Helgaas bhelg...@google.com wrote: On Sat, Jul 27, 2013 at 8:27 AM, Yinghai Lu ying...@kernel.org wrote: On Fri, Jul 26, 2013 at 10:39 AM, Bjorn Helgaas bhelg...@google.com wrote: [bhelgaas: changelog, drop printk] Signed-off-by: ethan.zhao ethan.z...@oracle.com Signed-off-by: Bjorn Helgaas bhelg...@google.com Acked-by: Yinghai Lu ying...@kerne.org looks like I had typo in ack. please update to Acked-by: Yinghai Lu ying...@kernel.org Done, thanks, sorry I didn't notice that :) -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] x86/PCI: MMCONFIG: cleanup and add address warning to pci_mmconfig_insert
[+cc Jiang, linux-pci, -cc bjorn.helg...@hp.com (dead address)] On Fri, Jul 26, 2013 at 05:10:39PM +0800, ethan zhao wrote: > Cleanup the -EINVAL return value handling and add warning message > for invalid > start,end,addr parameters. > > Signed-off-by: ethan.zhao This patch was corrupted, so I couldn't apply it directly. See Documentation/SubmittingPatches, sections 5-7. > --- > arch/x86/pci/mmconfig-shared.c | 12 ++-- > 1 files changed, 6 insertions(+), 6 deletions(-) > > diff --git a/arch/x86/pci/mmconfig-shared.c b/arch/x86/pci/mmconfig-shared.c > index 082e881..37f6c7f 100644 > --- a/arch/x86/pci/mmconfig-shared.c > +++ b/arch/x86/pci/mmconfig-shared.c > @@ -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; > +} 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? > > mutex_lock(_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(_mmcfg_lock); > -return -EINVAL; > -} > - > rc = -EBUSY; > cfg = pci_mmconfig_alloc(seg, start, end, addr); > if (cfg == NULL) { Author: ethan.zhao 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 Signed-off-by: Bjorn Helgaas Acked-by: Yinghai Lu diff --git a/arch/x86/pci/mmconfig-shared.c b/arch/x86/pci/mmconfig-shared.c index 082e881..5596c7b 100644 --- a/arch/x86/pci/mmconfig-shared.c +++ b/arch/x86/pci/mmconfig-shared.c @@ -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(_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(_mmcfg_lock); - return -EINVAL; - } - rc = -EBUSY; cfg = pci_mmconfig_alloc(seg, start, end, addr); if (cfg == NULL) { -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] x86/PCI: MMCONFIG: cleanup and add address warning to pci_mmconfig_insert
On Fri, Jul 26, 2013 at 2:10 AM, ethan zhao wrote: > Cleanup the -EINVAL return value handling and add warning message for > invalid > start,end,addr parameters. > > Signed-off-by: ethan.zhao > --- > arch/x86/pci/mmconfig-shared.c | 12 ++-- > 1 files changed, 6 insertions(+), 6 deletions(-) > > diff --git a/arch/x86/pci/mmconfig-shared.c b/arch/x86/pci/mmconfig-shared.c > index 082e881..37f6c7f 100644 > --- a/arch/x86/pci/mmconfig-shared.c > +++ b/arch/x86/pci/mmconfig-shared.c > @@ -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(_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(_mmcfg_lock); > -return -EINVAL; > -} > - > rc = -EBUSY; > cfg = pci_mmconfig_alloc(seg, start, end, addr); > if (cfg == NULL) { Acked-by: Yinghai Lu -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[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 --- arch/x86/pci/mmconfig-shared.c | 12 ++-- 1 files changed, 6 insertions(+), 6 deletions(-) diff --git a/arch/x86/pci/mmconfig-shared.c b/arch/x86/pci/mmconfig-shared.c index 082e881..37f6c7f 100644 --- a/arch/x86/pci/mmconfig-shared.c +++ b/arch/x86/pci/mmconfig-shared.c @@ -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(_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(_mmcfg_lock); -return -EINVAL; -} - rc = -EBUSY; cfg = pci_mmconfig_alloc(seg, start, end, addr); if (cfg == NULL) { -- 1.7.1 >From 4f7337340c8b3b03fcedfa02ca7b0c3ba4379711 Mon Sep 17 00:00:00 2001 From: ethan.zhao 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 --- arch/x86/pci/mmconfig-shared.c | 12 ++-- 1 files changed, 6 insertions(+), 6 deletions(-) diff --git a/arch/x86/pci/mmconfig-shared.c b/arch/x86/pci/mmconfig-shared.c index 082e881..37f6c7f 100644 --- a/arch/x86/pci/mmconfig-shared.c +++ b/arch/x86/pci/mmconfig-shared.c @@ -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(_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(_mmcfg_lock); - return -EINVAL; - } - rc = -EBUSY; cfg = pci_mmconfig_alloc(seg, start, end, addr); if (cfg == NULL) { -- 1.7.1
[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 ethan.z...@oracle.com --- arch/x86/pci/mmconfig-shared.c | 12 ++-- 1 files changed, 6 insertions(+), 6 deletions(-) diff --git a/arch/x86/pci/mmconfig-shared.c b/arch/x86/pci/mmconfig-shared.c index 082e881..37f6c7f 100644 --- a/arch/x86/pci/mmconfig-shared.c +++ b/arch/x86/pci/mmconfig-shared.c @@ -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) { -- 1.7.1 From 4f7337340c8b3b03fcedfa02ca7b0c3ba4379711 Mon Sep 17 00:00:00 2001 From: ethan.zhao ethan.z...@oracle.com 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 ethan.z...@oracle.com --- arch/x86/pci/mmconfig-shared.c | 12 ++-- 1 files changed, 6 insertions(+), 6 deletions(-) diff --git a/arch/x86/pci/mmconfig-shared.c b/arch/x86/pci/mmconfig-shared.c index 082e881..37f6c7f 100644 --- a/arch/x86/pci/mmconfig-shared.c +++ b/arch/x86/pci/mmconfig-shared.c @@ -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) { -- 1.7.1
Re: [PATCH] x86/PCI: MMCONFIG: cleanup and add address warning to pci_mmconfig_insert
On Fri, Jul 26, 2013 at 2:10 AM, ethan zhao ethan.z...@oracle.com wrote: Cleanup the -EINVAL return value handling and add warning message for invalid start,end,addr parameters. Signed-off-by: ethan.zhao ethan.z...@oracle.com --- arch/x86/pci/mmconfig-shared.c | 12 ++-- 1 files changed, 6 insertions(+), 6 deletions(-) diff --git a/arch/x86/pci/mmconfig-shared.c b/arch/x86/pci/mmconfig-shared.c index 082e881..37f6c7f 100644 --- a/arch/x86/pci/mmconfig-shared.c +++ b/arch/x86/pci/mmconfig-shared.c @@ -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) { Acked-by: Yinghai Lu ying...@kerne.org -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] x86/PCI: MMCONFIG: cleanup and add address warning to pci_mmconfig_insert
[+cc Jiang, linux-pci, -cc bjorn.helg...@hp.com (dead address)] On Fri, Jul 26, 2013 at 05:10:39PM +0800, ethan zhao wrote: Cleanup the -EINVAL return value handling and add warning message for invalid start,end,addr parameters. Signed-off-by: ethan.zhao ethan.z...@oracle.com This patch was corrupted, so I couldn't apply it directly. See Documentation/SubmittingPatches, sections 5-7. --- arch/x86/pci/mmconfig-shared.c | 12 ++-- 1 files changed, 6 insertions(+), 6 deletions(-) diff --git a/arch/x86/pci/mmconfig-shared.c b/arch/x86/pci/mmconfig-shared.c index 082e881..37f6c7f 100644 --- a/arch/x86/pci/mmconfig-shared.c +++ b/arch/x86/pci/mmconfig-shared.c @@ -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; +} 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? 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) { Author: ethan.zhao ethan.z...@oracle.com 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 ethan.z...@oracle.com Signed-off-by: Bjorn Helgaas bhelg...@google.com Acked-by: Yinghai Lu ying...@kerne.org diff --git a/arch/x86/pci/mmconfig-shared.c b/arch/x86/pci/mmconfig-shared.c index 082e881..5596c7b 100644 --- a/arch/x86/pci/mmconfig-shared.c +++ b/arch/x86/pci/mmconfig-shared.c @@ -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) { -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/