Re: [Qemu-devel] [PATCH v5 11/11] pci: Convert msi_init() to Error and fix callers to check it

[Cao jin]

On 05/15/2016 09:41 PM, Marcel Apfelbaum wrote:

> > diff --git a/hw/pci-bridge/pci_bridge_dev.c
> > b/hw/pci-bridge/pci_bridge_dev.c
> > index 9e31f0e..af71c98 100644
> > --- a/hw/pci-bridge/pci_bridge_dev.c
> > +++ b/hw/pci-bridge/pci_bridge_dev.c
> > @@ -53,6 +53,7 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
> >       PCIBridge *br = PCI_BRIDGE(dev);
> >       PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(dev);
> >       int err;
> > +    Error *local_err = NULL;
> >
> >       pci_bridge_initfn(dev, TYPE_PCI_BUS);
> >
> > @@ -74,11 +75,11 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
> >           goto slotid_error;
> >       }
> >
> > -    if ((bridge_dev->msi == ON_OFF_AUTO_AUTO ||
> > -                bridge_dev->msi == ON_OFF_AUTO_ON) &&
> > +    if (bridge_dev->msi != ON_OFF_AUTO_OFF &&
>
> So we made the msi property OnOffAuto, but we don't make a difference
> between ON and Auto?

That is why I am not quite sure about this device, msi has a relation 
with shpc.

From its previous behaviour, it can be seen that it don`t treat 'on' as 
'auto'.(I am not sure why it is different with others)

Its previous behaviour treat msi_init`s failure as fatal, and lead to 
device creation fail. According to Markus`s comments(if I understand him 
right), this device has no non-msi variants. I think my patch follows 
the previous behaviour.

And also according to function comments:
/* MSI is not applicable without SHPC */
which means device either has both, or neither(if I understand it 
right), so that is why I don`t make a difference

> >           msi_nonbroken) {
> > -        err = msi_init(dev, 0, 1, true, true);
> > +        err = msi_init(dev, 0, 1, true, true, &local_err);
> >           if (err < 0) {
> > +            error_report_err(local_err);
> >               goto msi_error;
> >           }
> >       }


--
Yours Sincerely,

Cao jin