Re: [PATCH V2 7/7] PCI: make reset poll time adjustable

[Bjorn Helgaas]

On Mon, Nov 27, 2017 at 01:20:28AM -0500, Sinan Kaya wrote:
> Introduce pci=resetpolltime= argument to override 60 seconds poll time in
> units of milliseconds.

I resist adding kernel parameters because they really complicate the
user experience.  Obviously you added this for a reason, but I don't
know what that is.  If we really need it, is there any way we could
automatically figure out in the kernel when we need different poll
times?

> Signed-off-by: Sinan Kaya 
> ---
>  Documentation/admin-guide/kernel-parameters.txt |  2 ++
>  drivers/pci/pci.c   | 13 -
>  2 files changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt 
> b/Documentation/admin-guide/kernel-parameters.txt
> index 0549662..a07d4f5 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -3071,6 +3071,8 @@
>   pcie_scan_all   Scan all possible PCIe devices.  Otherwise we
>   only look for one device below a PCIe downstream
>   port.
> + resetpolltime=  Adjusts the default poll time following hot 
> reset
> + and D3->D0 transition.
>  
>   pcie_aspm=  [PCIE] Forcibly enable or disable PCIe Active State 
> Power
>   Management.
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 8472c24..a6c3e25 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -127,7 +127,7 @@ static int __init pcie_port_pm_setup(char *str)
>  
>  /* time to wait after a reset for device to become responsive */
>  #define PCIE_RESET_READY_POLL_MS 6
> -
> +static unsigned long pci_reset_polltime = PCIE_RESET_READY_POLL_MS;
>  /**
>   * pci_bus_max_busnr - returns maximum PCI bus number of given bus' children
>   * @bus: pointer to PCI bus structure to search
> @@ -3904,7 +3904,7 @@ int pcie_flr(struct pci_dev *dev)
>*/
>   msleep(100);
>  
> - return pci_dev_wait(dev, "FLR", PCIE_RESET_READY_POLL_MS);
> + return pci_dev_wait(dev, "FLR", pci_reset_polltime);
>  }
>  EXPORT_SYMBOL_GPL(pcie_flr);
>  
> @@ -3945,7 +3945,7 @@ static int pci_af_flr(struct pci_dev *dev, int probe)
>*/
>   msleep(100);
>  
> - return pci_dev_wait(dev, "AF_FLR", PCIE_RESET_READY_POLL_MS);
> + return pci_dev_wait(dev, "AF_FLR", pci_reset_polltime);
>  }
>  
>  /**
> @@ -3990,7 +3990,7 @@ static int pci_pm_reset(struct pci_dev *dev, int probe)
>   pci_write_config_word(dev, dev->pm_cap + PCI_PM_CTRL, csr);
>   pci_dev_d3_sleep(dev);
>  
> - return pci_dev_wait(dev, "PM D3->D0", PCIE_RESET_READY_POLL_MS);
> + return pci_dev_wait(dev, "PM D3->D0", pci_reset_polltime);
>  }
>  
>  void pci_reset_secondary_bus(struct pci_dev *dev)
> @@ -4035,7 +4035,7 @@ int pci_reset_bridge_secondary_bus(struct pci_dev *dev)
>  {
>   pcibios_reset_secondary_bus(dev);
>  
> - return pci_dev_wait(dev, "bus reset", PCIE_RESET_READY_POLL_MS);
> + return pci_dev_wait(dev, "bus reset", pci_reset_polltime);
>  }
>  EXPORT_SYMBOL_GPL(pci_reset_bridge_secondary_bus);
>  
> @@ -5528,6 +5528,9 @@ static int __init pci_setup(char *str)
>   pcie_bus_config = PCIE_BUS_PEER2PEER;
>   } else if (!strncmp(str, "pcie_scan_all", 13)) {
>   pci_add_flags(PCI_SCAN_ALL_PCIE_DEVS);
> + } else if (!strncmp(str, "resetpolltime=", 14)) {
> + pci_reset_polltime =
> + simple_strtoul(str + 14, , 0);
>   } else {
>   printk(KERN_ERR "PCI: Unknown option `%s'\n",
>   str);
> -- 
> 1.9.1
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/6] PCI / PM: Support for LEAVE_SUSPENDED driver flag

[Bjorn Helgaas]

On Wed, Nov 08, 2017 at 02:28:18PM +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wyso...@intel.com>
> 
> Add support for DPM_FLAG_LEAVE_SUSPENDED to the PCI bus type by
> making it (a) set the power.may_skip_resume status bit for devices
> that, from its perspective, may be left in suspend after system
> wakeup from sleep and (b) return early from pci_pm_resume_noirq()
> for devices whose remaining resume callbacks during the transition
> under way are going to be skipped by the PM core.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wyso...@intel.com>
> Acked-by: Greg Kroah-Hartman <gre...@linuxfoundation.org>

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

> ---
>  Documentation/power/pci.txt |   11 +++
>  drivers/pci/pci-driver.c|   19 +--
>  2 files changed, 28 insertions(+), 2 deletions(-)
> 
> Index: linux-pm/drivers/pci/pci-driver.c
> ===
> --- linux-pm.orig/drivers/pci/pci-driver.c
> +++ linux-pm/drivers/pci/pci-driver.c
> @@ -699,7 +699,7 @@ static void pci_pm_complete(struct devic
>   pm_generic_complete(dev);
>  
>   /* Resume device if platform firmware has put it in reset-power-on */
> - if (dev->power.direct_complete && pm_resume_via_firmware()) {
> + if (pm_runtime_suspended(dev) && pm_resume_via_firmware()) {
>   pci_power_t pre_sleep_state = pci_dev->current_state;
>  
>   pci_update_current_state(pci_dev, pci_dev->current_state);
> @@ -783,8 +783,10 @@ static int pci_pm_suspend_noirq(struct d
>   struct pci_dev *pci_dev = to_pci_dev(dev);
>   const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
>  
> - if (dev_pm_smart_suspend_and_suspended(dev))
> + if (dev_pm_smart_suspend_and_suspended(dev)) {
> + dev->power.may_skip_resume = true;
>   return 0;
> + }
>  
>   if (pci_has_legacy_pm_support(pci_dev))
>   return pci_legacy_suspend_late(dev, PMSG_SUSPEND);
> @@ -838,6 +840,16 @@ static int pci_pm_suspend_noirq(struct d
>  Fixup:
>   pci_fixup_device(pci_fixup_suspend_late, pci_dev);
>  
> + /*
> +  * If the target system sleep state is suspend-to-idle, it is sufficient
> +  * to check whether or not the device's wakeup settings are good for
> +  * runtime PM.  Otherwise, the pm_resume_via_firmware() check will cause
> +  * pci_pm_complete() to take care of fixing up the device's state
> +  * anyway, if need be.
> +  */
> + dev->power.may_skip_resume = device_may_wakeup(dev) ||
> + !device_can_wakeup(dev);
> +
>   return 0;
>  }
>  
> @@ -847,6 +859,9 @@ static int pci_pm_resume_noirq(struct de
>   struct device_driver *drv = dev->driver;
>   int error = 0;
>  
> + if (dev_pm_may_skip_resume(dev))
> + return 0;
> +
>   /*
>* Devices with DPM_FLAG_SMART_SUSPEND may be left in runtime suspend
>* during system suspend, so update their runtime PM status to "active"
> Index: linux-pm/Documentation/power/pci.txt
> ===
> --- linux-pm.orig/Documentation/power/pci.txt
> +++ linux-pm/Documentation/power/pci.txt
> @@ -994,6 +994,17 @@ into D0 going forward), but if it is in
>  the function will set the power.direct_complete flag for it (to make the PM 
> core
>  skip the subsequent "thaw" callbacks for it) and return.
>  
> +Setting the DPM_FLAG_LEAVE_SUSPENDED flag means that the driver prefers the
> +device to be left in suspend after system-wide transitions to the working 
> state.
> +This flag is checked by the PM core, but the PCI bus type informs the PM core
> +which devices may be left in suspend from its perspective (that happens 
> during
> +the "noirq" phase of system-wide suspend and analogous transitions) and next 
> it
> +uses the dev_pm_may_skip_resume() helper to decide whether or not to return 
> from
> +pci_pm_resume_noirq() early, as the PM core will skip the remaining resume
> +callbacks for the device during the transition under way and will set its
> +runtime PM status to "suspended" if dev_pm_may_skip_resume() returns "true" 
> for
> +it.
> +
>  3.2. Device Runtime Power Management
>  
>  In addition to providing device power management callbacks PCI device drivers
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 5/6] PCI / PM: Take SMART_SUSPEND driver flag into account

[Bjorn Helgaas]

On Sat, Oct 28, 2017 at 12:27:45AM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wyso...@intel.com>
> 
> Make the PCI bus type take DPM_FLAG_SMART_SUSPEND into account in its
> system-wide PM callbacks and make sure that all code that should not
> run in parallel with pci_pm_runtime_resume() is executed in the "late"
> phases of system suspend, freeze and poweroff transitions.
> 
> [Note that the pm_runtime_suspended() check in pci_dev_keep_suspended()
> is an optimization, because if is not passed, all of the subsequent
> checks may be skipped and some of them are much more overhead in
> general.]
> 
> Also use the observation that if the device is in runtime suspend
> at the beginning of the "late" phase of a system-wide suspend-like
> transition, its state cannot change going forward (runtime PM is
> disabled for it at that time) until the transition is over and the
> subsequent system-wide PM callbacks should be skipped for it (as
> they generally assume the device to not be suspended), so add checks
> for that in pci_pm_suspend_late/noirq(), pci_pm_freeze_late/noirq()
> and pci_pm_poweroff_late/noirq().
> 
> Moreover, if pci_pm_resume_noirq() or pci_pm_restore_noirq() is
> called during the subsequent system-wide resume transition and if
> the device was left in runtime suspend previously, its runtime PM
> status needs to be changed to "active" as it is going to be put
> into the full-power state, so add checks for that too to these
> functions.
> 
> In turn, if pci_pm_thaw_noirq() runs after the device has been
> left in runtime suspend, the subsequent "thaw" callbacks need
> to be skipped for it (as they may not work correctly with a
> suspended device), so set the power.direct_complete flag for the
> device then to make the PM core skip those callbacks.
> 
> In addition to the above add a core helper for checking if
> DPM_FLAG_SMART_SUSPEND is set and the device runtime PM status is
> "suspended" at the same time, which is done quite often in the new
> code (and will be done elsewhere going forward too).
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wyso...@intel.com>
> Acked-by: Greg Kroah-Hartman <gre...@linuxfoundation.org>

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

> ---
> 
> -> v2: Implement the entire handling of DPM_FLAG_SMART_SUSPEND in
>the PCI bus type (instead of doing that in the core).
> 
> ---
>  Documentation/power/pci.txt |   14 +
>  drivers/base/power/main.c   |6 ++
>  drivers/pci/pci-driver.c|  103 
> 
>  include/linux/pm.h  |2 
>  4 files changed, 108 insertions(+), 17 deletions(-)
> 
> Index: linux-pm/drivers/pci/pci-driver.c
> ===
> --- linux-pm.orig/drivers/pci/pci-driver.c
> +++ linux-pm/drivers/pci/pci-driver.c
> @@ -734,18 +734,25 @@ static int pci_pm_suspend(struct device
>  
>   if (!pm) {
>   pci_pm_default_suspend(pci_dev);
> - goto Fixup;
> + return 0;
>   }
>  
>   /*
> -  * PCI devices suspended at run time need to be resumed at this point,
> -  * because in general it is necessary to reconfigure them for system
> -  * suspend.  Namely, if the device is supposed to wake up the system
> -  * from the sleep state, we may need to reconfigure it for this purpose.
> -  * In turn, if the device is not supposed to wake up the system from the
> -  * sleep state, we'll have to prevent it from signaling wake-up.
> +  * PCI devices suspended at run time may need to be resumed at this
> +  * point, because in general it may be necessary to reconfigure them for
> +  * system suspend.  Namely, if the device is expected to wake up the
> +  * system from the sleep state, it may have to be reconfigured for this
> +  * purpose, or if the device is not expected to wake up the system from
> +  * the sleep state, it should be prevented from signaling wakeup events
> +  * going forward.
> +  *
> +  * Also if the driver of the device does not indicate that its system
> +  * suspend callbacks can cope with runtime-suspended devices, it is
> +  * better to resume the device from runtime suspend here.
>*/
> - pm_runtime_resume(dev);
> + if (!dev_pm_test_driver_flags(dev, DPM_FLAG_SMART_SUSPEND) ||
> + !pci_dev_keep_suspended(pci_dev))
> + pm_runtime_resume(dev);
>  
>   pci_dev->state_saved = false;
>   if (pm->suspend) {
> @@ -765,17 +772,27 @@ static int pci_pm_suspend(struct device
>

Re: [PATCH 0/12] PM / sleep: Driver flags for system suspend/resume

[Bjorn Helgaas]

On Mon, Oct 16, 2017 at 03:12:35AM +0200, Rafael J. Wysocki wrote:
> Hi All,
> 
> Well, this took more time than expected, as I tried to cover everything I had
> in mind regarding PM flags for drivers.

For the parts that touch PCI,

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

I doubt there'll be conflicts with changes in my tree, but let me know if
you trip over any so I can watch for them when merging.

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [GIT PULL] PCI: Support for configurable PCI endpoint

[Bjorn Helgaas]

On Mon, Apr 10, 2017 at 10:43:28AM -0500, Bjorn Helgaas wrote:
> On Wed, Apr 05, 2017 at 02:22:20PM +0530, Kishon Vijay Abraham I wrote:
> > Hi Bjorn,
> > 
> > Please find the pull request for PCI endpoint support below. I've
> > also included all the history here.
> 
> Thanks, I applied these (with v7 of the first patch) to pci/host-designware
> for v4.12.

Ok, sorry, I screwed this up.  I think my branch actually had v5, not
v6.  But I *think* I fixed it.  Here's the diff from my branch to your
git tree.  Apparently you haven't pushed the v7 patch there, so I
*think* the diff below is the diff between v6 and v7 of that first
patch.

$ git diff pci/host-designware a5c85ba45c96
diff --git a/drivers/pci/endpoint/pci-epc-core.c 
b/drivers/pci/endpoint/pci-epc-core.c
index caa7be10e473..9ae9e59b2a74 100644
--- a/drivers/pci/endpoint/pci-epc-core.c
+++ b/drivers/pci/endpoint/pci-epc-core.c
@@ -83,7 +83,6 @@ struct pci_epc *pci_epc_get(const char *epc_name)
goto err;
}
 
-   class_dev_iter_exit();
get_device(>dev);
return epc;
}
diff --git a/drivers/pci/endpoint/pci-epf-core.c 
b/drivers/pci/endpoint/pci-epf-core.c
index 6877d6a5bcc9..92db7dcd911c 100644
--- a/drivers/pci/endpoint/pci-epf-core.c
+++ b/drivers/pci/endpoint/pci-epf-core.c
@@ -40,10 +40,8 @@ static struct device_type pci_epf_type;
  */
 void pci_epf_linkup(struct pci_epf *epf)
 {
-   if (!epf->driver) {
+   if (!epf->driver)
dev_WARN(>dev, "epf device not bound to driver\n");
-   return;
-   }
 
epf->driver->ops->linkup(epf);
 }
@@ -59,10 +57,8 @@ EXPORT_SYMBOL_GPL(pci_epf_linkup);
  */
 void pci_epf_unbind(struct pci_epf *epf)
 {
-   if (!epf->driver) {
+   if (!epf->driver)
dev_WARN(>dev, "epf device not bound to driver\n");
-   return;
-   }
 
epf->driver->ops->unbind(epf);
module_put(epf->driver->owner);
@@ -78,10 +74,8 @@ EXPORT_SYMBOL_GPL(pci_epf_unbind);
  */
 int pci_epf_bind(struct pci_epf *epf)
 {
-   if (!epf->driver) {
+   if (!epf->driver)
dev_WARN(>dev, "epf device not bound to driver\n");
-   return -EINVAL;
-   }
 
if (!try_module_get(epf->driver->owner))
return -EAGAIN;
@@ -233,7 +227,7 @@ struct pci_epf *pci_epf_create(const char *name)
epf->name = kstrdup(func_name, GFP_KERNEL);
if (!epf->name) {
ret = -ENOMEM;
-   goto free_func_name;
+   goto free_epf;
}
 
dev = >dev;
@@ -255,8 +249,6 @@ struct pci_epf *pci_epf_create(const char *name)
 put_dev:
put_device(dev);
kfree(epf->name);
-
-free_func_name:
kfree(func_name);
 
 free_epf:
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [GIT PULL] PCI: Support for configurable PCI endpoint

[Bjorn Helgaas]

On Wed, Apr 05, 2017 at 02:22:20PM +0530, Kishon Vijay Abraham I wrote:
> Hi Bjorn,
> 
> Please find the pull request for PCI endpoint support below. I've
> also included all the history here.

Thanks, I applied these (with v7 of the first patch) to pci/host-designware
for v4.12.

> Changes from v5:
> *) remove #syscon-cells property added in v5 and used 
>of_parse_phandle_with_fixed_args
> *) fix compilation error in make.cross ARCH=blackfin that was because
>pci_endpoint_test.c driver depends on COMPILE_TEST.
> 
> Changes from v4:
> *) add #syscon-cells property and used of_parse_phandle_with_args
>to perform a configuration in syscon module (as suggested by
>Rob Herring)
> *) Remove unnecessary white space.
> 
> Changes from v3:
> *) fixed a typo and adapted to https://lkml.org/lkml/2017/3/13/562.
> 
> Changes from v2:
> *) changed the configfs structure as suggested by Christoph Hellwig. With
>this change the framework creates configfs entry for EP function driver
>and EP controller. Previously these entries have to be created by the
>the user. (Haven't changed the epc core or epf core except for invoking
>configfs APIs to create entries for EP function driver and EP controller.
>That's mostly because the EP function device can still be created by
>directly invoking the epf core API without using configfs).
> *) Now the user has to use configfs entry 'start' to start the link.
>This was previously done by the function driver. However in the case of
>multi function EP, the function driver shouldn't start the link.
> 
> Changes from v1:
> *) The preparation patches for adding EP support is removed and is sent
>separately
> *) Added device ID for DRA74x/DRA72x and used it instead of
>using "PCI_ANY_ID"
> *) Added userguide for PCI endpoint test function
> 
> Major Improvements from RFC:
>  *) support multi-function devices (hw supported not virtual)
>  *) Access host side buffers
>  *) Raise MSI interrupts
>  *) Add user space program to use the host side PCI driver
>  *) Adapt all other users of designware to use the new design (only
> compile tested. Since I have only dra7xx boards, the new design
> has only been tested in dra7xx. I'd require the help of others
> to test the platforms they have access to).
> 
> This is based on
> git://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git
> pci/host-designware
> 
> Thanks
> Kishon
> 
> The following changes since commit 7ea64dcf602c21b3e5062ca90111ca4459fab403:
> 
>   __end__ (2017-04-04 15:29:37 -0500)
> 
> are available in the git repository at:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/kishon/pci-endpoint.git 
> tags/pci-endpoint-for-4.12
> 
> for you to fetch changes up to a5c85ba45c9682456077d7277196e91f8ea5fd5c:
> 
>   ARM: DRA7: clockdomain: Change the CLKTRCTRL of CM_PCIE_CLKSTCTRL to 
> SW_WKUP (2017-04-05 14:05:28 +0530)
> 
> 
> pci: endpoint: for 4.12
> 
>  *) Add PCI endpoint core layer
>  *) Modify designware and dra7xx driver to be configured in EP mode
>  *) Add a PCI endpoint *test* function driver and corresponding host
> driver
> 
> Signed-off-by: Kishon Vijay Abraham I 
> 
> 
> Kishon Vijay Abraham I (23):
>   PCI: endpoint: Add EP core layer to enable EP controller and EP 
> functions
>   Documentation: PCI: Guide to use PCI Endpoint Core Layer
>   PCI: endpoint: Introduce configfs entry for configuring EP functions
>   Documentation: PCI: Guide to use PCI endpoint configfs
>   PCI: endpoint: Create configfs entry for EPC device and EPF driver
>   Documentation: PCI: Add specification for the *PCI test* function device
>   PCI: endpoint: functions: Add an EP function to test PCI
>   Documentation: PCI: Add binding documentation for pci-test endpoint 
> function
>   PCI: dwc: designware: Add EP mode support
>   dt-bindings: PCI: Add DT bindings for PCI designware EP mode
>   PCI: dwc: dra7xx: Facilitate wrapper and MSI interrupts to be enabled 
> independently
>   PCI: dwc: dra7xx: Add EP mode support
>   dt-bindings: PCI: dra7xx: Add DT bindings for PCI dra7xx EP mode
>   PCI: dwc: dra7xx: Workaround for errata id i870
>   dt-bindings: PCI: dra7xx: Add DT bindings to enable unaligned access
>   PCI: Add device IDs for DRA74x and DRA72x
>   misc: Add host side PCI driver for PCI test function device
>   Documentation: misc-devices: Add Documentation for pci-endpoint-test 
> driver
>   tools: PCI: Add a userspace tool to test PCI endpoint
>   tools: PCI: Add sample test script to invoke pcitest
>   Documentation: PCI: Add userguide for PCI endpoint test function
>   MAINTAINERS: Add PCI Endpoint maintainer
>   ARM: DRA7: clockdomain: Change the CLKTRCTRL of CM_PCIE_CLKSTCTRL to 
> SW_WKUP
> 
>  Documentation/PCI/00-INDEX 

Re: [PATCH v6 01/23] PCI: endpoint: Add EP core layer to enable EP controller and EP functions

[Bjorn Helgaas]

On Wed, Apr 05, 2017 at 02:22:21PM +0530, Kishon Vijay Abraham I wrote:
> Introduce a new EP core layer in order to support endpoint functions in
> linux kernel. This comprises the EPC library (Endpoint Controller Library)
> and EPF library (Endpoint Function Library). EPC library implements
> functions specific to an endpoint controller and EPF library implements
> functions specific to an endpoint function.
> ...

> +/**
> + * pci_epf_linkup() - Notify the function driver that EPC device has
> + * established a connection with the Root Complex.
> + * @epf: the EPF device bound to the EPC device which has established
> + *the connection with the host
> + *
> + * Invoke to notify the function driver that EPC device has established
> + * a connection with the Root Complex.
> + */
> +void pci_epf_linkup(struct pci_epf *epf)
> +{
> + if (!epf->driver)
> + dev_WARN(>dev, "epf device not bound to driver\n");
> +
> + epf->driver->ops->linkup(epf);

I don't understand what's going on here.  We warn if epf->driver is
NULL, but the next thing we do is dereference it.

For NULL pointers that are symptoms of Linux defects, I usually prefer
not to check at all so that a dereference generates an oops and we can
debug the problem.  For NULL pointers caused by user error, we would
generally return an error that percolates up to the user.

I haven't competely wrapped my head around this endpoint support, but
I assume a NULL pointer here would be caused by user error, not
necessarily a Linux defect.  So why would we dereference a NULL
pointer?  And what happens when we do?  Is this just going to oops an
embedded Linux running inside the endpoint?  Is that the correct
behavior?

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [GIT PULL] PCI: Support for configurable PCI endpoint

[Bjorn Helgaas]

d be
+set to SW_WKUP. There are no issues when CLKSTCTRL is set to HW_AUTO in RC
+mode. However in EP mode, the host system is not able to access the
 MEMSPACE and setting the CLKSTCTRL to SW_WKUP fixes it.
 
 Acked-by: Tony Lindgren <t...@atomide.com>
 Signed-off-by: Kishon Vijay Abraham I <kis...@ti.com>
 Signed-off-by: Bjorn Helgaas <bhelg...@google.com>
 
-commit ea4a8033fd42
+commit 4d53ed89fe50
 Author: Kishon Vijay Abraham I <kis...@ti.com>
 Date:   Mon Mar 27 15:15:19 2017 +0530
 
-MAINTAINERS: add PCI EP maintainer
+MAINTAINERS: Add PCI Endpoint maintainer
 
-Add maintainer for the newly introduced PCI EP framework.
+Add maintainer for the newly introduced PCI Endpoint framework.
 
 Signed-off-by: Kishon Vijay Abraham I <kis...@ti.com>
 Signed-off-by: Bjorn Helgaas <bhelg...@google.com>
 
-commit 9c3eb6b9db09
+commit a11812c0dacc
 Author: Kishon Vijay Abraham I <kis...@ti.com>
 Date:   Mon Mar 27 15:15:18 2017 +0530
 
 Documentation: PCI: Add userguide for PCI endpoint test function
 
-Add documentation to help users use pci-epf-test function driver
-and pci_endpoint_test host driver for testing PCI.
+Add documentation to help users use pci-epf-test function driver and
+pci_endpoint_test host driver for testing PCI.
 
 Signed-off-by: Kishon Vijay Abraham I <kis...@ti.com>
 Signed-off-by: Bjorn Helgaas <bhelg...@google.com>
 
-commit ce58c0dca0fc
+commit c7ce6959f0a5
 Author: Kishon Vijay Abraham I <kis...@ti.com>
 Date:   Mon Mar 27 15:15:17 2017 +0530
 
 tools: PCI: Add sample test script to invoke pcitest
 
-Add a simple test script that invokes the pcitest userspace tool
-to perform all the PCI endpoint tests (BAR tests, interrupt tests,
-read tests, write tests and copy tests).
+Add a simple test script that invokes the pcitest userspace tool to perform
+all the PCI endpoint tests (BAR tests, interrupt tests, read tests, write
+tests and copy tests).
 
 Signed-off-by: Kishon Vijay Abraham I <kis...@ti.com>
 Signed-off-by: Bjorn Helgaas <bhelg...@google.com>
 
-commit fdc27ef5bde2
+commit f9ee26a038d8
 Author: Kishon Vijay Abraham I <kis...@ti.com>
 Date:   Mon Mar 27 15:15:16 2017 +0530
 
 tools: PCI: Add a userspace tool to test PCI endpoint
 
-Add a userspace tool to invoke the ioctls exposed by the
-PCI endpoint test driver to perform various PCI tests.
+Add a userspace tool to invoke the ioctls exposed by the PCI endpoint test
+driver to perform various PCI tests.
 
 Signed-off-by: Kishon Vijay Abraham I <kis...@ti.com>
     Signed-off-by: Bjorn Helgaas <bhelg...@google.com>
 
-commit d323b479a02e
+commit fea31fd3cf56
 Author: Kishon Vijay Abraham I <kis...@ti.com>
 Date:   Mon Mar 27 15:15:15 2017 +0530
 
@@ -72,21 +72,21 @@
 Signed-off-by: Kishon Vijay Abraham I <kis...@ti.com>
 Signed-off-by: Bjorn Helgaas <bhelg...@google.com>
 
-commit d26ec30392dd
+commit bedcd782937f
 Author: Kishon Vijay Abraham I <kis...@ti.com>
 Date:   Mon Mar 27 15:15:14 2017 +0530
 
-misc: Add host side pci driver for pci test function device
+misc: Add host side PCI driver for PCI test function device
 
-Add PCI endpoint test driver that can verify base address
-register, legacy interrupt/MSI interrupt and read/write/copy
-buffers between host and device. The corresponding pci-epf-test
-function driver should be used on the EP side.
+Add PCI endpoint test driver that can verify base address register, legacy
+interrupt/MSI interrupt and read/write/copy buffers between host and
+device. The corresponding pci-epf-test function driver should be used on
+the EP side.
 
 Signed-off-by: Kishon Vijay Abraham I <kis...@ti.com>
 Signed-off-by: Bjorn Helgaas <bhelg...@google.com>
 
-commit 5b43200ddaa7
+commit 8066f3c52feb
 Author: Kishon Vijay Abraham I <kis...@ti.com>
 Date:   Mon Mar 27 15:15:13 2017 +0530
 
@@ -98,115 +98,115 @@
 Signed-off-by: Kishon Vijay Abraham I <kis...@ti.com>
 Signed-off-by: Bjorn Helgaas <bhelg...@google.com>
 
-commit a5090d8e9668
+commit 25eef24289ee
 Author: Kishon Vijay Abraham I <kis...@ti.com>
 Date:   Mon Mar 27 15:15:12 2017 +0530
 
-dt-bindings: PCI: dra7xx: Add dt bindings to enable unaligned access
+dt-bindings: PCI: dra7xx: Add DT bindings to enable unaligned access
 
-    Update device tree binding documentation of TI's dra7xx PCI
-controller to include property for enabling unaligned mem access.
+Update device tree binding documentation of TI's dra7xx PCI controller to
+include property for enabling unaligned mem access.
 
 Signed-off-by: Kishon Vijay Abraham I <kis...@ti.com>
 Signed-off-by: Bjorn Helgaas <bhelg...@google.com>
 
-commit 3f080640

Re: [PATCH 9/9] kernel-api.rst: fix a series of errors when parsing C files

[Bjorn Helgaas]

On Thu, Mar 30, 2017 at 05:11:36PM -0300, Mauro Carvalho Chehab wrote:
> ./lib/string.c:134: WARNING: Inline emphasis start-string without end-string.
> ./mm/filemap.c:522: WARNING: Inline interpreted text or phrase reference 
> start-string without end-string.
> ./mm/filemap.c:1283: ERROR: Unexpected indentation.
> ./mm/filemap.c:3003: WARNING: Inline interpreted text or phrase reference 
> start-string without end-string.
> ./mm/vmalloc.c:1544: WARNING: Inline emphasis start-string without end-string.
> ./mm/page_alloc.c:4245: ERROR: Unexpected indentation.
> ./ipc/util.c:676: ERROR: Unexpected indentation.
> ./drivers/pci/irq.c:35: WARNING: Block quote ends without a blank line; 
> unexpected unindent.
> ./security/security.c:109: ERROR: Unexpected indentation.
> ./security/security.c:110: WARNING: Definition list ends without a blank 
> line; unexpected unindent.
> ./block/genhd.c:275: WARNING: Inline strong start-string without end-string.
> ./block/genhd.c:283: WARNING: Inline strong start-string without end-string.
> ./include/linux/clk.h:134: WARNING: Inline emphasis start-string without 
> end-string.
> ./include/linux/clk.h:134: WARNING: Inline emphasis start-string without 
> end-string.
> ./ipc/util.c:477: ERROR: Unknown target name: "s".
> 
> Signed-off-by: Mauro Carvalho Chehab <mche...@s-opensource.com>

Acked-by: Bjorn Helgaas <bhelg...@google.com>   # for drivers/pci/irq.c

> diff --git a/drivers/pci/irq.c b/drivers/pci/irq.c
> index 6684f153ab57..f9f2a0324ecc 100644
> --- a/drivers/pci/irq.c
> +++ b/drivers/pci/irq.c
> @@ -31,7 +31,7 @@ static void pci_note_irq_problem(struct pci_dev *pdev, 
> const char *reason)
>   * driver).
>   *
>   * Returns:
> - *  a suggestion for fixing it (although the driver is not required to
> + * a suggestion for fixing it (although the driver is not required to
>   * act on this).
>   */
>  enum pci_lost_interrupt_reason pci_lost_interrupt(struct pci_dev *pdev)
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 0/4] New Microsemi PCI Switch Management Driver

[Bjorn Helgaas]

On Thu, Mar 02, 2017 at 04:24:30PM -0700, Logan Gunthorpe wrote:
> Because getting it right correctly the first time is appearantly hard
> and I caught this mistake while reading the email I had just sent :(
> 
> I'm very sorry about the extra noise.
> 
> Logan
> 
> --
> 
> Changes since v6:
> 
> * I screwed up the device_unregister path and left a potential use
>   after free bug in. I'm not sure why I didn't see a failure because of
>   it but it all tested fine. This version is correct though.
> 
> Changes since v5:
> 
> * Reworked cleanup during unbind again. This time we follow the pattern
>   roughly suggested by Jason Gunthorpe: we use an alive flag
>   that's checked with a rw semaphore held by the cdev ops. Except
>   instead of using the rwsem we just use the already existing
>   mrpc_mutex. (Seeing the vast majority of locations that would use the
>   semaphore overlap with the existing mutex.)
> * Added a small performance enhancement so the code reads less io memory
>   if the user is quick in submitting their read request.
> 
> Changes since v4:
> 
> * Turns out pushing the pci release code into the device release
>   function didn't work as I would have liked. If you try to unbind the
>   device with an instance open, then you hit a kernel bug at
>   drivers/pci/msi.c:371. (This didn't occur in v3.)
> 
>   To solve this, we've moved the pci release code back into the
>   unregister function and reintroduced an alive flag. This time,
>   however, the alive flag is protected by mrpc_mutex and we're very
>   careful about what happens to devices still in use (they should
>   all be released through the timeout path and an ENODEV error
>   returned to userspace; while new commands are blocked with the
>   same error).
> 
> Changes since v3:
> 
> * Removed stdev_is_alive and moved pci deinit functions
>   into the device release function which only occurs after all
>   cdev instances are closed. (per Bjorn)
> * Reduced locking in read/write functions (per Bjorn)
> * Use pci_alloc_irq_vectors to vastly improve ISR initialization (Bjorn)
> * A few other minor nits and cleanup as noticed by Bjorn
> 
> Changes since v2:
> 
> * Collected reviewed and tested tags
> * Very minor fix to the error path in the create function
> 
> Changes since v1:
> 
> * Rebased onto 4.10-rc6 (cleanly)
> * Split the patch into a few more easily digestible patches (as
>   suggested by Greg Kroah-Hartman)
> * Folded switchtec.c into switchtec.h (per Greg)
> * Fixed a bunch of 32bit build warnings caught by the kbuild test robot
> * Fixed some issues in the documentation so it has a proper
>   reStructredText format (as noted by Jonathan Corbet)
> * Fixed padding and sizes in the IOCTL structures as noticed by Emil
>   Velikov and used pahole to verify their consistency across 32 and 64
>   bit builds
> * Reworked one of the IOCTL interfaces to be more future proof (per
>   Emil).
> 
> Changes since RFC:
> 
> * Fixed incorrect use of the drive model as pointed out by Greg
>   Kroah-Hartman
> * Used devm functions as suggested by Keith Busch
> * Added a handful of sysfs attributes to the switchtec class
> * Added a handful of IOCTLs to the switchtec device
> * A number of miscellaneous bug fixes
> 
> --
> 
> Hi,
> 
> This is a continuation of the RFC we posted lasted month [1] which
> proposes a management driver for Microsemi's Switchtec line of PCI
> switches. This hardware is still looking to be used in the Open
> Compute Platform
> 
> To make this entirely clear: the Switchtec products are compliant
> with the PCI specifications and are supported today with the standard
> in-kernel driver. However, these devices also expose a management endpoint
> on a separate PCI function address which can be used to perform some
> advanced operations. This is a driver for that function. See the patch
> for more information.
> 
> Since the RFC, we've made the changes requested by Greg Kroah-Hartman
> and Keith Busch, and we've also fleshed out a number of features. We've
> added a couple of IOCTLs and sysfs attributes which are documented in
> the patch. Significant work has also been done on the userspace tool
> which is available under a GPL license at [2]. We've also had testing
> done by some of the interested parties.
> 
> We hope to see this work included in either 4.11 or 4.12 assuming a
> smooth review process.
> 
> The patch is based off of the v4.10-rc6 release.
> 
> Thanks for your review,
> 
> Logan
> 
> [1] https://www.spinics.net/lists/linux-pci/msg56897.html
> [2] https://github.com/sbates130272/switchtec-user
> 
> Logan Gunthorpe (4):
>   MicroSemi Switchtec management interface driver
>   switchtec: Add user interface documentation
>   switchtec: Add sysfs attributes to the Switchtec driver
>   switchtec: Add IOCTLs to the Switchtec driver
> 
>  Documentation/ABI/testing/sysfs-class-switchtec |   96 ++
>  Documentation/ioctl/ioctl-number.txt|1 +
>  Documentation/switchtec.txt |   80 

Re: [PATCH v5 0/4] New Microsemi PCI Switch Management Driver

[Bjorn Helgaas]

On Tue, Feb 28, 2017 at 10:11:56AM -0700, Logan Gunthorpe wrote:
> 
> > This driver doesn't have anything to do with the PCI core, other than
> > using the pci_register_driver() interface (just like all other drivers
> > for PCI-connected devices), so drivers/pci doesn't really feel like
> > the right place for it.  Putting it in drivers/pci leads to the sort
> > of confusion you mentioned above ("To make this entirely clear ...").
> > 
> > Would drivers/perf or drivers/misc/switchtec/ be possible places for
> > it?
> 
> I made a similar argument when we made the decision of where to put the
> code. In the end, the device _is_ a PCI Switch and someone going through
> menuconfig or the source tree would probably look there first.
> 
> As for drivers/perf, our device does a fair bit more than performance
> counters so it doesn't seem like it really fits in there. drivers/misc
> just seems like a dumping ground which we'd prefer not to contribute to.
> We also considered drivers/char (seeing it exposes a char device), but
> that also seems like a dumping ground with stuff that belongs and other
> stuff that just ended up stuck between the cracks.
> 
> If you still feel strongly about this we can move it into misc, but I
> think from an organizational perspective pci/switch makes a bit more sense.

It's not a perfect fit in drivers/pci because it's not bus
infrastructure and I don't want to be the default maintainer of it,
but I agree there's not really an alternative that's clearly better,
so let's leave it where it is for now.

> In any case, I also wish we could have had this discussion 3 months ago
> when we posted the RFC and not when I have people pushing to get this
> merged.

You and me both!  I hope some of those same people are pushing to help
avoid the tragedy of the commons by helping review other
contributions.

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 0/4] New Microsemi PCI Switch Management Driver

[Bjorn Helgaas]

On Sat, Feb 25, 2017 at 11:53:13PM -0700, Logan Gunthorpe wrote:
> Changes since v4:
> 
> * Turns out pushing the pci release code into the device release
>   function didn't work as I would have liked. If you try to unbind the
>   device with an instance open, then you hit a kernel bug at
>   drivers/pci/msi.c:371. (This didn't occur in v3.)

This is in free_msi_irqs():

  368for_each_pci_msi_entry(entry, dev)
  369if (entry->irq)
  370for (i = 0; i < entry->nvec_used; i++)
  371BUG_ON(irq_has_action(entry->irq + i));

I don't think this is indicating a bug in the PCI core (although I do
think a BUG_ON() here is an excessive response).  I think it's an
indication that the driver didn't disconnect its ISR.  Without more
details of the failure it's hard to tell if the BUG_ON is a symptom of
a problem in the driver or what.

An "alive" flag feels racy, and I can't tell if it's really the best
way to deal with this, or if it's just avoiding the issue.  There must
be other drivers with the same cleanup issue -- do they handle it the
same way?

>   To solve this, we've moved the pci release code back into the
>   unregister function and reintroduced an alive flag. This time,
>   however, the alive flag is protected by mrpc_mutex and we're very
>   careful about what happens to devices still in use (they should
>   all be released through the timeout path and an ENODEV error
>   returned to userspace; while new commands are blocked with the
>   same error).
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 0/4] New Microsemi PCI Switch Management Driver

[Bjorn Helgaas]

Hi Logan,

On Sat, Feb 25, 2017 at 11:53:13PM -0700, Logan Gunthorpe wrote:
> ...
> This is a continuation of the RFC we posted lasted month [1] which
> proposes a management driver for Microsemi's Switchtec line of PCI
> switches. This hardware is still looking to be used in the Open
> Compute Platform
> 
> To make this entirely clear: the Switchtec products are compliant
> with the PCI specifications and are supported today with the standard
> in-kernel driver. However, these devices also expose a management endpoint
> on a separate PCI function address which can be used to perform some
> advanced operations. This is a driver for that function. See the patch
> for more information.
> 
> Since the RFC, we've made the changes requested by Greg Kroah-Hartman
> and Keith Busch, and we've also fleshed out a number of features. We've
> added a couple of IOCTLs and sysfs attributes which are documented in
> the patch. Significant work has also been done on the userspace tool
> which is available under a GPL license at [2]. We've also had testing
> done by some of the interested parties.
> 
> We hope to see this work included in either 4.11 or 4.12 assuming a
> smooth review process.
> 
> The patch is based off of the v4.10-rc6 release.
> 
> Thanks for your review,
> 
> Logan
> 
> [1] https://www.spinics.net/lists/linux-pci/msg56897.html
> [2] https://github.com/sbates130272/switchtec-user
> 
> Logan Gunthorpe (4):
>   MicroSemi Switchtec management interface driver
>   switchtec: Add user interface documentation
>   switchtec: Add sysfs attributes to the Switchtec driver
>   switchtec: Add IOCTLs to the Switchtec driver
> 
>  Documentation/ABI/testing/sysfs-class-switchtec |   96 ++
>  Documentation/ioctl/ioctl-number.txt|1 +
>  Documentation/switchtec.txt |   80 ++
>  MAINTAINERS |   11 +
>  drivers/pci/Kconfig |1 +
>  drivers/pci/Makefile|1 +
>  drivers/pci/switch/Kconfig  |   13 +
>  drivers/pci/switch/Makefile |1 +
>  drivers/pci/switch/switchtec.c  | 1535 
> +++
>  include/uapi/linux/switchtec_ioctl.h|  132 ++
>  10 files changed, 1871 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-class-switchtec
>  create mode 100644 Documentation/switchtec.txt
>  create mode 100644 drivers/pci/switch/Kconfig
>  create mode 100644 drivers/pci/switch/Makefile
>  create mode 100644 drivers/pci/switch/switchtec.c
>  create mode 100644 include/uapi/linux/switchtec_ioctl.h

This driver doesn't have anything to do with the PCI core, other than
using the pci_register_driver() interface (just like all other drivers
for PCI-connected devices), so drivers/pci doesn't really feel like
the right place for it.  Putting it in drivers/pci leads to the sort
of confusion you mentioned above ("To make this entirely clear ...").

Would drivers/perf or drivers/misc/switchtec/ be possible places for
it?

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/4] switchtec: Add sysfs attributes to the Switchtec driver

[Bjorn Helgaas]

On Thu, Feb 23, 2017 at 03:56:21PM -0700, Logan Gunthorpe wrote:
> 
> 
> On 23/02/17 03:43 PM, Bjorn Helgaas wrote:
> > This path seems a little generic.  I don't see other cases where a
> > product brand name ("Switchtec") appears at the top level of
> > /sys/class/...
> 
> Ok, well we are certainly open to suggestions, but there isn't really a
> generic version of this device available so I'm not sure how we would
> change that. Per device-type classes aren't that uncommon though, a
> quick grep shows things like:
> 
> platform/chrome/cros_ec_dev.c:40:static struct class cros_class
> s390/char/raw3270.h:94:extern struct class *class3270;
> net/ethernet/hisilicon/hns/hnae.c:19:static struct class *hnae_class;
> mfd/ucb1x00-core.c:490:static struct class ucb1x00_class
> 
> > My question is based on "ls Documentation/ABI/testing/sysfs-class*",
> > not on any great knowledge of sysfs, and I see Greg has already given
> > a Reviewed-by for this, so maybe this is the right approach.
> > 
> > It does seem like the path could include a clue that this is related
> > to PCI.
> 
> I mean, we could change it to pci-switchtec or something if you think
> that would be better..?? But I'm not sure how else to accommodate this.

I'm OK with it as-is.

> > Is there a link to the switch PCI device itself, e.g., to
> > /sys/devices/pci*?  Should these attributes simply be put in a
> > subdirectory there, e.g., in
> > 
> >   /sys/devices/pci:00/:00:00.0/stats/...
> 
> Well our device shows up here in the tree:
> 
> /sys/devices/pci:00/:00:03.0/:03:00.1/switchtec/switchtec0
> 
> (Which userspace can get to by following the link at
> /sys/class/switchtec/switchtec0) The switch is then always:
> 
> /sys/devices/pci:00/:00:03.0

That's exactly what I was looking for; I just didn't realize it was
connected like this.

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/4] MicroSemi Switchtec management interface driver

[Bjorn Helgaas]

[+cc Peter, Ingo, Arnaldo, Alexander, Christoph]

On Thu, Feb 02, 2017 at 11:06:00AM -0700, Logan Gunthorpe wrote:
> Microsemi's "Switchtec" line of PCI switch devices is already well
> supported by the kernel with standard PCI switch drivers. However, the
> Switchtec device advertises a special management endpoint with a separate
> PCI function address and class code. This endpoint enables some additional
> functionality which includes:
> 
>  * Packet and Byte Counters
>  * Switch Firmware Upgrades
>  * Event and Error logs
>  * Querying port link status
>  * Custom user firmware commands

Would it make any sense to integrate this with the perf tool?  It
looks like switchtec-user measures bandwidth and latency, which sounds
sort of perf-ish.

> This patch introduces the switchtec kernel module which provides
> PCI driver that exposes a char device. The char device provides
> userspace access to this interface through read, write and (optionally)
> poll calls.
> 
> A userspace tool and library which utilizes this interface is available
> at [1]. This tool takes inspiration (and borrows some code) from
> nvme-cli [2]. The tool is largely complete at this time but additional
> features may be added in the future.
> 
> [1] https://github.com/sbates130272/switchtec-user
> [2] https://github.com/linux-nvme/nvme-cli
> 
> Signed-off-by: Logan Gunthorpe 
> Signed-off-by: Stephen Bates 
> ---
>  MAINTAINERS|8 +
>  drivers/pci/Kconfig|1 +
>  drivers/pci/Makefile   |1 +
>  drivers/pci/switch/Kconfig |   13 +
>  drivers/pci/switch/Makefile|1 +
>  drivers/pci/switch/switchtec.c | 1028 
> 
>  6 files changed, 1052 insertions(+)
>  create mode 100644 drivers/pci/switch/Kconfig
>  create mode 100644 drivers/pci/switch/Makefile
>  create mode 100644 drivers/pci/switch/switchtec.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 5f10c28..9c35198 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -9507,6 +9507,14 @@ S: Maintained
>  F:   Documentation/devicetree/bindings/pci/aardvark-pci.txt
>  F:   drivers/pci/host/pci-aardvark.c
>  
> +PCI DRIVER FOR MICROSEMI SWITCHTEC
> +M:   Kurt Schwemmer 
> +M:   Stephen Bates 
> +M:   Logan Gunthorpe 
> +L:   linux-...@vger.kernel.org
> +S:   Maintained
> +F:   drivers/pci/switch/switchtec*
> +
>  PCI DRIVER FOR NVIDIA TEGRA
>  M:   Thierry Reding 
>  L:   linux-te...@vger.kernel.org
> diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
> index 6555eb7..f72e8c5 100644
> --- a/drivers/pci/Kconfig
> +++ b/drivers/pci/Kconfig
> @@ -133,3 +133,4 @@ config PCI_HYPERV
>  
>  source "drivers/pci/hotplug/Kconfig"
>  source "drivers/pci/host/Kconfig"
> +source "drivers/pci/switch/Kconfig"
> diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
> index 8db5079..15b46dd 100644
> --- a/drivers/pci/Makefile
> +++ b/drivers/pci/Makefile
> @@ -68,3 +68,4 @@ ccflags-$(CONFIG_PCI_DEBUG) := -DDEBUG
>  
>  # PCI host controller drivers
>  obj-y += host/
> +obj-y += switch/
> diff --git a/drivers/pci/switch/Kconfig b/drivers/pci/switch/Kconfig
> new file mode 100644
> index 000..4c49648
> --- /dev/null
> +++ b/drivers/pci/switch/Kconfig
> @@ -0,0 +1,13 @@
> +menu "PCI switch controller drivers"
> + depends on PCI
> +
> +config PCI_SW_SWITCHTEC
> + tristate "MicroSemi Switchtec PCIe Switch Management Driver"
> + help
> +  Enables support for the management interface for the MicroSemi
> +  Switchtec series of PCIe switches. Supports userspace access
> +  to submit MRPC commands to the switch via /dev/switchtecX
> +  devices. See  for more
> +  information.
> +
> +endmenu
> diff --git a/drivers/pci/switch/Makefile b/drivers/pci/switch/Makefile
> new file mode 100644
> index 000..37d8cfb
> --- /dev/null
> +++ b/drivers/pci/switch/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_PCI_SW_SWITCHTEC) += switchtec.o
> diff --git a/drivers/pci/switch/switchtec.c b/drivers/pci/switch/switchtec.c
> new file mode 100644
> index 000..e4a4294
> --- /dev/null
> +++ b/drivers/pci/switch/switchtec.c
> @@ -0,0 +1,1028 @@
> +/*
> + * Microsemi Switchtec(tm) PCIe Management Driver
> + * Copyright (c) 2017, Microsemi Corporation
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +

Re: [PATCH v2 3/4] switchtec: Add sysfs attributes to the Switchtec driver

[Bjorn Helgaas]

On Thu, Feb 02, 2017 at 11:06:02AM -0700, Logan Gunthorpe wrote:
> This patch adds a few read-only sysfs attributes which provide
> some device information that is exposed from the devices. Primarily
> component and device names and versions. These are documented in
> Documentation/ABI/testing/sysfs-class-switchtec.
> 
> Signed-off-by: Logan Gunthorpe 
> Signed-off-by: Stephen Bates 
> ---
>  Documentation/ABI/testing/sysfs-class-switchtec |  96 
>  MAINTAINERS |   1 +
>  drivers/pci/switch/switchtec.c  | 113 
> 
>  3 files changed, 210 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-class-switchtec
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-switchtec 
> b/Documentation/ABI/testing/sysfs-class-switchtec
> new file mode 100644
> index 000..48cb4c1
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-switchtec
> @@ -0,0 +1,96 @@
> +switchtec - Microsemi Switchtec PCI Switch Management Endpoint
> +
> +For details on this subsystem look at Documentation/switchtec.txt.
> +
> +What:/sys/class/switchtec

This path seems a little generic.  I don't see other cases where a
product brand name ("Switchtec") appears at the top level of
/sys/class/...

My question is based on "ls Documentation/ABI/testing/sysfs-class*",
not on any great knowledge of sysfs, and I see Greg has already given
a Reviewed-by for this, so maybe this is the right approach.

It does seem like the path could include a clue that this is related
to PCI.

Is there a link to the switch PCI device itself, e.g., to
/sys/devices/pci*?  Should these attributes simply be put in a
subdirectory there, e.g., in

  /sys/devices/pci:00/:00:00.0/stats/...

instead of in /sys/class/?  It seems like it'd be useful for userspace
to be able to connect this info with the switch somehow.

> +Date:05-Jan-2017
> +KernelVersion:   v4.11
> +Contact: Logan Gunthorpe 
> +Description: The switchtec class subsystem folder.
> + Each registered switchtec driver is represented by a switchtecX
> + subfolder (X being an integer >= 0).
> +
> +
> +What:/sys/class/switchtec/switchtec[0-9]+/component_id
> +Date:05-Jan-2017
> +KernelVersion:   v4.11
> +Contact: Logan Gunthorpe 
> +Description: Component identifier as stored in the hardware (eg. PM8543)
> + (read only)
> +Values:  arbitrary string.
> +
> +
> +What:/sys/class/switchtec/switchtec[0-9]+/component_revision
> +Date:05-Jan-2017
> +KernelVersion:   v4.11
> +Contact: Logan Gunthorpe 
> +Description: Component revision stored in the hardware (read only)
> +Values:  integer.
> +
> +
> +What:/sys/class/switchtec/switchtec[0-9]+/component_vendor
> +Date:05-Jan-2017
> +KernelVersion:   v4.11
> +Contact: Logan Gunthorpe 
> +Description: Component vendor as stored in the hardware (eg. MICROSEM)
> + (read only)
> +Values:  arbitrary string.
> +
> +
> +What:/sys/class/switchtec/switchtec[0-9]+/device_version
> +Date:05-Jan-2017
> +KernelVersion:   v4.11
> +Contact: Logan Gunthorpe 
> +Description: Device version as stored in the hardware (read only)
> +Values:  integer.
> +
> +
> +What:/sys/class/switchtec/switchtec[0-9]+/fw_version
> +Date:05-Jan-2017
> +KernelVersion:   v4.11
> +Contact: Logan Gunthorpe 
> +Description: Currently running firmware version (read only)
> +Values:  integer (in hexadecimal).
> +
> +
> +What:/sys/class/switchtec/switchtec[0-9]+/partition
> +Date:05-Jan-2017
> +KernelVersion:   v4.11
> +Contact: Logan Gunthorpe 
> +Description: Partition number for this device in the switch (read only)
> +Values:  integer.
> +
> +
> +What:/sys/class/switchtec/switchtec[0-9]+/partition_count
> +Date:05-Jan-2017
> +KernelVersion:   v4.11
> +Contact: Logan Gunthorpe 
> +Description: Total number of partitions in the switch (read only)
> +Values:  integer.
> +
> +
> +What:/sys/class/switchtec/switchtec[0-9]+/product_id
> +Date:05-Jan-2017
> +KernelVersion:   v4.11
> +Contact: Logan Gunthorpe 
> +Description: Product identifier as stored in the hardware (eg. PSX 48XG3)
> + (read only)
> +Values:  arbitrary string.
> +
> +
> +What:/sys/class/switchtec/switchtec[0-9]+/product_revision
> +Date:05-Jan-2017
> +KernelVersion:   v4.11
> +Contact: Logan Gunthorpe 

Re: [PATCH v2 0/4] New Microsemi PCI Switch Management Driver

[Bjorn Helgaas]

On Thu, Feb 23, 2017 at 01:36:51PM -0700, Logan Gunthorpe wrote:
> Hello,
> 
> We're still waiting on any kind of response from Bjorn. (If you're
> listening please say something!)
> 
> Does anyone have any suggestions for dealing with an unresponsive
> maintainer? Or a way for us to move forward with this quickly and get it
> merged?

I try to deal with regressions first and other bug fixes second.
After that, I look at things that add new functionality.  I try to
look at the new stuff in roughly chronological order, as you would see
here:

https://patchwork.ozlabs.org/project/linux-pci/list/?order=date=1

If other folks have feedback, as they did on your 12/17, 1/31, and
even the 2/2 posting, I generally let that get sorted out before I
look at it.  I apologize that I haven't responded to your queries
about posting a v3 vs updating v2.

To answer that question, it's much simpler for me to deal with a
fresh, clean new series than it is to tweak things in an
already-posted series, partly because a series with discussion other
than simple acks and reviewed-bys looks more like work-in-progress.

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: two small PCI documentation fixups

[Bjorn Helgaas]

On Wed, Feb 15, 2017 at 08:58:21AM +0100, Christoph Hellwig wrote:
> Avoid mentioning deprecated APIs and make the text a little
> easier to understand.

Applied to pci/msi for v4.11, thanks!
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 00/37] PCI: Support for configurable PCI endpoint

[Bjorn Helgaas]

On Wed, Feb 01, 2017 at 06:01:58PM +0530, Kishon Vijay Abraham I wrote:
> - list
> 
> Hi Bjorn,
> 
> How do you want to handle this series? I'll send one more version of the 
> series
> including the directory restructuring in the same series. Should it be based 
> on
> your -next?

I think if you base it on my pci/host-designware branch, it should
work pretty well.  I haven't merged that into -next yet, but I will
soon.

I'm not sure there was consensus on all the patches yet, but if you
can group the easy ones at the beginning, I should be able to at least
get quite a few out of the way.

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] pci: drop link_reset

[Bjorn Helgaas]

On Tue, Jan 24, 2017 at 07:35:56PM +0200, Michael S. Tsirkin wrote:
> No hardware seems to actually call link_reset, and
> no driver implements it as more than a nop stub.
> 
> This drops the mentions of the callback from everywhere.
> It's dropped from the documentation as well, but
> the doc really needs to be updated to reflect
> reality better (e.g. on pcie slot reset is the link reset).
> 
> This will be done in a later patch.
> 
> Signed-off-by: Michael S. Tsirkin 

Applied to pci/aer for v4.11, thanks.

> ---
> 
> changes from v2:
>   - drop from genwqe as well
> 
> Note: Doug has patches dropping the implementation from infiniband card
> drivers in his tree already. This is unlikely to cause conflicts though.
> 
>  Documentation/PCI/pci-error-recovery.txt | 24 +++-
>  drivers/infiniband/hw/hfi1/pcie.c| 10 --
>  drivers/infiniband/hw/qib/qib_pcie.c |  8 
>  drivers/media/pci/ngene/ngene-cards.c|  7 ---
>  drivers/misc/genwqe/card_base.c  |  1 -
>  include/linux/pci.h  |  3 ---
>  6 files changed, 3 insertions(+), 50 deletions(-)
> 
> diff --git a/Documentation/PCI/pci-error-recovery.txt 
> b/Documentation/PCI/pci-error-recovery.txt
> index ac26869..da3b217 100644
> --- a/Documentation/PCI/pci-error-recovery.txt
> +++ b/Documentation/PCI/pci-error-recovery.txt
> @@ -78,7 +78,6 @@ struct pci_error_handlers
>  {
>   int (*error_detected)(struct pci_dev *dev, enum pci_channel_state);
>   int (*mmio_enabled)(struct pci_dev *dev);
> - int (*link_reset)(struct pci_dev *dev);
>   int (*slot_reset)(struct pci_dev *dev);
>   void (*resume)(struct pci_dev *dev);
>  };
> @@ -104,8 +103,7 @@ if it implements any, it must implement error_detected(). 
> If a callback
>  is not implemented, the corresponding feature is considered unsupported.
>  For example, if mmio_enabled() and resume() aren't there, then it
>  is assumed that the driver is not doing any direct recovery and requires
> -a slot reset. If link_reset() is not implemented, the card is assumed to
> -not care about link resets. Typically a driver will want to know about
> +a slot reset.  Typically a driver will want to know about
>  a slot_reset().
>  
>  The actual steps taken by a platform to recover from a PCI error
> @@ -232,25 +230,9 @@ proceeds to STEP 4 (Slot Reset)
>  
>  STEP 3: Link Reset
>  --
> -The platform resets the link, and then calls the link_reset() callback
> -on all affected device drivers.  This is a PCI-Express specific state
> +The platform resets the link.  This is a PCI-Express specific step
>  and is done whenever a non-fatal error has been detected that can be
> -"solved" by resetting the link. This call informs the driver of the
> -reset and the driver should check to see if the device appears to be
> -in working condition.
> -
> -The driver is not supposed to restart normal driver I/O operations
> -at this point.  It should limit itself to "probing" the device to
> -check its recoverability status. If all is right, then the platform
> -will call resume() once all drivers have ack'd link_reset().
> -
> - Result codes:
> - (identical to STEP 3 (MMIO Enabled)
> -
> -The platform then proceeds to either STEP 4 (Slot Reset) or STEP 5
> -(Resume Operations).
> -
> ->>> The current powerpc implementation does not implement this callback.
> +"solved" by resetting the link.
>  
>  STEP 4: Slot Reset
>  --
> diff --git a/drivers/infiniband/hw/hfi1/pcie.c 
> b/drivers/infiniband/hw/hfi1/pcie.c
> index 4ac8f33..ebd941f 100644
> --- a/drivers/infiniband/hw/hfi1/pcie.c
> +++ b/drivers/infiniband/hw/hfi1/pcie.c
> @@ -598,15 +598,6 @@ pci_slot_reset(struct pci_dev *pdev)
>   return PCI_ERS_RESULT_CAN_RECOVER;
>  }
>  
> -static pci_ers_result_t
> -pci_link_reset(struct pci_dev *pdev)
> -{
> - struct hfi1_devdata *dd = pci_get_drvdata(pdev);
> -
> - dd_dev_info(dd, "HFI1 link_reset function called, ignored\n");
> - return PCI_ERS_RESULT_CAN_RECOVER;
> -}
> -
>  static void
>  pci_resume(struct pci_dev *pdev)
>  {
> @@ -625,7 +616,6 @@ pci_resume(struct pci_dev *pdev)
>  const struct pci_error_handlers hfi1_pci_err_handler = {
>   .error_detected = pci_error_detected,
>   .mmio_enabled = pci_mmio_enabled,
> - .link_reset = pci_link_reset,
>   .slot_reset = pci_slot_reset,
>   .resume = pci_resume,
>  };
> diff --git a/drivers/infiniband/hw/qib/qib_pcie.c 
> b/drivers/infiniband/hw/qib/qib_pcie.c
> index 6abe1c6..c379b83 100644
> --- a/drivers/infiniband/hw/qib/qib_pcie.c
> +++ b/drivers/infiniband/hw/qib/qib_pcie.c
> @@ -682,13 +682,6 @@ qib_pci_slot_reset(struct pci_dev *pdev)
>   return PCI_ERS_RESULT_CAN_RECOVER;
>  }
>  
> -static pci_ers_result_t
> -qib_pci_link_reset(struct pci_dev *pdev)
> -{
> - qib_devinfo(pdev, "QIB link_reset function called, ignored\n");
> - return PCI_ERS_RESULT_CAN_RECOVER;
> -}
> -
>  static 

Re: [PATCH] CodingStyle: Expand IS_ENABLED() documentation

[Bjorn Helgaas]

On Tue, Sep 27, 2016 at 09:32:08PM +0200, Paul Bolle wrote:
> On Tue, 2016-09-27 at 14:08 -0500, Bjorn Helgaas wrote:
> 
> > --- a/Documentation/CodingStyle
> > +++ b/Documentation/CodingStyle
> 
> > +Because the compiler processes the block, you have to use an #ifdef instead
> > +of IS_ENABLED() when code inside the block references symbols that will not
> > +exist if the condition is not met.  Different CONFIG_FOO autoconf.h symbols
> > +are generated for modular Kconfig options than for builtin ones, so you
> > +need "#if defined(CONFIG_FOO) || defined(CONFIG_FOO_MODULE)"
> 
> Isn't that equivalent to "#if IS_ENABLED(CONFIG_FOO)"?

Yep, I think so.  Sigh.  I'll look again.  Maybe what's already there can't
be improved.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] CodingStyle: Expand IS_ENABLED() documentation

[Bjorn Helgaas]

CodingStyle recommends IS_ENABLED(CONFIG_FOO) over #ifdef.  Add an example
of the #ifdef, since it's not completely obvious that in many cases the
#ifdef needs to test both CONFIG_FOO and CONFIG_FOO_MODULE.

Signed-off-by: Bjorn Helgaas <bhelg...@google.com>
---
 Documentation/CodingStyle |   15 +--
 include/linux/kconfig.h   |3 ++-
 2 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/Documentation/CodingStyle b/Documentation/CodingStyle
index a096836..b3e9743 100644
--- a/Documentation/CodingStyle
+++ b/Documentation/CodingStyle
@@ -911,8 +911,19 @@ The compiler will constant-fold the conditional away, and 
include or exclude
 the block of code just as with an #ifdef, so this will not add any runtime
 overhead.  However, this approach still allows the C compiler to see the code
 inside the block, and check it for correctness (syntax, types, symbol
-references, etc).  Thus, you still have to use an #ifdef if the code inside the
-block references symbols that will not exist if the condition is not met.
+references, etc).
+
+Because the compiler processes the block, you have to use an #ifdef instead
+of IS_ENABLED() when code inside the block references symbols that will not
+exist if the condition is not met.  Different CONFIG_FOO autoconf.h symbols
+are generated for modular Kconfig options than for builtin ones, so you
+need "#if defined(CONFIG_FOO) || defined(CONFIG_FOO_MODULE)" if FOO can be
+a module:
+
+   .config include/generated/autoconf.h
+   
+   CONFIG_FOO=y#define CONFIG_FOO 1
+   CONFIG_FOO=m#define CONFIG_FOO_MODULE 1
 
 At the end of any non-trivial #if or #ifdef block (more than a few lines),
 place a comment after the #endif on the same line, noting the conditional
diff --git a/include/linux/kconfig.h b/include/linux/kconfig.h
index 15ec117..51a5f66 100644
--- a/include/linux/kconfig.h
+++ b/include/linux/kconfig.h
@@ -60,7 +60,8 @@
 
 /*
  * IS_ENABLED(CONFIG_FOO) evaluates to 1 if CONFIG_FOO is set to 'y' or 'm',
- * 0 otherwise.
+ * 0 otherwise.  Note that CONFIG_FOO=y results in "#define CONFIG_FOO 1"
+ * in autoconf.h, while CONFIG_FOO=m results in "#define CONFIG_FOO_MODULE 1"
  */
 #define IS_ENABLED(option) __or(IS_BUILTIN(option), IS_MODULE(option))
 

--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RESEND PATCH v2 4/4] PCI: Add support for enforcing all MMIO BARs to be page aligned

[Bjorn Helgaas]

On Thu, Jun 02, 2016 at 01:46:51PM +0800, Yongji Xie wrote:
> When vfio passthrough a PCI device of which MMIO BARs are
> smaller than PAGE_SIZE, guest will not handle the mmio
> accesses to the BARs which leads to mmio emulations in host.
> 
> This is because vfio will not allow to passthrough one BAR's
> mmio page which may be shared with other BARs. Otherwise,
> there will be a backdoor that guest can use to access BARs
> of other guest.
> 
> To solve this issue, this patch modifies resource_alignment
> to support syntax where multiple devices get the same
> alignment. So we can use something like
> "pci=resource_alignment=*:*:*.*:noresize" to enforce the
> alignment of all MMIO BARs to be at least PAGE_SIZE so that
> one BAR's mmio page would not be shared with other BARs.
> 
> And we also define a macro PCIBIOS_MIN_ALIGNMENT to enable this
> automatically on PPC64 platform which can easily hit this issue
> because its PAGE_SIZE is 64KB.
> 
> Note that this would not be applied to VFs whose BARs are always
> page aligned and should be never reassigned according to SRIOV
> spec.

I see that SR-IOV spec r1.1, sec 3.3.13 requires that all VF BAR
resources be aligned on System Page Size, and must be sized to consume
an integral number of pages.

Where does it say VF BARs can't be reassigned?  I thought they *could*
be reassigned, as long as VFs are disabled when you do it.

> Signed-off-by: Yongji Xie 
> ---
>  Documentation/kernel-parameters.txt |2 ++
>  arch/powerpc/include/asm/pci.h  |2 ++
>  drivers/pci/pci.c   |   68 
> +--
>  3 files changed, 61 insertions(+), 11 deletions(-)
> 
> diff --git a/Documentation/kernel-parameters.txt 
> b/Documentation/kernel-parameters.txt
> index c4802f5..cb09503 100644
> --- a/Documentation/kernel-parameters.txt
> +++ b/Documentation/kernel-parameters.txt
> @@ -3003,6 +3003,8 @@ bytes respectively. Such letter suffixes can also be 
> entirely omitted.
>   aligned memory resources.
>   If  is not specified,
>   PAGE_SIZE is used as alignment.
> + , ,  and  can be set to
> + "*" which means match all values.
>   PCI-PCI bridge can be specified, if resource
>   windows need to be expanded.
>   noresize: Don't change the resources' sizes when
> diff --git a/arch/powerpc/include/asm/pci.h b/arch/powerpc/include/asm/pci.h
> index a6f3ac0..742fd34 100644
> --- a/arch/powerpc/include/asm/pci.h
> +++ b/arch/powerpc/include/asm/pci.h
> @@ -28,6 +28,8 @@
>  #define PCIBIOS_MIN_IO   0x1000
>  #define PCIBIOS_MIN_MEM  0x1000
>  
> +#define PCIBIOS_MIN_ALIGNMENT  PAGE_SIZE
> +
>  struct pci_dev;
>  
>  /* Values for the `which' argument to sys_pciconfig_iobase syscall.  */
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 3ee13e5..664f295 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -4759,7 +4759,12 @@ static resource_size_t 
> pci_specified_resource_alignment(struct pci_dev *dev,
>   int seg, bus, slot, func, align_order, count;
>   resource_size_t align = 0;
>   char *p;
> + bool invalid = false;
>  
> +#ifdef PCIBIOS_MIN_ALIGNMENT
> + align = PCIBIOS_MIN_ALIGNMENT;
> + *resize = false;
> +#endif

This PCIBIOS_MIN_ALIGNMENT part should be a separate patch by itself.

If you have PCIBIOS_MIN_ALIGNMENT enabled automatically for powerpc,
do you still need the command-line argument?

>   spin_lock(_alignment_lock);
>   p = resource_alignment_param;
>   while (*p) {
> @@ -4776,16 +4781,49 @@ static resource_size_t 
> pci_specified_resource_alignment(struct pci_dev *dev,
>   } else {
>   align_order = -1;
>   }
> - if (sscanf(p, "%x:%x:%x.%x%n",
> - , , , , ) != 4) {
> + if (p[0] == '*' && p[1] == ':') {
> + seg = -1;
> + count = 1;
> + } else if (sscanf(p, "%x%n", , ) != 1 ||
> + p[count] != ':') {
> + invalid = true;
> + break;
> + }
> + p += count + 1;
> + if (*p == '*') {
> + bus = -1;
> + count = 1;
> + } else if (sscanf(p, "%x%n", , ) != 1) {
> + invalid = true;
> + break;
> + }
> + p += count;
> + if (*p == '.') {
> + slot = bus;
> + bus = seg;
>   seg = 0;
> - if (sscanf(p, "%x:%x.%x%n",
> - , , , ) != 3) {
> - /* Invalid format */
> - 

Re: [RESEND PATCH v2 2/4] PCI: Do not Use IORESOURCE_STARTALIGN to identify bridge resources

[Bjorn Helgaas]

On Thu, Jun 02, 2016 at 01:46:49PM +0800, Yongji Xie wrote:
> Now we use the IORESOURCE_STARTALIGN to identify bridge resources
> in __assign_resources_sorted(). That's quite fragile. We can't
> make sure that the PCI devices' resources will not use
> IORESOURCE_STARTALIGN any more.

Can you explain this a little more?  I don't quite understand the
problem.  Maybe you can give an example of the problem?

> In this patch, we try to use a more robust way to identify
> bridge resources.
> 
> Signed-off-by: Yongji Xie 
> ---
>  drivers/pci/setup-bus.c |9 ++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> index 55641a3..216ddbc 100644
> --- a/drivers/pci/setup-bus.c
> +++ b/drivers/pci/setup-bus.c
> @@ -390,6 +390,7 @@ static void __assign_resources_sorted(struct list_head 
> *head,
>   struct pci_dev_resource *dev_res, *tmp_res, *dev_res2;
>   unsigned long fail_type;
>   resource_size_t add_align, align;
> + int index;
>  
>   /* Check if optional add_size is there */
>   if (!realloc_head || list_empty(realloc_head))
> @@ -410,11 +411,13 @@ static void __assign_resources_sorted(struct list_head 
> *head,
>  
>   /*
>* There are two kinds of additional resources in the list:
> -  * 1. bridge resource  -- IORESOURCE_STARTALIGN
> -  * 2. SR-IOV resource   -- IORESOURCE_SIZEALIGN
> +  * 1. bridge resource
> +  * 2. SR-IOV resource
>* Here just fix the additional alignment for bridge
>*/
> - if (!(dev_res->res->flags & IORESOURCE_STARTALIGN))
> + index = dev_res->res - dev_res->dev->resource;
> + if (index < PCI_BRIDGE_RESOURCES ||
> + index > PCI_BRIDGE_RESOURCE_END)

I think the code looks OK; at least, it seems to match the comment.
I'd just like to understand the problem a little better.

>   continue;
>  
>   add_align = get_res_add_align(realloc_head, dev_res->res);
> -- 
> 1.7.9.5
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RESEND PATCH v2 3/4] PCI: Add a new option for resource_alignment to reassign alignment

[Bjorn Helgaas]

On Thu, Jun 02, 2016 at 01:46:50PM +0800, Yongji Xie wrote:
> When using resource_alignment kernel parameter, the current
> implement reassigns the alignment by changing resources' size
> which can potentially break some drivers. For example, the driver
> uses the size to locate some register whose length is related
> to the size.
> 
> This patch adds a new option "noresize" for the parameter to
> solve this problem.

Why do we ever want to change the resource's size?  I understand that
you want to change the resource's *alignment*, and that part makes
sense.  But why change the *size*?  Changing the resource size doesn't
change the hardware BAR size; it just means the struct resource will
describe a region larger than what the BAR actually claims.  That
unnecessarily wastes space after the BAR.

This was a problem with the code even before your patch; I'm
suggesting that if you have a way to change the alignment without
changing the resource size, maybe we should do that all the time.
Then you wouldn't need to add the "noresize" option.

> Signed-off-by: Yongji Xie 
> ---
>  Documentation/kernel-parameters.txt |5 -
>  drivers/pci/pci.c   |   35 
> +--
>  2 files changed, 29 insertions(+), 11 deletions(-)
> 
> diff --git a/Documentation/kernel-parameters.txt 
> b/Documentation/kernel-parameters.txt
> index 82b42c9..c4802f5 100644
> --- a/Documentation/kernel-parameters.txt
> +++ b/Documentation/kernel-parameters.txt
> @@ -2997,13 +2997,16 @@ bytes respectively. Such letter suffixes can also be 
> entirely omitted.
>   window. The default value is 64 megabytes.
>   resource_alignment=
>   Format:
> - [ align>@][:]:.[; ...]
> + [ align>@][:]:.
> + [:noresize][; ...]
>   Specifies alignment and device to reassign
>   aligned memory resources.
>   If  is not specified,
>   PAGE_SIZE is used as alignment.
>   PCI-PCI bridge can be specified, if resource
>   windows need to be expanded.
> + noresize: Don't change the resources' sizes when
> + reassigning alignment.
>   ecrc=   Enable/disable PCIe ECRC (transaction layer
>   end-to-end CRC checking).
>   bios: Use BIOS/firmware settings. This is the
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index a259394..3ee13e5 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -4748,11 +4748,13 @@ static DEFINE_SPINLOCK(resource_alignment_lock);
>  /**
>   * pci_specified_resource_alignment - get resource alignment specified by 
> user.
>   * @dev: the PCI device to get
> + * @resize: whether or not to change resources' size when reassigning 
> alignment
>   *
>   * RETURNS: Resource alignment if it is specified.
>   *  Zero if it is not specified.
>   */
> -static resource_size_t pci_specified_resource_alignment(struct pci_dev *dev)
> +static resource_size_t pci_specified_resource_alignment(struct pci_dev *dev,
> + bool *resize)
>  {
>   int seg, bus, slot, func, align_order, count;
>   resource_size_t align = 0;
> @@ -4786,6 +4788,11 @@ static resource_size_t 
> pci_specified_resource_alignment(struct pci_dev *dev)
>   }
>   }
>   p += count;
> + if (!strncmp(p, ":noresize", 9)) {
> + *resize = false;
> + p += 9;
> + } else
> + *resize = true;
>   if (seg == pci_domain_nr(dev->bus) &&
>   bus == dev->bus->number &&
>   slot == PCI_SLOT(dev->devfn) &&
> @@ -4818,11 +4825,12 @@ void pci_reassigndev_resource_alignment(struct 
> pci_dev *dev)
>  {
>   int i;
>   struct resource *r;
> + bool resize = true;
>   resource_size_t align, size;
>   u16 command;
>  
>   /* check if specified PCI is target device to reassign */
> - align = pci_specified_resource_alignment(dev);
> + align = pci_specified_resource_alignment(dev, );
>   if (!align)
>   return;
>  
> @@ -4844,15 +4852,22 @@ void pci_reassigndev_resource_alignment(struct 
> pci_dev *dev)
>   if (!(r->flags & IORESOURCE_MEM))
>   continue;
>   size = resource_size(r);
> - if (size < align) {
> - size = align;
> - dev_info(>dev,
> - "Rounding up size of resource #%d to %#llx.\n",
> - i, (unsigned long long)size);
> + if (resize) {
> + if (size < 

Re: [RESEND PATCH v2 1/4] PCI: Ignore resource_alignment if PCI_PROBE_ONLY was set\\

[Bjorn Helgaas]

On Thu, Jun 02, 2016 at 01:46:48PM +0800, Yongji Xie wrote:
> The resource_alignment will releases memory resources allocated
> by firmware so that kernel can reassign new resources later on.
> But this will cause the problem that no resources can be
> allocated by kernel if PCI_PROBE_ONLY was set, e.g. on pSeries
> platform because PCI_PROBE_ONLY force kernel to use firmware
> setup and not to reassign any resources.
> 
> To solve this problem, this patch ignores resource_alignment
> if PCI_PROBE_ONLY was set.
> 
> Signed-off-by: Yongji Xie 
> ---
>  drivers/pci/pci.c |6 ++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index c8b4dbd..a259394 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -4761,6 +4761,12 @@ static resource_size_t 
> pci_specified_resource_alignment(struct pci_dev *dev)
>   spin_lock(_alignment_lock);
>   p = resource_alignment_param;
>   while (*p) {
> + if (pci_has_flag(PCI_PROBE_ONLY)) {
> + printk(KERN_ERR "PCI: Ignore resource_alignment 
> parameter: %s with PCI_PROBE_ONLY set\n",
> + p);
> + *p = 0;
> + break;

Wouldn't it be simpler to make pci_set_resource_alignment_param() fail
if PCI_PROBE_ONLY is set?

> + }
>   count = 0;
>   if (sscanf(p, "%d%n", _order, ) == 1 &&
>   p[count] == '@') {
> -- 
> 1.7.9.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
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v4 0/7] vfio-pci: Allow to mmap sub-page MMIO BARs and MSI-X table

[Bjorn Helgaas]

On Wed, Mar 16, 2016 at 06:51:56PM +0800, Yongji Xie wrote:
> 
> Ping.

This is mainly VFIO stuff, and Alex had some security concerns, so I'm
not going to spend much time looking at this until he's satisfied.

When I do, I'll be looking hard at the resource_alignment kernel
parameter.  I'm opposed to kernel parameters in general because
they're very difficult for users to use correctly, and they lead to
kernel code paths that are rarely tested and hard to maintain.  So
I'll be looking for an excuse to reject changes in that area.

The changelog for 2/7 says it "replaces IORESOURCE_STARTALIGN with
IORESOURCE_WINDOW."  But even a glance at the patch itself shows that
IORESOURCE_WINDOW is *added* to some places, and it doesn't *replace*
IORESOURCE_STARTALIGN.

The changelog for 4/7 says:

  This is because vfio will not allow to passthrough one BAR's mmio
  page which may be shared with other BARs.  To solve this performance
  issue ...

with no mention at all of the actual *reason* vfio doesn't allow that
passthrough.  If I understand correctly, that reason has to do with
security, so your justification must be much stronger than "solving a
performance issue."

> On 2016/3/7 15:48, Yongji Xie wrote:
> >Current vfio-pci implementation disallows to mmap
> >sub-page(size < PAGE_SIZE) MMIO BARs and MSI-X table. This is because
> >sub-page BARs' mmio page may be shared with other BARs and MSI-X table
> >should not be accessed directly from the guest for security reasons.
> >
> >But these will easily cause some performance issues for mmio accesses
> >in guest when vfio passthrough sub-page BARs or BARs containing MSI-X
> >table on PPC64 platform. This is because PAGE_SIZE is 64KB by default
> >on PPC64 platform and the big page may easily hit the sub-page MMIO
> >BARs' unmmapping and cause the unmmaping of the mmio page which
> >MSI-X table locate in, which lead to mmio emulation in host.
> >
> >For sub-page MMIO BARs' unmmapping, this patchset modifies
> >resource_alignment kernel parameter to enforce the alignment of all
> >MMIO BARs to be at least PAGE_SZIE so that sub-page BAR's mmio page
> >will not be shared with other BARs. Then we can mmap sub-page MMIO BARs
> >in vfio-pci driver with the modified resource_alignment.
> >
> >For MSI-X table's unmmapping, we think MSI-X table is safe to access
> >directly from userspace if PCI host bridge support filtering of MSIs
> >which can ensure that a given pci device can only shoot the MSIs
> >assigned for it. So we allow to mmap MSI-X table if
> >IOMMU_CAP_INTR_REMAP was set. And we add IOMMU_CAP_INTR_REMAP for
> >IODA host bridge on PPC64 platform.
> >
> >With this patchset applied, we can get almost 100% improvement on
> >performance for mmio accesses when we passthrough sub-page BARs to guest
> >in our test.
> >
> >The two vfio related patches(patch 5 and patch 6) are based on the
> >proposed patchset[1].
> >
> >Changelog v4:
> >- Rebase on v4.5-rc6 with patchset[1] applied.
> >- Remove resource_page_aligned kernel parameter
> >- Fix some problems with resource_alignment kernel parameter
> >- Modify resource_alignment kernel parameter to support multiple
> >   devices.
> >- Remove host bridge attribute: msi_filtered
> >- Use IOMMU_CAP_INTR_REMAP to check if MSI-X table can be mmapped
> >- Add IOMMU_CAP_INTR_REMAP for IODA host bridge on PPC64 platform
> >
> >Changelog v3:
> >- Rebase on new linux kernel mainline with the patchset[1] applied.
> >- Add a function to check whether PCI BARs'mmio page is shared with
> >   other BARs.
> >- Add a host bridge attribute to indicate PCI host bridge support
> >   filtering of MSIs.
> >- Use the new host bridge attribute to check if MSI-X table can
> >   be mmapped instead of CONFIG_EEH.
> >- Remove Kconfig option VFIO_PCI_MMAP_MSIX
> >
> >Changelog v2:
> >- Rebase on v4.4-rc6 with the patchset[1] applied.
> >- Use kernel parameter to enforce all MMIO BARs to be page aligned
> >   on PCI core code instead of doing it on PPC64 arch code.
> >- Remove flags: VFIO_DEVICE_FLAGS_PCI_PAGE_ALIGNED
> > VFIO_DEVICE_FLAGS_PCI_MSIX_MMAP
> >- Add a Kconfig option to support for mmapping MSI-X table.
> >
> >[1] http://www.spinics.net/lists/kvm/msg127812.html
> >
> >Yongji Xie (7):
> >   PCI: Add a new option for resource_alignment to reassign alignment
> >   PCI: Use IORESOURCE_WINDOW to identify bridge resources
> >   PCI: Ignore resource_alignment if PCI_PROBE_ONLY was set
> >   PCI: Modify resource_alignment to support multiple devices
> >   vfio-pci: Allow to mmap sub-page MMIO BARs if the mmio page is exclusive
> >   vfio-pci: Allow to mmap MSI-X table if IOMMU_CAP_INTR_REMAP was set
> >   powerpc/powernv/pci-ioda: Add IOMMU_CAP_INTR_REMAP for IODA host bridge
> >
> >  Documentation/kernel-parameters.txt   |9 ++-
> >  arch/powerpc/platforms/powernv/pci-ioda.c |   17 
> >  drivers/pci/pci.c |  126 
> > -
> >  drivers/pci/probe.c   |3 

Re: [RFC PATCH] arm: kernel: pci: remove pci=firmware command line parameter handling

[Bjorn Helgaas]

On Mon, Feb 29, 2016 at 05:43:20PM +, Lorenzo Pieralisi wrote:
> According to kernel documentation, the pci=firmware command line
> parameter is only meant to be used on IXP2000 ARM platforms to prevent
> the kernel from assigning PCI resources configured by the bootloader.
> 
> Since the IXP2000 ARM platforms support has been removed from the
> kernel in commit:
> 
> commit c65f2abf54a6 ("ARM: remove ixp23xx and ixp2000 platforms")
> 
> its platforms specific kernel parameters should be removed
> too from the kernel documentation along with the kernel code
> currently handling them in that they have just become obsolete.
> 
> This patch removes the pci=firmware command line parameter handling
> from ARM code and the related kernel parameters documentation
> section.
> 
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieral...@arm.com>

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

It looks like Rob merged c65f2abf54a6, so maybe he will merge this as
well?  Otherwise I'd be happy to merge it; just let me know if you
want me to.

> Cc: Arnd Bergmann <a...@arndb.de>
> Cc: Lennert Buytenhek <ker...@wantstofly.org>
> Cc: Jonathan Corbet <cor...@lwn.net>
> Cc: Bjorn Helgaas <bhelg...@google.com>
> Cc: Rob Herring <r...@kernel.org>
> Cc: Russell King <li...@arm.linux.org.uk>
> ---
>  Documentation/kernel-parameters.txt | 5 -
>  arch/arm/kernel/bios32.c| 3 ---
>  2 files changed, 8 deletions(-)
> 
> diff --git a/Documentation/kernel-parameters.txt 
> b/Documentation/kernel-parameters.txt
> index 9a53c92..e213acc 100644
> --- a/Documentation/kernel-parameters.txt
> +++ b/Documentation/kernel-parameters.txt
> @@ -2875,11 +2875,6 @@ bytes respectively. Such letter suffixes can also be 
> entirely omitted.
>   for broken drivers that don't call it.
>   skip_isa_align  [X86] do not align io start addr, so can
>   handle more pci cards
> - firmware[ARM] Do not re-enumerate the bus but instead
> - just use the configuration from the
> - bootloader. This is currently used on
> - IXP2000 systems where the bus has to be
> - configured a certain way for adjunct CPUs.
>   noearly [X86] Don't do any early type 1 scanning.
>   This might help on some broken boards which
>   machine check when some devices' config space
> diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
> index 066f7f9..05e61a2 100644
> --- a/arch/arm/kernel/bios32.c
> +++ b/arch/arm/kernel/bios32.c
> @@ -550,9 +550,6 @@ char * __init pcibios_setup(char *str)
>   if (!strcmp(str, "debug")) {
>   debug_pci = 1;
>   return NULL;
> - } else if (!strcmp(str, "firmware")) {
> - pci_add_flags(PCI_PROBE_ONLY);
> - return NULL;
>   }
>   return str;
>  }
> -- 
> 2.5.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
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html