* [PATCH] ixgbe: Fix disable interrupt twice
@ 2016-01-29 5:51 Michael Qiu
2016-01-29 5:58 ` [PATCH v2] " Michael Qiu
0 siblings, 1 reply; 16+ messages in thread
From: Michael Qiu @ 2016-01-29 5:51 UTC (permalink / raw)
To: dev
Currently, ixgbe vf and pf will disable interrupte 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>
---
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
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH v2] ixgbe: Fix disable interrupt twice 2016-01-29 5:51 [PATCH] ixgbe: Fix disable interrupt twice Michael Qiu @ 2016-01-29 5:58 ` Michael Qiu 2016-01-29 8:07 ` Lu, Wenzhuo 2016-02-23 2:10 ` Zhang, Helin 0 siblings, 2 replies; 16+ messages in thread From: Michael Qiu @ 2016-01-29 5:58 UTC (permalink / raw) To: dev 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(-) 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 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v2] ixgbe: Fix disable interrupt twice 2016-01-29 5:58 ` [PATCH v2] " Michael Qiu @ 2016-01-29 8:07 ` Lu, Wenzhuo 2016-02-01 8:05 ` Qiu, Michael 2016-02-23 2:10 ` Zhang, Helin 1 sibling, 1 reply; 16+ messages in thread From: Lu, Wenzhuo @ 2016-01-29 8:07 UTC (permalink / raw) To: Qiu, Michael, dev 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:) ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] ixgbe: Fix disable interrupt twice 2016-01-29 8:07 ` Lu, Wenzhuo @ 2016-02-01 8:05 ` Qiu, Michael 2016-02-02 1:03 ` Lu, Wenzhuo 0 siblings, 1 reply; 16+ messages in thread From: Qiu, Michael @ 2016-02-01 8:05 UTC (permalink / raw) To: Lu, Wenzhuo, dev 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 > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] ixgbe: Fix disable interrupt twice 2016-02-01 8:05 ` Qiu, Michael @ 2016-02-02 1:03 ` Lu, Wenzhuo 2016-02-02 2:06 ` Qiu, Michael 0 siblings, 1 reply; 16+ messages in thread From: Lu, Wenzhuo @ 2016-02-02 1:03 UTC (permalink / raw) To: Qiu, Michael, dev 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 > > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] ixgbe: Fix disable interrupt twice 2016-02-02 1:03 ` Lu, Wenzhuo @ 2016-02-02 2:06 ` Qiu, Michael 2016-02-02 2:14 ` Zhang, Helin 2016-02-02 2:26 ` Lu, Wenzhuo 0 siblings, 2 replies; 16+ messages in thread From: Qiu, Michael @ 2016-02-02 2:06 UTC (permalink / raw) To: Lu, Wenzhuo, dev [+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 > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] ixgbe: Fix disable interrupt twice 2016-02-02 2:06 ` Qiu, Michael @ 2016-02-02 2:14 ` Zhang, Helin 2016-02-02 2:57 ` Qiu, Michael 2016-02-02 2:26 ` Lu, Wenzhuo 1 sibling, 1 reply; 16+ messages in thread From: Zhang, Helin @ 2016-02-02 2:14 UTC (permalink / raw) To: Qiu, Michael, Lu, Wenzhuo, dev > -----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 > > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] ixgbe: Fix disable interrupt twice 2016-02-02 2:14 ` Zhang, Helin @ 2016-02-02 2:57 ` Qiu, Michael 2016-02-02 3:07 ` Zhang, Helin 2016-02-02 11:03 ` Ananyev, Konstantin 0 siblings, 2 replies; 16+ messages in thread From: Qiu, Michael @ 2016-02-02 2:57 UTC (permalink / raw) To: Zhang, Helin, Lu, Wenzhuo, dev 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 > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] ixgbe: Fix disable interrupt twice 2016-02-02 2:57 ` Qiu, Michael @ 2016-02-02 3:07 ` Zhang, Helin 2016-02-02 3:15 ` Qiu, Michael 2016-02-02 11:03 ` Ananyev, Konstantin 1 sibling, 1 reply; 16+ messages in thread From: Zhang, Helin @ 2016-02-02 3:07 UTC (permalink / raw) To: Qiu, Michael, Lu, Wenzhuo, dev > -----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 > > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] ixgbe: Fix disable interrupt twice 2016-02-02 3:07 ` Zhang, Helin @ 2016-02-02 3:15 ` Qiu, Michael 0 siblings, 0 replies; 16+ messages in thread From: Qiu, Michael @ 2016-02-02 3:15 UTC (permalink / raw) To: Zhang, Helin, Lu, Wenzhuo, dev 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 > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] ixgbe: Fix disable interrupt twice 2016-02-02 2:57 ` Qiu, Michael 2016-02-02 3:07 ` Zhang, Helin @ 2016-02-02 11:03 ` Ananyev, Konstantin 2016-02-19 8:07 ` Qiu, Michael 1 sibling, 1 reply; 16+ messages in thread From: Ananyev, Konstantin @ 2016-02-02 11:03 UTC (permalink / raw) To: Qiu, Michael, Zhang, Helin, Lu, Wenzhuo, dev > -----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 > > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] ixgbe: Fix disable interrupt twice 2016-02-02 11:03 ` Ananyev, Konstantin @ 2016-02-19 8:07 ` Qiu, Michael 2016-02-19 15:14 ` Ananyev, Konstantin 0 siblings, 1 reply; 16+ messages in thread From: Qiu, Michael @ 2016-02-19 8:07 UTC (permalink / raw) To: Ananyev, Konstantin, Zhang, Helin, Lu, Wenzhuo, dev 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 > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] ixgbe: Fix disable interrupt twice 2016-02-19 8:07 ` Qiu, Michael @ 2016-02-19 15:14 ` Ananyev, Konstantin 0 siblings, 0 replies; 16+ messages in thread From: Ananyev, Konstantin @ 2016-02-19 15:14 UTC (permalink / raw) To: Qiu, Michael, Zhang, Helin, Lu, Wenzhuo, dev 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 > > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] ixgbe: Fix disable interrupt twice 2016-02-02 2:06 ` Qiu, Michael 2016-02-02 2:14 ` Zhang, Helin @ 2016-02-02 2:26 ` Lu, Wenzhuo 1 sibling, 0 replies; 16+ messages in thread From: Lu, Wenzhuo @ 2016-02-02 2:26 UTC (permalink / raw) To: Qiu, Michael, dev 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 > > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] ixgbe: Fix disable interrupt twice 2016-01-29 5:58 ` [PATCH v2] " Michael Qiu 2016-01-29 8:07 ` Lu, Wenzhuo @ 2016-02-23 2:10 ` Zhang, Helin 2016-02-26 14:39 ` Bruce Richardson 1 sibling, 1 reply; 16+ messages in thread From: Zhang, Helin @ 2016-02-23 2:10 UTC (permalink / raw) To: Qiu, Michael, dev > -----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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] ixgbe: Fix disable interrupt twice 2016-02-23 2:10 ` Zhang, Helin @ 2016-02-26 14:39 ` Bruce Richardson 0 siblings, 0 replies; 16+ messages in thread From: Bruce Richardson @ 2016-02-26 14:39 UTC (permalink / raw) To: Zhang, Helin; +Cc: dev 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2016-02-26 14:39 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-01-29 5:51 [PATCH] ixgbe: Fix disable interrupt twice Michael Qiu 2016-01-29 5:58 ` [PATCH v2] " Michael Qiu 2016-01-29 8:07 ` Lu, Wenzhuo 2016-02-01 8:05 ` Qiu, Michael 2016-02-02 1:03 ` Lu, Wenzhuo 2016-02-02 2:06 ` Qiu, Michael 2016-02-02 2:14 ` Zhang, Helin 2016-02-02 2:57 ` Qiu, Michael 2016-02-02 3:07 ` Zhang, Helin 2016-02-02 3:15 ` Qiu, Michael 2016-02-02 11:03 ` Ananyev, Konstantin 2016-02-19 8:07 ` Qiu, Michael 2016-02-19 15:14 ` Ananyev, Konstantin 2016-02-02 2:26 ` Lu, Wenzhuo 2016-02-23 2:10 ` Zhang, Helin 2016-02-26 14:39 ` Bruce Richardson
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.