On 2020-06-03 10:04 a.m., Stankiewicz, Piotr wrote:
>> -----Original Message-----
>> From: Logan Gunthorpe <log...@deltatee.com>
>> Sent: Wednesday, June 3, 2020 5:48 PM
>>
>>
>>
>> On 2020-06-03 5:44 a.m., Piotr Stankiewicz wrote:
>>> When debugging an issue where I was asking the PCI machinery to enable a
>>> set of MSI-X vectors, without falling back on MSI, I ran across a
>>> behaviour which seems odd. The pci_alloc_irq_vectors_affinity() will
>>> always return -ENOSPC on failure, when allocating MSI-X vectors only,
>>> whereas with MSI fallback it will forward any error returned by
>>> __pci_enable_msi_range(). This is a confusing behaviour, so have the
>>> pci_alloc_irq_vectors_affinity() forward the error code from
>>> __pci_enable_msix_range() when appropriate.
>>>
>>> Signed-off-by: Piotr Stankiewicz <piotr.stankiew...@intel.com>
>>> Reviewed-by: Andy Shevchenko <andriy.shevche...@intel.com>
>>> ---
>>> drivers/pci/msi.c | 5 +++--
>>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
>>> index 6b43a5455c7a..443cc324b196 100644
>>> --- a/drivers/pci/msi.c
>>> +++ b/drivers/pci/msi.c
>>> @@ -1231,8 +1231,9 @@ int pci_alloc_irq_vectors_affinity(struct pci_dev
>> *dev, unsigned int min_vecs,
>>> }
>>> }
>>>
>>> - if (msix_vecs == -ENOSPC)
>>> - return -ENOSPC;
>>> + if (msix_vecs == -ENOSPC ||
>>> + (flags & (PCI_IRQ_MSI | PCI_IRQ_MSIX)) == PCI_IRQ_MSIX)
>>> + return msix_vecs;
>>> return msi_vecs;
>>> }
>>> EXPORT_SYMBOL(pci_alloc_irq_vectors_affinity);
>>>
>>
>> It occurs to me that we could clean this function up a bit more... I
>> don't see any need to have two variables for msi_vecs and msix_vecs and
>> then have a complicated bit of logic at the end to decide which to return.
>>
>> Why not instead just have one variable which is set by
>> __pci_enable_msix_range(), then __pci_enable_msi_range(), then returned
>> if they both fail?
>>
>
> That wouldn't preserve the original bit of logic where -ENOSPC is returned
> any time __pci_enable_msix_range() fails with -ENOSPC, irrespective of whether
> MSI fallback was requested. Though I don't know if that is desired behaviour.
That does look very odd, but ok... Then, couldn't we just set msi_vecs
to msix_vecs after calling __pci_enable_msix_range() such that if
__pci_enable_msi_range() doesn't get called we will return the same
error without needing the messy second conditional?
Logan
