Re: [PATCH v3 1/2] igb: fix link state on resume

[Akihiko Odaki]

On 2024/03/08 17:56, Laurent Vivier wrote:
> On resume igb_vm_state_change() always calls igb_autoneg_resume()
> that sets link_down to false, and thus activates the link even
> if we have disabled it.
>
> The problem can be reproduced starting qemu in paused state (-S) and
> then set the link to down. When we resume the machine the link appears
> to be up.
>
> Reproducer:
>
>     # qemu-system-x86_64 ... -device igb,netdev=netdev0,id=net0 -S
>
>     {"execute": "qmp_capabilities" }
>     {"execute": "set_link", "arguments": {"name": "net0", "up": false}}
>     {"execute": "cont" }
>
> To fix the problem, merge the content of igb_vm_state_change()
> into igb_core_post_load() as e1000 does.
>
> Buglink: https://issues.redhat.com/browse/RHEL-21867
> Suggested-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
>   hw/net/igb_core.c | 47 +++--------------------------------------------
>   hw/net/igb_core.h |  2 --
>   2 files changed, 3 insertions(+), 46 deletions(-)
>
> diff --git a/hw/net/igb_core.c b/hw/net/igb_core.c
> index 2a7a11aa9ed5..31ab959ab8ff 100644
> --- a/hw/net/igb_core.c
> +++ b/hw/net/igb_core.c
> @@ -160,14 +160,6 @@ igb_intmgr_timer_resume(IGBIntrDelayTimer *timer)
>       }
>   }
>   
> -static void
> -igb_intmgr_timer_pause(IGBIntrDelayTimer *timer)
> -{
> -    if (timer->running) {
> -        timer_del(timer->timer);
> -    }
> -}
> -
>   static void
>   igb_intrmgr_on_msix_throttling_timer(void *opaque)
>   {
> @@ -212,16 +204,6 @@ igb_intrmgr_resume(IGBCore *core)
>       }
>   }
>   
> -static void
> -igb_intrmgr_pause(IGBCore *core)
> -{
> -    int i;
> -
> -    for (i = 0; i < IGB_INTR_NUM; i++) {
> -        igb_intmgr_timer_pause(&core->eitr[i]);
> -    }
> -}
> -
>   static void
>   igb_intrmgr_reset(IGBCore *core)
>   {
> @@ -4290,12 +4272,6 @@ igb_core_read(IGBCore *core, hwaddr addr, unsigned size)
>       return 0;
>   }
>   
> -static inline void
> -igb_autoneg_pause(IGBCore *core)
> -{
> -    timer_del(core->autoneg_timer);
> -}
> -
>   static void
>   igb_autoneg_resume(IGBCore *core)
>   {
> @@ -4307,22 +4283,6 @@ igb_autoneg_resume(IGBCore *core)
>       }
>   }
>   
> -static void
> -igb_vm_state_change(void *opaque, bool running, RunState state)
> -{
> -    IGBCore *core = opaque;
> -
> -    if (running) {
> -        trace_e1000e_vm_state_running();
> -        igb_intrmgr_resume(core);
> -        igb_autoneg_resume(core);
> -    } else {
> -        trace_e1000e_vm_state_stopped();
> -        igb_autoneg_pause(core);
> -        igb_intrmgr_pause(core);
> -    }
> -}
> -
>   void
>   igb_core_pci_realize(IGBCore        *core,
>                        const uint16_t *eeprom_templ,
> @@ -4335,8 +4295,6 @@ igb_core_pci_realize(IGBCore        *core,
>                                          igb_autoneg_timer, core);
>       igb_intrmgr_pci_realize(core);
>   
> -    core->vmstate = qemu_add_vm_change_state_handler(igb_vm_state_change, core);
> -
>       for (i = 0; i < IGB_NUM_QUEUES; i++) {
>           net_tx_pkt_init(&core->tx[i].tx_pkt, E1000E_MAX_TX_FRAGS);
>       }
> @@ -4360,8 +4318,6 @@ igb_core_pci_uninit(IGBCore *core)
>   
>       igb_intrmgr_pci_unint(core);
>   
> -    qemu_del_vm_change_state_handler(core->vmstate);
> -
>       for (i = 0; i < IGB_NUM_QUEUES; i++) {
>           net_tx_pkt_uninit(core->tx[i].tx_pkt);
>       }
> @@ -4586,5 +4542,8 @@ igb_core_post_load(IGBCore *core)
>        */
>       nc->link_down = (core->mac[STATUS] & E1000_STATUS_LU) == 0;
>   
> +    igb_intrmgr_resume(core);
> +    igb_autoneg_resume(core);
> +

The comment present in the previous version is gone.

Regards,
Akihiko Odaki