Message-ID: <20131210231602.GH4699@google.com>
Date:	Tue, 10 Dec 2013 16:16:02 -0700
From:	Bjorn Helgaas <bhelgaas@...gle.com>
To:	Alexander Gordeev <agordeev@...hat.com>
Cc:	linux-kernel@...r.kernel.org,
	Michael Ellerman <michael@...erman.id.au>,
	Benjamin Herrenschmidt <benh@...nel.crashing.org>,
	Tejun Heo <tj@...nel.org>,
	Ben Hutchings <bhutchings@...arflare.com>,
	David Laight <David.Laight@...LAB.COM>,
	Mark Lord <kernel@...rt.ca>, "H. Peter Anvin" <hpa@...or.com>,
	linux-pci@...r.kernel.org
Subject: Re: [PATCH v3 11/11] PCI/MSI: Introduce pcim_enable_msi*() family
 helpers

On Tue, Nov 26, 2013 at 10:10:00AM +0100, Alexander Gordeev wrote:
> Currently many device drivers need contiguously call functions
> pci_enable_msix() for MSI-X or pci_enable_msi_block() for MSI
> in a loop until success or failure. This update generalizes
> this usage pattern and introduces pcim_enable_msi*() family
> helpers.

What's the reason for using the "pcim_" prefix?  To me, that suggests that
this is a "managed" interface in the sense described in
Documentation/driver-model/devres.txt, where resources are automatically
deallocated when the driver detaches.  But I don't see anything like that
happening in this patch.

> As result, device drivers do not have to deal with tri-state
> return values from pci_enable_msix() and pci_enable_msi_block()
> functions directly and expected to have more clearer and straight
> code.
> 
> So i.e. the request loop described in the documentation...
> 
> 	int foo_driver_enable_msix(struct foo_adapter *adapter,
> 				   int nvec)
> 	{
> 		while (nvec >= FOO_DRIVER_MINIMUM_NVEC) {
> 			rc = pci_enable_msix(adapter->pdev,
> 					     adapter->msix_entries,
> 					     nvec);
> 			if (rc > 0)
> 				nvec = rc;
> 			else
> 				return rc;
> 		}
> 
> 		return -ENOSPC;
> 	}
> 
> ...would turn into a single helper call....
> 
> 	rc = pcim_enable_msix_range(adapter->pdev,
> 				    adapter->msix_entries,
> 				    FOO_DRIVER_MINIMUM_NVEC,
> 				    nvec);
> 
> Device drivers with more specific requirements (i.e. a number of
> MSI-Xs which is a multiple of a certain number within a specified
> range) would still need to implement the loop using the two old
> functions.
> 
> Signed-off-by: Alexander Gordeev <agordeev@...hat.com>
> Suggested-by: Ben Hutchings <bhutchings@...arflare.com>
> Reviewed-by: Tejun Heo <tj@...nel.org>
> ---
>  Documentation/PCI/MSI-HOWTO.txt |  134 +++++++++++++++++++++++++++++++++++++--
>  drivers/pci/msi.c               |   74 +++++++++++++++++++++
>  include/linux/pci.h             |   55 ++++++++++++++++
>  3 files changed, 258 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/PCI/MSI-HOWTO.txt b/Documentation/PCI/MSI-HOWTO.txt
> index 5955389..044f9cd 100644
> --- a/Documentation/PCI/MSI-HOWTO.txt
> +++ b/Documentation/PCI/MSI-HOWTO.txt
> @@ -127,7 +127,62 @@ on the number of vectors that can be allocated; pci_enable_msi_block()
>  returns as soon as it finds any constraint that doesn't allow the
>  call to succeed.
>  
> -4.2.3 pci_disable_msi
> +4.2.3 pcim_enable_msi_range
> +
> +int pcim_enable_msi_range(struct pci_dev *dev, struct msix_entry *entries,
> +			  int minvec, int maxvec)
> +
> +This variation on pci_enable_msi_block() call allows a device driver to
> +request any number of MSIs within specified range 'minvec' to 'maxvec'.
> +Whenever possible device drivers are encouraged to use this function
> +rather than explicit request loop calling pci_enable_msi_block().
> +
> +If this function returns a negative number, it indicates an error and
> +the driver should not attempt to request any more MSI interrupts for
> +this device.
> +
> +If this function returns a positive number it indicates at least the
> +returned number of MSI interrupts have been successfully allocated (it may
> +have allocated more in order to satisfy the power-of-two requirement).
> +Device drivers can use this number to further initialize devices.
> +
> +4.2.4 pcim_enable_msi
> +
> +int pcim_enable_msi(struct pci_dev *dev,
> +		    struct msix_entry *entries, int maxvec)
> +
> +This variation on pci_enable_msi_block() call allows a device driver to
> +request any number of MSIs up to 'maxvec'. Whenever possible device drivers
> +are encouraged to use this function rather than explicit request loop
> +calling pci_enable_msi_block().
> +
> +If this function returns a negative number, it indicates an error and
> +the driver should not attempt to request any more MSI interrupts for
> +this device.
> +
> +If this function returns a positive number it indicates at least the
> +returned number of MSI interrupts have been successfully allocated (it may
> +have allocated more in order to satisfy the power-of-two requirement).
> +Device drivers can use this number to further initialize devices.
> +
> +4.2.5 pcim_enable_msi_exact
> +
> +int pcim_enable_msi_exact(struct pci_dev *dev,
> +			  struct msix_entry *entries, int nvec)
> +
> +This variation on pci_enable_msi_block() call allows a device driver to
> +request exactly 'nvec' MSIs.
> +
> +If this function returns a negative number, it indicates an error and
> +the driver should not attempt to request any more MSI interrupts for
> +this device.
> +
> +If this function returns the value of 'nvec' it indicates MSI interrupts
> +have been successfully allocated. No other value in case of success could
> +be returned. Device drivers can use this value to further allocate and
> +initialize device resources.
> +
> +4.2.6 pci_disable_msi
>  
>  void pci_disable_msi(struct pci_dev *dev)
>  
> @@ -142,7 +197,7 @@ on any interrupt for which it previously called request_irq().
>  Failure to do so results in a BUG_ON(), leaving the device with
>  MSI enabled and thus leaking its vector.
>  
> -4.2.4 pci_get_msi_cap
> +4.2.7 pci_get_msi_cap
>  
>  int pci_get_msi_cap(struct pci_dev *dev)
>  
> @@ -222,7 +277,76 @@ static int foo_driver_enable_msix(struct foo_adapter *adapter, int nvec)
>  	return -ENOSPC;
>  }
>  
> -4.3.2 pci_disable_msix
> +4.3.2 pcim_enable_msix_range
> +
> +int pcim_enable_msix_range(struct pci_dev *dev, struct msix_entry *entries,
> +			   int minvec, int maxvec)
> +
> +This variation on pci_enable_msix() call allows a device driver to request
> +any number of MSI-Xs within specified range 'minvec' to 'maxvec'. Whenever
> +possible device drivers are encouraged to use this function rather than
> +explicit request loop calling pci_enable_msix().
> +
> +If this function returns a negative number, it indicates an error and
> +the driver should not attempt to allocate any more MSI-X interrupts for
> +this device.
> +
> +If this function returns a positive number it indicates the number of
> +MSI-X interrupts that have been successfully allocated. Device drivers
> +can use this number to further allocate and initialize device resources.
> +
> +A modified function calling pci_enable_msix() in a loop might look like:
> +
> +static int foo_driver_enable_msix(struct foo_adapter *adapter, int nvec)
> +{
> +	rc = pcim_enable_msix_range(adapter->pdev, adapter->msix_entries,
> +				    FOO_DRIVER_MINIMUM_NVEC, nvec);
> +	if (rc < 0)
> +		return rc;
> +
> +	rc = foo_driver_init_other(adapter, rc);
> +	if (rc < 0)
> +		pci_disable_msix(adapter->pdev);
> +
> +	return rc;
> +}
> +
> +4.3.3 pcim_enable_msix
> +
> +int pcim_enable_msix(struct pci_dev *dev,
> +		     struct msix_entry *entries, int maxvec)
> +
> +This variation on pci_enable_msix() call allows a device driver to request
> +any number of MSI-Xs up to 'maxvec'. Whenever possible device drivers are
> +encouraged to use this function rather than explicit request loop calling
> +pci_enable_msix().
> +
> +If this function returns a negative number, it indicates an error and
> +the driver should not attempt to allocate any more MSI-X interrupts for
> +this device.
> +
> +If this function returns a positive number it indicates the number of
> +MSI-X interrupts that have been successfully allocated. Device drivers
> +can use this number to further allocate and initialize device resources.
> +
> +4.3.4 pcim_enable_msix_exact
> +
> +int pcim_enable_msix_exact(struct pci_dev *dev,
> +			   struct msix_entry *entries, int nvec)
> +
> +This variation on pci_enable_msix() call allows a device driver to request
> +exactly 'nvec' MSI-Xs.
> +
> +If this function returns a negative number, it indicates an error and
> +the driver should not attempt to allocate any more MSI-X interrupts for
> +this device.
> +
> +If this function returns the value of 'nvec' it indicates MSI-X interrupts
> +have been successfully allocated. No other value in case of success could
> +be returned. Device drivers can use this value to further allocate and
> +initialize device resources.
> +
> +4.3.5 pci_disable_msix
>  
>  void pci_disable_msix(struct pci_dev *dev)
>  
> @@ -236,14 +360,14 @@ on any interrupt for which it previously called request_irq().
>  Failure to do so results in a BUG_ON(), leaving the device with
>  MSI-X enabled and thus leaking its vector.
>  
> -4.3.3 The MSI-X Table
> +4.3.6 The MSI-X Table
>  
>  The MSI-X capability specifies a BAR and offset within that BAR for the
>  MSI-X Table.  This address is mapped by the PCI subsystem, and should not
>  be accessed directly by the device driver.  If the driver wishes to
>  mask or unmask an interrupt, it should call disable_irq() / enable_irq().
>  
> -4.3.4 pci_msix_table_size
> +4.3.7 pci_msix_table_size
>  
>  int pci_msix_table_size(struct pci_dev *dev)
>  
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index 6fe0add..c5fb4fb 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -1086,3 +1086,77 @@ void pci_msi_init_pci_dev(struct pci_dev *dev)
>  	if (dev->msix_cap)
>  		msix_set_enable(dev, 0);
>  }
> +
> +/**
> + * pcim_enable_msi_range - configure device's MSI capability structure
> + * @dev: device to configure
> + * @minvec: minimal number of interrupts to configure
> + * @maxvec: maximum number of interrupts to configure
> + *
> + * This function tries to allocate a maximum possible number of interrupts in a
> + * range between @minvec and @maxvec. It returns a negative errno if an error
> + * occurs. If it succeeds, it returns the actual number of interrupts allocated
> + * and updates the @dev's irq member to the lowest new interrupt number;
> + * the other interrupt numbers allocated to this device are consecutive.
> + **/
> +int pcim_enable_msi_range(struct pci_dev *dev, int minvec, int maxvec)
> +{
> +	int nvec = maxvec;
> +	int rc;
> +
> +	if (maxvec < minvec)
> +		return -ERANGE;
> +
> +	do {
> +		rc = pci_enable_msi_block(dev, nvec);
> +		if (rc < 0) {
> +			return rc;
> +		} else if (rc > 0) {
> +			if (rc < minvec)
> +				return -ENOSPC;
> +			nvec = rc;
> +		}
> +	} while (rc);
> +
> +	return nvec;
> +}
> +EXPORT_SYMBOL(pcim_enable_msi_range);
> +
> +/**
> + * pcim_enable_msix_range - configure device's MSI-X capability structure
> + * @dev: pointer to the pci_dev data structure of MSI-X device function
> + * @entries: pointer to an array of MSI-X entries
> + * @minvec: minimum number of MSI-X irqs requested
> + * @maxvec: maximum number of MSI-X irqs requested
> + *
> + * Setup the MSI-X capability structure of device function with a maximum
> + * possible number of interrupts in the range between @minvec and @maxvec
> + * upon its software driver call to request for MSI-X mode enabled on its
> + * hardware device function. It returns a negative errno if an error occurs.
> + * If it succeeds, it returns the actual number of interrupts allocated and
> + * indicates the successful configuration of MSI-X capability structure
> + * with new allocated MSI-X interrupts.
> + **/
> +int pcim_enable_msix_range(struct pci_dev *dev, struct msix_entry *entries,
> +			   int minvec, int maxvec)
> +{
> +	int nvec = maxvec;
> +	int rc;
> +
> +	if (maxvec < minvec)
> +		return -ERANGE;
> +
> +	do {
> +		rc = pci_enable_msix(dev, entries, nvec);
> +		if (rc < 0) {
> +			return rc;
> +		} else if (rc > 0) {
> +			if (rc < minvec)
> +				return -ENOSPC;
> +			nvec = rc;
> +		}
> +	} while (rc);
> +
> +	return nvec;
> +}
> +EXPORT_SYMBOL(pcim_enable_msix_range);
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 8af1217..4e02d88 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1194,6 +1194,37 @@ static inline int pci_msi_enabled(void)
>  {
>  	return 0;
>  }
> +
> +int pcim_enable_msi_range(struct pci_dev *dev, int minvec, int maxvec)
> +{
> +	return -ENOSYS;
> +}
> +static inline int pcim_enable_msi(struct pci_dev *dev, int maxvec)
> +{
> +	return -ENOSYS;
> +}
> +static inline int pcim_enable_msi_exact(struct pci_dev *dev, int nvec)
> +{
> +	return -ENOSYS;
> +}
> +
> +static inline int
> +pcim_enable_msix_range(struct pci_dev *dev, struct msix_entry *entries,
> +		       int minvec, int maxvec)
> +{
> +	return -ENOSYS;
> +}
> +static inline int
> +pcim_enable_msix(struct pci_dev *dev, struct msix_entry *entries, int maxvec)
> +{
> +	return -ENOSYS;
> +}
> +static inline int
> +pcim_enable_msix_exact(struct pci_dev *dev,
> +		       struct msix_entry *entries, int nvec)
> +{
> +	return -ENOSYS;
> +}
>  #else
>  int pci_get_msi_cap(struct pci_dev *dev);
>  int pci_enable_msi_block(struct pci_dev *dev, int nvec);
> @@ -1206,6 +1237,30 @@ void pci_disable_msix(struct pci_dev *dev);
>  void msi_remove_pci_irq_vectors(struct pci_dev *dev);
>  void pci_restore_msi_state(struct pci_dev *dev);
>  int pci_msi_enabled(void);
> +
> +int pcim_enable_msi_range(struct pci_dev *dev, int minvec, int maxvec);
> +static inline int pcim_enable_msi(struct pci_dev *dev, int maxvec)
> +{
> +	return pcim_enable_msi_range(dev, 1, maxvec);
> +}
> +static inline int pcim_enable_msi_exact(struct pci_dev *dev, int nvec)
> +{
> +	return pcim_enable_msi_range(dev, nvec, nvec);
> +}
> +
> +int pcim_enable_msix_range(struct pci_dev *dev, struct msix_entry *entries,
> +			   int minvec, int maxvec);
> +static inline int
> +pcim_enable_msix(struct pci_dev *dev, struct msix_entry *entries, int maxvec)
> +{
> +	return pcim_enable_msix_range(dev, entries, 1, maxvec);
> +}
> +static inline int
> +pcim_enable_msix_exact(struct pci_dev *dev,
> +		       struct msix_entry *entries, int nvec)
> +{
> +	return pcim_enable_msix_range(dev, entries, nvec, nvec);
> +}
>  #endif
>  
>  #ifdef CONFIG_PCIEPORTBUS
> -- 
> 1.7.7.6
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists