Re: [Qemu-devel] [PATCH v2 3/5] pci: Convert msix_init() to Error and fix callers to check it

[Markus Armbruster]

Cao jin <address@hidden> writes:

> On 09/12/2016 09:47 PM, Markus Armbruster wrote:
>> Cao jin <address@hidden> writes:
[...]
>>> diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
>>> index 188f954..4280c5d 100644
>>> --- a/hw/usb/hcd-xhci.c
>>> +++ b/hw/usb/hcd-xhci.c
>>> @@ -3594,25 +3594,6 @@ static void usb_xhci_realize(struct PCIDevice *dev, 
>>> Error **errp)
>>>       dev->config[PCI_CACHE_LINE_SIZE] = 0x10;
>>>       dev->config[0x60] = 0x30; /* release number */
>>>
>>> -    usb_xhci_init(xhci);
>>> -
>>> -    if (xhci->msi != ON_OFF_AUTO_OFF) {
>>> -        ret = msi_init(dev, 0x70, xhci->numintrs, true, false, &err);
>>> -        /* Any error other than -ENOTSUP(board's MSI support is broken)
>>> -         * is a programming error */
>>> -        assert(!ret || ret == -ENOTSUP);
>>> -        if (ret && xhci->msi == ON_OFF_AUTO_ON) {
>>> -            /* Can't satisfy user's explicit msi=on request, fail */
>>> -            error_append_hint(&err, "You have to use msi=auto (default) or 
>>> "
>>> -                    "msi=off with this machine type.\n");
>>> -            error_propagate(errp, err);
>>> -            return;
>>> -        }
>>> -        assert(!err || xhci->msi == ON_OFF_AUTO_AUTO);
>>> -        /* With msi=auto, we fall back to MSI off silently */
>>> -        error_free(err);
>>> -    }
>>> -
>>>       if (xhci->numintrs > MAXINTRS) {
>>>           xhci->numintrs = MAXINTRS;
>>>       }
>>> @@ -3622,21 +3603,60 @@ static void usb_xhci_realize(struct PCIDevice *dev, 
>>> Error **errp)
>>>       if (xhci->numintrs < 1) {
>>>           xhci->numintrs = 1;
>>>       }
>>> +
>>>       if (xhci->numslots > MAXSLOTS) {
>>>           xhci->numslots = MAXSLOTS;
>>>       }
>>>       if (xhci->numslots < 1) {
>>>           xhci->numslots = 1;
>>>       }
>>> +
>>>       if (xhci_get_flag(xhci, XHCI_FLAG_ENABLE_STREAMS)) {
>>>           xhci->max_pstreams_mask = 7; /* == 256 primary streams */
>>>       } else {
>>>           xhci->max_pstreams_mask = 0;
>>>       }
>>>
>>> -    xhci->mfwrap_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, 
>>> xhci_mfwrap_timer, xhci);
>>> +    if (xhci->msi != ON_OFF_AUTO_OFF) {
>>> +        ret = msi_init(dev, 0x70, xhci->numintrs, true, false, &err);
>>> +        /* Any error other than -ENOTSUP(board's MSI support is broken)
>>> +         * is a programming error */
>>> +        assert(!ret || ret == -ENOTSUP);
>>> +        if (ret && xhci->msi == ON_OFF_AUTO_ON) {
>>> +            /* Can't satisfy user's explicit msi=on request, fail */
>>> +            error_append_hint(&err, "You have to use msi=auto (default) or 
>>> "
>>> +                    "msi=off with this machine type.\n");
>>> +            error_propagate(errp, err);
>>> +            return;
>>> +        }
>>> +        assert(!err || xhci->msi == ON_OFF_AUTO_AUTO);
>>> +        /* With msi=auto, we fall back to MSI off silently */
>>> +        error_free(err);
>>> +    }
>>
>> Can you explain why you're moving this code?
>>
>
> Sorry I forget to mention this: msi_init() uses xhci->numintrs, but
> there is value checking/correcting on xhci->numintrs, it should be
> done before using.

If you do the move in a separate patch before this one, you can explain
it in its commit message.  Easier to review that way.