Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1440027 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 07/13] pci: Provide sensible irq vector alloc/free  routines Christoph Hellwig <hch@lst.de> - 2016-07-10 05:50 +0200
    Re: [PATCH 07/13] pci: Provide sensible irq vector alloc/free  routines Alexander Gordeev <agordeev@redhat.com> - 2016-07-11 12:40 +0200
      Re: [PATCH 07/13] pci: Provide sensible irq vector alloc/free  routines Christoph Hellwig <hch@lst.de> - 2016-07-12 11:20 +0200
        Re: [PATCH 07/13] pci: Provide sensible irq vector alloc/free  routines Alexander Gordeev <agordeev@redhat.com> - 2016-07-12 14:50 +0200

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

On Wed, Jul 06, 2016 at 10:05:45AM +0200, Alexander Gordeev wrote:
> > + 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.

> > +	/* 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.

> > +	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.

> > +		unsigned int flags)
> > +{
> > +	if (min_vecs > 1)
> > +		return -ENOSPC;
> 
> In case CONFIG_PCI_MSI is unset min_vecs > 1 is -EINVAL;

Ok, fixed.

[toc] | [next] | [standalone]

#1440456

On Sun, Jul 10, 2016 at 05:47:37AM +0200, Christoph Hellwig wrote:
> On Wed, Jul 06, 2016 at 10:05:45AM +0200, Alexander Gordeev wrote:
> > > + 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". 

I will try to paraphrase myself. The new API deprecates pci_enable_msi*_range
functions, but I am not that sure about others. Certainly, pci_msi*_vec_count
and pci_enable_msi*_exact could have (and AFAIR do have) uses that can not be
covered by automatic initialization of pci_alloc_irq_vectors().

[toc] | [prev] | [next] | [standalone]

#1441166

On Mon, Jul 11, 2016 at 12:43:41PM +0200, Alexander Gordeev wrote:
> > I diagreed - if we deprecated functions the only thing that should
> > be mentioned is a "don't use these". 
> 
> I will try to paraphrase myself. The new API deprecates pci_enable_msi*_range
> functions, but I am not that sure about others. Certainly, pci_msi*_vec_count
> and pci_enable_msi*_exact could have (and AFAIR do have) uses that can not be
> covered by automatic initialization of pci_alloc_irq_vectors().

pci_enable_msi*_exact is the equivalent of pci_enable_msi*_range
with minvecs == maxvecs and treating any return value >= 0 as 0.

I've updated the documentation so that the old usage examples are kept
around, but now use pci_alloc_irq_vectors.  I've also added a more detaild
blurb on pci_msi*_vec_count - I think there is no need for them, but
if I'm proven wrong we'll have to add a pci_irq_vector_count that handles
all interrupt types later.

[toc] | [prev] | [next] | [standalone]

#1441317

On Tue, Jul 12, 2016 at 11:13:00AM +0200, Christoph Hellwig wrote:
> On Mon, Jul 11, 2016 at 12:43:41PM +0200, Alexander Gordeev wrote:
> > > I diagreed - if we deprecated functions the only thing that should
> > > be mentioned is a "don't use these". 
> > 
> > I will try to paraphrase myself. The new API deprecates pci_enable_msi*_range
> > functions, but I am not that sure about others. Certainly, pci_msi*_vec__ount
> > and pci_enable_msi*_exact could have (and AFAIR do have) uses that can not be
> > covered by automatic initialization of pci_alloc_irq_vectors().
> 
> pci_enable_msi*_exact is the equivalent of pci_enable_msi*_range
> with minvecs == maxvecs and treating any return value >= 0 as 0.

Right. And people asked explicitly to introduce these helpers when
range functions were introduced in the first place. Since there is
handful of drivers that do use pci_enable_msi*_exact() I suppose a
need for them persists.

> I've updated the documentation so that the old usage examples are kept
> around, but now use pci_alloc_irq_vectors.  I've also added a more detaild
> blurb on pci_msi*_vec_count - I think there is no need for them, but
> if I'm proven wrong we'll have to add a pci_irq_vector_count that handles
> all interrupt types later.

I guess, it is up to Bjorn. But.

Your proposed pci_nr_irq_vectors() function (a) is not a replacement for
pci_msi*_vec_count() and (b) would be useless if I read its description
properly:

(a) Functions pci_msi*_vec_count() return number of vectors reported by
a PCI device. It is a constant for the device and a driver may make an
assumption based on this number;

(b) A number returned by pci_nr_irq_vectors() is not guaranteed what a
following call to pci_alloc_irq_vectors() can return (since the number
of actually allocated vectors might change between the two calls).
Therefore, a value returned by pci_nr_irq_vectors() can not be used for
anything.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web