[PATCH] drivers/misc/cxl: Avoid unnecessary error message

[PATCH] drivers/misc/cxl: Avoid unnecessary error message

[Gavin Shan]

The following error message was observed. It's complaining M32
memory window is missed on virtual PHB, which is a bit confusing.
The problem is the memory windows are never updated from its
device-tree node.

   PCI: Memory resource 0 not set for host bridge \
   /pciex@3fffe40000000/pci@0/device@0

This avoids the unnecessary error message by updating the PHB's
memory windows with pci_process_bridge_OF_ranges(). The function
is exported as well.

Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
---
 arch/powerpc/kernel/pci-common.c | 1 +
 drivers/misc/cxl/vphb.c          | 9 +++++++++
 2 files changed, 10 insertions(+)

diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
index 74bec54..b5ffd8a 100644
--- a/arch/powerpc/kernel/pci-common.c
+++ b/arch/powerpc/kernel/pci-common.c
@@ -824,6 +824,7 @@ void pci_process_bridge_OF_ranges(struct pci_controller *hose,
 		}
 	}
 }
+EXPORT_SYMBOL_GPL(pci_process_bridge_OF_ranges);
 
 /* Decide whether to display the domain number in /proc */
 int pci_proc_domain(struct pci_bus *bus)
diff --git a/drivers/misc/cxl/vphb.c b/drivers/misc/cxl/vphb.c
index 3519ace..8382761 100644
--- a/drivers/misc/cxl/vphb.c
+++ b/drivers/misc/cxl/vphb.c
@@ -215,6 +215,15 @@ int cxl_pci_vphb_add(struct cxl_afu *afu)
 	if (!phb)
 		return -ENODEV;
 
+	/* Parse IO and memory ranges */
+	if (dev_is_pci(parent)) {
+		struct pci_dev *pdev;
+
+		pdev = to_pci_dev(parent);
+		vphb_dn = pnv_pci_get_phb_node(pdev);
+		pci_process_bridge_OF_ranges(phb, vphb_dn, false);
+	}
+
 	/* Setup parent in sysfs */
 	phb->parent = parent;
 
-- 
2.7.4

Re: [PATCH] drivers/misc/cxl: Avoid unnecessary error message

[Andrew Donnellan]

On 31/01/17 14:43, Gavin Shan wrote:
> The following error message was observed. It's complaining M32
> memory window is missed on virtual PHB, which is a bit confusing.
> The problem is the memory windows are never updated from its
> device-tree node.
>
>    PCI: Memory resource 0 not set for host bridge \
>    /pciex@3fffe40000000/pci@0/device@0
>
> This avoids the unnecessary error message by updating the PHB's
> memory windows with pci_process_bridge_OF_ranges(). The function
> is exported as well.
>
> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>

I talked this over with Gavin in person today.

We don't set a memory window on the vPHB or its devices because it's not 
necessary.

The effect of this patch is to copy the memory resources from the *real* 
PHB to the vPHB, as given through the device tree. It shouldn't have any 
practical effect other than squashing this message.


> ---
>  arch/powerpc/kernel/pci-common.c | 1 +
>  drivers/misc/cxl/vphb.c          | 9 +++++++++
>  2 files changed, 10 insertions(+)
>
> diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
> index 74bec54..b5ffd8a 100644
> --- a/arch/powerpc/kernel/pci-common.c
> +++ b/arch/powerpc/kernel/pci-common.c
> @@ -824,6 +824,7 @@ void pci_process_bridge_OF_ranges(struct pci_controller *hose,
>  		}
>  	}
>  }
> +EXPORT_SYMBOL_GPL(pci_process_bridge_OF_ranges);
>
>  /* Decide whether to display the domain number in /proc */
>  int pci_proc_domain(struct pci_bus *bus)
> diff --git a/drivers/misc/cxl/vphb.c b/drivers/misc/cxl/vphb.c
> index 3519ace..8382761 100644
> --- a/drivers/misc/cxl/vphb.c
> +++ b/drivers/misc/cxl/vphb.c
> @@ -215,6 +215,15 @@ int cxl_pci_vphb_add(struct cxl_afu *afu)
>  	if (!phb)
>  		return -ENODEV;
>
> +	/* Parse IO and memory ranges */
> +	if (dev_is_pci(parent)) {

Is this ever going to be false?

> +		struct pci_dev *pdev;

I think we tend to keep declarations up at the top of the function?

> +
> +		pdev = to_pci_dev(parent);
> +		vphb_dn = pnv_pci_get_phb_node(pdev);
> +		pci_process_bridge_OF_ranges(phb, vphb_dn, false);
> +	}
> +
>  	/* Setup parent in sysfs */
>  	phb->parent = parent;
>
>

-- 
Andrew Donnellan              OzLabs, ADL Canberra
andrew.donnellan@au1.ibm.com  IBM Australia Limited

Re: [PATCH] drivers/misc/cxl: Avoid unnecessary error message

[Michael Ellerman]

Andrew Donnellan <andrew.donnellan@au1.ibm.com> writes:

> On 31/01/17 14:43, Gavin Shan wrote:
>> The following error message was observed. It's complaining M32
>> memory window is missed on virtual PHB, which is a bit confusing.
>> The problem is the memory windows are never updated from its
>> device-tree node.
>>
>>    PCI: Memory resource 0 not set for host bridge \
>>    /pciex@3fffe40000000/pci@0/device@0
>>
>> This avoids the unnecessary error message by updating the PHB's
>> memory windows with pci_process_bridge_OF_ranges(). The function
>> is exported as well.
>>
>> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
>
> I talked this over with Gavin in person today.
>
> We don't set a memory window on the vPHB or its devices because it's not 
> necessary.
>
> The effect of this patch is to copy the memory resources from the *real* 
> PHB to the vPHB, as given through the device tree. It shouldn't have any 
> practical effect other than squashing this message.

It sounds a bit backward to me. If we don't need the resources then
why have them?

If we have code that thinks that's an error, than maybe that's what
needs fixing, or special casing for the vPHB?

cheers

Re: [PATCH] drivers/misc/cxl: Avoid unnecessary error message

[Gavin Shan]

On Tue, Feb 07, 2017 at 07:14:33PM +1100, Andrew Donnellan wrote:
>On 31/01/17 14:43, Gavin Shan wrote:
>>The following error message was observed. It's complaining M32
>>memory window is missed on virtual PHB, which is a bit confusing.
>>The problem is the memory windows are never updated from its
>>device-tree node.
>>
>>   PCI: Memory resource 0 not set for host bridge \
>>   /pciex@3fffe40000000/pci@0/device@0
>>
>>This avoids the unnecessary error message by updating the PHB's
>>memory windows with pci_process_bridge_OF_ranges(). The function
>>is exported as well.
>>
>>Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
>
>I talked this over with Gavin in person today.
>
>We don't set a memory window on the vPHB or its devices because it's not
>necessary.
>
>The effect of this patch is to copy the memory resources from the *real* PHB
>to the vPHB, as given through the device tree. It shouldn't have any
>practical effect other than squashing this message.
>

Andrew, thanks for the explanation. I will provide more details
as reply to Michael's thread. We might need alternative fix for
this issue. Lets have more discussion in that thread if needed.

>
>>---
>> arch/powerpc/kernel/pci-common.c | 1 +
>> drivers/misc/cxl/vphb.c          | 9 +++++++++
>> 2 files changed, 10 insertions(+)
>>
>>diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
>>index 74bec54..b5ffd8a 100644
>>--- a/arch/powerpc/kernel/pci-common.c
>>+++ b/arch/powerpc/kernel/pci-common.c
>>@@ -824,6 +824,7 @@ void pci_process_bridge_OF_ranges(struct pci_controller *hose,
>> 		}
>> 	}
>> }
>>+EXPORT_SYMBOL_GPL(pci_process_bridge_OF_ranges);
>>
>> /* Decide whether to display the domain number in /proc */
>> int pci_proc_domain(struct pci_bus *bus)
>>diff --git a/drivers/misc/cxl/vphb.c b/drivers/misc/cxl/vphb.c
>>index 3519ace..8382761 100644
>>--- a/drivers/misc/cxl/vphb.c
>>+++ b/drivers/misc/cxl/vphb.c
>>@@ -215,6 +215,15 @@ int cxl_pci_vphb_add(struct cxl_afu *afu)
>> 	if (!phb)
>> 		return -ENODEV;
>>
>>+	/* Parse IO and memory ranges */
>>+	if (dev_is_pci(parent)) {
>
>Is this ever going to be false?
>

Yeah, I think it can be removed and to declare @pdev at the top of the function.

>>+		struct pci_dev *pdev;
>
>I think we tend to keep declarations up at the top of the function?
>

I think it's not bad idea if @pdev is invisible out of the block
of code, for a bit better code readability. However, those who
are maintaining the code might have their own personal tastes.
If it's your preferrence, I need to follow for sure.

>>+
>>+		pdev = to_pci_dev(parent);
>>+		vphb_dn = pnv_pci_get_phb_node(pdev);
>>+		pci_process_bridge_OF_ranges(phb, vphb_dn, false);
>>+	}
>>+
>> 	/* Setup parent in sysfs */
>> 	phb->parent = parent;
>>
>>

Thanks,
Gavin

Re: [PATCH] drivers/misc/cxl: Avoid unnecessary error message

[Gavin Shan]

On Tue, Feb 07, 2017 at 10:12:48PM +1100, Michael Ellerman wrote:
>Andrew Donnellan <andrew.donnellan@au1.ibm.com> writes:

.../...

>> The effect of this patch is to copy the memory resources from the *real* 
>> PHB to the vPHB, as given through the device tree. It shouldn't have any 
>> practical effect other than squashing this message.
>
>It sounds a bit backward to me. If we don't need the resources then
>why have them?
>
>If we have code that thinks that's an error, than maybe that's what
>needs fixing, or special casing for the vPHB?
>

Yeah, vPHB is a special case. There are basically two stages in PCI enumeration:
probing and then resource assignment. vPHB is different from *real* PHB as the
resource assignment is skipped on it. So vPHB doesn't need any resources to be
populated. However, there is a check in probing stage and it's where the warning
message comes from.

   drivers/misc/cxl/vphb.c::cxl_pci_vphb_add()
   arch/powerpc/kernel/pci-common.c::pcibios_scan_phb()
                                     pcibios_setup_phb_resources()

   static void pcibios_setup_phb_resources(struct pci_controller *hose,
                                        struct list_head *resources)
   {
       :
       for (i = 0; i < 3; ++i) {
            res = &hose->mem_resources[i];
            if (!res->flags) {
                if (i == 0)
                    printk(KERN_ERR "PCI: Memory resource 0 not set for "
                        "host bridge %s (domain %d)\n",
                        hose->dn->full_name, hose->global_number);
                continue;
            }
       :
   }

Alternatively, we can replace prink(KERN_ERR) with pr_debug(). It's going to
affect all PHBs including the real ones. Andrew and Michael, what do you think? :-)

Thanks,
Gavin 

Re: [PATCH] drivers/misc/cxl: Avoid unnecessary error message

[Andrew Donnellan]

On 08/02/17 10:21, Gavin Shan wrote:
> On Tue, Feb 07, 2017 at 10:12:48PM +1100, Michael Ellerman wrote:
>> Andrew Donnellan <andrew.donnellan@au1.ibm.com> writes:
>
> .../...
>
>>> The effect of this patch is to copy the memory resources from the *real*
>>> PHB to the vPHB, as given through the device tree. It shouldn't have any
>>> practical effect other than squashing this message.
>>
>> It sounds a bit backward to me. If we don't need the resources then
>> why have them?
>>
>> If we have code that thinks that's an error, than maybe that's what
>> needs fixing, or special casing for the vPHB?
>>
>
> Yeah, vPHB is a special case. There are basically two stages in PCI enumeration:
> probing and then resource assignment. vPHB is different from *real* PHB as the
> resource assignment is skipped on it. So vPHB doesn't need any resources to be
> populated. However, there is a check in probing stage and it's where the warning
> message comes from.
>
>    drivers/misc/cxl/vphb.c::cxl_pci_vphb_add()
>    arch/powerpc/kernel/pci-common.c::pcibios_scan_phb()
>                                      pcibios_setup_phb_resources()
>
>    static void pcibios_setup_phb_resources(struct pci_controller *hose,
>                                         struct list_head *resources)
>    {
>        :
>        for (i = 0; i < 3; ++i) {
>             res = &hose->mem_resources[i];
>             if (!res->flags) {
>                 if (i == 0)
>                     printk(KERN_ERR "PCI: Memory resource 0 not set for "
>                         "host bridge %s (domain %d)\n",
>                         hose->dn->full_name, hose->global_number);
>                 continue;
>             }
>        :
>    }
>
> Alternatively, we can replace prink(KERN_ERR) with pr_debug(). It's going to
> affect all PHBs including the real ones. Andrew and Michael, what do you think? :-)

In what other circumstances do we get this error printed on real PHBs?

-- 
Andrew Donnellan              OzLabs, ADL Canberra
andrew.donnellan@au1.ibm.com  IBM Australia Limited

Re: [PATCH] drivers/misc/cxl: Avoid unnecessary error message

[Gavin Shan]

On Wed, Feb 08, 2017 at 10:39:55AM +1100, Andrew Donnellan wrote:
>On 08/02/17 10:21, Gavin Shan wrote:
>>On Tue, Feb 07, 2017 at 10:12:48PM +1100, Michael Ellerman wrote:
>>>Andrew Donnellan <andrew.donnellan@au1.ibm.com> writes:
>>
>>.../...
>>
>>>>The effect of this patch is to copy the memory resources from the *real*
>>>>PHB to the vPHB, as given through the device tree. It shouldn't have any
>>>>practical effect other than squashing this message.
>>>
>>>It sounds a bit backward to me. If we don't need the resources then
>>>why have them?
>>>
>>>If we have code that thinks that's an error, than maybe that's what
>>>needs fixing, or special casing for the vPHB?
>>>
>>
>>Yeah, vPHB is a special case. There are basically two stages in PCI enumeration:
>>probing and then resource assignment. vPHB is different from *real* PHB as the
>>resource assignment is skipped on it. So vPHB doesn't need any resources to be
>>populated. However, there is a check in probing stage and it's where the warning
>>message comes from.
>>
>>   drivers/misc/cxl/vphb.c::cxl_pci_vphb_add()
>>   arch/powerpc/kernel/pci-common.c::pcibios_scan_phb()
>>                                     pcibios_setup_phb_resources()
>>
>>   static void pcibios_setup_phb_resources(struct pci_controller *hose,
>>                                        struct list_head *resources)
>>   {
>>       :
>>       for (i = 0; i < 3; ++i) {
>>            res = &hose->mem_resources[i];
>>            if (!res->flags) {
>>                if (i == 0)
>>                    printk(KERN_ERR "PCI: Memory resource 0 not set for "
>>                        "host bridge %s (domain %d)\n",
>>                        hose->dn->full_name, hose->global_number);
>>                continue;
>>            }
>>       :
>>   }
>>
>>Alternatively, we can replace prink(KERN_ERR) with pr_debug(). It's going to
>>affect all PHBs including the real ones. Andrew and Michael, what do you think? :-)
>
>In what other circumstances do we get this error printed on real PHBs?
>

When hose->mem_resources[0] isn't built from PHB's device-tree node. It means
the device-tree node's "ranges" isn't populated correctly by loader and it
should be very rare ... I never saw it before.

Thanks,
Gavin

Re: [PATCH] drivers/misc/cxl: Avoid unnecessary error message

[Michael Ellerman]

Gavin Shan <gwshan@linux.vnet.ibm.com> writes:

> On Wed, Feb 08, 2017 at 10:39:55AM +1100, Andrew Donnellan wrote:
>>On 08/02/17 10:21, Gavin Shan wrote:
>>>On Tue, Feb 07, 2017 at 10:12:48PM +1100, Michael Ellerman wrote:
>>>>Andrew Donnellan <andrew.donnellan@au1.ibm.com> writes:
>>>
>>>.../...
>>>
>>>>>The effect of this patch is to copy the memory resources from the *real*
>>>>>PHB to the vPHB, as given through the device tree. It shouldn't have any
>>>>>practical effect other than squashing this message.
>>>>
>>>>It sounds a bit backward to me. If we don't need the resources then
>>>>why have them?
>>>>
>>>>If we have code that thinks that's an error, than maybe that's what
>>>>needs fixing, or special casing for the vPHB?
>>>>
>>>
>>>Yeah, vPHB is a special case. There are basically two stages in PCI enumeration:
>>>probing and then resource assignment. vPHB is different from *real* PHB as the
>>>resource assignment is skipped on it. So vPHB doesn't need any resources to be
>>>populated. However, there is a check in probing stage and it's where the warning
>>>message comes from.
>>>
>>>   drivers/misc/cxl/vphb.c::cxl_pci_vphb_add()
>>>   arch/powerpc/kernel/pci-common.c::pcibios_scan_phb()
>>>                                     pcibios_setup_phb_resources()
>>>
>>>   static void pcibios_setup_phb_resources(struct pci_controller *hose,
>>>                                        struct list_head *resources)
>>>   {
>>>       :
>>>       for (i = 0; i < 3; ++i) {
>>>            res = &hose->mem_resources[i];
>>>            if (!res->flags) {
>>>                if (i == 0)
>>>                    printk(KERN_ERR "PCI: Memory resource 0 not set for "
>>>                        "host bridge %s (domain %d)\n",
>>>                        hose->dn->full_name, hose->global_number);
>>>                continue;
>>>            }
>>>       :
>>>   }
>>>
>>>Alternatively, we can replace prink(KERN_ERR) with pr_debug(). It's going to
>>>affect all PHBs including the real ones. Andrew and Michael, what do you think? :-)
>>
>>In what other circumstances do we get this error printed on real PHBs?
>>
>
> When hose->mem_resources[0] isn't built from PHB's device-tree node. It means
> the device-tree node's "ranges" isn't populated correctly by loader and it
> should be very rare ... I never saw it before.

Sounds to me like we could probably just drop the warning. Or make it pr_devel().

cheers

Re: [PATCH] drivers/misc/cxl: Avoid unnecessary error message

[Gavin Shan]

On Wed, Feb 08, 2017 at 01:41:19PM +1100, Michael Ellerman wrote:
>Gavin Shan <gwshan@linux.vnet.ibm.com> writes:
>
>> On Wed, Feb 08, 2017 at 10:39:55AM +1100, Andrew Donnellan wrote:
>>>On 08/02/17 10:21, Gavin Shan wrote:
>>>>On Tue, Feb 07, 2017 at 10:12:48PM +1100, Michael Ellerman wrote:
>>>>>Andrew Donnellan <andrew.donnellan@au1.ibm.com> writes:
>>>>
>>>>.../...
>>>>
>>>>>>The effect of this patch is to copy the memory resources from the *real*
>>>>>>PHB to the vPHB, as given through the device tree. It shouldn't have any
>>>>>>practical effect other than squashing this message.
>>>>>
>>>>>It sounds a bit backward to me. If we don't need the resources then
>>>>>why have them?
>>>>>
>>>>>If we have code that thinks that's an error, than maybe that's what
>>>>>needs fixing, or special casing for the vPHB?
>>>>>
>>>>
>>>>Yeah, vPHB is a special case. There are basically two stages in PCI enumeration:
>>>>probing and then resource assignment. vPHB is different from *real* PHB as the
>>>>resource assignment is skipped on it. So vPHB doesn't need any resources to be
>>>>populated. However, there is a check in probing stage and it's where the warning
>>>>message comes from.
>>>>
>>>>   drivers/misc/cxl/vphb.c::cxl_pci_vphb_add()
>>>>   arch/powerpc/kernel/pci-common.c::pcibios_scan_phb()
>>>>                                     pcibios_setup_phb_resources()
>>>>
>>>>   static void pcibios_setup_phb_resources(struct pci_controller *hose,
>>>>                                        struct list_head *resources)
>>>>   {
>>>>       :
>>>>       for (i = 0; i < 3; ++i) {
>>>>            res = &hose->mem_resources[i];
>>>>            if (!res->flags) {
>>>>                if (i == 0)
>>>>                    printk(KERN_ERR "PCI: Memory resource 0 not set for "
>>>>                        "host bridge %s (domain %d)\n",
>>>>                        hose->dn->full_name, hose->global_number);
>>>>                continue;
>>>>            }
>>>>       :
>>>>   }
>>>>
>>>>Alternatively, we can replace prink(KERN_ERR) with pr_debug(). It's going to
>>>>affect all PHBs including the real ones. Andrew and Michael, what do you think? :-)
>>>
>>>In what other circumstances do we get this error printed on real PHBs?
>>>
>>
>> When hose->mem_resources[0] isn't built from PHB's device-tree node. It means
>> the device-tree node's "ranges" isn't populated correctly by loader and it
>> should be very rare ... I never saw it before.
>
>Sounds to me like we could probably just drop the warning. Or make it pr_devel().
>

Agree, I will drop it and post the patch shortly. Thanks for confirm.

Thanks,
Gavin