Re: [PATCH 2/2] PCI/MSI: Phase out pci_enable_msi_block()

[Bjorn Helgaas <bhelgaas@google.com>]

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@redhat.com>
> Cc: linux-mips@linux-mips.org
> Cc: linuxppc-dev@lists.ozlabs.org
> Cc: linux-s390@vger.kernel.org
> Cc: x86@kernel.org
> Cc: linux-pci@vger.kernel.org
> ---
>  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@redhat.com
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html