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

[Bjorn Helgaas <helgaas@kernel.org>]

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