Re: [PATCH 2/4] hwmon: Use subdir-ccflags-* to inherit debug flag

[Bjorn Helgaas]

On Fri, Feb 05, 2021 at 10:28:32AM -0800, Guenter Roeck wrote:
> On Fri, Feb 05, 2021 at 05:44:13PM +0800, Yicong Yang wrote:
> > From: Junhao He 
> > 
> > Use subdir-ccflags-* instead of ccflags-* to inherit the debug
> > settings from Kconfig when traversing subdirectories.
> > 
> > Suggested-by: Bjorn Helgaas 
> > Signed-off-by: Junhao He 
> > Signed-off-by: Yicong Yang 
> 
> What problem does this fix ? Maybe I am missing it, but I don't see
> DEBUG being used in a subdirectory of drivers/hwmon.

It's my fault for raising this question [1].  Yicong fixed a real
problem in drivers/pci, where we are currently using

  ccflags-$(CONFIG_PCI_DEBUG) := -DDEBUG

so CONFIG_PCI_DEBUG=y turns on debug in drivers/pci, but not in the
subdirectories.  That's surprising to users.

So my question was whether we should default to using subdir-ccflags
for -DDEBUG in general, and only use ccflags when we have
subdirectories that have their own debug options, e.g.,

  drivers/i2c/Makefile:ccflags-$(CONFIG_I2C_DEBUG_CORE) := -DDEBUG
  drivers/i2c/algos/Makefile:ccflags-$(CONFIG_I2C_DEBUG_ALGO) := -DDEBUG
  drivers/i2c/busses/Makefile:ccflags-$(CONFIG_I2C_DEBUG_BUS) := -DDEBUG
  drivers/i2c/muxes/Makefile:ccflags-$(CONFIG_I2C_DEBUG_BUS) := -DDEBUG

I mentioned drivers/hwmon along with a few others that have
subdirectories, do not have per-subdirectory debug options, and use
ccflags.  I didn't try to determine whether those subdirectories
currently use -DDEBUG.

In the case of drivers/hwmon, several drivers do use pr_debug(),
and CONFIG_HWMON_DEBUG_CHIP=y turns those on.  But if somebody
were to add pr_debug() to drivers/hwmon/occ/common.c, for example,
CONFIG_HWMON_DEBUG_CHIP=y would *not* turn it on.  That sounds
surprising to me, but if that's what you intend, that's totally fine.

[1] https://lore.kernel.org/r/20210204161048.GA68790@bjorn-Precision-5520

> > ---
> >  drivers/hwmon/Makefile | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> > index 09a86c5..1c0c089 100644
> > --- a/drivers/hwmon/Makefile
> > +++ b/drivers/hwmon/Makefile
> > @@ -201,5 +201,5 @@ obj-$(CONFIG_SENSORS_XGENE) += xgene-hwmon.o
> >  obj-$(CONFIG_SENSORS_OCC)  += occ/
> >  obj-$(CONFIG_PMBUS)+= pmbus/
> >  
> > -ccflags-$(CONFIG_HWMON_DEBUG_CHIP) := -DDEBUG
> > +subdir-ccflags-$(CONFIG_HWMON_DEBUG_CHIP) := -DDEBUG
> >  
> > -- 
> > 2.8.1
> > 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] PCI: Rename d3_delay in the pci_dev struct to align with PCI specification

[Bjorn Helgaas]

On Thu, Jul 30, 2020 at 09:08:48PM +, Krzysztof Wilczyński wrote:
> Rename PCI-related variable "d3_delay" to "d3hot_delay" in the pci_dev
> struct to better align with the PCI Firmware specification (see PCI
> Firmware Specification, Revision 3.2, Section 4.6.9, p. 73).
> 
> The pci_dev struct already contains variable "d3cold_delay", thus
> renaming "d3_delay" to "d3hot_delay" reduces ambiguity as PCI devices
> support two variants of the D3 power state: D3hot and D3cold.
> 
> Also, rename other constants and variables, and updates code comments
> and documentation to ensure alignment with the PCI specification.
> 
> There is no change to the functionality.
> 
> Signed-off-by: Krzysztof Wilczyński 

Applied to pci/pm for v5.10, thanks!

> ---
>  Documentation/power/pci.rst   |  2 +-
>  arch/x86/pci/fixup.c  |  2 +-
>  arch/x86/pci/intel_mid_pci.c  |  2 +-
>  drivers/hid/intel-ish-hid/ipc/ipc.c   |  2 +-
>  drivers/net/ethernet/marvell/sky2.c   |  2 +-
>  drivers/pci/pci-acpi.c|  6 +-
>  drivers/pci/pci.c | 14 ++--
>  drivers/pci/pci.h |  4 +-
>  drivers/pci/quirks.c  | 68 +--
>  .../staging/media/atomisp/pci/atomisp_v4l2.c  |  2 +-
>  include/linux/pci.h   |  2 +-
>  include/uapi/linux/pci_regs.h |  2 +-
>  12 files changed, 54 insertions(+), 54 deletions(-)
> 
> diff --git a/Documentation/power/pci.rst b/Documentation/power/pci.rst
> index 1831e431f725..b04fb18cc4e2 100644
> --- a/Documentation/power/pci.rst
> +++ b/Documentation/power/pci.rst
> @@ -320,7 +320,7 @@ that these callbacks operate on::
>   unsigned intd2_support:1;   /* Low power state D2 is supported */
>   unsigned intno_d1d2:1;  /* D1 and D2 are forbidden */
>   unsigned intwakeup_prepared:1;  /* Device prepared for wake up */
> - unsigned intd3_delay;   /* D3->D0 transition time in ms */
> + unsigned intd3hot_delay;/* D3hot->D0 transition time in ms */
>   ...
>};
>  
> diff --git a/arch/x86/pci/fixup.c b/arch/x86/pci/fixup.c
> index 0c67a5a94de3..9e3d9cc6afc4 100644
> --- a/arch/x86/pci/fixup.c
> +++ b/arch/x86/pci/fixup.c
> @@ -587,7 +587,7 @@ DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0xa26d, 
> pci_invalid_bar);
>  static void pci_fixup_amd_ehci_pme(struct pci_dev *dev)
>  {
>   dev_info(>dev, "PME# does not work under D3, disabling it\n");
> - dev->pme_support &= ~((PCI_PM_CAP_PME_D3 | PCI_PM_CAP_PME_D3cold)
> + dev->pme_support &= ~((PCI_PM_CAP_PME_D3hot | PCI_PM_CAP_PME_D3cold)
>   >> PCI_PM_CAP_PME_SHIFT);
>  }
>  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_AMD, 0x7808, pci_fixup_amd_ehci_pme);
> diff --git a/arch/x86/pci/intel_mid_pci.c b/arch/x86/pci/intel_mid_pci.c
> index 00c62115f39c..979f310b67d4 100644
> --- a/arch/x86/pci/intel_mid_pci.c
> +++ b/arch/x86/pci/intel_mid_pci.c
> @@ -322,7 +322,7 @@ static void pci_d3delay_fixup(struct pci_dev *dev)
>*/
>   if (type1_access_ok(dev->bus->number, dev->devfn, PCI_DEVICE_ID))
>   return;
> - dev->d3_delay = 0;
> + dev->d3hot_delay = 0;
>  }
>  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_ANY_ID, pci_d3delay_fixup);
>  
> diff --git a/drivers/hid/intel-ish-hid/ipc/ipc.c 
> b/drivers/hid/intel-ish-hid/ipc/ipc.c
> index 8f8dfdf64833..a45ac7fa417b 100644
> --- a/drivers/hid/intel-ish-hid/ipc/ipc.c
> +++ b/drivers/hid/intel-ish-hid/ipc/ipc.c
> @@ -755,7 +755,7 @@ static int _ish_hw_reset(struct ishtp_device *dev)
>   csr |= PCI_D3hot;
>   pci_write_config_word(pdev, pdev->pm_cap + PCI_PM_CTRL, csr);
>  
> - mdelay(pdev->d3_delay);
> + mdelay(pdev->d3hot_delay);
>  
>   csr &= ~PCI_PM_CTRL_STATE_MASK;
>   csr |= PCI_D0;
> diff --git a/drivers/net/ethernet/marvell/sky2.c 
> b/drivers/net/ethernet/marvell/sky2.c
> index fe54764caea9..ce7a94060a96 100644
> --- a/drivers/net/ethernet/marvell/sky2.c
> +++ b/drivers/net/ethernet/marvell/sky2.c
> @@ -5104,7 +5104,7 @@ static int sky2_probe(struct pci_dev *pdev, const 
> struct pci_device_id *ent)
>   INIT_WORK(>restart_work, sky2_restart);
>  
>   pci_set_drvdata(pdev, hw);
> - pdev->d3_delay = 300;
> + pdev->d3hot_delay = 300;
>  
>   return 0;
>  
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index 7224b1e5f2a8..c54588ad2d9c 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -1167,7 +1167,7 @@ static struct acpi_device 
> *acpi_pci_find_companion(struct device *dev)
>   * @pdev: the PCI device whose delay is to be updated
>   * @handle: ACPI handle of this device
>   *
> - * Update the d3_delay and d3cold_delay of a PCI device from the ACPI _DSM
> + * Update the d3hot_delay and d3cold_delay of a PCI device from the ACPI _DSM
>   * control method of either the device itself or the PCI host bridge.
>   *
>   * 

Re: [PATCH] staging: atomisp: move null check to earlier point

[Bjorn Helgaas]

On Thu, Jul 30, 2020 at 11:45:45AM +0300, Dan Carpenter wrote:
> On Wed, Jul 29, 2020 at 06:13:44PM +0300, Andy Shevchenko wrote:
> > On Wed, Jul 29, 2020 at 5:00 PM Cengiz Can  wrote:
> > >
> > > `find_gmin_subdev` function that returns a pointer to `struct
> > > gmin_subdev` can return NULL.
> > >
> > > In `gmin_v2p8_ctrl` there's a call to this function but the possibility
> > > of a NULL was not checked before its being dereferenced. ie:
> > >
> > > ```
> > > /* Acquired here v */
> > > struct gmin_subdev *gs = find_gmin_subdev(subdev);
> > > int ret;
> > > int value;
> > >
> > > /*  v--Dereferenced here */
> > > if (gs->v2p8_gpio >= 0) {
> > > pr_info("atomisp_gmin_platform: 2.8v power on GPIO %d\n",
> > > gs->v2p8_gpio);
> > > ret = gpio_request(gs->v2p8_gpio, "camera_v2p8");
> > > if (!ret)
> > > ret = gpio_direction_output(gs->v2p8_gpio, 0);
> > > if (ret)
> > > pr_err("V2P8 GPIO initialization failed\n");
> > > }
> > > ```
> > >
> > > I have moved the NULL check before deref point.
> > 
> > "Move the NULL check..."
> > See Submitting Patches documentation how to avoid "This patch", "I", "we", 
> > etc.
> 
> I always feel like this is a pointless requirement.  We're turning
> into bureaucrats.

There is a danger of that, and I'm more guilty than most.  But I do
think there's value in consistent style because it allows readers to
focus on the content instead of being distracted by different margins,
grammar ("move vs. moved"), paragraph styles, quoting conventions,
etc.

Ideally we would scan previous commit logs (and the existing code!)
and make new changes fit seamlessly so it looks like everything was
done at the same time by the same person.

But often that doesn't happen.  Sometimes I take the liberty to tweak
things as I apply them to try to avoid trivial rework.

Bjorn
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v1 1/4] qlge/qlge_main.c: use genric power management

[Bjorn Helgaas]

Vaibhav: s/genric/generic/ in the subject

On Tue, Jun 30, 2020 at 12:09:36AM +0800, kernel test robot wrote:
> Hi Vaibhav,
> 
> Thank you for the patch! Yet something to improve:
> 
> [auto build test ERROR on staging/staging-testing]
> [also build test ERROR on v5.8-rc3 next-20200629]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use  as documented in
> https://git-scm.com/docs/git-format-patch]
> 
> url:
> https://github.com/0day-ci/linux/commits/Vaibhav-Gupta/drivers-staging-use-generic-power-management/20200629-163141
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git 
> 347fa58ff5558075eec98725029c443c80ffbf4a
> config: x86_64-rhel-7.6 (attached as .config)
> compiler: gcc-9 (Debian 9.3.0-13) 9.3.0
> reproduce (this is a W=1 build):
> # save the attached .config to linux build tree
> make W=1 ARCH=x86_64 
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot 

If the patch has already been merged and we need an incremental patch
that fixes *only* the build issue, I think it's fine to add a
"Reported-by" tag.

But if this patch hasn't been merged anywhere, I think adding the
"Reported-by" tag would be pointless and distracting.  This report
should result in a v2 posting of the patch with the build issue fixed.

There will be no evidence of the problem in the v2 patch.  The patch
itself contains other changes unrelated to the build issue, so
"Reported-by" makes so sense for them.  I would treat this as just
another review comment, and we don't usually credit those in the
commit log (though it's nice if they're mentioned in the v2 cover
letter so reviewers know what changed and why).

Is there any chance kbuild could be made smart enough to suggest the
tag only when it finds an issue in some list of published trees?

> All errors (new ones prefixed by >>):
> 
>drivers/staging/qlge/qlge_main.c: In function 'qlge_resume':
> >> drivers/staging/qlge/qlge_main.c:4793:17: error: 'pdev' undeclared (first 
> >> use in this function); did you mean 'qdev'?
> 4793 |  pci_set_master(pdev);
>  | ^~~~
>  | qdev
> ...
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3 0/7] PCI: PM: Move to D0 before calling pci_legacy_resume_early()

[Bjorn Helgaas]

On Mon, Oct 14, 2019 at 06:00:09PM -0500, Bjorn Helgaas wrote:
> From: Bjorn Helgaas 
> 
> Dexuan, the important thing here is the first patch, which is your [1],
> which I modified by doing pci_restore_state() as well as setting to D0:
> 
>   pci_set_power_state(pci_dev, PCI_D0);
>   pci_restore_state(pci_dev);
> 
> I'm proposing some more patches on top.  None are relevant to the problem
> you're solving; they're just minor doc and other updates in the same area.
> 
> Rafael, if you have a chance to look at these, I'd appreciate it.  I tried
> to make the doc match the code, but I'm no PM expert.
> 
> [1] 
> https://lore.kernel.org/r/ku1p153mb016637caead346f0aa8e3801bf...@ku1p153mb0166.apcp153.prod.outlook.com
> 
> 
> Dexuan Cui (1):
>   PCI/PM: Always return devices to D0 when thawing
> 
> Bjorn Helgaas (6):
>   PCI/PM: Correct pci_pm_thaw_noirq() documentation
>   PCI/PM: Clear PCIe PME Status even for legacy power management
>   PCI/PM: Run resume fixups before disabling wakeup events
>   PCI/PM: Make power management op coding style consistent
>   PCI/PM: Wrap long lines in documentation
>   PCI/MSI: Move power state check out of pci_msi_supported()
> 
>  Documentation/power/pci.rst | 38 +++---
>  drivers/pci/msi.c   |  6 +--
>  drivers/pci/pci-driver.c| 99 ++---
>  3 files changed, 71 insertions(+), 72 deletions(-)

Thanks Dexuan and Rafael for taking a look at these!

I applied the first six to pci/pm and the last to pci/msi, all for
v5.5.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 7/7] PCI/MSI: Move power state check out of pci_msi_supported()

[Bjorn Helgaas]

From: Bjorn Helgaas 

27e20603c54b ("PCI/MSI: Move D0 check into pci_msi_check_device()")
moved the power state check into pci_msi_check_device(), which was
subsequently renamed to pci_msi_supported().  This didn't change the
behavior, since both callers checked the power state.

However, it doesn't fit the current "pci_msi_supported()" name, which
should return what the device is capable of, independent of the power
state.

Move the power state check back into the callers for readability.  No
functional change intended.

Signed-off-by: Bjorn Helgaas 
---
 drivers/pci/msi.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index 0884bedcfc7a..20e9c729617c 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -861,7 +861,7 @@ static int pci_msi_supported(struct pci_dev *dev, int nvec)
if (!pci_msi_enable)
return 0;
 
-   if (!dev || dev->no_msi || dev->current_state != PCI_D0)
+   if (!dev || dev->no_msi)
return 0;
 
/*
@@ -972,7 +972,7 @@ static int __pci_enable_msix(struct pci_dev *dev, struct 
msix_entry *entries,
int nr_entries;
int i, j;
 
-   if (!pci_msi_supported(dev, nvec))
+   if (!pci_msi_supported(dev, nvec) || dev->current_state != PCI_D0)
return -EINVAL;
 
nr_entries = pci_msix_vec_count(dev);
@@ -1058,7 +1058,7 @@ static int __pci_enable_msi_range(struct pci_dev *dev, 
int minvec, int maxvec,
int nvec;
int rc;
 
-   if (!pci_msi_supported(dev, minvec))
+   if (!pci_msi_supported(dev, minvec) || dev->current_state != PCI_D0)
return -EINVAL;
 
/* Check whether driver already requested MSI-X IRQs */
-- 
2.23.0.700.g56cf767bdb-goog

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 1/7] PCI/PM: Always return devices to D0 when thawing

[Bjorn Helgaas]

From: Dexuan Cui 

pci_pm_thaw_noirq() is supposed to return the device to D0 and restore its
configuration registers, but previously it only did that for devices whose
drivers implemented the new power management ops.

Hibernation, e.g., via "echo disk > /sys/power/state", involves freezing
devices, creating a hibernation image, thawing devices, writing the image,
and powering off.  The fact that thawing did not return devices with legacy
power management to D0 caused errors, e.g., in this path:

  pci_pm_thaw_noirq
if (pci_has_legacy_pm_support(pci_dev)) # true for Mellanox VF driver
  return pci_legacy_resume_early(dev)   # ... legacy PM skips the rest
pci_set_power_state(pci_dev, PCI_D0)
pci_restore_state(pci_dev)
  pci_pm_thaw
if (pci_has_legacy_pm_support(pci_dev))
  pci_legacy_resume
drv->resume
  mlx4_resume
...
  pci_enable_msix_range
...
  if (dev->current_state != PCI_D0)  # <---
return -EINVAL;

which caused these warnings:

  mlx4_core a6d1:00:02.0: INTx is not supported in multi-function mode, aborting
  PM: dpm_run_callback(): pci_pm_thaw+0x0/0xd7 returns -95
  PM: Device a6d1:00:02.0 failed to thaw: error -95

Return devices to D0 and restore config registers for all devices, not just
those whose drivers support new power management.

[bhelgaas: also call pci_restore_state() before pci_legacy_resume_early(),
update comment, add stable tag, commit log]
Link: 
https://lore.kernel.org/r/ku1p153mb016637caead346f0aa8e3801bf...@ku1p153mb0166.apcp153.prod.outlook.com
Signed-off-by: Dexuan Cui 
Signed-off-by: Bjorn Helgaas 
Cc: sta...@vger.kernel.org  # v4.13+
---
 drivers/pci/pci-driver.c | 17 +++--
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index a8124e47bf6e..d4ac8ce8c1f9 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -1076,17 +1076,22 @@ static int pci_pm_thaw_noirq(struct device *dev)
return error;
}
 
-   if (pci_has_legacy_pm_support(pci_dev))
-   return pci_legacy_resume_early(dev);
-
/*
-* pci_restore_state() requires the device to be in D0 (because of MSI
-* restoration among other things), so force it into D0 in case the
-* driver's "freeze" callbacks put it into a low-power state directly.
+* Both the legacy ->resume_early() and the new pm->thaw_noirq()
+* callbacks assume the device has been returned to D0 and its
+* config state has been restored.
+*
+* In addition, pci_restore_state() restores MSI-X state in MMIO
+* space, which requires the device to be in D0, so return it to D0
+* in case the driver's "freeze" callbacks put it into a low-power
+* state.
 */
pci_set_power_state(pci_dev, PCI_D0);
pci_restore_state(pci_dev);
 
+   if (pci_has_legacy_pm_support(pci_dev))
+   return pci_legacy_resume_early(dev);
+
if (drv && drv->pm && drv->pm->thaw_noirq)
error = drv->pm->thaw_noirq(dev);
 
-- 
2.23.0.700.g56cf767bdb-goog

[PATCH 4/7] PCI/PM: Run resume fixups before disabling wakeup events

[Bjorn Helgaas]

From: Bjorn Helgaas 

pci_pm_resume() and pci_pm_restore() call pci_pm_default_resume(), which
runs resume fixups before disabling wakeup events:

  static void pci_pm_default_resume(struct pci_dev *pci_dev)
  {
pci_fixup_device(pci_fixup_resume, pci_dev);
pci_enable_wake(pci_dev, PCI_D0, false);
  }

pci_pm_runtime_resume() does both of these, but in the opposite order:

  pci_enable_wake(pci_dev, PCI_D0, false);
  pci_fixup_device(pci_fixup_resume, pci_dev);

We should always use the same ordering unless there's a reason to do
otherwise.  Change pci_pm_runtime_resume() to call pci_pm_default_resume()
instead of open-coding this, so the fixups are always done before disabling
wakeup events.

Signed-off-by: Bjorn Helgaas 
---
 drivers/pci/pci-driver.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index 0c3086793e4e..55acb658273f 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -1345,8 +1345,7 @@ static int pci_pm_runtime_resume(struct device *dev)
return 0;
 
pci_fixup_device(pci_fixup_resume_early, pci_dev);
-   pci_enable_wake(pci_dev, PCI_D0, false);
-   pci_fixup_device(pci_fixup_resume, pci_dev);
+   pci_pm_default_resume(pci_dev);
 
if (pm && pm->runtime_resume)
rc = pm->runtime_resume(dev);
-- 
2.23.0.700.g56cf767bdb-goog

[PATCH 6/7] PCI/PM: Wrap long lines in documentation

[Bjorn Helgaas]

From: Bjorn Helgaas 

Documentation/power/pci.rst is wrapped to fit in 80 columns, but directory
structure changes made a few lines longer.  Wrap them so they all fit in 80
columns again.

Signed-off-by: Bjorn Helgaas 
---
 Documentation/power/pci.rst | 28 ++--
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/Documentation/power/pci.rst b/Documentation/power/pci.rst
index 1525c594d631..db41a770a2f5 100644
--- a/Documentation/power/pci.rst
+++ b/Documentation/power/pci.rst
@@ -426,12 +426,12 @@ pm->runtime_idle() callback.
 2.4. System-Wide Power Transitions
 --
 There are a few different types of system-wide power transitions, described in
-Documentation/driver-api/pm/devices.rst.  Each of them requires devices to be 
handled
-in a specific way and the PM core executes subsystem-level power management
-callbacks for this purpose.  They are executed in phases such that each phase
-involves executing the same subsystem-level callback for every device belonging
-to the given subsystem before the next phase begins.  These phases always run
-after tasks have been frozen.
+Documentation/driver-api/pm/devices.rst.  Each of them requires devices to be
+handled in a specific way and the PM core executes subsystem-level power
+management callbacks for this purpose.  They are executed in phases such that
+each phase involves executing the same subsystem-level callback for every 
device
+belonging to the given subsystem before the next phase begins.  These phases
+always run after tasks have been frozen.
 
 2.4.1. System Suspend
 ^
@@ -636,12 +636,12 @@ System restore requires a hibernation image to be loaded 
into memory and the
 pre-hibernation memory contents to be restored before the pre-hibernation 
system
 activity can be resumed.
 
-As described in Documentation/driver-api/pm/devices.rst, the hibernation image 
is loaded
-into memory by a fresh instance of the kernel, called the boot kernel, which in
-turn is loaded and run by a boot loader in the usual way.  After the boot 
kernel
-has loaded the image, it needs to replace its own code and data with the code
-and data of the "hibernated" kernel stored within the image, called the image
-kernel.  For this purpose all devices are frozen just like before creating
+As described in Documentation/driver-api/pm/devices.rst, the hibernation image
+is loaded into memory by a fresh instance of the kernel, called the boot 
kernel,
+which in turn is loaded and run by a boot loader in the usual way.  After the
+boot kernel has loaded the image, it needs to replace its own code and data 
with
+the code and data of the "hibernated" kernel stored within the image, called 
the
+image kernel.  For this purpose all devices are frozen just like before 
creating
 the image during hibernation, in the
 
prepare, freeze, freeze_noirq
@@ -691,8 +691,8 @@ controlling the runtime power management of their devices.
 
 At the time of this writing there are two ways to define power management
 callbacks for a PCI device driver, the recommended one, based on using a
-dev_pm_ops structure described in Documentation/driver-api/pm/devices.rst, and 
the
-"legacy" one, in which the .suspend(), .suspend_late(), .resume_early(), and
+dev_pm_ops structure described in Documentation/driver-api/pm/devices.rst, and
+the "legacy" one, in which the .suspend(), .suspend_late(), .resume_early(), 
and
 .resume() callbacks from struct pci_driver are used.  The legacy approach,
 however, doesn't allow one to define runtime power management callbacks and is
 not really suitable for any new drivers.  Therefore it is not covered by this
-- 
2.23.0.700.g56cf767bdb-goog

[PATCH 5/7] PCI/PM: Make power management op coding style consistent

[Bjorn Helgaas]

From: Bjorn Helgaas 

Some of the power management ops use this style:

  struct device_driver *drv = dev->driver;
  if (drv && drv->pm && drv->pm->prepare(dev))
drv->pm->prepare(dev);

while others use this:

  const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
  if (pm && pm->runtime_resume)
pm->runtime_resume(dev);

Convert the first style to the second so they're all consistent.  Remove
local "error" variables when unnecessary.  No functional change intended.

Signed-off-by: Bjorn Helgaas 
---
 drivers/pci/pci-driver.c | 76 +++-
 1 file changed, 36 insertions(+), 40 deletions(-)

diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index 55acb658273f..abbf5c39cb9c 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -679,11 +679,11 @@ static bool pci_has_legacy_pm_support(struct pci_dev 
*pci_dev)
 
 static int pci_pm_prepare(struct device *dev)
 {
-   struct device_driver *drv = dev->driver;
struct pci_dev *pci_dev = to_pci_dev(dev);
+   const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
 
-   if (drv && drv->pm && drv->pm->prepare) {
-   int error = drv->pm->prepare(dev);
+   if (pm && pm->prepare) {
+   int error = pm->prepare(dev);
if (error < 0)
return error;
 
@@ -917,8 +917,7 @@ static int pci_pm_suspend_noirq(struct device *dev)
 static int pci_pm_resume_noirq(struct device *dev)
 {
struct pci_dev *pci_dev = to_pci_dev(dev);
-   struct device_driver *drv = dev->driver;
-   int error = 0;
+   const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
 
if (dev_pm_may_skip_resume(dev))
return 0;
@@ -946,17 +945,16 @@ static int pci_pm_resume_noirq(struct device *dev)
if (pci_has_legacy_pm_support(pci_dev))
return pci_legacy_resume_early(dev);
 
-   if (drv && drv->pm && drv->pm->resume_noirq)
-   error = drv->pm->resume_noirq(dev);
+   if (pm && pm->resume_noirq)
+   return pm->resume_noirq(dev);
 
-   return error;
+   return 0;
 }
 
 static int pci_pm_resume(struct device *dev)
 {
struct pci_dev *pci_dev = to_pci_dev(dev);
const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
-   int error = 0;
 
/*
 * This is necessary for the suspend error path in which resume is
@@ -972,12 +970,12 @@ static int pci_pm_resume(struct device *dev)
 
if (pm) {
if (pm->resume)
-   error = pm->resume(dev);
+   return pm->resume(dev);
} else {
pci_pm_reenable_device(pci_dev);
}
 
-   return error;
+   return 0;
 }
 
 #else /* !CONFIG_SUSPEND */
@@ -1038,16 +1036,16 @@ static int pci_pm_freeze(struct device *dev)
 static int pci_pm_freeze_noirq(struct device *dev)
 {
struct pci_dev *pci_dev = to_pci_dev(dev);
-   struct device_driver *drv = dev->driver;
+   const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
 
if (pci_has_legacy_pm_support(pci_dev))
return pci_legacy_suspend_late(dev, PMSG_FREEZE);
 
-   if (drv && drv->pm && drv->pm->freeze_noirq) {
+   if (pm && pm->freeze_noirq) {
int error;
 
-   error = drv->pm->freeze_noirq(dev);
-   suspend_report_result(drv->pm->freeze_noirq, error);
+   error = pm->freeze_noirq(dev);
+   suspend_report_result(pm->freeze_noirq, error);
if (error)
return error;
}
@@ -1066,8 +1064,8 @@ static int pci_pm_freeze_noirq(struct device *dev)
 static int pci_pm_thaw_noirq(struct device *dev)
 {
struct pci_dev *pci_dev = to_pci_dev(dev);
-   struct device_driver *drv = dev->driver;
-   int error = 0;
+   const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+   int error;
 
if (pcibios_pm_ops.thaw_noirq) {
error = pcibios_pm_ops.thaw_noirq(dev);
@@ -1091,10 +1089,10 @@ static int pci_pm_thaw_noirq(struct device *dev)
if (pci_has_legacy_pm_support(pci_dev))
return pci_legacy_resume_early(dev);
 
-   if (drv && drv->pm && drv->pm->thaw_noirq)
-   error = drv->pm->thaw_noirq(dev);
+   if (pm && pm->thaw_noirq)
+   return pm->thaw_noirq(dev);
 
-   return error;
+   return 0;
 }
 
 static int pci_pm_thaw(struct device *dev)
@@ -1165,24 +1163,24 @@ static int pci_pm_poweroff_late(struct 

[PATCH 3/7] PCI/PM: Clear PCIe PME Status even for legacy power management

[Bjorn Helgaas]

From: Bjorn Helgaas 

Previously, pci_pm_resume_noirq() cleared the PME Status bit in the Root
Status register only if the device had no driver or the driver did not
implement legacy power management.  It should clear PME Status regardless
of what sort of power management the driver supports, so do this before
checking for legacy power management.

This affects Root Ports and Root Complex Event Collectors, for which the
usual driver is the PCIe portdrv, which implements new power management, so
this change is just on principle, not to fix any actual defects.

Fixes: a39bd851dccf ("PCI/PM: Clear PCIe PME Status bit in core, not PCIe port 
driver")
Signed-off-by: Bjorn Helgaas 
---
 drivers/pci/pci-driver.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index d4ac8ce8c1f9..0c3086793e4e 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -941,12 +941,11 @@ static int pci_pm_resume_noirq(struct device *dev)
pci_pm_default_resume_early(pci_dev);
 
pci_fixup_device(pci_fixup_resume_early, pci_dev);
+   pcie_pme_root_status_cleanup(pci_dev);
 
if (pci_has_legacy_pm_support(pci_dev))
return pci_legacy_resume_early(dev);
 
-   pcie_pme_root_status_cleanup(pci_dev);
-
if (drv && drv->pm && drv->pm->resume_noirq)
error = drv->pm->resume_noirq(dev);
 
-- 
2.23.0.700.g56cf767bdb-goog

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 2/7] PCI/PM: Correct pci_pm_thaw_noirq() documentation

[Bjorn Helgaas]

From: Bjorn Helgaas 

According to the documentation, pci_pm_thaw_noirq() did not put the device
into the full-power state and restore its standard configuration registers.
This is incorrect, so update the documentation to match the code.

Signed-off-by: Bjorn Helgaas 
---
 Documentation/power/pci.rst | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/Documentation/power/pci.rst b/Documentation/power/pci.rst
index 0e2ef7429304..1525c594d631 100644
--- a/Documentation/power/pci.rst
+++ b/Documentation/power/pci.rst
@@ -600,17 +600,17 @@ using the following PCI bus type's callbacks::
 
 respectively.
 
-The first of them, pci_pm_thaw_noirq(), is analogous to pci_pm_resume_noirq(),
-but it doesn't put the device into the full power state and doesn't attempt to
-restore its standard configuration registers.  It also executes the device
-driver's pm->thaw_noirq() callback, if defined, instead of pm->resume_noirq().
+The first of them, pci_pm_thaw_noirq(), is analogous to pci_pm_resume_noirq().
+It puts the device into the full power state and restores its standard
+configuration registers.  It also executes the device driver's pm->thaw_noirq()
+callback, if defined, instead of pm->resume_noirq().
 
 The pci_pm_thaw() routine is similar to pci_pm_resume(), but it runs the device
 driver's pm->thaw() callback instead of pm->resume().  It is executed
 asynchronously for different PCI devices that don't depend on each other in a
 known way.
 
-The complete phase it the same as for system resume.
+The complete phase is the same as for system resume.
 
 After saving the image, devices need to be powered down before the system can
 enter the target sleep state (ACPI S4 for ACPI-based systems).  This is done in
-- 
2.23.0.700.g56cf767bdb-goog

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v3 0/7] PCI: PM: Move to D0 before calling pci_legacy_resume_early()

[Bjorn Helgaas]

From: Bjorn Helgaas 

Dexuan, the important thing here is the first patch, which is your [1],
which I modified by doing pci_restore_state() as well as setting to D0:

  pci_set_power_state(pci_dev, PCI_D0);
  pci_restore_state(pci_dev);

I'm proposing some more patches on top.  None are relevant to the problem
you're solving; they're just minor doc and other updates in the same area.

Rafael, if you have a chance to look at these, I'd appreciate it.  I tried
to make the doc match the code, but I'm no PM expert.

[1] 
https://lore.kernel.org/r/ku1p153mb016637caead346f0aa8e3801bf...@ku1p153mb0166.apcp153.prod.outlook.com


Dexuan Cui (1):
  PCI/PM: Always return devices to D0 when thawing

Bjorn Helgaas (6):
  PCI/PM: Correct pci_pm_thaw_noirq() documentation
  PCI/PM: Clear PCIe PME Status even for legacy power management
  PCI/PM: Run resume fixups before disabling wakeup events
  PCI/PM: Make power management op coding style consistent
  PCI/PM: Wrap long lines in documentation
  PCI/MSI: Move power state check out of pci_msi_supported()

 Documentation/power/pci.rst | 38 +++---
 drivers/pci/msi.c   |  6 +--
 drivers/pci/pci-driver.c| 99 ++---
 3 files changed, 71 insertions(+), 72 deletions(-)

-- 
2.23.0.700.g56cf767bdb-goog

Re: [PATCH v2] PCI: PM: Move to D0 before calling pci_legacy_resume_early()

[Bjorn Helgaas]

On Tue, Oct 08, 2019 at 07:32:27PM +0200, Rafael J. Wysocki wrote:
> On 10/7/2019 8:57 PM, Dexuan Cui wrote:
> > > -Original Message-
> > > From: Bjorn Helgaas 
> > > Sent: Monday, October 7, 2019 6:24 AM
> > > To: Dexuan Cui 
> > > Cc: lorenzo.pieral...@arm.com; linux-...@vger.kernel.org; Michael Kelley
> > > ; linux-hyp...@vger.kernel.org;
> > > linux-ker...@vger.kernel.org; driverdev-devel@linuxdriverproject.org; 
> > > Sasha
> > > Levin ; Haiyang Zhang
> > > ; KY Srinivasan ;
> > > o...@aepfle.de; a...@canonical.com; jasow...@redhat.com; vkuznets
> > > ; marcelo.ce...@canonical.com; Stephen Hemminger
> > > ; ja...@mellanox.com
> > > Subject: Re: [PATCH v2] PCI: PM: Move to D0 before calling
> > > pci_legacy_resume_early()
> > > 
> > > On Wed, Aug 14, 2019 at 01:06:55AM +, Dexuan Cui wrote:
> > > > In pci_legacy_suspend_late(), the device state is moved to PCI_UNKNOWN.
> > > > 
> > > > In pci_pm_thaw_noirq(), the state is supposed to be moved back to 
> > > > PCI_D0,
> > > > but the current code misses the pci_legacy_resume_early() path, so the
> > > > state remains in PCI_UNKNOWN in that path. As a result, in the resume
> > > > phase of hibernation, this causes an error for the Mellanox VF driver,
> > > > which fails to enable MSI-X because pci_msi_supported() is false due
> > > > to dev->current_state != PCI_D0:
> > > > 
> > > > mlx4_core a6d1:00:02.0: Detected virtual function - running in slave 
> > > > mode
> > > > mlx4_core a6d1:00:02.0: Sending reset
> > > > mlx4_core a6d1:00:02.0: Sending vhcr0
> > > > mlx4_core a6d1:00:02.0: HCA minimum page size:512
> > > > mlx4_core a6d1:00:02.0: Timestamping is not supported in slave mode
> > > > mlx4_core a6d1:00:02.0: INTx is not supported in multi-function mode,
> > > aborting
> > > > PM: dpm_run_callback(): pci_pm_thaw+0x0/0xd7 returns -95
> > > > PM: Device a6d1:00:02.0 failed to thaw: error -95
> > > > 
> > > > To be more accurate, the "resume" phase means the "thaw" callbacks which
> > > > run before the system enters hibernation: when the user runs the command
> > > > "echo disk > /sys/power/state" for hibernation, first the kernel 
> > > > "freezes"
> > > > all the devices and creates a hibernation image, then the kernel "thaws"
> > > > the devices including the disk/NIC, writes the memory to the disk, and
> > > > powers down. This patch fixes the error message for the Mellanox VF 
> > > > driver
> > > > in this phase.

Wordsmithing nit: what the patch does is not "fix the error message";
what it does is fix the *problem*, i.e., the fact that we can't
operate the device because we can't enable MSI-X.  The message is only
a symptom.

IIUC the relevant part of the system hibernation sequence is:

  pci_pm_freeze_noirq
  pci_pm_thaw_noirq
  pci_pm_thaw

And the execution flow is:

  pci_pm_freeze_noirq
if (pci_has_legacy_pm_support(pci_dev)) # true for mlx4
  pci_legacy_suspend_late(dev, PMSG_FREEZE)
pci_pm_set_unknown_state
  dev->current_state = PCI_UNKNOWN  # <---
  pci_pm_thaw_noirq
if (pci_has_legacy_pm_support(pci_dev)) # true
  pci_legacy_resume_early(dev)  # noop; mlx4 doesn't implement
  pci_pm_thaw   # returns -95 EOPNOTSUPP
if (pci_has_legacy_pm_support(pci_dev)) # true
  pci_legacy_resume
drv->resume
  mlx4_resume   # mlx4_driver.resume (legacy)
mlx4_load_one
  mlx4_enable_msi_x
pci_enable_msix_range
  __pci_enable_msix_range
__pci_enable_msix
  if (!pci_msi_supported())
if (dev->current_state != PCI_D0)  # <---
  return 0
return -EINVAL
err = -EOPNOTSUPP
"INTx is not supported ..."

(These are just my notes; you don't need to put them all into the
commit message.  I'm just sharing them in case I'm not understanding
correctly.)

> > > > When the system starts again, a fresh kernel starts to run, and when the
> > > > kernel detects that a hibernation image was saved, the kernel "quiesces"
> > > > the devices, and then "restores" the devices from the saved image. In 
> > > > this
> > > > path:
> > > > device_resume_noirq() -> ... ->

Re: [PATCH v2] PCI: PM: Move to D0 before calling pci_legacy_resume_early()

[Bjorn Helgaas]

On Wed, Aug 14, 2019 at 01:06:55AM +, Dexuan Cui wrote:
> 
> In pci_legacy_suspend_late(), the device state is moved to PCI_UNKNOWN.
> 
> In pci_pm_thaw_noirq(), the state is supposed to be moved back to PCI_D0,
> but the current code misses the pci_legacy_resume_early() path, so the
> state remains in PCI_UNKNOWN in that path. As a result, in the resume
> phase of hibernation, this causes an error for the Mellanox VF driver,
> which fails to enable MSI-X because pci_msi_supported() is false due
> to dev->current_state != PCI_D0:
> 
> mlx4_core a6d1:00:02.0: Detected virtual function - running in slave mode
> mlx4_core a6d1:00:02.0: Sending reset
> mlx4_core a6d1:00:02.0: Sending vhcr0
> mlx4_core a6d1:00:02.0: HCA minimum page size:512
> mlx4_core a6d1:00:02.0: Timestamping is not supported in slave mode
> mlx4_core a6d1:00:02.0: INTx is not supported in multi-function mode, aborting
> PM: dpm_run_callback(): pci_pm_thaw+0x0/0xd7 returns -95
> PM: Device a6d1:00:02.0 failed to thaw: error -95
> 
> To be more accurate, the "resume" phase means the "thaw" callbacks which
> run before the system enters hibernation: when the user runs the command
> "echo disk > /sys/power/state" for hibernation, first the kernel "freezes"
> all the devices and creates a hibernation image, then the kernel "thaws"
> the devices including the disk/NIC, writes the memory to the disk, and
> powers down. This patch fixes the error message for the Mellanox VF driver
> in this phase.
> 
> When the system starts again, a fresh kernel starts to run, and when the
> kernel detects that a hibernation image was saved, the kernel "quiesces"
> the devices, and then "restores" the devices from the saved image. In this
> path:
> device_resume_noirq() -> ... ->
>   pci_pm_restore_noirq() ->
> pci_pm_default_resume_early() ->
>   pci_power_up() moves the device states back to PCI_D0. This path is
> not broken and doesn't need my patch.
> 
> Signed-off-by: Dexuan Cui 

This looks like a bugfix for 5839ee7389e8 ("PCI / PM: Force devices to
D0 in pci_pm_thaw_noirq()") so maybe it should be marked for stable as
5839ee7389e8 was?

Rafael, could you confirm?

> ---
> 
> changes in v2:
>   Updated the changelog with more details.
> 
>  drivers/pci/pci-driver.c | 7 ---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index 36dbe960306b..27dfc68db9e7 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -1074,15 +1074,16 @@ static int pci_pm_thaw_noirq(struct device *dev)
>   return error;
>   }
>  
> - if (pci_has_legacy_pm_support(pci_dev))
> - return pci_legacy_resume_early(dev);
> -
>   /*
>* pci_restore_state() requires the device to be in D0 (because of MSI
>* restoration among other things), so force it into D0 in case the
>* driver's "freeze" callbacks put it into a low-power state directly.
>*/
>   pci_set_power_state(pci_dev, PCI_D0);
> +
> + if (pci_has_legacy_pm_support(pci_dev))
> + return pci_legacy_resume_early(dev);
> +
>   pci_restore_state(pci_dev);
>  
>   if (drv && drv->pm && drv->pm->thaw_noirq)
> -- 
> 2.19.1
> 

Re: [PATCH RESEND v3 00/26] Add definition for the number of standard PCI BARs

[Bjorn Helgaas]

On Sat, Sep 28, 2019 at 02:40:26AM +0300, Denis Efremov wrote:
> Code that iterates over all standard PCI BARs typically uses
> PCI_STD_RESOURCE_END, but this is error-prone because it requires
> "i <= PCI_STD_RESOURCE_END" rather than something like
> "i < PCI_STD_NUM_BARS". We could add such a definition and use it the same
> way PCI_SRIOV_NUM_BARS is used. The patchset also replaces constant (6)
> with new define PCI_STD_NUM_BARS where appropriate and removes local
> declarations for the number of PCI BARs.
> 
> Changes in v3:
>   - Updated commits description.
>   - Refactored "< PCI_ROM_RESOURCE" with "< PCI_STD_NUM_BARS" in loops.
>   - Refactored "<= BAR_5" with "< PCI_STD_NUM_BARS" in loops.
>   - Removed local define GASKET_NUM_BARS.
>   - Removed local define PCI_NUM_BAR_RESOURCES.
> 
> Changes in v2:
>   - Reversed checks in pci_iomap_range,pci_iomap_wc_range.
>   - Refactored loops in vfio_pci to keep PCI_STD_RESOURCES.
>   - Added 2 new patches to replace the magic constant with new define.
>   - Splitted net patch in v1 to separate stmmac and dwc-xlgmac patches.
> 
> Denis Efremov (26):
>   PCI: Add define for the number of standard PCI BARs
>   PCI: hv: Use PCI_STD_NUM_BARS
>   PCI: dwc: Use PCI_STD_NUM_BARS
>   PCI: endpoint: Use PCI_STD_NUM_BARS
>   misc: pci_endpoint_test: Use PCI_STD_NUM_BARS
>   s390/pci: Use PCI_STD_NUM_BARS
>   x86/PCI: Loop using PCI_STD_NUM_BARS
>   alpha/PCI: Use PCI_STD_NUM_BARS
>   ia64: Use PCI_STD_NUM_BARS
>   stmmac: pci: Loop using PCI_STD_NUM_BARS
>   net: dwc-xlgmac: Loop using PCI_STD_NUM_BARS
>   ixgb: use PCI_STD_NUM_BARS
>   e1000: Use PCI_STD_NUM_BARS
>   rapidio/tsi721: Loop using PCI_STD_NUM_BARS
>   efifb: Loop using PCI_STD_NUM_BARS
>   fbmem: use PCI_STD_NUM_BARS
>   vfio_pci: Loop using PCI_STD_NUM_BARS
>   scsi: pm80xx: Use PCI_STD_NUM_BARS
>   ata: sata_nv: Use PCI_STD_NUM_BARS
>   staging: gasket: Use PCI_STD_NUM_BARS
>   serial: 8250_pci: Use PCI_STD_NUM_BARS
>   pata_atp867x: Use PCI_STD_NUM_BARS
>   memstick: use PCI_STD_NUM_BARS
>   USB: core: Use PCI_STD_NUM_BARS
>   usb: pci-quirks: Use PCI_STD_NUM_BARS
>   devres: use PCI_STD_NUM_BARS
> 
>  arch/alpha/kernel/pci-sysfs.c |  8 ++---
>  arch/ia64/sn/pci/pcibr/pcibr_dma.c|  4 +--
>  arch/s390/include/asm/pci.h   |  5 +--
>  arch/s390/include/asm/pci_clp.h   |  6 ++--
>  arch/s390/pci/pci.c   | 16 +-
>  arch/s390/pci/pci_clp.c   |  6 ++--
>  arch/x86/pci/common.c |  2 +-
>  arch/x86/pci/intel_mid_pci.c  |  2 +-
>  drivers/ata/pata_atp867x.c|  2 +-
>  drivers/ata/sata_nv.c |  2 +-
>  drivers/memstick/host/jmb38x_ms.c |  2 +-
>  drivers/misc/pci_endpoint_test.c  |  8 ++---
>  drivers/net/ethernet/intel/e1000/e1000.h  |  1 -
>  drivers/net/ethernet/intel/e1000/e1000_main.c |  2 +-
>  drivers/net/ethernet/intel/ixgb/ixgb.h|  1 -
>  drivers/net/ethernet/intel/ixgb/ixgb_main.c   |  2 +-
>  .../net/ethernet/stmicro/stmmac/stmmac_pci.c  |  4 +--
>  .../net/ethernet/synopsys/dwc-xlgmac-pci.c|  2 +-
>  drivers/pci/controller/dwc/pci-dra7xx.c   |  2 +-
>  .../pci/controller/dwc/pci-layerscape-ep.c|  2 +-
>  drivers/pci/controller/dwc/pcie-artpec6.c |  2 +-
>  .../pci/controller/dwc/pcie-designware-plat.c |  2 +-
>  drivers/pci/controller/dwc/pcie-designware.h  |  2 +-
>  drivers/pci/controller/pci-hyperv.c   | 10 +++---
>  drivers/pci/endpoint/functions/pci-epf-test.c | 10 +++---
>  drivers/pci/pci-sysfs.c   |  4 +--
>  drivers/pci/pci.c | 13 
>  drivers/pci/proc.c|  4 +--
>  drivers/pci/quirks.c  |  4 +--
>  drivers/rapidio/devices/tsi721.c  |  2 +-
>  drivers/scsi/pm8001/pm8001_hwi.c  |  2 +-
>  drivers/scsi/pm8001/pm8001_init.c |  2 +-
>  drivers/staging/gasket/gasket_constants.h |  3 --
>  drivers/staging/gasket/gasket_core.c  | 12 +++
>  drivers/staging/gasket/gasket_core.h  |  4 +--
>  drivers/tty/serial/8250/8250_pci.c|  8 ++---
>  drivers/usb/core/hcd-pci.c|  2 +-
>  drivers/usb/host/pci-quirks.c |  2 +-
>  drivers/vfio/pci/vfio_pci.c   | 11 ---
>  drivers/vfio/pci/vfio_pci_config.c| 32 ++-
>  drivers/vfio/pci/vfio_pci_private.h   |  4 +--
>  drivers/video/fbdev/core/fbmem.c  |  4 +--
>  drivers/video/fbdev/efifb.c   |  2 +-
>  include/linux/pci-epc.h   |  2 +-
>  include/linux/pci.h   |  2 +-
>  include/uapi/linux/pci_regs.h |  1 +
>  lib/devres.c  |  2 +-
>  47 files changed, 112 insertions(+), 115 deletions(-)

Applied to pci/resource for v5.5, thanks!

I 

Re: [PATCH] PCI: PM: Also move to D0 before calling pci_legacy_resume_early()

[Bjorn Helgaas]

On Thu, Aug 08, 2019 at 06:46:51PM +, Dexuan Cui wrote:
> 
> In pci_legacy_suspend_late(), the device state is moved to PCI_UNKNOWN.
> In pci_pm_thaw_noirq(), the state is supposed to be moved back to PCI_D0,
> but the current code misses the pci_legacy_resume_early() path, so the
> state remains in PCI_UNKNOWN in that path, and during hiberantion this
> causes an error for the Mellanox VF driver, which fails to enable
> MSI-X: pci_msi_supported() is false due to dev->current_state != PCI_D0:

s/hiberantion/hibernation/

Actually, it sounds more like "during *resume*, this causes an error",
so maybe you want s/hiberantion/resume/ instead?

> mlx4_core a6d1:00:02.0: Detected virtual function - running in slave mode
> mlx4_core a6d1:00:02.0: Sending reset
> mlx4_core a6d1:00:02.0: Sending vhcr0
> mlx4_core a6d1:00:02.0: HCA minimum page size:512
> mlx4_core a6d1:00:02.0: Timestamping is not supported in slave mode
> mlx4_core a6d1:00:02.0: INTx is not supported in multi-function mode, aborting
> PM: dpm_run_callback(): pci_pm_thaw+0x0/0xd7 returns -95
> PM: Device a6d1:00:02.0 failed to thaw: error -95
> 
> Signed-off-by: Dexuan Cui 
> ---
>  drivers/pci/pci-driver.c | 7 ---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index 36dbe960306b..27dfc68db9e7 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -1074,15 +1074,16 @@ static int pci_pm_thaw_noirq(struct device *dev)
>   return error;
>   }
>  
> - if (pci_has_legacy_pm_support(pci_dev))
> - return pci_legacy_resume_early(dev);
> -
>   /*
>* pci_restore_state() requires the device to be in D0 (because of MSI
>* restoration among other things), so force it into D0 in case the
>* driver's "freeze" callbacks put it into a low-power state directly.
>*/
>   pci_set_power_state(pci_dev, PCI_D0);
> +
> + if (pci_has_legacy_pm_support(pci_dev))
> + return pci_legacy_resume_early(dev);
> +
>   pci_restore_state(pci_dev);
>  
>   if (drv && drv->pm && drv->pm->thaw_noirq)
> -- 
> 2.19.1
> 

Re: [PATCH v2] PCI: hv: Fix panic by calling hv_pci_remove_slots() earlier

[Bjorn Helgaas]

Thanks for updating this.  But you didn't update the subject line,
which is really still a little too low-level.  Maybe Lorenzo will fix
this.  Something like this, maybe?

  PCI: hv: Avoid use of hv_pci_dev->pci_slot after freeing it

On Fri, Aug 02, 2019 at 10:50:20PM +, Dexuan Cui wrote:
> 
> The slot must be removed before the pci_dev is removed, otherwise a panic
> can happen due to use-after-free.
> 
> Fixes: 15becc2b56c6 ("PCI: hv: Add hv_pci_remove_slots() when we unload the 
> driver")
> Signed-off-by: Dexuan Cui 
> Cc: sta...@vger.kernel.org
> ---
> 
> Changes in v2:
>   Improved the changelog accordign to the discussion with Bjorn Helgaas:
> https://lkml.org/lkml/2019/8/1/1173
> https://lkml.org/lkml/2019/8/2/1559
> 
>  drivers/pci/controller/pci-hyperv.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/pci-hyperv.c 
> b/drivers/pci/controller/pci-hyperv.c
> index 6b9cc6e60a..68c611d 100644
> --- a/drivers/pci/controller/pci-hyperv.c
> +++ b/drivers/pci/controller/pci-hyperv.c
> @@ -2757,8 +2757,8 @@ static int hv_pci_remove(struct hv_device *hdev)
>   /* Remove the bus from PCI's point of view. */
>   pci_lock_rescan_remove();
>   pci_stop_root_bus(hbus->pci_bus);
> - pci_remove_root_bus(hbus->pci_bus);
>   hv_pci_remove_slots(hbus);
> + pci_remove_root_bus(hbus->pci_bus);
>   pci_unlock_rescan_remove();
>   hbus->state = hv_pcibus_removed;
>   }
> -- 
> 1.8.3.1
> 

Re: [PATCH] PCI: hv: Fix panic by calling hv_pci_remove_slots() earlier

[Bjorn Helgaas]

On Fri, Aug 02, 2019 at 08:31:26PM +, Dexuan Cui wrote:
> > From: Bjorn Helgaas 
> > Sent: Friday, August 2, 2019 12:41 PM
> > The subject line only describes the mechanical code change, which is
> > obvious from the patch.  It would be better if we could say something
> > about *why* we need this.
> 
> Hi Bjorn,
> Sorry. I'll try to write a better changelog in v2. :-)
>  
> > On Fri, Aug 02, 2019 at 01:32:28AM +, Dexuan Cui wrote:
> > >
> > > When a slot is removed, the pci_dev must still exist.
> > >
> > > pci_remove_root_bus() removes and free all the pci_devs, so
> > > hv_pci_remove_slots() must be called before pci_remove_root_bus(),
> > > otherwise a general protection fault can happen, if the kernel is built
> > 
> > "general protection fault" is an x86 term that doesn't really say what
> > the issue is.  I suspect this would be a "use-after-free" problem.
> 
> Yes, it's use-after-free. I'll fix the the wording.
>  
> > > --- a/drivers/pci/controller/pci-hyperv.c
> > > +++ b/drivers/pci/controller/pci-hyperv.c
> > > @@ -2757,8 +2757,8 @@ static int hv_pci_remove(struct hv_device *hdev)
> > >   /* Remove the bus from PCI's point of view. */
> > >   pci_lock_rescan_remove();
> > >   pci_stop_root_bus(hbus->pci_bus);
> > > - pci_remove_root_bus(hbus->pci_bus);
> > >   hv_pci_remove_slots(hbus);
> > > + pci_remove_root_bus(hbus->pci_bus);
> > 
> > I'm curious about why we need hv_pci_remove_slots() at all.  None of
> > the other callers of pci_stop_root_bus() and pci_remove_root_bus() do
> > anything similar to hv_pci_remove_slots().
> > 
> > Surely some of those callers also support slots, so there must be some
> > other path that calls pci_destroy_slot() in those cases.  Can we use a
> > similar strategy here?
> 
> Originally Stephen Heminger added the slot code for pci-hyperv.c:
> a15f2c08c708 ("PCI: hv: support reporting serial number as slot information")
> So he may know this better. My understanding is: we can not use the similar
> stragegy used in the 2 other users of pci_create_slot():
> 
> drivers/pci/hotplug/pci_hotplug_core.c calls pci_create_slot().
> It looks drivers/pci/hotplug/ is quite different from pci-hyperv.c because
> pci-hyper-v uses a simple *private* hot-plug protocol, making it impossible
> to use the API pci_hp_register() and pci_hp_destroy() -> pci_destroy_slot().
> 
> drivers/acpi/pci_slot.c calls pci_create_slot(), and saves the created slots 
> in
> the static "slot_list" list in the same file. Again, since pci-hyper-v uses a 
> private
> PCI-device-discovery protocol (which is based on VMBus rather the emulated
> ACPI and PCI), acpi_pci_slot_enumerate() can not find the PCI devices that are
> discovered by pci-hyperv, so we can not use the standard register_slot() ->
> pci_create_slot() to create the slots and hence acpi_pci_slot_remove() -> 
> pci_destroy_slot() can not work for pci-hyperv.

Hmm, ok.  This still doesn't seem right to me, but I think the bottom
line will be that the current slot registration interfaces just don't
work quite right for all the cases we want them to.

Maybe it would be a good project for somebody to rethink them, but it
doesn't seem practical for *this* patch.  Thanks for looking into it
this far!

> I think I can use this as the v2 changelog:
> 
> The slot must be removed before the pci_dev is removed, otherwise a panic
> can happen due to use-after-free.

Sounds good.

Bjorn

Re: [PATCH] PCI: hv: Fix panic by calling hv_pci_remove_slots() earlier

[Bjorn Helgaas]

Hi Dexuan,

The subject line only describes the mechanical code change, which is
obvious from the patch.  It would be better if we could say something
about *why* we need this.

On Fri, Aug 02, 2019 at 01:32:28AM +, Dexuan Cui wrote:
> 
> When a slot is removed, the pci_dev must still exist.
> 
> pci_remove_root_bus() removes and free all the pci_devs, so
> hv_pci_remove_slots() must be called before pci_remove_root_bus(),
> otherwise a general protection fault can happen, if the kernel is built

"general protection fault" is an x86 term that doesn't really say what
the issue is.  I suspect this would be a "use-after-free" problem.

> with the memory debugging options.
> 
> Fixes: 15becc2b56c6 ("PCI: hv: Add hv_pci_remove_slots() when we unload the 
> driver")
> Signed-off-by: Dexuan Cui 
> Cc: sta...@vger.kernel.org
> 
> ---
> 
> When pci-hyperv is unloaded, this panic can happen:
> 
>  general protection fault:
>  CPU: 2 PID: 1091 Comm: rmmod Not tainted 5.2.0+
>  RIP: 0010:pci_slot_release+0x30/0xd0
>  Call Trace:
>   kobject_release+0x65/0x190
>   pci_destroy_slot+0x25/0x60
>   hv_pci_remove+0xec/0x110 [pci_hyperv]
>   vmbus_remove+0x20/0x30 [hv_vmbus]
>   device_release_driver_internal+0xd5/0x1b0
>   driver_detach+0x44/0x7c
>   bus_remove_driver+0x75/0xc7
>   vmbus_driver_unregister+0x50/0xbd [hv_vmbus]
>   __x64_sys_delete_module+0x136/0x200
>   do_syscall_64+0x5e/0x220
> 
>  drivers/pci/controller/pci-hyperv.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/pci-hyperv.c 
> b/drivers/pci/controller/pci-hyperv.c
> index 6b9cc6e60a..68c611d 100644
> --- a/drivers/pci/controller/pci-hyperv.c
> +++ b/drivers/pci/controller/pci-hyperv.c
> @@ -2757,8 +2757,8 @@ static int hv_pci_remove(struct hv_device *hdev)
>   /* Remove the bus from PCI's point of view. */
>   pci_lock_rescan_remove();
>   pci_stop_root_bus(hbus->pci_bus);
> - pci_remove_root_bus(hbus->pci_bus);
>   hv_pci_remove_slots(hbus);
> + pci_remove_root_bus(hbus->pci_bus);

I'm curious about why we need hv_pci_remove_slots() at all.  None of
the other callers of pci_stop_root_bus() and pci_remove_root_bus() do
anything similar to hv_pci_remove_slots().

Surely some of those callers also support slots, so there must be some
other path that calls pci_destroy_slot() in those cases.  Can we use a
similar strategy here?

>   pci_unlock_rescan_remove();
>   hbus->state = hv_pcibus_removed;
>   }
> -- 
> 1.8.3.1
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 3/3] PCI: hv: Use vPCI protocol version 1.2 for v4.9

[Bjorn Helgaas]

On Fri, Jan 18, 2019 at 02:17:18AM +0530, Ajay Kaher wrote:
> Update the Hyper-V vPCI driver to use the Server-2016 version of the vPCI
> protocol, fixing MSI creation and retargeting issues.
> 
> Replaced hv_tmp_cpu_nr_to_vp_nr() with vmbus_cpu_number_to_vp_number()
> to make this patch compatibale for linux v4.9.

s/compatibale for/compatible with/

This change (to make it compatible with v4.9) sounds like it should be
in its own separate patch.  I don't see any use of
hv_tmp_cpu_nr_to_vp_nr() being removed, so the changelog doesn't quite
make sense.

> Signed-off-by: Jork Loeser 
> Signed-off-by: Bjorn Helgaas 

I did not sign off on this; please remove.

> Reviewed-by: K. Y. Srinivasan 
> Acked-by: K. Y. Srinivasan 
> Signed-off-by: Ajay Kaher 

> + * struct hv_msi_desc2 - 1.2 version of hv_msi_desc
> + * @vector:  IDT entry
> + * @delivery_mode:   As defined in Intel's Programmer's
> + *   Reference Manual, Volume 3, Chapter 8.
> + * @vector_count:Number of contiguous entries in the
> + *   Interrupt Descriptor Table that are
> + *   occupied by this Message-Signaled
> + *   Interrupt. For "MSI", as first defined
> + *   in PCI 2.2, this can be between 1 and
> + *   32. For "MSI-X," as first defined in PCI
> + *   3.0, this must be 1, as each MSI-X table
> + *   entry would have its own descriptor.

Please reflow these descriptions to take advantage of an 80 column
width.  They are currently wrapped to fit in 50 columns, which is
unnecessarily short.

> + default:
> + /* As we only negotiate protocol versions known to this driver,
> +  * this path should never hit. However, this is it not a hot
> +  * path so we print a message to aid future updates.
> +  */
> + dev_err(>hdev->device,
> + "Unexpected vPCI protocol, update driver.");

Include the actual protocol version in the message?
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/3] PCI: hv: Add vPCI version protocol negotiation

[Bjorn Helgaas]

On Fri, Jan 18, 2019 at 02:17:17AM +0530, Ajay Kaher wrote:
> Hyper-V vPCI offers different protocol versions.  Add the infra for
> negotiating the one to use.
> 
> Signed-off-by: Jork Loeser 
> Signed-off-by: Bjorn Helgaas 

I did not sign off on this, please remove.

> Reviewed-by: K. Y. Srinivasan 
> Acked-by: K. Y. Srinivasan 
> Signed-off-by: Ajay Kaher 
> ---
>  drivers/pci/host/pci-hyperv.c | 72 
> +++
>  1 file changed, 53 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
> index 9e44adf..6eed85b 100644
> --- a/drivers/pci/host/pci-hyperv.c
> +++ b/drivers/pci/host/pci-hyperv.c
> @@ -65,22 +65,37 @@
>   * major version.
>   */
>  
> -#define PCI_MAKE_VERSION(major, minor) ((u32)(((major) << 16) | (major)))
> +#define PCI_MAKE_VERSION(major, minor) ((u32)(((major) << 16) | (minor)))

This looks like a bug fix that should be in its own patch.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/3] PCI: hv: Allocate physically contiguous hypercall params buffer

[Bjorn Helgaas]

On Fri, Jan 18, 2019 at 02:17:16AM +0530, Ajay Kaher wrote:
> hv_do_hypercall() assumes that we pass a segment from a physically
> contiguous buffer.  A buffer allocated on the stack may not work if
> CONFIG_VMAP_STACK=y is set.
> 
> Use kmalloc() to allocate this buffer.
> 
> Reported-by: Haiyang Zhang 
> Signed-off-by: Long Li 
> Signed-off-by: Bjorn Helgaas 

I did not sign off on this; please remove this.  Signed-off-by should
only be added by the person mentioned.

> Acked-by: K. Y. Srinivasan 
> Signed-off-by: Ajay Kaher 
> ---
>  drivers/pci/host/pci-hyperv.c | 29 +++--
>  1 file changed, 19 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
> index b4d8ccf..9e44adf 100644
> --- a/drivers/pci/host/pci-hyperv.c
> +++ b/drivers/pci/host/pci-hyperv.c
> @@ -383,6 +383,8 @@ struct hv_pcibus_device {
>   struct msi_domain_info msi_info;
>   struct msi_controller msi_chip;
>   struct irq_domain *irq_domain;
> + struct retarget_msi_interrupt retarget_msi_interrupt_params;
> + spinlock_t retarget_msi_interrupt_lock;
>  };
>  
>  /*
> @@ -780,34 +782,40 @@ void hv_irq_unmask(struct irq_data *data)
>  {
>   struct msi_desc *msi_desc = irq_data_get_msi_desc(data);
>   struct irq_cfg *cfg = irqd_cfg(data);
> - struct retarget_msi_interrupt params;
> + struct retarget_msi_interrupt *params;
>   struct hv_pcibus_device *hbus;
>   struct cpumask *dest;
>   struct pci_bus *pbus;
>   struct pci_dev *pdev;
>   int cpu;
> + unsigned long flags;
>  
>   dest = irq_data_get_affinity_mask(data);
>   pdev = msi_desc_to_pci_dev(msi_desc);
>   pbus = pdev->bus;
>   hbus = container_of(pbus->sysdata, struct hv_pcibus_device, sysdata);
>  
> - memset(, 0, sizeof(params));
> - params.partition_id = HV_PARTITION_ID_SELF;
> - params.source = 1; /* MSI(-X) */
> - params.address = msi_desc->msg.address_lo;
> - params.data = msi_desc->msg.data;
> - params.device_id = (hbus->hdev->dev_instance.b[5] << 24) |
> + spin_lock_irqsave(>retarget_msi_interrupt_lock, flags);
> +
> + params = >retarget_msi_interrupt_params;
> + memset(params, 0, sizeof(*params));
> + params->partition_id = HV_PARTITION_ID_SELF;
> + params->source = 1; /* MSI(-X) */
> + params->address = msi_desc->msg.address_lo;
> + params->data = msi_desc->msg.data;
> + params->device_id = (hbus->hdev->dev_instance.b[5] << 24) |
>  (hbus->hdev->dev_instance.b[4] << 16) |
>  (hbus->hdev->dev_instance.b[7] << 8) |
>  (hbus->hdev->dev_instance.b[6] & 0xf8) |
>  PCI_FUNC(pdev->devfn);
> - params.vector = cfg->vector;
> + params->vector = cfg->vector;
>  
>   for_each_cpu_and(cpu, dest, cpu_online_mask)
> - params.vp_mask |= (1ULL << vmbus_cpu_number_to_vp_number(cpu));
> + params->vp_mask |= (1ULL << vmbus_cpu_number_to_vp_number(cpu));
> +
> + hv_do_hypercall(HVCALL_RETARGET_INTERRUPT, params, NULL);
>  
> - hv_do_hypercall(HVCALL_RETARGET_INTERRUPT, , NULL);
> + spin_unlock_irqrestore(>retarget_msi_interrupt_lock, flags);
>  
>   pci_msi_unmask_irq(data);
>  }
> @@ -2212,6 +2220,7 @@ static int hv_pci_probe(struct hv_device *hdev,
>   INIT_LIST_HEAD(>resources_for_children);
>   spin_lock_init(>config_lock);
>   spin_lock_init(>device_list_lock);
> + spin_lock_init(>retarget_msi_interrupt_lock);
>   sema_init(>enum_sem, 1);
>   init_completion(>remove_event);
>  
> -- 
> 2.7.4
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] PCI: hv: Fix a __local_bh_enable_ip warning in hv_compose_msi_msg()

[Bjorn Helgaas]

On Wed, Jun 13, 2018 at 08:32:13PM +, Dexuan Cui wrote:
> > From: Dexuan Cui
> > Sent: Wednesday, June 6, 2018 17:15
> > To: Haiyang Zhang ; Lorenzo Pieralisi
> > ; Bjorn Helgaas ;
> > linux-...@vger.kernel.org; KY Srinivasan ; Stephen
> > Hemminger ; o...@aepfle.de;
> > a...@canonical.com; jasow...@redhat.com
> > Cc: linux-ker...@vger.kernel.org; driverdev-devel@linuxdriverproject.org;
> > vkuzn...@redhat.com; marcelo.ce...@canonical.com
> > Subject: RE: [PATCH] PCI: hv: Fix a __local_bh_enable_ip warning in
> > hv_compose_msi_msg()
> > 
> > > From: Haiyang Zhang
> > > Sent: Friday, May 25, 2018 12:52
> > > To: Dexuan Cui ; Lorenzo Pieralisi
> > > ; Bjorn Helgaas ;
> > > linux-...@vger.kernel.org; KY Srinivasan ; Stephen
> > > Hemminger ; o...@aepfle.de;
> > > a...@canonical.com; jasow...@redhat.com
> > > Cc: linux-ker...@vger.kernel.org; driverdev-devel@linuxdriverproject.org;
> > > vkuzn...@redhat.com; marcelo.ce...@canonical.com
> > > Subject: RE: [PATCH] PCI: hv: Fix a __local_bh_enable_ip warning in
> > > hv_compose_msi_msg()
> > >
> > > > From: Dexuan Cui
> > > > Sent: Tuesday, May 22, 2018 8:18 PM
> > > > To: Lorenzo Pieralisi ; Bjorn Helgaas
> > > > ; linux-...@vger.kernel.org; KY Srinivasan
> > > > ; Stephen Hemminger ;
> > > > o...@aepfle.de; a...@canonical.com; jasow...@redhat.com
> > > > Cc: linux-ker...@vger.kernel.org; 
> > > > driverdev-devel@linuxdriverproject.org;
> > > > Haiyang Zhang ; vkuzn...@redhat.com;
> > > > marcelo.ce...@canonical.com
> > > > Subject: [PATCH] PCI: hv: Fix a __local_bh_enable_ip warning in
> > > > hv_compose_msi_msg()
> > > >
> > > >
> > > > Commit de0aa7b2f97d ("PCI: hv: Fix 2 hang issues in
> > > hv_compose_msi_msg()")
> > > > uses local_bh_disable()/enable(), because hv_pci_onchannelcallback() can
> > > also
> > > > run in tasklet context as the channel event callback.
> > > >
> > > > With CONFIG_PROVE_LOCKING=y in the latest mainline, or old kernels that
> > > > don't have commit f71b74bca637 ("irq/softirqs: Use lockdep to assert 
> > > > IRQs
> > > are
> > > > disabled/enabled"), it turns out can we trigger a warning at the 
> > > > beginning
> > of
> > > > __local_bh_enable_ip(), because the upper layer irq code can call
> > > > hv_compose_msi_msg() with local irqs disabled.
> > > >
> > > > Let's fix the warning by switching to local_irq_save()/restore(). This 
> > > > is not an
> > > > issue because hv_pci_onchannelcallback() is not slow, and it not a hot 
> > > > path.
> > > >
> > > > Fixes: de0aa7b2f97d ("PCI: hv: Fix 2 hang issues in
> > hv_compose_msi_msg()")
> > > > Signed-off-by: Dexuan Cui 
> > > > Cc: 
> > > > Cc: Stephen Hemminger 
> > > > Cc: K. Y. Srinivasan 
> > > > ---
> > >
> > > Reviewed-by: Haiyang Zhang 
> > >
> > > Thanks you.
> > 
> > Hi Lorenzo,
> > 
> > Can I have your reply to this patch?
> > 
> > -- Dexuan
> 
> It looks Lorenzo's pci.git tree has not been updated for 3+ weeks.
> I guess Lorenzo may be on vacation. 
> 
> @Bjorn, can this patch go through your tree?
> Should I resubmit it?

No need to resubmit it, Lorenzo has been out for a bit, but I'm sure
he'll pick this up as he catches up.

You might, however, fix the commit log:

  This is not an issue because hv_pci_onchannelcallback() is not slow,
  and it not a hot path.

This has at least one typo (I think you mean "and *is* not a hot
path").

I also don't understand the sentence as a whole because the
hv_pci_onchannelcallback() comment says it's called whenever the host
sends a packet to this channel, and that *does* sound like a hot path.

I also don't understand the "hv_pci_onchannelcallback() is not slow"
part.  In other words, you're saying hv_pci_onchannelcallback() is
fast and it's not a hot path.  And apparently this has something to do
with the difference between local_bh_disable() and local_irq_save()?

Bjorn
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v7] Revert "PCI: hv: Use device serial number as PCI domain"

[Bjorn Helgaas]

On Thu, Apr 12, 2018 at 10:17:42AM +0100, Lorenzo Pieralisi wrote:
> On Thu, Apr 12, 2018 at 02:44:42AM +, Sridhar Pitchai wrote:
> > When Linux runs as a guest VM in Hyper-V and Hyper-V adds the virtual PCI
> > bus to the guest, Hyper-V always provides unique PCI domain.
> > 
> > commit 4a9b0933bdfc ("PCI: hv: Use device serial number as PCI domain")
> > overrode unique domain with the serial number of the first device added to
> > the virtual PCI bus.
> > 
> > The reason for that patch was to have a consistent and short name for the
> > device, but Hyper-V doesn't provide unique serial numbers. Using non-unique
> > serial numbers as domain IDs leads to duplicate device addresses, which
> > causes PCI bus registration to fail.
> > 
> > Revert commit 4a9b0933bdfc ("PCI: hv: Use device serial number as PCI
> > domain") so we can reliably support multiple devices being assigned to
> > a guest.
> > 
> > Fixes: 4a9b0933bdfc ("PCI: hv: Use device serial number as PCI domain")
> > Signed-off-by: Sridhar Pitchai 
> > Cc: sta...@vger.kernel.org
> 
> I am still not happy with this patch.
> 
> -  You do not explain at all the dependency on commit 0c195567a8f6 and
>you should because that's fundamental, if that patch is not present
>this revert breaks the kernel as per previous discussions[1].
> -  You are sending this patch to all stable kernels that contain the
>commit you are fixing - some that may not contain the commit above
>(that was merged in v4.14), you are breaking those kernels, if not
>explain me why please

If there's a dependency on 0c195567a8f6, I totally agree that
needs to be cleared up.  I was assuming that turned out to be
irrelevant.

> [1] https://marc.info/?l=linux-pci=152158684221212=2
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v6] PCI: hv: Make sure the bus domain is really unique

[Bjorn Helgaas]

I would make the subject:

  Revert "PCI: hv: Use device serial number as PCI domain"

so it matches the original commit.  Otherwise you really need quite a
lot of driver internal knowledge to connect them.

On Wed, Apr 11, 2018 at 12:36:25AM +, Sridhar Pitchai wrote:
> When Linux runs as a guest VM in Hyper-V and Hyper-V adds the virtual
> PCI bus to the guest, Hyper-V always provides unique PCI domain.
> 
> commit 4a9b0933bdfc ("PCI: hv: Use device serial number as PCI domain")
> overrode unique domain with the serial number of the first device added
> to the virtual PCI bus. The reason for that patch is to have a consistent
> and short name for the device. But commit 4a9b0933bdfc ("PCI: hv: Use
> device serial number as PCI domain") will not guarantee unique domain id.

Possible alternate text:

  The reason for that patch was to have a consistent and short name
  for the device, but Hyper-V doesn't provide unique serial numbers
  (in spite of the code comment claiming that it does).  Using
  non-unique serial numbers as domain IDs leads to duplicate device
  addresses, which causes PCI bus registration to fail.
  
> For example, if the serial number of the device is 0 and there exists a
> PCI bus with domain 0 already, this will cause the PCI bus registration
> with kernel fails.
> 
> commit 4a9b0933bdfc ("PCI: hv: Use device serial number as PCI
> domain") need to be reverted.

I think the above sentence is redundant and could be removed.

> Revert commit 4a9b0933bdfc ("PCI: hv: Use device serial number as PCI
> domain") so we can reliably support multiple devices being assigned to
> a guest.
> 
> Fixes: 4a9b0933bdfc ("PCI: hv: Use device serial number as PCI domain")
> Signed-off-by: Sridhar Pitchai <sridhar.pitc...@microsoft.com>
> Cc: sta...@vger.kernel.org

Regardless of what you do with the feedback above,

Reviewed-by: Bjorn Helgaas <bhelg...@google.com>

Thanks a lot for persisting with this!

> ---
> Changes in v6:
> * fix the commit comment. [Lorenzo Pieralisi, Bjorn Helgaas]
> ---
>  drivers/pci/host/pci-hyperv.c | 11 ---
>  1 file changed, 11 deletions(-)
> 
> diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
> index 2faf38eab785..ac67e56e451a 100644
> --- a/drivers/pci/host/pci-hyperv.c
> +++ b/drivers/pci/host/pci-hyperv.c
> @@ -1518,17 +1518,6 @@ static struct hv_pci_dev *new_pcichild_device(struct 
> hv_pcibus_device *hbus,
>   get_pcichild(hpdev, hv_pcidev_ref_childlist);
>   spin_lock_irqsave(>device_list_lock, flags);
>  
> - /*
> -  * When a device is being added to the bus, we set the PCI domain
> -  * number to be the device serial number, which is non-zero and
> -  * unique on the same VM.  The serial numbers start with 1, and
> -  * increase by 1 for each device.  So device names including this
> -  * can have shorter names than based on the bus instance UUID.
> -  * Only the first device serial number is used for domain, so the
> -  * domain number will not change after the first device is added.
> -  */
> - if (list_empty(>children))
> - hbus->sysdata.domain = desc->ser;
>   list_add_tail(>list_entry, >children);
>   spin_unlock_irqrestore(>device_list_lock, flags);
>   return hpdev;
> -- 
> 2.14.1
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v5] PCI: hv: Make sure the bus domain is really unique

[Bjorn Helgaas]

On Fri, Mar 30, 2018 at 07:35:04PM +, Sridhar Pitchai wrote:
>   >When Linux runs as a guest VM in Hyper-V and Hyper-V adds the virtual
>   >PCI bus to the guest, Hyper-V always provides unique PCI domain.
>   >
>   >commit 4a9b0933bdfc ("PCI: hv: Use device serial number as PCI domain")
>   >overrode unique domain with the serial number of the first device added
>   >to the virtual PCI bus. The reason for that patch is to have a consistent
>   >and short name for the device. But commit 4a9b0933bdfc ("PCI: hv: Use
>   >device serial number as PCI domain") will not guarantee unique domain id.
>   >For example, if the serial number of the device is 0 and there exists a
>   >PCI bus with domain 0 already, this will cause the PCI bus registration
>   >with kernel fails.
>   >
>   >commit 4a9b0933bdfc ("PCI: hv: Use device serial number as PCI
>   >domain") need to be reverted. Also, we no longer need it as commit
>   >4a1626dd233e ("netvsc: transparent VF management") remove the
>   >need for it.

Do you mean 0c195567a8f6 ("netvsc: transparent VF management")?

I don't see a 4a1626dd233e commit in Linus' tree.

The connection between 0c195567a8f6 ("netvsc: transparent VF
management") and this Hyper-V domain ID issue is not clear to me at
all.  But obviously I'm not a networking or a Hyper-V person.

A hint in the changelog about what that connection is would be nice.

>   >Revert commit 4a9b0933bdfc ("PCI: hv: Use device serial number as PCI
>   >domain") so we can reliably support multiple devices being assigned to
>   >a guest.
>   >
>   >This revert should only be backported to kernels that contain commit
>   >4a1626dd233e ("netvsc: transparent VF management").
>   >
>   >Fixes: 4a9b0933bdfc ("PCI: hv: Use device serial number as PCI domain")
>   >Signed-off-by: Sridhar Pitchai <sridhar.pitc...@microsoft.com>
>   >Cc: sta...@vger.kernel.org

This backporting guidance is very confusing.

4a9b0933bdfc ("PCI: hv: Use device serial number as PCI domain")
appeared in v4.11.

0c195567a8f6 ("netvsc: transparent VF management") appeared in v4.14.

The changelog and the "Fixes" tag both suggest that 4a9b0933bdfc was
flat-out buggy and should be reverted unconditionally.  That would
mean this patch should be applied to v4.11 and later stable kernels.

But the changelog also says "only revert 4a9b0933bdfc if you have
0c195567a8f6".  That would mean this patch should only be applied to
v4.14 and later stable kernels.

Which is it?  You can answer in this thread if you want, but Lorenzo
will ultimately be looking for a v6 patch with a corrected changelog.

>   >---
>   >Changes in v5:
>   >* fix the commit comment. [Lorenzo Pieralisi, Bjorn Helgaas]
>   >* fixed the patch white space.
>   >---
>   > drivers/pci/host/pci-hyperv.c | 11 ---
>   > 1 file changed, 11 deletions(-)
>   >
>   >diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
>   >index 2faf38eab785..ac67e56e451a 100644
>   >--- a/drivers/pci/host/pci-hyperv.c
>   >+++ b/drivers/pci/host/pci-hyperv.c
>   >@@ -1518,17 +1518,6 @@ static struct hv_pci_dev 
> *new_pcichild_device(struct hv_pcibus_device *hbus,
>   >   get_pcichild(hpdev, hv_pcidev_ref_childlist);
>   >   spin_lock_irqsave(>device_list_lock, flags);
>   > 
>   >-  /*
>   >-   * When a device is being added to the bus, we set the PCI domain
>   >-   * number to be the device serial number, which is non-zero and
>   >-   * unique on the same VM.  The serial numbers start with 1, and
>   >-   * increase by 1 for each device.  So device names including this
>   >-   * can have shorter names than based on the bus instance UUID.
>   >-   * Only the first device serial number is used for domain, so the
>   >-   * domain number will not change after the first device is added.
>   >-   */
>   >-  if (list_empty(>children))
>   >-  hbus->sysdata.domain = desc->ser;
>   >   list_add_tail(>list_entry, >children);
>   >   spin_unlock_irqrestore(>device_list_lock, flags);
>   >   return hpdev;
>   >-- 
> >2.14.1
> 
> Hi Lorenzo,
> Did you get a chance to look at the patch v5. Please let me know if
> anything I need to address further.

I think Lorenzo is out this week, but I suspect he'll look at it next
week.

Bjorn
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3]PCI: hv: fix PCI-BUS domainID corruption

[Bjorn Helgaas]

[I composed most of this before seeing Lorenzo's response, so sorry
about the duplication.  Maybe seeing things stated a different way
will help :)]

On Tue, Mar 20, 2018 at 11:00:36PM +, Sridhar Pitchai wrote:
> Hi Lorenzo,
>Transparent SRIOV is exposing the NIC directly to the kernel via
> para-virtual device, unlike creating a netdev and associating it with the bond
> driver. Further descriptions here,
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=0c195567a8f6e82ea5535cd9f1d54a1626dd233e
> 
> Previously, when using the bond driver, unique and persistent VF NIC name
> was required, so we used serial number as PCI domain which is included as
> part of the VF NIC name.  Transparent SRIOV mode puts VF NIC based on MAC
> match as a slave of synthetic NIC, so VF NIC’s name is no longer important.

Hi Sridhar,

A few hints about submitting patches more efficiently:

1) You never have to ask "Are we OK with the explanation?  If so, I'll
send a patch with updated changelog."  That forces an extra
round-trip.  Simply post a new version with your proposed update.  If
Lorenzo has more questions, he'll say so and you can do another
version.

2) When Lorenzo is asking for clarification, he's not really asking
for the clarification in an email response, because the email thread
will soon be forgotten and lost in the archives.  What we really want
is for the permanent git changelog to make sense to someone in the
future.  The easiest way is to post a new patch version with a
revised changelog that answers the questions.

3) Please capitalize and punctuate consistently with previous history,
e.g., "PCI: hv: Fix domain ID corruption" for your title, "SR-IOV"
(not "SRIOV") and "bus" (not "BUS") in changelog.  Both "para-virtual"
and "paravirtual" are used in the kernel, but "paravirtual" is much
more common.  Run "git log" and "git log --oneline" on your file and
follow the same style.

4) When you reference a previous commit, please use this style:
0c195567a8f6 ("netvsc: transparent VF management"), i.e., 12-char SHA1
followed by title.  You seem to have removed some spaces from the
commit you mention in the "Fixes" tag.

And a few content questions/observations of my own:

1) "Fix domain ID corruption" isn't a very good title because it
suggests you're fixing a memory corruption or similar defect.  But in
fact, I think you're removing something that used to be a feature
(added by 4a9b0933bdfc ("PCI: hv: Use device serial number as PCI
domain")) but is now no longer needed and in fact now causes a
problem.

2) Your changelog does mention 4a9b0933bdfc, which is good, but
there must be some other  that makes it safe to remove
4a9b0933bdfc, i.e.,  removes the need for using the device
serial number as the PCI domain.   *must* be mentioned in
the changelog.  Otherwise, people may backport this patch to a kernel
that doesn't include , and things will break.

3) I don't understand what you mean by "transparent SR-IOV mode".  Is
that something different than regular SR-IOV?  If so, what exactly is
the difference?  I don't think the PCIe specs mention a "transparent
mode", so is it a Hyper-V thing?  It seems important, but I don't see
any pci-hyperv.c commits that mention it.

Here's a stab at the sort of changelog I would be looking for.
Obviously I don't understand much about Hyper-V and pci-hyperv.c, so
please correct the things I got wrong:

  When Linux runs as a guest in a Hyper-V VM, pci-hyperv.c
  paravirtualizes access to PCI devices assigned to the guest.  For
  each of those devices, hv_pci_probe() creates a virtual PCI bus in
  its own unique PCI domain.

  4a9b0933bdfc ("PCI: hv: Use device serial number as PCI domain")
  overrode that unique PCI domain to be the Hyper-V device serial
  number to make device names more convenient .

  One problem with 4a9b0933bdfc is that the Hyper-V device serial
  number is not necessarily unique, so we may end up with two buses
  with the same domain and bus number, and adding the second bus
  fails.

  We no longer need to override the PCI domain numbers because  removed the need for that.

  Revert 4a9b0933bdfc ("PCI: hv: Use device serial number as PCI
  domain") so we can reliably support multiple devices being assigned
  to a guest.

  This revert should only be backported to kernels that contain
  .

Bjorn
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] PCI BUS domain ID corruption

[Bjorn Helgaas]

[+cc Lorenzo]

On Thu, Mar 8, 2018 at 12:55 PM, Sridhar Pitchai
 wrote:
> Hi,
>
> I would like to submit the following patch. This patch addresses the issue
> when we try to add a VMBUS, the domain ID for the PCI bus is overwritten.

Hi Sridhar,

Thanks for the patch!

This area is maintained by Lorenzo now (cc'd), so please copy him on
the next round.

We can't apply it as-is because it contains no Signed-off-by (see
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst).

This email is also too fancy (HTML, etc) and I think will be rejected
by the mailing lists (see
http://vger.kernel.org/majordomo-info.html#taboo).  What you want is a
plain-text email.

Please also run "git log drivers/pci/host/pci-hyperv.c" and make your
subject and changelog match the format and style of previous patches.

It looks like this basically reverts 4a9b0933bdfc ("PCI: hv: Use
device serial number as PCI domain"), so you should explain what was
wrong with that commit.

Also, it doesn't look like this would apply cleanly to v4.16-rc1.
Your patch shows this:

 -   hbus->sysdata.domain = desc->ser & 0x;

but v4.16-rc1 has this:

hbus->sysdata.domain = desc->ser;

Bjorn

> srpitcha@ linux srpitcha/patch >cat 0001-PCI-BUS-domain-ID-curruption.patch
>
> From a0c407f3e2d57c84ac349f064dcee1d2961e5ca3 Mon Sep 17 00:00:00 2001
>
> From: Sridhar Pitchai 
>
> Date: Thu, 8 Mar 2018 12:33:50 -0800
>
> Subject: [PATCH] PCI BUS domain ID corruption
>
>
>
> When PCI BUS is added, PCI_BUS domain ID is set. When PCI_BUS and device
> to
>
> the bus is racing against each other, the first device tends to
> overwrite
>
> the domain ID. In order to avoid the race, this patch make sure when a
>
> device is added to a bus it never updates the bus domain ID. Since we
> have
>
> the transparent SRIOV mode now, the short VF device name is no longer
>
> needed.
>
>
>
> diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
>
> index 1713bfc..6d43f81 100644
>
> --- a/drivers/pci/host/pci-hyperv.c
>
> +++ b/drivers/pci/host/pci-hyperv.c
>
> @@ -1321,19 +1321,6 @@ static struct hv_pci_dev *new_pcichild_device(struct
> hv_pcibus_device *hbus,
>
> get_pcichild(hpdev, hv_pcidev_ref_childlist);
>
> spin_lock_irqsave(>device_list_lock, flags);
>
>
>
> -   /*
>
> -* When a device is being added to the bus, we set the PCI domain
>
> -* number to be the device serial number, which is non-zero and
>
> -* unique on the same VM.  The serial numbers start with 1, and
>
> -* increase by 1 for each device.  So device names including this
>
> -* can have shorter names than based on the bus instance UUID.
>
> -* Only the first device serial number is used for domain, so the
>
> -* domain number will not change after the first device is added.
>
> -* The lower 16 bits of the serial number is used, otherwise some
>
> -* drivers may not be able to handle it.
>
> -*/
>
> -   if (list_empty(>children))
>
> -   hbus->sysdata.domain = desc->ser & 0x;
>
> list_add_tail(>list_entry, >children);
>
> spin_unlock_irqrestore(>device_list_lock, flags);
>
> return hpdev;
>
> --
>
> 2.7.4
>
>
>
> srpitcha@ linux srpitcha/patch >
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: PROBLEM: 4.15.0-rc3 APIC causes lockups on Core 2 Duo laptop

[Bjorn Helgaas]

On Thu, Dec 28, 2017 at 06:30:58PM -0500, Alexandru Chirvasitu wrote:
> Attached, but heads up on this: when redirecting the output of lspci
> -vvv to a text file as root I get
> 
> pcilib: sysfs_read_vpd: read failed: Input/output error
> 
> I can find bugs filed for various distros to this same effect, but
> haven't tracked down any explanations.

This is a tangent, but I think you should *always* see "Input/output
error" on this system when running "lspci -vvv" as root, regardless of
whether you redirect the output (the error probably goes to stderr,
not stdout, so it's probably easy to miss when not redirecting the
output).

I think this is the -EIO return from pci_vpd_read(), which probably
means pci_vpd_size() returned 0 for one of your devices, which means
the VPD data provided by the device wasn't formatted correctly.  If
this happens, you should see a warning in dmesg about it ("invalid VPD
tag" or similar) -- could you verify that?

It's possible we should return something other than -EIO, or maybe
pcilib should do something other than emitting the warning.  In
pcilib, sysfs_read_vpd() emits the warning [1], and it would seem sort
of ugly to special-case EIO, so maybe we should change this in the
kernel.

It looks like your Qualcomm Atheros Attansic NIC at 06:00.0 is the
only device with VPD, so that's probably the one:

  06:00.0 Ethernet controller: Qualcomm Atheros Attansic L2 Fast Ethernet
Capabilities: [6c] Vital Product Data
  Not readable

I think lspci would still print "Not readable" if we just made the
kernel return 0 instead of -EIO [2].

Bjorn

[1] 
https://git.kernel.org/pub/scm/utils/pciutils/pciutils.git/tree/lib/sysfs.c#n410
[2] https://git.kernel.org/pub/scm/utils/pciutils/pciutils.git/tree/ls-vpd.c#n87
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] PCI: hv: use effective affinity mask

[Bjorn Helgaas]

On Fri, Nov 10, 2017 at 08:55:07AM +, Adrian Suhov (Cloudbase Solutions 
SRL) wrote:
> Hi,
> 
> I've also tested this and it's working good. Kernels tested:
>  - next-20171109 on top of Ubuntu 16.04
>  - MSFT kernel - 4.14.0-rc5 with patch applied - on top of RHEL 7.3
> 
> Adrian

Thanks, Adrian.  I added this to the patch:

  Tested-by: Adrian Suhov <v-ads...@microsoft.com>

> -Original Message-
> From: Bjorn Helgaas [mailto:helg...@kernel.org] 
> Sent: Wednesday, November 8, 2017 3:08 AM
> To: Dexuan Cui <de...@microsoft.com>
> Cc: Bjorn Helgaas <bhelg...@google.com>; linux-...@vger.kernel.org; Jake 
> Oshins <ja...@microsoft.com>; KY Srinivasan <k...@microsoft.com>; Stephen 
> Hemminger <sthem...@microsoft.com>; de...@linuxdriverproject.org; 
> linux-ker...@vger.kernel.org; Haiyang Zhang <haiya...@microsoft.com>; Jork 
> Loeser <jork.loe...@microsoft.com>; Chris Valean (Cloudbase Solutions SRL) 
> <v-chv...@microsoft.com>; Adrian Suhov (Cloudbase Solutions SRL) 
> <v-ads...@microsoft.com>; Simon Xiao <six...@microsoft.com>; 'Eyal Mizrachi' 
> <eya...@mellanox.com>; Jack Morgenstein <ja...@mellanox.com>; Armen Guezalian 
> <arm...@mellanox.com>; Firas Mahameed <fi...@mellanox.com>; Tziporet Koren 
> <tzipo...@mellanox.com>; Daniel Jurgens <dani...@mellanox.com>
> Subject: Re: [PATCH] PCI: hv: use effective affinity mask
> 
> On Wed, Nov 01, 2017 at 08:30:53PM +, Dexuan Cui wrote:
> > 
> > The effective_affinity_mask is always set when an interrupt is 
> > assigned in
> > __assign_irq_vector() -> apic->cpu_mask_to_apicid(), e.g. for struct 
> > apic
> > apic_physflat: -> default_cpu_mask_to_apicid() -> 
> > irq_data_update_effective_affinity(), but it looks d->common->affinity 
> > remains all-1's before the user space or the kernel changes it later.
> > 
> > In the early allocation/initialization phase of an irq, we should use 
> > the effective_affinity_mask, otherwise Hyper-V may not deliver the 
> > interrupt to the expected cpu. Without the patch, if we assign 7 
> > Mellanox ConnectX-3 VFs to a 32-vCPU VM, one of the VFs may fail to receive 
> > interrupts.
> > 
> > Signed-off-by: Dexuan Cui <de...@microsoft.com>
> > Cc: Jake Oshins <ja...@microsoft.com>
> > Cc: Jork Loeser <jloe...@microsoft.com>
> > Cc: Stephen Hemminger <sthem...@microsoft.com>
> > Cc: K. Y. Srinivasan <k...@microsoft.com>
> > ---
> > 
> > Please consider this for v4.14, if it's not too late.
> 
> What would be the rationale for putting it in v4.14?  After the merge window, 
> I usually only merge fixes for problems introduced during the merge window, 
> or for serious regressions.  I can't tell if this fits into that or not.
> 
> >  drivers/pci/host/pci-hyperv.c | 8 +---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/pci/host/pci-hyperv.c 
> > b/drivers/pci/host/pci-hyperv.c index 5ccb47d..8b5f66d 100644
> > --- a/drivers/pci/host/pci-hyperv.c
> > +++ b/drivers/pci/host/pci-hyperv.c
> > @@ -879,7 +879,7 @@ static void hv_irq_unmask(struct irq_data *data)
> > int cpu;
> > u64 res;
> >  
> > -   dest = irq_data_get_affinity_mask(data);
> > +   dest = irq_data_get_effective_affinity_mask(data);
> > pdev = msi_desc_to_pci_dev(msi_desc);
> > pbus = pdev->bus;
> > hbus = container_of(pbus->sysdata, struct hv_pcibus_device, 
> > sysdata); @@ -1042,6 +1042,7 @@ static void hv_compose_msi_msg(struct 
> > irq_data *data, struct msi_msg *msg)
> > struct hv_pci_dev *hpdev;
> > struct pci_bus *pbus;
> > struct pci_dev *pdev;
> > +   struct cpumask *dest;
> > struct compose_comp_ctxt comp;
> > struct tran_int_desc *int_desc;
> > struct {
> > @@ -1056,6 +1057,7 @@ static void hv_compose_msi_msg(struct irq_data *data, 
> > struct msi_msg *msg)
> > int ret;
> >  
> > pdev = msi_desc_to_pci_dev(irq_data_get_msi_desc(data));
> > +   dest = irq_data_get_effective_affinity_mask(data);
> > pbus = pdev->bus;
> > hbus = container_of(pbus->sysdata, struct hv_pcibus_device, sysdata);
> > hpdev = get_pcichild_wslot(hbus, devfn_to_wslot(pdev->devfn)); @@ 
> > -1081,14 +1083,14 @@ static void hv_compose_msi_msg(struct irq_data *data, 
> > struct msi_msg *msg)
> > switch (pci_protocol_version) {
> > case PCI_PROTOCOL_VERSION_1_1:
> > size = hv_compose_msi_req_v1(_pkts.v1,
> > - 

Re: [PATCH] PCI: hv: use effective affinity mask

[Bjorn Helgaas]

On Wed, Nov 08, 2017 at 01:27:02AM +, Dexuan Cui wrote:
> > From: Bjorn Helgaas [mailto:helg...@kernel.org]
> > Sent: Tuesday, November 7, 2017 5:08 PM
> > On Wed, Nov 01, 2017 at 08:30:53PM +, Dexuan Cui wrote:
> > >
> > > Please consider this for v4.14, if it's not too late.
> > 
> > What would be the rationale for putting it in v4.14?  After the merge
> > window, I usually only merge fixes for problems introduced during the
> > merge window, or for serious regressions.  I can't tell if this fits
> > into that or not.
> 
> The patch was sent last Wednesday and I hoped it could catch the 
> merge window to be in v4.14 so we won't have to request a backport
> for the v4.14 stable kernel in future. And I was not exactly sure when
> the merge window was. Sorry :-)
> 
> The patch is not fixing a serious regression. It just fixes a long standing
> issue from the beginning of the Hyper-V vPCI driver: 1 of 7 Mellanox 
> VFs of a 32-vCPU VM running on Hyper-V may not receive interrupts.

No problem.  I added a stable tag so the backport should happen
automatically and put it on my pci/host-hv branch for v4.15, with Jake's
Reviewed-by.  Thanks!

Bjorn
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] PCI: hv: use effective affinity mask

[Bjorn Helgaas]

On Wed, Nov 01, 2017 at 08:30:53PM +, Dexuan Cui wrote:
> 
> The effective_affinity_mask is always set when an interrupt is assigned in
> __assign_irq_vector() -> apic->cpu_mask_to_apicid(), e.g. for struct apic
> apic_physflat: -> default_cpu_mask_to_apicid() ->
> irq_data_update_effective_affinity(), but it looks d->common->affinity
> remains all-1's before the user space or the kernel changes it later.
> 
> In the early allocation/initialization phase of an irq, we should use the
> effective_affinity_mask, otherwise Hyper-V may not deliver the interrupt
> to the expected cpu. Without the patch, if we assign 7 Mellanox ConnectX-3
> VFs to a 32-vCPU VM, one of the VFs may fail to receive interrupts.
> 
> Signed-off-by: Dexuan Cui 
> Cc: Jake Oshins 
> Cc: Jork Loeser 
> Cc: Stephen Hemminger 
> Cc: K. Y. Srinivasan 
> ---
> 
> Please consider this for v4.14, if it's not too late.

What would be the rationale for putting it in v4.14?  After the merge
window, I usually only merge fixes for problems introduced during the
merge window, or for serious regressions.  I can't tell if this fits
into that or not.

>  drivers/pci/host/pci-hyperv.c | 8 +---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
> index 5ccb47d..8b5f66d 100644
> --- a/drivers/pci/host/pci-hyperv.c
> +++ b/drivers/pci/host/pci-hyperv.c
> @@ -879,7 +879,7 @@ static void hv_irq_unmask(struct irq_data *data)
>   int cpu;
>   u64 res;
>  
> - dest = irq_data_get_affinity_mask(data);
> + dest = irq_data_get_effective_affinity_mask(data);
>   pdev = msi_desc_to_pci_dev(msi_desc);
>   pbus = pdev->bus;
>   hbus = container_of(pbus->sysdata, struct hv_pcibus_device, sysdata);
> @@ -1042,6 +1042,7 @@ static void hv_compose_msi_msg(struct irq_data *data, 
> struct msi_msg *msg)
>   struct hv_pci_dev *hpdev;
>   struct pci_bus *pbus;
>   struct pci_dev *pdev;
> + struct cpumask *dest;
>   struct compose_comp_ctxt comp;
>   struct tran_int_desc *int_desc;
>   struct {
> @@ -1056,6 +1057,7 @@ static void hv_compose_msi_msg(struct irq_data *data, 
> struct msi_msg *msg)
>   int ret;
>  
>   pdev = msi_desc_to_pci_dev(irq_data_get_msi_desc(data));
> + dest = irq_data_get_effective_affinity_mask(data);
>   pbus = pdev->bus;
>   hbus = container_of(pbus->sysdata, struct hv_pcibus_device, sysdata);
>   hpdev = get_pcichild_wslot(hbus, devfn_to_wslot(pdev->devfn));
> @@ -1081,14 +1083,14 @@ static void hv_compose_msi_msg(struct irq_data *data, 
> struct msi_msg *msg)
>   switch (pci_protocol_version) {
>   case PCI_PROTOCOL_VERSION_1_1:
>   size = hv_compose_msi_req_v1(_pkts.v1,
> - irq_data_get_affinity_mask(data),
> + dest,
>   hpdev->desc.win_slot.slot,
>   cfg->vector);
>   break;
>  
>   case PCI_PROTOCOL_VERSION_1_2:
>   size = hv_compose_msi_req_v2(_pkts.v2,
> - irq_data_get_affinity_mask(data),
> + dest,
>   hpdev->desc.win_slot.slot,
>   cfg->vector);
>   break;
> -- 
> 2.7.4
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] PCI: hv: use effective affinity mask

[Bjorn Helgaas]

On Wed, Nov 01, 2017 at 08:52:56PM +, Jake Oshins wrote:
> > -Original Message-
> > From: Dexuan Cui
> > Sent: Wednesday, November 1, 2017 1:31 PM
> > To: Bjorn Helgaas <bhelg...@google.com>; linux-...@vger.kernel.org; Jake
> > Oshins <ja...@microsoft.com>; KY Srinivasan <k...@microsoft.com>;
> > Stephen Hemminger <sthem...@microsoft.com>
> > Cc: de...@linuxdriverproject.org; linux-ker...@vger.kernel.org; Haiyang
> > Zhang <haiya...@microsoft.com>; Jork Loeser
> > <jork.loe...@microsoft.com>; Chris Valean (Cloudbase Solutions SRL)  > chv...@microsoft.com>; Adrian Suhov (Cloudbase Solutions SRL)  > ads...@microsoft.com>; Simon Xiao <six...@microsoft.com>; 'Eyal
> > Mizrachi' <eya...@mellanox.com>; Jack Morgenstein
> > <ja...@mellanox.com>; Armen Guezalian <arm...@mellanox.com>; Firas
> > Mahameed <fi...@mellanox.com>; Tziporet Koren
> > <tzipo...@mellanox.com>; Daniel Jurgens <dani...@mellanox.com>
> > Subject: [PATCH] PCI: hv: use effective affinity mask
> > 
> > 
> > The effective_affinity_mask is always set when an interrupt is assigned in
> > __assign_irq_vector() -> apic->cpu_mask_to_apicid(), e.g. for struct apic
> > apic_physflat: -> default_cpu_mask_to_apicid() ->
> > irq_data_update_effective_affinity(), but it looks d->common->affinity
> > remains all-1's before the user space or the kernel changes it later.
> > 
> > In the early allocation/initialization phase of an irq, we should use the
> > effective_affinity_mask, otherwise Hyper-V may not deliver the interrupt to
> > the expected cpu. Without the patch, if we assign 7 Mellanox ConnectX-3
> > VFs to a 32-vCPU VM, one of the VFs may fail to receive interrupts.
> > 
> > Signed-off-by: Dexuan Cui <de...@microsoft.com>
> > Cc: Jake Oshins <ja...@microsoft.com>
> > Cc: Jork Loeser <jloe...@microsoft.com>
> > Cc: Stephen Hemminger <sthem...@microsoft.com>
> > Cc: K. Y. Srinivasan <k...@microsoft.com>
> > ---
> > 
> > Please consider this for v4.14, if it's not too late.
> > 
> >  drivers/pci/host/pci-hyperv.c | 8 +---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
> > index 5ccb47d..8b5f66d 100644
> > --- a/drivers/pci/host/pci-hyperv.c
> > +++ b/drivers/pci/host/pci-hyperv.c
> > @@ -879,7 +879,7 @@ static void hv_irq_unmask(struct irq_data *data)
> > int cpu;
> > u64 res;
> > 
> > -   dest = irq_data_get_affinity_mask(data);
> > +   dest = irq_data_get_effective_affinity_mask(data);
> > pdev = msi_desc_to_pci_dev(msi_desc);
> > pbus = pdev->bus;
> > hbus = container_of(pbus->sysdata, struct hv_pcibus_device,
> > sysdata); @@ -1042,6 +1042,7 @@ static void hv_compose_msi_msg(struct
> > irq_data *data, struct msi_msg *msg)
> > struct hv_pci_dev *hpdev;
> > struct pci_bus *pbus;
> > struct pci_dev *pdev;
> > +   struct cpumask *dest;
> > struct compose_comp_ctxt comp;
> > struct tran_int_desc *int_desc;
> > struct {
> > @@ -1056,6 +1057,7 @@ static void hv_compose_msi_msg(struct irq_data
> > *data, struct msi_msg *msg)
> > int ret;
> > 
> > pdev = msi_desc_to_pci_dev(irq_data_get_msi_desc(data));
> > +   dest = irq_data_get_effective_affinity_mask(data);
> > pbus = pdev->bus;
> > hbus = container_of(pbus->sysdata, struct hv_pcibus_device,
> > sysdata);
> > hpdev = get_pcichild_wslot(hbus, devfn_to_wslot(pdev->devfn));
> > @@ -1081,14 +1083,14 @@ static void hv_compose_msi_msg(struct irq_data
> > *data, struct msi_msg *msg)
> > switch (pci_protocol_version) {
> > case PCI_PROTOCOL_VERSION_1_1:
> > size = hv_compose_msi_req_v1(_pkts.v1,
> > -   irq_data_get_affinity_mask(data),
> > +   dest,
> > hpdev->desc.win_slot.slot,
> > cfg->vector);
> > break;
> > 
> > case PCI_PROTOCOL_VERSION_1_2:
> > size = hv_compose_msi_req_v2(_pkts.v2,
> > -   irq_data_get_affinity_mask(data),
> > +   dest,
> > hpdev->desc.win_slot.slot,
> > cfg->vector);
> > break;
> > --
> > 2.7.4
> 
> Signed-off-by: Jake Oshins <ja...@microsoft.com>

I'm not sure what this means.

Per Documentation/process/submitting-patches.rst, "Signed-off-by"
means you were "involved in the development of the patch, or that
he/she was in the patch's delivery path."  You weren't in the delivery
path (I got it from Dexuan), and if you were involved in development,
your Signed-off-by would normally appear in the original posting.

Should this be a Reviewed-by tag?
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [patch 0/3] x86/PCI/MSI: Make sure that irq reservation mode works everywhere

[Bjorn Helgaas]

On Tue, Oct 17, 2017 at 09:54:56AM +0200, Thomas Gleixner wrote:
> Dexuan reported that the recent rework of the vector allocation mode in x86
> broke HyperV PCI passtrough because the rework missed to add the
> MSI_FLAG_MUST_REACTIVATE flag to the HyperV/PCI interrupt domain info.
> 
> The simple solution would be to set the flag in the HyperV/PCI driver but
> it's better to make this generic and let the PCI/MSI core code set the flag
> when reservation mode is enabled. That ensures that future users of this
> wont trip over the same problem.
> 
> Thanks,
> 
>   tglx
> 
> ---
>  arch/x86/Kconfig   |2 +-
>  arch/x86/kernel/apic/msi.c |5 ++---
>  drivers/pci/msi.c  |2 ++
>  kernel/irq/Kconfig |3 +++
>  4 files changed, 8 insertions(+), 4 deletions(-)

This mentions 4900be83602b ("x86/vector/msi: Switch to global reservation
mode"), which I don't have, so I assume it's an x86 thing.  So I guess
you'll probably merge this via the same tree?

Here's my ack for the PCI part:

Acked-by: Bjorn Helgaas <bhelg...@google.com>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] pci: Fix a possible sleep-in-atomic bug in pci_set_power_state

[Bjorn Helgaas]

On Mon, Oct 09, 2017 at 12:15:17PM -0500, Bjorn Helgaas wrote:
> [+cc Rafael, linux-pm]
> 
> Hi Jia-Ju,
> 
> On Mon, Oct 09, 2017 at 04:16:20PM +0800, Jia-Ju Bai wrote:
> > The drivers vt6655 and gma500 call pci_set_power_state under a spinlock, 
> > which may sleep.
> > The function call paths are:
> > gma_power_begin (acquire the spinlock) (drivers/gpu/drm/gma500/power.c)
> >   gma_resume_pci
> > pci_set_power_state
> >   __pci_start_power_transition (drivers/pci/pci.c)
> > msleep --> may sleep
> > 
> > gma_power_begin (acquire the spinlock) (drivers/gpu/drm/gma500/power.c)
> >   gma_resume_pci
> > pci_enable_device
> >   pci_enable_device_flags (drivers/pci/pci.c)
> > do_pci_enable_device
> >   pci_set_power_state
> > __pci_start_power_transition
> >   msleep --> may sleep
> > 
> > vt6655_suspend (acquire the spinlock) (drivers/staging/vt6655/device_main.c)
> >   pci_set_power_state
> > __pci_start_power_transition (drivers/pci/pci.c)
> >   msleep --> may sleep
> > 
> > To fix these bugs, msleep is replaced with mdelay in 
> > __pci_start_power_transition
> > 
> > These bugs are found by my static analysis tool and my code review.
> 
> We can either
> 
>   - change pci_set_power_state() so it can be called while holding a
> spinlock (as this patch does), or
> 
>   - change the drivers so they don't hold the spinlock while calling 
> pci_set_power_state().
> 
> I think the latter is better because d3cold_delay is typically 100ms,
> and that's a long time to spin with interrupts disabled.
> 
> I assume it's easy to produce an actual failure here?  Why haven't we
> seen bug reports about this?

Sigh, could have saved myself some time if I'd read the whole thread
before responding :)  Sorry for repeating what Greg already said!

> > Signed-off-by: Jia-Ju Bai <baijiaju1...@163.com>
> > ---
> >  drivers/pci/pci.c |2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index 6078dfc..7b763a3 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -823,7 +823,7 @@ static void __pci_start_power_transition(struct pci_dev 
> > *dev, pci_power_t state)
> >  */
> > if (dev->runtime_d3cold) {
> > if (dev->d3cold_delay)
> > -   msleep(dev->d3cold_delay);
> > +   mdelay(dev->d3cold_delay);
> > /*
> >  * When powering on a bridge from D3cold, the
> >  * whole hierarchy may be powered on into
> > -- 
> > 1.7.9.5
> > 
> > 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] pci: Fix a possible sleep-in-atomic bug in pci_set_power_state

[Bjorn Helgaas]

[+cc Rafael, linux-pm]

Hi Jia-Ju,

On Mon, Oct 09, 2017 at 04:16:20PM +0800, Jia-Ju Bai wrote:
> The drivers vt6655 and gma500 call pci_set_power_state under a spinlock, 
> which may sleep.
> The function call paths are:
> gma_power_begin (acquire the spinlock) (drivers/gpu/drm/gma500/power.c)
>   gma_resume_pci
> pci_set_power_state
>   __pci_start_power_transition (drivers/pci/pci.c)
> msleep --> may sleep
> 
> gma_power_begin (acquire the spinlock) (drivers/gpu/drm/gma500/power.c)
>   gma_resume_pci
> pci_enable_device
>   pci_enable_device_flags (drivers/pci/pci.c)
> do_pci_enable_device
>   pci_set_power_state
> __pci_start_power_transition
>   msleep --> may sleep
> 
> vt6655_suspend (acquire the spinlock) (drivers/staging/vt6655/device_main.c)
>   pci_set_power_state
> __pci_start_power_transition (drivers/pci/pci.c)
>   msleep --> may sleep
> 
> To fix these bugs, msleep is replaced with mdelay in 
> __pci_start_power_transition
> 
> These bugs are found by my static analysis tool and my code review.

We can either

  - change pci_set_power_state() so it can be called while holding a
spinlock (as this patch does), or

  - change the drivers so they don't hold the spinlock while calling 
pci_set_power_state().

I think the latter is better because d3cold_delay is typically 100ms,
and that's a long time to spin with interrupts disabled.

I assume it's easy to produce an actual failure here?  Why haven't we
seen bug reports about this?

Bjorn

> Signed-off-by: Jia-Ju Bai 
> ---
>  drivers/pci/pci.c |2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 6078dfc..7b763a3 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -823,7 +823,7 @@ static void __pci_start_power_transition(struct pci_dev 
> *dev, pci_power_t state)
>*/
>   if (dev->runtime_d3cold) {
>   if (dev->d3cold_delay)
> - msleep(dev->d3cold_delay);
> + mdelay(dev->d3cold_delay);
>   /*
>* When powering on a bridge from D3cold, the
>* whole hierarchy may be powered on into
> -- 
> 1.7.9.5
> 
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 08/15] PCI: make device_type const

[Bjorn Helgaas]

On Sat, Aug 19, 2017 at 01:52:19PM +0530, Bhumika Goyal wrote:
> Make this const as it is only stored in the type field of a device
> structure, which is const.
> Done using Coccinelle.
> 
> Signed-off-by: Bhumika Goyal 

Applied to pci/misc for v4.14, thanks!

> ---
>  drivers/pci/endpoint/pci-epf-core.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/endpoint/pci-epf-core.c 
> b/drivers/pci/endpoint/pci-epf-core.c
> index 6877d6a..9d0de12 100644
> --- a/drivers/pci/endpoint/pci-epf-core.c
> +++ b/drivers/pci/endpoint/pci-epf-core.c
> @@ -27,7 +27,7 @@
>  #include 
>  
>  static struct bus_type pci_epf_bus_type;
> -static struct device_type pci_epf_type;
> +static const struct device_type pci_epf_type;
>  
>  /**
>   * pci_epf_linkup() - Notify the function driver that EPC device has
> @@ -275,7 +275,7 @@ static void pci_epf_dev_release(struct device *dev)
>   kfree(epf);
>  }
>  
> -static struct device_type pci_epf_type = {
> +static const struct device_type pci_epf_type = {
>   .release= pci_epf_dev_release,
>  };
>  
> -- 
> 1.9.1
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] hv: fix msi affinity when device requests all possible CPU's

[Bjorn Helgaas]

On Tue, Jul 04, 2017 at 02:59:42PM -0700, Stephen Hemminger wrote:
> On Sun, 2 Jul 2017 16:38:19 -0500
> Bjorn Helgaas <helg...@kernel.org> wrote:
> 
> > On Wed, Jun 28, 2017 at 04:22:04PM -0700, Stephen Hemminger wrote:
> > > When Intel 10G (ixgbevf) is passed to a Hyper-V guest with SR-IOV,
> > > the driver requests affinity with all possible CPU's (0-239) even
> > > those CPU's are not online (and will never be). Because of this the device
> > > is unable to correctly get MSI interrupt's setup.
> > > 
> > > This was caused by the change in 4.12 that converted this affinity
> > > into all possible CPU's (0-31) but then host reports
> > > an error since this is larger than the number of online cpu's.
> > > 
> > > Previously, this worked (up to 4.12-rc1) because only online cpu's
> > > would be put in mask passed to the host.
> > > 
> > > This patch applies only to 4.12.
> > > The driver in linux-next needs a a different fix because of the changes
> > > to PCI host protocol version.  
> > 
> > If Linus decides to postpone v4.12 a week, I can ask him to pull this.  But
> > I suspect he will release v4.12 today.  In that case, I don't know what to
> > do with this other than maybe send it to Greg for a -stable release.
> 
> Looks like this will have to be queued for 4.12 stable.

I assume you'll take care of this, right?  It sounds like there's nothing
to do for upstream because it needs a different fix.

Bjorn
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] hv: fix msi affinity when device requests all possible CPU's

[Bjorn Helgaas]

On Wed, Jun 28, 2017 at 04:22:04PM -0700, Stephen Hemminger wrote:
> When Intel 10G (ixgbevf) is passed to a Hyper-V guest with SR-IOV,
> the driver requests affinity with all possible CPU's (0-239) even
> those CPU's are not online (and will never be). Because of this the device
> is unable to correctly get MSI interrupt's setup.
> 
> This was caused by the change in 4.12 that converted this affinity
> into all possible CPU's (0-31) but then host reports
> an error since this is larger than the number of online cpu's.
> 
> Previously, this worked (up to 4.12-rc1) because only online cpu's
> would be put in mask passed to the host.
> 
> This patch applies only to 4.12.
> The driver in linux-next needs a a different fix because of the changes
> to PCI host protocol version.

If Linus decides to postpone v4.12 a week, I can ask him to pull this.  But
I suspect he will release v4.12 today.  In that case, I don't know what to
do with this other than maybe send it to Greg for a -stable release.

> Fixes: 433fcf6b7b31 ("PCI: hv: Specify CPU_AFFINITY_ALL for MSI affinity when 
> >= 32 CPUs")
> Signed-off-by: Stephen Hemminger 
> ---
>  drivers/pci/host/pci-hyperv.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
> index 84936383e269..3cadfcca3ae9 100644
> --- a/drivers/pci/host/pci-hyperv.c
> +++ b/drivers/pci/host/pci-hyperv.c
> @@ -900,10 +900,12 @@ static void hv_compose_msi_msg(struct irq_data *data, 
> struct msi_msg *msg)
>* processors because Hyper-V only supports 64 in a guest.
>*/
>   affinity = irq_data_get_affinity_mask(data);
> + cpumask_and(affinity, affinity, cpu_online_mask);
> +
>   if (cpumask_weight(affinity) >= 32) {
>   int_pkt->int_desc.cpu_mask = CPU_AFFINITY_ALL;
>   } else {
> - for_each_cpu_and(cpu, affinity, cpu_online_mask) {
> + for_each_cpu(cpu, affinity) {
>   int_pkt->int_desc.cpu_mask |=
>   (1ULL << vmbus_cpu_number_to_vp_number(cpu));
>   }
> -- 
> 2.11.0
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] pci-hyperv: Use only 16 bit integer for PCI domain

[Bjorn Helgaas]

[+cc Christoph]

On Wed, May 24, 2017 at 01:39:15PM -0700, Haiyang Zhang wrote:
> From: Haiyang Zhang 
> 
> This patch uses the lower 16 bits of the serial number as PCI
> domain, otherwise some drivers may not be able to handle it.
> 
> Besides Nvidia drivers, we also found X.org, and DPDK handle
> only 16 bit PCI domain.

If you've sent patches to X.org and DPDK, please includes URLs to
them.

Christoph pointed out the conflict with VMD: vmd_find_free_domain()
allocates domains starting at 0x1 to avoid the 16-bit domains
returned by ACPI _SEG.

I think we need a solution that works for both Nvidia/Hyper-V and VMD.
As it is, it looks like this will fix one place but break things
elsewhere.

If you believe that this will not break VMD, please explain.

Bjorn

> Signed-off-by: Haiyang Zhang 
> ---
>  drivers/pci/host/pci-hyperv.c |4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
> index 8493638..51a815d 100644
> --- a/drivers/pci/host/pci-hyperv.c
> +++ b/drivers/pci/host/pci-hyperv.c
> @@ -1335,9 +1335,11 @@ static void put_pcichild(struct hv_pci_dev *hpdev,
>* can have shorter names than based on the bus instance UUID.
>* Only the first device serial number is used for domain, so the
>* domain number will not change after the first device is added.
> +  * The lower 16 bits of the serial number is used, otherwise some
> +  * drivers may not be able to handle it.
>*/
>   if (list_empty(>children))
> - hbus->sysdata.domain = desc->ser;
> + hbus->sysdata.domain = desc->ser & 0x;
>   list_add_tail(>list_entry, >children);
>   spin_unlock_irqrestore(>device_list_lock, flags);
>   return hpdev;
> -- 
> 1.7.1
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH-v2 0/5] Hyper-V vPCI: use vPCI protocol version 1.2

[Bjorn Helgaas]

On Wed, May 24, 2017 at 01:41:23PM -0700, Jork Loeser wrote:
> From: Jork Loeser 
> 
> Update the Hyper-V vPCI driver to use the Server-2016 version of the vPCI
> protocol, fixing MSI creation and retargeting issues.
> 
> Changes since v1:
> - reduced spew in protocol negotiation (Dan Carpenter)
> - work-around work racing Hyper-V patch (Stephen Hemminger)
> - formatting (Dan)
> 
> Jork Loeser (5):
>   Hyper-V vPCI: Minor format and semantic fix
>   Hyper-V vPCI: Use page allocation for hbus structure
>   PCI-HyperV vPCI: Temporary own CPU-number-to-vCPU-number infra
>   Hyper-V vPCI: Add vPCI version protocol negotiation
>   Hyper-V vPCI: use vPCI protocol version 1.2
> 
>  arch/x86/include/uapi/asm/hyperv.h |   6 +
>  drivers/pci/host/pci-hyperv.c  | 445 
> ++---
>  2 files changed, 370 insertions(+), 81 deletions(-)

Applied with K.Y.'s ack to pci/host-hv for v4.13, thanks!
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH-v2 0/5] Hyper-V vPCI: use vPCI protocol version 1.2

[Bjorn Helgaas]

On Wed, May 24, 2017 at 01:41:23PM -0700, Jork Loeser wrote:
> From: Jork Loeser 
> 
> Update the Hyper-V vPCI driver to use the Server-2016 version of the vPCI
> protocol, fixing MSI creation and retargeting issues.
> 
> Changes since v1:
> - reduced spew in protocol negotiation (Dan Carpenter)
> - work-around work racing Hyper-V patch (Stephen Hemminger)
> - formatting (Dan)
> 
> Jork Loeser (5):
>   Hyper-V vPCI: Minor format and semantic fix
>   Hyper-V vPCI: Use page allocation for hbus structure
>   PCI-HyperV vPCI: Temporary own CPU-number-to-vCPU-number infra
>   Hyper-V vPCI: Add vPCI version protocol negotiation
>   Hyper-V vPCI: use vPCI protocol version 1.2
> 
>  arch/x86/include/uapi/asm/hyperv.h |   6 +
>  drivers/pci/host/pci-hyperv.c  | 445 
> ++---
>  2 files changed, 370 insertions(+), 81 deletions(-)

Waiting for a maintainer ack:

  Hyper-V CORE AND DRIVERS
  M:  "K. Y. Srinivasan" 
  M:  Haiyang Zhang 
  M:  Stephen Hemminger 
  ...
  F:  drivers/pci/host/pci-hyperv.c
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] pci-hyperv: Use only 16 bit integer for PCI domain

[Bjorn Helgaas]

On Thu, Apr 20, 2017 at 11:35 AM, Haiyang Zhang
 wrote:
> From: Haiyang Zhang 
>
> This patch uses the lower 16 bits of the serial number as PCI
> domain, otherwise some drivers may not be able to handle it.

Can you give any more details about this?  Which drivers, for
instance?  Why do drivers care about the domain at all?  Can we or
should we make this more explicit and consistent in the PCI core,
e.g., pci_domain_nr() is currently defined to return "int"; maybe it
should be u32?  (Although I think "int" is the same size as "u32" on
all arches anyway).

> Signed-off-by: Haiyang Zhang 
> ---
>  drivers/pci/host/pci-hyperv.c |4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
> index e73880c..b18dff3 100644
> --- a/drivers/pci/host/pci-hyperv.c
> +++ b/drivers/pci/host/pci-hyperv.c
> @@ -1334,9 +1334,11 @@ static void put_pcichild(struct hv_pci_dev *hpdev,
>  * can have shorter names than based on the bus instance UUID.
>  * Only the first device serial number is used for domain, so the
>  * domain number will not change after the first device is added.
> +* The lower 16 bits of the serial number is used, otherwise some
> +* drivers may not be able to handle it.
>  */
> if (list_empty(>children))
> -   hbus->sysdata.domain = desc->ser;
> +   hbus->sysdata.domain = desc->ser & 0x;
> list_add_tail(>list_entry, >children);
> spin_unlock_irqrestore(>device_list_lock, flags);
> return hpdev;
> --
> 1.7.1
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 17/29] drivers, pci: convert hv_pci_dev.refs from atomic_t to refcount_t

[Bjorn Helgaas]

On Tue, Apr 18, 2017 at 5:40 AM, Reshetova, Elena
<elena.reshet...@intel.com> wrote:
>
>
>> On Mon, 6 Mar 2017 15:38:29 -0600
>> Bjorn Helgaas <helg...@kernel.org> wrote:
>>
>> > [+cc Hyper-V folks, -cc others]
>> >
>> > On Mon, Mar 06, 2017 at 04:21:04PM +0200, Elena Reshetova wrote:
>> > > refcount_t type and corresponding API should be
>> > > used instead of atomic_t when the variable is used as
>> > > a reference counter. This allows to avoid accidental
>> > > refcounter overflows that might lead to use-after-free
>> > > situations.
>> > >
>> > > Signed-off-by: Elena Reshetova <elena.reshet...@intel.com>
>> > > Signed-off-by: Hans Liljestrand <ishkam...@gmail.com>
>> > > Signed-off-by: Kees Cook <keesc...@chromium.org>
>> > > Signed-off-by: David Windsor <dwind...@gmail.com>
>>
>>
>> Reviewed-by: Stephen Hemminger <sthem...@microsoft.com>
>
> Getting back on this: could you take the patch via your tree or should I 
> resubmit to some dedicated place with a new review-by added?

Sorry, for some reason I had assumed this would all go as part of the
larger series.  I applied it to my pci/host-hv branch with Stephen's
reviewed-by for v4.12.

Thanks for the ping!

Bjorn
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 0/2] pci-hyperv: Some miscellaneous fixes

[Bjorn Helgaas]

On Tue, Apr 04, 2017 at 07:54:33PM +, KY Srinivasan wrote:
> > -Original Message-
> > From: Bjorn Helgaas [mailto:helg...@kernel.org]
> > Sent: Tuesday, April 4, 2017 12:04 PM
> > To: KY Srinivasan <k...@microsoft.com>
> > Cc: linux-...@vger.kernel.org; linux-ker...@vger.kernel.org;
> > de...@linuxdriverproject.org; o...@aepfle.de; a...@canonical.com;
> > vkuzn...@redhat.com; jasow...@redhat.com;
> > leann.ogasaw...@canonical.com; marcelo.ce...@canonical.com; Stephen
> > Hemminger <sthem...@microsoft.com>
> > Subject: Re: [PATCH 0/2] pci-hyperv: Some miscellaneous fixes
> > 
> > On Fri, Mar 24, 2017 at 11:06:40AM -0700, k...@exchange.microsoft.com
> > wrote:
> > > From: K. Y. Srinivasan <k...@microsoft.com>
> > >
> > > Some miscellaneous fixes.
> > >
> > > K. Y. Srinivasan (2):
> > >   pci-hyperv: Fix a bug in specifying CPU affinity
> > >   pci-hyperv: Fix an atomic bug
> > >
> > >  drivers/pci/host/pci-hyperv.c |   13 +
> > >  1 files changed, 9 insertions(+), 4 deletions(-)
> > 
> > I applied these with Long's reviewed-by to pci/host-hv for v4.12 with the
> > following subject lines:
> > 
> >   PCI: hv: Specify CPU_AFFINITY_ALL for MSI affinity when >= 32 CPUs
> >   PCI: hv: Allocate interrupt descriptors with GFP_ATOMIC
> > 
> Thank you.
> 
> > There were two copies of [1/2], which makes this error-prone.  I applied
> > the second one that defines CPU_AFFINITY_ALL.
> 
> Thanks again; sorry for the confusion.
> 
> > 
> > They're both marked for stable, but without any clue about when the
> > problems were introduced or how serious they are, I did not queue them for
> > v4.11.  If you want them in v4.11, please supply those additional details.
> 
> I think these issues have been there since   the driver got merged upstream.
> I would want this to be applied against 4.11 as well. Let me know if I should 
> resend
> these patches.

No need to resend, but I do need more details about what issues these fix
and why they're more urgent than garden-variety bug fixes that are planned
for v4.12.  In general for-linus is for fixing problems we added during the
merge window, or other high-priority, low-risk changes.

Bjorn
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 0/2] pci-hyperv: Some miscellaneous fixes

[Bjorn Helgaas]

On Fri, Mar 24, 2017 at 11:06:40AM -0700, k...@exchange.microsoft.com wrote:
> From: K. Y. Srinivasan 
> 
> Some miscellaneous fixes.
> 
> K. Y. Srinivasan (2):
>   pci-hyperv: Fix a bug in specifying CPU affinity
>   pci-hyperv: Fix an atomic bug
> 
>  drivers/pci/host/pci-hyperv.c |   13 +
>  1 files changed, 9 insertions(+), 4 deletions(-)

I applied these with Long's reviewed-by to pci/host-hv for v4.12 with the
following subject lines:

  PCI: hv: Specify CPU_AFFINITY_ALL for MSI affinity when >= 32 CPUs
  PCI: hv: Allocate interrupt descriptors with GFP_ATOMIC

There were two copies of [1/2], which makes this error-prone.  I applied
the second one that defines CPU_AFFINITY_ALL.

They're both marked for stable, but without any clue about when the
problems were introduced or how serious they are, I did not queue them for
v4.11.  If you want them in v4.11, please supply those additional details.

Bjorn
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/2 v5] pci-hyperv: properly handle pci bus remove

[Bjorn Helgaas]

On Thu, Mar 23, 2017 at 02:58:10PM -0700, Long Li wrote:
> From: Long Li 
> 
> hv_pci_devices_present is called in hv_pci_remove when we remove a PCI
> device from host (e.g. by disabling SRIOV on a device). In hv_pci_remove,
> the bus is already removed before the call, so we don't need to rescan the
> bus in the workqueue scheduled from hv_pci_devices_present.
> 
> By introducing status hv_pcibus_removed, we can avoid this situation.
> 
> Signed-off-by: Long Li 
> Reported-by: Xiaofeng Wang 
> Acked-by: K. Y. Srinivasan 

Applied both patches to pci/host-hv for v4.12, thanks!  They both applied
perfectly :)

> ---
>  drivers/pci/host/pci-hyperv.c | 20 +---
>  1 file changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
> index ada9856..39fafda 100644
> --- a/drivers/pci/host/pci-hyperv.c
> +++ b/drivers/pci/host/pci-hyperv.c
> @@ -350,6 +350,7 @@ enum hv_pcibus_state {
>   hv_pcibus_init = 0,
>   hv_pcibus_probed,
>   hv_pcibus_installed,
> + hv_pcibus_removed,
>   hv_pcibus_maximum
>  };
>  
> @@ -1504,13 +1505,24 @@ static void pci_devices_present_work(struct 
> work_struct *work)
>   put_pcichild(hpdev, hv_pcidev_ref_initial);
>   }
>  
> - /* Tell the core to rescan bus because there may have been changes. */
> - if (hbus->state == hv_pcibus_installed) {
> + switch(hbus->state) {
> + case hv_pcibus_installed:
> + /*
> + * Tell the core to rescan bus
> + * because there may have been changes.
> + */
>   pci_lock_rescan_remove();
>   pci_scan_child_bus(hbus->pci_bus);
>   pci_unlock_rescan_remove();
> - } else {
> + break;
> +
> + case hv_pcibus_init:
> + case hv_pcibus_probed:
>   survey_child_resources(hbus);
> + break;
> +
> + default:
> + break;
>   }
>  
>   up(>enum_sem);
> @@ -2185,6 +2197,7 @@ static int hv_pci_probe(struct hv_device *hdev,
>   hbus = kzalloc(sizeof(*hbus), GFP_KERNEL);
>   if (!hbus)
>   return -ENOMEM;
> + hbus->state = hv_pcibus_init;
>  
>   /*
>* The PCI bus "domain" is what is called "segment" in ACPI and
> @@ -2348,6 +2361,7 @@ static int hv_pci_remove(struct hv_device *hdev)
>   pci_stop_root_bus(hbus->pci_bus);
>   pci_remove_root_bus(hbus->pci_bus);
>   pci_unlock_rescan_remove();
> + hbus->state = hv_pcibus_removed;
>   }
>  
>   hv_pci_bus_exit(hdev);
> -- 
> 2.7.4
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/2 v4] pci-hyperv: properly handle pci bus remove

[Bjorn Helgaas]

On Tue, Feb 28, 2017 at 02:19:45AM +, Long Li wrote:
> hv_pci_devices_present is called in hv_pci_remove when we remove a PCI
> device from host (e.g. by disabling SRIOV on a device). In hv_pci_remove,
> the bus is already removed before the call, so we don't need to rescan 
> the bus in the workqueue scheduled from hv_pci_devices_present. By 
> introducing status hv_pcibus_removed, we can avoid this situation.
> 
> Signed-off-by: Long Li 
> Reported-by: Xiaofeng Wang 
> Acked-by: K. Y. Srinivasan 

This didn't apply for me because saving it to a file resulted in some
encoded file with "=3D" instead of "=".  I see that mutt is smart enough to
deal with that in this reply, so there's probably a way for it to decode it
when saving to a file, but I don't know it.

I tried to apply it by hand, but the hunk in hv_pci_remove() doesn't match
the context.  I think your patch is based on something previous to
17978524a636 ("PCI: hv: Fix hv_pci_remove() for hot-remove").  Please
refresh the patch so it applies to my "master" branch (currently
v4.11-rc1).

Also, the "default: break;" case below is redundant and can be removed.

So I'll wait for your updated versions of both these patches.

> ---
>  drivers/pci/host/pci-hyperv.c | 20 +---
>  1 file changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
> index a8deeca..4a37598 100644
> --- a/drivers/pci/host/pci-hyperv.c
> +++ b/drivers/pci/host/pci-hyperv.c
> @@ -348,6 +348,7 @@ enum hv_pcibus_state {
>   hv_pcibus_init = 0,
>   hv_pcibus_probed,
>   hv_pcibus_installed,
> + hv_pcibus_removed,
>   hv_pcibus_maximum
>  };
>  
> @@ -1481,13 +1482,24 @@ static void pci_devices_present_work(struct 
> work_struct *work)
>   put_pcichild(hpdev, hv_pcidev_ref_initial);
>   }
>  
> - /* Tell the core to rescan bus because there may have been changes. */
> - if (hbus->state == hv_pcibus_installed) {
> + switch (hbus->state) {
> + case hv_pcibus_installed:
> + /*
> +  * Tell the core to rescan bus
> +  * because there may have been changes.
> +  */
>   pci_lock_rescan_remove();
>   pci_scan_child_bus(hbus->pci_bus);
>   pci_unlock_rescan_remove();
> - } else {
> + break;
> +
> + case hv_pcibus_init:
> + case hv_pcibus_probed:
>   survey_child_resources(hbus);
> + break;
> +
> + default:
> + break;

^ This is redundant.

>   }
>  
>   up(>enum_sem);
> @@ -2163,6 +2175,7 @@ static int hv_pci_probe(struct hv_device *hdev,
>   hbus = kzalloc(sizeof(*hbus), GFP_KERNEL);
>   if (!hbus)
>   return -ENOMEM;
> + hbus->state = hv_pcibus_init;
>  
>   /*
>* The PCI bus "domain" is what is called "segment" in ACPI and
> @@ -2305,6 +2318,7 @@ static int hv_pci_remove(struct hv_device *hdev)
>   pci_stop_root_bus(hbus->pci_bus);
>   pci_remove_root_bus(hbus->pci_bus);
>   pci_unlock_rescan_remove();
> + hbus->state = hv_pcibus_removed;
>   }
>  
>   ret = hv_send_resources_released(hdev);
> -- 
> 1.8.5.6
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 3/5] pci: set msi_domain_ops as __ro_after_init

[Bjorn Helgaas]

On Tue, Mar 14, 2017 at 11:50:50AM -0700, Jessica Frazelle wrote:
> I can update the patch series, sorry haven't had much time to devote
> to this the past few weeks, but will update in the next day.

Thanks, Jessica!  No problem, I know the feeling :)

Bjorn
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 3/5] pci: set msi_domain_ops as __ro_after_init

[Bjorn Helgaas]

On Thu, Feb 16, 2017 at 03:38:05PM +0100, Thomas Gleixner wrote:
> On Thu, 16 Feb 2017, Bjorn Helgaas wrote:
> > On Wed, Feb 15, 2017 at 10:16:32PM +0100, Thomas Gleixner wrote:
> > 
> > > I think I suggested to Jiang to do that 'update with default functions' to
> > > 
> > > - avoid exporting the world and some more
> > > 
> > > - have the flexibility to add new functions to the ops w/o updating a
> > >   gazillion of existing usage sites, which has saved us lots of chaising 
> > > in
> > >   the last years
> > > 
> > > - avoid the if (ops->ptr) ops->ptr(); else default_fn(); constructs all
> > >   over the place.
> > > 
> > > I admit I did not think about the fact that this makes the structs non
> > > const.
> > > 
> > > Mopping that up by exporting the default functions and setting all the
> > > function pointers is tedious and requires a full tree sweep when we add 
> > > new
> > > stuff. There's also code shared between PCI/platform/DT based stuff, so
> > > that becomes interesting.
> > 
> > It's legal to initialize a field multiple times, and the last one
> > takes precedence, so doing this might at least avoid the full tree
> > sweeps:
> > 
> >   static struct msi_domain_ops vmd_msi_domain_ops = {
> > MSI_DOMAIN_DEFAULT_OPS,
> > .get_hwirq = vmd_get_hwirq,
> >   };
> > 
> > The functions referenced by MSI_DOMAIN_DEFAULT_OPS would still have to
> > be exported, though.
> 
> Hmm, that'd work. Though it will fall apart for those pieces where we share
> code across backends. But I did not yet go through all the places and check
> them.
> 
> > > Doing the if (ops->ptr) ops->ptr() else default_fn(); dance should be
> > > simpler to pull off. There are not that many sites to look at, but then we
> > > have some of the GICv3 code using the domain ops out of core.
> > > 
> > > For now doing the __ro_after_init is definitely the simplest and fastest
> > > solution to tighten these statically allocated structures.
> > 
> > I'm OK with __ro_after_init, at least as an interim solution.
> > 
> > I do think it would be good to audit all the uses of
> > MSI_FLAG_USE_DEF_DOM_OPS and MSI_FLAG_USE_DEF_CHIP_OPS, since they
> > seem to be the primary indicator of when the struct might be modified.
> > I suspect we could add __ro_after_init to more than just pci-hyperv.c,
> > vmd.c, and msi.c
> 
> Agreed. I have it on my radar.

This seems like a worthwhile change, so I don't want to just drop this
patch.  But if we're going to do something, I'd like to do it
everywhere that it makes sense, all at the same time.

It looks like the v2 series was split up by subsystem, which is fine
with me.  I'll happily apply the PCI parts (or ack them, since it
might make sense to apply all of them via the same non-PCI tree).

But I *would* like to include the following users of
MSI_FLAG_USE_DEF_DOM_OPS and MSI_FLAG_USE_DEF_CHIP_OPS at the same
time (or explain why __ro_after_init won't work for them):

  pci-xgene-msi.c
  pcie-altera-msi.c
  pcie-iproc-msi.c
  pcie-xilinx-nwl.c

Bjorn
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [RFC PATCH v2 06/32] x86/pci: Use memremap when walking setup data

[Bjorn Helgaas]

On Fri, Mar 03, 2017 at 03:15:34PM -0600, Tom Lendacky wrote:
> On 3/3/2017 2:42 PM, Bjorn Helgaas wrote:
> >On Thu, Mar 02, 2017 at 10:13:10AM -0500, Brijesh Singh wrote:
> >>From: Tom Lendacky <thomas.lenda...@amd.com>
> >>
> >>The use of ioremap will force the setup data to be mapped decrypted even
> >>though setup data is encrypted.  Switch to using memremap which will be
> >>able to perform the proper mapping.
> >
> >How should callers decide whether to use ioremap() or memremap()?
> >
> >memremap() existed before SME and SEV, and this code is used even if
> >SME and SEV aren't supported, so the rationale for this change should
> >not need the decryption argument.
> 
> When SME or SEV is active an ioremap() will remove the encryption bit
> from the pagetable entry when it is mapped.  This allows MMIO, which
> doesn't support SME/SEV, to be performed successfully.  So my take is
> that ioremap() should be used for MMIO and memremap() for pages in RAM.

OK, thanks.  The commit message should say something like "this is
RAM, not MMIO, so we should map it with memremap(), not ioremap()".
That's the part that determines whether the change is correct.

You can mention the encryption part, too, but it's definitely
secondary because the change has to make sense on its own, without
SME/SEV.

The following commits (from https://github.com/codomania/tip/branches)
all do basically the same thing so the changelogs (and summaries)
should all be basically the same:

  cb0d0d1eb0a6 x86: Change early_ioremap to early_memremap for BOOT data
  91acb68b8333 x86/pci: Use memremap when walking setup data
  4f687503e23f x86: Access the setup data through sysfs decrypted
  e90246b8c229 x86: Access the setup data through debugfs decrypted

I would collect them all together and move them to the beginning of
your series, since they don't depend on anything else.

Also, change "x86/pci: " to "x86/PCI" so it matches the previous
convention.

> >>Signed-off-by: Tom Lendacky <thomas.lenda...@amd.com>

Acked-by: Bjorn Helgaas <bhelg...@google.com>

> >>---
> >> arch/x86/pci/common.c |4 ++--
> >> 1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >>diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c
> >>index a4fdfa7..0b06670 100644
> >>--- a/arch/x86/pci/common.c
> >>+++ b/arch/x86/pci/common.c
> >>@@ -691,7 +691,7 @@ int pcibios_add_device(struct pci_dev *dev)
> >>
> >>pa_data = boot_params.hdr.setup_data;
> >>while (pa_data) {
> >>-   data = ioremap(pa_data, sizeof(*rom));
> >>+   data = memremap(pa_data, sizeof(*rom), MEMREMAP_WB);
> >
> >I can't quite connect the dots here.  ioremap() on x86 would do
> >ioremap_nocache().  memremap(MEMREMAP_WB) would do arch_memremap_wb(),
> >which is ioremap_cache().  Is making a cacheable mapping the important
> >difference?
> 
> The memremap(MEMREMAP_WB) will actually check to see if it can perform
> a __va(pa_data) in try_ram_remap() and then fallback to the
> arch_memremap_wb().  So it's actually the __va() vs the ioremap_cache()
> that is the difference.
> 
> Thanks,
> Tom
> 
> >
> >>if (!data)
> >>return -ENOMEM;
> >>
> >>@@ -710,7 +710,7 @@ int pcibios_add_device(struct pci_dev *dev)
> >>}
> >>}
> >>pa_data = data->next;
> >>-   iounmap(data);
> >>+   memunmap(data);
> >>}
> >>set_dma_domain_ops(dev);
> >>set_dev_domain_options(dev);
> >>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 17/29] drivers, pci: convert hv_pci_dev.refs from atomic_t to refcount_t

[Bjorn Helgaas]

[+cc Hyper-V folks, -cc others]

On Mon, Mar 06, 2017 at 04:21:04PM +0200, Elena Reshetova wrote:
> refcount_t type and corresponding API should be
> used instead of atomic_t when the variable is used as
> a reference counter. This allows to avoid accidental
> refcounter overflows that might lead to use-after-free
> situations.
> 
> Signed-off-by: Elena Reshetova 
> Signed-off-by: Hans Liljestrand 
> Signed-off-by: Kees Cook 
> Signed-off-by: David Windsor 
> ---
>  drivers/pci/host/pci-hyperv.c | 9 +
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
> index cd114c6..870deed 100644
> --- a/drivers/pci/host/pci-hyperv.c
> +++ b/drivers/pci/host/pci-hyperv.c
> @@ -56,6 +56,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  
>  /*
> @@ -421,7 +422,7 @@ enum hv_pcidev_ref_reason {
>  struct hv_pci_dev {
>   /* List protected by pci_rescan_remove_lock */
>   struct list_head list_entry;
> - atomic_t refs;
> + refcount_t refs;
>   enum hv_pcichild_state state;
>   struct pci_function_description desc;
>   bool reported_missing;
> @@ -1254,13 +1255,13 @@ static void q_resource_requirements(void *context, 
> struct pci_response *resp,
>  static void get_pcichild(struct hv_pci_dev *hpdev,
>   enum hv_pcidev_ref_reason reason)
>  {
> - atomic_inc(>refs);
> + refcount_inc(>refs);
>  }
>  
>  static void put_pcichild(struct hv_pci_dev *hpdev,
>   enum hv_pcidev_ref_reason reason)
>  {
> - if (atomic_dec_and_test(>refs))
> + if (refcount_dec_and_test(>refs))
>   kfree(hpdev);
>  }
>  
> @@ -1314,7 +1315,7 @@ static struct hv_pci_dev *new_pcichild_device(struct 
> hv_pcibus_device *hbus,
>   wait_for_completion(_pkt.host_event);
>  
>   hpdev->desc = *desc;
> - get_pcichild(hpdev, hv_pcidev_ref_initial);
> + refcount_set(>refs, 1);
>   get_pcichild(hpdev, hv_pcidev_ref_childlist);
>   spin_lock_irqsave(>device_list_lock, flags);
>   list_add_tail(>list_entry, >children);
> -- 
> 2.7.4
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [RFC PATCH v2 06/32] x86/pci: Use memremap when walking setup data

[Bjorn Helgaas]

On Thu, Mar 02, 2017 at 10:13:10AM -0500, Brijesh Singh wrote:
> From: Tom Lendacky 
> 
> The use of ioremap will force the setup data to be mapped decrypted even
> though setup data is encrypted.  Switch to using memremap which will be
> able to perform the proper mapping.

How should callers decide whether to use ioremap() or memremap()?

memremap() existed before SME and SEV, and this code is used even if
SME and SEV aren't supported, so the rationale for this change should
not need the decryption argument.

> Signed-off-by: Tom Lendacky 
> ---
>  arch/x86/pci/common.c |4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c
> index a4fdfa7..0b06670 100644
> --- a/arch/x86/pci/common.c
> +++ b/arch/x86/pci/common.c
> @@ -691,7 +691,7 @@ int pcibios_add_device(struct pci_dev *dev)
>  
>   pa_data = boot_params.hdr.setup_data;
>   while (pa_data) {
> - data = ioremap(pa_data, sizeof(*rom));
> + data = memremap(pa_data, sizeof(*rom), MEMREMAP_WB);

I can't quite connect the dots here.  ioremap() on x86 would do
ioremap_nocache().  memremap(MEMREMAP_WB) would do arch_memremap_wb(),
which is ioremap_cache().  Is making a cacheable mapping the important
difference?

>   if (!data)
>   return -ENOMEM;
>  
> @@ -710,7 +710,7 @@ int pcibios_add_device(struct pci_dev *dev)
>   }
>   }
>   pa_data = data->next;
> - iounmap(data);
> + memunmap(data);
>   }
>   set_dma_domain_ops(dev);
>   set_dev_domain_options(dev);
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [RFC PATCH v2 00/32] x86: Secure Encrypted Virtualization (AMD)

[Bjorn Helgaas]

On Thu, Mar 02, 2017 at 10:12:01AM -0500, Brijesh Singh wrote:
> This RFC series provides support for AMD's new Secure Encrypted Virtualization
> (SEV) feature. This RFC is build upon Secure Memory Encryption (SME) RFCv4 
> [1].

What kernel version is this series based on?
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] pci-hyperv: Use device serial number as PCI domain

[Bjorn Helgaas]

On Mon, Feb 13, 2017 at 06:10:11PM +, Haiyang Zhang wrote:
> 
> This allows PCI domain numbers starts with 1, and also unique
> on the same VM. So names, such as VF NIC names, that include
> domain number as part of the name, can be shorter than that
> based on part of bus UUID previously. The new names will also
> stay same for VMs created with copied VHD and same number of
> devices.
> 
> Signed-off-by: Haiyang Zhang 
> Reviewed-by: K. Y. Srinivasan 

Applied to pci/host-hv for v4.11, thanks!

I assume Stephen meant a "Reviewed-by", not a "Signed-off-by", so that's
what I added.

> ---
>  drivers/pci/host/pci-hyperv.c |   10 ++
>  1 files changed, 10 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
> index 3efcc7b..b92b565 100644
> --- a/drivers/pci/host/pci-hyperv.c
> +++ b/drivers/pci/host/pci-hyperv.c
> @@ -1315,6 +1315,16 @@ static void put_pcichild(struct hv_pci_dev *hpdev,
>   get_pcichild(hpdev, hv_pcidev_ref_initial);
>   get_pcichild(hpdev, hv_pcidev_ref_childlist);
>   spin_lock_irqsave(>device_list_lock, flags);
> + /* When a device is being added into the bus, we set the PCI domain
> +  * number to be the device serial number, which is non zero and
> +  * unique on the same VM. The serial numbers start with 1, and
> +  * increase by 1 for each device. So device names including this
> +  * can have shorter names than based on the bus instance UUID.
> +  * Only the first device serial number is used for domain, so the
> +  * domain number will not change after the first device is added.
> +  */
> + if (list_empty(>children))
> + hbus->sysdata.domain = desc->ser;
>   list_add_tail(>list_entry, >children);
>   spin_unlock_irqrestore(>device_list_lock, flags);
>   return hpdev;
> -- 
> 1.7.1
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 3/5] pci: set msi_domain_ops as __ro_after_init

[Bjorn Helgaas]

On Wed, Feb 15, 2017 at 10:16:32PM +0100, Thomas Gleixner wrote:

> I think I suggested to Jiang to do that 'update with default functions' to
> 
> - avoid exporting the world and some more
> 
> - have the flexibility to add new functions to the ops w/o updating a
>   gazillion of existing usage sites, which has saved us lots of chaising in
>   the last years
> 
> - avoid the if (ops->ptr) ops->ptr(); else default_fn(); constructs all
>   over the place.
> 
> I admit I did not think about the fact that this makes the structs non
> const.
> 
> Mopping that up by exporting the default functions and setting all the
> function pointers is tedious and requires a full tree sweep when we add new
> stuff. There's also code shared between PCI/platform/DT based stuff, so
> that becomes interesting.

It's legal to initialize a field multiple times, and the last one
takes precedence, so doing this might at least avoid the full tree
sweeps:

  static struct msi_domain_ops vmd_msi_domain_ops = {
MSI_DOMAIN_DEFAULT_OPS,
.get_hwirq = vmd_get_hwirq,
  };

The functions referenced by MSI_DOMAIN_DEFAULT_OPS would still have to
be exported, though.

> Doing the if (ops->ptr) ops->ptr() else default_fn(); dance should be
> simpler to pull off. There are not that many sites to look at, but then we
> have some of the GICv3 code using the domain ops out of core.
> 
> For now doing the __ro_after_init is definitely the simplest and fastest
> solution to tighten these statically allocated structures.

I'm OK with __ro_after_init, at least as an interim solution.

I do think it would be good to audit all the uses of
MSI_FLAG_USE_DEF_DOM_OPS and MSI_FLAG_USE_DEF_CHIP_OPS, since they
seem to be the primary indicator of when the struct might be modified.
I suspect we could add __ro_after_init to more than just pci-hyperv.c,
vmd.c, and msi.c

Bjorn
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 3/5] pci: set msi_domain_ops as __ro_after_init

[Bjorn Helgaas]

[+cc Kees, Thomas, Marc]

Hi Jess,

Thanks for the patch!

On Fri, Feb 10, 2017 at 05:37:56PM -0800, Jess Frazelle wrote:
> Marked msi_domain_ops structs as __ro_after_init when called only during init.
> This protects the data structure from accidental corruption.
> 
> Suggested-by: Kees Cook 
> Signed-off-by: Jess Frazelle 
> ---
>  drivers/pci/host/pci-hyperv.c | 2 +-
>  drivers/pci/host/vmd.c| 2 +-
>  drivers/pci/msi.c | 2 +-

I understand the value of __ro_after_init, but I'm not certain about
sprinkling it around in seemingly random places because it's hard to
know where to put it and whether we put it in all the right places.

How did you choose these three files to change?  There are other
instances of struct msi_domain_ops that use MSI_FLAG_USE_DEF_DOM_OPS.
Should they be changed, too?  If not, is there a rule to figure out
which ones should be made __ro_after_init?

I wonder if adding __ro_after_init is really the best solution.  I
looked at VMD to see how vmd_msi_domain_ops is updated.  Here are the
definitions:

  static struct msi_domain_ops vmd_msi_domain_ops = {
.get_hwirq = vmd_get_hwirq,
.msi_init  = vmd_msi_init,
...
  };

  static struct msi_domain_info vmd_msi_domain_info = {
.flags = MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS |
 MSI_FLAG_PCI_MSIX,
.ops = _msi_domain_ops,
...
  };

Both vmd_msi_domain_ops and vmd_msi_domain_info are statically
initialized, but not completely.  Then we pass a pointer to
pci_msi_create_irq_domain(), which fills in defaults for some of the
function pointers that weren't statically initialized:

  vmd_enable_domain()
pci_msi_create_irq_domain(NULL, _msi_domain_info, ...)
  if (info->flags & MSI_FLAG_USE_DEF_DOM_OPS)
pci_msi_domain_update_dom_ops(info)
  if (ops->set_desc == NULL)
ops->msi_check = pci_msi_domain_check_cap

We know at build-time what all the function pointers will be, so in
principle we should be able to make the struct const, which would be
even better than __ro_after_init.

For example, we could require that callers set every function pointer
before calling pci_msi_create_irq_domain(), using the default ones
(pci_msi_domain_set_desc, pci_msi_domain_check_cap,
pci_msi_domain_handle_error) if it doesn't need to override them,
e.g.,

  static struct msi_domain_ops vmd_msi_domain_ops = {
.get_hwirq = vmd_get_hwirq,
.msi_check = pci_msi_domain_check_cap,
  };

Or we could leave NULL pointers in the structure and have the code
that calls through the function pointers check for NULL and call the
default itself, e.g.,

  if (ops->msi_check)
ops->msi_check(...)
  else
pci_msi_domain_check_cap(...)

It looks like the "USE_DEF_OPS" framework was added by Jiang Liu with
the commits below.  I would CC: him for his thoughts, but I don't
have a current email address.

  aeeb59657c35 ("genirq: Provide default callbacks for msi_domain_ops")
  3878eaefb89a ("PCI/MSI: Enhance core to support hierarchy irqdomain")

Bjorn

>  3 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
> index 3efcc7bdc5fb..f05b93689d8f 100644
> --- a/drivers/pci/host/pci-hyperv.c
> +++ b/drivers/pci/host/pci-hyperv.c
> @@ -958,7 +958,7 @@ static irq_hw_number_t hv_msi_domain_ops_get_hwirq(struct 
> msi_domain_info *info,
>   return arg->msi_hwirq;
>  }
> 
> -static struct msi_domain_ops hv_msi_ops = {
> +static struct msi_domain_ops hv_msi_ops __ro_after_init = {
>   .get_hwirq  = hv_msi_domain_ops_get_hwirq,
>   .msi_prepare= pci_msi_prepare,
>   .set_desc   = pci_msi_set_desc,
> diff --git a/drivers/pci/host/vmd.c b/drivers/pci/host/vmd.c
> index 18ef1a93c10a..152c461538e4 100644
> --- a/drivers/pci/host/vmd.c
> +++ b/drivers/pci/host/vmd.c
> @@ -253,7 +253,7 @@ static void vmd_set_desc(msi_alloc_info_t *arg, struct 
> msi_desc *desc)
>   arg->desc = desc;
>  }
> 
> -static struct msi_domain_ops vmd_msi_domain_ops = {
> +static struct msi_domain_ops vmd_msi_domain_ops __ro_after_init = {
>   .get_hwirq  = vmd_get_hwirq,
>   .msi_init   = vmd_msi_init,
>   .msi_free   = vmd_msi_free,
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index 50c5003295ca..93141d5e2d1c 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -1413,7 +1413,7 @@ static void pci_msi_domain_set_desc(msi_alloc_info_t 
> *arg,
>  #define pci_msi_domain_set_desc  NULL
>  #endif
> 
> -static struct msi_domain_ops pci_msi_domain_ops_default = {
> +static struct msi_domain_ops pci_msi_domain_ops_default __ro_after_init = {
>   .set_desc   = pci_msi_domain_set_desc,
>   .msi_check  = pci_msi_domain_check_cap,
>   .handle_error   = pci_msi_domain_handle_error,
> --
> 2.11.0
> 
___
devel mailing list
de...@linuxdriverproject.org

Re: [Resend PATCH 1/2 v3] pci-hyperv: properly handle pci bus remove

[Bjorn Helgaas]

On Fri, Feb 10, 2017 at 7:18 PM, Long Li <lon...@microsoft.com> wrote:
> Hi Bjorn,
>
> This patch and the other one in the series ([Resend PATCH 2/2 v3] pci-hyperv: 
> lock pci bus on device eject) have been Acked.
>
> Is there anything else should be done before it can be merged? Please let me 
> know.

Sorry, I don't know what happened here.  I see your Jan 23 posting in
my work email (bhelg...@google.com), but I don't see it on the
linux-pci or linux-kernel lists, and patchwork [1] doesn't have a copy
either.   I suspect there was something about your email that made
vger drop it (maybe an HTML or other "fancy" stuff per
http://vger.kernel.org/majordomo-info.html).

Patchwork works by subscribing to linux-pci and collecting things that
look like patches.  Then I work from patchwork as a to-do list.
That's a convenient way to ensure that patches appear on the mailing
list before I apply them.  It also means that if a patch doesn't
appear on linux-pci and subsequently in patchwork, I don't know about
it.

Patchwork does have copies of previous versions, but I marked them
"changes requested".  When I do that, the patch drops off the to-do
list because I'm expecting a new version, which *will* appear on the
list.  I don't mark things "changes requested" if I'm only waiting for
an ack, so it looks like the only change I was looking for was a
changelog revision.  Normally I just tweak changelogs myself, so I
apologize for not doing that in this case.

Anyway, can you just post the current version, including the acks, and
make sure it shows up on the mailing list?

I'm sorry this has languished so long.  Thanks for reminding me about
it so we can sort this out.

[1] 
https://patchwork.ozlabs.org/project/linux-pci/list/?submitter=69886=*==both=

>> -Original Message-
>> From: KY Srinivasan
>> Sent: Friday, January 27, 2017 10:42 AM
>> To: Long Li <lon...@microsoft.com>; Haiyang Zhang
>> <haiya...@microsoft.com>; Bjorn Helgaas <bhelg...@google.com>
>> Cc: de...@linuxdriverproject.org; linux-...@vger.kernel.org; linux-
>> ker...@vger.kernel.org; Long Li <lon...@microsoft.com>
>> Subject: RE: [Resend PATCH 1/2 v3] pci-hyperv: properly handle pci bus
>> remove
>>
>>
>>
>> > -Original Message-
>> > From: Long Li [mailto:lon...@exchange.microsoft.com]
>> > Sent: Monday, January 23, 2017 9:45 PM
>> > To: KY Srinivasan <k...@microsoft.com>; Haiyang Zhang
>> > <haiya...@microsoft.com>; Bjorn Helgaas <bhelg...@google.com>
>> > Cc: de...@linuxdriverproject.org; linux-...@vger.kernel.org; linux-
>> > ker...@vger.kernel.org; Long Li <lon...@microsoft.com>
>> > Subject: [Resend PATCH 1/2 v3] pci-hyperv: properly handle pci bus
>> > remove
>> >
>> > [This sender failed our fraud detection checks and may not be who they
>> > appear to be. Learn about spoofing at
>> > http://aka.ms/LearnAboutSpoofing]
>> >
>> > From: Long Li <lon...@microsoft.com>
>> >
>> > hv_pci_devices_present is called in hv_pci_remove when we remove a PCI
>> > device from host (e.g. by disabling SRIOV on a device). In
>> > hv_pci_remove, the bus is already removed before the call, so we don't
>> > need to rescan the bus in the workqueue scheduled from
>> > hv_pci_devices_present. By introducing status hv_pcibus_removed, we
>> can avoid this situation.
>> >
>> > Signed-off-by: Long Li <lon...@microsoft.com>
>> > Reported-by: Xiaofeng Wang <xiaof...@redhat.com>
>> Acked-by: K. Y. Srinivasan <k...@microsoft.com>
>> > ---
>> >  drivers/pci/host/pci-hyperv.c | 20 +---
>> >  1 file changed, 17 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/drivers/pci/host/pci-hyperv.c
>> > b/drivers/pci/host/pci-hyperv.c index a8deeca..4a37598 100644
>> > --- a/drivers/pci/host/pci-hyperv.c
>> > +++ b/drivers/pci/host/pci-hyperv.c
>> > @@ -348,6 +348,7 @@ enum hv_pcibus_state {
>> > hv_pcibus_init = 0,
>> > hv_pcibus_probed,
>> > hv_pcibus_installed,
>> > +   hv_pcibus_removed,
>> > hv_pcibus_maximum
>> >  };
>> >
>> > @@ -1481,13 +1482,24 @@ static void pci_devices_present_work(struct
>> > work_struct *work)
>> > put_pcichild(hpdev, hv_pcidev_ref_initial);
>> > }
>> >
>> > -   /* Tell the core to rescan bus because there may have been changes.
>> */
>> > -   if (hbus->state == hv_pcibus_installed) {
>&

Re: [PATCH v1 0/3] PCI: Configure PCIe MPS settings

[Bjorn Helgaas]

On Wed, Feb 08, 2017 at 04:49:22PM -0600, Bjorn Helgaas wrote:
> [Some of you will get this twice because Gmail and I aren't getting along
> today; sorry]
> 
> The PCI core doesn't configure the PCIe MPS settings by itself.  Each
> host bridge driver has to call pcie_bus_configure_settings() to make
> this happen.
> 
> Jon fixed this already for pcie-iproc.c.  I propose these similar
> patches for other drivers.
> 
> HV guys, I included you because create_root_hv_pci_bus() is one place
> that calls pci_scan_child_bus() but does not call
> pcie_bus_configure_settings().  I know you probably don't strictly
> *need* to configure MPS settings in a paravirtual front-end, but the
> PCI core does other device configuration in this path:
> 
>   pci_scan_child_bus
> pci_scan_slot
>   pci_scan_single_device
> pci_device_add
>   pci_configure_device
>   pci_init_capabilities
> 
> and I would like to eventually migrate the MPS configuration into that
> same path.  Since we do this other configuration for HV devices
> already, I think pcie_bus_configure_settings() should also work (even
> if it ends up not doing anything to real devices).
> 
> It would make that eventual migration easier if all the
> pci_scan_child_bus() callers had the same pattern of calling
> pcie_bus_configure_settings().
> 
> So would it make sense to make a similar patch for HV?  It looks easy
> to add it to create_root_hv_pci_bus(), but I don't know exactly what
> to do about pci_devices_present_work().
> 
> ---
> 
> Bjorn Helgaas (3):
>   PCI: xilinx: Configure PCIe MPS settings
>   PCI: versatile: Configure PCIe MPS settings
>   PCI: xgene: Configure PCIe MPS settings
> 
> 
>  drivers/pci/host/pci-versatile.c |4 +++-
>  drivers/pci/host/pci-xgene.c |4 +++-
>  drivers/pci/host/pcie-xilinx.c   |4 +++-
>  3 files changed, 9 insertions(+), 3 deletions(-)

I applied these to pci/host-xilinx, pci/host-versatile, and
pci/host-xgene, respectively, for v4.11.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v1 1/3] PCI: xilinx: Configure PCIe MPS settings

[Bjorn Helgaas]

On Wed, Feb 08, 2017 at 11:19:56PM -0800, Christoph Hellwig wrote:
> On Wed, Feb 08, 2017 at 04:49:30PM -0600, Bjorn Helgaas wrote:
> > +   list_for_each_entry(child, >children, node)
> > +   pcie_bus_configure_settings(child);
> 
> This loop is duplicated in just about every driver, so it it
> might be a good idea to provide a littler helper for it.

Yeah, you're right.  One reason I didn't is because I would like to
remove these if we can move the MPS setup into the pci_device_add()
path.

We ought to be able to do at least the minimal "configure this new
device to match the existing hierarchy; if that's impossible, disable
the device" sort of thing there.

We probably would still want some sort of bus- or root port- or host
bridge-level configuration that can take a broader view, and that
might be done where we currently have this loop.  But I don't know 
exactly what that should look like.

Bjorn
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] PCI: hv: fix wslot_to_devfn()

[Bjorn Helgaas]

On Tue, Feb 07, 2017 at 09:00:10AM +, Dexuan Cui wrote:
> The devfn of 00:02.0 is 0x10.
> devfn_to_wslot(0x10) == 0x2, and wslot_to_devfn(0x2) should be 0x10,
> while it's 0x2 in the current code.
> 
> Due to this, hv_eject_device_work() -> pci_get_domain_bus_and_slot()
> returns NULL and pci_stop_and_remove_bus_device() is not called.
> 
> Later when the real device driver's .remove() is invoked by
> hv_pci_remove() -> pci_stop_root_bus(), some warnings can be noticed
> because the VM has lost the access to the underlying device at that
> time.
> 
> Signed-off-by: Jake Oshins 
> Signed-off-by: Dexuan Cui 
> Cc: sta...@vger.kernel.org
> Cc: K. Y. Srinivasan 
> CC: Haiyang Zhang 
> Cc: Stephen Hemminger 

Applied with Haiyang's ack to pci/host-hv for v4.11, thanks!

> ---
>  drivers/pci/host/pci-hyperv.c | 8 +---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> The patch is co-made by Jake and me.
> 
> diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
> index 3efcc7b..cd114c6 100644
> --- a/drivers/pci/host/pci-hyperv.c
> +++ b/drivers/pci/host/pci-hyperv.c
> @@ -130,7 +130,8 @@ enum pci_message_type {
>   */
>  union win_slot_encoding {
>   struct {
> - u32 func:8;
> + u32 dev:5;
> + u32 func:3;
>   u32 reserved:24;
>   } bits;
>   u32 slot;
> @@ -485,7 +486,8 @@ static u32 devfn_to_wslot(int devfn)
>   union win_slot_encoding wslot;
>  
>   wslot.slot = 0;
> - wslot.bits.func = PCI_SLOT(devfn) | (PCI_FUNC(devfn) << 5);
> + wslot.bits.dev = PCI_SLOT(devfn);
> + wslot.bits.func = PCI_FUNC(devfn);
>  
>   return wslot.slot;
>  }
> @@ -503,7 +505,7 @@ static int wslot_to_devfn(u32 wslot)
>   union win_slot_encoding slot_no;
>  
>   slot_no.slot = wslot;
> - return PCI_DEVFN(0, slot_no.bits.func);
> + return PCI_DEVFN(slot_no.bits.dev, slot_no.bits.func);
>  }
>  
>  /*
> -- 
> 1.8.3.1
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v1 2/3] PCI: versatile: Configure PCIe MPS settings

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelg...@google.com>

Make sure PCIe MPS settings are valid when we enumerate a new hierarchy.

Based-on-patch-by: Jon Mason <jon.ma...@broadcom.com>
Signed-off-by: Bjorn Helgaas <bhelg...@google.com>
---
 drivers/pci/host/pci-versatile.c |4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/host/pci-versatile.c b/drivers/pci/host/pci-versatile.c
index b7dc07002f13..5ebee7d37ff5 100644
--- a/drivers/pci/host/pci-versatile.c
+++ b/drivers/pci/host/pci-versatile.c
@@ -124,7 +124,7 @@ static int versatile_pci_probe(struct platform_device *pdev)
int ret, i, myslot = -1;
u32 val;
void __iomem *local_pci_cfg_base;
-   struct pci_bus *bus;
+   struct pci_bus *bus, *child;
LIST_HEAD(pci_res);
 
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
@@ -204,6 +204,8 @@ static int versatile_pci_probe(struct platform_device *pdev)
 
pci_fixup_irqs(pci_common_swizzle, of_irq_parse_and_map_pci);
pci_assign_unassigned_bus_resources(bus);
+   list_for_each_entry(child, >children, node)
+   pcie_bus_configure_settings(child);
pci_bus_add_devices(bus);
 
return 0;

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v1 3/3] PCI: xgene: Configure PCIe MPS settings

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelg...@google.com>

Make sure PCIe MPS settings are valid when we enumerate a new hierarchy.

Based-on-patch-by: Jon Mason <jon.ma...@broadcom.com>
Signed-off-by: Bjorn Helgaas <bhelg...@google.com>
---
 drivers/pci/host/pci-xgene.c |4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/host/pci-xgene.c b/drivers/pci/host/pci-xgene.c
index 7c3b54b9eb17..8091a8778ae7 100644
--- a/drivers/pci/host/pci-xgene.c
+++ b/drivers/pci/host/pci-xgene.c
@@ -638,7 +638,7 @@ static int xgene_pcie_probe_bridge(struct platform_device 
*pdev)
struct device_node *dn = dev->of_node;
struct xgene_pcie_port *port;
resource_size_t iobase = 0;
-   struct pci_bus *bus;
+   struct pci_bus *bus, *child;
int ret;
LIST_HEAD(res);
 
@@ -681,6 +681,8 @@ static int xgene_pcie_probe_bridge(struct platform_device 
*pdev)
 
pci_scan_child_bus(bus);
pci_assign_unassigned_bus_resources(bus);
+   list_for_each_entry(child, >children, node)
+   pcie_bus_configure_settings(child);
pci_bus_add_devices(bus);
return 0;
 

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v1 1/3] PCI: xilinx: Configure PCIe MPS settings

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelg...@google.com>

Make sure PCIe MPS settings are valid when we enumerate a new hierarchy.

Based-on-patch-by: Jon Mason <jon.ma...@broadcom.com>
Signed-off-by: Bjorn Helgaas <bhelg...@google.com>
---
 drivers/pci/host/pcie-xilinx.c |4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/host/pcie-xilinx.c b/drivers/pci/host/pcie-xilinx.c
index c8616fadccf1..7f030f5d750b 100644
--- a/drivers/pci/host/pcie-xilinx.c
+++ b/drivers/pci/host/pcie-xilinx.c
@@ -632,7 +632,7 @@ static int xilinx_pcie_probe(struct platform_device *pdev)
 {
struct device *dev = >dev;
struct xilinx_pcie_port *port;
-   struct pci_bus *bus;
+   struct pci_bus *bus, *child;
int err;
resource_size_t iobase = 0;
LIST_HEAD(res);
@@ -686,6 +686,8 @@ static int xilinx_pcie_probe(struct platform_device *pdev)
 #ifndef CONFIG_MICROBLAZE
pci_fixup_irqs(pci_common_swizzle, of_irq_parse_and_map_pci);
 #endif
+   list_for_each_entry(child, >children, node)
+   pcie_bus_configure_settings(child);
pci_bus_add_devices(bus);
return 0;
 

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v1 0/3] PCI: Configure PCIe MPS settings

[Bjorn Helgaas]

[Some of you will get this twice because Gmail and I aren't getting along
today; sorry]

The PCI core doesn't configure the PCIe MPS settings by itself.  Each
host bridge driver has to call pcie_bus_configure_settings() to make
this happen.

Jon fixed this already for pcie-iproc.c.  I propose these similar
patches for other drivers.

HV guys, I included you because create_root_hv_pci_bus() is one place
that calls pci_scan_child_bus() but does not call
pcie_bus_configure_settings().  I know you probably don't strictly
*need* to configure MPS settings in a paravirtual front-end, but the
PCI core does other device configuration in this path:

  pci_scan_child_bus
pci_scan_slot
  pci_scan_single_device
pci_device_add
  pci_configure_device
  pci_init_capabilities

and I would like to eventually migrate the MPS configuration into that
same path.  Since we do this other configuration for HV devices
already, I think pcie_bus_configure_settings() should also work (even
if it ends up not doing anything to real devices).

It would make that eventual migration easier if all the
pci_scan_child_bus() callers had the same pattern of calling
pcie_bus_configure_settings().

So would it make sense to make a similar patch for HV?  It looks easy
to add it to create_root_hv_pci_bus(), but I don't know exactly what
to do about pci_devices_present_work().

---

Bjorn Helgaas (3):
  PCI: xilinx: Configure PCIe MPS settings
  PCI: versatile: Configure PCIe MPS settings
  PCI: xgene: Configure PCIe MPS settings


 drivers/pci/host/pci-versatile.c |4 +++-
 drivers/pci/host/pci-xgene.c |4 +++-
 drivers/pci/host/pcie-xilinx.c   |4 +++-
 3 files changed, 9 insertions(+), 3 deletions(-)
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v1 3/3] PCI: xgene: Configure PCIe MPS settings

[Bjorn Helgaas]

Make sure PCIe MPS settings are valid when we enumerate a new hierarchy.

Based-on-patch-by: Jon Mason <jon.ma...@broadcom.com>
Signed-off-by: Bjorn Helgaas <bhelg...@google.com>
---
 drivers/pci/host/pci-xgene.c |4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/host/pci-xgene.c b/drivers/pci/host/pci-xgene.c
index 7c3b54b9eb17..8091a8778ae7 100644
--- a/drivers/pci/host/pci-xgene.c
+++ b/drivers/pci/host/pci-xgene.c
@@ -638,7 +638,7 @@ static int xgene_pcie_probe_bridge(struct platform_device 
*pdev)
struct device_node *dn = dev->of_node;
struct xgene_pcie_port *port;
resource_size_t iobase = 0;
-   struct pci_bus *bus;
+   struct pci_bus *bus, *child;
int ret;
LIST_HEAD(res);
 
@@ -681,6 +681,8 @@ static int xgene_pcie_probe_bridge(struct platform_device 
*pdev)
 
pci_scan_child_bus(bus);
pci_assign_unassigned_bus_resources(bus);
+   list_for_each_entry(child, >children, node)
+   pcie_bus_configure_settings(child);
pci_bus_add_devices(bus);
return 0;
 

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v1 2/3] PCI: versatile: Configure PCIe MPS settings

[Bjorn Helgaas]

Make sure PCIe MPS settings are valid when we enumerate a new hierarchy.

Based-on-patch-by: Jon Mason <jon.ma...@broadcom.com>
Signed-off-by: Bjorn Helgaas <bhelg...@google.com>
---
 drivers/pci/host/pci-versatile.c |4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/host/pci-versatile.c b/drivers/pci/host/pci-versatile.c
index b7dc07002f13..5ebee7d37ff5 100644
--- a/drivers/pci/host/pci-versatile.c
+++ b/drivers/pci/host/pci-versatile.c
@@ -124,7 +124,7 @@ static int versatile_pci_probe(struct platform_device *pdev)
int ret, i, myslot = -1;
u32 val;
void __iomem *local_pci_cfg_base;
-   struct pci_bus *bus;
+   struct pci_bus *bus, *child;
LIST_HEAD(pci_res);
 
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
@@ -204,6 +204,8 @@ static int versatile_pci_probe(struct platform_device *pdev)
 
pci_fixup_irqs(pci_common_swizzle, of_irq_parse_and_map_pci);
pci_assign_unassigned_bus_resources(bus);
+   list_for_each_entry(child, >children, node)
+   pcie_bus_configure_settings(child);
pci_bus_add_devices(bus);
 
return 0;

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v1 1/3] PCI: xilinx: Configure PCIe MPS settings

[Bjorn Helgaas]

Make sure PCIe MPS settings are valid when we enumerate a new hierarchy.

Based-on-patch-by: Jon Mason <jon.ma...@broadcom.com>
Signed-off-by: Bjorn Helgaas <bhelg...@google.com>
---
 drivers/pci/host/pcie-xilinx.c |4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/host/pcie-xilinx.c b/drivers/pci/host/pcie-xilinx.c
index c8616fadccf1..7f030f5d750b 100644
--- a/drivers/pci/host/pcie-xilinx.c
+++ b/drivers/pci/host/pcie-xilinx.c
@@ -632,7 +632,7 @@ static int xilinx_pcie_probe(struct platform_device *pdev)
 {
struct device *dev = >dev;
struct xilinx_pcie_port *port;
-   struct pci_bus *bus;
+   struct pci_bus *bus, *child;
int err;
resource_size_t iobase = 0;
LIST_HEAD(res);
@@ -686,6 +686,8 @@ static int xilinx_pcie_probe(struct platform_device *pdev)
 #ifndef CONFIG_MICROBLAZE
pci_fixup_irqs(pci_common_swizzle, of_irq_parse_and_map_pci);
 #endif
+   list_for_each_entry(child, >children, node)
+   pcie_bus_configure_settings(child);
pci_bus_add_devices(bus);
return 0;
 

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v1 0/3] PCI: Configure PCIe MPS settings

[Bjorn Helgaas]

The PCI core doesn't configure the PCIe MPS settings by itself.  Each host
bridge driver has to call pcie_bus_configure_settings() to make this
happen.

Jon fixed this for pcie-iproc.c.  I propose these similar patches for other
drivers.   

HV guys, I included you because create_root_hv_pci_bus() is one place that
calls pci_scan_child_bus() but does not call pcie_bus_configure_settings().
I know you probably don't strictly *need* to configure MPS settings in a
paravirtual front-end, but the PCI core does other device configuration in
this path:

  pci_scan_child_bus
pci_scan_slot
  pci_scan_single_device
pci_device_add
  pci_configure_device
  pci_init_capabilities

and I would like to eventually migrate the MPS configuration into that same
path.  Since we do this other configuration for HV devices already, I think
pcie_bus_configure_settings() should also work (even if it ends up not
doing anything to real devices).

It would make that eventual migration easier if all the
pci_scan_child_bus() callers had the same pattern of calling
pcie_bus_configure_settings().

So would it make sense to make a similar patch for HV?  It looks easy to
add it to create_root_hv_pci_bus(), but I don't know exactly what to do
about pci_devices_present_work().

---

Bjorn Helgaas (3):
  PCI: xilinx: Configure PCIe MPS settings
  PCI: versatile: Configure PCIe MPS settings
  PCI: xgene: Configure PCIe MPS settings


 drivers/pci/host/pci-versatile.c |4 +++-
 drivers/pci/host/pci-xgene.c |4 +++-
 drivers/pci/host/pcie-xilinx.c   |4 +++-
 3 files changed, 9 insertions(+), 3 deletions(-)
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] pci-hyperv: use kmalloc to allocate hypercall params buffer

[Bjorn Helgaas]

On Tue, Nov 08, 2016 at 02:04:38PM -0800, Long Li wrote:
> From: Long Li 
> 
> hv_do_hypercall assumes that we pass a segment from a physically
> continuous buffer. Buffer allocated on the stack may not work if
> CONFIG_VMAP_STACK=y is set.
> 
> Change to use kmalloc to allocate this buffer.
> 
> The v2 patch adds locking to access the pre-allocated buffer.
> 
> Signed-off-by: Long Li 
> Reported-by: Haiyang Zhang 

Applied with KY's ack to pci/host-hv, thanks!

> ---
>  drivers/pci/host/pci-hyperv.c | 29 +++--
>  1 file changed, 19 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
> index 763ff87..ca553df 100644
> --- a/drivers/pci/host/pci-hyperv.c
> +++ b/drivers/pci/host/pci-hyperv.c
> @@ -378,6 +378,8 @@ struct hv_pcibus_device {
>   struct msi_domain_info msi_info;
>   struct msi_controller msi_chip;
>   struct irq_domain *irq_domain;
> + struct retarget_msi_interrupt retarget_msi_interrupt_params;
> + spinlock_t retarget_msi_interrupt_lock;;
>  };
>  
>  /*
> @@ -774,34 +776,40 @@ void hv_irq_unmask(struct irq_data *data)
>  {
>   struct msi_desc *msi_desc = irq_data_get_msi_desc(data);
>   struct irq_cfg *cfg = irqd_cfg(data);
> - struct retarget_msi_interrupt params;
> + struct retarget_msi_interrupt *params;
>   struct hv_pcibus_device *hbus;
>   struct cpumask *dest;
>   struct pci_bus *pbus;
>   struct pci_dev *pdev;
>   int cpu;
> + unsigned long flags;
>  
>   dest = irq_data_get_affinity_mask(data);
>   pdev = msi_desc_to_pci_dev(msi_desc);
>   pbus = pdev->bus;
>   hbus = container_of(pbus->sysdata, struct hv_pcibus_device, sysdata);
>  
> - memset(, 0, sizeof(params));
> - params.partition_id = HV_PARTITION_ID_SELF;
> - params.source = 1; /* MSI(-X) */
> - params.address = msi_desc->msg.address_lo;
> - params.data = msi_desc->msg.data;
> - params.device_id = (hbus->hdev->dev_instance.b[5] << 24) |
> + spin_lock_irqsave(>retarget_msi_interrupt_lock, flags);
> +
> + params = >retarget_msi_interrupt_params;
> + memset(params, 0, sizeof(*params));
> + params->partition_id = HV_PARTITION_ID_SELF;
> + params->source = 1; /* MSI(-X) */
> + params->address = msi_desc->msg.address_lo;
> + params->data = msi_desc->msg.data;
> + params->device_id = (hbus->hdev->dev_instance.b[5] << 24) |
>  (hbus->hdev->dev_instance.b[4] << 16) |
>  (hbus->hdev->dev_instance.b[7] << 8) |
>  (hbus->hdev->dev_instance.b[6] & 0xf8) |
>  PCI_FUNC(pdev->devfn);
> - params.vector = cfg->vector;
> + params->vector = cfg->vector;
>  
>   for_each_cpu_and(cpu, dest, cpu_online_mask)
> - params.vp_mask |= (1ULL << vmbus_cpu_number_to_vp_number(cpu));
> + params->vp_mask |= (1ULL << vmbus_cpu_number_to_vp_number(cpu));
> +
> + hv_do_hypercall(HVCALL_RETARGET_INTERRUPT, params, NULL);
>  
> - hv_do_hypercall(HVCALL_RETARGET_INTERRUPT, , NULL);
> + spin_unlock_irqrestore(>retarget_msi_interrupt_lock, flags);
>  
>   pci_msi_unmask_irq(data);
>  }
> @@ -2186,6 +2194,7 @@ static int hv_pci_probe(struct hv_device *hdev,
>   INIT_LIST_HEAD(>resources_for_children);
>   spin_lock_init(>config_lock);
>   spin_lock_init(>device_list_lock);
> + spin_lock_init(>retarget_msi_interrupt_lock);
>   sema_init(>enum_sem, 1);
>   init_completion(>remove_event);
>  
> -- 
> 2.7.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] pci-hyperv: use kmalloc to allocate hypercall params buffer

[Bjorn Helgaas]

On Tue, Nov 08, 2016 at 02:04:38PM -0800, Long Li wrote:
> From: Long Li 
> 
> hv_do_hypercall assumes that we pass a segment from a physically
> continuous buffer. Buffer allocated on the stack may not work if
> CONFIG_VMAP_STACK=y is set.
> 
> Change to use kmalloc to allocate this buffer.
> 
> The v2 patch adds locking to access the pre-allocated buffer.
> 
> Signed-off-by: Long Li 
> Reported-by: Haiyang Zhang 

Waiting for a maintainer ack for this.

$ ./scripts/get_maintainer.pl -f drivers/pci/host/pci-hyperv.c
"K. Y. Srinivasan"  (maintainer:Hyper-V CORE AND DRIVERS)
Haiyang Zhang  (maintainer:Hyper-V CORE AND DRIVERS)
...

> ---
>  drivers/pci/host/pci-hyperv.c | 29 +++--
>  1 file changed, 19 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
> index 763ff87..ca553df 100644
> --- a/drivers/pci/host/pci-hyperv.c
> +++ b/drivers/pci/host/pci-hyperv.c
> @@ -378,6 +378,8 @@ struct hv_pcibus_device {
>   struct msi_domain_info msi_info;
>   struct msi_controller msi_chip;
>   struct irq_domain *irq_domain;
> + struct retarget_msi_interrupt retarget_msi_interrupt_params;
> + spinlock_t retarget_msi_interrupt_lock;;
>  };
>  
>  /*
> @@ -774,34 +776,40 @@ void hv_irq_unmask(struct irq_data *data)
>  {
>   struct msi_desc *msi_desc = irq_data_get_msi_desc(data);
>   struct irq_cfg *cfg = irqd_cfg(data);
> - struct retarget_msi_interrupt params;
> + struct retarget_msi_interrupt *params;
>   struct hv_pcibus_device *hbus;
>   struct cpumask *dest;
>   struct pci_bus *pbus;
>   struct pci_dev *pdev;
>   int cpu;
> + unsigned long flags;
>  
>   dest = irq_data_get_affinity_mask(data);
>   pdev = msi_desc_to_pci_dev(msi_desc);
>   pbus = pdev->bus;
>   hbus = container_of(pbus->sysdata, struct hv_pcibus_device, sysdata);
>  
> - memset(, 0, sizeof(params));
> - params.partition_id = HV_PARTITION_ID_SELF;
> - params.source = 1; /* MSI(-X) */
> - params.address = msi_desc->msg.address_lo;
> - params.data = msi_desc->msg.data;
> - params.device_id = (hbus->hdev->dev_instance.b[5] << 24) |
> + spin_lock_irqsave(>retarget_msi_interrupt_lock, flags);
> +
> + params = >retarget_msi_interrupt_params;
> + memset(params, 0, sizeof(*params));
> + params->partition_id = HV_PARTITION_ID_SELF;
> + params->source = 1; /* MSI(-X) */
> + params->address = msi_desc->msg.address_lo;
> + params->data = msi_desc->msg.data;
> + params->device_id = (hbus->hdev->dev_instance.b[5] << 24) |
>  (hbus->hdev->dev_instance.b[4] << 16) |
>  (hbus->hdev->dev_instance.b[7] << 8) |
>  (hbus->hdev->dev_instance.b[6] & 0xf8) |
>  PCI_FUNC(pdev->devfn);
> - params.vector = cfg->vector;
> + params->vector = cfg->vector;
>  
>   for_each_cpu_and(cpu, dest, cpu_online_mask)
> - params.vp_mask |= (1ULL << vmbus_cpu_number_to_vp_number(cpu));
> + params->vp_mask |= (1ULL << vmbus_cpu_number_to_vp_number(cpu));
> +
> + hv_do_hypercall(HVCALL_RETARGET_INTERRUPT, params, NULL);
>  
> - hv_do_hypercall(HVCALL_RETARGET_INTERRUPT, , NULL);
> + spin_unlock_irqrestore(>retarget_msi_interrupt_lock, flags);
>  
>   pci_msi_unmask_irq(data);
>  }
> @@ -2186,6 +2194,7 @@ static int hv_pci_probe(struct hv_device *hdev,
>   INIT_LIST_HEAD(>resources_for_children);
>   spin_lock_init(>config_lock);
>   spin_lock_init(>device_list_lock);
> + spin_lock_init(>retarget_msi_interrupt_lock);
>   sema_init(>enum_sem, 1);
>   init_completion(>remove_event);
>  
> -- 
> 2.7.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 0/3] PCI: hv: clean-up and 2 fixes to the hot-remove case

[Bjorn Helgaas]

On Thu, Nov 10, 2016 at 07:16:22AM +, Dexuan Cui wrote:
> PATCH 1 is just a clean-up. There should be no functional change.
> 
> PATCH 2 and 3 are for device hot-remove case. 
> Currently the driver will stop working or even cause panic, if we do
> hot add/remove quickly a few times. With the 2 patches, everything works
> reliably in my tests now.
> 
> There can be still a potential issue with hot-remove when we unload 
> the driver at the same time. That would require more work of proper
> synchronization among the 3 paths: the .probe/.remove, the channel callback,
> and the offloaded hv_pci_devices_present()/hv_eject_device_work().
> 
> But for now, PATCH 2 and 3 do improve the situation a lot.
> 
> Dexuan Cui (3):
>   PCI: hv: use the correct buffer size in new_pcichild_device()
>   PCI: hv: fix hv_pci_remove() for hot-remove
>   PCI: hv: delete the device earlier from hbus->children for hot-remove
> 
>  drivers/pci/host/pci-hyperv.c | 67 
> ++-
>  1 file changed, 40 insertions(+), 27 deletions(-)

I applied all three of these to pci/host-hv for v4.10, thanks!

Jake, I converted your "looks good to me" to Reviewed-by tags.

K. Y., I added your acks on 2 & 3.  If you acked 1, I missed it.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/2 v3] pci-hyperv: properly handle pci bus remove

[Bjorn Helgaas]

On Mon, Oct 03, 2016 at 11:42:47PM -0700, Long Li wrote:
> From: Long Li 
> 
> hv_pci_devices_present is called in hv_pci_remove when we remove a PCI device 
> from host (e.g. by disabling SRIOV on a device). In hv_pci_remove, the bus is 
> already removed before the call, so we don't need to rescan the bus in the 
> workqueue scheduled from hv_pci_devices_present. By introducing status 
> hv_pcibus_removed, we can avoid this situation.
> 
> Signed-off-by: Long Li 
> Tested-by: Cathy Avery 
> Reported-by: Xiaofeng Wang 

I need an ack from the Hyper-V maintainers.  I see acks for previous
versions, but I don't know whether you've changed things that would
invalidate those acks.  If the acks still apply, please include them
and repost these patches.

Also, please run "git log --oneline drivers/pci/host/pci-hyperv.c" and make
your subject line match the previous ones.

> ---
>  drivers/pci/host/pci-hyperv.c | 20 +---
>  1 file changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
> index a8deeca..4a37598 100644
> --- a/drivers/pci/host/pci-hyperv.c
> +++ b/drivers/pci/host/pci-hyperv.c
> @@ -348,6 +348,7 @@ enum hv_pcibus_state {
>   hv_pcibus_init = 0,
>   hv_pcibus_probed,
>   hv_pcibus_installed,
> + hv_pcibus_removed,
>   hv_pcibus_maximum
>  };
>  
> @@ -1481,13 +1482,24 @@ static void pci_devices_present_work(struct 
> work_struct *work)
>   put_pcichild(hpdev, hv_pcidev_ref_initial);
>   }
>  
> - /* Tell the core to rescan bus because there may have been changes. */
> - if (hbus->state == hv_pcibus_installed) {
> + switch (hbus->state) {
> + case hv_pcibus_installed:
> + /*
> +  * Tell the core to rescan bus
> +  * because there may have been changes.
> +  */
>   pci_lock_rescan_remove();
>   pci_scan_child_bus(hbus->pci_bus);
>   pci_unlock_rescan_remove();
> - } else {
> + break;
> +
> + case hv_pcibus_init:
> + case hv_pcibus_probed:
>   survey_child_resources(hbus);
> + break;
> +
> + default:
> + break;
>   }
>  
>   up(>enum_sem);
> @@ -2163,6 +2175,7 @@ static int hv_pci_probe(struct hv_device *hdev,
>   hbus = kzalloc(sizeof(*hbus), GFP_KERNEL);
>   if (!hbus)
>   return -ENOMEM;
> + hbus->state = hv_pcibus_init;
>  
>   /*
>* The PCI bus "domain" is what is called "segment" in ACPI and
> @@ -2305,6 +2318,7 @@ static int hv_pci_remove(struct hv_device *hdev)
>   pci_stop_root_bus(hbus->pci_bus);
>   pci_remove_root_bus(hbus->pci_bus);
>   pci_unlock_rescan_remove();
> + hbus->state = hv_pcibus_removed;
>   }
>  
>   ret = hv_send_resources_released(hdev);
> -- 
> 1.8.5.6
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] PCI: hv: Make unnecessarily global IRQ masking functions static

[Bjorn Helgaas]

On Mon, Oct 31, 2016 at 12:04:09PM +0100, Tobias Klauser wrote:
> Make hv_irq_mask and hv_irq_unmask static as they are only used in
> pci-hyperv.c
> 
> This fixes a sparse warning.
> 
> Signed-off-by: Tobias Klauser 

Applied with KY's ack to pci/host-hv for v4.10, thanks!

> ---
>  drivers/pci/host/pci-hyperv.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
> index 763ff8745828..06c98695c06c 100644
> --- a/drivers/pci/host/pci-hyperv.c
> +++ b/drivers/pci/host/pci-hyperv.c
> @@ -755,7 +755,7 @@ static int hv_set_affinity(struct irq_data *data, const 
> struct cpumask *dest,
>   return parent->chip->irq_set_affinity(parent, dest, force);
>  }
>  
> -void hv_irq_mask(struct irq_data *data)
> +static void hv_irq_mask(struct irq_data *data)
>  {
>   pci_msi_mask_irq(data);
>  }
> @@ -770,7 +770,7 @@ void hv_irq_mask(struct irq_data *data)
>   * is built out of this PCI bus's instance GUID and the function
>   * number of the device.
>   */
> -void hv_irq_unmask(struct irq_data *data)
> +static void hv_irq_unmask(struct irq_data *data)
>  {
>   struct msi_desc *msi_desc = irq_data_get_msi_desc(data);
>   struct irq_cfg *cfg = irqd_cfg(data);
> -- 
> 2.9.0
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/2 v2] pci-hyperv: properly handle pci bus remove

[Bjorn Helgaas]

On Wed, Sep 14, 2016 at 07:10:01PM -0700, Long Li wrote:
> From: Long Li 
> 
> hv_pci_devices_present is called in hv_pci_remove when we remove a PCI device 
> from host (e.g. by disabling SRIOV on a device). In hv_pci_remove, the bus is 
> already removed before the call, so we don't need to rescan the bus in the 
> workqueue scheduled from hv_pci_devices_present. By introducing status 
> hv_pcibus_removed, we can avoid this situation.
> 
> The patch fixes the following kernel panic.
> 
> [  383.853124] Workqueue: events pci_devices_present_work [pci_hyperv]
> [  383.853124] task: 88007f5f8000 ti: 88007f60 task.ti:
> 88007f60
> [  383.853124] RIP: 0010:[]  []
> pci_is_pcie+0x6/0x20
> [  383.853124] RSP: 0018:88007f603d38  EFLAGS: 00010206
> [  383.853124] RAX: 88007f5f8000 RBX: 642f3d4854415056 RCX:
> 88007f603fd8
> [  383.853124] RDX:  RSI:  RDI:
> 642f3d4854415056
> [  383.853124] RBP: 88007f603d68 R08: 0246 R09:
> a045eb9e
> [  383.853124] R10: 88007b419a80 R11: ea0001c0ef40 R12:
> 880003ee1c00
> [  383.853124] R13: 63702f30303a3137 R14:  R15:
> 0246
> [  383.853124] FS:  () GS:88007b40()
> knlGS:
> [  383.853124] CS:  0010 DS:  ES:  CR0: 80050033
> [  383.853124] CR2: 7f68b3f52350 CR3: 03546000 CR4:
> 000406f0
> [  383.853124] DR0:  DR1:  DR2:
> 
> [  383.853124] DR3:  DR6: 0ff0 DR7:
> 0400
> [  383.853124] Stack:
> [  383.853124]  88007f603d68 8134db17 0008
> 880003ee1c00
> [  383.853124]  63702f30303a3137 880003d8edb8 88007f603da0
> 8134ee2d
> [  383.853124]  880003d8ed00 88007f603dd8 880075fec320
> 880003d8edb8
> [  383.853124] Call Trace:
> [  383.853124]  [] ? pci_scan_slot+0x27/0x140
> [  383.853124]  [] pci_scan_child_bus+0x3d/0x150
> [  383.853124]  []
> pci_devices_present_work+0x3ea/0x400 [pci_hyperv]
> [  383.853124]  [] process_one_work+0x17b/0x470
> [  383.853124]  [] worker_thread+0x126/0x410
> [  383.853124]  [] ? rescuer_thread+0x460/0x460
> [  383.853124]  [] kthread+0xcf/0xe0
> [  383.853124]  [] ?
> kthread_create_on_node+0x140/0x140
> [  383.853124]  [] ret_from_fork+0x58/0x90
> [  383.853124]  [] ?
> kthread_create_on_node+0x140/0x140
> [  383.853124] Code: 89 e5 5d 25 f0 00 00 00 c1 f8 04 c3 66 0f 1f 84 00
> 00 00 00 00 66 66 66 66 90 55 0f b6 47 4a 48 89 e5 5d c3 90 66 66 66 66
> 90 55 <80> 7f 4a 00 48 89 e5 5d 0f 95 c0 c3 0f 1f 40 00 66 2e 0f 1f 84
> [  383.853124] RIP  [] pci_is_pcie+0x6/0x20
> [  383.853124]  RSP 

Personally, I would remove the timestamps and addresses from this trace
because I don't think they contribute to diagnosing the problem.

> Signed-off-by: Long Li 

I'm ready to apply these but am waiting for an ack from the maintainers
listed in MAINTAINERS (feel free to update that if it's out of date).

> ---
>  drivers/pci/host/pci-hyperv.c | 20 +---
>  1 file changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
> index a8deeca..4a37598 100644
> --- a/drivers/pci/host/pci-hyperv.c
> +++ b/drivers/pci/host/pci-hyperv.c
> @@ -348,6 +348,7 @@ enum hv_pcibus_state {
>   hv_pcibus_init = 0,
>   hv_pcibus_probed,
>   hv_pcibus_installed,
> + hv_pcibus_removed,
>   hv_pcibus_maximum
>  };
>  
> @@ -1481,13 +1482,24 @@ static void pci_devices_present_work(struct 
> work_struct *work)
>   put_pcichild(hpdev, hv_pcidev_ref_initial);
>   }
>  
> - /* Tell the core to rescan bus because there may have been changes. */
> - if (hbus->state == hv_pcibus_installed) {
> + switch (hbus->state) {
> + case hv_pcibus_installed:
> + /*
> +  * Tell the core to rescan bus
> +  * because there may have been changes.
> +  */
>   pci_lock_rescan_remove();
>   pci_scan_child_bus(hbus->pci_bus);
>   pci_unlock_rescan_remove();
> - } else {
> + break;
> +
> + case hv_pcibus_init:
> + case hv_pcibus_probed:
>   survey_child_resources(hbus);
> + break;
> +
> + default:
> + break;
>   }
>  
>   up(>enum_sem);
> @@ -2163,6 +2175,7 @@ static int hv_pci_probe(struct hv_device *hdev,
>   hbus = kzalloc(sizeof(*hbus), GFP_KERNEL);
>   if (!hbus)
>   return -ENOMEM;
> + hbus->state = hv_pcibus_init;
>  
>   /*
>* The PCI bus "domain" is what is called "segment" in ACPI and
> @@ -2305,6 +2318,7 @@ static int hv_pci_remove(struct hv_device *hdev)
>   pci_stop_root_bus(hbus->pci_bus);
>   pci_remove_root_bus(hbus->pci_bus);
>   

Re: [PATCH 0/5] PCI: hv: some minor bug fixes and cleanups

[Bjorn Helgaas]

On Tue, Aug 23, 2016 at 04:42:41AM +, Dexuan Cui wrote:
> 
> 1. use zero-length array to make the code more readable.
> 2. remove an unused struct member.
> 3. small error handling improvement to some error cases.
> 
> Dexuan Cui (5):
>   PCI: hv: use zero-length message in struct pci_packet
>   PCI: hv: use pci_function_description[0] in struct definitions
>   PCI: hv: remove the unused 'wrk' in struct hv_pcibus_device
>   PCI: hv: hv_compose_msi_msg: handle the 'ret' value
>   PCI: hv: hv_pci_generic_compl(): handle the error case
> 
>  drivers/pci/host/pci-hyperv.c | 56 
> ++-
>  1 file changed, 29 insertions(+), 27 deletions(-)

Applied with KY's ack to pci/host-hv for v4.9, thanks!
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 0/5] PCI: hv: some minor bug fixes and cleanups

[Bjorn Helgaas]

On Tue, Aug 23, 2016 at 04:42:41AM +, Dexuan Cui wrote:
> 
> 1. use zero-length array to make the code more readable.
> 2. remove an unused struct member.
> 3. small error handling improvement to some error cases.
> 
> Dexuan Cui (5):
>   PCI: hv: use zero-length message in struct pci_packet
>   PCI: hv: use pci_function_description[0] in struct definitions
>   PCI: hv: remove the unused 'wrk' in struct hv_pcibus_device
>   PCI: hv: hv_compose_msi_msg: handle the 'ret' value
>   PCI: hv: hv_pci_generic_compl(): handle the error case
> 
>  drivers/pci/host/pci-hyperv.c | 56 
> ++-
>  1 file changed, 29 insertions(+), 27 deletions(-)

I'm waiting for an ack from the Hyper-V maintainers:

  Hyper-V CORE AND DRIVERS
  M:  "K. Y. Srinivasan" 
  M:  Haiyang Zhang 
  L:  de...@linuxdriverproject.org
  S:  Maintained
  ...
  F:  drivers/pci/host/pci-hyperv.c
  ...
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH -next] PCI: hv: Use list_move_tail instead of list_del/list_add_tail

[Bjorn Helgaas]

On Thu, Jul 28, 2016 at 04:16:48PM +, Wei Yongjun wrote:
> Using list_move_tail() instead of list_del() + list_add_tail().
> 
> Signed-off-by: Wei Yongjun 

Applied to pci/host-hv for v4.9, thanks!

> ---
>  drivers/pci/host/pci-hyperv.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
> index 6955ffdb..a8deeca 100644
> --- a/drivers/pci/host/pci-hyperv.c
> +++ b/drivers/pci/host/pci-hyperv.c
> @@ -1466,8 +1466,7 @@ static void pci_devices_present_work(struct work_struct 
> *work)
>   if (hpdev->reported_missing) {
>   found = true;
>   put_pcichild(hpdev, hv_pcidev_ref_childlist);
> - list_del(>list_entry);
> - list_add_tail(>list_entry, );
> + list_move_tail(>list_entry, );
>   break;
>   }
>   }
> 
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] PCI: hv: Fix interrupt cleanup path

[Bjorn Helgaas]

On Tue, Jul 12, 2016 at 11:31:24AM -0400, Cathy Avery wrote:
> SR-IOV disabled from the host causes a memory leak.
> pci-hyperv usually first receives a PCI_EJECT notification
> and then proceeds to delete the hpdev list entry in
> hv_eject_device_work(). Later in hv_msi_free() since the
> device is no longer on the device list hpdev is NULL
> and hv_msi_free returns without freeing int_desc as part of
> hv_int_desc_free().
> 
> Signed-off-by: Cathy Avery 

Applied with Jake's ack to pci/host-hv for v4.8, thanks!

For some reason, Jake's ack appears in patchwork and in my personal
email, but I don't see it on the mailing list.  Maybe something in
http://vger.kernel.org/majordomo-info.html#taboo is relevant.

> ---
>  drivers/pci/host/pci-hyperv.c | 14 --
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
> index 7e9b2de..449d053 100644
> --- a/drivers/pci/host/pci-hyperv.c
> +++ b/drivers/pci/host/pci-hyperv.c
> @@ -732,16 +732,18 @@ static void hv_msi_free(struct irq_domain *domain, 
> struct msi_domain_info *info,
>  
>   pdev = msi_desc_to_pci_dev(msi);
>   hbus = info->data;
> - hpdev = get_pcichild_wslot(hbus, devfn_to_wslot(pdev->devfn));
> - if (!hpdev)
> + int_desc = irq_data_get_irq_chip_data(irq_data);
> + if (!int_desc)
>   return;
>  
> - int_desc = irq_data_get_irq_chip_data(irq_data);
> - if (int_desc) {
> - irq_data->chip_data = NULL;
> - hv_int_desc_free(hpdev, int_desc);
> + irq_data->chip_data = NULL;
> + hpdev = get_pcichild_wslot(hbus, devfn_to_wslot(pdev->devfn));
> + if (!hpdev) {
> + kfree(int_desc);
> + return;
>   }
>  
> + hv_int_desc_free(hpdev, int_desc);
>   put_pcichild(hpdev, hv_pcidev_ref_by_slot);
>  }
>  
> -- 
> 2.5.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 0/2] PCI: hv: fix a couple of issues in hv_pci_onchannelcallback()

[Bjorn Helgaas]

On Fri, Jun 10, 2016 at 06:53:36PM -0500, Bjorn Helgaas wrote:
> On Mon, May 30, 2016 at 04:17:57PM +0200, Vitaly Kuznetsov wrote:
> > kmemleak helped me to identify a memory leak in hv_pci_onchannelcallback()
> > and while fixing it I stumbled upon an unrelated issue(s) there.
> > 
> > Vitaly Kuznetsov (2):
> >   PCI: hv: don't leak buffer in hv_pci_onchannelcallback()
> >   PCI: hv: handle all pending messages in hv_pci_onchannelcallback()
> 
> I applied both to for-linus for v4.7 with Jake's acks, thanks, Vitaly.

Somehow I must have been thinking these were fixes for things we
merged or broke during the v4.7 merge window, but that doesn't look
like the case.  So I'm going to merge these for v4.8 instead, on the
theory that the v4.7-rc cycles are primarily for stabilization.

Bjorn
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 0/2] PCI: hv: fix a couple of issues in hv_pci_onchannelcallback()

[Bjorn Helgaas]

On Mon, May 30, 2016 at 04:17:57PM +0200, Vitaly Kuznetsov wrote:
> kmemleak helped me to identify a memory leak in hv_pci_onchannelcallback()
> and while fixing it I stumbled upon an unrelated issue(s) there.
> 
> Vitaly Kuznetsov (2):
>   PCI: hv: don't leak buffer in hv_pci_onchannelcallback()
>   PCI: hv: handle all pending messages in hv_pci_onchannelcallback()

I applied both to for-linus for v4.7 with Jake's acks, thanks, Vitaly.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 0/2] PCI: hv: fix a couple of issues in hv_pci_onchannelcallback()

[Bjorn Helgaas]

On Fri, Jun 10, 2016 at 02:05:33PM +0200, Vitaly Kuznetsov wrote:
> Vitaly Kuznetsov  writes:
> 
> > kmemleak helped me to identify a memory leak in hv_pci_onchannelcallback()
> > and while fixing it I stumbled upon an unrelated issue(s) there.
> >
> > Vitaly Kuznetsov (2):
> >   PCI: hv: don't leak buffer in hv_pci_onchannelcallback()
> >   PCI: hv: handle all pending messages in hv_pci_onchannelcallback()
> >
> 
> Bjorn,
> 
> sorry for the ping but with both patches acked by Jake is there anything
> else required for this series to get merged? It would be nice to have
> these fixes in 4.7 but even knowing that they're queued for 4.8 is OK.

Nothing else required, but I'm glad you mentioned that these should go
in v4.7.  By default I merge things to -next, which would be for v4.8
right now.

Bjorn
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH [RFC]] PCI: hv: add explicit fencing to config space access

[Bjorn Helgaas]

On Tue, May 03, 2016 at 02:22:00PM +0200, Vitaly Kuznetsov wrote:
> I'm trying to pass-through Broadcom BCM5720 NIC (Dell Device 1f5b) on Dell
> R720 server. Everything works fine when target VM has only one CPU, but
> SMP guests reboot when NIC driver is trying to access PCI config space
> (with  hv_pcifront_read_config/hv_pcifront_write_config). The reboot
> appears to be induced by the hypervisor and no crash is observed. Windows
> event logs are not helpful at all ('Virtual machine ... has quit
> unexpectedly'). The particular access point is always different and
> putting debug between them (printk/mdelay/...) moves the issue further
> away. The server model affects the issue as well: on Dell R420 I'm able to
> pass-through BCM5720 NIC to SMP guests without issues.
> 
> While I'm obviously failing to reveal the essence of the issue I was able
> to come up with a (possible) solution: if explicit fencing is put to
> hv_pcifront_read_config/hv_pcifront_write_config the issue goes away. The
> essential minimum is rmb() at the end on _hv_pcifront_read_config() and
> wmb() at the end of _hv_pcifront_write_config() but I'm not confident it
> will be sufficient for all hardware. I suggest the following fencing:
> 1) wmb()/mb() between choosing the function and writing to its space.
> 2) mb() before releasing the spinlock in both _hv_pcifront_read_config()/
>_hv_pcifront_write_config to ensure that consecutive reads/writes to
>   the space won't get re-ordered as drivers may count on that.
> Config space access is not supposed to be performance-critical so this
> explicit fencing is not supposed to bring any slowdown.
> 
> Signed-off-by: Vitaly Kuznetsov 

Applied with Jake's ack to pci/host-hv for v4.7, thanks, Vitaly.

I changed "fence" to "barrier" in the changelog to follow the
common Linux terminology.

> ---
>  drivers/pci/host/pci-hyperv.c | 14 ++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
> index c17e792..7e9b2de 100644
> --- a/drivers/pci/host/pci-hyperv.c
> +++ b/drivers/pci/host/pci-hyperv.c
> @@ -553,6 +553,8 @@ static void _hv_pcifront_read_config(struct hv_pci_dev 
> *hpdev, int where,
>   spin_lock_irqsave(>hbus->config_lock, flags);
>   /* Choose the function to be read. (See comment above) */
>   writel(hpdev->desc.win_slot.slot, hpdev->hbus->cfg_addr);
> + /* Make sure the function was chosen before we start reading. */
> + mb();
>   /* Read from that function's config space. */
>   switch (size) {
>   case 1:
> @@ -565,6 +567,11 @@ static void _hv_pcifront_read_config(struct hv_pci_dev 
> *hpdev, int where,
>   *val = readl(addr);
>   break;
>   }
> + /*
> +  * Make sure the write was done before we release the spinlock
> +  * allowing consecutive reads/writes.
> +  */
> + mb();
>   spin_unlock_irqrestore(>hbus->config_lock, flags);
>   } else {
>   dev_err(>hbus->hdev->device,
> @@ -592,6 +599,8 @@ static void _hv_pcifront_write_config(struct hv_pci_dev 
> *hpdev, int where,
>   spin_lock_irqsave(>hbus->config_lock, flags);
>   /* Choose the function to be written. (See comment above) */
>   writel(hpdev->desc.win_slot.slot, hpdev->hbus->cfg_addr);
> + /* Make sure the function was chosen before we start writing. */
> + wmb();
>   /* Write to that function's config space. */
>   switch (size) {
>   case 1:
> @@ -604,6 +613,11 @@ static void _hv_pcifront_write_config(struct hv_pci_dev 
> *hpdev, int where,
>   writel(val, addr);
>   break;
>   }
> + /*
> +  * Make sure the write was done before we release the spinlock
> +  * allowing consecutive reads/writes.
> +  */
> + mb();
>   spin_unlock_irqrestore(>hbus->config_lock, flags);
>   } else {
>   dev_err(>hbus->hdev->device,
> -- 
> 2.5.5
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] PCI: hv: report resources release after stopping the bus

[Bjorn Helgaas]

On Fri, Apr 29, 2016 at 11:39:10AM +0200, Vitaly Kuznetsov wrote:
> Kernel hang is observed when pci-hyperv module is release with device
> drivers still attached. E.g. when I do 'rmmod pci_hyperv' with BCM5720
> device pass-through-ed (tg3 module) I see the following:
> 
>  NMI watchdog: BUG: soft lockup - CPU#1 stuck for 22s! [rmmod:2104]
>  ...
>  Call Trace:
>   [] tg3_read_mem+0x87/0x100 [tg3]
>   [] ? 0xa063f000
>   [] tg3_poll_fw+0x85/0x150 [tg3]
>   [] tg3_chip_reset+0x357/0x8c0 [tg3]
>   [] tg3_halt+0x3b/0x190 [tg3]
>   [] tg3_stop+0x171/0x230 [tg3]
>   ...
>   [] tg3_remove_one+0x90/0x140 [tg3]
>   [] pci_device_remove+0x39/0xc0
>   [] __device_release_driver+0xa1/0x160
>   [] device_release_driver+0x23/0x30
>   [] pci_stop_bus_device+0x8a/0xa0
>   [] pci_stop_root_bus+0x36/0x60
>   [] hv_pci_remove+0x238/0x260 [pci_hyperv]
> 
> The problem seems to be that we report local resources release before
> stopping the bus and removing devices from it and device drivers may
> try to perform some operations with these resources on shutdown. Move
> resources release report after we do pci_stop_root_bus().
> 
> Signed-off-by: Vitaly Kuznetsov 

Applied with Jake's ack to pci/host-hv for v4.7, thanks, Vitaly!

> ---
>  drivers/pci/host/pci-hyperv.c | 10 +-
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
> index f2559b6..c17e792 100644
> --- a/drivers/pci/host/pci-hyperv.c
> +++ b/drivers/pci/host/pci-hyperv.c
> @@ -2268,11 +2268,6 @@ static int hv_pci_remove(struct hv_device *hdev)
>  
>   hbus = hv_get_drvdata(hdev);
>  
> - ret = hv_send_resources_released(hdev);
> - if (ret)
> - dev_err(>device,
> - "Couldn't send resources released packet(s)\n");
> -
>   memset(_packet, 0, sizeof(pkt.teardown_packet));
>   init_completion(_pkt.host_event);
>   pkt.teardown_packet.completion_func = hv_pci_generic_compl;
> @@ -2295,6 +2290,11 @@ static int hv_pci_remove(struct hv_device *hdev)
>   pci_unlock_rescan_remove();
>   }
>  
> + ret = hv_send_resources_released(hdev);
> + if (ret)
> + dev_err(>device,
> + "Couldn't send resources released packet(s)\n");
> +
>   vmbus_close(hdev->channel);
>  
>   /* Delete any children which might still exist. */
> -- 
> 2.5.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v5 2/6] drivers:hv: Call vmbus_mmio_free() to reverse vmbus_mmio_allocate()

[Bjorn Helgaas]

On Tue, Apr 05, 2016 at 06:18:18PM -0700, Jake Oshins wrote:
> Existing code just called release_mem_region().  Adding a
> wrapper around it allows the more complex range tracking
> that is introduced later in this patch series.
> 
> Signed-off-by: Jake Oshins <ja...@microsoft.com>

With typo fix below,

Acked-by: Bjorn Helgaas <bhelg...@google.com>

This is mostly non-PCI, so I assume somebody else will merge this series.

> ---
>  drivers/hv/vmbus_drv.c  | 15 +++
>  drivers/pci/host/pci-hyperv.c   | 14 +++---
>  drivers/video/fbdev/hyperv_fb.c |  4 ++--
>  include/linux/hyperv.h  |  2 +-
>  4 files changed, 25 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index 799518b..60553c1 100644
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c
> @@ -1188,6 +1188,21 @@ exit:
>  EXPORT_SYMBOL_GPL(vmbus_allocate_mmio);
>  
>  /**
> + * vmbus_free_mmio() - Free a memory-mapped I/O range.
> + * @start:   Base address of region to release.
> + * @size:Size of the range to be allocated
> + *
> + * This function releases anything requested by
> + * vmbus_mmio_allocate().

s/vmbus_mmio_allocate/vmbus_allocate_mmio/, I think.

> + */
> +void vmbus_free_mmio(resource_size_t start, resource_size_t size)
> +{
> + release_mem_region(start, size);
> +
> +}
> +EXPORT_SYMBOL_GPL(vmbus_free_mmio);
> +
> +/**
>   * vmbus_cpu_number_to_vp_number() - Map CPU to VP.
>   * @cpu_number: CPU number in Linux terms
>   *
> diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
> index ed651ba..f2559b6 100644
> --- a/drivers/pci/host/pci-hyperv.c
> +++ b/drivers/pci/host/pci-hyperv.c
> @@ -1795,14 +1795,14 @@ static void hv_pci_free_bridge_windows(struct 
> hv_pcibus_device *hbus)
>  
>   if (hbus->low_mmio_space && hbus->low_mmio_res) {
>   hbus->low_mmio_res->flags |= IORESOURCE_BUSY;
> - release_mem_region(hbus->low_mmio_res->start,
> -resource_size(hbus->low_mmio_res));
> + vmbus_free_mmio(hbus->low_mmio_res->start,
> + resource_size(hbus->low_mmio_res));
>   }
>  
>   if (hbus->high_mmio_space && hbus->high_mmio_res) {
>   hbus->high_mmio_res->flags |= IORESOURCE_BUSY;
> - release_mem_region(hbus->high_mmio_res->start,
> -resource_size(hbus->high_mmio_res));
> + vmbus_free_mmio(hbus->high_mmio_res->start,
> + resource_size(hbus->high_mmio_res));
>   }
>  }
>  
> @@ -1880,8 +1880,8 @@ static int hv_pci_allocate_bridge_windows(struct 
> hv_pcibus_device *hbus)
>  
>  release_low_mmio:
>   if (hbus->low_mmio_res) {
> - release_mem_region(hbus->low_mmio_res->start,
> -resource_size(hbus->low_mmio_res));
> + vmbus_free_mmio(hbus->low_mmio_res->start,
> + resource_size(hbus->low_mmio_res));
>   }
>  
>   return ret;
> @@ -1924,7 +1924,7 @@ static int hv_allocate_config_window(struct 
> hv_pcibus_device *hbus)
>  
>  static void hv_free_config_window(struct hv_pcibus_device *hbus)
>  {
> - release_mem_region(hbus->mem_config->start, PCI_CONFIG_MMIO_LENGTH);
> + vmbus_free_mmio(hbus->mem_config->start, PCI_CONFIG_MMIO_LENGTH);
>  }
>  
>  /**
> diff --git a/drivers/video/fbdev/hyperv_fb.c b/drivers/video/fbdev/hyperv_fb.c
> index e2451bd..2fd49b2 100644
> --- a/drivers/video/fbdev/hyperv_fb.c
> +++ b/drivers/video/fbdev/hyperv_fb.c
> @@ -743,7 +743,7 @@ static int hvfb_getmem(struct hv_device *hdev, struct 
> fb_info *info)
>  err3:
>   iounmap(fb_virt);
>  err2:
> - release_mem_region(par->mem->start, screen_fb_size);
> + vmbus_free_mmio(par->mem->start, screen_fb_size);
>   par->mem = NULL;
>  err1:
>   if (!gen2vm)
> @@ -758,7 +758,7 @@ static void hvfb_putmem(struct fb_info *info)
>   struct hvfb_par *par = info->par;
>  
>   iounmap(info->screen_base);
> - release_mem_region(par->mem->start, screen_fb_size);
> + vmbus_free_mmio(par->mem->start, screen_fb_size);
>   par->mem = NULL;
>  }
>  
> diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
> index aa0fadc..ecd81c3 100644
> --- a/include/linux/hyperv.h
> +++ b/include/linux/hyperv.h
> @@ -1091,7 +1091,7 @@ int vmbus_allocate_mmio(struct resource **new, struct 
> hv_device *device_ob

Re: [PATCH v4 3/7] drivers:hv: Use new vmbus_mmio_free() from client drivers.

[Bjorn Helgaas]

Hi Jake,

On Fri, Apr 01, 2016 at 05:47:43PM -0700, Jake Oshins wrote:
> This patch modifies all the callers of vmbus_mmio_allocate()
> to call vmbus_mmio_free() instead of release_mem_region().

This changelog merely restates the C code.  Presumably there's some
important difference between release_mem_region() and
vmbus_mmio_free(), and we need a hint about what that is.

Oh, I see, there actually is no difference *yet*, but it's coming.
I'd combine this with patch 2.  Then the patch is obviously correct
all by itself, and the changelog for patch 2 makes clear what's
happening.

In changelogs, don't bother with "this patch does" or "this function
is introduced."  The context is obvious because the changelog is part
of the commit.  Write imperative sentences, e.g., "Call
vmbus_mmio_free() instead of release_mem_region()."

> Signed-off-by: Jake Oshins <ja...@microsoft.com>

I think this is the only change that touches PCI, so I assume this
series will be merged by somebody else.

Acked-by: Bjorn Helgaas <bhelg...@google.com>

> ---
>  drivers/pci/host/pci-hyperv.c   | 14 +++---
>  drivers/video/fbdev/hyperv_fb.c |  4 ++--
>  2 files changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
> index ed651ba..f2559b6 100644
> --- a/drivers/pci/host/pci-hyperv.c
> +++ b/drivers/pci/host/pci-hyperv.c
> @@ -1795,14 +1795,14 @@ static void hv_pci_free_bridge_windows(struct 
> hv_pcibus_device *hbus)
>  
>   if (hbus->low_mmio_space && hbus->low_mmio_res) {
>   hbus->low_mmio_res->flags |= IORESOURCE_BUSY;
> - release_mem_region(hbus->low_mmio_res->start,
> -resource_size(hbus->low_mmio_res));
> + vmbus_free_mmio(hbus->low_mmio_res->start,
> + resource_size(hbus->low_mmio_res));
>   }
>  
>   if (hbus->high_mmio_space && hbus->high_mmio_res) {
>   hbus->high_mmio_res->flags |= IORESOURCE_BUSY;
> - release_mem_region(hbus->high_mmio_res->start,
> -resource_size(hbus->high_mmio_res));
> + vmbus_free_mmio(hbus->high_mmio_res->start,
> + resource_size(hbus->high_mmio_res));
>   }
>  }
>  
> @@ -1880,8 +1880,8 @@ static int hv_pci_allocate_bridge_windows(struct 
> hv_pcibus_device *hbus)
>  
>  release_low_mmio:
>   if (hbus->low_mmio_res) {
> - release_mem_region(hbus->low_mmio_res->start,
> -resource_size(hbus->low_mmio_res));
> + vmbus_free_mmio(hbus->low_mmio_res->start,
> + resource_size(hbus->low_mmio_res));
>   }
>  
>   return ret;
> @@ -1924,7 +1924,7 @@ static int hv_allocate_config_window(struct 
> hv_pcibus_device *hbus)
>  
>  static void hv_free_config_window(struct hv_pcibus_device *hbus)
>  {
> - release_mem_region(hbus->mem_config->start, PCI_CONFIG_MMIO_LENGTH);
> + vmbus_free_mmio(hbus->mem_config->start, PCI_CONFIG_MMIO_LENGTH);
>  }
>  
>  /**
> diff --git a/drivers/video/fbdev/hyperv_fb.c b/drivers/video/fbdev/hyperv_fb.c
> index e2451bd..2fd49b2 100644
> --- a/drivers/video/fbdev/hyperv_fb.c
> +++ b/drivers/video/fbdev/hyperv_fb.c
> @@ -743,7 +743,7 @@ static int hvfb_getmem(struct hv_device *hdev, struct 
> fb_info *info)
>  err3:
>   iounmap(fb_virt);
>  err2:
> - release_mem_region(par->mem->start, screen_fb_size);
> + vmbus_free_mmio(par->mem->start, screen_fb_size);
>   par->mem = NULL;
>  err1:
>   if (!gen2vm)
> @@ -758,7 +758,7 @@ static void hvfb_putmem(struct fb_info *info)
>   struct hvfb_par *par = info->par;
>  
>   iounmap(info->screen_base);
> - release_mem_region(par->mem->start, screen_fb_size);
> + vmbus_free_mmio(par->mem->start, screen_fb_size);
>   par->mem = NULL;
>  }
>  
> -- 
> 1.9.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] PCI: Remove usage of pci_domain_nr when the PCI bus doesn't yet exist

[Bjorn Helgaas]

On Wed, Feb 24, 2016 at 06:56:36PM +, ja...@microsoft.com wrote:
> From: Jake Oshins 
> 
> This patch fixes a race condition in this driver.  Using the
> function pci_domain_nr() only works if the PCI bus has already
> been fully created.  This patch just deletes one call site,
> as it was in debug prints which aren't strictly necessary.
> Another call site is changed to look for the data in the same
> structure that is passed in when creating the PCI bus in the
> first place.
> 
> Signed-off-by: Jake Oshins 

I folded this into the original "Add paravirtual PCI front-end for
Microsoft Hyper-V VMs" patch and updated the pci/host-hv branch.

Thanks, Jake!

> ---
>  drivers/pci/host/pci-hyperv.c | 15 ---
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
> index 9391dee..ed651ba 100644
> --- a/drivers/pci/host/pci-hyperv.c
> +++ b/drivers/pci/host/pci-hyperv.c
> @@ -1265,11 +1265,6 @@ static struct hv_pci_dev *new_pcichild_device(struct 
> hv_pcibus_device *hbus,
>   if (!hpdev)
>   return NULL;
>  
> - dev_info(>hdev->device,
> -  "New child device (%p) [%04x:%04x] at %04x:00:00.%02x\n",
> -  hpdev, desc->v_id, desc->d_id, pci_domain_nr(hbus->pci_bus),
> -  desc->win_slot.bits.func);
> -
>   hpdev->hbus = hbus;
>  
>   memset(, 0, sizeof(pkt));
> @@ -1558,9 +1553,15 @@ static void hv_eject_device_work(struct work_struct 
> *work)
>   return;
>   }
>  
> + /*
> +  * Ejection can come before or after the PCI bus has been set up, so
> +  * attempt to find it and tear down the bus state, if it exists.  This
> +  * must be done without constructs like pci_domain_nr(hbus->pci_bus)
> +  * because hbus->pci_bus may not exist yet.
> +  */
>   wslot = wslot_to_devfn(hpdev->desc.win_slot.slot);
> - pdev = pci_get_domain_bus_and_slot(pci_domain_nr(hpdev->hbus->pci_bus),
> -0, wslot);
> + pdev = pci_get_domain_bus_and_slot(hpdev->hbus->sysdata.domain, 0,
> +wslot);
>   if (pdev) {
>   pci_stop_and_remove_bus_device(pdev);
>   pci_dev_put(pdev);
> -- 
> 1.9.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH RESEND v3 0/3] PCI: hv: New paravirtual PCI front-end driver

[Bjorn Helgaas]

On Tue, Feb 16, 2016 at 09:56:20PM +, ja...@microsoft.com wrote:
> From: Jake Oshins <ja...@microsoft.com>
> 
> This version incorporates more feedback from Bjorn Helgaas.  Most notably,
> I removed some debugging code and I consistently used architectural
> means for getting the PCI domain instead of just reaching into the sysdata.
> 
> This is a resend of patches that enable PCI pass-through within Hyper-V
> VMs.  This patch series only includes those which were deemed appropriate
> for being incorportated via the PCI tree.  All other patches in previous
> patch series have gone through other trees and are now in mainline.
> 
> The first two patches modify PCI so that new root PCI buses can be marked with
> an associated fwnode_handle, and so that root PCI buses can look up their
> associated IRQ domain by that handle.
> 
> The last patch, introduces a new driver, hv_pcifront, which exposes root PCI
> buses in a Hyper-V VM.  These root PCI buses expose real PCIe devices, or PCI
> Virtual Functions.
> 
> Jake Oshins (3):
>   PCI: Add fwnode_handle to pci_sysdata
>   PCI: irqdomain: Look up IRQ domain by fwnode_handle
>   PCI: hv: New paravirtual PCI front-end for Hyper-V VMs
> 
>  MAINTAINERS   |1 +
>  arch/x86/include/asm/pci.h|   15 +
>  drivers/pci/Kconfig   |7 +
>  drivers/pci/host/Makefile |1 +
>  drivers/pci/host/pci-hyperv.c | 2359 
> +
>  drivers/pci/probe.c   |   15 +
>  include/linux/pci.h   |4 +
>  7 files changed, 2402 insertions(+)
>  create mode 100644 drivers/pci/host/pci-hyperv.c

Applied to pci/host-hv, thanks, Jake!

This is a new driver, and I don't think there's any risk of breaking
anything, so I think we should still be able to merge this for v4.5.

Bjorn
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH RESEND v2 3/3] PCI: hv: New paravirtual PCI front-end for Hyper-V VMs

[Bjorn Helgaas]

On Tue, Feb 16, 2016 at 09:46:55PM +, Jake Oshins wrote:
> > -Original Message-
> > From: Bjorn Helgaas [mailto:helg...@kernel.org]
> > Sent: Tuesday, February 16, 2016 8:46 AM
> > To: Jake Oshins <ja...@microsoft.com>
> > Cc: bhelg...@google.com; linux-...@vger.kernel.org;
> > gre...@linuxfoundation.org; KY Srinivasan <k...@microsoft.com>; linux-
> > ker...@vger.kernel.org; de...@linuxdriverproject.org; o...@aepfle.de;
> > a...@canonical.com; vkuzn...@redhat.com; t...@linutronix.de; Haiyang
> > Zhang <haiya...@microsoft.com>; marc.zyng...@arm.com; Hadden
> > Hoppert <hadd...@microsoft.com>
> > Subject: Re: [PATCH RESEND v2 3/3] PCI: hv: New paravirtual PCI front-end
> > for Hyper-V VMs
> > 
> > Hi Jake,
> > 
> > Looks good to me overall; I marked a few nits below.
> > 
> > The only real question I have is about domain number allocation.  See
> > the note below.
> > 
> [snip]
> > > +
> > > + /*
> > > +  * The PCI bus "domain" is what is called "segment" in
> > > +  * ACPI and other specs.  Pull it from the instance ID,
> > > +  * to get something unique.  Bytes 8 and 9 are what is used
> > > +  * in Windows guests, so do the same thing for consistency.
> > > +  */
> > > +
> > > + hbus->sysdata.domain = hdev->dev_instance.b[9] |
> > > +hdev->dev_instance.b[8] << 8;
> > 
> > How do we know this is unique?  We don't have any idea what the
> > platform will put in _SEG, so I think there's a potential conflict
> > here.  The Intel VMD driver (arch/x86/pci/vmd.c) has a similar
> > problem, and it looks for unused domain numbers starting at 0x1
> > (see vmd_find_free_domain()).
> > 
> 
> Bjorn, thank you for your very thorough reviews.  I deeply appreciate it.  I 
> checked the Hyper-V code and it currently does guarantee that these values 
> are unique.  When I resend the series, I'll add a comment to that effect.  
> I'll also add a comment to Hyper-V that says that it has to stay that way.

I'm not familiar with how Hyper-V works, but I guess you're saying
that Hyper-V controls what the guest sees via _SEG as well as what it
sees via the instance ID.

Bjorn
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH RESEND v2 3/3] PCI: hv: New paravirtual PCI front-end for Hyper-V VMs

[Bjorn Helgaas]

Hi Jake,

Looks good to me overall; I marked a few nits below.

The only real question I have is about domain number allocation.  See
the note below.

On Tue, Feb 09, 2016 at 07:24:28PM +, ja...@microsoft.com wrote:
> From: Jake Oshins 
> 
> This patch introduces a new driver which exposes a root PCI bus whenever a
> PCI Express device is passed through to a guest VM under Hyper-V. The
> device can be single- or multi-function. The interrupts for the devices
> are managed by an IRQ domain, implemented within the driver.
> 
> Signed-off-by: Jake Oshins 
> ---
>  MAINTAINERS   |1 +
>  drivers/pci/Kconfig   |7 +
>  drivers/pci/host/Makefile |1 +
>  drivers/pci/host/pci-hyperv.c | 2373 
> +
>  4 files changed, 2382 insertions(+)
>  create mode 100644 drivers/pci/host/pci-hyperv.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 30aca4a..b68c015 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -5193,6 +5193,7 @@ F:  arch/x86/kernel/cpu/mshyperv.c
>  F:   drivers/hid/hid-hyperv.c
>  F:   drivers/hv/
>  F:   drivers/input/serio/hyperv-keyboard.c
> +F:   drivers/pci/host/pci-hyperv.c
>  F:   drivers/net/hyperv/
>  F:   drivers/scsi/storvsc_drv.c
>  F:   drivers/video/fbdev/hyperv_fb.c
> diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
> index 73de4ef..54a5441 100644
> --- a/drivers/pci/Kconfig
> +++ b/drivers/pci/Kconfig
> @@ -118,4 +118,11 @@ config PCI_LABEL
>   def_bool y if (DMI || ACPI)
>   select NLS
>  
> +config PCI_HYPERV
> +tristate "Hyper-V PCI Frontend"
> +depends on PCI && X86 && HYPERV && PCI_MSI && PCI_MSI_IRQ_DOMAIN && 
> X86_64
> +help
> +  The PCI device frontend driver allows the kernel to import 
> arbitrary
> +  PCI devices from a PCI backend to support PCI driver domains.
> +
>  source "drivers/pci/host/Kconfig"
> diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
> index 7b2f20c..152daf9 100644
> --- a/drivers/pci/host/Makefile
> +++ b/drivers/pci/host/Makefile
> @@ -2,6 +2,7 @@ obj-$(CONFIG_PCIE_DW) += pcie-designware.o
>  obj-$(CONFIG_PCI_DRA7XX) += pci-dra7xx.o
>  obj-$(CONFIG_PCI_EXYNOS) += pci-exynos.o
>  obj-$(CONFIG_PCI_IMX6) += pci-imx6.o
> +obj-$(CONFIG_PCI_HYPERV) += pci-hyperv.o
>  obj-$(CONFIG_PCI_MVEBU) += pci-mvebu.o
>  obj-$(CONFIG_PCI_TEGRA) += pci-tegra.o
>  obj-$(CONFIG_PCI_RCAR_GEN2) += pci-rcar-gen2.o
> diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
> new file mode 100644
> index 000..2ca43f1
> --- /dev/null
> +++ b/drivers/pci/host/pci-hyperv.c
> @@ -0,0 +1,2373 @@
> +/*
> + * Copyright (c) Microsoft Corporation.
> + *
> + * Author:
> + *   Jake Oshins 
> + *
> + * This driver acts as a paravirtual front-end for PCI Express root buses.
> + * When a PCI Express function (either an entire device or an SR-IOV
> + * Virtual Function) is being passed through to the VM, this driver exposes
> + * a new bus to the guest VM.  This is modeled as a root PCI bus because
> + * no bridges are being exposed to the VM.  In fact, with a "Generation 2"
> + * VM within Hyper-V, there may seem to be no PCI bus at all in the VM
> + * until a device as been exposed using this driver.
> + *
> + * Each root PCI bus has its own PCI domain, which is called "Segment" in
> + * the PCI Firmware Specifications.  Thus while each device passed through
> + * to the VM using this front-end will appear at "device 0", the domain will
> + * be unique.  Typically, each bus will have one PCI function on it, though
> + * this driver does support more than one.
> + *
> + * In order to map the interrupts from the device through to the guest VM,
> + * this driver also implements an IRQ Domain, which handles interrupts 
> (either
> + * MSI or MSI-X) associated with the functions on the bus.  As interrupts are
> + * set up, torn down, or reaffined, this driver communicates with the
> + * underlying hypervisor to adjust the mappings in the I/O MMU so that each
> + * interrupt will be delivered to the correct virtual processor at the right
> + * vector.  This driver does not support level-triggered (line-based)
> + * interrupts, and will report that the Interrupt Line register in the
> + * function's configuration space is zero.
> + *
> + * The rest of this driver mostly maps PCI concepts onto underlying Hyper-V
> + * facilities.  For instance, the configuration space of a function exposed
> + * by Hyper-V is mapped into a single page of memory space, and the
> + * read and write handlers for config space must be aware of this mechanism.
> + * Similarly, device setup and teardown involves messages sent to and from
> + * the PCI back-end driver in Hyper-V.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published
> + * by the Free Software Foundation.
> + *
> 

Re: [PATCH RESEND 3/3] PCI: hv: New paravirtual PCI front-end for Hyper-V VMs

[Bjorn Helgaas]

Hi Jake,

On Tue, Feb 02, 2016 at 05:41:43PM +, ja...@microsoft.com wrote:
> From: Jake Oshins 
> 
> This patch introduces a new driver which exposes a root PCI bus whenever a
> PCI Express device is passed through to a guest VM under Hyper-V. The
> device can be single- or multi-function. The interrupts for the devices
> are managed by an IRQ domain, implemented within the driver.
> 
> Signed-off-by: Jake Oshins 
> ---
>  MAINTAINERS|1 +
>  drivers/pci/Kconfig|7 +
>  drivers/pci/host/Makefile  |1 +
>  drivers/pci/host/hv_pcifront.c | 2248 
> 

This is grossly similar to the other host controller drivers in
drivers/pci/host, so maybe we could name it similarly, too, e.g.,
pci-hyperv.c and CONFIG_PCI_HYPERV?  Or maybe even
CONFIG_PCI_HYPERV_GUEST?

>  4 files changed, 2257 insertions(+)
>  create mode 100644 drivers/pci/host/hv_pcifront.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 5161fb9..53f0959 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -5185,6 +5185,7 @@ F:  arch/x86/kernel/cpu/mshyperv.c
>  F:   drivers/hid/hid-hyperv.c
>  F:   drivers/hv/
>  F:   drivers/input/serio/hyperv-keyboard.c
> +F:   drivers/pci/host/hv_pcifront.c
>  F:   drivers/net/hyperv/
>  F:   drivers/scsi/storvsc_drv.c
>  F:   drivers/video/fbdev/hyperv_fb.c
> diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
> index 73de4ef..573b8d6 100644
> --- a/drivers/pci/Kconfig
> +++ b/drivers/pci/Kconfig
> @@ -118,4 +118,11 @@ config PCI_LABEL
>   def_bool y if (DMI || ACPI)
>   select NLS
>  
> +config HYPERV_VPCI
> +tristate "Hyper-V PCI Frontend"
> +depends on PCI && X86 && HYPERV && PCI_MSI && PCI_MSI_IRQ_DOMAIN && 
> X86_64
> +help
> +  The PCI device frontend driver allows the kernel to import 
> arbitrary
> +  PCI devices from a PCI backend to support PCI driver domains.
> +
>  source "drivers/pci/host/Kconfig"
> diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
> index 7b2f20c..6102aa9 100644
> --- a/drivers/pci/host/Makefile
> +++ b/drivers/pci/host/Makefile
> @@ -2,6 +2,7 @@ obj-$(CONFIG_PCIE_DW) += pcie-designware.o
>  obj-$(CONFIG_PCI_DRA7XX) += pci-dra7xx.o
>  obj-$(CONFIG_PCI_EXYNOS) += pci-exynos.o
>  obj-$(CONFIG_PCI_IMX6) += pci-imx6.o
> +obj-$(CONFIG_HYPERV_VPCI) += hv_pcifront.o
>  obj-$(CONFIG_PCI_MVEBU) += pci-mvebu.o
>  obj-$(CONFIG_PCI_TEGRA) += pci-tegra.o
>  obj-$(CONFIG_PCI_RCAR_GEN2) += pci-rcar-gen2.o
> diff --git a/drivers/pci/host/hv_pcifront.c b/drivers/pci/host/hv_pcifront.c
> new file mode 100644
> index 000..d2ed1be
> --- /dev/null
> +++ b/drivers/pci/host/hv_pcifront.c
> @@ -0,0 +1,2248 @@
> +/*
> + * Copyright (c) Microsoft Corporation.
> + *
> + * Author:
> + *   Jake Oshins 
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published
> + * by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE or
> + * NON INFRINGEMENT.  See the GNU General Public License for more
> + * details.
> + *
> + */

A comment here would help a lot.  Something to the effect of "This
driver runs in a Linux kernel running as a Hyper-V guest on top of a
Windows host.  When the host exports a PCI device to the guest via
Vmbus, this driver creates a virtual PCI root bus for that device to
live on and facilitates config access, resource management for BARs,
and MSI configuration."  Or whatever this actually does.

> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

Please use dev_info(), dev_err(), etc., everywhere possible below.

> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +/*
> + * Protocol versions. The low word is the minor version, the high word the 
> major
> + * version.
> + */
> +
> +#define PCI_MAKE_VERSION(major, minor) ((__u32)(((major) << 16) | (major)))
> +#define PCI_MAJOR_VERSION(version) ((__u32)(version) >> 16)
> +#define PCI_MINOR_VERSION(version) ((__u32)(version) & 0xff)

I don't know what the rules are for using "u32" vs "__u32".  There are
around 10x as many instances of u32 as there are of __u32 in the
kernel.  I see that you do use "u32" sometimes below, so you probably have
a reason for using __u32 here.

> +enum {
> + PCI_PROTOCOL_VERSION_1_1 = PCI_MAKE_VERSION(1, 1),
> + PCI_PROTOCOL_VERSION_CURRENT = PCI_PROTOCOL_VERSION_1_1
> +};
> +
> +#define PCI_CONFIG_MMIO_LENGTH   0x2000
> +#define MAX_SUPPORTED_MSI_MESSAGES 0x400
> +
> +/*
> + * Message Types
> + */
> +
> +enum pci_message_type {
> + /*
> +  * Version 1.1
> +  */
> + PCI_MESSAGE_BASE= 0x4249,
> 

Re: [PATCH RESEND 1/3] PCI: Add fwnode_handle to pci_sysdata

[Bjorn Helgaas]

Hi Jake,

On Tue, Feb 02, 2016 at 05:41:41PM +, ja...@microsoft.com wrote:
> From: Jake Oshins 
> 
> This patch adds an fwnode_handle to struct pci_sysdata, which is
> used by the next patch in the series when trying to locate an
> IRQ domain associated with a root PCI bus.
> 
> Signed-off-by: Jake Oshins 
> ---
>  arch/x86/include/asm/pci.h | 15 +++
>  drivers/pci/probe.c|  1 +
>  include/linux/pci.h|  4 
>  3 files changed, 20 insertions(+)
> 
> diff --git a/arch/x86/include/asm/pci.h b/arch/x86/include/asm/pci.h
> index 4625943..6fc3c7c 100644
> --- a/arch/x86/include/asm/pci.h
> +++ b/arch/x86/include/asm/pci.h
> @@ -20,6 +20,9 @@ struct pci_sysdata {
>  #ifdef CONFIG_X86_64
>   void*iommu; /* IOMMU private data */
>  #endif
> +#ifdef CONFIG_PCI_MSI_IRQ_DOMAIN
> + void*fwnode;/* IRQ domain for MSI assignment */
> +#endif
>  };
>  
>  extern int pci_routeirq;
> @@ -32,6 +35,7 @@ extern int noioapicreroute;
>  static inline int pci_domain_nr(struct pci_bus *bus)
>  {
>   struct pci_sysdata *sd = bus->sysdata;
> +
>   return sd->domain;
>  }
>  
> @@ -41,6 +45,17 @@ static inline int pci_proc_domain(struct pci_bus *bus)
>  }
>  #endif
>  
> +#ifdef CONFIG_PCI_MSI_IRQ_DOMAIN
> +static inline void *_pci_root_bus_fwnode(struct pci_bus *bus)
> +{
> + struct pci_sysdata *sd = bus->sysdata;
> +
> + return sd->fwnode;
> +}
> +
> +#define pci_root_bus_fwnode  _pci_root_bus_fwnode
> +#endif
> +
>  /* Can be used to override the logic in pci_scan_bus for skipping
> already-configured bus numbers - to be used for buggy BIOSes
> or architectures with incomplete PCI setup by the loader */
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 6d7ab9b..b207e74 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -15,6 +15,7 @@
>  #include 
>  #include 
>  #include 
> +#include 

You're not adding a use of anything in irqdomain.h.  It looks like
this hunk should be moved to the second patch.

>  #include 
>  #include "pci.h"
>  
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 27df4a6..cd05a8e 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1515,6 +1515,10 @@ static inline int pci_get_new_domain_nr(void) { return 
> -ENOSYS; }
>  
>  #include 
>  
> +#ifndef pci_root_bus_fwnode
> +#define pci_root_bus_fwnode(bus) ((void)(bus), NULL)

Huh, interesting.  This is new for me; I guess the idea is that we at
least evaluate "bus" even when pci_root_bus_fwnode isn't defined, so
the compiler can catch egregious errors?

> +#endif
> +
>  /* these helpers provide future and backwards compatibility
>   * for accessing popular PCI BAR info */
>  #define pci_resource_start(dev, bar) ((dev)->resource[(bar)].start)
> -- 
> 1.9.1
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH RESEND 1/3] PCI: Add fwnode_handle to pci_sysdata

[Bjorn Helgaas]

On Wed, Feb 03, 2016 at 06:32:20PM +, Jake Oshins wrote:
> > -Original Message-
> > From: Bjorn Helgaas [mailto:helg...@kernel.org]
> > Sent: Wednesday, February 3, 2016 10:25 AM
> > To: Jake Oshins <ja...@microsoft.com>
> > Cc: gre...@linuxfoundation.org; KY Srinivasan <k...@microsoft.com>; linux-
> > ker...@vger.kernel.org; de...@linuxdriverproject.org; Haiyang Zhang
> > <haiya...@microsoft.com>; marc.zyng...@arm.com;
> > bhelg...@google.com; linux-...@vger.kernel.org
> > Subject: Re: [PATCH RESEND 1/3] PCI: Add fwnode_handle to pci_sysdata
> > 
> > Hi Jake,
> > 
> > On Tue, Feb 02, 2016 at 05:41:41PM +, ja...@microsoft.com wrote:

> > > diff --git a/include/linux/pci.h b/include/linux/pci.h index
> > > 27df4a6..cd05a8e 100644
> > > --- a/include/linux/pci.h
> > > +++ b/include/linux/pci.h
> > > @@ -1515,6 +1515,10 @@ static inline int pci_get_new_domain_nr(void) {
> > > return -ENOSYS; }
> > >
> > >  #include 
> > >
> > > +#ifndef pci_root_bus_fwnode
> > > +#define pci_root_bus_fwnode(bus) ((void)(bus), NULL)
> > 
> > Huh, interesting.  This is new for me; I guess the idea is that we at least
> > evaluate "bus" even when pci_root_bus_fwnode isn't defined, so the
> > compiler can catch egregious errors?
> > 
> 
> This was a suggestion by Mark Zyngier.  It made the non-x86 architectures 
> build benignly.  If you'd like it done differently, I'm open to suggestion.

Something like "#define pci_root_bus_fwnode(bus) NULL" would be
typical.  What I'm curious about is the use of the comma operator.
I'm not opposed to it; I'm just trying to understand why it makes a
difference.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 04/12] drivers:pci: Add IRQ domain lookup by PCI domain

[Bjorn Helgaas]

Hi Jake,

I don't know how this will be morphed after Marc's comments, but when you
repost this series, please take a look at the existing change history,
e.g.,  with "git log --oneline drivers/pci/probe.c", and make your subject
lines follow the existing conventions.  In this case, they look like:

  PCI: Make pci_msi_setup_pci_dev() non-static for use by arch code
  PCI: Set MPS to match upstream bridge
  PCI: Move MPS configuration check to pci_configure_device()
  PCI: Add pci_scan_root_bus_msi()
  PCI: Tolerate hierarchies with no Root Port
  ...

Bjorn

On Fri, Sep 11, 2015 at 12:01:03AM +, ja...@microsoft.com wrote:
> From: Jake Oshins 
> 
> The PCI driver currently looks up IRQ domains for root PCI buses by walking
> up the Open Firmware tree looking for any that cover this particular PCI root.
> Since x86 PCs don't implement Open Firmware, this patch offers an alternative
> lookup by the PCI domain ID (known as "segment" in the PCI and ACPI specs.)
> 
> I could have tried to build a (fake) Open Firmware tree and leverage the old
> code, but I rejected that possibility both because it would have required
> changes in lots of other places and because most distributions don't even
> compile in the OF infrastructure when targeting PCs.
> 
> Signed-off-by: Jake Oshins 
> ---
>  drivers/pci/probe.c | 11 +++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 0b2be17..e7e5ff3 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -12,6 +12,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include "pci.h"
>  
> @@ -663,12 +664,22 @@ static void pci_set_bus_speed(struct pci_bus *bus)
>  static struct irq_domain *pci_host_bridge_msi_domain(struct pci_bus *bus)
>  {
>   struct irq_domain *d;
> + int pci_domain;
>  
>   /*
>* Any firmware interface that can resolve the msi_domain
>* should be called from here.
>*/
>   d = pci_host_bridge_of_msi_domain(bus);
> + if (d)
> + return d;
> +
> + /*
> +  * If firmware couldn't help find, it try looking it up by PCI
> +  * domain/segment.
> +  */
> + pci_domain = pci_domain_nr(bus);
> + d = irq_find_matching_host(NULL, DOMAIN_BUS_PCI_MSI, _domain);
>  
>   return d;
>  }
> -- 
> 1.9.1
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 0/25] Replace DEFINE_PCI_DEVICE_TABLE macro use

[Bjorn Helgaas]

[+cc Jingoo]

On Fri, Jul 18, 2014 at 12:50 PM, James Bottomley
james.bottom...@hansenpartnership.com wrote:
 On Fri, 2014-07-18 at 11:17 -0700, Greg KH wrote:
 On Fri, Jul 18, 2014 at 09:54:32AM -0700, James Bottomley wrote:
  On Fri, 2014-07-18 at 09:43 -0700, Greg KH wrote:
   On Fri, Jul 18, 2014 at 12:22:13PM -0400, John W. Linville wrote:
On Fri, Jul 18, 2014 at 05:26:47PM +0200, Benoit Taine wrote:
 We should prefer `const struct pci_device_id` over
 `DEFINE_PCI_DEVICE_TABLE` to meet kernel coding style guidelines.
 This issue was reported by checkpatch.
   
Honestly, I prefer the macro -- it stands-out more.  Maybe the style
guidelines and/or checkpatch should change instead?
  
   The macro is horrid, no other bus has this type of thing just to save a
   few characters in typing
 
  OK, so this is the macro:
 
  #define DEFINE_PCI_DEVICE_TABLE(_table) \
  const struct pci_device_id _table[]
 
  Could you explain what's so horrible?
 
  The reason it's useful today is that people forget the const (and
  sometimes the [] making it a true table instead of a pointer).  If you
  use the DEFINE_PCI_DEVICE_TABLE macro, the compile breaks if you use it
  wrongly (good) and you automatically get the correct annotations.

 We have almost 1000 more uses of the non-macro version than the macro
 version in the kernel today:
 $ git grep -w DEFINE_PCI_DEVICE_TABLE | wc -l
 262
 $ git grep const struct pci_device_id | wc -l
 1254

 My big complaint is that we need to be consistant, either pick one or
 the other and stick to it.  As the macro is the least used, it's easiest
 to fix up, and it also is more consistant with all other kernel
 subsystems which do not have such a macro.

 I've a weak preference for consistency, but not at the expense of
 hundreds of patches churning the kernel to remove an innocuous macro.
 Churn costs time and effort.

 As there is no need for the __init macro mess anymore, there's no real
 need for the DEFINE_PCI_DEVICE_TABLE macro either.  I think checkpatch
 will catch the use of non-const users for the id table already today, it
 catches lots of other uses like this already.

   , so why should PCI be special in this regard
   anymore?
 
  I think the PCI usage dwarfs most other bus types now, so you could turn
  the question around.  However, I don't think majority voting is a good
  guide to best practise; lets debate the merits for their own sake.

 Not really dwarf, USB is close with over 700 such structures:
 $ git grep const struct usb_device_id | wc -l
 725

 And i2c is almost just as big as PCI:
 $ git grep const struct i2c_device_id | wc -l
 1223

 So again, this macro is not consistent with the majority of PCI drivers,
 nor with any other type of device id declaration in the kernel, which
 is why I feel it should be removed.

 And finally, the PCI documentation itself says to not use this macro, so
 this isn't a new thing.  From Documentation/PCI/pci.txt:

   The ID table is an array of struct pci_device_id entries ending with an
   all-zero entry.  Definitions with static const are generally preferred.
   Use of the deprecated macro DEFINE_PCI_DEVICE_TABLE should be avoided.

 That wording went into the file last December, when we last talked about
 this and everyone in that discussion agreed to remove the macro for the
 above reasons.

 Consistency matters.

 In this case, I don't think it does that much ... a cut and paste either
 way (from a macro or non-macro based driver) yields correct code.  Since
 there's no bug here and no apparent way to misuse the macro, why bother?

 Anyway, it's PCI code ... let the PCI maintainer decide.  However, if he
 does want this do it as one big bang patch via either the PCI or Trivial
 tree (latter because Jiří has experience doing this, but the former
 might be useful so the decider feels the pain ...)

I don't feel strongly either way, so I guess I'm OK with this, and in
the spirit of feeling the pain, I'm willing to handle it.  Jingoo
proposed similar patches, so it might be nice to give him some credit.

Benoit, how about if you wait until about half-way through the merge
window after v3.16 releases, generate an up-to-date single patch, and
post that?  Then we can try to get it in before v3.17-rc1 to minimize
merge hassles.

Bjorn
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 0/4] pci_ids, 8250_pci: remove PCI_VENDOR_ID_ADDIDATA_OLD

[Bjorn Helgaas]

On Tue, Jul 16, 2013 at 9:14 AM, Ian Abbott abbo...@mev.co.uk wrote:
 The 8250_pci driver uses PCI_VENDOR_ID_ADDIDATA_OLD (0x10e8),
 PCI_DEVICE_ID_ADDIDATA_APCI7800 (0x818e) to recognize the original
 ADDI-DATA APCI-7800 PCI serial card.  However vendor ID 0x10e8 was
 assigned by PCI-SIG to Applied Micro Circuits Corporation (AMCC) and the
 associated device ID 0x818e was assigned by AMCC to ADDI-DATA.

 Comedi already defines PCI_VENDOR_ID_AMCC as 0x10e8 in one of its header
 files, so that definition can be moved into pci_ids.h and the 8250_pci
 driver changed to use it.  The PCI_DEVICE_ID_ADDIDATA_APCI7800 define
 seems out of place in pci_ids.h since it isn't associated with
 ADDI-DATA's vendor ID but with AMCC's vendor ID.  It's only used in
 8250_pci.c so it can be moved there and renamed to something more
 sensible.

 1) pci_ids.h: move PCI_VENDOR_ID_AMCC here
 2) serial: 8250_pci: replace PCI_VENDOR_ID_ADDIDATA_OLD
 3) serial: 8250_pci: use local device ID for ADDI-DATA APCI-7800
 4) pci_ids.h: remove PCI_VENDOR_ID_ADDIDATA_OLD and
PCI_DEVICE_ID_ADDIDATA_APCI7800

  drivers/staging/comedi/comedidev.h | 1 -
  drivers/tty/serial/8250/8250_pci.c | 9 +
  include/linux/pci_ids.h| 4 ++--
  3 files changed, 7 insertions(+), 7 deletions(-)

For patches 1  4 (the ones that touch pci_ids.h):

Acked-by: Bjorn Helgaas bhelg...@google.com

Please merge them along with the 8250 changes.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel