Date:   Mon, 30 Mar 2020 17:43:33 +0000
From:   "Derrick, Jonathan" <jonathan.derrick@...el.com>
To:     "helgaas@...nel.org" <helgaas@...nel.org>
CC:     "mr.nuke.me@...il.com" <mr.nuke.me@...il.com>,
        "hch@....de" <hch@....de>,
        "Shevchenko, Andriy" <andriy.shevchenko@...el.com>,
        "lorenzo.pieralisi@....com" <lorenzo.pieralisi@....com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "mika.westerberg@...ux.intel.com" <mika.westerberg@...ux.intel.com>,
        "Baldysiak, Pawel" <pawel.baldysiak@...el.com>,
        "linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
        "lukas@...ner.de" <lukas@...ner.de>,
        "okaya@...nel.org" <okaya@...nel.org>,
        "kbusch@...nel.org" <kbusch@...nel.org>,
        "stuart.w.hayes@...il.com" <stuart.w.hayes@...il.com>
Subject: Re: [RFC 0/9] PCIe Hotplug Slot Emulation driver

Hi Bjorn,

On Sat, 2020-03-28 at 16:51 -0500, Bjorn Helgaas wrote:
> [+cc Stuart, Lukas]
> 
> On Fri, Feb 07, 2020 at 04:59:58PM -0700, Jon Derrick wrote:
> > This set adds an emulation driver for PCIe Hotplug. There may be platforms with
> > specific configurations that can support hotplug but don't provide the logical
> > slot hotplug hardware. For instance, the platform may use an
> > electrically-tolerant interposer between the slot and the device.
> > 
> > This driver utilizes the pci-bridge-emul architecture to manage register reads
> > and writes. The underlying functionality of the hotplug emulation driver uses
> > the Data Link Layer Link Active Reporting mechanism in a polling loop, but can
> > tolerate other event sources such as AER or DPC.
> > 
> > When enabled and a slot is managed by the driver, all port services are managed
> > by the kernel. This is done to ensure that firmware hotplug and error
> > architecture does not (correctly) halt/machine check the system when hotplug is
> > performed on a non-hotplug slot.
> > 
> > The driver offers two active mode: Auto and Force.
> > auto: The driver will bind to non-hotplug slots
> > force: The driver will bind to all slots and overrides the slot's services
> > 
> > There are three kernel params:
> > pciehp.pciehp_emul_mode={off, auto, force}
> > pciehp.pciehp_emul_time=<msecs polling time> (def 1000, min 100, max 60000)
> > pciehp.pciehp_emul_ports=<PCI [S]BDF/ID format string>
> > 
> > The pciehp_emul_ports kernel parameter takes a semi-colon tokenized string
> > representing PCI [S]BDFs and IDs. The pciehp_emul_mode will then be applied to
> > only those slots, leaving other slots unmanaged by pciehp_emul.
> > 
> > The string follows the pci_dev_str_match() format:
> > 
> >   [<domain>:]<bus>:<device>.<func>[/<device>.<func>]*
> >   pci:<vendor>:<device>[:<subvendor>:<subdevice>]
> > 
> > When using the path format, the path for the device can be obtained
> > using 'lspci -t' and further specified using the upstream bridge and the
> > downstream port's device-function to be more robust against bus
> > renumbering.
> > 
> > When using the vendor-device format, a value of '0' in any field acts as
> > a wildcard for that field, matching all values.
> > 
> > The driver is enabled with CONFIG_HOTPLUG_PCI_PCIE_EMUL=y.
> > 
> > The driver should be considered 'use at own risk' unless the platform/hardware
> > vendor recommends this mode.
> 
> There's a lot of good work in here, and I don't claim to understand
> the use case and all the benefits.
I've received more info that the customer use case is an AIC that
breaks out 1-4 M.2 cards which have been made hotplug tolerant.


> 
> But it seems like quite a lot of additional code and complexity in an
> area that's already pretty subtle, so I'm not yet convinced that it's
> all worthwhile.
> 
> It seems like this would rely on Data Link Layer Link Active
> Reporting.  Is that something we could add to pciehp as a generic
> feature without making a separate driver for it?  I haven't looked at
> this for a while, but I would assume that if we find out that a link
> went down, pciehp could/should be smart enough to notice that even if
> it didn't come via the usual pciehp Slot Status path.
I had a plan to do V2 by intercepting bus_ops rather than indirecting
slot_ops in pciehp. That should touch /a lot/ less code.

The problem I saw with adding DLLLA as a primary signal in pciehp is
that most of the pciehp boilerplate relies on valid Slot register
logic. I don't know how reliable pciehp will be if there's no backing
slot register logic, emulated or real. Consider how many slot
capability reads are in hpc.

I could add a non-slot flag check to each of those callers, but it
might be worse than the emulation alternative.

What do you think?

Thanks

> 
> > Jon Derrick (9):
> >   PCI: pci-bridge-emul: Update PCIe register behaviors
> >   PCI: pci-bridge-emul: Eliminate reserved member
> >   PCI: pci-bridge-emul: Provide a helper to set behavior
> >   PCI: pciehp: Indirect slot register operations
> >   PCI: Add pcie_port_slot_emulated stub
> >   PCI: pciehp: Expose the poll loop to other drivers
> >   PCI: Move pci_dev_str_match to search.c
> >   PCI: pciehp: Add hotplug slot emulation driver
> >   PCI: pciehp: Wire up pcie_port_emulate_slot and pciehp_emul
> > 
> >  drivers/pci/hotplug/Makefile      |   4 +
> >  drivers/pci/hotplug/pciehp.h      |  28 +++
> >  drivers/pci/hotplug/pciehp_emul.c | 378 ++++++++++++++++++++++++++++++++++++++
> >  drivers/pci/hotplug/pciehp_hpc.c  | 136 ++++++++++----
> >  drivers/pci/pci-acpi.c            |   3 +
> >  drivers/pci/pci-bridge-emul.c     |  95 +++++-----
> >  drivers/pci/pci-bridge-emul.h     |  10 +
> >  drivers/pci/pci.c                 | 163 ----------------
> >  drivers/pci/pcie/Kconfig          |  14 ++
> >  drivers/pci/pcie/portdrv_core.c   |  14 +-
> >  drivers/pci/probe.c               |   2 +-
> >  drivers/pci/search.c              | 162 ++++++++++++++++
> >  include/linux/pci.h               |   8 +
> >  13 files changed, 775 insertions(+), 242 deletions(-)
> >  create mode 100644 drivers/pci/hotplug/pciehp_emul.c
> > 
> > -- 
> > 1.8.3.1
> > 

Powered by blists - more mailing lists