From: krishna kumar <krishnak@linux.ibm.com>
To: Sourabh Jain <sourabhjain@linux.ibm.com>,
mpe@ellerman.id.au, npiggin@gmail.com
Cc: nathanl@linux.ibm.com, gbatra@linux.ibm.com,
linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org,
aneesh.kumar@kernel.org, brking@linux.vnet.ibm.com,
tpearson@raptorengineering.com, oohall@gmail.com,
bhelgaas@google.com, mahesh.salgaonkar@in.ibm.com,
linuxppc-dev@lists.ozlabs.org
Subject: Re: [PATCH 2/2] arch/powerpc: hotplug driver bridge support
Date: Tue, 14 May 2024 17:38:28 +0530 [thread overview]
Message-ID: <1873d93c-4325-4fba-b11c-9ba740b22202@linux.ibm.com> (raw)
In-Reply-To: <581d163f-837d-401a-a50d-2305de6c8804@linux.ibm.com>
Thanks Sourabh for review. I have incorporated your comments and will be
sending my patch soon.
Best Regards,
Krishna
On 5/13/24 15:15, Sourabh Jain wrote:
> Hello Krishna,
>
> Is "arch" in commit title really required?
>
> On 09/05/24 17:35, Krishna Kumar wrote:
>> There is an issue with the hotplug operation when it's done on the
>> bridge/switch slot. The bridge-port and devices behind the bridge, which
>> become offline by hot-unplug operation, don't get hot-plugged/enabled by
>> doing hot-plug operation on that slot. Only the first port of the bridge
>> gets enabled and the remaining port/devices remain unplugged. The hot
>> plug/unplug operation is done by the hotplug driver
>> (drivers/pci/hotplug/pnv_php.c).
>>
>> Root Cause Analysis: This behavior is due to missing code for the DPC
>> switch/bridge. The existing driver depends on pci_hp_add_devices()
>> function for device enablement. This function calls pci_scan_slot() on
>> only one device-node/port of the bridge, not on all the siblings'
>> device-node/port.
>>
>> The missing code needs to be added which will find all the sibling
>> device-nodes/bridge-ports and will run explicit pci_scan_slot() on
>> those. A new function has been added for this purpose which gets
>> invoked from pci_hp_add_devices(). This new function
>> pci_traverse_sibling_nodes_and_scan_slot() gets all the sibling
>> bridge-ports by traversal and explicitly invokes pci_scan_slot on them.
>>
>>
>> Signed-off-by: Krishna Kumar <krishnak@linux.ibm.com>
>> ---
>>
>> Command for reproducing the issue :
>>
>> For hot unplug/disable - echo 0 > /sys/bus/pci/slots/C5/power
>> For hot plug/enable - echo 1 > /sys/bus/pci/slots/C5/power
>>
>> where C5 is slot associated with bridge.
>>
>> Scenario/Tests:
>> Output of lspci -nn before test is given below. This snippet contains
>> devices used for testing on Powernv machine.
>>
>> 0004:02:00.0 PCI bridge [0604]: PMC-Sierra Inc. Device [11f8:4052]
>> 0004:02:01.0 PCI bridge [0604]: PMC-Sierra Inc. Device [11f8:4052]
>> 0004:02:02.0 PCI bridge [0604]: PMC-Sierra Inc. Device [11f8:4052]
>> 0004:02:03.0 PCI bridge [0604]: PMC-Sierra Inc. Device [11f8:4052]
>> 0004:08:00.0 Serial Attached SCSI controller [0107]:
>> Broadcom / LSI SAS3216 PCI-Express Fusion-MPT SAS-3 [1000:00c9] (rev 01)
>> 0004:09:00.0 Serial Attached SCSI controller [0107]:
>> Broadcom / LSI SAS3216 PCI-Express Fusion-MPT SAS-3 [1000:00c9] (rev 01)
>>
>> Output of lspci -tv before test is as follows:
>>
>> # lspci -tv
>> +-[0004:00]---00.0-[01-0e]--+-00.0-[02-0e]--+-00.0-[03-07]--
>> | | +-01.0-[08]----00.0 Broadcom / LSI
>> SAS3216 PCI-Express Fusion-MPT SAS-3
>> | | +-02.0-[09]----00.0 Broadcom / LSI
>> SAS3216 PCI-Express Fusion-MPT SAS-3
>> | | \-03.0-[0a-0e]--
>> | \-00.1 PMC-Sierra Inc. Device 4052
>>
>> C5(bridge) and C6(End Point) slot address are as below:
>> # cat /sys/bus/pci/slots/C5/address
>> 0004:02:00
>> # cat /sys/bus/pci/slots/C6/address
>> 0004:09:00
>>
>> Hot-unplug operation on slot associated with bridge:
>> # echo 0 > /sys/bus/pci/slots/C5/power
>> # lspci -tv
>> +-[0004:00]---00.0-[01-0e]--+-00.0-[02-0e]--
>> | \-00.1 PMC-Sierra Inc. Device 4052
>>
>> From the above lspci -tv output, it can be observed that hot unplug
>> operation has removed all the PMC-Sierra bridge ports like:
>> 00.0-[03-07], 01.0-[08], 02.0-[09], 03.0-[0a-0e] and the SAS devices
>> behind the bridge-port. Without the fix, when the hot plug operation is
>> done on the same slot, it adds only the first bridge port and doesn't
>> restore all the bridge-ports and devices that it unplugged earlier.
>> Below snippet shows this.
>>
>> Hot-plug operation on the bridge slot without the fix:
>> # echo 1 > /sys/bus/pci/slots/C5/power
>> # lspci -tv
>> +-[0004:00]---00.0-[01-0e]--+-00.0-[02-0e]--+-00.0-[03-07]--
>> | \-00.1 PMC-Sierra Inc. Device 4052
>>
>> After the fix, it restores all the devices in the same manner how it
>> unplugged them earlier during the hot unplug operation. The below
>> snippet
>> shows the same.
>> Hot-plug operation on bridge slot with the fix:
>> # echo 1 > /sys/bus/pci/slots/C5/power
>> # lspci -tv
>> +-[0004:00]---00.0-[01-0e]--+-00.0-[02-0e]--+-00.0-[03-07]--
>> | | +-01.0-[08]----00.0 Broadcom / LSI
>> SAS3216 PCI-Express Fusion-MPT SAS-3
>> | | +-02.0-[09]----00.0 Broadcom / LSI
>> SAS3216 PCI-Express Fusion-MPT SAS-3
>> | | \-03.0-[0a-0e]--
>> | \-00.1 PMC-Sierra Inc. Device 4052
>>
>> Removal of End point device behind bridge are also intact and behaving
>> correctly.
>> Hot-unplug operation on Endpoint device C6:
>> # echo 0 > /sys/bus/pci/slots/C6/power
>> # lspci -tv
>> +-[0004:00]---00.0-[01-0e]--+-00.0-[02-0e]--+-00.0-[03-07]--
>> | | +-01.0-[08]----00.0 Broadcom / LSI
>> SAS3216 PCI-Express Fusion-MPT SAS-3
>> | | +-02.0-[09]--
>> | | \-03.0-[0a-0e]--
>> | \-00.1 PMC-Sierra Inc. Device 4052
>>
>> Hot-plug operation on Endpoint device C6:
>> # echo 1 > /sys/bus/pci/slots/C6/power
>> # lspci -tv
>> +-[0004:00]---00.0-[01-0e]--+-00.0-[02-0e]--+-00.0-[03-07]--
>> | | +-01.0-[08]----00.0 Broadcom / LSI
>> SAS3216 PCI-Express Fusion-MPT SAS-3
>> | | +-02.0-[09]----00.0 Broadcom / LSI
>> SAS3216 PCI-Express Fusion-MPT SAS-3
>> | | \-03.0-[0a-0e]--
>> | \-00.1 PMC-Sierra Inc. Device 4052
>>
>>
>>
>> arch/powerpc/include/asm/ppc-pci.h | 4 +++
>> arch/powerpc/kernel/pci-hotplug.c | 5 ++--
>> arch/powerpc/kernel/pci_dn.c | 42 ++++++++++++++++++++++++++++++
>> 3 files changed, 48 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/ppc-pci.h
>> b/arch/powerpc/include/asm/ppc-pci.h
>> index a8b7e8682f5b..a5d5ee4ff7c0 100644
>> --- a/arch/powerpc/include/asm/ppc-pci.h
>> +++ b/arch/powerpc/include/asm/ppc-pci.h
>> @@ -28,6 +28,10 @@ struct pci_dn;
>> void *pci_traverse_device_nodes(struct device_node *start,
>> void *(*fn)(struct device_node *, void *),
>> void *data);
>> +
>> +void *pci_traverse_sibling_nodes_and_scan_slot(struct device_node
>> *start,
>> + struct pci_bus *bus);
>> +
>> extern void pci_devs_phb_init_dynamic(struct pci_controller *phb);
>> #if defined(CONFIG_IOMMU_API) && (defined(CONFIG_PPC_PSERIES) || \
>> diff --git a/arch/powerpc/kernel/pci-hotplug.c
>> b/arch/powerpc/kernel/pci-hotplug.c
>> index 0fe251c6ac2c..639a3d592fe2 100644
>> --- a/arch/powerpc/kernel/pci-hotplug.c
>> +++ b/arch/powerpc/kernel/pci-hotplug.c
>> @@ -106,7 +106,7 @@ EXPORT_SYMBOL_GPL(pci_hp_remove_devices);
>> */
>> void pci_hp_add_devices(struct pci_bus *bus)
>> {
>> - int slotno, mode, max;
>> + int mode, max;
>> struct pci_dev *dev;
>> struct pci_controller *phb;
>> struct device_node *dn = pci_bus_to_OF_node(bus);
>> @@ -129,8 +129,7 @@ void pci_hp_add_devices(struct pci_bus *bus)
>> * order for fully rescan all the way down to pick them up.
>> * They can have been removed during partial hotplug.
>> */
>> - slotno = PCI_SLOT(PCI_DN(dn->child)->devfn);
>> - pci_scan_slot(bus, PCI_DEVFN(slotno, 0));
>> + pci_traverse_sibling_nodes_and_scan_slot(dn, bus);
>> max = bus->busn_res.start;
>> /*
>> * Scan bridges that are already configured. We don't touch
>> diff --git a/arch/powerpc/kernel/pci_dn.c b/arch/powerpc/kernel/pci_dn.c
>> index 38561d6a2079..2e202f9cec21 100644
>> --- a/arch/powerpc/kernel/pci_dn.c
>> +++ b/arch/powerpc/kernel/pci_dn.c
>> @@ -493,4 +493,46 @@ static void pci_dev_pdn_setup(struct pci_dev *pdev)
>> pdn = pci_get_pdn(pdev);
>> pdev->dev.archdata.pci_data = pdn;
>> }
>> +
>> +void *pci_traverse_sibling_nodes_and_scan_slot(struct device_node
>> *start, struct pci_bus *bus)
>
> Two things regarding the return type of the above function.
>
> 1. Function only returns NULL
> 2. Caller of this function doesn't take any action based on the return
> value of this function.
>
> How about changing the return type from void * to just void?
>
>> +{
>> + struct device_node *dn;
>> + struct device_node *parent;
>
> parent variable is not really required.
>
>> + int slotno;
>> +
>> + const __be32 *classp1;
>> + u32 class1 = 0;
>> +
>> + classp1 = of_get_property(start->child, "class-code", NULL);
>> + if (classp1)
>> + class1 = of_read_number(classp1, 1);
>> +
>> + /* Call of pci_scan_slot for non-bridge/EP case */
>> + if (!((class1 >> 8) == PCI_CLASS_BRIDGE_PCI)) {
>> + slotno = PCI_SLOT(PCI_DN(start->child)->devfn);
>> + pci_scan_slot(bus, PCI_DEVFN(slotno, 0));
>> + return NULL;
>> + }
>> +
>> + /* Iterate all siblings */
>> + parent = start;
>> + for_each_child_of_node(parent, dn) {
>> + const __be32 *classp;
>> + u32 class = 0;
>> +
>> + classp = of_get_property(dn, "class-code", NULL);
>> + if (classp)
>> + class = of_read_number(classp, 1);
>> +
>> + /* Call of pci_scan_slot on each sibling-nodes/bridge-ports */
>> + if ((class >> 8) == PCI_CLASS_BRIDGE_PCI) {
>> + slotno = PCI_SLOT(PCI_DN(dn)->devfn);
>> + pci_scan_slot(bus, PCI_DEVFN(slotno, 0));
>> + }
>> + }
>> +
>> + return NULL;
>
> Some level of code duplication can be avoided if we push both cases
> 1. Call of pci_scan_slot for non-bridge/EP case
> 2. Call of pci_scan_slot for bridge port
>
> inside `for_each_child_of_node` macro.
>
> Something like this.
>
> u32 class;
> int slotno;
> const __be32 *classp;
> struct device_node *dn;
>
> for_each_child_of_node(start, dn) {
> class = 0;
> classp = of_get_property(dn, "class-code", NULL);
> if (classp)
> class = of_read_number(classp, 1);
>
> /* Call of pci_scan_slot for non-bridge/EP case */
> if (!((class >> 8) == PCI_CLASS_BRIDGE_PCI) && start->child ==
> dn) {
> slotno = PCI_SLOT(PCI_DN(dn)->devfn);
> pci_scan_slot(bus, PCI_DEVFN(slotno, 0));
> of_node_put(dn);
> return;
> }
>
> /* Call of pci_scan_slot for bridge port */
> if ((class >> 8) == PCI_CLASS_BRIDGE_PCI) {
> slotno = PCI_SLOT(PCI_DN(dn)->devfn);
> pci_scan_slot(bus, PCI_DEVFN(slotno, 0));
> }
> }
>
>
>> +}
>> +EXPORT_SYMBOL_GPL(pci_traverse_sibling_nodes_and_scan_slot);
>
> What is the need for exporting the above function?
>
>> +
>> DECLARE_PCI_FIXUP_EARLY(PCI_ANY_ID, PCI_ANY_ID, pci_dev_pdn_setup);
>
> - Sourabh Jain
prev parent reply other threads:[~2024-05-14 12:08 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-09 12:05 [PATCH 0/2] PCI hotplug driver fixes Krishna Kumar
2024-05-09 12:05 ` [PATCH 1/2] pci/hotplug/pnv_php: Fix hotplug driver crash on Powernv Krishna Kumar
2024-05-09 12:05 ` [PATCH 2/2] arch/powerpc: hotplug driver bridge support Krishna Kumar
2024-05-11 5:42 ` Andy Shevchenko
2024-05-14 12:05 ` krishna kumar
2024-05-13 9:45 ` Sourabh Jain
2024-05-14 12:08 ` krishna kumar [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1873d93c-4325-4fba-b11c-9ba740b22202@linux.ibm.com \
--to=krishnak@linux.ibm.com \
--cc=aneesh.kumar@kernel.org \
--cc=bhelgaas@google.com \
--cc=brking@linux.vnet.ibm.com \
--cc=gbatra@linux.ibm.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=mahesh.salgaonkar@in.ibm.com \
--cc=mpe@ellerman.id.au \
--cc=nathanl@linux.ibm.com \
--cc=npiggin@gmail.com \
--cc=oohall@gmail.com \
--cc=sourabhjain@linux.ibm.com \
--cc=tpearson@raptorengineering.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).