DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] igb_uio: remove out-of-date comment
@ 2019-01-15 15:46 Xiaolong Ye
  2019-01-15 16:08 ` Burakov, Anatoly
  2019-01-16  0:34 ` [dpdk-dev] [PATCH v2] " Xiaolong Ye
  0 siblings, 2 replies; 12+ messages in thread
From: Xiaolong Ye @ 2019-01-15 15:46 UTC (permalink / raw)
  To: dev, Ferruh Yigit; +Cc: Xiaolong Ye

The comment for igbuio_pci_irqhandler is out of date as the code evolves,
remove it to avoid misleading.

Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
---
 kernel/linux/igb_uio/igb_uio.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/kernel/linux/igb_uio/igb_uio.c b/kernel/linux/igb_uio/igb_uio.c
index 3cf394bdf..7e69b4498 100644
--- a/kernel/linux/igb_uio/igb_uio.c
+++ b/kernel/linux/igb_uio/igb_uio.c
@@ -185,11 +185,6 @@ igbuio_pci_irqcontrol(struct uio_info *info, s32 irq_state)
 	return 0;
 }
 
-/**
- * This is interrupt handler which will check if the interrupt is for the right device.
- * If yes, disable it here and will be enable later.
- */
-static irqreturn_t
 igbuio_pci_irqhandler(int irq, void *dev_id)
 {
 	struct rte_uio_pci_dev *udev = (struct rte_uio_pci_dev *)dev_id;
-- 
2.17.1

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-dev] [PATCH] igb_uio: remove out-of-date comment
  2019-01-15 15:46 [dpdk-dev] [PATCH] igb_uio: remove out-of-date comment Xiaolong Ye
@ 2019-01-15 16:08 ` Burakov, Anatoly
  2019-01-15 23:54   ` Ye Xiaolong
  2019-01-16  0:34 ` [dpdk-dev] [PATCH v2] " Xiaolong Ye
  1 sibling, 1 reply; 12+ messages in thread
From: Burakov, Anatoly @ 2019-01-15 16:08 UTC (permalink / raw)
  To: Xiaolong Ye, dev, Ferruh Yigit

On 15-Jan-19 3:46 PM, Xiaolong Ye wrote:
> The comment for igbuio_pci_irqhandler is out of date as the code evolves,
> remove it to avoid misleading.
> 
> Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
> ---
>   kernel/linux/igb_uio/igb_uio.c | 5 -----
>   1 file changed, 5 deletions(-)
> 
> diff --git a/kernel/linux/igb_uio/igb_uio.c b/kernel/linux/igb_uio/igb_uio.c
> index 3cf394bdf..7e69b4498 100644
> --- a/kernel/linux/igb_uio/igb_uio.c
> +++ b/kernel/linux/igb_uio/igb_uio.c
> @@ -185,11 +185,6 @@ igbuio_pci_irqcontrol(struct uio_info *info, s32 irq_state)
>   	return 0;
>   }
>   
> -/**
> - * This is interrupt handler which will check if the interrupt is for the right device.
> - * If yes, disable it here and will be enable later.
> - */
> -static irqreturn_t
>   igbuio_pci_irqhandler(int irq, void *dev_id)
>   {
>   	struct rte_uio_pci_dev *udev = (struct rte_uio_pci_dev *)dev_id;
> 

I think you removed one extra line there...

-- 
Thanks,
Anatoly

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-dev] [PATCH] igb_uio: remove out-of-date comment
  2019-01-15 16:08 ` Burakov, Anatoly
@ 2019-01-15 23:54   ` Ye Xiaolong
  0 siblings, 0 replies; 12+ messages in thread
From: Ye Xiaolong @ 2019-01-15 23:54 UTC (permalink / raw)
  To: Burakov, Anatoly; +Cc: dev, Ferruh Yigit

On 01/15, Burakov, Anatoly wrote:
>On 15-Jan-19 3:46 PM, Xiaolong Ye wrote:
>> The comment for igbuio_pci_irqhandler is out of date as the code evolves,
>> remove it to avoid misleading.
>> 
>> Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
>> ---
>>   kernel/linux/igb_uio/igb_uio.c | 5 -----
>>   1 file changed, 5 deletions(-)
>> 
>> diff --git a/kernel/linux/igb_uio/igb_uio.c b/kernel/linux/igb_uio/igb_uio.c
>> index 3cf394bdf..7e69b4498 100644
>> --- a/kernel/linux/igb_uio/igb_uio.c
>> +++ b/kernel/linux/igb_uio/igb_uio.c
>> @@ -185,11 +185,6 @@ igbuio_pci_irqcontrol(struct uio_info *info, s32 irq_state)
>>   	return 0;
>>   }
>> -/**
>> - * This is interrupt handler which will check if the interrupt is for the right device.
>> - * If yes, disable it here and will be enable later.
>> - */
>> -static irqreturn_t
>>   igbuio_pci_irqhandler(int irq, void *dev_id)
>>   {
>>   	struct rte_uio_pci_dev *udev = (struct rte_uio_pci_dev *)dev_id;
>> 
>
>I think you removed one extra line there...

oops, thanks for the catch, I'll send a v2.

Thanks,
Xiaolong

>
>-- 
>Thanks,
>Anatoly

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [dpdk-dev] [PATCH v2] igb_uio: remove out-of-date comment
  2019-01-15 15:46 [dpdk-dev] [PATCH] igb_uio: remove out-of-date comment Xiaolong Ye
  2019-01-15 16:08 ` Burakov, Anatoly
@ 2019-01-16  0:34 ` Xiaolong Ye
  2019-01-16  1:38   ` Stephen Hemminger
  1 sibling, 1 reply; 12+ messages in thread
From: Xiaolong Ye @ 2019-01-16  0:34 UTC (permalink / raw)
  To: dev, Ferruh Yigit; +Cc: Xiaolong Ye

The comment for igbuio_pci_irqhandler is out of date as the code evolves,
remove it to avoid misleading.

Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
---
 kernel/linux/igb_uio/igb_uio.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/kernel/linux/igb_uio/igb_uio.c b/kernel/linux/igb_uio/igb_uio.c
index 3cf394bdf..d6ac51e79 100644
--- a/kernel/linux/igb_uio/igb_uio.c
+++ b/kernel/linux/igb_uio/igb_uio.c
@@ -185,10 +185,6 @@ igbuio_pci_irqcontrol(struct uio_info *info, s32 irq_state)
 	return 0;
 }
 
-/**
- * This is interrupt handler which will check if the interrupt is for the right device.
- * If yes, disable it here and will be enable later.
- */
 static irqreturn_t
 igbuio_pci_irqhandler(int irq, void *dev_id)
 {
-- 
2.17.1

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-dev] [PATCH v2] igb_uio: remove out-of-date comment
  2019-01-16  0:34 ` [dpdk-dev] [PATCH v2] " Xiaolong Ye
@ 2019-01-16  1:38   ` Stephen Hemminger
  2019-01-16  5:17     ` Ye Xiaolong
  2019-01-16  7:48     ` Ye Xiaolong
  0 siblings, 2 replies; 12+ messages in thread
From: Stephen Hemminger @ 2019-01-16  1:38 UTC (permalink / raw)
  To: Xiaolong Ye; +Cc: dev, Ferruh Yigit

On Wed, 16 Jan 2019 08:34:52 +0800
Xiaolong Ye <xiaolong.ye@intel.com> wrote:

> The comment for igbuio_pci_irqhandler is out of date as the code evolves,
> remove it to avoid misleading.
> 
> Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
> ---
>  kernel/linux/igb_uio/igb_uio.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/kernel/linux/igb_uio/igb_uio.c b/kernel/linux/igb_uio/igb_uio.c
> index 3cf394bdf..d6ac51e79 100644
> --- a/kernel/linux/igb_uio/igb_uio.c
> +++ b/kernel/linux/igb_uio/igb_uio.c
> @@ -185,10 +185,6 @@ igbuio_pci_irqcontrol(struct uio_info *info, s32 irq_state)
>  	return 0;
>  }
>  
> -/**
> - * This is interrupt handler which will check if the interrupt is for the right device.
> - * If yes, disable it here and will be enable later.
> - */
>  static irqreturn_t
>  igbuio_pci_irqhandler(int irq, void *dev_id)
>  {

The comment is partially correct; if you look at the legacy case.

Maybe better to move the comment to pci_check_and_mask_intx in compat.h?
I see there is another incorrect comment there.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-dev] [PATCH v2] igb_uio: remove out-of-date comment
  2019-01-16  1:38   ` Stephen Hemminger
@ 2019-01-16  5:17     ` Ye Xiaolong
  2019-01-16  7:48     ` Ye Xiaolong
  1 sibling, 0 replies; 12+ messages in thread
From: Ye Xiaolong @ 2019-01-16  5:17 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: dev, Ferruh Yigit

On 01/15, Stephen Hemminger wrote:
>On Wed, 16 Jan 2019 08:34:52 +0800
>Xiaolong Ye <xiaolong.ye@intel.com> wrote:
>
>> The comment for igbuio_pci_irqhandler is out of date as the code evolves,
>> remove it to avoid misleading.
>> 
>> Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
>> ---
>>  kernel/linux/igb_uio/igb_uio.c | 4 ----
>>  1 file changed, 4 deletions(-)
>> 
>> diff --git a/kernel/linux/igb_uio/igb_uio.c b/kernel/linux/igb_uio/igb_uio.c
>> index 3cf394bdf..d6ac51e79 100644
>> --- a/kernel/linux/igb_uio/igb_uio.c
>> +++ b/kernel/linux/igb_uio/igb_uio.c
>> @@ -185,10 +185,6 @@ igbuio_pci_irqcontrol(struct uio_info *info, s32 irq_state)
>>  	return 0;
>>  }
>>  
>> -/**
>> - * This is interrupt handler which will check if the interrupt is for the right device.
>> - * If yes, disable it here and will be enable later.
>> - */
>>  static irqreturn_t
>>  igbuio_pci_irqhandler(int irq, void *dev_id)
>>  {
>
>The comment is partially correct; if you look at the legacy case.
>
>Maybe better to move the comment to pci_check_and_mask_intx in compat.h?
>I see there is another incorrect comment there.
>

Hmm, you're right, this comment is correct for the legacy case, I'll move the
comment to pci_check_and_mask_intx as you suggested.

Thanks,
Xiaolong

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-dev] [PATCH v2] igb_uio: remove out-of-date comment
  2019-01-16  1:38   ` Stephen Hemminger
  2019-01-16  5:17     ` Ye Xiaolong
@ 2019-01-16  7:48     ` Ye Xiaolong
  2019-01-16 18:55       ` Stephen Hemminger
  1 sibling, 1 reply; 12+ messages in thread
From: Ye Xiaolong @ 2019-01-16  7:48 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: dev, Ferruh Yigit

Hi, Stephen

On 01/15, Stephen Hemminger wrote:
>On Wed, 16 Jan 2019 08:34:52 +0800
>Xiaolong Ye <xiaolong.ye@intel.com> wrote:
>
>> The comment for igbuio_pci_irqhandler is out of date as the code evolves,
>> remove it to avoid misleading.
>> 
>> Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
>> ---
>>  kernel/linux/igb_uio/igb_uio.c | 4 ----
>>  1 file changed, 4 deletions(-)
>> 
>> diff --git a/kernel/linux/igb_uio/igb_uio.c b/kernel/linux/igb_uio/igb_uio.c
>> index 3cf394bdf..d6ac51e79 100644
>> --- a/kernel/linux/igb_uio/igb_uio.c
>> +++ b/kernel/linux/igb_uio/igb_uio.c
>> @@ -185,10 +185,6 @@ igbuio_pci_irqcontrol(struct uio_info *info, s32 irq_state)
>>  	return 0;
>>  }
>>  
>> -/**
>> - * This is interrupt handler which will check if the interrupt is for the right device.
>> - * If yes, disable it here and will be enable later.
>> - */
>>  static irqreturn_t
>>  igbuio_pci_irqhandler(int irq, void *dev_id)
>>  {
>
>The comment is partially correct; if you look at the legacy case.
>
>Maybe better to move the comment to pci_check_and_mask_intx in compat.h?
>I see there is another incorrect comment there.
>

As I tried to understand pci_check_and_mask_intx behavior, I noticed that it 
was introduced by commit 399a3f0d ("igb_uio: fix IRQ mode handling"), and it
was derived from igbuio_set_interrupt_mask, however there were two parameters
in igbuio_set_interrupt_mask as below:

	static int
	igbuio_set_interrupt_mask(struct rte_uio_pci_dev *udev, int32_t state)

while the pci_check_and_mask_intx only has 1 parameter,

	static bool pci_check_and_mask_intx(struct pci_dev *pdev)

but in its function body it still use the "state" accorrding to commit 399a3f0d

+static bool pci_check_and_mask_intx(struct pci_dev *pdev)
 {
-       /* Some function names changes between 3.2.0 and 3.3.0... */
-#if LINUX_VERSION_CODE < KERNEL_VERSION(3, 3, 0)
-       pci_unblock_user_cfg_access(pdev);
-#else
-       pci_cfg_access_unlock(pdev);
-#endif
+       bool pending;
+       uint32_t status;
+
+       pci_block_user_cfg_access(dev);
+       pci_read_config_dword(pdev, PCI_COMMAND, &status);
+
+       /* interrupt is not ours, goes to out */
+       pending = (((status >> 16) & PCI_STATUS_INTERRUPT) != 0);
+       if (pending) {
+               uint16_t old, new;
+
+               old = status;
+               if (state != 0) <=========================== state still in use
+                       new = old & (~PCI_COMMAND_INTX_DISABLE);
+               else
+                       new = old | PCI_COMMAND_INTX_DISABLE;
+
+               if (old != new)
+                       pci_write_config_word(pdev, PCI_COMMAND, new);
+       }
+       pci_unblock_user_cfg_access(dev);
+
+       return pending;

and later this was fixed as a typo in commit 5b2f8137 ("igb_uio: fix typos for 
kernel older than 3.3") which seems not correct.

                old = status;
-               if (state != 0)
+               if (status != 0)

I feel like that pci_check_and_mask_intx still needs two parameters from code
point of view, please correct me if I was wrong or miss anything, or you think 
it's sensible, I'll cook a patch for it. 

Thanks,
Xiaolong

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-dev] [PATCH v2] igb_uio: remove out-of-date comment
  2019-01-16  7:48     ` Ye Xiaolong
@ 2019-01-16 18:55       ` Stephen Hemminger
  2019-01-17  2:13         ` Ye Xiaolong
  0 siblings, 1 reply; 12+ messages in thread
From: Stephen Hemminger @ 2019-01-16 18:55 UTC (permalink / raw)
  To: Ye Xiaolong; +Cc: dev, Ferruh Yigit

On Wed, 16 Jan 2019 15:48:36 +0800
Ye Xiaolong <xiaolong.ye@intel.com> wrote:

> Hi, Stephen
> 
> On 01/15, Stephen Hemminger wrote:
> >On Wed, 16 Jan 2019 08:34:52 +0800
> >Xiaolong Ye <xiaolong.ye@intel.com> wrote:
> >  
> >> The comment for igbuio_pci_irqhandler is out of date as the code evolves,
> >> remove it to avoid misleading.
> >> 
> >> Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
> >> ---
> >>  kernel/linux/igb_uio/igb_uio.c | 4 ----
> >>  1 file changed, 4 deletions(-)
> >> 
> >> diff --git a/kernel/linux/igb_uio/igb_uio.c b/kernel/linux/igb_uio/igb_uio.c
> >> index 3cf394bdf..d6ac51e79 100644
> >> --- a/kernel/linux/igb_uio/igb_uio.c
> >> +++ b/kernel/linux/igb_uio/igb_uio.c
> >> @@ -185,10 +185,6 @@ igbuio_pci_irqcontrol(struct uio_info *info, s32 irq_state)
> >>  	return 0;
> >>  }
> >>  
> >> -/**
> >> - * This is interrupt handler which will check if the interrupt is for the right device.
> >> - * If yes, disable it here and will be enable later.
> >> - */
> >>  static irqreturn_t
> >>  igbuio_pci_irqhandler(int irq, void *dev_id)
> >>  {  
> >
> >The comment is partially correct; if you look at the legacy case.
> >
> >Maybe better to move the comment to pci_check_and_mask_intx in compat.h?
> >I see there is another incorrect comment there.
> >  
> 
> As I tried to understand pci_check_and_mask_intx behavior, I noticed that it 
> was introduced by commit 399a3f0d ("igb_uio: fix IRQ mode handling"), and it
> was derived from igbuio_set_interrupt_mask, however there were two parameters
> in igbuio_set_interrupt_mask as below:
> 
> 	static int
> 	igbuio_set_interrupt_mask(struct rte_uio_pci_dev *udev, int32_t state)
> 
> while the pci_check_and_mask_intx only has 1 parameter,
> 
> 	static bool pci_check_and_mask_intx(struct pci_dev *pdev)
> 
> but in its function body it still use the "state" accorrding to commit 399a3f0d
> 
> +static bool pci_check_and_mask_intx(struct pci_dev *pdev)
>  {
> -       /* Some function names changes between 3.2.0 and 3.3.0... */
> -#if LINUX_VERSION_CODE < KERNEL_VERSION(3, 3, 0)
> -       pci_unblock_user_cfg_access(pdev);
> -#else
> -       pci_cfg_access_unlock(pdev);
> -#endif
> +       bool pending;
> +       uint32_t status;
> +
> +       pci_block_user_cfg_access(dev);
> +       pci_read_config_dword(pdev, PCI_COMMAND, &status);
> +
> +       /* interrupt is not ours, goes to out */
> +       pending = (((status >> 16) & PCI_STATUS_INTERRUPT) != 0);
> +       if (pending) {
> +               uint16_t old, new;
> +
> +               old = status;
> +               if (state != 0) <=========================== state still in use
> +                       new = old & (~PCI_COMMAND_INTX_DISABLE);
> +               else
> +                       new = old | PCI_COMMAND_INTX_DISABLE;
> +
> +               if (old != new)
> +                       pci_write_config_word(pdev, PCI_COMMAND, new);
> +       }
> +       pci_unblock_user_cfg_access(dev);
> +
> +       return pending;
> 
> and later this was fixed as a typo in commit 5b2f8137 ("igb_uio: fix typos for 
> kernel older than 3.3") which seems not correct.
> 
>                 old = status;
> -               if (state != 0)
> +               if (status != 0)
> 
> I feel like that pci_check_and_mask_intx still needs two parameters from code
> point of view, please correct me if I was wrong or miss anything, or you think 
> it's sensible, I'll cook a patch for it. 
> 
> Thanks,
> Xiaolong

This code is copy of upstream kernel function, you should look at that.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-dev] [PATCH v2] igb_uio: remove out-of-date comment
  2019-01-16 18:55       ` Stephen Hemminger
@ 2019-01-17  2:13         ` Ye Xiaolong
  2019-01-22  1:52           ` Ye Xiaolong
  2019-01-29 12:49           ` Ferruh Yigit
  0 siblings, 2 replies; 12+ messages in thread
From: Ye Xiaolong @ 2019-01-17  2:13 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: dev, Ferruh Yigit

On 01/16, Stephen Hemminger wrote:
>On Wed, 16 Jan 2019 15:48:36 +0800
>Ye Xiaolong <xiaolong.ye@intel.com> wrote:
>
>> Hi, Stephen
>> 
>> On 01/15, Stephen Hemminger wrote:
>> >On Wed, 16 Jan 2019 08:34:52 +0800
>> >Xiaolong Ye <xiaolong.ye@intel.com> wrote:
>> >  
>> >> The comment for igbuio_pci_irqhandler is out of date as the code evolves,
>> >> remove it to avoid misleading.
>> >> 
>> >> Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
>> >> ---
>> >>  kernel/linux/igb_uio/igb_uio.c | 4 ----
>> >>  1 file changed, 4 deletions(-)
>> >> 
>> >> diff --git a/kernel/linux/igb_uio/igb_uio.c b/kernel/linux/igb_uio/igb_uio.c
>> >> index 3cf394bdf..d6ac51e79 100644
>> >> --- a/kernel/linux/igb_uio/igb_uio.c
>> >> +++ b/kernel/linux/igb_uio/igb_uio.c
>> >> @@ -185,10 +185,6 @@ igbuio_pci_irqcontrol(struct uio_info *info, s32 irq_state)
>> >>  	return 0;
>> >>  }
>> >>  
>> >> -/**
>> >> - * This is interrupt handler which will check if the interrupt is for the right device.
>> >> - * If yes, disable it here and will be enable later.
>> >> - */
>> >>  static irqreturn_t
>> >>  igbuio_pci_irqhandler(int irq, void *dev_id)
>> >>  {  
>> >
>> >The comment is partially correct; if you look at the legacy case.
>> >
>> >Maybe better to move the comment to pci_check_and_mask_intx in compat.h?
>> >I see there is another incorrect comment there.
>> >  
>> 
>> As I tried to understand pci_check_and_mask_intx behavior, I noticed that it 
>> was introduced by commit 399a3f0d ("igb_uio: fix IRQ mode handling"), and it
>> was derived from igbuio_set_interrupt_mask, however there were two parameters
>> in igbuio_set_interrupt_mask as below:
>> 
>> 	static int
>> 	igbuio_set_interrupt_mask(struct rte_uio_pci_dev *udev, int32_t state)
>> 
>> while the pci_check_and_mask_intx only has 1 parameter,
>> 
>> 	static bool pci_check_and_mask_intx(struct pci_dev *pdev)
>> 
>> but in its function body it still use the "state" accorrding to commit 399a3f0d
>> 
>> +static bool pci_check_and_mask_intx(struct pci_dev *pdev)
>>  {
>> -       /* Some function names changes between 3.2.0 and 3.3.0... */
>> -#if LINUX_VERSION_CODE < KERNEL_VERSION(3, 3, 0)
>> -       pci_unblock_user_cfg_access(pdev);
>> -#else
>> -       pci_cfg_access_unlock(pdev);
>> -#endif
>> +       bool pending;
>> +       uint32_t status;
>> +
>> +       pci_block_user_cfg_access(dev);
>> +       pci_read_config_dword(pdev, PCI_COMMAND, &status);
>> +
>> +       /* interrupt is not ours, goes to out */
>> +       pending = (((status >> 16) & PCI_STATUS_INTERRUPT) != 0);
>> +       if (pending) {
>> +               uint16_t old, new;
>> +
>> +               old = status;
>> +               if (state != 0) <=========================== state still in use
>> +                       new = old & (~PCI_COMMAND_INTX_DISABLE);
>> +               else
>> +                       new = old | PCI_COMMAND_INTX_DISABLE;
>> +
>> +               if (old != new)
>> +                       pci_write_config_word(pdev, PCI_COMMAND, new);
>> +       }
>> +       pci_unblock_user_cfg_access(dev);
>> +
>> +       return pending;
>> 
>> and later this was fixed as a typo in commit 5b2f8137 ("igb_uio: fix typos for 
>> kernel older than 3.3") which seems not correct.
>> 
>>                 old = status;
>> -               if (state != 0)
>> +               if (status != 0)
>> 
>> I feel like that pci_check_and_mask_intx still needs two parameters from code
>> point of view, please correct me if I was wrong or miss anything, or you think 
>> it's sensible, I'll cook a patch for it. 
>> 
>> Thanks,
>> Xiaolong
>
>This code is copy of upstream kernel function, you should look at that.

Thanks, just checked the kernel version of pci_check_and_mask_intx, and it is a 
wrap of pci_check_and_set_intx_mask(pdev, true).


	bool pci_check_and_mask_intx(struct pci_dev *dev)
	{
		return pci_check_and_set_intx_mask(dev, true);
	}


	static bool pci_check_and_set_intx_mask(struct pci_dev *dev, bool mask)
	{
		struct pci_bus *bus = dev->bus;
		...
		origcmd = cmd_status_dword;
		newcmd = origcmd & ~PCI_COMMAND_INTX_DISABLE;
		if (mask)
			newcmd |= PCI_COMMAND_INTX_DISABLE;
		if (newcmd != origcmd)
			bus->ops->write(bus, dev->devfn, PCI_COMMAND, 2, newcmd);

	done:
		raw_spin_unlock_irqrestore(&pci_lock, flags);

		return mask_updated;
	}

if mask is set to ture, PCI_COMMAND_INTX_DISABLE will be set in newcmd.

And current dpdk version doesn't align with it:

	static bool pci_check_and_mask_intx(struct pci_dev *pdev)
	{
		bool pending;
		uint32_t status;

		pci_block_user_cfg_access(pdev);
		pci_read_config_dword(pdev, PCI_COMMAND, &status);

		/* interrupt is not ours, goes to out */
		pending = (((status >> 16) & PCI_STATUS_INTERRUPT) != 0);
		if (pending) {
			uint16_t old, new;

			old = status;
			if (status != 0)
				new = old & (~PCI_COMMAND_INTX_DISABLE);
			else
				new = old | PCI_COMMAND_INTX_DISABLE;

			if (old != new)
				pci_write_config_word(pdev, PCI_COMMAND, new);
		}
		pci_unblock_user_cfg_access(pdev);

		return pending;
	}

In this code, if the interrupt is triggered by this device, both pending and
status will not be 0, so the PCI_COMMAND_INTX_DISABLE will be cleared for new.

To align the behavior with upstream kernel, we may need change like:

diff --git a/kernel/linux/igb_uio/compat.h b/kernel/linux/igb_uio/compat.h
index 8dbb896ae..a78c97f91 100644
--- a/kernel/linux/igb_uio/compat.h
+++ b/kernel/linux/igb_uio/compat.h
@@ -107,10 +107,7 @@ static bool pci_check_and_mask_intx(struct pci_dev *pdev)
                uint16_t old, new;

                old = status;
-               if (status != 0)
-                       new = old & (~PCI_COMMAND_INTX_DISABLE);
-               else
-                       new = old | PCI_COMMAND_INTX_DISABLE;
+               new = old | PCI_COMMAND_INTX_DISABLE;

                if (old != new)
                        pci_write_config_word(pdev, PCI_COMMAND, new);


Please correct me if I was wrong.

Thanks,
Xiaolong

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-dev] [PATCH v2] igb_uio: remove out-of-date comment
  2019-01-17  2:13         ` Ye Xiaolong
@ 2019-01-22  1:52           ` Ye Xiaolong
  2019-01-29 12:49           ` Ferruh Yigit
  1 sibling, 0 replies; 12+ messages in thread
From: Ye Xiaolong @ 2019-01-22  1:52 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: dev, Ferruh Yigit

Hi, Stephen

On 01/17, Ye Xiaolong wrote:
>On 01/16, Stephen Hemminger wrote:
>>On Wed, 16 Jan 2019 15:48:36 +0800
>>Ye Xiaolong <xiaolong.ye@intel.com> wrote:
>>
>>> Hi, Stephen
>>> 
>>> On 01/15, Stephen Hemminger wrote:
>>> >On Wed, 16 Jan 2019 08:34:52 +0800
>>> >Xiaolong Ye <xiaolong.ye@intel.com> wrote:
>>> >  
>>> >> The comment for igbuio_pci_irqhandler is out of date as the code evolves,
>>> >> remove it to avoid misleading.
>>> >> 
>>> >> Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
>>> >> ---
>>> >>  kernel/linux/igb_uio/igb_uio.c | 4 ----
>>> >>  1 file changed, 4 deletions(-)
>>> >> 
>>> >> diff --git a/kernel/linux/igb_uio/igb_uio.c b/kernel/linux/igb_uio/igb_uio.c
>>> >> index 3cf394bdf..d6ac51e79 100644
>>> >> --- a/kernel/linux/igb_uio/igb_uio.c
>>> >> +++ b/kernel/linux/igb_uio/igb_uio.c
>>> >> @@ -185,10 +185,6 @@ igbuio_pci_irqcontrol(struct uio_info *info, s32 irq_state)
>>> >>  	return 0;
>>> >>  }
>>> >>  
>>> >> -/**
>>> >> - * This is interrupt handler which will check if the interrupt is for the right device.
>>> >> - * If yes, disable it here and will be enable later.
>>> >> - */
>>> >>  static irqreturn_t
>>> >>  igbuio_pci_irqhandler(int irq, void *dev_id)
>>> >>  {  
>>> >
>>> >The comment is partially correct; if you look at the legacy case.
>>> >
>>> >Maybe better to move the comment to pci_check_and_mask_intx in compat.h?
>>> >I see there is another incorrect comment there.
>>> >  
>>> 
>>> As I tried to understand pci_check_and_mask_intx behavior, I noticed that it 
>>> was introduced by commit 399a3f0d ("igb_uio: fix IRQ mode handling"), and it
>>> was derived from igbuio_set_interrupt_mask, however there were two parameters
>>> in igbuio_set_interrupt_mask as below:
>>> 
>>> 	static int
>>> 	igbuio_set_interrupt_mask(struct rte_uio_pci_dev *udev, int32_t state)
>>> 
>>> while the pci_check_and_mask_intx only has 1 parameter,
>>> 
>>> 	static bool pci_check_and_mask_intx(struct pci_dev *pdev)
>>> 
>>> but in its function body it still use the "state" accorrding to commit 399a3f0d
>>> 
>>> +static bool pci_check_and_mask_intx(struct pci_dev *pdev)
>>>  {
>>> -       /* Some function names changes between 3.2.0 and 3.3.0... */
>>> -#if LINUX_VERSION_CODE < KERNEL_VERSION(3, 3, 0)
>>> -       pci_unblock_user_cfg_access(pdev);
>>> -#else
>>> -       pci_cfg_access_unlock(pdev);
>>> -#endif
>>> +       bool pending;
>>> +       uint32_t status;
>>> +
>>> +       pci_block_user_cfg_access(dev);
>>> +       pci_read_config_dword(pdev, PCI_COMMAND, &status);
>>> +
>>> +       /* interrupt is not ours, goes to out */
>>> +       pending = (((status >> 16) & PCI_STATUS_INTERRUPT) != 0);
>>> +       if (pending) {
>>> +               uint16_t old, new;
>>> +
>>> +               old = status;
>>> +               if (state != 0) <=========================== state still in use
>>> +                       new = old & (~PCI_COMMAND_INTX_DISABLE);
>>> +               else
>>> +                       new = old | PCI_COMMAND_INTX_DISABLE;
>>> +
>>> +               if (old != new)
>>> +                       pci_write_config_word(pdev, PCI_COMMAND, new);
>>> +       }
>>> +       pci_unblock_user_cfg_access(dev);
>>> +
>>> +       return pending;
>>> 
>>> and later this was fixed as a typo in commit 5b2f8137 ("igb_uio: fix typos for 
>>> kernel older than 3.3") which seems not correct.
>>> 
>>>                 old = status;
>>> -               if (state != 0)
>>> +               if (status != 0)
>>> 
>>> I feel like that pci_check_and_mask_intx still needs two parameters from code
>>> point of view, please correct me if I was wrong or miss anything, or you think 
>>> it's sensible, I'll cook a patch for it. 
>>> 
>>> Thanks,
>>> Xiaolong
>>
>>This code is copy of upstream kernel function, you should look at that.
>
>Thanks, just checked the kernel version of pci_check_and_mask_intx, and it is a 
>wrap of pci_check_and_set_intx_mask(pdev, true).
>
>
>	bool pci_check_and_mask_intx(struct pci_dev *dev)
>	{
>		return pci_check_and_set_intx_mask(dev, true);
>	}
>
>
>	static bool pci_check_and_set_intx_mask(struct pci_dev *dev, bool mask)
>	{
>		struct pci_bus *bus = dev->bus;
>		...
>		origcmd = cmd_status_dword;
>		newcmd = origcmd & ~PCI_COMMAND_INTX_DISABLE;
>		if (mask)
>			newcmd |= PCI_COMMAND_INTX_DISABLE;
>		if (newcmd != origcmd)
>			bus->ops->write(bus, dev->devfn, PCI_COMMAND, 2, newcmd);
>
>	done:
>		raw_spin_unlock_irqrestore(&pci_lock, flags);
>
>		return mask_updated;
>	}
>
>if mask is set to ture, PCI_COMMAND_INTX_DISABLE will be set in newcmd.
>
>And current dpdk version doesn't align with it:
>
>	static bool pci_check_and_mask_intx(struct pci_dev *pdev)
>	{
>		bool pending;
>		uint32_t status;
>
>		pci_block_user_cfg_access(pdev);
>		pci_read_config_dword(pdev, PCI_COMMAND, &status);
>
>		/* interrupt is not ours, goes to out */
>		pending = (((status >> 16) & PCI_STATUS_INTERRUPT) != 0);
>		if (pending) {
>			uint16_t old, new;
>
>			old = status;
>			if (status != 0)
>				new = old & (~PCI_COMMAND_INTX_DISABLE);
>			else
>				new = old | PCI_COMMAND_INTX_DISABLE;
>
>			if (old != new)
>				pci_write_config_word(pdev, PCI_COMMAND, new);
>		}
>		pci_unblock_user_cfg_access(pdev);
>
>		return pending;
>	}
>
>In this code, if the interrupt is triggered by this device, both pending and
>status will not be 0, so the PCI_COMMAND_INTX_DISABLE will be cleared for new.
>
>To align the behavior with upstream kernel, we may need change like:
>
>diff --git a/kernel/linux/igb_uio/compat.h b/kernel/linux/igb_uio/compat.h
>index 8dbb896ae..a78c97f91 100644
>--- a/kernel/linux/igb_uio/compat.h
>+++ b/kernel/linux/igb_uio/compat.h
>@@ -107,10 +107,7 @@ static bool pci_check_and_mask_intx(struct pci_dev *pdev)
>                uint16_t old, new;
>
>                old = status;
>-               if (status != 0)
>-                       new = old & (~PCI_COMMAND_INTX_DISABLE);
>-               else
>-                       new = old | PCI_COMMAND_INTX_DISABLE;
>+               new = old | PCI_COMMAND_INTX_DISABLE;
>
>                if (old != new)
>                        pci_write_config_word(pdev, PCI_COMMAND, new);
>
>
>Please correct me if I was wrong.

Does my above fix make any sense?

Thanks,
Xiaolong
>
>Thanks,
>Xiaolong

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-dev] [PATCH v2] igb_uio: remove out-of-date comment
  2019-01-17  2:13         ` Ye Xiaolong
  2019-01-22  1:52           ` Ye Xiaolong
@ 2019-01-29 12:49           ` Ferruh Yigit
  2019-01-31  8:13             ` Ye Xiaolong
  1 sibling, 1 reply; 12+ messages in thread
From: Ferruh Yigit @ 2019-01-29 12:49 UTC (permalink / raw)
  To: Ye Xiaolong, Stephen Hemminger; +Cc: dev

On 1/17/2019 2:13 AM, Ye Xiaolong wrote:
> On 01/16, Stephen Hemminger wrote:
>> On Wed, 16 Jan 2019 15:48:36 +0800
>> Ye Xiaolong <xiaolong.ye@intel.com> wrote:
>>
>>> Hi, Stephen
>>>
>>> On 01/15, Stephen Hemminger wrote:
>>>> On Wed, 16 Jan 2019 08:34:52 +0800
>>>> Xiaolong Ye <xiaolong.ye@intel.com> wrote:
>>>>  
>>>>> The comment for igbuio_pci_irqhandler is out of date as the code evolves,
>>>>> remove it to avoid misleading.
>>>>>
>>>>> Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
>>>>> ---
>>>>>  kernel/linux/igb_uio/igb_uio.c | 4 ----
>>>>>  1 file changed, 4 deletions(-)
>>>>>
>>>>> diff --git a/kernel/linux/igb_uio/igb_uio.c b/kernel/linux/igb_uio/igb_uio.c
>>>>> index 3cf394bdf..d6ac51e79 100644
>>>>> --- a/kernel/linux/igb_uio/igb_uio.c
>>>>> +++ b/kernel/linux/igb_uio/igb_uio.c
>>>>> @@ -185,10 +185,6 @@ igbuio_pci_irqcontrol(struct uio_info *info, s32 irq_state)
>>>>>  	return 0;
>>>>>  }
>>>>>  
>>>>> -/**
>>>>> - * This is interrupt handler which will check if the interrupt is for the right device.
>>>>> - * If yes, disable it here and will be enable later.
>>>>> - */
>>>>>  static irqreturn_t
>>>>>  igbuio_pci_irqhandler(int irq, void *dev_id)
>>>>>  {  
>>>>
>>>> The comment is partially correct; if you look at the legacy case.
>>>>
>>>> Maybe better to move the comment to pci_check_and_mask_intx in compat.h?
>>>> I see there is another incorrect comment there.
>>>>  
>>>
>>> As I tried to understand pci_check_and_mask_intx behavior, I noticed that it 
>>> was introduced by commit 399a3f0d ("igb_uio: fix IRQ mode handling"), and it
>>> was derived from igbuio_set_interrupt_mask, however there were two parameters
>>> in igbuio_set_interrupt_mask as below:
>>>
>>> 	static int
>>> 	igbuio_set_interrupt_mask(struct rte_uio_pci_dev *udev, int32_t state)
>>>
>>> while the pci_check_and_mask_intx only has 1 parameter,
>>>
>>> 	static bool pci_check_and_mask_intx(struct pci_dev *pdev)
>>>
>>> but in its function body it still use the "state" accorrding to commit 399a3f0d
>>>
>>> +static bool pci_check_and_mask_intx(struct pci_dev *pdev)
>>>  {
>>> -       /* Some function names changes between 3.2.0 and 3.3.0... */
>>> -#if LINUX_VERSION_CODE < KERNEL_VERSION(3, 3, 0)
>>> -       pci_unblock_user_cfg_access(pdev);
>>> -#else
>>> -       pci_cfg_access_unlock(pdev);
>>> -#endif
>>> +       bool pending;
>>> +       uint32_t status;
>>> +
>>> +       pci_block_user_cfg_access(dev);
>>> +       pci_read_config_dword(pdev, PCI_COMMAND, &status);
>>> +
>>> +       /* interrupt is not ours, goes to out */
>>> +       pending = (((status >> 16) & PCI_STATUS_INTERRUPT) != 0);
>>> +       if (pending) {
>>> +               uint16_t old, new;
>>> +
>>> +               old = status;
>>> +               if (state != 0) <=========================== state still in use
>>> +                       new = old & (~PCI_COMMAND_INTX_DISABLE);
>>> +               else
>>> +                       new = old | PCI_COMMAND_INTX_DISABLE;
>>> +
>>> +               if (old != new)
>>> +                       pci_write_config_word(pdev, PCI_COMMAND, new);
>>> +       }
>>> +       pci_unblock_user_cfg_access(dev);
>>> +
>>> +       return pending;
>>>
>>> and later this was fixed as a typo in commit 5b2f8137 ("igb_uio: fix typos for 
>>> kernel older than 3.3") which seems not correct.
>>>
>>>                 old = status;
>>> -               if (state != 0)
>>> +               if (status != 0)
>>>
>>> I feel like that pci_check_and_mask_intx still needs two parameters from code
>>> point of view, please correct me if I was wrong or miss anything, or you think 
>>> it's sensible, I'll cook a patch for it. 
>>>
>>> Thanks,
>>> Xiaolong
>>
>> This code is copy of upstream kernel function, you should look at that.
> 
> Thanks, just checked the kernel version of pci_check_and_mask_intx, and it is a 
> wrap of pci_check_and_set_intx_mask(pdev, true).
> 
> 
> 	bool pci_check_and_mask_intx(struct pci_dev *dev)
> 	{
> 		return pci_check_and_set_intx_mask(dev, true);
> 	}
> 
> 
> 	static bool pci_check_and_set_intx_mask(struct pci_dev *dev, bool mask)
> 	{
> 		struct pci_bus *bus = dev->bus;
> 		...
> 		origcmd = cmd_status_dword;
> 		newcmd = origcmd & ~PCI_COMMAND_INTX_DISABLE;
> 		if (mask)
> 			newcmd |= PCI_COMMAND_INTX_DISABLE;
> 		if (newcmd != origcmd)
> 			bus->ops->write(bus, dev->devfn, PCI_COMMAND, 2, newcmd);
> 
> 	done:
> 		raw_spin_unlock_irqrestore(&pci_lock, flags);
> 
> 		return mask_updated;
> 	}
> 
> if mask is set to ture, PCI_COMMAND_INTX_DISABLE will be set in newcmd.
> 
> And current dpdk version doesn't align with it:
> 
> 	static bool pci_check_and_mask_intx(struct pci_dev *pdev)
> 	{
> 		bool pending;
> 		uint32_t status;
> 
> 		pci_block_user_cfg_access(pdev);
> 		pci_read_config_dword(pdev, PCI_COMMAND, &status);
> 
> 		/* interrupt is not ours, goes to out */
> 		pending = (((status >> 16) & PCI_STATUS_INTERRUPT) != 0);
> 		if (pending) {
> 			uint16_t old, new;
> 
> 			old = status;
> 			if (status != 0)
> 				new = old & (~PCI_COMMAND_INTX_DISABLE);
> 			else
> 				new = old | PCI_COMMAND_INTX_DISABLE;
> 
> 			if (old != new)
> 				pci_write_config_word(pdev, PCI_COMMAND, new);
> 		}
> 		pci_unblock_user_cfg_access(pdev);
> 
> 		return pending;
> 	}
> 
> In this code, if the interrupt is triggered by this device, both pending and
> status will not be 0, so the PCI_COMMAND_INTX_DISABLE will be cleared for new.
> 
> To align the behavior with upstream kernel, we may need change like:
> 
> diff --git a/kernel/linux/igb_uio/compat.h b/kernel/linux/igb_uio/compat.h
> index 8dbb896ae..a78c97f91 100644
> --- a/kernel/linux/igb_uio/compat.h
> +++ b/kernel/linux/igb_uio/compat.h
> @@ -107,10 +107,7 @@ static bool pci_check_and_mask_intx(struct pci_dev *pdev)
>                 uint16_t old, new;
> 
>                 old = status;
> -               if (status != 0)
> -                       new = old & (~PCI_COMMAND_INTX_DISABLE);
> -               else
> -                       new = old | PCI_COMMAND_INTX_DISABLE;
> +               new = old | PCI_COMMAND_INTX_DISABLE;
> 
>                 if (old != new)
>                         pci_write_config_word(pdev, PCI_COMMAND, new);
> 
> 
> Please correct me if I was wrong.

Hi Xiaolong,

Your analysis looks correct, thanks for history provided, it seems renaming
"state" to "status" was not correct [1].

The intention of the 'pci_check_and_mask_intx()' is mask the interrupt if it is
from this device, so your suggestion looks correct to me.

Only concern is how we can test the change, most probably existing code is also
failing but able to find its way since it is not tested fully. To be sure we are
also not missing anything, I think better to test the code before getting it.
Are you aware of any way to test updated code?


And related to the initial patch, removing the comment, I think the comment is
informative within the context of 'igbuio_pci_irqhandler()', so I suggest moving
if from begging of the function into it just above legacy interrupt change.

[1]
5b2f8137 ("igb_uio: fix typos for kernel older than 3.3")


Thanks,
ferruh

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-dev] [PATCH v2] igb_uio: remove out-of-date comment
  2019-01-29 12:49           ` Ferruh Yigit
@ 2019-01-31  8:13             ` Ye Xiaolong
  0 siblings, 0 replies; 12+ messages in thread
From: Ye Xiaolong @ 2019-01-31  8:13 UTC (permalink / raw)
  To: Ferruh Yigit; +Cc: Stephen Hemminger, dev

Hi, Ferruh

On 01/29, Ferruh Yigit wrote:
[snip]
>
>Hi Xiaolong,
>
>Your analysis looks correct, thanks for history provided, it seems renaming
>"state" to "status" was not correct [1].
>
>The intention of the 'pci_check_and_mask_intx()' is mask the interrupt if it is
>from this device, so your suggestion looks correct to me.
>
>Only concern is how we can test the change, most probably existing code is also
>failing but able to find its way since it is not tested fully. To be sure we are
>also not missing anything, I think better to test the code before getting it.
>Are you aware of any way to test updated code?
>

Yes, I am also thinking about how to verify the change, will update once I find
a proper method.

>
>And related to the initial patch, removing the comment, I think the comment is
>informative within the context of 'igbuio_pci_irqhandler()', so I suggest moving
>if from begging of the function into it just above legacy interrupt change.
>

Sure, will do.

Thanks,
Xiaolong

>[1]
>5b2f8137 ("igb_uio: fix typos for kernel older than 3.3")
>
>
>Thanks,
>ferruh

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2019-01-31  8:14 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-15 15:46 [dpdk-dev] [PATCH] igb_uio: remove out-of-date comment Xiaolong Ye
2019-01-15 16:08 ` Burakov, Anatoly
2019-01-15 23:54   ` Ye Xiaolong
2019-01-16  0:34 ` [dpdk-dev] [PATCH v2] " Xiaolong Ye
2019-01-16  1:38   ` Stephen Hemminger
2019-01-16  5:17     ` Ye Xiaolong
2019-01-16  7:48     ` Ye Xiaolong
2019-01-16 18:55       ` Stephen Hemminger
2019-01-17  2:13         ` Ye Xiaolong
2019-01-22  1:52           ` Ye Xiaolong
2019-01-29 12:49           ` Ferruh Yigit
2019-01-31  8:13             ` Ye Xiaolong

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).