* [PATCH AUTOSEL 5.16 36/52] PCI/MSI: Decouple MSI[-X] disable from pcim_release() [not found] <20220117165853.1470420-1-sashal@kernel.org> @ 2022-01-17 16:58 ` Sasha Levin 2022-01-17 17:08 ` Greg Kroah-Hartman 0 siblings, 1 reply; 3+ messages in thread From: Sasha Levin @ 2022-01-17 16:58 UTC (permalink / raw) To: linux-kernel, stable Cc: Thomas Gleixner, Nishanth Menon, Michael Kelley, Greg Kroah-Hartman, Sasha Levin, bhelgaas, linux-pci From: Thomas Gleixner <tglx@linutronix.de> [ Upstream commit 3f35d2cf9fbc656db82579d849cc69c373b1ad0d ] The MSI core will introduce runtime allocation of MSI related data. This data will be devres managed and has to be set up before enabling PCI/MSI[-X]. This would introduce an ordering issue vs. pcim_release(). The setup order is: pcim_enable_device() devres_alloc(pcim_release...); ... pci_irq_alloc() msi_setup_device_data() devres_alloc(msi_device_data_release, ...) and once the device is released these release functions are invoked in the opposite order: msi_device_data_release() ... pcim_release() pci_disable_msi[x]() which is obviously wrong, because pci_disable_msi[x]() requires the MSI data to be available to tear down the MSI[-X] interrupts. Remove the MSI[-X] teardown from pcim_release() and add an explicit action to be installed on the attempt of enabling PCI/MSI[-X]. This allows the MSI core data allocation to be ordered correctly in a subsequent step. Reported-by: Nishanth Menon <nm@ti.com> Signed-off-by: Thomas Gleixner <tglx@linutronix.de> Tested-by: Michael Kelley <mikelley@microsoft.com> Tested-by: Nishanth Menon <nm@ti.com> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Link: https://lore.kernel.org/r/87tuf9rdoj.ffs@tglx Signed-off-by: Sasha Levin <sashal@kernel.org> --- drivers/pci/msi.c | 33 +++++++++++++++++++++++++++++++++ drivers/pci/pci.c | 5 ----- include/linux/pci.h | 3 ++- 3 files changed, 35 insertions(+), 6 deletions(-) diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c index d84cf30bb2790..1093f099846eb 100644 --- a/drivers/pci/msi.c +++ b/drivers/pci/msi.c @@ -461,6 +461,31 @@ void pci_restore_msi_state(struct pci_dev *dev) } EXPORT_SYMBOL_GPL(pci_restore_msi_state); +static void pcim_msi_release(void *pcidev) +{ + struct pci_dev *dev = pcidev; + + dev->is_msi_managed = false; + pci_free_irq_vectors(dev); +} + +/* + * Needs to be separate from pcim_release to prevent an ordering problem + * vs. msi_device_data_release() in the MSI core code. + */ +static int pcim_setup_msi_release(struct pci_dev *dev) +{ + int ret; + + if (!pci_is_managed(dev) || dev->is_msi_managed) + return 0; + + ret = devm_add_action(&dev->dev, pcim_msi_release, dev); + if (!ret) + dev->is_msi_managed = true; + return ret; +} + static struct msi_desc * msi_setup_entry(struct pci_dev *dev, int nvec, struct irq_affinity *affd) { @@ -1029,6 +1054,10 @@ static int __pci_enable_msi_range(struct pci_dev *dev, int minvec, int maxvec, if (nvec > maxvec) nvec = maxvec; + rc = pcim_setup_msi_release(dev); + if (rc) + return rc; + for (;;) { if (affd) { nvec = irq_calc_affinity_vectors(minvec, nvec, affd); @@ -1072,6 +1101,10 @@ static int __pci_enable_msix_range(struct pci_dev *dev, if (WARN_ON_ONCE(dev->msix_enabled)) return -EINVAL; + rc = pcim_setup_msi_release(dev); + if (rc) + return rc; + for (;;) { if (affd) { nvec = irq_calc_affinity_vectors(minvec, nvec, affd); diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index 3d2fb394986a4..f3f606c232a8a 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -2024,11 +2024,6 @@ static void pcim_release(struct device *gendev, void *res) struct pci_devres *this = res; int i; - if (dev->msi_enabled) - pci_disable_msi(dev); - if (dev->msix_enabled) - pci_disable_msix(dev); - for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) if (this->region_mask & (1 << i)) pci_release_region(dev, i); diff --git a/include/linux/pci.h b/include/linux/pci.h index 18a75c8e615cd..e26000404e3c3 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -425,7 +425,8 @@ struct pci_dev { unsigned int ats_enabled:1; /* Address Translation Svc */ unsigned int pasid_enabled:1; /* Process Address Space ID */ unsigned int pri_enabled:1; /* Page Request Interface */ - unsigned int is_managed:1; + unsigned int is_managed:1; /* Managed via devres */ + unsigned int is_msi_managed:1; /* MSI release via devres installed */ unsigned int needs_freset:1; /* Requires fundamental reset */ unsigned int state_saved:1; unsigned int is_physfn:1; -- 2.34.1 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH AUTOSEL 5.16 36/52] PCI/MSI: Decouple MSI[-X] disable from pcim_release() 2022-01-17 16:58 ` [PATCH AUTOSEL 5.16 36/52] PCI/MSI: Decouple MSI[-X] disable from pcim_release() Sasha Levin @ 2022-01-17 17:08 ` Greg Kroah-Hartman 2022-01-22 18:39 ` Sasha Levin 0 siblings, 1 reply; 3+ messages in thread From: Greg Kroah-Hartman @ 2022-01-17 17:08 UTC (permalink / raw) To: Sasha Levin Cc: linux-kernel, stable, Thomas Gleixner, Nishanth Menon, Michael Kelley, bhelgaas, linux-pci On Mon, Jan 17, 2022 at 11:58:37AM -0500, Sasha Levin wrote: > From: Thomas Gleixner <tglx@linutronix.de> > > [ Upstream commit 3f35d2cf9fbc656db82579d849cc69c373b1ad0d ] > > The MSI core will introduce runtime allocation of MSI related data. This > data will be devres managed and has to be set up before enabling > PCI/MSI[-X]. This would introduce an ordering issue vs. pcim_release(). > > The setup order is: > > pcim_enable_device() > devres_alloc(pcim_release...); > ... > pci_irq_alloc() > msi_setup_device_data() > devres_alloc(msi_device_data_release, ...) > > and once the device is released these release functions are invoked in the > opposite order: > > msi_device_data_release() > ... > pcim_release() > pci_disable_msi[x]() > > which is obviously wrong, because pci_disable_msi[x]() requires the MSI > data to be available to tear down the MSI[-X] interrupts. > > Remove the MSI[-X] teardown from pcim_release() and add an explicit action > to be installed on the attempt of enabling PCI/MSI[-X]. > > This allows the MSI core data allocation to be ordered correctly in a > subsequent step. > > Reported-by: Nishanth Menon <nm@ti.com> > Signed-off-by: Thomas Gleixner <tglx@linutronix.de> > Tested-by: Michael Kelley <mikelley@microsoft.com> > Tested-by: Nishanth Menon <nm@ti.com> > Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Link: https://lore.kernel.org/r/87tuf9rdoj.ffs@tglx > Signed-off-by: Sasha Levin <sashal@kernel.org> > --- > drivers/pci/msi.c | 33 +++++++++++++++++++++++++++++++++ > drivers/pci/pci.c | 5 ----- > include/linux/pci.h | 3 ++- > 3 files changed, 35 insertions(+), 6 deletions(-) > > diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c > index d84cf30bb2790..1093f099846eb 100644 > --- a/drivers/pci/msi.c > +++ b/drivers/pci/msi.c > @@ -461,6 +461,31 @@ void pci_restore_msi_state(struct pci_dev *dev) > } > EXPORT_SYMBOL_GPL(pci_restore_msi_state); > > +static void pcim_msi_release(void *pcidev) > +{ > + struct pci_dev *dev = pcidev; > + > + dev->is_msi_managed = false; > + pci_free_irq_vectors(dev); > +} > + > +/* > + * Needs to be separate from pcim_release to prevent an ordering problem > + * vs. msi_device_data_release() in the MSI core code. > + */ > +static int pcim_setup_msi_release(struct pci_dev *dev) > +{ > + int ret; > + > + if (!pci_is_managed(dev) || dev->is_msi_managed) > + return 0; > + > + ret = devm_add_action(&dev->dev, pcim_msi_release, dev); > + if (!ret) > + dev->is_msi_managed = true; > + return ret; > +} > + > static struct msi_desc * > msi_setup_entry(struct pci_dev *dev, int nvec, struct irq_affinity *affd) > { > @@ -1029,6 +1054,10 @@ static int __pci_enable_msi_range(struct pci_dev *dev, int minvec, int maxvec, > if (nvec > maxvec) > nvec = maxvec; > > + rc = pcim_setup_msi_release(dev); > + if (rc) > + return rc; > + > for (;;) { > if (affd) { > nvec = irq_calc_affinity_vectors(minvec, nvec, affd); > @@ -1072,6 +1101,10 @@ static int __pci_enable_msix_range(struct pci_dev *dev, > if (WARN_ON_ONCE(dev->msix_enabled)) > return -EINVAL; > > + rc = pcim_setup_msi_release(dev); > + if (rc) > + return rc; > + > for (;;) { > if (affd) { > nvec = irq_calc_affinity_vectors(minvec, nvec, affd); > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index 3d2fb394986a4..f3f606c232a8a 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -2024,11 +2024,6 @@ static void pcim_release(struct device *gendev, void *res) > struct pci_devres *this = res; > int i; > > - if (dev->msi_enabled) > - pci_disable_msi(dev); > - if (dev->msix_enabled) > - pci_disable_msix(dev); > - > for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) > if (this->region_mask & (1 << i)) > pci_release_region(dev, i); > diff --git a/include/linux/pci.h b/include/linux/pci.h > index 18a75c8e615cd..e26000404e3c3 100644 > --- a/include/linux/pci.h > +++ b/include/linux/pci.h > @@ -425,7 +425,8 @@ struct pci_dev { > unsigned int ats_enabled:1; /* Address Translation Svc */ > unsigned int pasid_enabled:1; /* Process Address Space ID */ > unsigned int pri_enabled:1; /* Page Request Interface */ > - unsigned int is_managed:1; > + unsigned int is_managed:1; /* Managed via devres */ > + unsigned int is_msi_managed:1; /* MSI release via devres installed */ > unsigned int needs_freset:1; /* Requires fundamental reset */ > unsigned int state_saved:1; > unsigned int is_physfn:1; > -- > 2.34.1 > I do not think this is needed for the stable trees, as it only showed up in the MSI rework that Thomas added for 5.17-rc1. So please drop this from all of the AUTOSEL kernels. thanks, greg k-h ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH AUTOSEL 5.16 36/52] PCI/MSI: Decouple MSI[-X] disable from pcim_release() 2022-01-17 17:08 ` Greg Kroah-Hartman @ 2022-01-22 18:39 ` Sasha Levin 0 siblings, 0 replies; 3+ messages in thread From: Sasha Levin @ 2022-01-22 18:39 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: linux-kernel, stable, Thomas Gleixner, Nishanth Menon, Michael Kelley, bhelgaas, linux-pci On Mon, Jan 17, 2022 at 06:08:34PM +0100, Greg Kroah-Hartman wrote: >On Mon, Jan 17, 2022 at 11:58:37AM -0500, Sasha Levin wrote: >> From: Thomas Gleixner <tglx@linutronix.de> >> >> [ Upstream commit 3f35d2cf9fbc656db82579d849cc69c373b1ad0d ] >> >> The MSI core will introduce runtime allocation of MSI related data. This >> data will be devres managed and has to be set up before enabling >> PCI/MSI[-X]. This would introduce an ordering issue vs. pcim_release(). >> >> The setup order is: >> >> pcim_enable_device() >> devres_alloc(pcim_release...); >> ... >> pci_irq_alloc() >> msi_setup_device_data() >> devres_alloc(msi_device_data_release, ...) >> >> and once the device is released these release functions are invoked in the >> opposite order: >> >> msi_device_data_release() >> ... >> pcim_release() >> pci_disable_msi[x]() >> >> which is obviously wrong, because pci_disable_msi[x]() requires the MSI >> data to be available to tear down the MSI[-X] interrupts. >> >> Remove the MSI[-X] teardown from pcim_release() and add an explicit action >> to be installed on the attempt of enabling PCI/MSI[-X]. >> >> This allows the MSI core data allocation to be ordered correctly in a >> subsequent step. >> >> Reported-by: Nishanth Menon <nm@ti.com> >> Signed-off-by: Thomas Gleixner <tglx@linutronix.de> >> Tested-by: Michael Kelley <mikelley@microsoft.com> >> Tested-by: Nishanth Menon <nm@ti.com> >> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >> Link: https://lore.kernel.org/r/87tuf9rdoj.ffs@tglx >> Signed-off-by: Sasha Levin <sashal@kernel.org> >> --- >> drivers/pci/msi.c | 33 +++++++++++++++++++++++++++++++++ >> drivers/pci/pci.c | 5 ----- >> include/linux/pci.h | 3 ++- >> 3 files changed, 35 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c >> index d84cf30bb2790..1093f099846eb 100644 >> --- a/drivers/pci/msi.c >> +++ b/drivers/pci/msi.c >> @@ -461,6 +461,31 @@ void pci_restore_msi_state(struct pci_dev *dev) >> } >> EXPORT_SYMBOL_GPL(pci_restore_msi_state); >> >> +static void pcim_msi_release(void *pcidev) >> +{ >> + struct pci_dev *dev = pcidev; >> + >> + dev->is_msi_managed = false; >> + pci_free_irq_vectors(dev); >> +} >> + >> +/* >> + * Needs to be separate from pcim_release to prevent an ordering problem >> + * vs. msi_device_data_release() in the MSI core code. >> + */ >> +static int pcim_setup_msi_release(struct pci_dev *dev) >> +{ >> + int ret; >> + >> + if (!pci_is_managed(dev) || dev->is_msi_managed) >> + return 0; >> + >> + ret = devm_add_action(&dev->dev, pcim_msi_release, dev); >> + if (!ret) >> + dev->is_msi_managed = true; >> + return ret; >> +} >> + >> static struct msi_desc * >> msi_setup_entry(struct pci_dev *dev, int nvec, struct irq_affinity *affd) >> { >> @@ -1029,6 +1054,10 @@ static int __pci_enable_msi_range(struct pci_dev *dev, int minvec, int maxvec, >> if (nvec > maxvec) >> nvec = maxvec; >> >> + rc = pcim_setup_msi_release(dev); >> + if (rc) >> + return rc; >> + >> for (;;) { >> if (affd) { >> nvec = irq_calc_affinity_vectors(minvec, nvec, affd); >> @@ -1072,6 +1101,10 @@ static int __pci_enable_msix_range(struct pci_dev *dev, >> if (WARN_ON_ONCE(dev->msix_enabled)) >> return -EINVAL; >> >> + rc = pcim_setup_msi_release(dev); >> + if (rc) >> + return rc; >> + >> for (;;) { >> if (affd) { >> nvec = irq_calc_affinity_vectors(minvec, nvec, affd); >> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c >> index 3d2fb394986a4..f3f606c232a8a 100644 >> --- a/drivers/pci/pci.c >> +++ b/drivers/pci/pci.c >> @@ -2024,11 +2024,6 @@ static void pcim_release(struct device *gendev, void *res) >> struct pci_devres *this = res; >> int i; >> >> - if (dev->msi_enabled) >> - pci_disable_msi(dev); >> - if (dev->msix_enabled) >> - pci_disable_msix(dev); >> - >> for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) >> if (this->region_mask & (1 << i)) >> pci_release_region(dev, i); >> diff --git a/include/linux/pci.h b/include/linux/pci.h >> index 18a75c8e615cd..e26000404e3c3 100644 >> --- a/include/linux/pci.h >> +++ b/include/linux/pci.h >> @@ -425,7 +425,8 @@ struct pci_dev { >> unsigned int ats_enabled:1; /* Address Translation Svc */ >> unsigned int pasid_enabled:1; /* Process Address Space ID */ >> unsigned int pri_enabled:1; /* Page Request Interface */ >> - unsigned int is_managed:1; >> + unsigned int is_managed:1; /* Managed via devres */ >> + unsigned int is_msi_managed:1; /* MSI release via devres installed */ >> unsigned int needs_freset:1; /* Requires fundamental reset */ >> unsigned int state_saved:1; >> unsigned int is_physfn:1; >> -- >> 2.34.1 >> > >I do not think this is needed for the stable trees, as it only showed up >in the MSI rework that Thomas added for 5.17-rc1. So please drop this >from all of the AUTOSEL kernels. Dropped, thanks! -- Thanks, Sasha ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-01-22 18:39 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20220117165853.1470420-1-sashal@kernel.org>
2022-01-17 16:58 ` [PATCH AUTOSEL 5.16 36/52] PCI/MSI: Decouple MSI[-X] disable from pcim_release() Sasha Levin
2022-01-17 17:08 ` Greg Kroah-Hartman
2022-01-22 18:39 ` Sasha Levin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).