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

[Nicholas Johnson]

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