Phase out pci_enable_msi_block()
Subsystems (5)
cover Phase out pci_enable_msi_block()
Hello,
This series is against 3.15-rc1.
This update obsoletes pci_enable_msi_block() function in favor of pci_enable_msi_range() and pci_enable_msi_exact().
Alexander Gordeev (2):
GenWQE: Use pci_enable_msi_exact() instead of pci_enable_msi_block() PCI/MSI: Phase out pci_enable_msi_block()
drivers/misc/genwqe/card_utils.c | 2 +- drivers/pci/msi.c | 79 ++++++++++++++++---------------------- include/linux/pci.h | 5 +-- 3 files changed, 35 insertions(+), 51 deletions(-)
Signature · 1 line
1.7.7.6
PATCH 1/2 GenWQE: Use pci_enable_msi_exact() instead of pci_enable_msi_block()
As result of deprecation of MSI-X/MSI enablement functions pci_enable_msix() and pci_enable_msi_block() all drivers using these two interfaces need to be updated to use the new pci_enable_msi_range() or pci_enable_msi_exact() and pci_enable_msix_range() or pci_enable_msix_exact() interfaces.
--- drivers/misc/genwqe/card_utils.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/misc/genwqe/card_utils.c +1 -1
@@ -628,7 +628,7 @@ int genwqe_set_interrupt_capability(struct genwqe_dev *cd, int count)int rc;struct pci_dev *pci_dev = cd->pci_dev; - rc =pci_enable_msi_block (pci_dev, count); + rc =pci_enable_msi_exact (pci_dev, count);if (rc ==0 ) cd->flags |=GENWQE_FLAG_MSI_ENABLED ;return rc;
Signature · 1 line
1.7.7.6
Re: [PATCH 1/2] GenWQE: Use pci_enable_msi_exact() instead of pci_enable_msi_block()
Quoted 7 lines · Alexander Gordeev
As result of deprecation of MSI-X/MSI enablement functions
pci_enable_msix()andpci_enable_msi_block()all drivers using these two interfaces need to be updated to use the newpci_enable_msi_range()orpci_enable_msi_exact()andpci_enable_msix_range()orpci_enable_msix_exact()interfaces.
Quoted 8 lines
Signed-off-by Alexander Gordeev <[email protected]>Cc Frank Haverkamp <[email protected]>Cc Greg Kroah-Hartman <[email protected]>Acked-by Frank Haverkamp <[email protected]>--- drivers/misc/genwqe/card_utils.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
Bjorn Helgaas
maintainerRe: [PATCH 1/2] GenWQE: Use pci_enable_msi_exact() instead of pci_enable_msi_block()
In reply to Alexander GordeevQuoted 7 lines · Alexander Gordeev
As result of deprecation of MSI-X/MSI enablement functions
pci_enable_msix()andpci_enable_msi_block()all drivers using these two interfaces need to be updated to use the newpci_enable_msi_range()orpci_enable_msi_exact()andpci_enable_msix_range()orpci_enable_msix_exact()interfaces.
Quoted 5 lines
Signed-off-by Alexander Gordeev <[email protected]>Cc Frank Haverkamp <[email protected]>Cc Greg Kroah-Hartman <[email protected]>Acked-by Frank Haverkamp <[email protected]>
Greg, since you acked this rather than saying you applied it, I applied this to my pci/msi branch. If you'd rather take it yourself, just let me know.
Bjorn
Quoted 24 lines
--- drivers/misc/genwqe/card_utils.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)drivers/misc/genwqe/card_utils.c +1
-1@@ -628,7 +628,7 @@ int genwqe_set_interrupt_capability(struct genwqe_dev *cd, int count)int rc;struct pci_dev *pci_dev = cd->pci_dev; - rc =pci_enable_msi_block (pci_dev, count); + rc =pci_enable_msi_exact (pci_dev, count);if (rc ==0 ) cd->flags |=GENWQE_FLAG_MSI_ENABLED ;return rc;Signature · 6 lines
1.7.7.6 -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Greg Kroah-Hartman
maintainerRe: [PATCH 1/2] GenWQE: Use pci_enable_msi_exact() instead of pci_enable_msi_block()
Quoted 14 lines · Bjorn Helgaas
Quoted 7 lines · Alexander Gordeev
As result of deprecation of MSI-X/MSI enablement functions
pci_enable_msix()andpci_enable_msi_block()all drivers using these two interfaces need to be updated to use the newpci_enable_msi_range()orpci_enable_msi_exact()andpci_enable_msix_range()orpci_enable_msix_exact()interfaces.Quoted 5 lines
Signed-off-by Alexander Gordeev <[email protected]>Cc Frank Haverkamp <[email protected]>Cc Greg Kroah-Hartman <[email protected]>Acked-by Frank Haverkamp <[email protected]>
Quoted 3 lines
Greg, since you acked this rather than saying you applied it, I applied this to my pci/msi branch. If you'd rather take it yourself, just let me know.
Nope, through your tree is fine, I have enough other patches in my trees :)
greg k-h
PATCH 2/2 PCI/MSI: Phase out pci_enable_msi_block()
In reply to Alexander GordeevThere are no users of pci_enable_msi_block() function have left. Obsolete it in favor of pci_enable_msi_range() and pci_enable_msi_exact() functions.
Up until now, when enabling MSI mode for a device, a single successful call to arch_msi_check_device() was followed by a single call to arch_setup_msi_irqs() function.
Yet, if arch_msi_check_device() returned success we should be able to call arch_setup_msi_irqs() multiple times - while it returns a number of MSI vectors that could have been allocated (a third state).
This update makes use of the assumption described above. It could have broke things had the architectures done any pre- allocations or switch to some state in a call to function arch_msi_check_device(). But because arch_msi_check_device() is expected stateless and MSI resources are allocated in a follow-up call to arch_setup_msi_irqs() we should be fine.
--- drivers/pci/msi.c | 79 +++++++++++++++++++++----------------------------- include/linux/pci.h | 5 +-- 2 files changed, 34 insertions(+), 50 deletions(-)
drivers/pci/msi.c +33 -46
@@ -883,50 +883,6 @@ int pci_msi_vec_count(struct pci_dev *dev) }EXPORT_SYMBOL (pci_msi_vec_count); -/** -* pci_enable_msi_block - configure device's MSI capability structure -* @dev: device to configure -* @nvec: number of interrupts to configure -* -* Allocate IRQs for a device with the MSI capability. -* This function returns a negative errno if an error occurs. If it -* is unable to allocate the number of interrupts requested, it returns -* the number of interrupts it might be able to allocate. If it successfully -* allocates at least the number of interrupts requested, it returns 0 and -* updates the @dev's irq member to the lowest new interrupt number; the -* other interrupt numbers allocated to this device are consecutive. -*/ -int pci_enable_msi_block (struct pci_dev *dev,int nvec) -{ -int status, maxvec; - -if (dev->current_state !=PCI_D0 ) -return -EINVAL ; - - maxvec =pci_msi_vec_count (dev); -if (maxvec <0 ) -return maxvec; -if (nvec > maxvec) -return maxvec; - - status =pci_msi_check_device (dev, nvec,PCI_CAP_ID_MSI ); -if (status) -return status; - -WARN_ON (!!dev->msi_enabled); - -/* Check whether driver already requested MSI-X irqs */ -if (dev->msix_enabled) { -dev_info (&dev->dev,"can't enable MSI " -"(MSI-X already enabled)\n" ); -return -EINVAL ; - } - - status =msi_capability_init (dev, nvec); -return status; -} -EXPORT_SYMBOL (pci_enable_msi_block); -void pci_msi_shutdown (struct pci_dev *dev) {struct msi_desc *desc;
@@ -1132,14 +1088,45 @@ void pci_msi_init_pci_dev(struct pci_dev *dev)**/ int pci_enable_msi_range (struct pci_dev *dev,int minvec,int maxvec) { -int nvec = maxvec; +int nvec;int rc; +if (dev->current_state !=PCI_D0 ) +return -EINVAL ; + +WARN_ON (!!dev->msi_enabled); + +/* Check whether driver already requested MSI-X irqs */ +if (dev->msix_enabled) { +dev_info (&dev->dev, +"can't enable MSI (MSI-X already enabled)\n" ); +return -EINVAL ; + } +if (maxvec < minvec)return -ERANGE ; + nvec =pci_msi_vec_count (dev); +if (nvec <0 ) +return nvec; +else if (nvec < minvec) +return -EINVAL ; +else if (nvec > maxvec) + nvec = maxvec; + +do { + rc =pci_msi_check_device (dev, nvec,PCI_CAP_ID_MSI ); +if (rc <0 ) { +return rc; + }else if (rc >0 ) { +if (rc < minvec) +return -ENOSPC ; + nvec = rc; + } + }while (rc); + do { - rc = pci_enable_msi_block(dev, nvec); + rc = msi_capability_init(dev, nvec); if (rc < 0) { return rc; } else if (rc > 0) {
include/linux/pci.h +1 -4
@@ -1158,7 +1158,6 @@ struct msix_entry {#ifdef CONFIG_PCI_MSI int pci_msi_vec_count (struct pci_dev *dev); -int pci_enable_msi_block (struct pci_dev *dev,int nvec);void pci_msi_shutdown (struct pci_dev *dev);void pci_disable_msi (struct pci_dev *dev);int pci_msix_vec_count (struct pci_dev *dev);
@@ -1188,8 +1187,6 @@ static inline int pci_enable_msix_exact(struct pci_dev *dev, }#else static inline int pci_msi_vec_count (struct pci_dev *dev) {return -ENOSYS ; } -static inline int pci_enable_msi_block (struct pci_dev *dev,int nvec) -{return -ENOSYS ; }static inline void pci_msi_shutdown (struct pci_dev *dev) { }static inline void pci_disable_msi (struct pci_dev *dev) { }static inline int pci_msix_vec_count (struct pci_dev *dev) {return -ENOSYS ; }
@@ -1244,7 +1241,7 @@ static inline void pcie_set_ecrc_checking(struct pci_dev *dev) { }static inline void pcie_ecrc_get_policy (char *str) { }#endif -#define pci_enable_msi (pdev) pci_enable_msi_block(pdev, 1) +#define pci_enable_msi (pdev) pci_enable_msi_range(pdev, 1, 1)#ifdef CONFIG_HT_IRQ /* The functions a driver should call */
Signature · 1 line
1.7.7.6
Re: [PATCH 2/2] PCI/MSI: Phase out pci_enable_msi_block()
include/linux/pci.h static inline void pcie_set_ecrc_checking(struct pci_dev *dev) { } · line 1244
Quoted 6 lines · Alexander Gordeev
include/linux/pci.h +1
-1@@ -1244,7 +1241,7 @@ static inline void pcie_set_ecrc_checking(struct pci_dev *dev) { }static inline void pcie_ecrc_get_policy (char *str) { }#endif -#define pci_enable_msi (pdev) pci_enable_msi_block(pdev, 1) +#define pci_enable_msi (pdev) pci_enable_msi_range(pdev, 1, 1)
Self-Nack, it should have been pci_enable_msi_exact(pdev, 1) ^^^
Quoted 2 lines
include/linux/pci.h +0
-0#ifdef CONFIG_HT_IRQ /* The functions a driver should call */
Signature · 3 lines
Regards, Alexander Gordeev [email protected]
PATCH 2/2 PCI/MSI: Phase out pci_enable_msi_block()
There are no users of pci_enable_msi_block() function have left. Obsolete it in favor of pci_enable_msi_range() and pci_enable_msi_exact() functions.
Up until now, when enabling MSI mode for a device a single successful call to arch_msi_check_device() was followed by a single call to arch_setup_msi_irqs() function.
Yet, if arch_msi_check_device() returned success we should be able to call arch_setup_msi_irqs() multiple times - while it returns a number of MSI vectors that could have been allocated (a third state).
This update makes use of the assumption described above. It could have broke things had the architectures done any pre- allocations or switch to some state in a call to function arch_msi_check_device(). But because arch_msi_check_device() is expected stateless and MSI resources are allocated in a follow-up call to arch_setup_msi_irqs() we should be fine.
--- drivers/pci/msi.c | 79 +++++++++++++++++++++----------------------------- include/linux/pci.h | 5 +-- 2 files changed, 34 insertions(+), 50 deletions(-)
drivers/pci/msi.c +33 -46
@@ -883,50 +883,6 @@ int pci_msi_vec_count(struct pci_dev *dev) }EXPORT_SYMBOL (pci_msi_vec_count); -/** -* pci_enable_msi_block - configure device's MSI capability structure -* @dev: device to configure -* @nvec: number of interrupts to configure -* -* Allocate IRQs for a device with the MSI capability. -* This function returns a negative errno if an error occurs. If it -* is unable to allocate the number of interrupts requested, it returns -* the number of interrupts it might be able to allocate. If it successfully -* allocates at least the number of interrupts requested, it returns 0 and -* updates the @dev's irq member to the lowest new interrupt number; the -* other interrupt numbers allocated to this device are consecutive. -*/ -int pci_enable_msi_block (struct pci_dev *dev,int nvec) -{ -int status, maxvec; - -if (dev->current_state !=PCI_D0 ) -return -EINVAL ; - - maxvec =pci_msi_vec_count (dev); -if (maxvec <0 ) -return maxvec; -if (nvec > maxvec) -return maxvec; - - status =pci_msi_check_device (dev, nvec,PCI_CAP_ID_MSI ); -if (status) -return status; - -WARN_ON (!!dev->msi_enabled); - -/* Check whether driver already requested MSI-X irqs */ -if (dev->msix_enabled) { -dev_info (&dev->dev,"can't enable MSI " -"(MSI-X already enabled)\n" ); -return -EINVAL ; - } - - status =msi_capability_init (dev, nvec); -return status; -} -EXPORT_SYMBOL (pci_enable_msi_block); -void pci_msi_shutdown (struct pci_dev *dev) {struct msi_desc *desc;
@@ -1132,14 +1088,45 @@ void pci_msi_init_pci_dev(struct pci_dev *dev)**/ int pci_enable_msi_range (struct pci_dev *dev,int minvec,int maxvec) { -int nvec = maxvec; +int nvec;int rc; +if (dev->current_state !=PCI_D0 ) +return -EINVAL ; + +WARN_ON (!!dev->msi_enabled); + +/* Check whether driver already requested MSI-X irqs */ +if (dev->msix_enabled) { +dev_info (&dev->dev, +"can't enable MSI (MSI-X already enabled)\n" ); +return -EINVAL ; + } +if (maxvec < minvec)return -ERANGE ; + nvec =pci_msi_vec_count (dev); +if (nvec <0 ) +return nvec; +else if (nvec < minvec) +return -EINVAL ; +else if (nvec > maxvec) + nvec = maxvec; + +do { + rc =pci_msi_check_device (dev, nvec,PCI_CAP_ID_MSI ); +if (rc <0 ) { +return rc; + }else if (rc >0 ) { +if (rc < minvec) +return -ENOSPC ; + nvec = rc; + } + }while (rc); + do { - rc = pci_enable_msi_block(dev, nvec); + rc = msi_capability_init(dev, nvec); if (rc < 0) { return rc; } else if (rc > 0) {
include/linux/pci.h +1 -4
@@ -1158,7 +1158,6 @@ struct msix_entry {#ifdef CONFIG_PCI_MSI int pci_msi_vec_count (struct pci_dev *dev); -int pci_enable_msi_block (struct pci_dev *dev,int nvec);void pci_msi_shutdown (struct pci_dev *dev);void pci_disable_msi (struct pci_dev *dev);int pci_msix_vec_count (struct pci_dev *dev);
@@ -1188,8 +1187,6 @@ static inline int pci_enable_msix_exact(struct pci_dev *dev, }#else static inline int pci_msi_vec_count (struct pci_dev *dev) {return -ENOSYS ; } -static inline int pci_enable_msi_block (struct pci_dev *dev,int nvec) -{return -ENOSYS ; }static inline void pci_msi_shutdown (struct pci_dev *dev) { }static inline void pci_disable_msi (struct pci_dev *dev) { }static inline int pci_msix_vec_count (struct pci_dev *dev) {return -ENOSYS ; }
@@ -1244,7 +1241,7 @@ static inline void pcie_set_ecrc_checking(struct pci_dev *dev) { }static inline void pcie_ecrc_get_policy (char *str) { }#endif -#define pci_enable_msi (pdev) pci_enable_msi_block(pdev, 1) +#define pci_enable_msi (pdev) pci_enable_msi_exact(pdev, 1)#ifdef CONFIG_HT_IRQ /* The functions a driver should call */
Signature · 6 lines
1.7.7.6 -- Regards, Alexander Gordeev [email protected]
Re: [PATCH 2/2] PCI/MSI: Phase out pci_enable_msi_block()
Quoted 3 lines · Alexander Gordeev
There are no users of
pci_enable_msi_block()function have left. Obsolete it in favor ofpci_enable_msi_range()andpci_enable_msi_exact()functions.
Hi Bjorn,
How about this one?
Thanks!
Signature · 3 lines
Regards, Alexander Gordeev [email protected]
Bjorn Helgaas
maintainerRe: [PATCH 2/2] PCI/MSI: Phase out pci_enable_msi_block()
In reply to Alexander GordeevQuoted 3 lines · Alexander Gordeev
There are no users of
pci_enable_msi_block()function have left. Obsolete it in favor ofpci_enable_msi_range()andpci_enable_msi_exact()functions.
I mistakenly assumed this would have to wait because I thought there were other pci_enable_msi_block() users that wouldn't be removed until the v3.16 merge window. But I think I was wrong: I put your GenWQE patch in my tree, and I think that was the last use, so I can just add this patch on top.
But I need a little help understanding the changelog:
Quoted 3 lines
Up until now, when enabling MSI mode for a device a single successful call to
arch_msi_check_device()was followed by a single call toarch_setup_msi_irqs()function.
I understand this part; the following two paths call arch_msi_check_device() once and then arch_setup_msi_irqs() once:
pci_enable_msi_block
pci_msi_check_device
arch_msi_check_device
msi_capability_init
arch_setup_msi_irqs
pci_enable_msix
pci_msi_check_device
arch_msi_check_device
msix_capability_init
arch_setup_msi_irqs
Quoted 4 lines
Yet, if
arch_msi_check_device()returned success we should be able to callarch_setup_msi_irqs()multiple times - while it returns a number of MSI vectors that could have been allocated (a third state).
I don't know what you mean by "a third state."
Previously we only called arch_msi_check_device() once. After your patch, pci_enable_msi_range() can call it several times. The only non-trivial implementation of arch_msi_check_device() is in powerpc, and all the ppc_md.msi_check_device() possibilities look safe to call multiple times.
After your patch, the pci_enable_msi_range() path can also call arch_setup_msi_irqs() several times. I don't see a problem with that -- even if the first call succeeds and allocates something, then a subsequent call fails, I assume the allocations will be cleaned up when msi_capability_init() calls free_msi_irqs().
Quoted 6 lines
This update makes use of the assumption described above. It could have broke things had the architectures done any pre- allocations or switch to some state in a call to function
arch_msi_check_device(). But becausearch_msi_check_device()is expected stateless and MSI resources are allocated in a follow-up call toarch_setup_msi_irqs()we should be fine.
I guess you mean that your patch assumes arch_msi_check_device() is stateless. That looks like a safe assumption to me.
arch_setup_msi_irqs() is clearly not stateless, but I assume free_msi_irqs() is enough to clean up any state if we fail.
Bjorn
Quoted 155 lines
Signed-off-by Alexander Gordeev <[email protected]>--- drivers/pci/msi.c | 79 +++++++++++++++++++++----------------------------- include/linux/pci.h | 5 +-- 2 files changed, 34 insertions(+), 50 deletions(-)drivers/pci/msi.c +33
-46@@ -883,50 +883,6 @@ int pci_msi_vec_count(struct pci_dev *dev) }EXPORT_SYMBOL (pci_msi_vec_count); -/** -* pci_enable_msi_block - configure device's MSI capability structure -* @dev: device to configure -* @nvec: number of interrupts to configure -* -* Allocate IRQs for a device with the MSI capability. -* This function returns a negative errno if an error occurs. If it -* is unable to allocate the number of interrupts requested, it returns -* the number of interrupts it might be able to allocate. If it successfully -* allocates at least the number of interrupts requested, it returns 0 and -* updates the @dev's irq member to the lowest new interrupt number; the -* other interrupt numbers allocated to this device are consecutive. -*/ -int pci_enable_msi_block (struct pci_dev *dev,int nvec) -{ -int status, maxvec; - -if (dev->current_state !=PCI_D0 ) -return -EINVAL ; - - maxvec =pci_msi_vec_count (dev); -if (maxvec <0 ) -return maxvec; -if (nvec > maxvec) -return maxvec; - - status =pci_msi_check_device (dev, nvec,PCI_CAP_ID_MSI ); -if (status) -return status; - -WARN_ON (!!dev->msi_enabled); - -/* Check whether driver already requested MSI-X irqs */ -if (dev->msix_enabled) { -dev_info (&dev->dev,"can't enable MSI " -"(MSI-X already enabled)\n" ); -return -EINVAL ; - } - - status =msi_capability_init (dev, nvec); -return status; -} -EXPORT_SYMBOL (pci_enable_msi_block); -void pci_msi_shutdown (struct pci_dev *dev) {struct msi_desc *desc;@@ -1132,14 +1088,45 @@ void pci_msi_init_pci_dev(struct pci_dev *dev)**/ int pci_enable_msi_range (struct pci_dev *dev,int minvec,int maxvec) { -int nvec = maxvec; +int nvec;int rc; +if (dev->current_state !=PCI_D0 ) +return -EINVAL ; + +WARN_ON (!!dev->msi_enabled); + +/* Check whether driver already requested MSI-X irqs */ +if (dev->msix_enabled) { +dev_info (&dev->dev, +"can't enable MSI (MSI-X already enabled)\n" ); +return -EINVAL ; + } +if (maxvec < minvec)return -ERANGE ; + nvec =pci_msi_vec_count (dev); +if (nvec <0 ) +return nvec; +else if (nvec < minvec) +return -EINVAL ; +else if (nvec > maxvec) + nvec = maxvec; + +do { + rc =pci_msi_check_device (dev, nvec,PCI_CAP_ID_MSI ); +if (rc <0 ) { +return rc; + }else if (rc >0 ) { +if (rc < minvec) +return -ENOSPC ; + nvec = rc; + } + }while (rc); + do { - rc = pci_enable_msi_block(dev, nvec); + rc = msi_capability_init(dev, nvec); if (rc < 0) { return rc; } else if (rc > 0) {include/linux/pci.h +1
-4@@ -1158,7 +1158,6 @@ struct msix_entry {#ifdef CONFIG_PCI_MSI int pci_msi_vec_count (struct pci_dev *dev); -int pci_enable_msi_block (struct pci_dev *dev,int nvec);void pci_msi_shutdown (struct pci_dev *dev);void pci_disable_msi (struct pci_dev *dev);int pci_msix_vec_count (struct pci_dev *dev);@@ -1188,8 +1187,6 @@ static inline int pci_enable_msix_exact(struct pci_dev *dev, }#else static inline int pci_msi_vec_count (struct pci_dev *dev) {return -ENOSYS ; } -static inline int pci_enable_msi_block (struct pci_dev *dev,int nvec) -{return -ENOSYS ; }static inline void pci_msi_shutdown (struct pci_dev *dev) { }static inline void pci_disable_msi (struct pci_dev *dev) { }static inline int pci_msix_vec_count (struct pci_dev *dev) {return -ENOSYS ; }@@ -1244,7 +1241,7 @@ static inline void pcie_set_ecrc_checking(struct pci_dev *dev) { }static inline void pcie_ecrc_get_policy (char *str) { }#endif -#define pci_enable_msi (pdev) pci_enable_msi_block(pdev, 1) +#define pci_enable_msi (pdev) pci_enable_msi_exact(pdev, 1)#ifdef CONFIG_HT_IRQ /* The functions a driver should call */ Signature · 10 lines
1.7.7.6 -- Regards, Alexander Gordeev [email protected] -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] PCI/MSI: Phase out pci_enable_msi_block()
Quoted 4 lines · Bjorn Helgaas
I mistakenly assumed this would have to wait because I thought there were other
pci_enable_msi_block()users that wouldn't be removed until the v3.16 merge window. But I think I was wrong: I put your GenWQE patch in my tree, and I think that was the last use, so I can just add this patch on top.
That is right, GenWQE is the last one.
Quoted 21 lines
But I need a little help understanding the changelog:
Quoted 3 lines
Up until now, when enabling MSI mode for a device a single successful call to
arch_msi_check_device()was followed by a single call toarch_setup_msi_irqs()function.I understand this part; the following two paths call
arch_msi_check_device()once and thenarch_setup_msi_irqs()once:
pci_enable_msi_block
pci_msi_check_device
arch_msi_check_device
msi_capability_init
arch_setup_msi_irqs
pci_enable_msix
pci_msi_check_device
arch_msi_check_device
msix_capability_init
arch_setup_msi_irqs
Quoted 6 lines
Quoted 4 lines
Yet, if
arch_msi_check_device()returned success we should be able to callarch_setup_msi_irqs()multiple times - while it returns a number of MSI vectors that could have been allocated (a third state).I don't know what you mean by "a third state."
That is "a number of MSI vectors that could have been allocated", which is neither success nor failure. In previous conversations someone branded it this as "third state".
Quoted 4 lines
Previously we only called
arch_msi_check_device()once. After your patch,pci_enable_msi_range()can call it several times. The only non-trivial implementation ofarch_msi_check_device()is in powerpc, and all theppc_md.msi_check_device()possibilities look safe to call multiple times.
Yep, I see it the same way.
Quoted 5 lines
After your patch, the
pci_enable_msi_range()path can also callarch_setup_msi_irqs()several times. I don't see a problem with that -- even if the first call succeeds and allocates something, then a subsequent call fails, I assume the allocations will be cleaned up whenmsi_capability_init()callsfree_msi_irqs().
Well, the potential problem related to the fact arch_msi_check_device() could be called with 'nvec1' while arch_setup_msi_irqs() could be called with 'nvec2', where 'nvec1' > 'nvec2'.
While it is not a problem with current implementations, in therory it is possible free_msi_irqs() could be called with 'nvec2' and fail to clean up 'nvec1' - 'nvec2' number of resources.
The only assumption that makes the above scenario impossible is if arch_msi_check_device() is stateless.
Again, that is purely theoretical with no particular architecture in mind.
Quoted 9 lines
Quoted 6 lines
This update makes use of the assumption described above. It could have broke things had the architectures done any pre- allocations or switch to some state in a call to function
arch_msi_check_device(). But becausearch_msi_check_device()is expected stateless and MSI resources are allocated in a follow-up call toarch_setup_msi_irqs()we should be fine.I guess you mean that your patch assumes
arch_msi_check_device()is stateless. That looks like a safe assumption to me.
Moreover, if arch_msi_check_device() was not stateless then it would be superfluous, since all state switches and allocations are done in arch_setup_msi_irqs() anyway.
In fact, I think arch_msi_check_device() could be eliminated, but I do not want to engage with PPC folks at this stage ;)
Quoted 4 lines
arch_setup_msi_irqs()is clearly not stateless, but I assumefree_msi_irqs()is enough to clean up any state if we fail.Bjorn
Signature · 3 lines
Regards, Alexander Gordeev [email protected]