* [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).