two pci_alloc_irq_vectors improvements

two pci_alloc_irq_vectors improvements

[Christoph Hellwig]

Hi Bjorn, hi Alex,

below are two patches I'd love to see in 4.8 to improve the
pci_alloc_irq_vectors interface.  I've realized we need these while
starting a mass conversion of the MSI-X users to the new interface,
and getting it into 4.8 before users show up should make life a lot
easier.

The positive flags things comes out of the fact that a lot of driver
want MSI-X only without any fallback, or some elaborate differences
for the different interrupt schemes.  For example while most modern
devices use one MSI-X vector per queue and scale nicely from the
MSI to the MSI-X scheme many older devices use a MSI-X vector per
functionality in the ISR, so they might wan to allocate them
differently.

The second one ensures the legacy interrupt line is actually enabled
before using it, because some devices like MSI-X might actually have
it disable by default.

[PATCH 1/2] pci: use positive flags in pci_alloc_irq_vectors

[Christoph Hellwig]

Instead of using PCI_IRQ_NO* to disable behavior in pci_alloc_irq_vectors
switch to passing the inverted flags to enable each of them individually.

This is based on a number of pending driver conversions that just happend
to be a whole more obvious to read this way, and given that we have no
users in the tree yet it can still easily be done.

I've also added a PCI_IRQ_ALL_TYPES catchall to keep the case of accepting
all interrupt types very simple.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 Documentation/PCI/MSI-HOWTO.txt | 21 +++++++++------------
 drivers/pci/msi.c               | 15 +++++++--------
 include/linux/pci.h             | 10 ++++++----
 3 files changed, 22 insertions(+), 24 deletions(-)

diff --git a/Documentation/PCI/MSI-HOWTO.txt b/Documentation/PCI/MSI-HOWTO.txt
index c55df29..8faf14a 100644
--- a/Documentation/PCI/MSI-HOWTO.txt
+++ b/Documentation/PCI/MSI-HOWTO.txt
@@ -94,14 +94,11 @@ has a requirements for a minimum number of vectors the driver can pass a
 min_vecs argument set to this limit, and the PCI core will return -ENOSPC
 if it can't meet the minimum number of vectors.
 
-The flags argument should normally be set to 0, but can be used to pass the
-PCI_IRQ_NOMSI and PCI_IRQ_NOMSIX flag in case a device claims to support
-MSI or MSI-X, but the support is broken, or to pass PCI_IRQ_NOLEGACY in
-case the device does not support legacy interrupt lines.
-
-By default this function will spread the interrupts around the available
-CPUs, but this feature can be disabled by passing the PCI_IRQ_NOAFFINITY
-flag.
+The flags argument is used to specify which type of interrupt can be used
+by the device and the driver (PCI_IRQ_LEGACY, PCI_IRQ_MSI, PCI_IRQ_MSIX).
+A conveniant short-hand (PCI_IRQ_ALL_TYPES) is also avaiable to ask for
+any possible kind of interrupt. If the PCI_IRQ_NOAFFINITY flag is set,
+pci_alloc_irq_vectors will spread the interrupts around the available CPUs.
 
 To get the Linux IRQ numbers passed to request_irq() and free_irq() and the
 vectors, use the following function:
@@ -131,7 +128,7 @@ larger than the number supported by the device it will automatically be
 capped to the supported limit, so there is no need to query the number of
 vectors supported beforehand:
 
-	nvec = pci_alloc_irq_vectors(pdev, 1, nvec, 0);
+	nvec = pci_alloc_irq_vectors(pdev, 1, nvec, PCI_IRQ_ALL_TYPES)
 	if (nvec < 0)
 		goto out_err;
 
@@ -140,7 +137,7 @@ interrupts it can request a particular number of interrupts by passing that
 number to pci_alloc_irq_vectors() function as both 'min_vecs' and
 'max_vecs' parameters:
 
-	ret = pci_alloc_irq_vectors(pdev, nvec, nvec, 0);
+	ret = pci_alloc_irq_vectors(pdev, nvec, nvec, PCI_IRQ_ALL_TYPES);
 	if (ret < 0)
 		goto out_err;
 
@@ -148,7 +145,7 @@ The most notorious example of the request type described above is enabling
 the single MSI mode for a device.  It could be done by passing two 1s as
 'min_vecs' and 'max_vecs':
 
-	ret = pci_alloc_irq_vectors(pdev, 1, 1, 0);
+	ret = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_ALL_TYPES);
 	if (ret < 0)
 		goto out_err;
 
@@ -156,7 +153,7 @@ Some devices might not support using legacy line interrupts, in which case
 the PCI_IRQ_NOLEGACY flag can be used to fail the request if the platform
 can't provide MSI or MSI-X interrupts:
 
-	nvec = pci_alloc_irq_vectors(pdev, 1, nvec, PCI_IRQ_NOLEGACY);
+	nvec = pci_alloc_irq_vectors(pdev, 1, nvec, PCI_IRQ_MSI | PCI_IRQ_MSIX);
 	if (nvec < 0)
 		goto out_err;
 
diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index a02981e..9233e7f 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -1069,7 +1069,7 @@ static int __pci_enable_msi_range(struct pci_dev *dev, int minvec, int maxvec,
 		nvec = maxvec;
 
 	for (;;) {
-		if (!(flags & PCI_IRQ_NOAFFINITY)) {
+		if (flags & PCI_IRQ_AFFINITY) {
 			dev->irq_affinity = irq_create_affinity_mask(&nvec);
 			if (nvec < minvec)
 				return -ENOSPC;
@@ -1105,7 +1105,7 @@ static int __pci_enable_msi_range(struct pci_dev *dev, int minvec, int maxvec,
  **/
 int pci_enable_msi_range(struct pci_dev *dev, int minvec, int maxvec)
 {
-	return __pci_enable_msi_range(dev, minvec, maxvec, PCI_IRQ_NOAFFINITY);
+	return __pci_enable_msi_range(dev, minvec, maxvec, 0);
 }
 EXPORT_SYMBOL(pci_enable_msi_range);
 
@@ -1120,7 +1120,7 @@ static int __pci_enable_msix_range(struct pci_dev *dev,
 		return -ERANGE;
 
 	for (;;) {
-		if (!(flags & PCI_IRQ_NOAFFINITY)) {
+		if (flags & PCI_IRQ_AFFINITY) {
 			dev->irq_affinity = irq_create_affinity_mask(&nvec);
 			if (nvec < minvec)
 				return -ENOSPC;
@@ -1160,8 +1160,7 @@ static int __pci_enable_msix_range(struct pci_dev *dev,
 int pci_enable_msix_range(struct pci_dev *dev, struct msix_entry *entries,
 		int minvec, int maxvec)
 {
-	return __pci_enable_msix_range(dev, entries, minvec, maxvec,
-			PCI_IRQ_NOAFFINITY);
+	return __pci_enable_msix_range(dev, entries, minvec, maxvec, 0);
 }
 EXPORT_SYMBOL(pci_enable_msix_range);
 
@@ -1187,21 +1186,21 @@ int pci_alloc_irq_vectors(struct pci_dev *dev, unsigned int min_vecs,
 {
 	int vecs = -ENOSPC;
 
-	if (!(flags & PCI_IRQ_NOMSIX)) {
+	if (flags & PCI_IRQ_MSIX) {
 		vecs = __pci_enable_msix_range(dev, NULL, min_vecs, max_vecs,
 				flags);
 		if (vecs > 0)
 			return vecs;
 	}
 
-	if (!(flags & PCI_IRQ_NOMSI)) {
+	if (flags & PCI_IRQ_MSI) {
 		vecs = __pci_enable_msi_range(dev, min_vecs, max_vecs, flags);
 		if (vecs > 0)
 			return vecs;
 	}
 
 	/* use legacy irq if allowed */
-	if (!(flags & PCI_IRQ_NOLEGACY) && min_vecs == 1)
+	if ((flags & PCI_IRQ_LEGACY) && min_vecs == 1)
 		return 1;
 	return vecs;
 }
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 2599a98..fbc1fa6 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1251,10 +1251,12 @@ resource_size_t pcibios_iov_resource_alignment(struct pci_dev *dev, int resno);
 int pci_set_vga_state(struct pci_dev *pdev, bool decode,
 		      unsigned int command_bits, u32 flags);
 
-#define PCI_IRQ_NOLEGACY	(1 << 0) /* don't use legacy interrupts */
-#define PCI_IRQ_NOMSI		(1 << 1) /* don't use MSI interrupts */
-#define PCI_IRQ_NOMSIX		(1 << 2) /* don't use MSI-X interrupts */
-#define PCI_IRQ_NOAFFINITY	(1 << 3) /* don't auto-assign affinity */
+#define PCI_IRQ_LEGACY		(1 << 0) /* allow legacy interrupts */
+#define PCI_IRQ_MSI		(1 << 1) /* allow MSI interrupts */
+#define PCI_IRQ_MSIX		(1 << 2) /* allow MSI-X interrupts */
+#define PCI_IRQ_AFFINITY	(1 << 3) /* auto-assign affinity */
+#define PCI_IRQ_ALL_TYPES \
+	(PCI_IRQ_LEGACY | PCI_IRQ_MSI | PCI_IRQ_MSIX)
 
 /* kmem_cache style wrapper around pci_alloc_consistent() */
 
-- 
2.1.4

[PATCH 2/2] pci: call pci_intx when using legacy interrupts in pci_alloc_irq_vectors

[Christoph Hellwig]

ahci currently insists on an explicit call to pci_intx before falling back
from MSI or MSI-X to legacy irqs.  As pci_intx is a no-op if the command
register already contains the right value is seems safe and useful to add
this call to pci_alloc_irq_vectors so that ahci can just use
pci_alloc_irq_vectors.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/pci/msi.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index 9233e7f..593698e 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -1200,8 +1200,11 @@ int pci_alloc_irq_vectors(struct pci_dev *dev, unsigned int min_vecs,
 	}
 
 	/* use legacy irq if allowed */
-	if ((flags & PCI_IRQ_LEGACY) && min_vecs == 1)
+	if ((flags & PCI_IRQ_LEGACY) && min_vecs == 1) {
+		pci_intx(dev, 1);
 		return 1;
+	}
+
 	return vecs;
 }
 EXPORT_SYMBOL(pci_alloc_irq_vectors);
-- 
2.1.4

Re: two pci_alloc_irq_vectors improvements

[Christoph Hellwig]

Bjorn, Alex - any chance to get a look at these quickly?  It would
be nice to have this in place before starting the conversions
for the 4.9 merge window..

Re: two pci_alloc_irq_vectors improvements

[Bjorn Helgaas]

Hi Christoph,

On Thu, Aug 11, 2016 at 07:11:03AM -0700, Christoph Hellwig wrote:
> Hi Bjorn, hi Alex,
> 
> below are two patches I'd love to see in 4.8 to improve the
> pci_alloc_irq_vectors interface.  I've realized we need these while
> starting a mass conversion of the MSI-X users to the new interface,
> and getting it into 4.8 before users show up should make life a lot
> easier.
> 
> The positive flags things comes out of the fact that a lot of driver
> want MSI-X only without any fallback, or some elaborate differences
> for the different interrupt schemes.  For example while most modern
> devices use one MSI-X vector per queue and scale nicely from the
> MSI to the MSI-X scheme many older devices use a MSI-X vector per
> functionality in the ISR, so they might wan to allocate them
> differently.
> 
> The second one ensures the legacy interrupt line is actually enabled
> before using it, because some devices like MSI-X might actually have
> it disable by default.

I really like this change.  It's much nicer to read

  if (flags & PCI_IRQ_AFFINITY)

than it was to read

  if (!(flags & PCI_IRQ_NOAFFINITY))

Speaking of affinity, the original documentation said "By default this
function will spread the interrupts around the available CPUs".  After
these patches, you have to pass PCI_IRQ_AFFINITY to get that behavior.
Are you planning to have drivers use

  pci_alloc_irq_vectors(dev, 1, nvec, PCI_IRQ_ALL_TYPES | PCI_IRQ_AFFINITY)

to explicitly ask for affinity?

I applied these to for-linus with the intent of merging them for v4.8.
I fixed a couple typos in the first one as shown below.

Any objections, Alex?

Bjorn


diff --git a/Documentation/PCI/MSI-HOWTO.txt b/Documentation/PCI/MSI-HOWTO.txt
index 8faf14a..cd9c9f6 100644
--- a/Documentation/PCI/MSI-HOWTO.txt
+++ b/Documentation/PCI/MSI-HOWTO.txt
@@ -96,9 +96,9 @@ if it can't meet the minimum number of vectors.
 
 The flags argument is used to specify which type of interrupt can be used
 by the device and the driver (PCI_IRQ_LEGACY, PCI_IRQ_MSI, PCI_IRQ_MSIX).
-A conveniant short-hand (PCI_IRQ_ALL_TYPES) is also avaiable to ask for
-any possible kind of interrupt. If the PCI_IRQ_NOAFFINITY flag is set,
-pci_alloc_irq_vectors will spread the interrupts around the available CPUs.
+A convenient short-hand (PCI_IRQ_ALL_TYPES) is also available to ask for
+any possible kind of interrupt.  If the PCI_IRQ_AFFINITY flag is set,
+pci_alloc_irq_vectors() will spread the interrupts around the available CPUs.
 
 To get the Linux IRQ numbers passed to request_irq() and free_irq() and the
 vectors, use the following function:
@@ -150,8 +150,7 @@ the single MSI mode for a device.  It could be done by passing two 1s as
 		goto out_err;
 
 Some devices might not support using legacy line interrupts, in which case
-the PCI_IRQ_NOLEGACY flag can be used to fail the request if the platform
-can't provide MSI or MSI-X interrupts:
+the driver can specify that only MSI or MSI-X is acceptable:
 
 	nvec = pci_alloc_irq_vectors(pdev, 1, nvec, PCI_IRQ_MSI | PCI_IRQ_MSIX);
 	if (nvec < 0)

Re: two pci_alloc_irq_vectors improvements

[Christoph Hellwig]

On Tue, Aug 16, 2016 at 02:34:18PM -0500, Bjorn Helgaas wrote:
> Speaking of affinity, the original documentation said "By default this
> function will spread the interrupts around the available CPUs".  After
> these patches, you have to pass PCI_IRQ_AFFINITY to get that behavior.
> Are you planning to have drivers use
> 
>   pci_alloc_irq_vectors(dev, 1, nvec, PCI_IRQ_ALL_TYPES | PCI_IRQ_AFFINITY)
> 
> to explicitly ask for affinity?

Yes, at least for now.  During my mass conversion attempts I found
enough drivers that don't use MSI-X for spreading queues over CPUs
but instead for different kinds of interrupts.  I'd been to much in
my NVMe and RDMA world earlier to assume everyone else would do something
that sensible..

> I applied these to for-linus with the intent of merging them for v4.8.
> I fixed a couple typos in the first one as shown below.

Thanks.

Re: [PATCH 1/2] pci: use positive flags in pci_alloc_irq_vectors

[Alexander Gordeev]

On Thu, Aug 11, 2016 at 07:11:04AM -0700, Christoph Hellwig wrote:
> Instead of using PCI_IRQ_NO* to disable behavior in pci_alloc_irq_vectors
> switch to passing the inverted flags to enable each of them individually.
> 
> This is based on a number of pending driver conversions that just happend
> to be a whole more obvious to read this way, and given that we have no
> users in the tree yet it can still easily be done.
> 
> I've also added a PCI_IRQ_ALL_TYPES catchall to keep the case of accepting
> all interrupt types very simple.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  Documentation/PCI/MSI-HOWTO.txt | 21 +++++++++------------
>  drivers/pci/msi.c               | 15 +++++++--------
>  include/linux/pci.h             | 10 ++++++----
>  3 files changed, 22 insertions(+), 24 deletions(-)
> 
> diff --git a/Documentation/PCI/MSI-HOWTO.txt b/Documentation/PCI/MSI-HOWTO.txt
> index c55df29..8faf14a 100644
> --- a/Documentation/PCI/MSI-HOWTO.txt
> +++ b/Documentation/PCI/MSI-HOWTO.txt
> @@ -94,14 +94,11 @@ has a requirements for a minimum number of vectors the driver can pass a
>  min_vecs argument set to this limit, and the PCI core will return -ENOSPC
>  if it can't meet the minimum number of vectors.
>  
> -The flags argument should normally be set to 0, but can be used to pass the
> -PCI_IRQ_NOMSI and PCI_IRQ_NOMSIX flag in case a device claims to support
> -MSI or MSI-X, but the support is broken, or to pass PCI_IRQ_NOLEGACY in
> -case the device does not support legacy interrupt lines.
> -
> -By default this function will spread the interrupts around the available
> -CPUs, but this feature can be disabled by passing the PCI_IRQ_NOAFFINITY
> -flag.
> +The flags argument is used to specify which type of interrupt can be used
> +by the device and the driver (PCI_IRQ_LEGACY, PCI_IRQ_MSI, PCI_IRQ_MSIX).
> +A conveniant short-hand (PCI_IRQ_ALL_TYPES) is also avaiable to ask for
> +any possible kind of interrupt. If the PCI_IRQ_NOAFFINITY flag is set,
> +pci_alloc_irq_vectors will spread the interrupts around the available CPUs.
>  
>  To get the Linux IRQ numbers passed to request_irq() and free_irq() and the
>  vectors, use the following function:
> @@ -131,7 +128,7 @@ larger than the number supported by the device it will automatically be
>  capped to the supported limit, so there is no need to query the number of
>  vectors supported beforehand:
>  
> -	nvec = pci_alloc_irq_vectors(pdev, 1, nvec, 0);
> +	nvec = pci_alloc_irq_vectors(pdev, 1, nvec, PCI_IRQ_ALL_TYPES)
>  	if (nvec < 0)
>  		goto out_err;
>  
> @@ -140,7 +137,7 @@ interrupts it can request a particular number of interrupts by passing that
>  number to pci_alloc_irq_vectors() function as both 'min_vecs' and
>  'max_vecs' parameters:
>  
> -	ret = pci_alloc_irq_vectors(pdev, nvec, nvec, 0);
> +	ret = pci_alloc_irq_vectors(pdev, nvec, nvec, PCI_IRQ_ALL_TYPES);
>  	if (ret < 0)
>  		goto out_err;
>  
> @@ -148,7 +145,7 @@ The most notorious example of the request type described above is enabling
>  the single MSI mode for a device.  It could be done by passing two 1s as
>  'min_vecs' and 'max_vecs':
>  
> -	ret = pci_alloc_irq_vectors(pdev, 1, 1, 0);
> +	ret = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_ALL_TYPES);
>  	if (ret < 0)
>  		goto out_err;
>  
> @@ -156,7 +153,7 @@ Some devices might not support using legacy line interrupts, in which case
>  the PCI_IRQ_NOLEGACY flag can be used to fail the request if the platform
>  can't provide MSI or MSI-X interrupts:
>  
> -	nvec = pci_alloc_irq_vectors(pdev, 1, nvec, PCI_IRQ_NOLEGACY);
> +	nvec = pci_alloc_irq_vectors(pdev, 1, nvec, PCI_IRQ_MSI | PCI_IRQ_MSIX);
>  	if (nvec < 0)
>  		goto out_err;
>  
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index a02981e..9233e7f 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -1069,7 +1069,7 @@ static int __pci_enable_msi_range(struct pci_dev *dev, int minvec, int maxvec,
>  		nvec = maxvec;
>  
>  	for (;;) {
> -		if (!(flags & PCI_IRQ_NOAFFINITY)) {
> +		if (flags & PCI_IRQ_AFFINITY) {
>  			dev->irq_affinity = irq_create_affinity_mask(&nvec);
>  			if (nvec < minvec)
>  				return -ENOSPC;
> @@ -1105,7 +1105,7 @@ static int __pci_enable_msi_range(struct pci_dev *dev, int minvec, int maxvec,
>   **/
>  int pci_enable_msi_range(struct pci_dev *dev, int minvec, int maxvec)
>  {
> -	return __pci_enable_msi_range(dev, minvec, maxvec, PCI_IRQ_NOAFFINITY);
> +	return __pci_enable_msi_range(dev, minvec, maxvec, 0);
>  }
>  EXPORT_SYMBOL(pci_enable_msi_range);
>  
> @@ -1120,7 +1120,7 @@ static int __pci_enable_msix_range(struct pci_dev *dev,
>  		return -ERANGE;
>  
>  	for (;;) {
> -		if (!(flags & PCI_IRQ_NOAFFINITY)) {
> +		if (flags & PCI_IRQ_AFFINITY) {
>  			dev->irq_affinity = irq_create_affinity_mask(&nvec);
>  			if (nvec < minvec)
>  				return -ENOSPC;
> @@ -1160,8 +1160,7 @@ static int __pci_enable_msix_range(struct pci_dev *dev,
>  int pci_enable_msix_range(struct pci_dev *dev, struct msix_entry *entries,
>  		int minvec, int maxvec)
>  {
> -	return __pci_enable_msix_range(dev, entries, minvec, maxvec,
> -			PCI_IRQ_NOAFFINITY);
> +	return __pci_enable_msix_range(dev, entries, minvec, maxvec, 0);
>  }
>  EXPORT_SYMBOL(pci_enable_msix_range);
>  
> @@ -1187,21 +1186,21 @@ int pci_alloc_irq_vectors(struct pci_dev *dev, unsigned int min_vecs,
>  {
>  	int vecs = -ENOSPC;
>  
> -	if (!(flags & PCI_IRQ_NOMSIX)) {
> +	if (flags & PCI_IRQ_MSIX) {
>  		vecs = __pci_enable_msix_range(dev, NULL, min_vecs, max_vecs,
>  				flags);
>  		if (vecs > 0)
>  			return vecs;
>  	}
>  
> -	if (!(flags & PCI_IRQ_NOMSI)) {
> +	if (flags & PCI_IRQ_MSI) {
>  		vecs = __pci_enable_msi_range(dev, min_vecs, max_vecs, flags);
>  		if (vecs > 0)
>  			return vecs;
>  	}
>  
>  	/* use legacy irq if allowed */
> -	if (!(flags & PCI_IRQ_NOLEGACY) && min_vecs == 1)
> +	if ((flags & PCI_IRQ_LEGACY) && min_vecs == 1)
>  		return 1;
>  	return vecs;
>  }
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 2599a98..fbc1fa6 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1251,10 +1251,12 @@ resource_size_t pcibios_iov_resource_alignment(struct pci_dev *dev, int resno);
>  int pci_set_vga_state(struct pci_dev *pdev, bool decode,
>  		      unsigned int command_bits, u32 flags);
>  
> -#define PCI_IRQ_NOLEGACY	(1 << 0) /* don't use legacy interrupts */
> -#define PCI_IRQ_NOMSI		(1 << 1) /* don't use MSI interrupts */
> -#define PCI_IRQ_NOMSIX		(1 << 2) /* don't use MSI-X interrupts */
> -#define PCI_IRQ_NOAFFINITY	(1 << 3) /* don't auto-assign affinity */
> +#define PCI_IRQ_LEGACY		(1 << 0) /* allow legacy interrupts */
> +#define PCI_IRQ_MSI		(1 << 1) /* allow MSI interrupts */
> +#define PCI_IRQ_MSIX		(1 << 2) /* allow MSI-X interrupts */
> +#define PCI_IRQ_AFFINITY	(1 << 3) /* auto-assign affinity */
> +#define PCI_IRQ_ALL_TYPES \
> +	(PCI_IRQ_LEGACY | PCI_IRQ_MSI | PCI_IRQ_MSIX)
>  
>  /* kmem_cache style wrapper around pci_alloc_consistent() */
>  

Reviewed-by: Alexander Gordeev <agordeev@redhat.com>

> -- 
> 2.1.4
> 

Re: [PATCH 2/2] pci: call pci_intx when using legacy interrupts in pci_alloc_irq_vectors

[Alexander Gordeev]

On Thu, Aug 11, 2016 at 07:11:05AM -0700, Christoph Hellwig wrote:
> ahci currently insists on an explicit call to pci_intx before falling back
> from MSI or MSI-X to legacy irqs.  As pci_intx is a no-op if the command
> register already contains the right value is seems safe and useful to add
> this call to pci_alloc_irq_vectors so that ahci can just use
> pci_alloc_irq_vectors.

Looking at ahci_init_interrupts() (and probably at commit d684a90d
("ahci: per-port msix support")) it looks like pci_alloc_irq_vectors()
is able to preserve the current AHCI logic?

> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/pci/msi.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index 9233e7f..593698e 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -1200,8 +1200,11 @@ int pci_alloc_irq_vectors(struct pci_dev *dev, unsigned int min_vecs,
>  	}
>  
>  	/* use legacy irq if allowed */
> -	if ((flags & PCI_IRQ_LEGACY) && min_vecs == 1)
> +	if ((flags & PCI_IRQ_LEGACY) && min_vecs == 1) {
> +		pci_intx(dev, 1);

It would rather called pci_intx_for_msi() here. But because it is
a generic code I am not sure what implications it has for all
drivers out there.

>  		return 1;
> +	}
> +
>  	return vecs;
>  }
>  EXPORT_SYMBOL(pci_alloc_irq_vectors);
> -- 
> 2.1.4
> 

Re: [PATCH 2/2] pci: call pci_intx when using legacy interrupts in pci_alloc_irq_vectors

[Alexander Gordeev]

On Thu, Aug 18, 2016 at 11:20:07AM +0200, Alexander Gordeev wrote:
> On Thu, Aug 11, 2016 at 07:11:05AM -0700, Christoph Hellwig wrote:
> > ahci currently insists on an explicit call to pci_intx before falling back
> > from MSI or MSI-X to legacy irqs.  As pci_intx is a no-op if the command
> > register already contains the right value is seems safe and useful to add
> > this call to pci_alloc_irq_vectors so that ahci can just use
> > pci_alloc_irq_vectors.
> 
> Looking at ahci_init_interrupts() (and probably at commit d684a90d
> ("ahci: per-port msix support")) it looks like pci_alloc_irq_vectors()
> is able to preserve the current AHCI logic?
     ^^^^
  is *not* able (sorry)

> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > ---
> >  drivers/pci/msi.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> > index 9233e7f..593698e 100644
> > --- a/drivers/pci/msi.c
> > +++ b/drivers/pci/msi.c
> > @@ -1200,8 +1200,11 @@ int pci_alloc_irq_vectors(struct pci_dev *dev, unsigned int min_vecs,
> >  	}
> >  
> >  	/* use legacy irq if allowed */
> > -	if ((flags & PCI_IRQ_LEGACY) && min_vecs == 1)
> > +	if ((flags & PCI_IRQ_LEGACY) && min_vecs == 1) {
> > +		pci_intx(dev, 1);
> 
> It would rather called pci_intx_for_msi() here. But because it is
> a generic code I am not sure what implications it has for all
> drivers out there.
> 
> >  		return 1;
> > +	}
> > +
> >  	return vecs;
> >  }
> >  EXPORT_SYMBOL(pci_alloc_irq_vectors);
> > -- 
> > 2.1.4
> > 

Re: [PATCH 2/2] pci: call pci_intx when using legacy interrupts in pci_alloc_irq_vectors

[Christoph Hellwig]

On Thu, Aug 18, 2016 at 11:20:07AM +0200, Alexander Gordeev wrote:
> On Thu, Aug 11, 2016 at 07:11:05AM -0700, Christoph Hellwig wrote:
> > ahci currently insists on an explicit call to pci_intx before falling back
> > from MSI or MSI-X to legacy irqs.  As pci_intx is a no-op if the command
> > register already contains the right value is seems safe and useful to add
> > this call to pci_alloc_irq_vectors so that ahci can just use
> > pci_alloc_irq_vectors.
> 
> Looking at ahci_init_interrupts() (and probably at commit d684a90d
> ("ahci: per-port msix support")) it looks like pci_alloc_irq_vectors()
> is able to preserve the current AHCI logic?

Not quite.  For the currentl logic we need 3 calls to
pci_alloc_irq_vectors, and I have a patch to implement that.  From
looking at the changelogs and intentions I think we can consolidate
that down to two calls (per-port vectors and single vectors) and I
will propose that as an RFC on top of the base which, which already
is a huge simplification of the driver.

> > @@ -1200,8 +1200,11 @@ int pci_alloc_irq_vectors(struct pci_dev *dev, unsigned int min_vecs,
> >  	}
> >  
> >  	/* use legacy irq if allowed */
> > -	if ((flags & PCI_IRQ_LEGACY) && min_vecs == 1)
> > +	if ((flags & PCI_IRQ_LEGACY) && min_vecs == 1) {
> > +		pci_intx(dev, 1);
> 
> It would rather called pci_intx_for_msi() here. But because it is
> a generic code I am not sure what implications it has for all
> drivers out there.

It probably should be pci_intx_for_msi.  For now I'm not touching
drivers that need the quirk, so how about getting the intx in
now so that the conversion can start, and I'll send a follow on
to convert to pci_intx_for_msi with Cc to all the relevant parties
for the quirk as a follow on?

Re: [PATCH 2/2] pci: call pci_intx when using legacy interrupts in pci_alloc_irq_vectors

[Alexander Gordeev]

On Thu, Aug 18, 2016 at 05:26:49PM +0200, Christoph Hellwig wrote:
> It probably should be pci_intx_for_msi.  For now I'm not touching
> drivers that need the quirk, so how about getting the intx in
> now so that the conversion can start, and I'll send a follow on
> to convert to pci_intx_for_msi with Cc to all the relevant parties
> for the quirk as a follow on?

Probably should work.