From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by dpdk.org (Postfix) with ESMTP id EBC63A3 for ; Tue, 29 Jan 2019 13:49:43 +0100 (CET) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by fmsmga105.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 29 Jan 2019 04:49:43 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.56,537,1539673200"; d="scan'208";a="315810917" Received: from fyigit-mobl.ger.corp.intel.com (HELO [10.237.221.22]) ([10.237.221.22]) by fmsmga005.fm.intel.com with ESMTP; 29 Jan 2019 04:49:41 -0800 To: Ye Xiaolong , Stephen Hemminger Cc: dev@dpdk.org References: <20190115154626.34516-1-xiaolong.ye@intel.com> <20190116003452.37403-1-xiaolong.ye@intel.com> <20190115173822.25a4a979@shemminger-XPS-13-9360> <20190116074836.GB39571@intel.com> <20190116105521.0254cdb2@shemminger-XPS-13-9360> <20190117021354.GA47093@intel.com> From: Ferruh Yigit Openpgp: preference=signencrypt Autocrypt: addr=ferruh.yigit@intel.com; prefer-encrypt=mutual; keydata= mQINBFXZCFABEADCujshBOAaqPZpwShdkzkyGpJ15lmxiSr3jVMqOtQS/sB3FYLT0/d3+bvy qbL9YnlbPyRvZfnP3pXiKwkRoR1RJwEo2BOf6hxdzTmLRtGtwWzI9MwrUPj6n/ldiD58VAGQ +iR1I/z9UBUN/ZMksElA2D7Jgg7vZ78iKwNnd+vLBD6I61kVrZ45Vjo3r+pPOByUBXOUlxp9 GWEKKIrJ4eogqkVNSixN16VYK7xR+5OUkBYUO+sE6etSxCr7BahMPKxH+XPlZZjKrxciaWQb +dElz3Ab4Opl+ZT/bK2huX+W+NJBEBVzjTkhjSTjcyRdxvS1gwWRuXqAml/sh+KQjPV1PPHF YK5LcqLkle+OKTCa82OvUb7cr+ALxATIZXQkgmn+zFT8UzSS3aiBBohg3BtbTIWy51jNlYdy ezUZ4UxKSsFuUTPt+JjHQBvF7WKbmNGS3fCid5Iag4tWOfZoqiCNzxApkVugltxoc6rG2TyX CmI2rP0mQ0GOsGXA3+3c1MCdQFzdIn/5tLBZyKy4F54UFo35eOX8/g7OaE+xrgY/4bZjpxC1 1pd66AAtKb3aNXpHvIfkVV6NYloo52H+FUE5ZDPNCGD0/btFGPWmWRmkPybzColTy7fmPaGz cBcEEqHK4T0aY4UJmE7Ylvg255Kz7s6wGZe6IR3N0cKNv++O7QARAQABtCVGZXJydWggWWln aXQgPGZlcnJ1aC55aWdpdEBpbnRlbC5jb20+iQJVBBMBAgA/AhsDBgsJCAcDAgYVCAIJCgsE FgIDAQIeAQIXgBYhBNI2U4dCLsKE45mBx/kz60PfE2EfBQJbughWBQkHwjOGAAoJEPkz60Pf E2Eft84QAIbKWqhgqRfoiw/BbXbA1+qm2o4UgkCRQ0yJgt9QsnbpOmPKydHH0ixCliNz1J8e mRXCkMini1bTpnzp7spOjQGLeAFkNFz6BMq8YF2mVWbGEDE9WgnAxZdi0eLY7ZQnHbE6AxKL SXmpe9INb6z3ztseFt7mqje/W/6DWYIMnH3Yz9KzxujFWDcq8UCAvPkxVQXLTMpauhFgYeEx Nub5HbvhxTfUkapLwRQsSd/HbywzqZ3s/bbYMjj5JO3tgMiM9g9HOjv1G2f1dQjHi5YQiTZl 1eIIqQ3pTic6ROaiZqNmQFXPsoOOFfXF8nN2zg8kl/sSdoXWHhama5hbwwtl1vdaygQYlmdK H2ueiFh/UvT3WG3waNv2eZiEbHV8Rk52Xyn2w1G90lV0fYC6Ket1Xjoch7kjwbx793Kz/RfQ rmBY8/S4DTGn3oq3dMdQY+b6+7VMUeLMMh2CXYO9ErkOq+qNTD1IY+cBAkXnaDbQfz0zbste ZGWH74FAZ9nCpDOqbRTrBL42aMGhfOWEyeA1x7+hl6JZfabBWAuf4nnCXuorKHzBXTrf7u7p fXsKQClWRW77PF1VmzrtKNVSytQAmlCWApQIw20AarFipXmVdIjHmJPU611WoyxZPb4JTOxx 5cv9B+nr/RIB+v5dcStyHCCwO1be7nBDdCgd4F6kTQPLuQINBFfWTL4BEACnNA29e8TarUsB L5n6eLZHXcFvVwNLVlirWOClHXf44o2KnN3ww+eBEmKVfEFo9MSuGDNHS8Zw1NiGMYxLIUgd U6gGrVVs/VrQWL82pbMk6jCj98N+BXIri+6K1z+AImz7ax7iF1kDgRAnFWU0znWWBgM2mM8Y gDjcxfXk4sCKnvf6Gjo08Ey5zmqx7dekAKU2EEp8Q1EJY3jbymLdZWRP4AFFMTS1rGMk0/tt v71NBg1GobCcbNfn9chK/jhqxYhAJqq86RdJQkt3/9x1U1Oq0vXCt4JVVHmkxePtUiuWTTt+ aYlUAsKYZsWvncExvw77x2ArYDmaK0yfjh37wp0lY7DOJHFxoyT8tyWZlLci/VMRG2Ja33xj 0CN4C1yBg+QDeV3QFxQo42iA/ykdXPUR3ezmsND3XKvVLTC4DNb3V/EZQ7jBj64+bEK0VW4G B31VP00ApNQvSoczsIOAKdk97RNbpmPw6q10ILIB+9T1xbnFYzshzGF17oC0/GENIHATx8vZ masOZoDiOZQpeneLgnFE9JfzhLTxv6wNZcc/HLXRQVTkDsQr8ERtkAoHCf1E5+b5Yr7pfnE4 YuhET746o25S53ELUYPIs49qoJsEJL34/oexMfPGyPIlrbufiNyty5jc/1MRwUlhJlJ5IOHy ZUa+6CLR7GdImusFkPJUJwARAQABiQI8BBgBAgAmAhsMFiEE0jZTh0IuwoTjmYHH+TPrQ98T YR8FAlu6CHAFCQXE7zIACgkQ+TPrQ98TYR9nXxAAqNBgkYNyGuWUuy0GwDQCbu3iiMyH1+D7 llafPcK4NYy1Z4AYuVwC9nmLaoj+ozdqS3ncRo57ncRsKEJC46nDJJZYZ5LSJVn63Y3NBF86 lxQAgjj2oyZEwaLKtKbAFsXL43jv1pUGgSvWwYtDwHITXXFQto9rZEuUDRFSx4sg9OR+Q6/6 LY+nQQ3OdHlBkflzYMPcWgDcvcTAO6yasLEUf7UcYoSWTyMYjLB4QuNlXzTswzGVMssJF/vo V8lD1eqqaSUWG3STF6GVLQOr1NLvN5+kUBiEStHFxBpgSCvYY9sNV8FS6N24CAWMBl+10W+D 2h1yiiP5dOdPcBDYKsgqDD91/sP0WdyMJkwdQJtD49f9f+lYloxHnSAxMleOpyscg1pldw+i mPaUY1bmIknLhhkqfMmjywQOXpac5LRMibAAYkcB8v7y3kwELnt8mhqqZy6LUsqcWygNbH/W K3GGt5tRpeIXeJ25x8gg5EBQ0Jnvp/IbBYQfPLtXH0Myq2QuAhk/1q2yEIbVjS+7iowEZNyE 56K63WBJxsJPB2mvmLgn98GqB4G6GufP1ndS0XDti/2K0o8rep9xoY/JDGi0n0L0tk9BHyoP Y7kaEpu7UyY3nVdRLe5H1/MnFG8hdJ97WqnPS0buYZlrbTV0nRFL/NI2VABl18vEEXvNQiO+ vM8= Message-ID: Date: Tue, 29 Jan 2019 12:49:41 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101 Thunderbird/60.4.0 MIME-Version: 1.0 In-Reply-To: <20190117021354.GA47093@intel.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH v2] igb_uio: remove out-of-date comment X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 29 Jan 2019 12:49:44 -0000 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 wrote: >> >>> Hi, Stephen >>> >>> On 01/15, Stephen Hemminger wrote: >>>> On Wed, 16 Jan 2019 08:34:52 +0800 >>>> 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 >>>>> --- >>>>> 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