[dpdk-dev,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.

Fixes: 0eb609239efd ("ixgbe: enable Rx queue interrupts for PF and VF")

Signed-off-by: Michael Qiu <michael.qiu@intel.com>
---
 v2 --> v1:
     fix error in commit log word "interrupte"

 drivers/net/ixgbe/ixgbe_ethdev.c | 6 ------
 1 file changed, 6 deletions(-)
  

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:)
  

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().

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.

Thanks,
Michael
>
  

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.
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. Sounds more like why we don't
need this patch :)

> 
> Thanks,
> Michael
> >
  

[+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
>
  

> -----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:
> > 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?
[Zhang, Helin] in ixgbe PMD, it can be seen that uninit() calls dev_close(),
which calls dev_stop(). So I think the disabling can be done only in dev_stop().
All others can make use of dev_stop to disable the interrupt.

Regards,
Helin

> 
> 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
> >
  

Hi Michael,

Acked-by: Wenzhuo Lu <wenzhuo.lu@intel.com>

> -----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:
> > 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?
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
> 
> > 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
> >
  

On 2/2/2016 10:14 AM, Zhang, Helin wrote:
>
>> -----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:
>>> 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?
> [Zhang, Helin] in ixgbe PMD, it can be seen that uninit() calls dev_close(),
> which calls dev_stop(). So I think the disabling can be done only in dev_stop().
> All others can make use of dev_stop to disable the interrupt.

As I said, if it is in dev_stop, it will has issue when dev_start -->
dev_stop --> dev_start, this also could applied in i40e and fm10k. If
you want to put it in dev_stop, better to remove enable interrupts in
init stage, and only put it in dev_start.

Thanks,
Michael
> Regards,
> Helin
>
>> 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
>
  

> -----Original Message-----
> From: Qiu, Michael
> Sent: Tuesday, February 2, 2016 10:57 AM
> To: Zhang, Helin <helin.zhang@intel.com>; Lu, Wenzhuo
> <wenzhuo.lu@intel.com>; dev@dpdk.org
> Cc: Zhou, Danny <danny.zhou@intel.com>; Liu, Yong <yong.liu@intel.com>;
> Liang, Cunming <cunming.liang@intel.com>
> Subject: Re: [PATCH v2] ixgbe: Fix disable interrupt twice
> 
> On 2/2/2016 10:14 AM, Zhang, Helin wrote:
> >
> >> -----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:
> >>> 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?
> > [Zhang, Helin] in ixgbe PMD, it can be seen that uninit() calls
> > dev_close(), which calls dev_stop(). So I think the disabling can be done only in
> dev_stop().
> > All others can make use of dev_stop to disable the interrupt.
> 
> As I said, if it is in dev_stop, it will has issue when dev_start --> dev_stop -->
> dev_start, this also could applied in i40e and fm10k. If you want to put it in
> dev_stop, better to remove enable interrupts in init stage, and only put it in
> dev_start.
Oh, yes, you are talking about the refactoring. That's good, and reasonable.
Please do more validation with LSC, mailbox, rx interrupts, to make sure there
is no issue introduced.

Thanks,
Helin

> 
> Thanks,
> Michael
> > Regards,
> > Helin
> >
> >> 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
> >
  

On 2/2/2016 11:07 AM, Zhang, Helin wrote:
>
>> -----Original Message-----
>> From: Qiu, Michael
>> Sent: Tuesday, February 2, 2016 10:57 AM
>> To: Zhang, Helin <helin.zhang@intel.com>; Lu, Wenzhuo
>> <wenzhuo.lu@intel.com>; dev@dpdk.org
>> Cc: Zhou, Danny <danny.zhou@intel.com>; Liu, Yong <yong.liu@intel.com>;
>> Liang, Cunming <cunming.liang@intel.com>
>> Subject: Re: [PATCH v2] ixgbe: Fix disable interrupt twice
>>
>> On 2/2/2016 10:14 AM, Zhang, Helin wrote:
>>>> -----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:
>>>>> 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?
>>> [Zhang, Helin] in ixgbe PMD, it can be seen that uninit() calls
>>> dev_close(), which calls dev_stop(). So I think the disabling can be done only in
>> dev_stop().
>>> All others can make use of dev_stop to disable the interrupt.
>> As I said, if it is in dev_stop, it will has issue when dev_start --> dev_stop -->
>> dev_start, this also could applied in i40e and fm10k. If you want to put it in
>> dev_stop, better to remove enable interrupts in init stage, and only put it in
>> dev_start.
> Oh, yes, you are talking about the refactoring. That's good, and reasonable.
> Please do more validation with LSC, mailbox, rx interrupts, to make sure there
> is no issue introduced.

I have no plan to do code refactor, it includes lots of validation, and
will influence many components, time is limited for 2.3. I would like
keep it in uninit and remove it from stop, this only affect ixgbe, and I
have done validation for it.

Thanks,
Michael
> Thanks,
> Helin
>
>> Thanks,
>> Michael
>>> Regards,
>>> Helin
>>>
>>>> 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
>
  

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Qiu, Michael
> Sent: Tuesday, February 02, 2016 2:57 AM
> To: Zhang, Helin; Lu, Wenzhuo; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2] ixgbe: Fix disable interrupt twice
> 
> On 2/2/2016 10:14 AM, Zhang, Helin wrote:
> >
> >> -----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:
> >>> 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?
> > [Zhang, Helin] in ixgbe PMD, it can be seen that uninit() calls dev_close(),
> > which calls dev_stop(). So I think the disabling can be done only in dev_stop().
> > All others can make use of dev_stop to disable the interrupt.
> 
> As I said, if it is in dev_stop, it will has issue when dev_start -->
> dev_stop --> dev_start, this also could applied in i40e and fm10k. If
> you want to put it in dev_stop, better to remove enable interrupts in
> init stage, and only put it in dev_start.

We can't remove enabling interrupt at init stage and put it only in dev_start().
That means PF couldn't handle interrupts from VF till dev_start() will be executed on PF
 - which could never happen.
For same reason we can't disable all interrupts in dev_stop().
See: http://dpdk.org/ml/archives/dev/2015-November/027238.html
Konstantin

> 
> Thanks,
> Michael
> > Regards,
> > Helin
> >
> >> 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
> >
  

On 2016/2/2 19:03, Ananyev, Konstantin wrote:
>

[...]

>>>> 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?
>>> [Zhang, Helin] in ixgbe PMD, it can be seen that uninit() calls dev_close(),
>>> which calls dev_stop(). So I think the disabling can be done only in dev_stop().
>>> All others can make use of dev_stop to disable the interrupt.
>> As I said, if it is in dev_stop, it will has issue when dev_start -->
>> dev_stop --> dev_start, this also could applied in i40e and fm10k. If
>> you want to put it in dev_stop, better to remove enable interrupts in
>> init stage, and only put it in dev_start.
> We can't remove enabling interrupt at init stage and put it only in dev_start().
> That means PF couldn't handle interrupts from VF till dev_start() will be executed on PF
>  - which could never happen.
> For same reason we can't disable all interrupts in dev_stop().
> See: http://dpdk.org/ml/archives/dev/2015-November/027238.html

Hi, Konstantin

Yes, you are right.

So the only way to fix this issue should remove it in dev_stop(), and
left it in uinit() stage, which my patch does.

Am I right?

Thanks,
Michael
> Konstantin
>
>> Thanks,
>> Michael
>>> Regards,
>>> Helin
>>>
>>>> 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
>
  

Hi Michael


> 
> On 2016/2/2 19:03, Ananyev, Konstantin wrote:
> >
> 
> [...]
> 
> >>>> 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?
> >>> [Zhang, Helin] in ixgbe PMD, it can be seen that uninit() calls dev_close(),
> >>> which calls dev_stop(). So I think the disabling can be done only in dev_stop().
> >>> All others can make use of dev_stop to disable the interrupt.
> >> As I said, if it is in dev_stop, it will has issue when dev_start -->
> >> dev_stop --> dev_start, this also could applied in i40e and fm10k. If
> >> you want to put it in dev_stop, better to remove enable interrupts in
> >> init stage, and only put it in dev_start.
> > We can't remove enabling interrupt at init stage and put it only in dev_start().
> > That means PF couldn't handle interrupts from VF till dev_start() will be executed on PF
> >  - which could never happen.
> > For same reason we can't disable all interrupts in dev_stop().
> > See: http://dpdk.org/ml/archives/dev/2015-November/027238.html
> 
> Hi, Konstantin
> 
> Yes, you are right.
> 
> So the only way to fix this issue should remove it in dev_stop(), and
> left it in uinit() stage, which my patch does.
> 
> Am I right?

Yes, I think so.
PF should be able to receive MBOX interrupts  after dev_stop().
Konstantin

> 
> Thanks,
> Michael
> > Konstantin
> >
> >> Thanks,
> >> Michael
> >>> Regards,
> >>> Helin
> >>>
> >>>> 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
> >
  

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Michael Qiu
> Sent: Friday, January 29, 2016 1:58 PM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [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.
> 
> Fixes: 0eb609239efd ("ixgbe: enable Rx queue interrupts for PF and VF")
> 
> Signed-off-by: Michael Qiu <michael.qiu@intel.com>
Acked-by: Helin Zhang <helin.zhang@intel.com>

> ---
>  v2 --> v1:
>      fix error in commit log word "interrupte"
> 
>  drivers/net/ixgbe/ixgbe_ethdev.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
> b/drivers/net/ixgbe/ixgbe_ethdev.c
> index 4c4c6df..a561f8d 100644
> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> @@ -2192,9 +2192,6 @@ ixgbe_dev_stop(struct rte_eth_dev *dev)
>  	/* disable interrupts */
>  	ixgbe_disable_intr(hw);
> 
> -	/* disable intr eventfd mapping */
> -	rte_intr_disable(intr_handle);
> -
>  	/* reset the NIC */
>  	ixgbe_pf_reset_hw(hw);
>  	hw->adapter_stopped = 0;
> @@ -3898,9 +3895,6 @@ ixgbevf_dev_stop(struct rte_eth_dev *dev)
> 
>  	ixgbe_dev_clear_queues(dev);
> 
> -	/* disable intr eventfd mapping */
> -	rte_intr_disable(intr_handle);
> -
>  	/* Clean datapath event and queue/vec mapping */
>  	rte_intr_efd_disable(intr_handle);
>  	if (intr_handle->intr_vec != NULL) {
> --
> 1.9.3
  

On Tue, Feb 23, 2016 at 02:10:57AM +0000, Zhang, Helin wrote:
> 
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Michael Qiu
> > Sent: Friday, January 29, 2016 1:58 PM
> > To: dev@dpdk.org
> > Subject: [dpdk-dev] [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.
> > 
> > Fixes: 0eb609239efd ("ixgbe: enable Rx queue interrupts for PF and VF")
> > 
> > Signed-off-by: Michael Qiu <michael.qiu@intel.com>
> Acked-by: Helin Zhang <helin.zhang@intel.com>
> 
applied to dpdk-next-net/rel_16_04

/Bruce