Re: [dpdk-dev] [PATCH v2] ixgbe: Fix disable interrupt twice

["Qiu, Michael" <michael.qiu@intel.com>]

[+cc helin]

On 2/2/2016 9:03 AM, Lu, Wenzhuo wrote:
> Hi Michael,
>
>> -----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:
>>> Hi Michael,
>>>
>>>> -----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?
>>> 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.
> 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?

Thanks,
Michael

> Maybe we can follow fm10k's style.
>
>> 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

>  Sounds more like why we don't
> need this patch :)
>
>> Thanks,
>> Michael
>