Re: [PATCH v12 0/4] PCI: Patch series to improve Thunderbolt enumeration

From: Nicholas Johnson
Date: Fri Dec 20 2019 - 03:50:23 EST


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@xxxxxxxxxx>
> 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