Hi Michael,

Acked-by: Wenzhuo Lu <redacted>

-----Original Message-----
From: Qiu, Michael
Sent: Tuesday, February 2, 2016 10:07 AM
To: Lu, Wenzhuo; dev@dpdk.org
Cc: Zhou, Danny; Liu, Yong; Liang, Cunming; Zhang, Helin
Subject: Re: [PATCH v2] ixgbe: Fix disable interrupt twice

[+cc helin]

On 2/2/2016 9:03 AM, Lu, Wenzhuo wrote:

quoted

Hi Michael,

quoted

-----Original Message-----
From: Qiu, Michael
Sent: Monday, February 1, 2016 4:05 PM
To: Lu, Wenzhuo; dev@dpdk.org
Cc: Zhou, Danny; Liu, Yong; Liang, Cunming
Subject: Re: [PATCH v2] ixgbe: Fix disable interrupt twice

On 1/29/2016 4:07 PM, Lu, Wenzhuo wrote:

quoted

Hi Michael,

quoted

-----Original Message-----
From: Qiu, Michael
Sent: Friday, January 29, 2016 1:58 PM
To: dev@dpdk.org
Cc: Zhou, Danny; Liu, Yong; Liang, Cunming; Lu, Wenzhuo; Qiu,
Michael
Subject: [PATCH v2] ixgbe: Fix disable interrupt twice

Currently, ixgbe vf and pf will disable interrupt twice in stop
stage and uninit stage. It will cause an error:

    testpmd> quit

    Shutting down port 0...
    Stopping ports...
    Done
    Closing ports...
    EAL: Error disabling MSI-X interrupts for fd 26
    Done

Becasue the interrupt already been disabled in stop stage.
Since it is enabled in init stage, better remove from stop stage.

I'm afraid it's not a good idea to just remove the intr_disable from dev_stop.
I think dev_stop have the chance to be used independently with
dev_unint. In

this scenario, we still need intr_disable, right?

quoted

Maybe what we need is some check before we disable the intr:)

Yes, indeed we need some check in disable intr, but it need
additional fields in "struct rte_intr_handle",  and it's much saft to
do so, but as I check i40e/fm10k code, only ixgbe disable it in dev_stop().

I found fm10k doesn't enable intr in dev_start. So, I think it's OK. But i40e

enables intr in dev_start.

quoted

To my opinion, it's more like i40e misses the intr_disable in dev_stop.

I don't think i40e miss it, because it not the right please to disable interrupt.
because all interrupts are enabled in init stage.

Actually, ixgbe enable the interrupt in init stage, but in dev_start, it disable it first
and re-enable, so it just the same with doing nothing about interrupt.

Just think below:

1. start the port.(interrupt already enabled in init stage, disable -->
re-enable)
2. stop the port.(disable interrupt)
3. start port again(Try to disable, but failed, already disabled)

Would you think the code has issue?

Got your point. So, dev_start/stop will not influence the state of intr enabling/disabling.
The intr will be enabled/disabled during dev_init/unint. 
Thanks.

Thanks,
Michael

quoted

Maybe we can follow fm10k's style.

quoted

On other hand, if we remove it in dev_stop, any side effect? In ixgbe
start, it will always disable it first and then re-enable it, so it's safe.

I think you mean we can disable intr anyway even if it has been disabled.

Actually, we couldn't, DPDK call VFIO ioctl to kernel to disable interrupts, and if
we try disable twice, it will return and error.
That's why I mean we need a flag to show the interrupts stats. If it already
disabled, we do not need call in to kernel. just return and give a warning
message.

Thanks,
Michael

quoted

 Sounds more like why we don't
need this patch :)

quoted

Thanks,
Michael