[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] e1000e: remove internal interrupt flag
From: |
Dmitry Fleytman |
Subject: |
Re: [Qemu-devel] [PATCH] e1000e: remove internal interrupt flag |
Date: |
Thu, 18 Aug 2016 23:27:46 +0300 |
> On 18 Aug 2016, at 19:41 PM, Markus Armbruster <address@hidden> wrote:
>
> Dmitry Fleytman <address@hidden> writes:
>
>>> On 18 Aug 2016, at 17:15, Cao jin <address@hidden> wrote:
>>>
>>> Commit 66bf7d58 removed internal msi state flag E1000E_USE_MSI,
>>> E1000E_USE_MSIX
>>> is not necessary too, remove it now. And interrupt flag field intr_state
>>> also
>>> can be removed now.
>>>
>>> CC: Dmitry Fleytman <address@hidden>
>>> CC: Jason Wang <address@hidden>
>>> CC: Markus Armbruster <address@hidden>
>>> CC: Marcel Apfelbaum <address@hidden>
>>> CC: Michael S. Tsirkin <address@hidden>
>>> CC: Paolo Bonzini <address@hidden>
>>> Signed-off-by: Cao jin <address@hidden>
>>> ---
>>> hw/net/e1000e.c | 8 +-------
>>> 1 file changed, 1 insertion(+), 7 deletions(-)
>>>
>>> diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
>>> index 82a7be1..72aad21 100644
>>> --- a/hw/net/e1000e.c
>>> +++ b/hw/net/e1000e.c
>>> @@ -69,7 +69,6 @@ typedef struct E1000EState {
>>> uint16_t subsys_ven_used;
>>> uint16_t subsys_used;
>>>
>>> - uint32_t intr_state;
>>> bool disable_vnet;
>>>
>>> E1000ECore core;
>>> @@ -89,8 +88,6 @@ typedef struct E1000EState {
>>> #define E1000E_MSIX_TABLE (0x0000)
>>> #define E1000E_MSIX_PBA (0x2000)
>>>
>>> -#define E1000E_USE_MSIX BIT(0)
>>> -
>>> static uint64_t
>>> e1000e_mmio_read(void *opaque, hwaddr addr, unsigned size)
>>> {
>>> @@ -302,8 +299,6 @@ e1000e_init_msix(E1000EState *s)
>>> } 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;
>>
>> So you are removing error handling here. But what if e1000e_use_msix_vectors
>> fails?
>
> Before the patch, E1000E_USE_MSIX is set exactly when MSI-X was enabled
> successfully. It's only use is in MSI-X cleanup (next hunk): cleanup is
> skipped when the flag isn't set.
>
> The flag is pointless, because we can just as well test whether MSI-X is
> enabled directly. That's what the patch does.
I see.
Acked-by: Dmitry Fleytman <address@hidden>
>
>>
>>> }
>>> }
>>> }
>>> @@ -311,7 +306,7 @@ e1000e_init_msix(E1000EState *s)
>>> static void
>>> e1000e_cleanup_msix(E1000EState *s)
>>> {
>>> - if (s->intr_state & E1000E_USE_MSIX) {
>>> + if (msix_enabled(PCI_DEVICE(s))) {
>>> e1000e_unuse_msix_vectors(s, E1000E_MSIX_VEC_NUM);
>>> msix_uninit(PCI_DEVICE(s), &s->msix, &s->msix);
>>> }
>>> @@ -601,7 +596,6 @@ static const VMStateDescription e1000e_vmstate = {
>>> VMSTATE_MSIX(parent_obj, E1000EState),
>>>
>>> VMSTATE_UINT32(ioaddr, E1000EState),
>>> - VMSTATE_UINT32(intr_state, E1000EState),
>
> We can still do this, because as Paolo pointed out, no released version
> of QEMU had this device.
>
>>> VMSTATE_UINT32(core.rxbuf_min_shift, E1000EState),
>>> VMSTATE_UINT8(core.rx_desc_len, E1000EState),
>>> VMSTATE_UINT32_ARRAY(core.rxbuf_sizes, E1000EState,
>>> --
>>> 2.1.0
>
> Reviewed-by: Markus Armbruster <address@hidden>