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

[Ferruh Yigit <ferruh.yigit@intel.com>]

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