Re: [PATCH RFC 07/77] PCI/MSI: Re-design MSI/MSI-X interrupts enablement pattern
Hello, Alexander. On Tue, Oct 08, 2013 at 09:48:26AM +0200, Alexander Gordeev wrote: If there are many which duplicate the above pattern, it'd probably be worthwhile to provide a helper? It's usually a good idea to reduce the amount of boilerplate code in drivers. I wanted to limit discussion in v1 to as little changes as possible. I 'planned' those helper(s) for a separate effort if/when the most important change is accepted and soaked a bit. The thing is doing it this way generates more churns and noises. Once the simpler ones live behind a wrapper which can be built on the existing interface, we can have both reduced cost and more latitude on the complex cases. If we do things this way, it breaks all drivers using this interface until they're converted, right? Right. And the rest of the series does it. Which breaks bisection which we shouldn't do. Also, it probably isn't the best idea to flip the behavior like this as this can go completely unnoticed (no compiler warning or anything, the same function just behaves differently). Maybe it'd be a better idea to introduce a simpler interface that most can be converted to? Well, an *other* interface is a good idea. What do you mean with the simpler here? I'm still talking about a simpler wrapper for common cases, which is the important part anyway. Thanks. -- tejun ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH RFC 07/77] PCI/MSI: Re-design MSI/MSI-X interrupts enablement pattern
On Mon, Oct 07, 2013 at 02:17:49PM -0400, Tejun Heo wrote:
Hello,
On Wed, Oct 02, 2013 at 12:48:23PM +0200, Alexander Gordeev wrote:
+static int foo_driver_enable_msi(struct foo_adapter *adapter, int nvec)
+{
+ rc = pci_get_msi_cap(adapter-pdev);
+ if (rc 0)
+ return rc;
+
+ nvec = min(nvec, rc);
+ if (nvec FOO_DRIVER_MINIMUM_NVEC) {
+ return -ENOSPC;
+
+ rc = pci_enable_msi_block(adapter-pdev, nvec);
+ return rc;
+}
If there are many which duplicate the above pattern, it'd probably be
worthwhile to provide a helper? It's usually a good idea to reduce
the amount of boilerplate code in drivers.
I wanted to limit discussion in v1 to as little changes as possible.
I 'planned' those helper(s) for a separate effort if/when the most
important change is accepted and soaked a bit.
@@ -975,7 +951,7 @@ int pci_enable_msix(struct pci_dev *dev, struct
msix_entry *entries, int nvec)
if (nr_entries 0)
return nr_entries;
if (nvec nr_entries)
- return nr_entries;
+ return -EINVAL;
/* Check for any invalid entries */
for (i = 0; i nvec; i++) {
If we do things this way, it breaks all drivers using this interface
until they're converted, right?
Right. And the rest of the series does it.
Also, it probably isn't the best idea
to flip the behavior like this as this can go completely unnoticed (no
compiler warning or anything, the same function just behaves
differently). Maybe it'd be a better idea to introduce a simpler
interface that most can be converted to?
Well, an *other* interface is a good idea. What do you mean with the
simpler here?
tejun
--
Regards,
Alexander Gordeev
agord...@redhat.com
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH RFC 07/77] PCI/MSI: Re-design MSI/MSI-X interrupts enablement pattern
Hello,
On Wed, Oct 02, 2013 at 12:48:23PM +0200, Alexander Gordeev wrote:
+static int foo_driver_enable_msi(struct foo_adapter *adapter, int nvec)
+{
+ rc = pci_get_msi_cap(adapter-pdev);
+ if (rc 0)
+ return rc;
+
+ nvec = min(nvec, rc);
+ if (nvec FOO_DRIVER_MINIMUM_NVEC) {
+ return -ENOSPC;
+
+ rc = pci_enable_msi_block(adapter-pdev, nvec);
+ return rc;
+}
If there are many which duplicate the above pattern, it'd probably be
worthwhile to provide a helper? It's usually a good idea to reduce
the amount of boilerplate code in drivers.
static int foo_driver_enable_msix(struct foo_adapter *adapter, int nvec)
{
+ rc = pci_msix_table_size(adapter-pdev);
+ if (rc 0)
+ return rc;
+
+ nvec = min(nvec, rc);
+ if (nvec FOO_DRIVER_MINIMUM_NVEC) {
+ return -ENOSPC;
+
+ for (i = 0; i nvec; i++)
+ adapter-msix_entries[i].entry = i;
+
+ rc = pci_enable_msix(adapter-pdev, adapter-msix_entries, nvec);
+ return rc;
}
Ditto.
@@ -975,7 +951,7 @@ int pci_enable_msix(struct pci_dev *dev, struct
msix_entry *entries, int nvec)
if (nr_entries 0)
return nr_entries;
if (nvec nr_entries)
- return nr_entries;
+ return -EINVAL;
/* Check for any invalid entries */
for (i = 0; i nvec; i++) {
If we do things this way, it breaks all drivers using this interface
until they're converted, right? Also, it probably isn't the best idea
to flip the behavior like this as this can go completely unnoticed (no
compiler warning or anything, the same function just behaves
differently). Maybe it'd be a better idea to introduce a simpler
interface that most can be converted to?
Thanks.
--
tejun
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH RFC 07/77] PCI/MSI: Re-design MSI/MSI-X interrupts enablement pattern
Currently pci_enable_msi_block() and pci_enable_msix() interfaces
return a error code in case of failure, 0 in case of success and a
positive value which indicates the number of MSI-X/MSI interrupts
that could have been allocated. The latter value should be passed
to a repeated call to the interfaces until a failure or success.
This technique proved to be confusing and error-prone. Vast share
of device drivers simply fail to follow the described guidelines.
This update converts pci_enable_msix() and pci_enable_msi_block()
interfaces to canonical kernel functions and makes them return a
error code in case of failure or 0 in case of success.
As result, device drivers will cease to use the overcomplicated
repeated fallbacks technique and resort to a straightforward
pattern - determine the number of MSI/MSI-X interrupts required
before calling pci_enable_msix() and pci_enable_msi_block()
interfaces.
Device drivers will use their knowledge of underlying hardware
to determine the number of MSI/MSI-X interrupts required.
The simplest case would be requesting all available interrupts -
to obtain that value device drivers will use pci_get_msi_cap()
interface for MSI and pci_msix_table_size() for MSI-X.
More complex cases would entail matching device capabilities
with the system environment, i.e. limiting number of hardware
queues (and hence associated MSI/MSI-X interrupts) to the number
of online CPUs.
Suggested-by: Tejun Heo t...@kernel.org
Signed-off-by: Alexander Gordeev agord...@redhat.com
---
Documentation/PCI/MSI-HOWTO.txt | 71 ++---
arch/mips/pci/msi-octeon.c |2 +-
arch/powerpc/kernel/msi.c|2 +-
arch/powerpc/platforms/pseries/msi.c |2 +-
arch/s390/pci/pci.c |2 +-
arch/x86/kernel/apic/io_apic.c |2 +-
drivers/pci/msi.c| 52 +++--
7 files changed, 58 insertions(+), 75 deletions(-)
diff --git a/Documentation/PCI/MSI-HOWTO.txt b/Documentation/PCI/MSI-HOWTO.txt
index 1f37ce2..40abcfb 100644
--- a/Documentation/PCI/MSI-HOWTO.txt
+++ b/Documentation/PCI/MSI-HOWTO.txt
@@ -111,21 +111,27 @@ the device are in the range dev-irq to dev-irq + count
- 1.
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 is
-less than 'count' and indicates the number of interrupts that could have
-been allocated. In neither case is the irq value updated or the device
-switched into MSI mode.
-
-The device driver must decide what action to take if
-pci_enable_msi_block() returns a value less than the number requested.
-For instance, the driver could still make use of fewer interrupts;
-in this case the driver should call pci_enable_msi_block()
-again. Note that it is not guaranteed to succeed, even when the
-'count' has been reduced to the value returned from a previous call to
-pci_enable_msi_block(). This is because there are multiple constraints
-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.
+this device.
+
+Device drivers should normally call pci_get_msi_cap() function before
+calling this function to determine maximum number of MSI interrupts
+a device can send.
+
+A sequence to achieve that might look like:
+
+static int foo_driver_enable_msi(struct foo_adapter *adapter, int nvec)
+{
+ rc = pci_get_msi_cap(adapter-pdev);
+ if (rc 0)
+ return rc;
+
+ nvec = min(nvec, rc);
+ if (nvec FOO_DRIVER_MINIMUM_NVEC) {
+ return -ENOSPC;
+
+ rc = pci_enable_msi_block(adapter-pdev, nvec);
+ return rc;
+}
4.2.3 pci_enable_msi_block_auto
@@ -218,9 +224,7 @@ interrupts assigned to the MSI-X vectors so it can free
them again later.
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 it returns a positive number, it indicates the maximum
-number of interrupt vectors that could have been allocated. See example
-below.
+this device.
This function, in contrast with pci_enable_msi(), does not adjust
dev-irq. The device will not generate interrupts for this interrupt
@@ -229,24 +233,27 @@ number once MSI-X is enabled.
Device drivers should normally call this function once per device
during the initialization phase.
-It is ideal if drivers can cope with a variable number of MSI-X interrupts;
-there are many reasons why the platform may not be able to provide the
-exact number that a driver asks for.
+Device drivers should normally call pci_msix_table_size() function before
+calling this function to determine maximum number of MSI-X interrupts
+a device can send.
-A request loop to achieve that might look like:
+A sequence to achieve that might look
