| Message ID | 1471444747-6277-5-git-send-email-caoj.fnst@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> On 17 Aug 2016, at 17:39, Cao jin <caoj.fnst@cn.fujitsu.com> wrote: > > Internal helper function: e1000e_init_msix(), e1000e_cleanup_msix() > is unnecessary, remove them all. Is there any reason to drop these functions? They exist to improve code readability and modularisation. > MSI-X state flag is used by intr_state > which exists in vmstate, keep it for migration compatibility. > > CC: Dmitry Fleytman <dmitry@daynix.com> > CC: Jason Wang <jasowang@redhat.com> > CC: Markus Armbruster <armbru@redhat.com> > CC: Marcel Apfelbaum <marcel@redhat.com> > CC: Michael S. Tsirkin <mst@redhat.com> > Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com> > --- > hw/net/e1000e.c | 49 ++++++++++++++++--------------------------------- > 1 file changed, 16 insertions(+), 33 deletions(-) > > diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c > index ba37fe9..4b4eb46 100644 > --- a/hw/net/e1000e.c > +++ b/hw/net/e1000e.c > @@ -288,37 +288,6 @@ e1000e_use_msix_vectors(E1000EState *s, int num_vectors) > } > > static void > -e1000e_init_msix(E1000EState *s) > -{ > - PCIDevice *d = PCI_DEVICE(s); > - int res = msix_init(PCI_DEVICE(s), E1000E_MSIX_VEC_NUM, > - &s->msix, > - E1000E_MSIX_IDX, E1000E_MSIX_TABLE, > - &s->msix, > - E1000E_MSIX_IDX, E1000E_MSIX_PBA, > - 0xA0, NULL); > - > - if (res < 0) { > - trace_e1000e_msix_init_fail(res); > - } else { > - if (!e1000e_use_msix_vectors(s, E1000E_MSIX_VEC_NUM)) { > - msix_uninit(d, &s->msix, &s->msix); > - } else { > - s->intr_state |= E1000E_USE_MSIX; > - } > - } > -} > - > -static void > -e1000e_cleanup_msix(E1000EState *s) > -{ > - if (s->intr_state & E1000E_USE_MSIX) { > - e1000e_unuse_msix_vectors(s, E1000E_MSIX_VEC_NUM); > - msix_uninit(PCI_DEVICE(s), &s->msix, &s->msix); > - } > -} > - > -static void > e1000e_init_net_peer(E1000EState *s, PCIDevice *pci_dev, uint8_t *macaddr) > { > DeviceState *dev = DEVICE(pci_dev); > @@ -462,7 +431,20 @@ static void e1000e_pci_realize(PCIDevice *pci_dev, Error **errp) > qemu_macaddr_default_if_unset(&s->conf.macaddr); > macaddr = s->conf.macaddr.a; > > - e1000e_init_msix(s); > + ret = msix_init(pci_dev, E1000E_MSIX_VEC_NUM, > + &s->msix, > + E1000E_MSIX_IDX, E1000E_MSIX_TABLE, > + &s->msix, > + E1000E_MSIX_IDX, E1000E_MSIX_PBA, > + 0xA0, NULL); > + > + if (ret) { > + trace_e1000e_msix_init_fail(ret); > + } else { > + /* Won't fail, for simplicity, no need to check return value */ > + e1000e_use_msix_vectors(s, E1000E_MSIX_VEC_NUM); > + s->intr_state |= E1000E_USE_MSIX; > + } > > if (pcie_endpoint_cap_v1_init(pci_dev, e1000e_pcie_offset) < 0) { > hw_error("Failed to initialize PCIe capability"); > @@ -511,7 +493,8 @@ static void e1000e_pci_uninit(PCIDevice *pci_dev) > > qemu_del_nic(s->nic); > > - e1000e_cleanup_msix(s); > + e1000e_unuse_msix_vectors(s, E1000E_MSIX_VEC_NUM); > + msix_uninit(pci_dev, &s->msix, &s->msix); > msi_uninit(pci_dev); > } > > -- > 2.1.0 > > >
On 08/18/2016 01:23 PM, Dmitry Fleytman wrote: > > >> On 17 Aug 2016, at 17:39, Cao jin <caoj.fnst@cn.fujitsu.com> wrote: >> >> Internal helper function: e1000e_init_msix(), e1000e_cleanup_msix() >> is unnecessary, remove them all. > > Is there any reason to drop these functions? > They exist to improve code readability and modularisation. > previous commit 66bf7d58d removed the corresponding function of msi, so, keep the consistency is the reason
diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c index ba37fe9..4b4eb46 100644 --- a/hw/net/e1000e.c +++ b/hw/net/e1000e.c @@ -288,37 +288,6 @@ e1000e_use_msix_vectors(E1000EState *s, int num_vectors) } static void -e1000e_init_msix(E1000EState *s) -{ - PCIDevice *d = PCI_DEVICE(s); - int res = msix_init(PCI_DEVICE(s), E1000E_MSIX_VEC_NUM, - &s->msix, - E1000E_MSIX_IDX, E1000E_MSIX_TABLE, - &s->msix, - E1000E_MSIX_IDX, E1000E_MSIX_PBA, - 0xA0, NULL); - - if (res < 0) { - trace_e1000e_msix_init_fail(res); - } else { - if (!e1000e_use_msix_vectors(s, E1000E_MSIX_VEC_NUM)) { - msix_uninit(d, &s->msix, &s->msix); - } else { - s->intr_state |= E1000E_USE_MSIX; - } - } -} - -static void -e1000e_cleanup_msix(E1000EState *s) -{ - if (s->intr_state & E1000E_USE_MSIX) { - e1000e_unuse_msix_vectors(s, E1000E_MSIX_VEC_NUM); - msix_uninit(PCI_DEVICE(s), &s->msix, &s->msix); - } -} - -static void e1000e_init_net_peer(E1000EState *s, PCIDevice *pci_dev, uint8_t *macaddr) { DeviceState *dev = DEVICE(pci_dev); @@ -462,7 +431,20 @@ static void e1000e_pci_realize(PCIDevice *pci_dev, Error **errp) qemu_macaddr_default_if_unset(&s->conf.macaddr); macaddr = s->conf.macaddr.a; - e1000e_init_msix(s); + ret = msix_init(pci_dev, E1000E_MSIX_VEC_NUM, + &s->msix, + E1000E_MSIX_IDX, E1000E_MSIX_TABLE, + &s->msix, + E1000E_MSIX_IDX, E1000E_MSIX_PBA, + 0xA0, NULL); + + if (ret) { + trace_e1000e_msix_init_fail(ret); + } else { + /* Won't fail, for simplicity, no need to check return value */ + e1000e_use_msix_vectors(s, E1000E_MSIX_VEC_NUM); + s->intr_state |= E1000E_USE_MSIX; + } if (pcie_endpoint_cap_v1_init(pci_dev, e1000e_pcie_offset) < 0) { hw_error("Failed to initialize PCIe capability"); @@ -511,7 +493,8 @@ static void e1000e_pci_uninit(PCIDevice *pci_dev) qemu_del_nic(s->nic); - e1000e_cleanup_msix(s); + e1000e_unuse_msix_vectors(s, E1000E_MSIX_VEC_NUM); + msix_uninit(pci_dev, &s->msix, &s->msix); msi_uninit(pci_dev); }
Internal helper function: e1000e_init_msix(), e1000e_cleanup_msix() is unnecessary, remove them all. MSI-X state flag is used by intr_state which exists in vmstate, keep it for migration compatibility. CC: Dmitry Fleytman <dmitry@daynix.com> CC: Jason Wang <jasowang@redhat.com> CC: Markus Armbruster <armbru@redhat.com> CC: Marcel Apfelbaum <marcel@redhat.com> CC: Michael S. Tsirkin <mst@redhat.com> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com> --- hw/net/e1000e.c | 49 ++++++++++++++++--------------------------------- 1 file changed, 16 insertions(+), 33 deletions(-)