[dpdk-dev] [PATCH] igb_uio: remove out-of-date comment

[dpdk-dev] [PATCH] igb_uio: remove out-of-date comment

[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

Re: [dpdk-dev] [PATCH] igb_uio: remove out-of-date comment

[Burakov, Anatoly]

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

Re: [dpdk-dev] [PATCH] igb_uio: remove out-of-date comment

[Ye Xiaolong]

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

[dpdk-dev] [PATCH v2] igb_uio: remove out-of-date comment

[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

Re: [dpdk-dev] [PATCH v2] igb_uio: remove out-of-date comment

[Stephen Hemminger]

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.

Re: [dpdk-dev] [PATCH v2] igb_uio: remove out-of-date comment

[Ye Xiaolong]

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

Re: [dpdk-dev] [PATCH v2] igb_uio: remove out-of-date comment

[Ye Xiaolong]

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

Re: [dpdk-dev] [PATCH v2] igb_uio: remove out-of-date comment

[Stephen Hemminger]

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.

Re: [dpdk-dev] [PATCH v2] igb_uio: remove out-of-date comment

[Ye Xiaolong]

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

Re: [dpdk-dev] [PATCH v2] igb_uio: remove out-of-date comment

[Ye Xiaolong]

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

Re: [dpdk-dev] [PATCH v2] igb_uio: remove out-of-date comment

[Ferruh Yigit]

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

Re: [dpdk-dev] [PATCH v2] igb_uio: remove out-of-date comment

[Ye Xiaolong]

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