From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=46169 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1P0BAO-0001gh-GJ for qemu-devel@nongnu.org; Mon, 27 Sep 2010 06:42:41 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1P0BAN-0000LR-7r for qemu-devel@nongnu.org; Mon, 27 Sep 2010 06:42:40 -0400 Received: from mx1.redhat.com ([209.132.183.28]:51012) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1P0BAM-0000LL-SU for qemu-devel@nongnu.org; Mon, 27 Sep 2010 06:42:39 -0400 Date: Mon, 27 Sep 2010 12:36:37 +0200 From: "Michael S. Tsirkin" Message-ID: <20100927103636.GA6374@redhat.com> References: <8d5f54a16ee43afe3280dd715659cd34fb1a7d44.1284528424.git.yamahata@valinux.co.jp> <20100922115016.GE16423@redhat.com> <20100924025021.GB4701@valinux.co.jp> <20100926124651.GF19655@redhat.com> <20100927060324.GA28732@valinux.co.jp> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20100927060324.GA28732@valinux.co.jp> Subject: [Qemu-devel] Re: [PATCH v3 06/13] pcie/aer: helper functions for pcie aer capability. List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Isaku Yamahata Cc: skandasa@cisco.com, etmartin@cisco.com, qemu-devel@nongnu.org, wexu2@cisco.com On Mon, Sep 27, 2010 at 03:03:24PM +0900, Isaku Yamahata wrote: > On Sun, Sep 26, 2010 at 02:46:51PM +0200, Michael S. Tsirkin wrote: > > > > > +static inline void pcie_aer_errmsg(PCIDevice *dev, const PCIE_AERErrMsg *msg) > > > > > +{ > > > > > + assert(pci_is_express(dev)); > > > > > + assert(dev->exp.aer_errmsg); > > > > > + dev->exp.aer_errmsg(dev, msg); > > > > > > > > Why do we want the indirection? Why not have users just call the function? > > > > > > To handle error signaling uniformly. > > > Please see > > > 6.2.5. Sequence of Device Error Signaling and Logging Operations > > > and figure 6-2 and 6-3. > > > > My question was: the only difference appears to be between bridge and > > non-bridge devices: bridge has to do more stuff, but most code is > > common. So this seems to be a very roundabout way to do this. > > Can't we just have a common function with an if (bridge) statement up front? > > If we ever only expect 2 implementations, I think a function pointer > > is overkill. > > Not 2, but 3. root port, upstream or downstream and normal device. > So you want something like the following? > More than 2 is a good reason for me, I prefer function pointer. Heh, shall we change all switch statements to pointers then? That's not a good idea IMO, automated tools like ctags become useless for navigation, you have to look up where it's actually set. We do have these things in places where it brings benefit, which is typically where it can take different values at runtime. > switch (pcie_cap_get_type(dev)) > case PCI_EXP_TYPE_ROOT_PORT: > pcie_aer_errmsg_root_port(); > break; > case PCI_EXP_TYPE_DOWNSTREAM: > case PCI_EXP_TYPE_UPSTREAM: > pcie_aer_errmsg_vbridge(); > break; > default: > pcie_aer_errmsg_alldev(); > break; Yes, and note that in fact it's not even that: all of root and ports are also devices, so we end up with: if (pcie_is_root_port(dev)) { pcie_aer_errmsg_root_port(); } else if (pcie_is_bridge_port(dev)) { pcie_aer_errmsg_vbridge(); } pcie_aer_errmsg_alldev(); which is in fact cleaner than calling pcie_aer_errmsg_alldev from pcie_aer_errmsg_vbridge, I think. > > -- > yamahata