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

[Gavin Shan <gwshan@linux.vnet.ibm.com>]

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