Thread (25 messages) 25 messages, 3 authors, 2016-07-12

Re: [PATCH 07/13] pci: Provide sensible irq vector alloc/free routines

From: Christoph Hellwig <hch@lst.de>
Date: 2016-07-10 03:47:43
Also in: linux-nvme, linux-pci, lkml

On Wed, Jul 06, 2016 at 10:05:45AM +0200, Alexander Gordeev wrote:
quoted
+ pci_enable_msi, pci_enable_msi_range, pci_enable_msi_exact, pci_disable_msi,
+ pci_msi_vec_count, pci_enable_msix_range, pci_enable_msix_exact,
+ pci_disable_msix, pci_msix_vec_count
Description of these functions can be removed when all drivers migrated
to the new API. Also implementation descriptions + examples would still
be needed AFAICT.
I diagreed - if we deprecated functions the only thing that should
be mentioned is a "don't use these". 
This function's code almost matches the existing pci_enable_msix_range()
so pci_enable_msix_range() should be reworked instead IMHO.
That's what earlier versions of the code did.  However due to the
fact that we want to avoid over-allocating the msix_vectors array
(minor) and get the vectors count of the affinity mask right (major,
as pointed out by you last time) I had to move the allocations inside
the helpers that loop around the atctual enablement.  I didn't want
to change the function to a different version of the algorithm just
before removing them relatively soon.  But given that strong preference
for changing these simple functions instead of duplicating them I've
changed that patch to do that now.
We do not need to keep msix_entry array, since it only needed for
pci_irq_vector() function. But the same info could be retrieved from
msi_desc::irq.
Indeed.  Avoiding this allocation makes these interfaces quite a bit
simpler.  It requires a few prep patches, but I think it's definitively
worth, so the next version will avoid the need for the msix_entry array.
quoted
+	/* use legacy irq if allowed */
+	if (min_vecs == 1)
+		return 1;
+	return -ENOSPC;
The original error code (in vecs) would be overridden with -ENOSPC here.
Ok, fixed.
quoted
+	WARN_ON_ONCE(!dev->msi_enabled && nr > 0);
+	return dev->irq + nr;
I think this function should check irq number existence and return the
vector number or -EINVAL;
Ok, fixed.
quoted
+		unsigned int flags)
+{
+	if (min_vecs > 1)
+		return -ENOSPC;
In case CONFIG_PCI_MSI is unset min_vecs > 1 is -EINVAL;
Ok, fixed.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help