diff mbox

[4/6] e1000e: drop unnecessary funtions

Message ID 1471444747-6277-5-git-send-email-caoj.fnst@cn.fujitsu.com
State New
Headers show

Commit Message

Cao jin Aug. 17, 2016, 2:39 p.m. UTC
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(-)

Comments

Dmitry Fleytman Aug. 18, 2016, 5:23 a.m. UTC | #1
> 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
> 
> 
>
Cao jin Aug. 18, 2016, 7:38 a.m. UTC | #2
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 mbox

Patch

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);
 }