[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20191023104345.GY2819@lahna.fi.intel.com>
Date: Wed, 23 Oct 2019 13:43:45 +0300
From: "mika.westerberg@...ux.intel.com" <mika.westerberg@...ux.intel.com>
To: Nicholas Johnson <nicholas.johnson-opensource@...look.com.au>
Cc: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
"bhelgaas@...gle.com" <bhelgaas@...gle.com>,
"corbet@....net" <corbet@....net>,
"benh@...nel.crashing.org" <benh@...nel.crashing.org>,
"logang@...tatee.com" <logang@...tatee.com>
Subject: Re: [PATCH 1/1] PCI: Add hp_mmio_size and hp_mmio_pref_size
parameters
On Wed, Oct 23, 2019 at 10:33:42AM +0000, Nicholas Johnson wrote:
> On Wed, Oct 23, 2019 at 01:03:59PM +0300, mika.westerberg@...ux.intel.com wrote:
> > On Wed, Oct 23, 2019 at 09:57:17AM +0000, Nicholas Johnson wrote:
> > > On Wed, Oct 23, 2019 at 12:47:43PM +0300, mika.westerberg@...ux.intel.com wrote:
> > > > On Wed, Oct 23, 2019 at 08:37:48AM +0000, Nicholas Johnson wrote:
> > > > > } else if (!strncmp(str, "hpmemsize=", 10)) {
> > > > > - pci_hotplug_mem_size = memparse(str + 10, &str);
> > > > > + pci_hotplug_mmio_size =
> > > > > + memparse(str + 10, &str);
> > > > > + pci_hotplug_mmio_pref_size =
> > > > > + memparse(str + 10, &str);
> > > >
> > > > Does this actually work correctly? The first memparse(str + 10, &str)
> > > > modifies str so the next call will not start from the correct position
> > > > anymore.
> > > I have been using this for a long time now and have not had any issues.
> > > Does it modify str? I thought that was done by the loop.
> >
> > If you add "hpmemsize=xxx" in the command line and print both
> > pci_hotplug_mmio_size and pci_hotplug_mmio_pref_size after the
> > assignment, do they have the same value? If yes, then there is no
> > problem.
> Looking at lib/cmdline.c line 125, it looks like there is no point in me
> testing it. It looks like you are right.
>
> What is the better fix?
>
> pci_hotplug_mmio_size = pci_hotplug_mmio_pref_size = memparse(str + 10, &str);
>
> ^ Could be too long, even if we are ignoring the 80-character limit.
I prefer this:
pci_hotplug_mmio_size = memparse(str + 10, &str);
pci_hotplug_mmio_pref_size = pci_hotplug_mmio_size;
And you can ignore the 80-char limit. The above is much more readable
IMHO.
Powered by blists - more mailing lists