Re: [Qemu-devel] [PATCH v6 06/11] usb xhci: change msi/msix property type

[Markus Armbruster]

Cao jin <address@hidden> writes:

>>From bit to enum OnOffAuto
>
> cc: Gerd Hoffmann <address@hidden>
> cc: Michael S. Tsirkin <address@hidden>
> cc: Markus Armbruster <address@hidden>
> cc: Marcel Apfelbaum <address@hidden>
>
> Signed-off-by: Cao jin <address@hidden>
> ---
>  hw/usb/hcd-xhci.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
> index 43ba615..bbe5cca 100644
> --- a/hw/usb/hcd-xhci.c
> +++ b/hw/usb/hcd-xhci.c
> @@ -461,6 +461,8 @@ struct XHCIState {
>      uint32_t numslots;
>      uint32_t flags;
>      uint32_t max_pstreams_mask;
> +    OnOffAuto msi;
> +    OnOffAuto msix;
>  
>      /* Operational Registers */
>      uint32_t usbcmd;
> @@ -498,9 +500,7 @@ typedef struct XHCIEvRingSeg {
>  } XHCIEvRingSeg;
>  
>  enum xhci_flags {
> -    XHCI_FLAG_USE_MSI = 1,
> -    XHCI_FLAG_USE_MSI_X,
> -    XHCI_FLAG_SS_FIRST,
> +    XHCI_FLAG_SS_FIRST = 1,
>      XHCI_FLAG_FORCE_PCIE_ENDCAP,
>      XHCI_FLAG_ENABLE_STREAMS,
>  };
> @@ -3648,10 +3648,12 @@ static void usb_xhci_realize(struct PCIDevice *dev, 
> Error **errp)
>          assert(ret >= 0);
>      }
>  
> -    if (xhci_get_flag(xhci, XHCI_FLAG_USE_MSI)) {
> +    if (xhci->msi == ON_OFF_AUTO_ON ||
> +        xhci->msi == ON_OFF_AUTO_AUTO) {

Easier:
       if (xhci->msi != ON_OFF_AUTO_OFF) {

Hmm, you switch to this simpler conditional in PATCH 11, when you move
this code.  I'd use the simpler conditional from the start.  Since it
doesn't affect the final state, this is a suggestion, not a demand.

>          msi_init(dev, 0x70, xhci->numintrs, true, false);

Shouldn't we check for errors here?  Hmm, you do it in PATCH 11.  Okay,
but I'd add a /* TODO check for errors */ comment here.

>      }
> -    if (xhci_get_flag(xhci, XHCI_FLAG_USE_MSI_X)) {
> +    if (xhci->msix == ON_OFF_AUTO_ON ||
> +        xhci->msix == ON_OFF_AUTO_AUTO) {

Likewise.

>          msix_init(dev, xhci->numintrs,
>                    &xhci->mem, 0, OFF_MSIX_TABLE,
>                    &xhci->mem, 0, OFF_MSIX_PBA,

Likewise.

> @@ -3872,8 +3874,8 @@ static const VMStateDescription vmstate_xhci = {
>  };
>  
>  static Property xhci_properties[] = {
> -    DEFINE_PROP_BIT("msi",      XHCIState, flags, XHCI_FLAG_USE_MSI, true),
> -    DEFINE_PROP_BIT("msix",     XHCIState, flags, XHCI_FLAG_USE_MSI_X, true),
> +    DEFINE_PROP_ON_OFF_AUTO("msi", XHCIState, msi, ON_OFF_AUTO_AUTO),
> +    DEFINE_PROP_ON_OFF_AUTO("msix", XHCIState, msix, ON_OFF_AUTO_AUTO),
>      DEFINE_PROP_BIT("superspeed-ports-first",
>                      XHCIState, flags, XHCI_FLAG_SS_FIRST, true),
>      DEFINE_PROP_BIT("force-pcie-endcap", XHCIState, flags,