* [PATCH v12 0/4] PCI: Patch series to improve Thunderbolt enumeration
@ 2019-12-09 12:59 Nicholas Johnson
2019-12-10 21:38 ` Bjorn Helgaas
2019-12-17 15:12 ` Bjorn Helgaas
0 siblings, 2 replies; 11+ messages in thread
From: Nicholas Johnson @ 2019-12-09 12:59 UTC (permalink / raw)
To: linux-kernel
Cc: linux-pci, Bjorn Helgaas, Mika Westerberg,
Benjamin Herrenschmidt, Logan Gunthorpe, Nicholas Johnson
Hi all,
Since last time:
Reverse Christmas tree for a couple of variables
Changed while to whilst (sounds more formal, and so that
grepping for "while" only brings up code)
Made sure they still apply to latest Linux v5.5-rc1
Kind regards,
Nicholas
Nicholas Johnson (4):
PCI: Consider alignment of hot-added bridges when distributing
available resources
PCI: In extend_bridge_window() change available to new_size
PCI: Change extend_bridge_window() to set resource size directly
PCI: Allow extend_bridge_window() to shrink resource if necessary
drivers/pci/setup-bus.c | 182 +++++++++++++++++++---------------------
1 file changed, 88 insertions(+), 94 deletions(-)
--
2.24.0
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v12 0/4] PCI: Patch series to improve Thunderbolt enumeration
2019-12-09 12:59 [PATCH v12 0/4] PCI: Patch series to improve Thunderbolt enumeration Nicholas Johnson
@ 2019-12-10 21:38 ` Bjorn Helgaas
2019-12-12 10:23 ` Nicholas Johnson
2019-12-17 15:12 ` Bjorn Helgaas
1 sibling, 1 reply; 11+ messages in thread
From: Bjorn Helgaas @ 2019-12-10 21:38 UTC (permalink / raw)
To: Nicholas Johnson
Cc: linux-kernel, linux-pci, Mika Westerberg, Benjamin Herrenschmidt,
Logan Gunthorpe
On Mon, Dec 09, 2019 at 12:59:29PM +0000, Nicholas Johnson wrote:
> Hi all,
>
> Since last time:
> Reverse Christmas tree for a couple of variables
>
> Changed while to whilst (sounds more formal, and so that
> grepping for "while" only brings up code)
>
> Made sure they still apply to latest Linux v5.5-rc1
>
> Kind regards,
> Nicholas
>
> Nicholas Johnson (4):
> PCI: Consider alignment of hot-added bridges when distributing
> available resources
> PCI: In extend_bridge_window() change available to new_size
> PCI: Change extend_bridge_window() to set resource size directly
> PCI: Allow extend_bridge_window() to shrink resource if necessary
>
> drivers/pci/setup-bus.c | 182 +++++++++++++++++++---------------------
> 1 file changed, 88 insertions(+), 94 deletions(-)
Applied to pci/resource for v5.6, thanks!
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v12 0/4] PCI: Patch series to improve Thunderbolt enumeration
2019-12-10 21:38 ` Bjorn Helgaas
@ 2019-12-12 10:23 ` Nicholas Johnson
2019-12-13 18:04 ` Bjorn Helgaas
0 siblings, 1 reply; 11+ messages in thread
From: Nicholas Johnson @ 2019-12-12 10:23 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: linux-kernel, linux-pci, Mika Westerberg, Benjamin Herrenschmidt,
Logan Gunthorpe
On Tue, Dec 10, 2019 at 03:38:36PM -0600, Bjorn Helgaas wrote:
> On Mon, Dec 09, 2019 at 12:59:29PM +0000, Nicholas Johnson wrote:
> > Hi all,
> >
> > Since last time:
> > Reverse Christmas tree for a couple of variables
> >
> > Changed while to whilst (sounds more formal, and so that
> > grepping for "while" only brings up code)
> >
> > Made sure they still apply to latest Linux v5.5-rc1
> >
> > Kind regards,
> > Nicholas
> >
> > Nicholas Johnson (4):
> > PCI: Consider alignment of hot-added bridges when distributing
> > available resources
> > PCI: In extend_bridge_window() change available to new_size
> > PCI: Change extend_bridge_window() to set resource size directly
> > PCI: Allow extend_bridge_window() to shrink resource if necessary
> >
> > drivers/pci/setup-bus.c | 182 +++++++++++++++++++---------------------
> > 1 file changed, 88 insertions(+), 94 deletions(-)
>
> Applied to pci/resource for v5.6, thanks!
Thank you all for your time, support and patience with me. I have
learned a lot in the past year.
I will obviously stick around to address any potential concerns with the
patches, but it also seems like kernel development is what I want to do
as a career. Hopefully I can take this beyond a hobby despite my
physical location. Perth, Western Australia is not big on this. Perhaps
there are companies open to telecommuting employees. In any case, you
will continue to see me around.
Kind regards,
Nicholas
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v12 0/4] PCI: Patch series to improve Thunderbolt enumeration
2019-12-12 10:23 ` Nicholas Johnson
@ 2019-12-13 18:04 ` Bjorn Helgaas
0 siblings, 0 replies; 11+ messages in thread
From: Bjorn Helgaas @ 2019-12-13 18:04 UTC (permalink / raw)
To: Nicholas Johnson
Cc: Bjorn Helgaas, linux-kernel, linux-pci, Mika Westerberg,
Benjamin Herrenschmidt, Logan Gunthorpe
On Thu, Dec 12, 2019 at 4:23 AM Nicholas Johnson
<nicholas.johnson-opensource@outlook.com.au> wrote:
>
> On Tue, Dec 10, 2019 at 03:38:36PM -0600, Bjorn Helgaas wrote:
> > On Mon, Dec 09, 2019 at 12:59:29PM +0000, Nicholas Johnson wrote:
> > > Hi all,
> > >
> > > Since last time:
> > > Reverse Christmas tree for a couple of variables
> > >
> > > Changed while to whilst (sounds more formal, and so that
> > > grepping for "while" only brings up code)
> > >
> > > Made sure they still apply to latest Linux v5.5-rc1
> > >
> > > Kind regards,
> > > Nicholas
> > >
> > > Nicholas Johnson (4):
> > > PCI: Consider alignment of hot-added bridges when distributing
> > > available resources
> > > PCI: In extend_bridge_window() change available to new_size
> > > PCI: Change extend_bridge_window() to set resource size directly
> > > PCI: Allow extend_bridge_window() to shrink resource if necessary
> > >
> > > drivers/pci/setup-bus.c | 182 +++++++++++++++++++---------------------
> > > 1 file changed, 88 insertions(+), 94 deletions(-)
> >
> > Applied to pci/resource for v5.6, thanks!
> Thank you all for your time, support and patience with me. I have
> learned a lot in the past year.
>
> I will obviously stick around to address any potential concerns with the
> patches, but it also seems like kernel development is what I want to do
> as a career. Hopefully I can take this beyond a hobby despite my
> physical location. Perth, Western Australia is not big on this. Perhaps
> there are companies open to telecommuting employees. In any case, you
> will continue to see me around.
Thanks for your work. Springfield, Missouri, is not big on kernel
development either, so I can sympathize with that :)
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v12 0/4] PCI: Patch series to improve Thunderbolt enumeration
2019-12-09 12:59 [PATCH v12 0/4] PCI: Patch series to improve Thunderbolt enumeration Nicholas Johnson
2019-12-10 21:38 ` Bjorn Helgaas
@ 2019-12-17 15:12 ` Bjorn Helgaas
2019-12-18 0:54 ` Nicholas Johnson
1 sibling, 1 reply; 11+ messages in thread
From: Bjorn Helgaas @ 2019-12-17 15:12 UTC (permalink / raw)
To: Nicholas Johnson
Cc: linux-kernel, linux-pci, Mika Westerberg, Benjamin Herrenschmidt,
Logan Gunthorpe
On Mon, Dec 09, 2019 at 12:59:29PM +0000, Nicholas Johnson wrote:
> Hi all,
>
> Since last time:
> Reverse Christmas tree for a couple of variables
>
> Changed while to whilst (sounds more formal, and so that
> grepping for "while" only brings up code)
>
> Made sure they still apply to latest Linux v5.5-rc1
>
> Kind regards,
> Nicholas
>
> Nicholas Johnson (4):
> PCI: Consider alignment of hot-added bridges when distributing
> available resources
> PCI: In extend_bridge_window() change available to new_size
> PCI: Change extend_bridge_window() to set resource size directly
> PCI: Allow extend_bridge_window() to shrink resource if necessary
I have tentatively applied these to pci/resource for v5.6, but I need
some kind of high-level description for why we want these changes.
The commit logs describe what the code does, and that's good, but we
really need a little more of the *why* and what the user-visible
benefit is. I know some of this was in earlier postings, but it seems
to have gotten lost along the way.
Bjorn
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v12 0/4] PCI: Patch series to improve Thunderbolt enumeration
2019-12-17 15:12 ` Bjorn Helgaas
@ 2019-12-18 0:54 ` Nicholas Johnson
2019-12-20 0:03 ` Bjorn Helgaas
0 siblings, 1 reply; 11+ messages in thread
From: Nicholas Johnson @ 2019-12-18 0:54 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: linux-kernel, linux-pci, Mika Westerberg, Benjamin Herrenschmidt,
Logan Gunthorpe
On Tue, Dec 17, 2019 at 09:12:48AM -0600, Bjorn Helgaas wrote:
> On Mon, Dec 09, 2019 at 12:59:29PM +0000, Nicholas Johnson wrote:
> > Hi all,
> >
> > Since last time:
> > Reverse Christmas tree for a couple of variables
> >
> > Changed while to whilst (sounds more formal, and so that
> > grepping for "while" only brings up code)
> >
> > Made sure they still apply to latest Linux v5.5-rc1
> >
> > Kind regards,
> > Nicholas
> >
> > Nicholas Johnson (4):
> > PCI: Consider alignment of hot-added bridges when distributing
> > available resources
> > PCI: In extend_bridge_window() change available to new_size
> > PCI: Change extend_bridge_window() to set resource size directly
> > PCI: Allow extend_bridge_window() to shrink resource if necessary
>
> I have tentatively applied these to pci/resource for v5.6, but I need
> some kind of high-level description for why we want these changes.
I could not find these in linux-next (whereas it was almost immediate
last time), so this must be why.
>
> The commit logs describe what the code does, and that's good, but we
> really need a little more of the *why* and what the user-visible
> benefit is. I know some of this was in earlier postings, but it seems
> to have gotten lost along the way.
Is this explanation going into the commit notes, or is this just to get
it past reviewers, Greg K-H and Linus Torvalds?
Here is an attempt:
Although Thunderbolt 3 does not need special treatment like Cardbus, it
does require the kernel to be up to date with PCI hotplug and capable of
assigning resources when native enumeration mode is used (as opposed to
BIOS-assisted mode). Operating systems have neglected this over the
years, as PCI hotplug is not widely used.
Thunderbolt has a structure where each downstream facing port is a PCI
hotplug bridge. Peripheral devices may have a second Thunderbolt port
for daisy chaining more devices, much like Firewire did.
In this example, the controller is at bus 03, and this controller has
two ports. Bus 05 is the NHI (Native Host Interface) and Bus 39 is the
USB controller. Bus 07 is a Thunderbolt dock, as is Bus 0d, daisy
chained after the first dock. The hotplug bridges are 04.01, 04.04,
07.04, 0d.04, and depending on the implementation, 1c.4 under the host
controller.
+-1c.4-[03-6c]----00.0-[04-6c]--+-00.0-[05]----00.0
| +-01.0-[06-38]----00.0-[07-38]--+-00.0-[08]----00.0
| | +-01.0-[09]----00.0
| | +-02.0-[0a]--
| | +-03.0-[0b]--
| | \-04.0-[0c-38]----00.0-[0d-38]--+-00.0-[0e]----00.0
| | +-01.0-[0f]----00.0
| | +-02.0-[10]--
| | +-03.0-[11]--
| | \-04.0-[12-38]--
| +-02.0-[39]----00.0
| \-04.0-[3a-6c]--
The host controller implementation appears differently in new Intel Ice
Lake systems, with the Thunderbolt controllers integrated into the CPU,
with the Thunderbolt ports appearing as chipset root ports.
Ice Lake with 2/4 Thunderbolt 3 ports implemented (0d.0 is the USB
controller and 0d.2 is NHI #0):
-[0000:00]-+-00.0
+-02.0
+-04.0
+-07.0-[01-2b]--
+-07.1-[2c-56]--
+-0d.0
+-0d.2
Thunderbolt is unusual in that we have nested hotplug bridges. Mika
Westerberg <mika.westerberg@linux.intel.com> has done most of the work
required to assign all unused resources (busn and mem) from the parent
bridge window to the downstream hotplug bridge, allowing for more
Thunderbolt devices to be daisy-chained. However, in
drivers/pci/setup-bus.c in pci_bus_distribute_available_resources(), I
noticed problems with hot-adding Thunderbolt devices containing PCI
devices with resource alignment above the minimum 1M. Furthermore, I
found it more reliable to cease the use of additional size resource
lists, which are considered "optional" when allocating. The operation of
pci_bus_distribute_available_resources() is only as guaranteed as the
resource assignment. None of this would work with resources assigned by
the kernel parameters
pci=hpmmiosize=nn[KMG],hpmmiosize=nn[KMG],hpmmioprefsize=nn[KMG] - it
would only work if the resources under the root port were assigned by
firmware.
I have solved the alignment problem with patch 1/4 in
pci_bus_distribute_available_resources() and the remaining issues with
patches 2-4 by changing extend_bridge_window() which is now named
adjust_bridge_window(). You can find the link to the bug report filed by
Mika Westerberg in patch 1/4 commit log.
To sum up, although this could be applicable to any hypothetical
application with nested PCI hotplug bridges, the intended end user
facing case is Thunderbolt 3 with native enumeration mode. My changes:
- Allow for hot-adding PCI devices with >1M alignment of BARs
- Offer improved resilience and reliability due to removal of add_size
- Allow this to work with resources assigned with the kernel parameters
pci=hpmmiosize=nn[KMG],hpmmiosize=nn[KMG],hpmmioprefsize=nn[KMG] as
opposed to only firmware-allocated resources.
Please let me know if this is not adequate for the required purposes.
Thanks!
Kind regards,
Nicholas Johnson
>
> Bjorn
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v12 0/4] PCI: Patch series to improve Thunderbolt enumeration
2019-12-18 0:54 ` Nicholas Johnson
@ 2019-12-20 0:03 ` Bjorn Helgaas
2019-12-20 8:50 ` Nicholas Johnson
0 siblings, 1 reply; 11+ messages in thread
From: Bjorn Helgaas @ 2019-12-20 0:03 UTC (permalink / raw)
To: Nicholas Johnson
Cc: linux-kernel, linux-pci, Mika Westerberg, Benjamin Herrenschmidt,
Logan Gunthorpe
On Wed, Dec 18, 2019 at 12:54:25AM +0000, Nicholas Johnson wrote:
> On Tue, Dec 17, 2019 at 09:12:48AM -0600, Bjorn Helgaas wrote:
> > On Mon, Dec 09, 2019 at 12:59:29PM +0000, Nicholas Johnson wrote:
> > > Hi all,
> > >
> > > Since last time:
> > > Reverse Christmas tree for a couple of variables
> > >
> > > Changed while to whilst (sounds more formal, and so that
> > > grepping for "while" only brings up code)
> > >
> > > Made sure they still apply to latest Linux v5.5-rc1
> > >
> > > Kind regards,
> > > Nicholas
> > >
> > > Nicholas Johnson (4):
> > > PCI: Consider alignment of hot-added bridges when distributing
> > > available resources
> > > PCI: In extend_bridge_window() change available to new_size
> > > PCI: Change extend_bridge_window() to set resource size directly
> > > PCI: Allow extend_bridge_window() to shrink resource if necessary
> >
> > I have tentatively applied these to pci/resource for v5.6, but I need
> > some kind of high-level description for why we want these changes.
> I could not find these in linux-next (whereas it was almost immediate
> last time), so this must be why.
>
> > The commit logs describe what the code does, and that's good, but we
> > really need a little more of the *why* and what the user-visible
> > benefit is. I know some of this was in earlier postings, but it seems
> > to have gotten lost along the way.
>
> Is this explanation going into the commit notes, or is this just to get
> it past reviewers, Greg K-H and Linus Torvalds?
This is for the commit log of the merge commit, i.e., it should answer
the question of "why should we merge this branch?" Typically this is
short, e.g., here's the merge commit for the pci/resource branch that
was merged for v5.5:
commit 774800cb099f ("Merge branch 'pci/resource'")
Author: Bjorn Helgaas <bhelgaas@google.com>
Date: Thu Nov 28 08:54:36 2019 -0600
Merge branch 'pci/resource'
- Protect pci_reassign_bridge_resources() against concurrent
addition/removal (Benjamin Herrenschmidt)
- Fix bridge dma_ranges resource list cleanup (Rob Herring)
- Add PCI_STD_NUM_BARS for the number of standard BARs (Denis Efremov)
- Add "pci=hpmmiosize" and "pci=hpmmioprefsize" parameters to control the
MMIO and prefetchable MMIO window sizes of hotplug bridges
independently (Nicholas Johnson)
- Fix MMIO/MMIO_PREF window assignment that assigned more space than
desired (Nicholas Johnson)
- Only enforce bus numbers from bridge EA if the bridge has EA devices
downstream (Subbaraya Sundeep)
* pci/resource:
PCI: Do not use bus number zero from EA capability
PCI: Avoid double hpmemsize MMIO window assignment
PCI: Add "pci=hpmmiosize" and "pci=hpmmioprefsize" parameters
PCI: Add PCI_STD_NUM_BARS for the number of standard BARs
PCI: Fix missing bridge dma_ranges resource list cleanup
PCI: Protect pci_reassign_bridge_resources() against concurrent addition/removal
The logs for individual commits are obviously longer but should answer
the same question in more detail.
Basically, I'm not comfortable asking Linus to pull material unless I
can explain what the benefit is. I'm still struggling to articulate
the benefit in this case. I think it makes hotplug work better in
some cases where we need more alignment than we currently have, but
that's pretty sketchy.
Bjorn
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v12 0/4] PCI: Patch series to improve Thunderbolt enumeration
2019-12-20 0:03 ` Bjorn Helgaas
@ 2019-12-20 8:50 ` Nicholas Johnson
2019-12-20 10:18 ` Mika Westerberg
2019-12-20 14:38 ` Bjorn Helgaas
0 siblings, 2 replies; 11+ messages in thread
From: Nicholas Johnson @ 2019-12-20 8:50 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: linux-kernel, linux-pci, Mika Westerberg, Benjamin Herrenschmidt,
Logan Gunthorpe
On Thu, Dec 19, 2019 at 06:03:58PM -0600, Bjorn Helgaas wrote:
> On Wed, Dec 18, 2019 at 12:54:25AM +0000, Nicholas Johnson wrote:
> > On Tue, Dec 17, 2019 at 09:12:48AM -0600, Bjorn Helgaas wrote:
> > > On Mon, Dec 09, 2019 at 12:59:29PM +0000, Nicholas Johnson wrote:
> > > > Hi all,
> > > >
> > > > Since last time:
> > > > Reverse Christmas tree for a couple of variables
> > > >
> > > > Changed while to whilst (sounds more formal, and so that
> > > > grepping for "while" only brings up code)
> > > >
> > > > Made sure they still apply to latest Linux v5.5-rc1
> > > >
> > > > Kind regards,
> > > > Nicholas
> > > >
> > > > Nicholas Johnson (4):
> > > > PCI: Consider alignment of hot-added bridges when distributing
> > > > available resources
Prevent failure to assign hot-added Thunderbolt PCI BARs with alignment >1M
> > > > PCI: In extend_bridge_window() change available to new_size
Change variable name in extend_bridge_window() to better reflect its
purpose
^ I would have preferred this not be its own commit. Is it too late to
squash it back together with patch 3/4?
> > > > PCI: Change extend_bridge_window() to set resource size directly
Use guaranteed PCI resource size instead of optional add_size when
adjusting bridge windows
> > > > PCI: Allow extend_bridge_window() to shrink resource if necessary
Prevent failure to extend nested hotplug bridge window if pci=hpmmiosize
or similar kernel parameter used
> > >
> > > I have tentatively applied these to pci/resource for v5.6, but I need
> > > some kind of high-level description for why we want these changes.
> > I could not find these in linux-next (whereas it was almost immediate
> > last time), so this must be why.
> >
> > > The commit logs describe what the code does, and that's good, but we
> > > really need a little more of the *why* and what the user-visible
> > > benefit is. I know some of this was in earlier postings, but it seems
> > > to have gotten lost along the way.
> >
> > Is this explanation going into the commit notes, or is this just to get
> > it past reviewers, Greg K-H and Linus Torvalds?
>
> This is for the commit log of the merge commit, i.e., it should answer
> the question of "why should we merge this branch?" Typically this is
> short, e.g., here's the merge commit for the pci/resource branch that
> was merged for v5.5:
>
> commit 774800cb099f ("Merge branch 'pci/resource'")
> Author: Bjorn Helgaas <bhelgaas@google.com>
> Date: Thu Nov 28 08:54:36 2019 -0600
>
> Merge branch 'pci/resource'
>
> - Protect pci_reassign_bridge_resources() against concurrent
> addition/removal (Benjamin Herrenschmidt)
>
> - Fix bridge dma_ranges resource list cleanup (Rob Herring)
>
> - Add PCI_STD_NUM_BARS for the number of standard BARs (Denis Efremov)
>
> - Add "pci=hpmmiosize" and "pci=hpmmioprefsize" parameters to control the
> MMIO and prefetchable MMIO window sizes of hotplug bridges
> independently (Nicholas Johnson)
>
> - Fix MMIO/MMIO_PREF window assignment that assigned more space than
> desired (Nicholas Johnson)
>
> - Only enforce bus numbers from bridge EA if the bridge has EA devices
> downstream (Subbaraya Sundeep)
>
> * pci/resource:
> PCI: Do not use bus number zero from EA capability
> PCI: Avoid double hpmemsize MMIO window assignment
> PCI: Add "pci=hpmmiosize" and "pci=hpmmioprefsize" parameters
> PCI: Add PCI_STD_NUM_BARS for the number of standard BARs
> PCI: Fix missing bridge dma_ranges resource list cleanup
> PCI: Protect pci_reassign_bridge_resources() against concurrent addition/removal
>
> The logs for individual commits are obviously longer but should answer
> the same question in more detail.
>
> Basically, I'm not comfortable asking Linus to pull material unless I
> can explain what the benefit is. I'm still struggling to articulate
> the benefit in this case. I think it makes hotplug work better in
> some cases where we need more alignment than we currently have, but
> that's pretty sketchy.
In my opinion, fixing failure to assign is a clear reason to merge,
especially when the failure will result in a user wondering why the
device they plugged in does not work. If that is not enough to get past
Linus Torvalds then I will have to go back to the drawing board, because
it is the best I can think of (for now).
I have had a go above for the four patches.
Mika, what would you have written if you had to do this?
>
> Bjorn
Thanks!
Kind regards,
Nicholas
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v12 0/4] PCI: Patch series to improve Thunderbolt enumeration
2019-12-20 8:50 ` Nicholas Johnson
@ 2019-12-20 10:18 ` Mika Westerberg
2019-12-20 14:38 ` Bjorn Helgaas
1 sibling, 0 replies; 11+ messages in thread
From: Mika Westerberg @ 2019-12-20 10:18 UTC (permalink / raw)
To: Nicholas Johnson
Cc: Bjorn Helgaas, linux-kernel, linux-pci, Benjamin Herrenschmidt,
Logan Gunthorpe
On Fri, Dec 20, 2019 at 08:50:14AM +0000, Nicholas Johnson wrote:
> Mika, what would you have written if you had to do this?
Heh, I'm not the best person to ask :) Something like below maybe?
Improve PCI resource allocation on systems with Thunderbolt
add-in-card but no support from BIOS side.
I think that's what your series is about.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v12 0/4] PCI: Patch series to improve Thunderbolt enumeration
2019-12-20 8:50 ` Nicholas Johnson
2019-12-20 10:18 ` Mika Westerberg
@ 2019-12-20 14:38 ` Bjorn Helgaas
2019-12-21 5:48 ` Nicholas Johnson
1 sibling, 1 reply; 11+ messages in thread
From: Bjorn Helgaas @ 2019-12-20 14:38 UTC (permalink / raw)
To: Nicholas Johnson
Cc: linux-kernel, linux-pci, Mika Westerberg, Benjamin Herrenschmidt,
Logan Gunthorpe
On Fri, Dec 20, 2019 at 08:50:14AM +0000, Nicholas Johnson wrote:
> On Thu, Dec 19, 2019 at 06:03:58PM -0600, Bjorn Helgaas wrote:
> > On Wed, Dec 18, 2019 at 12:54:25AM +0000, Nicholas Johnson wrote:
> > > On Tue, Dec 17, 2019 at 09:12:48AM -0600, Bjorn Helgaas wrote:
> > > > On Mon, Dec 09, 2019 at 12:59:29PM +0000, Nicholas Johnson wrote:
> > > > > Nicholas Johnson (4):
> > > > > PCI: Consider alignment of hot-added bridges when distributing
> > > > > available resources
> Prevent failure to assign hot-added Thunderbolt PCI BARs with alignment >1M
>
> > > > > PCI: In extend_bridge_window() change available to new_size
> Change variable name in extend_bridge_window() to better reflect its
> purpose
>
> ^ I would have preferred this not be its own commit. Is it too late to
> squash it back together with patch 3/4?
Not too late; it's trivial to squash it. I consider these branches to
be drafts, subject to revision until I ask Linus to pull them.
But ... why? In general, the smaller the patch the better.
> > Basically, I'm not comfortable asking Linus to pull material unless I
> > can explain what the benefit is. I'm still struggling to articulate
> > the benefit in this case. I think it makes hotplug work better in
> > some cases where we need more alignment than we currently have, but
> > that's pretty sketchy.
>
> In my opinion, fixing failure to assign is a clear reason to merge,
> especially when the failure will result in a user wondering why the
> device they plugged in does not work.
Sure. But there's nothing specific in the commit logs about what the
problem is and how these fix it.
For example, I think the first patch ("PCI: Consider alignment of
hot-added bridges when distributing available resources") is something
to do with increasing the alignment of bridge windows to account for
descendants that require greater alignment.
But the log says "Rewrite pci_bus_distribute_available_resources to
better handle bridges with different resource alignment requirements."
That says nothing about what the problem was or how you fix it.
Ideally that patch would very specifically change *alignment* details.
It currently also contains a bunch of other changes (interface change,
removing "b" in favor "dev->subordinate", etc). These make the patch
bigger and harder to understand and justify. Some of these lead up to
the alignment change but possibly could be split to separate patches.
Bjorn
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v12 0/4] PCI: Patch series to improve Thunderbolt enumeration
2019-12-20 14:38 ` Bjorn Helgaas
@ 2019-12-21 5:48 ` Nicholas Johnson
0 siblings, 0 replies; 11+ messages in thread
From: Nicholas Johnson @ 2019-12-21 5:48 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: linux-kernel, linux-pci, Mika Westerberg, Benjamin Herrenschmidt,
Logan Gunthorpe
On Fri, Dec 20, 2019 at 08:38:56AM -0600, Bjorn Helgaas wrote:
> On Fri, Dec 20, 2019 at 08:50:14AM +0000, Nicholas Johnson wrote:
> > On Thu, Dec 19, 2019 at 06:03:58PM -0600, Bjorn Helgaas wrote:
> > > On Wed, Dec 18, 2019 at 12:54:25AM +0000, Nicholas Johnson wrote:
> > > > On Tue, Dec 17, 2019 at 09:12:48AM -0600, Bjorn Helgaas wrote:
> > > > > On Mon, Dec 09, 2019 at 12:59:29PM +0000, Nicholas Johnson wrote:
>
> > > > > > Nicholas Johnson (4):
> > > > > > PCI: Consider alignment of hot-added bridges when distributing
> > > > > > available resources
> > Prevent failure to assign hot-added Thunderbolt PCI BARs with alignment >1M
> >
> > > > > > PCI: In extend_bridge_window() change available to new_size
> > Change variable name in extend_bridge_window() to better reflect its
> > purpose
> >
> > ^ I would have preferred this not be its own commit. Is it too late to
> > squash it back together with patch 3/4?
>
> Not too late; it's trivial to squash it. I consider these branches to
> be drafts, subject to revision until I ask Linus to pull them.
>
> But ... why? In general, the smaller the patch the better.
Now that I had to justify why were are putting it in, it does not fix a
bug or have any effect on the end user. But it seems I was mistaken in
that I have to write reasons for each patch. It seems like it is one for
the whole series.
>
> > > Basically, I'm not comfortable asking Linus to pull material unless I
> > > can explain what the benefit is. I'm still struggling to articulate
> > > the benefit in this case. I think it makes hotplug work better in
> > > some cases where we need more alignment than we currently have, but
> > > that's pretty sketchy.
> >
> > In my opinion, fixing failure to assign is a clear reason to merge,
> > especially when the failure will result in a user wondering why the
> > device they plugged in does not work.
>
> Sure. But there's nothing specific in the commit logs about what the
> problem is and how these fix it.
>
> For example, I think the first patch ("PCI: Consider alignment of
> hot-added bridges when distributing available resources") is something
> to do with increasing the alignment of bridge windows to account for
> descendants that require greater alignment.
>
> But the log says "Rewrite pci_bus_distribute_available_resources to
> better handle bridges with different resource alignment requirements."
> That says nothing about what the problem was or how you fix it.
When I first started the patches a year ago, it was all about my
Thunderbolt add-in cards. Since then, I have realised that although that
is the motivation, there is nothing in this code specific to
Thunderbolt. Hence, I tried to write the logs in terms of what it means
for PCI in general, and tried to leave mentions of Thunderbolt out. If I
recall correctly, the fact that there is nothing specific to Thunderbolt
here has been brought up by reviewers.
>
> Ideally that patch would very specifically change *alignment* details.
> It currently also contains a bunch of other changes (interface change,
> removing "b" in favor "dev->subordinate", etc). These make the patch
> bigger and harder to understand and justify. Some of these lead up to
> the alignment change but possibly could be split to separate patches.
I tried to split the first patch up, but it got very messy, very
quickly. I tried to do the interface change first, but I found variable
names to clash when making partial changes. The overall number of lines
of diff would have been much greater to achieve the same thing, and each
patch was not a whole lot smaller.
I can re-submit with changed commit logs if you wish, to mention the
Thunderbolt problems.
Patches 2, 3 and 4 can be applied first if that helps.
I can try to split 1 up more, but it is going to be a lot of work and
I am not convinced you will like the results much more.
>
> Bjorn
Nicholas
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2019-12-21 5:49 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-09 12:59 [PATCH v12 0/4] PCI: Patch series to improve Thunderbolt enumeration Nicholas Johnson
2019-12-10 21:38 ` Bjorn Helgaas
2019-12-12 10:23 ` Nicholas Johnson
2019-12-13 18:04 ` Bjorn Helgaas
2019-12-17 15:12 ` Bjorn Helgaas
2019-12-18 0:54 ` Nicholas Johnson
2019-12-20 0:03 ` Bjorn Helgaas
2019-12-20 8:50 ` Nicholas Johnson
2019-12-20 10:18 ` Mika Westerberg
2019-12-20 14:38 ` Bjorn Helgaas
2019-12-21 5:48 ` Nicholas Johnson
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).