[Qemu-devel] Re: [PATCH v3 06/13] pcie/aer: helper functions for pcie aer capability.

[Michael S. Tsirkin]

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