[pcie] Change how ref-counting works.

Eliminate the confusing ref-count redirects introduced to deal with
multiple inheritance issues.  Instead, leverage vtables and some
macros to implement something which behaves much like virtual
inheritance, but without actually using virtual inheritance.  See the
extensive document in pcie_ref_counted.h for details.

Change-Id: I8dd7e400f2a25ca2296d8f50c580f76ab3c3cf39

kernel/dev/pcie/include/dev/pcie_bridge.h

@@ -11,6 +11,7 @@
 #include <magenta/errors.h>
 #include <dev/pcie_bus_driver.h>
 #include <dev/pcie_device.h>
+#include <dev/pcie_ref_counted.h>
 #include <dev/pcie_upstream_node.h>
 
 #include <region-alloc/region-alloc.h>
@@ -29,6 +30,9 @@ class PcieBridge : public PcieDevice,
     // Disallow copying, assigning and moving.
     DISALLOW_COPY_ASSIGN_AND_MOVE(PcieBridge);
 
+    // Implement ref counting, do not let derived classes override.
+    PCIE_IMPLEMENT_REFCOUNTED;
+
     // Device overrides
     void Unplug() override;
 
@@ -40,10 +44,6 @@ class PcieBridge : public PcieDevice,
     // Properties
     PcieBusDriver& driver() { return PcieDevice::driver(); }
 
-    // RefCounting dismbiguation... see comment in PcieUpstreamNode
-    void AddRef() { PcieDevice::AddRef(); }
-    bool Release() __WARN_UNUSED_RESULT { return PcieDevice::Release(); }
-
     uint64_t pf_mem_base()        const { return pf_mem_base_; }
     uint64_t pf_mem_limit()       const { return pf_mem_limit_; }
     uint32_t mem_base()           const { return mem_base_; }

kernel/dev/pcie/include/dev/pcie_device.h

@@ -15,11 +15,11 @@
 #include <dev/pcie_caps.h>
 #include <dev/pcie_constants.h>
 #include <dev/pcie_irqs.h>
+#include <dev/pcie_ref_counted.h>
 #include <dev/pcie_regs.h>
 #include <kernel/mutex.h>
 #include <kernel/spinlock.h>
 #include <mxtl/macros.h>
-#include <mxtl/ref_counted.h>
 #include <mxtl/ref_ptr.h>
 #include <sys/types.h>
 
@@ -42,20 +42,24 @@ struct pcie_bar_info_t {
 };
 
 /*
- * Struct used to manage the relationship between a PCIe device/function and its
+ * Base used to manage the relationship between a PCIe device/function and its
  * associated driver.  During a bus scan/probe operation, all drivers will have
  * their registered probe methods called until a driver claims a device.  A
  * driver may claim a device by returning a pointer to a driver-managed
  * pcie_device_state struct, with the driver owned fields filled out.
  */
-class PcieDevice : public mxtl::RefCounted<PcieDevice> {
+class PcieDevice {
 public:
     static mxtl::RefPtr<PcieDevice> Create(PcieUpstreamNode& upstream, uint dev_id, uint func_id);
+
     virtual ~PcieDevice();
 
     // Disallow copying, assigning and moving.
     DISALLOW_COPY_ASSIGN_AND_MOVE(PcieDevice);
 
+    // Require that derived classes implement ref counting.
+    PCIE_REQUIRE_REFCOUNTED;
+
     mxtl::RefPtr<PcieUpstreamNode> GetUpstream();
 
     status_t     Claim();
@@ -312,8 +316,7 @@ class PcieDevice : public mxtl::RefCounted<PcieDevice> {
 protected:
     friend class PcieUpstreamNode;
     friend class PcieBusDriver;  // TODO(johngro): remove this.  Currently used for IRQ swizzle.
-    PcieDevice(PcieBusDriver& bus_drv,
-               uint bus_id, uint dev_id, uint func_id, bool is_bridge = false);
+    PcieDevice(PcieBusDriver& bus_drv, uint bus_id, uint dev_id, uint func_id, bool is_bridge);
 
     void ModifyCmdLocked(uint16_t clr_bits, uint16_t set_bits);
     void AssignCmdLocked(uint16_t value) { ModifyCmdLocked(0xFFFF, value); }

kernel/dev/pcie/include/dev/pcie_ref_counted.h

@@ -0,0 +1,146 @@
+// Copyright 2016 The Fuchsia Authors
+// Copyright (c) 2016, Google, Inc. All rights reserved
+//
+// Use of this source code is governed by a MIT-style
+// license that can be found in the LICENSE file or at
+// https://opensource.org/licenses/MIT
+
+#pragma once
+
+#include <assert.h>
+#include <magenta/atomic.h>
+#include <magenta/compiler.h>
+#include <mxtl/ref_counted.h>
+
+/**
+ * Notes on class hierarchy and RefCounting
+ *
+ * The PCI/PCIe device class hierarchy consists of 3 main types of object.
+ *
+ * ++ PcieRoot
+ * A root of a PCI/PCIe device tree.  Roots do not have standard config
+ * registers, but do have a collection of downstream PcieDevice children.  In
+ * addition, PCIe roots (as opposed to plain PCI roots) have a special set of
+ * registers called the "root complex control block".  The PCIe bus driver
+ * supports systems which have multiple roots and maintains a collection of
+ * roots which were registered by the system.
+ *
+ * ++ PcieDevice
+ * The actual devices in the PCIe hierarchy.  Devices have a set of PCI/PCIe
+ * config registers, can allocate apertures in Memory and I/O space, can map
+ * interrupts, and can have drivers attached to them.  All devices are the child
+ * of either a PcieRoot or a PcieBridge, but have no children themselves.
+ *
+ * ++ PcieBridge
+ * PcieBridges are devices with children.  Because they are devices, bridges
+ * have config, can map registers, deliver interrupts, have drivers bound to
+ * them, and are always the child of either a PcieRoot or another PcieBridge.
+ * In addition (unlike PcieDevices), Bridges have roots.
+ *
+ * In order to avoid code duplication, two classes have been introduced and
+ * PcieBridge makes limited use of multiple inheritance in order to be a device
+ * with children, while not being a root.  The classes introduced are...
+ *
+ * ++ PcieUpstreamNode
+ * A PcieUpstreamNode is an object which can have PcieDevice children.  Roots
+ * and bridges are both are upstream nodes.  Devices hold a reference to their
+ * upstream node, without needing to understand whether they are downstream of a
+ * root or a bridge.
+ *
+ * ++ PcieDeviceImpl
+ * A small class used to deal with some of the ref counting issues which arise
+ * from this arrangement.  More on this later.
+ *
+ * A simple diagram of the class hierarchy looks like this.
+ *
+ *            +---------------+       +--------+
+ *            | Upstream Node |       | Device |
+ *            +---------------+       +--------+
+ *              |    |                  |   |
+ * +------+     |    |    +--------+    |   |
+ * | Root | <---/    \--->| Bridge |<---/   |
+ * +------+               +--------+        |
+ *                                          |
+ *                    +------------+        |
+ *                    | DeviceImpl |<-------/
+ *                    +------------+
+ *
+ * ==== RefCounting ====
+ *
+ * Object lifetimes are managed using mxtl::RefPtr<>.  Because of this, all
+ * objects must provide an implementation of AddRef/Release/Adopt which is
+ * compatible with mxtl::RefPtr<>.  The bus driver holds RefPtr<Root>s,
+ * UpstreamNodes hold RefPtr<Device>s and Devices hold RefPtr<UpstreamNode>s
+ * back to their owners.
+ *
+ * RefPtr to both Devices and UpstreamNodes exist in the system, so both object
+ * must expose an interface to reference counting which is compatible with
+ * mxtl::RefPtr<>.  Because a Bridge is both an UpstreamNode and a Device,
+ * simply having Device and UpstreamNode derive from mxtl::RefCounted<> (which
+ * would be standard practice) will not work.  The Bridge object which results
+ * from this would have two different ref counts which would end up being
+ * manipulated independently.
+ *
+ * A simple solution to this would be to have all of the objects in the system
+ * inherit virtually from an implementation of mxtl::RefCounted<>.
+ * Unfortunately, the power that be strictly prohibit the use of virtual
+ * inheritance in this codebase.  Because of this, a different solution needs to
+ * be provided.  Here is how this system works.
+ *
+ * Two macros have been defined (below).  One or the other of them *must* be
+ * included in the public section of every class involved in this hierarchy.
+ *
+ * ++ PCIE_REQUIRE_REFCOUNTED
+ * Any class which is a base class of any other class in this hierarchy *must*
+ * include this macro.  It defines pure virtual AddRef/Release/Adopt members
+ * whose signatures are compatible with mxtl::RefPtr.  This requires an
+ * implementation to exist in derived classes, redirects ref counting behavior
+ * to this implementation, and prevents accidental instantiation of the base
+ * class.  UpstreamNode and Device REQUIRE_REFCOUNTED.
+ *
+ * ++ PCIE_IMPLEMENT_REFCOUNTED
+ * Any class which is a child of one or more of the base classes *must* include
+ * this macro.  This macro wraps an implementation of mxtl::RefCounted<> (so
+ * that code duplication is minimized, and atomic ref count access is consistent
+ * throughout the system), and marks the virtual AddRef/Release/Adopt methods as
+ * being final, which helps to prevent a different implementation accidentally
+ * being added to the class hierarchy.  Root, Bridge and DeviceImpl
+ * IMPLEMENT_REFCOUNTED.
+ *
+ * Finally, coming back to the issue of DeviceImpl...
+ * Because Device is a base class for Bridge, it cannot IMPLEMENT_REFCOUNTED
+ * itself.  Instead, it must REQUIRE_REFCOUNTED (redirecting ref counting
+ * behavior to the Bridge implementation).  This means that Device can no longer
+ * be instantiated (because it is abstract).  DeviceImpl is a small class which
+ * does nothing but derive from Device and implement the ref counting.  Its
+ * implementation exists inside of an anonymous namespace inside
+ * pcie_device.cpp so none of the rest of the system ever sees it.
+ * PcieDevice::Create returns a mxtl::RefPtr<PcieDevice> which actually points
+ * to an instance of DeviceImpl created by the Create method.
+ */
+
+#if (LK_DEBUGLEVEL > 1)
+#define __PCIE_REQUIRE_ADOPT virtual void Adopt() = 0
+#else   // if (LK_DEBUGLEVEL > 1)
+#define __PCIE_REQUIRE_ADOPT
+#endif  // if (LK_DEBUGLEVEL > 1)
+
+#define PCIE_REQUIRE_REFCOUNTED \
+    __PCIE_REQUIRE_ADOPT; \
+    virtual void AddRef()  = 0; \
+    virtual bool Release() = 0 \
+
+#if (LK_DEBUGLEVEL > 1)
+#define __PCIE_IMPLEMENT_ADOPT void Adopt() final { ref_count_impl_.Adopt(); }
+#else   // if (LK_DEBUGLEVEL > 1)
+#define __PCIE_IMPLEMENT_ADOPT
+#endif  // if (LK_DEBUGLEVEL > 1)
+
+#define PCIE_IMPLEMENT_REFCOUNTED \
+private: \
+    mxtl::RefCounted<void> ref_count_impl_; \
+public: \
+    __PCIE_IMPLEMENT_ADOPT; \
+    void AddRef() final { ref_count_impl_.AddRef(); } \
+    bool Release() final __WARN_UNUSED_RESULT { return ref_count_impl_.Release(); } \
+    using __force_semicolon = int

kernel/dev/pcie/include/dev/pcie_root.h

@@ -8,28 +8,27 @@
 #pragma once
 
 #include <dev/pcie_bus_driver.h>
+#include <dev/pcie_ref_counted.h>
 #include <dev/pcie_upstream_node.h>
 #include <magenta/compiler.h>
 #include <mxtl/macros.h>
 #include <mxtl/ref_ptr.h>
-#include <mxtl/ref_counted.h>
 
-class PcieRoot : public mxtl::RefCounted<PcieRoot>,
-                 public PcieUpstreamNode {
+class PcieRoot : public PcieUpstreamNode {
 public:
     static mxtl::RefPtr<PcieRoot> Create(PcieBusDriver& bus_drv, uint managed_bus_id);
 
     // Disallow copying, assigning and moving.
     DISALLOW_COPY_ASSIGN_AND_MOVE(PcieRoot);
 
+    // Implement ref counting, do not let derived classes override.
+    PCIE_IMPLEMENT_REFCOUNTED;
+
     PcieBusDriver& driver() { return bus_drv_; }
     RegionAllocator& mmio_lo_regions() override { return bus_drv_.mmio_lo_regions(); }
     RegionAllocator& mmio_hi_regions() override { return bus_drv_.mmio_hi_regions(); }
     RegionAllocator& pio_regions()     override { return bus_drv_.pio_regions(); }
 
-    void AddRef() { RefCounted<PcieRoot>::AddRef(); }
-    bool Release() __WARN_UNUSED_RESULT { return RefCounted<PcieRoot>::Release(); }
-
     // TODO(johngro) : Move Legacy IRQ swizling support out of PciePlatform and into roots
     // TODO(johngro) : Add support for RCRB (root complex register block)  Consider splitting
     // PcieRoot into PciRoot and PcieRoot (since PciRoots don't have RCRBs)

kernel/dev/pcie/include/dev/pcie_upstream_node.h

@@ -9,6 +9,7 @@
 
 #include <dev/pcie_bus_driver.h>
 #include <dev/pcie_device.h>
+#include <dev/pcie_ref_counted.h>
 #include <mxtl/macros.h>
 #include <mxtl/ref_ptr.h>
 #include <sys/types.h>
@@ -30,6 +31,9 @@ class PcieUpstreamNode {
     // Disallow copying, assigning and moving.
     DISALLOW_COPY_ASSIGN_AND_MOVE(PcieUpstreamNode);
 
+    // Require that derived classes implement ref counting.
+    PCIE_REQUIRE_REFCOUNTED;
+
     mxtl::RefPtr<PcieDevice> GetDownstream(uint ndx) { return bus_drv_.GetDownstream(*this, ndx); }
     PcieBusDriver& driver() { return bus_drv_; }
 
@@ -40,26 +44,6 @@ class PcieUpstreamNode {
     virtual RegionAllocator& mmio_hi_regions() = 0;
     virtual RegionAllocator& pio_regions() = 0;
 
-    // Explicit implementation of AddRef and Release.
-    //
-    // We want to be able to hold RefPtrs to PcieUpstreamNodes, so normally we
-    // would just derive from mxtl::RefCounted<>.  Unfortunately, PcieBridges
-    // are both PcieUpstreamNodes as well as PcieDevices, and PcieDevices are
-    // already RefCounted.  One could solve this problem with virtual
-    // inheritance, but one runs the risk of being murdered by one's colleagues
-    // if one were to try such a thing.
-    //
-    // Instead of getting murdered, we ensure that all of the derived classes of
-    // UpstreamNode (Bridge/Device and Root) inherit from RefCounted, and then
-    // have explicit implementations of AddRef/Release which use type to perform
-    // a safe downcast to our derived class and then call their AddRef/Release
-    // implementation.  There is a minor performance penalty for this, but since
-    // external users deal almost exclusively with PcieDevices and nothing else,
-    // it is not really on the critical path.  Plus, it is much better than
-    // getting murdered.
-    void AddRef();
-    bool Release() __WARN_UNUSED_RESULT;
-
 protected:
     friend class PcieBusDriver;
     PcieUpstreamNode(PcieBusDriver& bus_drv, Type type, uint mbus_id)

kernel/dev/pcie/pcie_device.cpp

@@ -27,6 +27,47 @@
 
 #define LOCAL_TRACE 0
 
+namespace {  // anon namespace.  Externals do not need to know about PcieDeviceImpl
+class PcieDeviceImpl : public PcieDevice {
+public:
+    static mxtl::RefPtr<PcieDevice> Create(PcieUpstreamNode& upstream, uint dev_id, uint func_id);
+
+    // Disallow copying, assigning and moving.
+    DISALLOW_COPY_ASSIGN_AND_MOVE(PcieDeviceImpl);
+
+    // Implement ref counting, do not let derived classes override.
+    PCIE_IMPLEMENT_REFCOUNTED;
+
+protected:
+    PcieDeviceImpl(PcieBusDriver& bus_drv, uint bus_id, uint dev_id, uint func_id)
+        : PcieDevice(bus_drv, bus_id, dev_id, func_id, false) { }
+};
+
+mxtl::RefPtr<PcieDevice> PcieDeviceImpl::Create(PcieUpstreamNode& upstream,
+                                                uint dev_id, uint func_id) {
+    AllocChecker ac;
+    auto raw_dev = new (&ac) PcieDeviceImpl(upstream.driver(),
+                                            upstream.managed_bus_id(),
+                                            dev_id,
+                                            func_id);
+    if (!ac.check()) {
+        TRACEF("Out of memory attemping to create PCIe device %02x:%02x.%01x.\n",
+                upstream.managed_bus_id(), dev_id, func_id);
+        return nullptr;
+    }
+
+    auto dev = mxtl::AdoptRef(static_cast<PcieDevice*>(raw_dev));
+    status_t res = raw_dev->Init(upstream);
+    if (res != NO_ERROR) {
+        TRACEF("Failed to initialize PCIe device %02x:%02x.%01x. (res %d)\n",
+                upstream.managed_bus_id(), dev_id, func_id, res);
+        return nullptr;
+    }
+
+    return dev;
+}
+}  // namespace
+
 PcieDevice::PcieDevice(PcieBusDriver& bus_drv,
                        uint bus_id, uint dev_id, uint func_id, bool is_bridge)
     : bus_drv_(bus_drv),
@@ -57,25 +98,7 @@ PcieDevice::~PcieDevice() {
 }
 
 mxtl::RefPtr<PcieDevice> PcieDevice::Create(PcieUpstreamNode& upstream, uint dev_id, uint func_id) {
-    AllocChecker ac;
-    auto raw_dev = new (&ac) PcieDevice(upstream.driver(),
-                                        upstream.managed_bus_id(), dev_id, func_id);
-    if (!ac.check()) {
-        DEBUG_ASSERT(raw_dev == nullptr);
-        TRACEF("Out of memory attemping to create PCIe device %02x:%02x.%01x.\n",
-                upstream.managed_bus_id(), dev_id, func_id);
-        return nullptr;
-    }
-
-    auto dev = mxtl::AdoptRef(raw_dev);
-    status_t res = dev->Init(upstream);
-    if (res != NO_ERROR) {
-        TRACEF("Failed to initialize PCIe device %02x:%02x.%01x. (res %d)\n",
-                upstream.managed_bus_id(), dev_id, func_id, res);
-        return nullptr;
-    }
-
-    return dev;
+    return PcieDeviceImpl::Create(upstream, dev_id, func_id);
 }
 
 status_t PcieDevice::Init(PcieUpstreamNode& upstream) {