On Mon, Apr 14, 2014 at 03:28:35PM +0200, Alexander Gordeev wrote:
> 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.
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:
> 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.
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
> 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).
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().
> 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.
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
> Signed-off-by: Alexander Gordeev <agordeev@xxxxxxxxxx>
> Cc: linux-mips@xxxxxxxxxxxxxx
> Cc: linuxppc-dev@xxxxxxxxxxxxxxxx
> Cc: linux-s390@xxxxxxxxxxxxxxx
> Cc: x86@xxxxxxxxxx
> Cc: linux-pci@xxxxxxxxxxxxxxx
> ---
> drivers/pci/msi.c | 79 +++++++++++++++++++++-----------------------------
> include/linux/pci.h | 5 +--
> 2 files changed, 34 insertions(+), 50 deletions(-)
>
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index 955ab79..fc669ab 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -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) {
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index aab57b4..499755e 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -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 */
> --
> 1.7.7.6
>
> --
> Regards,
> Alexander Gordeev
> agordeev@xxxxxxxxxx
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
