Re: [PATCH v8 1/6] PCI: Consider alignment of hot-added bridges when distributing available resources

[Bjorn Helgaas]

On Wed, Oct 23, 2019 at 09:08:42AM +0000, Nicholas Johnson wrote:
> On Tue, Oct 08, 2019 at 02:38:12PM +0300, mika.westerberg@xxxxxxxxxxxxxxx wrote:
> > On Fri, Jul 26, 2019 at 12:53:19PM +0000, Nicholas Johnson wrote:

> > >  	/*
> > > -	 * Calculate the total amount of extra resource space we can
> > > -	 * pass to bridges below this one.  This is basically the
> > > -	 * extra space reduced by the minimal required space for the
> > > -	 * non-hotplug bridges.
> > > +	 * Reduce the available resource space by what the
> > > +	 * bridge and devices below it occupy.
> > 
> > This can be widen:
> I avoided changing comments because Bjorn said it creates distracting 
> noise. But I am considering changing tactics because what I have been 
> doing has not been working.

I think Mika's point was not that you should avoid changing the
comment, but that your new comment could be rewrapped so it used the
whole 80 column width, which matches the rest of the file.  That's
trivial to do and if you don't do it I can do it while applying the
patch.

> > 	/*
> > 	 * Reduce the available resource space by what the bridge and
> > 	 * devices below it occupy.
> > 	 */
> > 
> > >  	 */
> > > -	remaining_io = available_io;
> > > -	remaining_mmio = available_mmio;
> > > -	remaining_mmio_pref = available_mmio_pref;
> > > -
> > >  	for_each_pci_bridge(dev, bus) {
> > > -		const struct resource *res;
> > > +		struct resource *res;
> > > +		resource_size_t used_size;
> > 
> > Some people like "reverse christmas tree" format better:
> We had this discussion a while ago, and Bjorn piped in and said it is 
> not enforced. However, I will give it a go this time.

I usually don't request changes in the order, so it doesn't really
matter to me, but I personally put the declarations in the order of
their use in the code below.

> > 		resource_size_t used_size;
> > 		struct resource *res;

> > > -		pci_bus_distribute_available_resources(b, add_list, io, mmio,
> > > -						       mmio_pref);
> > > +		io.start = io.end + 1;
> > 
> > I think you can also write it like:
> > 
> > 		io.start += io_per_hp;
> You are possibly correct - and it is impressive that you saw that. I 
> never did. The way that I have written it fits in with the thought 
> patterns I used to create it ("set the start of the next window to be 
> just after the end of the last"). I will take this suggestion as you 
> wanting it written that way (provided testing goes fine).
> 
> > > +		mmio.start = mmio.end + 1;
> > > +		mmio_pref.start = mmio_pref.end + 1;

I assume you'll do that for mmio.start and mmio_pref.start as well?

Bjorn