Re: [nicholas.johnson-opensource@outlook.com.au: [PATCH v6 4/4] PCI: Add pci=hpmemprefsize parameter to set MMIO_PREF size independently]

[Nicholas Johnson <nicholas.johnson-opensource@outlook.com.au>]

On Thu, Jun 20, 2019 at 08:47:12AM -0500, Bjorn Helgaas wrote:
> On Wed, Jun 19, 2019 at 07:35:21PM -0600, Logan Gunthorpe wrote:
> > On 2019-06-19 6:56 p.m., Nicholas Johnson wrote:
> > > On Wed, Jun 19, 2019 at 10:45:38AM -0600, Logan Gunthorpe wrote:
> > >> On 2019-06-19 8:01 a.m., Nicholas Johnson wrote:
> > >>> ----- Forwarded message from Nicholas Johnson <nicholas.johnson-opensource@outlook.com.au> -----
> > >>>
> > >>> Date: Thu, 23 May 2019 06:29:28 +0800
> > >>> From: Nicholas Johnson <nicholas.johnson-opensource@outlook.com.au>
> > >>> To: linux-kernel@vger.kernel.org
> > >>> Cc: linux-pci@vger.kernel.org, bhelgaas@google.com, mika.westerberg@linux.intel.com, corbet@lwn.net, Nicholas Johnson <nicholas.johnson-opensource@outlook.com.au>
> > >>> Subject: [PATCH v6 4/4] PCI: Add pci=hpmemprefsize parameter to set MMIO_PREF size independently
> > >>> X-Mailer: git-send-email 2.19.1
> > >>>
> > >>> Add kernel parameter pci=hpmemprefsize=nn[KMG] to control
> > >>> MMIO_PREF size for PCI hotplug bridges.
> > >>
> > >> Makes sense.
> > >>
> > >>> Change behaviour of pci=hpmemsize=nn[KMG] to not set MMIO_PREF
> > >>> size if hpmempref has been specified, rather than controlling
> > >>> both MMIO and MMIO_PREF sizes unconditionally.
> > >>
> > >> I don't think I like that fact that hpmemsize behaves differently
> > >> if hpmempref size is specfied before it. I'd probably suggest
> > >> having three parameters: hpmemsize which sets both as it always
> > >> has, a pref one and a regular one which each set one of
> > >> parameters.
> > > 
> > > It does not matter if hpmempref is specified before or after
> > > hpmemsize.  I made sure of that.
> > 
> > > Originally, I proposed to depreciate hpiosize, hpmemsize, and
> > > introduce: hp_io_size, hp_mmio_size, hp_mmio_pref_size, each
> > > controlling its own window exclusively.
> > > 
> > > The patch had the old parameters work with a warning, and if the
> > > new ones were specified, they would override the old ones. Then,
> > > after a few kernel releases, the old ones could be removed.
> > 
> > Well I don't like that either. No need to depreciate hpmemsize.
> > 
> > > Bjorn insisted that there be nil changes which break the existing
> > > parameters, and the solution he requested was to leave hpmemsize
> > > to work exactly the same (controlling both MMIO and MMIO_PREF),
> > > unless hpmemprefsize is given, which will take control of
> > > MMIO_PREF from hpmemsize.
> > 
> > I agree with Bjorn here too but my suggestion is to leave hpmemsize
> > alone and have it set both values as it has always done. And add two
> > new parameters to set one or the other. Then there's none of this
> > "sets one if the other one wasn't set". Also, if I only want to
> > change the non-preftechable version then your method leaves no way
> > to do so without setting the preftechable version.
> 
> Adding two new parameters sounds like a good idea to me.
Yeah, that is basically what I did originally (except I depreciated the 
old ones rather than keeping them).

I did it this way on your direct advice in keeping with minimal lines of 
diff, minimal disruption, etc. If I were to do this, the number of lines 
of diff will increase and then I will be fielding complaints that it is 
too large to sign off.

I am already scrambling to make last minute changes before end of 
release to the other patches and I am not even convinced that that stuff 
is going to get accepted based on proximity to deadline and how many 
change requests are flying around.

So I am going to have to respectfully decline to do this for now. I need 
to know earlier in the release cycle if I am going to go back on stuff I 
have already been asked to do.

Cheers.