| Message ID | 20210826221916.127243-1-pwaskiewicz@jumptrading.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/1] i40e: Avoid double IRQ free on error path in probe() | expand |
On Thu, 2021-08-26 at 17:19 -0500, PJ Waskiewicz wrote: > This fixes an error path condition when probe() fails due to the > default VSI not being available or online yet in the firmware. If > that happens, the previous teardown path would clear the interrupt > scheme, which also freed the IRQs with the OS. Then the error path > for the switch setup (pre-VSI) would attempt to free the OS IRQs > as well. Hi PJ, These comments are from the i40e team. Yes in case we fail and go to err_vsis label in i40e_probe() we will call i40e_reset_interrupt_capability twice but this is not a problem. This is because pci_disable_msi/pci_disable_msix will be called only if appropriate flags are set on PF and if this function is called ones it will clear those flags. So even if we call i40e_reset_interrupt_capability twice we will not disable msi vectors twice. The issue here is different however. It is failing in free_irq because we are trying to free already free vector. This is because setup of misc irq vectors in i40e_probe is done after i40e_setup_pf_switch. If i40e_setup_pf_switch fails then we will jump to err_vsis and call i40e_clear_interrupt_scheme which will try to free those misc irq vectors which were not yet allocated. We should have the proper fix for this ready soon.
On Mon, Aug 30, 2021 at 08:52:41PM +0000, Nguyen, Anthony L wrote: > On Thu, 2021-08-26 at 17:19 -0500, PJ Waskiewicz wrote: > > This fixes an error path condition when probe() fails due to the > > default VSI not being available or online yet in the firmware. If > > that happens, the previous teardown path would clear the interrupt > > scheme, which also freed the IRQs with the OS. Then the error path > > for the switch setup (pre-VSI) would attempt to free the OS IRQs > > as well. > > Hi PJ, Hi Tony, > > These comments are from the i40e team. > > Yes in case we fail and go to err_vsis label in i40e_probe() we will > call i40e_reset_interrupt_capability twice but this is not a problem. > This is because pci_disable_msi/pci_disable_msix will be called only if > appropriate flags are set on PF and if this function is called ones it > will clear those flags. So even if we call > i40e_reset_interrupt_capability twice we will not disable msi vectors > twice. > > The issue here is different however. It is failing in free_irq because > we are trying to free already free vector. This is because setup of > misc irq vectors in i40e_probe is done after i40e_setup_pf_switch. If > i40e_setup_pf_switch fails then we will jump to err_vsis and call > i40e_clear_interrupt_scheme which will try to free those misc irq > vectors which were not yet allocated. We should have the proper fix for > this ready soon. Yes, I'm aware of what's happening here and why it's failing. Sadly, I am pretty sure I wrote this code back in like 2011 or 2012, and being an error path, it hasn't really been tested. I don't really care how this gets fixed to be honest. We hit this in production when our LOM, for whatever reason, failed to initialize the internal switch on host boot. We escalated to our distro vendor, they did escalate to Intel, and it wasn't really prioritized. So I sent a patch that does fix the issue. If the team wants to respin this somehow, go ahead. But this does fix the immediate issue that when bailing out in probe() due to the main VSI not being online for whatever reason, the driver blindly attempts to clean up the misc MSI-X vector twice. This change fixes that behavior. I'd like this to not languish waiting for a different fix, since I'd like to point our distro vendor to this (or another) patch to cherry-pick, so we can get this into production. Otherwise our platform rollout hitting this problem is going to be quite bumpy, which is very much not ideal. Cheers, -PJ ________________________________ Note: This email is for the confidential use of the named addressee(s) only and may contain proprietary, confidential, or privileged information and/or personal data. If you are not the intended recipient, you are hereby notified that any review, dissemination, or copying of this email is strictly prohibited, and requested to notify the sender immediately and destroy this email and any attachments. Email transmission cannot be guaranteed to be secure or error-free. The Company, therefore, does not make any guarantees as to the completeness or accuracy of this email or any attachments. This email is for informational purposes only and does not constitute a recommendation, offer, request, or solicitation of any kind to buy, sell, subscribe, redeem, or perform any type of transaction of a financial product. Personal data, as defined by applicable data protection and privacy laws, contained in this email may be processed by the Company, and any of its affiliated or related companies, for legal, compliance, and/or business-related purposes. You may have rights regarding your personal data; for information on exercising these rights or the Company’s treatment of personal data, please email datarequests@jumptrading.com.
Hi Tony, > -----Original Message----- > From: PJ Waskiewicz <pwaskiewicz@jumptrading.com> > Sent: Tuesday, August 31, 2021 1:59 PM > To: Nguyen, Anthony L <anthony.l.nguyen@intel.com> > Cc: intel-wired-lan@lists.osuosl.org; pjwaskiewicz@gmail.com; Loktionov, > Aleksandr <aleksandr.loktionov@intel.com>; Fijalkowski, Maciej > <maciej.fijalkowski@intel.com>; Dziedziuch, SylwesterX > <sylwesterx.dziedziuch@intel.com>; davem@davemloft.net; Brandeburg, > Jesse <jesse.brandeburg@intel.com>; netdev@vger.kernel.org; PJ > Waskiewicz <pwaskiewicz@jumptrading.com> > Subject: Re: [PATCH 1/1] i40e: Avoid double IRQ free on error path in probe() > > On Mon, Aug 30, 2021 at 08:52:41PM +0000, Nguyen, Anthony L wrote: > > On Thu, 2021-08-26 at 17:19 -0500, PJ Waskiewicz wrote: > > > This fixes an error path condition when probe() fails due to the > > > default VSI not being available or online yet in the firmware. If > > > that happens, the previous teardown path would clear the interrupt > > > scheme, which also freed the IRQs with the OS. Then the error path > > > for the switch setup (pre-VSI) would attempt to free the OS IRQs as > > > well. > > > > Hi PJ, > > Hi Tony, > > > > > These comments are from the i40e team. > > > > Yes in case we fail and go to err_vsis label in i40e_probe() we will > > call i40e_reset_interrupt_capability twice but this is not a problem. > > This is because pci_disable_msi/pci_disable_msix will be called only > > if appropriate flags are set on PF and if this function is called ones > > it will clear those flags. So even if we call > > i40e_reset_interrupt_capability twice we will not disable msi vectors > > twice. > > > > The issue here is different however. It is failing in free_irq because > > we are trying to free already free vector. This is because setup of > > misc irq vectors in i40e_probe is done after i40e_setup_pf_switch. If > > i40e_setup_pf_switch fails then we will jump to err_vsis and call > > i40e_clear_interrupt_scheme which will try to free those misc irq > > vectors which were not yet allocated. We should have the proper fix > > for this ready soon. > > Yes, I'm aware of what's happening here and why it's failing. Sadly, I am > pretty sure I wrote this code back in like 2011 or 2012, and being an error > path, it hasn't really been tested. > > I don't really care how this gets fixed to be honest. We hit this in production > when our LOM, for whatever reason, failed to initialize the internal switch on > host boot. We escalated to our distro vendor, they did escalate to Intel, and > it wasn't really prioritized. So I sent a patch that does fix the issue. > > If the team wants to respin this somehow, go ahead. But this does fix the > immediate issue that when bailing out in probe() due to the main VSI not > being online for whatever reason, the driver blindly attempts to clean up the > misc MSI-X vector twice. This change fixes that behavior. I'd like this to not > languish waiting for a different fix, since I'd like to point our distro vendor to > this (or another) patch to cherry-pick, so we can get this into production. > Otherwise our platform rollout hitting this problem is going to be quite > bumpy, which is very much not ideal. It's been 2 weeks since I replied. Any update on this? Maciej had already reviewed the patch, so hoping we can just move along with it, or get something else out soon? I'd really like this to not just fall into a void waiting for a different patch when this fixes the issue. -PJ ________________________________ Note: This email is for the confidential use of the named addressee(s) only and may contain proprietary, confidential, or privileged information and/or personal data. If you are not the intended recipient, you are hereby notified that any review, dissemination, or copying of this email is strictly prohibited, and requested to notify the sender immediately and destroy this email and any attachments. Email transmission cannot be guaranteed to be secure or error-free. The Company, therefore, does not make any guarantees as to the completeness or accuracy of this email or any attachments. This email is for informational purposes only and does not constitute a recommendation, offer, request, or solicitation of any kind to buy, sell, subscribe, redeem, or perform any type of transaction of a financial product. Personal data, as defined by applicable data protection and privacy laws, contained in this email may be processed by the Company, and any of its affiliated or related companies, for legal, compliance, and/or business-related purposes. You may have rights regarding your personal data; for information on exercising these rights or the Company’s treatment of personal data, please email datarequests@jumptrading.com.
On Mon, 2021-09-13 at 19:37 +0000, PJ Waskiewicz wrote: > Hi Tony, > > > -----Original Message----- > > From: PJ Waskiewicz <pwaskiewicz@jumptrading.com> > > Sent: Tuesday, August 31, 2021 1:59 PM > > To: Nguyen, Anthony L <anthony.l.nguyen@intel.com> > > Cc: intel-wired-lan@lists.osuosl.org; pjwaskiewicz@gmail.com; > > Loktionov, > > Aleksandr <aleksandr.loktionov@intel.com>; Fijalkowski, Maciej > > <maciej.fijalkowski@intel.com>; Dziedziuch, SylwesterX > > <sylwesterx.dziedziuch@intel.com>; davem@davemloft.net; Brandeburg, > > Jesse <jesse.brandeburg@intel.com>; netdev@vger.kernel.org; PJ > > Waskiewicz <pwaskiewicz@jumptrading.com> > > Subject: Re: [PATCH 1/1] i40e: Avoid double IRQ free on error path > > in probe() > > > > On Mon, Aug 30, 2021 at 08:52:41PM +0000, Nguyen, Anthony L wrote: > > > On Thu, 2021-08-26 at 17:19 -0500, PJ Waskiewicz wrote: > > > > This fixes an error path condition when probe() fails due to > > > > the > > > > default VSI not being available or online yet in the firmware. > > > > If > > > > that happens, the previous teardown path would clear the > > > > interrupt > > > > scheme, which also freed the IRQs with the OS. Then the error > > > > path > > > > for the switch setup (pre-VSI) would attempt to free the OS > > > > IRQs as > > > > well. > > > > > > Hi PJ, > > > > Hi Tony, > > > > > These comments are from the i40e team. > > > > > > Yes in case we fail and go to err_vsis label in i40e_probe() we > > > will > > > call i40e_reset_interrupt_capability twice but this is not a > > > problem. > > > This is because pci_disable_msi/pci_disable_msix will be called > > > only > > > if appropriate flags are set on PF and if this function is called > > > ones > > > it will clear those flags. So even if we call > > > i40e_reset_interrupt_capability twice we will not disable msi > > > vectors > > > twice. > > > > > > The issue here is different however. It is failing in free_irq > > > because > > > we are trying to free already free vector. This is because setup > > > of > > > misc irq vectors in i40e_probe is done after > > > i40e_setup_pf_switch. If > > > i40e_setup_pf_switch fails then we will jump to err_vsis and call > > > i40e_clear_interrupt_scheme which will try to free those misc irq > > > vectors which were not yet allocated. We should have the proper > > > fix > > > for this ready soon. > > > > Yes, I'm aware of what's happening here and why it's failing. > > Sadly, I am > > pretty sure I wrote this code back in like 2011 or 2012, and being > > an error > > path, it hasn't really been tested. > > > > I don't really care how this gets fixed to be honest. We hit this > > in production > > when our LOM, for whatever reason, failed to initialize the > > internal switch on > > host boot. We escalated to our distro vendor, they did escalate to > > Intel, and > > it wasn't really prioritized. So I sent a patch that does fix the > > issue. > > > > If the team wants to respin this somehow, go ahead. But this does > > fix the > > immediate issue that when bailing out in probe() due to the main > > VSI not > > being online for whatever reason, the driver blindly attempts to > > clean up the > > misc MSI-X vector twice. This change fixes that behavior. I'd like > > this to not > > languish waiting for a different fix, since I'd like to point our > > distro vendor to > > this (or another) patch to cherry-pick, so we can get this into > > production. > > Otherwise our platform rollout hitting this problem is going to be > > quite > > bumpy, which is very much not ideal. > > It's been 2 weeks since I replied. Any update on this? Maciej had > already reviewed the patch, so hoping we can just move along with it, > or get something else out soon? > > I'd really like this to not just fall into a void waiting for a > different patch when this fixes the issue. Hi PJ, I haven't seen a recent update on this. I'm asking for an update. Otherwise, Alex and Sylwester are on this thread; perhaps they have some info. Thanks, Tony > -PJ > > ________________________________ > > Note: This email is for the confidential use of the named > addressee(s) only and may contain proprietary, confidential, or > privileged information and/or personal data. If you are not the > intended recipient, you are hereby notified that any review, > dissemination, or copying of this email is strictly prohibited, and > requested to notify the sender immediately and destroy this email and > any attachments. Email transmission cannot be guaranteed to be secure > or error-free. The Company, therefore, does not make any guarantees > as to the completeness or accuracy of this email or any attachments. > This email is for informational purposes only and does not constitute > a recommendation, offer, request, or solicitation of any kind to buy, > sell, subscribe, redeem, or perform any type of transaction of a > financial product. Personal data, as defined by applicable data > protection and privacy laws, contained in this email may be processed > by the Company, and any of its affiliated or related companies, for > legal, compliance, and/or business-related purposes. You may have > rights regarding your personal data; for information on exercising > these rights or the Company’s treatment of personal data, please > email datarequests@jumptrading.com.
> On Mon, 2021-09-13 at 19:37 +0000, PJ Waskiewicz wrote: > > Hi Tony, > > > > > -----Original Message----- > > > From: PJ Waskiewicz <pwaskiewicz@jumptrading.com> > > > Sent: Tuesday, August 31, 2021 1:59 PM > > > To: Nguyen, Anthony L <anthony.l.nguyen@intel.com> > > > Cc: intel-wired-lan@lists.osuosl.org; pjwaskiewicz@gmail.com; > > > Loktionov, Aleksandr <aleksandr.loktionov@intel.com>; Fijalkowski, > > > Maciej <maciej.fijalkowski@intel.com>; Dziedziuch, SylwesterX > > > <sylwesterx.dziedziuch@intel.com>; davem@davemloft.net; Brandeburg, > > > Jesse <jesse.brandeburg@intel.com>; netdev@vger.kernel.org; PJ > > > Waskiewicz <pwaskiewicz@jumptrading.com> > > > Subject: Re: [PATCH 1/1] i40e: Avoid double IRQ free on error path > > > in probe() > > > > > > On Mon, Aug 30, 2021 at 08:52:41PM +0000, Nguyen, Anthony L wrote: > > > > On Thu, 2021-08-26 at 17:19 -0500, PJ Waskiewicz wrote: > > > > > This fixes an error path condition when probe() fails due to the > > > > > default VSI not being available or online yet in the firmware. > > > > > If > > > > > that happens, the previous teardown path would clear the > > > > > interrupt scheme, which also freed the IRQs with the OS. Then > > > > > the error path for the switch setup (pre-VSI) would attempt to > > > > > free the OS IRQs as well. > > > > > > > > Hi PJ, > > > > > > Hi Tony, > > > > > > > These comments are from the i40e team. > > > > > > > > Yes in case we fail and go to err_vsis label in i40e_probe() we > > > > will call i40e_reset_interrupt_capability twice but this is not a > > > > problem. > > > > This is because pci_disable_msi/pci_disable_msix will be called > > > > only if appropriate flags are set on PF and if this function is > > > > called ones it will clear those flags. So even if we call > > > > i40e_reset_interrupt_capability twice we will not disable msi > > > > vectors twice. > > > > > > > > The issue here is different however. It is failing in free_irq > > > > because we are trying to free already free vector. This is because > > > > setup of misc irq vectors in i40e_probe is done after > > > > i40e_setup_pf_switch. If i40e_setup_pf_switch fails then we will > > > > jump to err_vsis and call i40e_clear_interrupt_scheme which will > > > > try to free those misc irq vectors which were not yet allocated. > > > > We should have the proper fix for this ready soon. > > > > > > Yes, I'm aware of what's happening here and why it's failing. > > > Sadly, I am > > > pretty sure I wrote this code back in like 2011 or 2012, and being > > > an error path, it hasn't really been tested. > > > > > > I don't really care how this gets fixed to be honest. We hit this in > > > production when our LOM, for whatever reason, failed to initialize > > > the internal switch on host boot. We escalated to our distro vendor, > > > they did escalate to Intel, and it wasn't really prioritized. So I > > > sent a patch that does fix the issue. > > > > > > If the team wants to respin this somehow, go ahead. But this does > > > fix the immediate issue that when bailing out in probe() due to the > > > main VSI not being online for whatever reason, the driver blindly > > > attempts to clean up the misc MSI-X vector twice. This change fixes > > > that behavior. I'd like this to not languish waiting for a different > > > fix, since I'd like to point our distro vendor to this (or another) > > > patch to cherry-pick, so we can get this into production. > > > Otherwise our platform rollout hitting this problem is going to be > > > quite bumpy, which is very much not ideal. > > > > It's been 2 weeks since I replied. Any update on this? Maciej had > > already reviewed the patch, so hoping we can just move along with it, > > or get something else out soon? > > > > I'd really like this to not just fall into a void waiting for a > > different patch when this fixes the issue. > > Hi PJ, > > I haven't seen a recent update on this. I'm asking for an update. > Otherwise, Alex and Sylwester are on this thread; perhaps they have some > info. > > Thanks, > Tony > Hello, The driver does not blindly try to free MSI-X vector twice here. This is guarded by I40E_FLAG_MSIX_ENABLED and I40E_FLAG_MSI_ENABLED flags. Only if those flags are set we will try to free MSI/MSI-X vectors in i40e_reset_interrupt_capability(). Additionally i40e_reset_interrupt_capability() clears those flags every time it is called so even if we call it twice in a row the driver will not free the vectors twice. I really can't see how this patch is fixing anything as the issue here is not with MSI vectors but with misc IRQ vectors. We have a proper patch for this ready in OOT and we will upstream it soon. The problem here is that in i40e_clear_interrupt_scheme() driver calls i40e_free_misc_vector() but in case VSI setup fails misc vector is not allocated yet and we get a call trace in free_irq that we are trying to free IRQ that has not been allocated yet. Regards Sylwester Dziedziuch > > -PJ > > > > ________________________________ > > > > Note: This email is for the confidential use of the named > > addressee(s) only and may contain proprietary, confidential, or > > privileged information and/or personal data. If you are not the > > intended recipient, you are hereby notified that any review, > > dissemination, or copying of this email is strictly prohibited, and > > requested to notify the sender immediately and destroy this email and > > any attachments. Email transmission cannot be guaranteed to be secure > > or error-free. The Company, therefore, does not make any guarantees as > > to the completeness or accuracy of this email or any attachments. > > This email is for informational purposes only and does not constitute > > a recommendation, offer, request, or solicitation of any kind to buy, > > sell, subscribe, redeem, or perform any type of transaction of a > > financial product. Personal data, as defined by applicable data > > protection and privacy laws, contained in this email may be processed > > by the Company, and any of its affiliated or related companies, for > > legal, compliance, and/or business-related purposes. You may have > > rights regarding your personal data; for information on exercising > > these rights or the Company’s treatment of personal data, please email > > datarequests@jumptrading.com.
Hi Sylwester, > -----Original Message----- > From: Dziedziuch, SylwesterX <sylwesterx.dziedziuch@intel.com> > Sent: Tuesday, September 14, 2021 1:24 AM > To: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; PJ Waskiewicz > <pwaskiewicz@jumptrading.com> > Cc: davem@davemloft.net; pjwaskiewicz@gmail.com; Fijalkowski, Maciej > <maciej.fijalkowski@intel.com>; Loktionov, Aleksandr > <aleksandr.loktionov@intel.com>; netdev@vger.kernel.org; Brandeburg, > Jesse <jesse.brandeburg@intel.com>; intel-wired-lan@lists.osuosl.org > Subject: RE: [PATCH 1/1] i40e: Avoid double IRQ free on error path in probe() > [snip] > > > It's been 2 weeks since I replied. Any update on this? Maciej had > > > already reviewed the patch, so hoping we can just move along with > > > it, or get something else out soon? > > > > > > I'd really like this to not just fall into a void waiting for a > > > different patch when this fixes the issue. > > > > Hi PJ, > > > > I haven't seen a recent update on this. I'm asking for an update. > > Otherwise, Alex and Sylwester are on this thread; perhaps they have > > some info. > > > > Thanks, > > Tony > > > > Hello, > > The driver does not blindly try to free MSI-X vector twice here. This is > guarded by I40E_FLAG_MSIX_ENABLED and I40E_FLAG_MSI_ENABLED flags. > Only if those flags are set we will try to free MSI/MSI-X vectors in > i40e_reset_interrupt_capability(). Additionally > i40e_reset_interrupt_capability() clears those flags every time it is called so > even if we call it twice in a row the driver will not free the vectors twice. I > really can't see how this patch is fixing anything as the issue here is not with > MSI vectors but with misc IRQ vectors. We have a proper patch for this ready > in OOT and we will upstream it soon. The problem here is that in > i40e_clear_interrupt_scheme() driver calls i40e_free_misc_vector() but in > case VSI setup fails misc vector is not allocated yet and we get a call trace in > free_irq that we are trying to free IRQ that has not been allocated yet. That's fine. I do see the guards for the queue vectors. I saw them before. The point is i40e_clear_interrupt_scheme() tries to free the MISC vector without guard, or without any check if it was allocated before. In the error path, it tries to free it. We get an oops for a double-free of an IRQ (also read: free an unallocated interrupt). I know how this code works. I wrote the original reset/clear interrupt scheme functions in ixgbe, and ported them to i40e when I wrote the initial driver. We hit a problem in production, and I'm trying to patch it where we don't need to call clear_interrupt_scheme() if we fail to bring the main VSI online during probe. I don't see why this needs to be a semantic discussion over how the vectors are freed. We have a valid oops, still have it upstream. I've also checked the OOT driver on SourceForge released in July. It has the same problem: static void i40e_clear_interrupt_scheme(struct i40e_pf *pf) { int i; i40e_free_misc_vector(pf); i40e_put_lump(pf->irq_pile, pf->iwarp_base_vector, I40E_IWARP_IRQ_PILE_ID); [...] I've also been told by some friends that no fix exists in internal git either. So please, either propose a fix, ask me to change the patch, or merge it. I'd really like to have our OS vendor be able to pick up this fix asap once it hits an upstream tree. Cheers, -PJ Waskiewicz > Regards > Sylwester Dziedziuch > > > > -PJ > > > > > > ________________________________ > > > > > > Note: This email is for the confidential use of the named > > > addressee(s) only and may contain proprietary, confidential, or > > > privileged information and/or personal data. If you are not the > > > intended recipient, you are hereby notified that any review, > > > dissemination, or copying of this email is strictly prohibited, and > > > requested to notify the sender immediately and destroy this email > > > and any attachments. Email transmission cannot be guaranteed to be > > > secure or error-free. The Company, therefore, does not make any > > > guarantees as to the completeness or accuracy of this email or any > attachments. > > > This email is for informational purposes only and does not > > > constitute a recommendation, offer, request, or solicitation of any > > > kind to buy, sell, subscribe, redeem, or perform any type of > > > transaction of a financial product. Personal data, as defined by > > > applicable data protection and privacy laws, contained in this email > > > may be processed by the Company, and any of its affiliated or > > > related companies, for legal, compliance, and/or business-related > > > purposes. You may have rights regarding your personal data; for > > > information on exercising these rights or the Company’s treatment of > > > personal data, please email datarequests@jumptrading.com. ________________________________ Note: This email is for the confidential use of the named addressee(s) only and may contain proprietary, confidential, or privileged information and/or personal data. If you are not the intended recipient, you are hereby notified that any review, dissemination, or copying of this email is strictly prohibited, and requested to notify the sender immediately and destroy this email and any attachments. Email transmission cannot be guaranteed to be secure or error-free. The Company, therefore, does not make any guarantees as to the completeness or accuracy of this email or any attachments. This email is for informational purposes only and does not constitute a recommendation, offer, request, or solicitation of any kind to buy, sell, subscribe, redeem, or perform any type of transaction of a financial product. Personal data, as defined by applicable data protection and privacy laws, contained in this email may be processed by the Company, and any of its affiliated or related companies, for legal, compliance, and/or business-related purposes. You may have rights regarding your personal data; for information on exercising these rights or the Company’s treatment of personal data, please email datarequests@jumptrading.com.
Hello PJ > -----Original Message----- > From: PJ Waskiewicz <pwaskiewicz@jumptrading.com> > Sent: Tuesday, September 14, 2021 11:41 PM > To: Dziedziuch, SylwesterX <sylwesterx.dziedziuch@intel.com>; Nguyen, > Anthony L <anthony.l.nguyen@intel.com> > Cc: davem@davemloft.net; pjwaskiewicz@gmail.com; Fijalkowski, Maciej > <maciej.fijalkowski@intel.com>; Loktionov, Aleksandr > <aleksandr.loktionov@intel.com>; netdev@vger.kernel.org; Brandeburg, Jesse > <jesse.brandeburg@intel.com>; intel-wired-lan@lists.osuosl.org; > Machnikowski, Maciej <maciej.machnikowski@intel.com> > Subject: RE: [PATCH 1/1] i40e: Avoid double IRQ free on error path in probe() > > Hi Sylwester, > > > -----Original Message----- > > From: Dziedziuch, SylwesterX <sylwesterx.dziedziuch@intel.com> > > Sent: Tuesday, September 14, 2021 1:24 AM > > To: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; PJ Waskiewicz > > <pwaskiewicz@jumptrading.com> > > Cc: davem@davemloft.net; pjwaskiewicz@gmail.com; Fijalkowski, Maciej > > <maciej.fijalkowski@intel.com>; Loktionov, Aleksandr > > <aleksandr.loktionov@intel.com>; netdev@vger.kernel.org; Brandeburg, > > Jesse <jesse.brandeburg@intel.com>; intel-wired-lan@lists.osuosl.org > > Subject: RE: [PATCH 1/1] i40e: Avoid double IRQ free on error path in > > probe() > > > > [snip] > > > > > It's been 2 weeks since I replied. Any update on this? Maciej > > > > had already reviewed the patch, so hoping we can just move along > > > > with it, or get something else out soon? > > > > > > > > I'd really like this to not just fall into a void waiting for a > > > > different patch when this fixes the issue. > > > > > > Hi PJ, > > > > > > I haven't seen a recent update on this. I'm asking for an update. > > > Otherwise, Alex and Sylwester are on this thread; perhaps they have > > > some info. > > > > > > Thanks, > > > Tony > > > > > > > Hello, > > > > The driver does not blindly try to free MSI-X vector twice here. This > > is guarded by I40E_FLAG_MSIX_ENABLED and I40E_FLAG_MSI_ENABLED > flags. > > Only if those flags are set we will try to free MSI/MSI-X vectors in > > i40e_reset_interrupt_capability(). Additionally > > i40e_reset_interrupt_capability() clears those flags every time it is > > called so even if we call it twice in a row the driver will not free > > the vectors twice. I really can't see how this patch is fixing > > anything as the issue here is not with MSI vectors but with misc IRQ > > vectors. We have a proper patch for this ready in OOT and we will > > upstream it soon. The problem here is that in > > i40e_clear_interrupt_scheme() driver calls i40e_free_misc_vector() but > > in case VSI setup fails misc vector is not allocated yet and we get a > > call trace in free_irq that we are trying to free IRQ that has not been > allocated yet. > > That's fine. I do see the guards for the queue vectors. I saw them before. The > point is i40e_clear_interrupt_scheme() tries to free the MISC vector without > guard, or without any check if it was allocated before. In the error path, it tries > to free it. We get an oops for a double-free of an IRQ (also read: free an > unallocated interrupt). > > I know how this code works. I wrote the original reset/clear interrupt scheme > functions in ixgbe, and ported them to i40e when I wrote the initial driver. We > hit a problem in production, and I'm trying to patch it where we don't need to > call clear_interrupt_scheme() if we fail to bring the main VSI online during > probe. I don't see why this needs to be a semantic discussion over how the > vectors are freed. We have a valid oops, still have it upstream. > > I've also checked the OOT driver on SourceForge released in July. It has the > same problem: > > static void i40e_clear_interrupt_scheme(struct i40e_pf *pf) { > int i; > > i40e_free_misc_vector(pf); > > i40e_put_lump(pf->irq_pile, pf->iwarp_base_vector, > I40E_IWARP_IRQ_PILE_ID); [...] > > I've also been told by some friends that no fix exists in internal git either. So > please, either propose a fix, ask me to change the patch, or merge it. I'd really > like to have our OS vendor be able to pick up this fix asap once it hits an > upstream tree. > > Cheers, > -PJ Waskiewicz > You are right the problem is with misc IRQ vector but as far as I can see this patch only moves i40e_reset_interrupt_capability() outside of i40e_clear_interrupt_scheme(). It does not fix the problem of i40e_free_misc_vector() on unallocated vector in error path. We have a proper fix for this that adds additional check for __I40E_MISC_IRQ_REQUESTED bit to i40e_free_misc_vector(): if (pf->flags & I40E_FLAG_MSIX_ENABLED && pf->msix_entries && test_bit(__I40E_MISC_IRQ_REQUESTED, pf->state)) { This bit is set only if misc vector was properly allocated. The patch will be on intel-wired soon. > > Regards > > Sylwester Dziedziuch > > > > > > -PJ > > > > > > > > ________________________________ > > > > > > > > Note: This email is for the confidential use of the named > > > > addressee(s) only and may contain proprietary, confidential, or > > > > privileged information and/or personal data. If you are not the > > > > intended recipient, you are hereby notified that any review, > > > > dissemination, or copying of this email is strictly prohibited, > > > > and requested to notify the sender immediately and destroy this > > > > email and any attachments. Email transmission cannot be guaranteed > > > > to be secure or error-free. The Company, therefore, does not make > > > > any guarantees as to the completeness or accuracy of this email or > > > > any > > attachments. > > > > This email is for informational purposes only and does not > > > > constitute a recommendation, offer, request, or solicitation of > > > > any kind to buy, sell, subscribe, redeem, or perform any type of > > > > transaction of a financial product. Personal data, as defined by > > > > applicable data protection and privacy laws, contained in this > > > > email may be processed by the Company, and any of its affiliated > > > > or related companies, for legal, compliance, and/or > > > > business-related purposes. You may have rights regarding your > > > > personal data; for information on exercising these rights or the > > > > Company’s treatment of personal data, please email > datarequests@jumptrading.com. > > ________________________________ > > Note: This email is for the confidential use of the named addressee(s) only and > may contain proprietary, confidential, or privileged information and/or > personal data. If you are not the intended recipient, you are hereby notified > that any review, dissemination, or copying of this email is strictly prohibited, > and requested to notify the sender immediately and destroy this email and > any attachments. Email transmission cannot be guaranteed to be secure or > error-free. The Company, therefore, does not make any guarantees as to the > completeness or accuracy of this email or any attachments. This email is for > informational purposes only and does not constitute a recommendation, offer, > request, or solicitation of any kind to buy, sell, subscribe, redeem, or perform > any type of transaction of a financial product. Personal data, as defined by > applicable data protection and privacy laws, contained in this email may be > processed by the Company, and any of its affiliated or related companies, for > legal, compliance, and/or business-related purposes. You may have rights > regarding your personal data; for information on exercising these rights or the > Company’s treatment of personal data, please email > datarequests@jumptrading.com.
Hi Sylwester, > > You are right the problem is with misc IRQ vector but as far as I can see this > patch only moves i40e_reset_interrupt_capability() outside of > i40e_clear_interrupt_scheme(). It does not fix the problem of > i40e_free_misc_vector() on unallocated vector in error path. We have a > proper fix for this that adds additional check for > __I40E_MISC_IRQ_REQUESTED bit to i40e_free_misc_vector(): It does fix the problem if you call the function when the MISC vector hasn't been allocated. Yes, I moved reset_interrupt_capability() out so it could be separately called in the probe() error cleanup path. > if (pf->flags & I40E_FLAG_MSIX_ENABLED && pf->msix_entries && > test_bit(__I40E_MISC_IRQ_REQUESTED, pf->state)) { > > This bit is set only if misc vector was properly allocated. The patch will be on > intel-wired soon. This isn't even in the OOT driver from SourceForge. And even if you used that to guard freeing the vector or not, the first bit of that function is still writing to a register to disable that cause in the hardware: static void i40e_free_misc_vector(struct i40e_pf *pf) { /* Disable ICR 0 */ wr32(&pf->hw, I40E_PFINT_ICR0_ENA, 0); i40e_flush(&pf->hw); Would you still want to do that blindly if the vector wasn't allocated in the first place? Seems excessive, but it'd be harmless. Seems like not calling this function altogether would be cleaner and generate less MMIO activity if the MISC vector wasn't allocated at all and we're falling out of an error path... I am really at a loss here. This is clearly broken. We have an Oops. We get these occasionally on boot, and it's really annoying to deal with on production machines. What is the definition of "soon" here for this new patch to show up? My distro vendor would love to pull some sort of fix in so we can get it into our build images, and stop having this problem. My patch fixes the immediate problem. If you don't like the patch (which it appears you don't; that's fine), then stalling or saying a different fix is coming "soon" is really not a great support model. This would be great to merge, and then if you want to make it "better" on your schedule, it's open source, and you can submit a patch. Or I'll be happy to respin the patch, but still calling free_misc_vector() in an error path when the MISC vector was never allocated seems like a bad design decision to me. -PJ ________________________________ Note: This email is for the confidential use of the named addressee(s) only and may contain proprietary, confidential, or privileged information and/or personal data. If you are not the intended recipient, you are hereby notified that any review, dissemination, or copying of this email is strictly prohibited, and requested to notify the sender immediately and destroy this email and any attachments. Email transmission cannot be guaranteed to be secure or error-free. The Company, therefore, does not make any guarantees as to the completeness or accuracy of this email or any attachments. This email is for informational purposes only and does not constitute a recommendation, offer, request, or solicitation of any kind to buy, sell, subscribe, redeem, or perform any type of transaction of a financial product. Personal data, as defined by applicable data protection and privacy laws, contained in this email may be processed by the Company, and any of its affiliated or related companies, for legal, compliance, and/or business-related purposes. You may have rights regarding your personal data; for information on exercising these rights or the Company’s treatment of personal data, please email datarequests@jumptrading.com.
Hello PJ, > -----Original Message----- > From: PJ Waskiewicz <pwaskiewicz@jumptrading.com> > Sent: Saturday, September 18, 2021 4:02 AM > To: Dziedziuch, SylwesterX <sylwesterx.dziedziuch@intel.com>; Nguyen, > Anthony L <anthony.l.nguyen@intel.com> > Cc: davem@davemloft.net; pjwaskiewicz@gmail.com; Fijalkowski, Maciej > <maciej.fijalkowski@intel.com>; Loktionov, Aleksandr > <aleksandr.loktionov@intel.com>; netdev@vger.kernel.org; Brandeburg, Jesse > <jesse.brandeburg@intel.com>; intel-wired-lan@lists.osuosl.org; > Machnikowski, Maciej <maciej.machnikowski@intel.com> > Subject: RE: [PATCH 1/1] i40e: Avoid double IRQ free on error path in probe() > > Hi Sylwester, > > > > > You are right the problem is with misc IRQ vector but as far as I can > > see this patch only moves i40e_reset_interrupt_capability() outside of > > i40e_clear_interrupt_scheme(). It does not fix the problem of > > i40e_free_misc_vector() on unallocated vector in error path. We have a > > proper fix for this that adds additional check for > > __I40E_MISC_IRQ_REQUESTED bit to i40e_free_misc_vector(): > > It does fix the problem if you call the function when the MISC vector hasn't > been allocated. Yes, I moved reset_interrupt_capability() out so it could be > separately called in the probe() error cleanup path. > > > if (pf->flags & I40E_FLAG_MSIX_ENABLED && pf->msix_entries && > > test_bit(__I40E_MISC_IRQ_REQUESTED, pf->state)) { > > > > This bit is set only if misc vector was properly allocated. The patch > > will be on intel-wired soon. > > This isn't even in the OOT driver from SourceForge. And even if you used that > to guard freeing the vector or not, the first bit of that function is still writing to > a register to disable that cause in the hardware: > > static void i40e_free_misc_vector(struct i40e_pf *pf) { > /* Disable ICR 0 */ > wr32(&pf->hw, I40E_PFINT_ICR0_ENA, 0); > i40e_flush(&pf->hw); > > Would you still want to do that blindly if the vector wasn't allocated in the first > place? Seems excessive, but it'd be harmless. Seems like not calling this > function altogether would be cleaner and generate less MMIO activity if the > MISC vector wasn't allocated at all and we're falling out of an error path... > > I am really at a loss here. This is clearly broken. We have an Oops. We get > these occasionally on boot, and it's really annoying to deal with on production > machines. What is the definition of "soon" here for this new patch to show > up? My distro vendor would love to pull some sort of fix in so we can get it > into our build images, and stop having this problem. My patch fixes the > immediate problem. If you don't like the patch (which it appears you don't; > that's fine), then stalling or saying a different fix is coming "soon" is really not a > great support model. This would be great to merge, and then if you want to > make it "better" on your schedule, it's open source, and you can submit a > patch. Or I'll be happy to respin the patch, but still calling free_misc_vector() > in an error path when the MISC vector was never allocated seems like a bad > design decision to me. > > -PJ I totally agree that we shouldn’t call free_misc_vector if misc vector wasn't allocated yet but to me this is not what your patch is doing. free_misc_vector() is called in clear_interrupt_scheme not reset_interrupt_capability(). In your patch clear_interrupt_scheme() will still be called when pf switch setup fails in i40e_probe() and it will still call free_misc_vector on unallocated misc IRQ. Other proper way to fix this would be moving free_misc_vector() out of clear_interrupt_scheme() and calling it separately when misc vector was really allocated. As for the hw register being written in our patch as you said it's harmless. The patch we've prepared should be on iwl today. BR Sylwester Dziedziuch > > ________________________________ > > Note: This email is for the confidential use of the named addressee(s) only and > may contain proprietary, confidential, or privileged information and/or > personal data. If you are not the intended recipient, you are hereby notified > that any review, dissemination, or copying of this email is strictly prohibited, > and requested to notify the sender immediately and destroy this email and > any attachments. Email transmission cannot be guaranteed to be secure or > error-free. The Company, therefore, does not make any guarantees as to the > completeness or accuracy of this email or any attachments. This email is for > informational purposes only and does not constitute a recommendation, offer, > request, or solicitation of any kind to buy, sell, subscribe, redeem, or perform > any type of transaction of a financial product. Personal data, as defined by > applicable data protection and privacy laws, contained in this email may be > processed by the Company, and any of its affiliated or related companies, for > legal, compliance, and/or business-related purposes. You may have rights > regarding your personal data; for information on exercising these rights or the > Company’s treatment of personal data, please email > datarequests@jumptrading.com.
> Hello PJ, Hello, > > > static void i40e_free_misc_vector(struct i40e_pf *pf) { > > /* Disable ICR 0 */ > > wr32(&pf->hw, I40E_PFINT_ICR0_ENA, 0); > > i40e_flush(&pf->hw); > > > > Would you still want to do that blindly if the vector wasn't allocated > > in the first place? Seems excessive, but it'd be harmless. Seems > > like not calling this function altogether would be cleaner and > > generate less MMIO activity if the MISC vector wasn't allocated at all and > we're falling out of an error path... > > > > I am really at a loss here. This is clearly broken. We have an Oops. > > We get these occasionally on boot, and it's really annoying to deal > > with on production machines. What is the definition of "soon" here > > for this new patch to show up? My distro vendor would love to pull > > some sort of fix in so we can get it into our build images, and stop > > having this problem. My patch fixes the immediate problem. If you > > don't like the patch (which it appears you don't; that's fine), then > > stalling or saying a different fix is coming "soon" is really not a > > great support model. This would be great to merge, and then if you > > want to make it "better" on your schedule, it's open source, and you > > can submit a patch. Or I'll be happy to respin the patch, but still > > calling free_misc_vector() in an error path when the MISC vector was > never allocated seems like a bad design decision to me. > > > > -PJ > > I totally agree that we shouldn’t call free_misc_vector if misc vector wasn't > allocated yet but to me this is not what your patch is doing. > free_misc_vector() is called in clear_interrupt_scheme not > reset_interrupt_capability(). In your patch clear_interrupt_scheme() will still > be called when pf switch setup fails in i40e_probe() and it will still call > free_misc_vector on unallocated misc IRQ. Other proper way to fix this > would be moving free_misc_vector() out of clear_interrupt_scheme() and > calling it separately when misc vector was really allocated. As for the hw > register being written in our patch as you said it's harmless. The patch we've > prepared should be on iwl today. > Ok, I see the patch on IWL....let's discuss.... Just above, I pointed out that if the MISC vector hasn't been allocated at all, then we don't want to call free_misc_vector() at all. That would also mean the suggestion to check the atomic state bit to avoid the actual free would *still* have the code call free_misc_vector(), and only skip the actual free (after doing an unnecessary MMIO write and then read to flush). I pointed out that wouldn't be ideal, and you, just above, agreed. Yet, the fix you guys submitted to IWL does exactly that. So are we just saying things to bury this thread and hope it goes away, or just willfully not doing what we agreed on? It's pretty disheartening to consider feedback, agree to it, then completely ignore it. That's not how open source works... Also, regardless how you guys want the code to work, it's usually good form to include a "Reported-by:" in a patch if you're deciding not to take a proposed patch. It's even better form to include the Oops that was reported in the first patch, since that makes things like ${SEARCH_ENGINE} work for people running into the same issue have a chance to find a solution. Not doing either of these when someone else has done work to identify an issue, test a fix, and propose it, is not a good way to attract more people to work on this driver in the future. If we wanted to do something where free_misc_vector() isn't called if the state flag isn't set, then why not something like this, which would keep in the spirit of what we agreed on above: diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c index 1d1f52756a93..a40193bcc7b7 100644 --- a/drivers/net/ethernet/intel/i40e/i40e_main.c +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c @@ -4868,7 +4868,9 @@ static void i40e_clear_interrupt_scheme(struct i40e_pf *pf) { int i; - i40e_free_misc_vector(pf); + /* Only attempt to free the misc vector if it's already allocated */ + if (test_bit(__I40E_MISC_IRQ_REQUESTED, pf->state)) + i40e_free_misc_vector(pf); i40e_put_lump(pf->irq_pile, pf->iwarp_base_vector, I40E_IWARP_IRQ_PILE_ID); -PJ ________________________________ Note: This email is for the confidential use of the named addressee(s) only and may contain proprietary, confidential, or privileged information and/or personal data. If you are not the intended recipient, you are hereby notified that any review, dissemination, or copying of this email is strictly prohibited, and requested to notify the sender immediately and destroy this email and any attachments. Email transmission cannot be guaranteed to be secure or error-free. The Company, therefore, does not make any guarantees as to the completeness or accuracy of this email or any attachments. This email is for informational purposes only and does not constitute a recommendation, offer, request, or solicitation of any kind to buy, sell, subscribe, redeem, or perform any type of transaction of a financial product. Personal data, as defined by applicable data protection and privacy laws, contained in this email may be processed by the Company, and any of its affiliated or related companies, for legal, compliance, and/or business-related purposes. You may have rights regarding your personal data; for information on exercising these rights or the Company’s treatment of personal data, please email datarequests@jumptrading.com.
> > Hello PJ, > > Hello, Hi Tony and Sylwester, Any updates/comments on my reply from a few days ago on this? -PJ > > > > > static void i40e_free_misc_vector(struct i40e_pf *pf) { > > > /* Disable ICR 0 */ > > > wr32(&pf->hw, I40E_PFINT_ICR0_ENA, 0); > > > i40e_flush(&pf->hw); > > > > > > Would you still want to do that blindly if the vector wasn't > > > allocated in the first place? Seems excessive, but it'd be > > > harmless. Seems like not calling this function altogether would be > > > cleaner and generate less MMIO activity if the MISC vector wasn't > > > allocated at all and > > we're falling out of an error path... > > > > > > I am really at a loss here. This is clearly broken. We have an Oops. > > > We get these occasionally on boot, and it's really annoying to deal > > > with on production machines. What is the definition of "soon" here > > > for this new patch to show up? My distro vendor would love to pull > > > some sort of fix in so we can get it into our build images, and stop > > > having this problem. My patch fixes the immediate problem. If you > > > don't like the patch (which it appears you don't; that's fine), then > > > stalling or saying a different fix is coming "soon" is really not a > > > great support model. This would be great to merge, and then if you > > > want to make it "better" on your schedule, it's open source, and you > > > can submit a patch. Or I'll be happy to respin the patch, but still > > > calling free_misc_vector() in an error path when the MISC vector was > > never allocated seems like a bad design decision to me. > > > > > > -PJ > > > > I totally agree that we shouldn’t call free_misc_vector if misc vector > > wasn't allocated yet but to me this is not what your patch is doing. > > free_misc_vector() is called in clear_interrupt_scheme not > > reset_interrupt_capability(). In your patch clear_interrupt_scheme() > > will still be called when pf switch setup fails in i40e_probe() and it > > will still call free_misc_vector on unallocated misc IRQ. Other proper > > way to fix this would be moving free_misc_vector() out of > > clear_interrupt_scheme() and calling it separately when misc vector > > was really allocated. As for the hw register being written in our > > patch as you said it's harmless. The patch we've prepared should be on iwl > today. > > > > Ok, I see the patch on IWL....let's discuss.... > > Just above, I pointed out that if the MISC vector hasn't been allocated at all, > then we don't want to call free_misc_vector() at all. That would also mean > the suggestion to check the atomic state bit to avoid the actual free would > *still* have the code call free_misc_vector(), and only skip the actual free > (after doing an unnecessary MMIO write and then read to flush). I pointed > out that wouldn't be ideal, and you, just above, agreed. Yet, the fix you guys > submitted to IWL does exactly that. So are we just saying things to bury this > thread and hope it goes away, or just willfully not doing what we agreed on? > It's pretty disheartening to consider feedback, agree to it, then completely > ignore it. That's not how open source works... > > Also, regardless how you guys want the code to work, it's usually good form > to include a "Reported-by:" in a patch if you're deciding not to take a > proposed patch. It's even better form to include the Oops that was reported > in the first patch, since that makes things like ${SEARCH_ENGINE} work for > people running into the same issue have a chance to find a solution. Not > doing either of these when someone else has done work to identify an issue, > test a fix, and propose it, is not a good way to attract more people to work on > this driver in the future. > > If we wanted to do something where free_misc_vector() isn't called if the > state flag isn't set, then why not something like this, which would keep in the > spirit of what we agreed on above: > > diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c > b/drivers/net/ethernet/intel/i40e/i40e_main.c > index 1d1f52756a93..a40193bcc7b7 100644 > --- a/drivers/net/ethernet/intel/i40e/i40e_main.c > +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c > @@ -4868,7 +4868,9 @@ static void i40e_clear_interrupt_scheme(struct > i40e_pf *pf) { > int i; > > - i40e_free_misc_vector(pf); > + /* Only attempt to free the misc vector if it's already allocated */ > + if (test_bit(__I40E_MISC_IRQ_REQUESTED, pf->state)) > + i40e_free_misc_vector(pf); > > i40e_put_lump(pf->irq_pile, pf->iwarp_base_vector, > I40E_IWARP_IRQ_PILE_ID); > > -PJ ________________________________ Note: This email is for the confidential use of the named addressee(s) only and may contain proprietary, confidential, or privileged information and/or personal data. If you are not the intended recipient, you are hereby notified that any review, dissemination, or copying of this email is strictly prohibited, and requested to notify the sender immediately and destroy this email and any attachments. Email transmission cannot be guaranteed to be secure or error-free. The Company, therefore, does not make any guarantees as to the completeness or accuracy of this email or any attachments. This email is for informational purposes only and does not constitute a recommendation, offer, request, or solicitation of any kind to buy, sell, subscribe, redeem, or perform any type of transaction of a financial product. Personal data, as defined by applicable data protection and privacy laws, contained in this email may be processed by the Company, and any of its affiliated or related companies, for legal, compliance, and/or business-related purposes. You may have rights regarding your personal data; for information on exercising these rights or the Company’s treatment of personal data, please email datarequests@jumptrading.com.
Hello PJ, Sorry for the late response. I am applying your suggestions to the patch. The updated version will be on IWL in a moment. > -----Original Message----- > From: PJ Waskiewicz <pwaskiewicz@jumptrading.com> > Sent: Thursday, September 23, 2021 5:17 PM > To: Dziedziuch, SylwesterX <sylwesterx.dziedziuch@intel.com>; Nguyen, > Anthony L <anthony.l.nguyen@intel.com> > Cc: davem@davemloft.net; pjwaskiewicz@gmail.com; Fijalkowski, Maciej > <maciej.fijalkowski@intel.com>; Loktionov, Aleksandr > <aleksandr.loktionov@intel.com>; netdev@vger.kernel.org; Brandeburg, Jesse > <jesse.brandeburg@intel.com>; intel-wired-lan@lists.osuosl.org; > Machnikowski, Maciej <maciej.machnikowski@intel.com> > Subject: RE: [PATCH 1/1] i40e: Avoid double IRQ free on error path in probe() > > > > Hello PJ, > > > > Hello, > > Hi Tony and Sylwester, > > Any updates/comments on my reply from a few days ago on this? > > -PJ > > > > > > > > static void i40e_free_misc_vector(struct i40e_pf *pf) { > > > > /* Disable ICR 0 */ > > > > wr32(&pf->hw, I40E_PFINT_ICR0_ENA, 0); > > > > i40e_flush(&pf->hw); > > > > > > > > Would you still want to do that blindly if the vector wasn't > > > > allocated in the first place? Seems excessive, but it'd be > > > > harmless. Seems like not calling this function altogether would > > > > be cleaner and generate less MMIO activity if the MISC vector > > > > wasn't allocated at all and > > > we're falling out of an error path... > > > > > > > > I am really at a loss here. This is clearly broken. We have an Oops. > > > > We get these occasionally on boot, and it's really annoying to > > > > deal with on production machines. What is the definition of > > > > "soon" here for this new patch to show up? My distro vendor would > > > > love to pull some sort of fix in so we can get it into our build > > > > images, and stop having this problem. My patch fixes the > > > > immediate problem. If you don't like the patch (which it appears > > > > you don't; that's fine), then stalling or saying a different fix > > > > is coming "soon" is really not a great support model. This would > > > > be great to merge, and then if you want to make it "better" on > > > > your schedule, it's open source, and you can submit a patch. Or > > > > I'll be happy to respin the patch, but still calling > > > > free_misc_vector() in an error path when the MISC vector was > > > never allocated seems like a bad design decision to me. > > > > > > > > -PJ > > > > > > I totally agree that we shouldn’t call free_misc_vector if misc > > > vector wasn't allocated yet but to me this is not what your patch is doing. > > > free_misc_vector() is called in clear_interrupt_scheme not > > > reset_interrupt_capability(). In your patch clear_interrupt_scheme() > > > will still be called when pf switch setup fails in i40e_probe() and > > > it will still call free_misc_vector on unallocated misc IRQ. Other > > > proper way to fix this would be moving free_misc_vector() out of > > > clear_interrupt_scheme() and calling it separately when misc vector > > > was really allocated. As for the hw register being written in our > > > patch as you said it's harmless. The patch we've prepared should be > > > on iwl > > today. > > > > > > > Ok, I see the patch on IWL....let's discuss.... > > > > Just above, I pointed out that if the MISC vector hasn't been > > allocated at all, then we don't want to call free_misc_vector() at > > all. That would also mean the suggestion to check the atomic state > > bit to avoid the actual free would > > *still* have the code call free_misc_vector(), and only skip the > > actual free (after doing an unnecessary MMIO write and then read to > > flush). I pointed out that wouldn't be ideal, and you, just above, > > agreed. Yet, the fix you guys submitted to IWL does exactly that. So > > are we just saying things to bury this thread and hope it goes away, or just > willfully not doing what we agreed on? > > It's pretty disheartening to consider feedback, agree to it, then > > completely ignore it. That's not how open source works... > > > > Also, regardless how you guys want the code to work, it's usually good > > form to include a "Reported-by:" in a patch if you're deciding not to > > take a proposed patch. It's even better form to include the Oops that > > was reported in the first patch, since that makes things like > > ${SEARCH_ENGINE} work for people running into the same issue have a > > chance to find a solution. Not doing either of these when someone > > else has done work to identify an issue, test a fix, and propose it, > > is not a good way to attract more people to work on this driver in the future. > > > > If we wanted to do something where free_misc_vector() isn't called if > > the state flag isn't set, then why not something like this, which > > would keep in the spirit of what we agreed on above: > > > > diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c > > b/drivers/net/ethernet/intel/i40e/i40e_main.c > > index 1d1f52756a93..a40193bcc7b7 100644 > > --- a/drivers/net/ethernet/intel/i40e/i40e_main.c > > +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c > > @@ -4868,7 +4868,9 @@ static void i40e_clear_interrupt_scheme(struct > > i40e_pf *pf) { > > int i; > > > > - i40e_free_misc_vector(pf); > > + /* Only attempt to free the misc vector if it's already allocated */ > > + if (test_bit(__I40E_MISC_IRQ_REQUESTED, pf->state)) > > + i40e_free_misc_vector(pf); > > > > i40e_put_lump(pf->irq_pile, pf->iwarp_base_vector, > > I40E_IWARP_IRQ_PILE_ID); > > > > -PJ > > ________________________________ > > Note: This email is for the confidential use of the named addressee(s) only and > may contain proprietary, confidential, or privileged information and/or > personal data. If you are not the intended recipient, you are hereby notified > that any review, dissemination, or copying of this email is strictly prohibited, > and requested to notify the sender immediately and destroy this email and > any attachments. Email transmission cannot be guaranteed to be secure or > error-free. The Company, therefore, does not make any guarantees as to the > completeness or accuracy of this email or any attachments. This email is for > informational purposes only and does not constitute a recommendation, offer, > request, or solicitation of any kind to buy, sell, subscribe, redeem, or perform > any type of transaction of a financial product. Personal data, as defined by > applicable data protection and privacy laws, contained in this email may be > processed by the Company, and any of its affiliated or related companies, for > legal, compliance, and/or business-related purposes. You may have rights > regarding your personal data; for information on exercising these rights or the > Company’s treatment of personal data, please email > datarequests@jumptrading.com.
diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c index 1d1f52756a93..b1cbd0eae83c 100644 --- a/drivers/net/ethernet/intel/i40e/i40e_main.c +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c @@ -4862,7 +4862,8 @@ static void i40e_reset_interrupt_capability(struct i40e_pf *pf) * @pf: board private structure * * We go through and clear interrupt specific resources and reset the structure - * to pre-load conditions + * to pre-load conditions. OS interrupt teardown must be done separately due + * to VSI vs PF instantiation, and different teardown path requirements. **/ static void i40e_clear_interrupt_scheme(struct i40e_pf *pf) { @@ -4877,7 +4878,6 @@ static void i40e_clear_interrupt_scheme(struct i40e_pf *pf) for (i = 0; i < pf->num_alloc_vsi; i++) if (pf->vsi[i]) i40e_vsi_free_q_vectors(pf->vsi[i]); - i40e_reset_interrupt_capability(pf); } /** @@ -10523,6 +10523,7 @@ static void i40e_rebuild(struct i40e_pf *pf, bool reinit, bool lock_acquired) */ free_irq(pf->pdev->irq, pf); i40e_clear_interrupt_scheme(pf); + i40e_reset_interrupt_capability(pf); if (i40e_restore_interrupt_scheme(pf)) goto end_unlock; } @@ -15908,6 +15909,7 @@ static void i40e_remove(struct pci_dev *pdev) /* Clear all dynamic memory lists of rings, q_vectors, and VSIs */ rtnl_lock(); i40e_clear_interrupt_scheme(pf); + i40e_reset_interrupt_capability(pf); for (i = 0; i < pf->num_alloc_vsi; i++) { if (pf->vsi[i]) { if (!test_bit(__I40E_RECOVERY_MODE, pf->state)) @@ -16130,6 +16132,7 @@ static void i40e_shutdown(struct pci_dev *pdev) */ rtnl_lock(); i40e_clear_interrupt_scheme(pf); + i40e_reset_interrupt_capability(pf); rtnl_unlock(); if (system_state == SYSTEM_POWER_OFF) { @@ -16182,6 +16185,7 @@ static int __maybe_unused i40e_suspend(struct device *dev) * to CPU0. */ i40e_clear_interrupt_scheme(pf); + i40e_reset_interrupt_capability(pf); rtnl_unlock();
This fixes an error path condition when probe() fails due to the default VSI not being available or online yet in the firmware. If that happens, the previous teardown path would clear the interrupt scheme, which also freed the IRQs with the OS. Then the error path for the switch setup (pre-VSI) would attempt to free the OS IRQs as well. [ 14.597121] i40e 0000:31:00.0: setup of MAIN VSI failed [ 14.712167] i40e 0000:31:00.0: setup_pf_switch failed: -11 [ 14.755318] ------------[ cut here ]------------ [ 14.766261] Trying to free already-free IRQ 266 [ 14.777224] WARNING: CPU: 0 PID: 5 at kernel/irq/manage.c:1731 __free_irq+0x9a/0x300 [ 14.791341] Modules linked in: XXXXXX [ 14.825361] CPU: 0 PID: 5 Comm: kworker/0:0 Not tainted <kernel omitted> [ 14.840630] Hardware name: XXXXXX [ 14.854924] Workqueue: events work_for_cpu_fn [ 14.866482] RIP: 0010:__free_irq+0x9a/0x300 [ 14.877638] Code: 08 75 0e e9 3c 02 00 00 4c 39 6b 08 74 59 48 89 da 48 8b 5a 18 48 85 db 75 ee 8b 74 24 04 48 c7 c7 58 a0 aa b4 e8 3f 2e f9 ff <0f> 0b 4c 89 f6 48 89 ef e8 f9 69 7b 00 49 8b 47 40 48 8b 80 80 00 [ 14.910571] RSP: 0000:ff6a6ad7401dfb60 EFLAGS: 00010086 [ 14.923265] RAX: 0000000000000000 RBX: ff3c97328eb56000 RCX: 0000000000000006 [ 14.937853] RDX: 0000000000000007 RSI: 0000000000000092 RDI: ff3c97333ee16a00 [ 14.952290] RBP: ff3c9731cff4caa4 R08: 00000000000006b8 R09: 0000000000aaaaaa [ 14.966781] R10: 0000000000000000 R11: ff6a6ad768aff020 R12: ff3c9731cff4cb80 [ 14.981436] R13: ff3c97328eb56000 R14: 0000000000000246 R15: ff3c9731cff4ca00 [ 14.996041] FS: 0000000000000000(0000) GS:ff3c97333ee00000(0000) knlGS:0000000000000000 [ 15.011511] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 15.024493] CR2: 00007fb6ac002000 CR3: 00000004f8c0a001 CR4: 0000000000761ef0 [ 15.039373] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 [ 15.054426] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 [ 15.068781] PKRU: 55555554 [ 15.078902] Call Trace: [ 15.088421] ? synchronize_irq+0x3a/0xa0 [ 15.099556] free_irq+0x2e/0x60 [ 15.109863] i40e_clear_interrupt_scheme+0x53/0x190 [i40e] [ 15.122718] i40e_probe.part.108+0x134b/0x1a40 [i40e] [ 15.135343] ? kmem_cache_alloc+0x158/0x1c0 [ 15.146688] ? acpi_ut_update_ref_count.part.1+0x8e/0x345 [ 15.159217] ? acpi_ut_update_object_reference+0x15e/0x1e2 [ 15.171879] ? strstr+0x21/0x70 [ 15.181802] ? irq_get_irq_data+0xa/0x20 [ 15.193198] ? mp_check_pin_attr+0x13/0xc0 [ 15.203909] ? irq_get_irq_data+0xa/0x20 [ 15.214310] ? mp_map_pin_to_irq+0xd3/0x2f0 [ 15.225206] ? acpi_register_gsi_ioapic+0x93/0x170 [ 15.236351] ? pci_conf1_read+0xa4/0x100 [ 15.246586] ? pci_bus_read_config_word+0x49/0x70 [ 15.257608] ? do_pci_enable_device+0xcc/0x100 [ 15.268337] local_pci_probe+0x41/0x90 [ 15.279016] work_for_cpu_fn+0x16/0x20 [ 15.289545] process_one_work+0x1a7/0x360 [ 15.300214] worker_thread+0x1cf/0x390 [ 15.309980] ? create_worker+0x1a0/0x1a0 [ 15.319854] kthread+0x112/0x130 [ 15.328806] ? kthread_flush_work_fn+0x10/0x10 [ 15.338739] ret_from_fork+0x1f/0x40 [ 15.347622] ---[ end trace 5220551832349274 ]--- Break apart the clear and reset schemes so that clear no longer calls i40_reset_interrupt_capability(), allowing that to be called across error paths in probe(). Fixes: 41c445ff0f482 ("i40e: main driver core") Signed-off-by: PJ Waskiewicz <pwaskiewicz@jumptrading.com> Reviewed-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com> --- drivers/net/ethernet/intel/i40e/i40e_main.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) -- 2.27.0