linux-pci.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Yijing Wang <wangyijing@huawei.com>
To: Jon Mason <jdmason@kudzu.us>, Bjorn Helgaas <bhelgaas@google.com>
Cc: "linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
	Hanjun Guo <guohanjun@huawei.com>,
	Jiang Liu <jiang.liu@huawei.com>
Subject: Re: [PATCH v5 1/2] PCI: fix the only slot identification in pcie_find_smpss()
Date: Tue, 20 Aug 2013 10:55:00 +0800	[thread overview]
Message-ID: <5212DA84.6070409@huawei.com> (raw)
In-Reply-To: <CAPoiz9wMo+sr1EWp+fPdwKTBoJpQGjZg-dASRMHAtvPBE+G4Eg@mail.gmail.com>

>> Actually, I don't think it matters whether the slot is currently
>> occupied or not.  If the bridge supports hot-plug, we have to handle
>> the current device (if any) as well as any other device that might be
>> added after removing the current device.
> 
> Yes, this is true.  So, we want to see if there is the ability to:
> 1. have a device hotplugged
> and
> 2a. there are other devices under the same root port
> or
> 2b. there are multiple hotplug slots under the same root port
> 
> The patch covers part 2b (unless I missunderstand and it only covers
> the devices present and not the slots available), but we need part 2a
> to be covered.  I believe your suggested code above covers 2a.  Am I
> off my rocker?

I'm a little confused, let us list the possible pcie topo here:

1.
root port -------------->[slot]-(function device a)
			       -(function device b)

Here there is only one slot directly connected to root port,
this slot is inserted a physical device which contain function device a and b.
After we insert physical device into this slot, hotplug driver will call
pci_configure_slot()---->pcie_bus_configure_settings() to set device mps.

As Jon's original patch, in this case we should update root port and slot device
mps to the minmum mpss of root port and slot device. But in pcie_find_smpss()
list_is_singular() is used to check whether there are other devices on the fabric.
list_is_singular() in this case return false. because more than one devices found in devices list.
But we expect the result is true.

And Bjorn think root port always connected to only one hot-plug slot, so use list_is_singular() or pci_only_one_slot()
to check other devices on the fabric make no sense. I agree with him if there is no broken hardware platform that
a root port connected to more than one slot.

2.		(is_hotplug_bridge)
root port --------------->Port A--------->[slot]
		bus a	 |
			 |Port or devices ?

If Jon's original patch is correct, I guess the topo is like above.
In this case, Port A is hotplug bridge and connected directly to root port.
So if there is no other port or devices in bus a, we can configure mps to the minmum
mpss of root port and Port A and slot device.

But does this topo exist? root port has only one link, the pcie link is point to point.

3.                                                (hotplug bridge)
root port ---------------Up steam port----------->Downsteam port A -------->[slot]
						 |
						 |Downsteam port B -------->[slot]  ?

This topo is common, but DP A is not directly connected to root port, so the original patch
can not cover this case. If DP B is not exist. there is only one Downstream port A, like:

                                                (hotplug bridge)
root port ---------------Up steam port----------->Downsteam port A -------->[slot]

In this case, we can also configure mps after a pcie card inserted into slot, because
although the system is running, because root port UP port DP port A and slot are not running.
But this case is rare.

So, Jon, does your original patch want to cover the case 1 or case 2 ?
For case 1, we can remove the list_is_singular() check.
For case 2, do you have some example hardware platform ?
Or there are other pcie topos not exist here?

Thanks!
Yijing.



  reply	other threads:[~2013-08-20  2:55 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-08-15  2:18 [PATCH v5 0/2] update device mps Yijing Wang
2013-08-15  2:18 ` [PATCH v5 1/2] PCI: fix the only slot identification in pcie_find_smpss() Yijing Wang
2013-08-16 22:17   ` Bjorn Helgaas
2013-08-19  2:49     ` Yijing Wang
2013-08-19 17:42       ` Jon Mason
2013-08-19 18:17         ` Bjorn Helgaas
2013-08-19 19:39           ` Bjorn Helgaas
2013-08-19 23:45             ` Jon Mason
2013-08-20  2:55               ` Yijing Wang [this message]
2013-08-20  4:44                 ` Bjorn Helgaas
2013-08-20 12:23                   ` Yijing Wang
2013-08-20 20:12                     ` Bjorn Helgaas
2013-08-19 23:34           ` Jon Mason
2013-08-15  2:18 ` [PATCH v5 2/2] PCI: update device mps when doing pci hotplug Yijing Wang

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=5212DA84.6070409@huawei.com \
    --to=wangyijing@huawei.com \
    --cc=bhelgaas@google.com \
    --cc=guohanjun@huawei.com \
    --cc=jdmason@kudzu.us \
    --cc=jiang.liu@huawei.com \
    --cc=linux-pci@vger.kernel.org \
    /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 NNTP newsgroup(s).