Re: [PATCH v7 1/4] PCI: Introduce pcim_alloc_irq_vectors()

Andy Shevchenko
2022-12-16T08:34:34Z

On Thu, Jun 17, 2021 at 10:58:23AM -0500, Bjorn Helgaas wrote:
> On Thu, Jun 17, 2021 at 04:20:16PM +0300, Andy Shevchenko wrote:
> > On Wed, Jun 16, 2021 at 02:25:43PM -0500, Bjorn Helgaas wrote:
> > > On Fri, Jun 11, 2021 at 12:37:22PM +0300, Andy Shevchenko wrote:
> > > > On Thu, Jun 10, 2021 at 05:41:43PM -0500, Bjorn Helgaas wrote:
> > > > > On Mon, Jun 07, 2021 at 11:39:13PM +0800, Dejin Zheng wrote:
> > > > > > Introduce pcim_alloc_irq_vectors(), a device-managed version of
> > > > > > pci_alloc_irq_vectors(). Introducing this function can simplify
> > > > > > the error handling path in many drivers.
> > > > > >
> > > > > > And use pci_free_irq_vectors() to replace some code in pcim_release(),
> > > > > > they are equivalent, and no functional change. It is more explicit
> > > > > > that pcim_alloc_irq_vectors() is a device-managed function.

...

> > > > > > @@ -1989,10 +1989,7 @@ static void pcim_release(struct device *gendev, void *res)
> > > > > > struct pci_devres *this = res;
> > > > > > int i;
> > > > > >
> > > > > > - if (dev->msi_enabled)
> > > > > > - pci_disable_msi(dev);
> > > > > > - if (dev->msix_enabled)
> > > > > > - pci_disable_msix(dev);
> > > > > > + pci_free_irq_vectors(dev);
> > > > >
> > > > > If I understand correctly, this hunk is a nice simplification, but
> > > > > actually has nothing to do with making pcim_alloc_irq_vectors(). I
> > > > > have it split to a separate patch in my local tree. Or am I wrong
> > > > > about that?
> > > >
> > > > It's a good simplification that had to be done when pci_free_irq_vectors()
> > > > appeared.
> > >
> > > Sorry to be pedantic. You say the simplification "had to be done,"
> > > but AFAICT there was no actual *requirement* for this simplification
> > > to be done since pci_free_irq_vectors() is functionally identical to
> > > the previous code.
> > > I think we should do it because it's a little
> > > simpler, but not because it *fixes* anything.
> >
> > It makes things more straightforward. So it definitely "fixes" something, but
> > not the code in this case, rather how we maintain this code.
> >
> > > > But here is the fact that indirectly it's related to the pcim_*()
> > > > APIs, i.e. pcim_alloc_irq_vectors(), because you may noticed this is inside
> > > > pcim_release().
> > >
> > > Yes. For posterity, my notes about the call chain (after applying
> > > this patch):
> > >
> > > pci_alloc_irq_vectors
> > > pci_alloc_irq_vectors_affinity
> > > __pci_enable_msix_range # MSI-X path
> > > __pci_enable_msix
> > > msix_capability_init
> > > msix_setup_entries
> > > for (...)
> > > entry = alloc_msi_entry
> > > kzalloc(msi_desc) <--- alloc
> > > kmemdup(msi_desc->affinity) <--- alloc
> > > dev->msix_enabled = 1 # MSI-X enabled
> > > __pci_enable_msi_range # MSI path
> > > msi_capability_init
> > > msi_setup_entry
> > > alloc_msi_entry <--- alloc
> > > dev->msi_enabled = 1 # MSI enabled
> > >
> > > pcim_release
> > > pci_free_irq_vectors
> > > pci_disable_msix # MSI-X
> > > if (!dev->msix_enabled)
> > > return
> > > pci_msix_shutdown
> > > dev->msix_enabled = 0 # MSI-X disabled
> > > free_msi_irqs
> > > list_for_each_entry_safe(..., msi_list, ...)
> > > free_msi_entry
> > > kfree(msi_desc->affinity) <--- free
> > > kfree(msi_desc) <--- free
> > > pci_disable_msi # MSI
> > > if (!dev->msi_enabled)
> > > return
> > > pci_msi_shutdown
> > > dev->msi_enabled = 0 # MSI disabled
> > > free_msi_irqs <--- free
> > >
> > > So I *think* (correct me if I'm wrong):
> > >
> > > - If a driver calls pcim_enable_device(), we will call
> > > pcim_release() when the last reference to the device is dropped.
> > >
> > > - pci_alloc_irq_vectors() allocates msi_desc and irq_affinity_desc
> > > structures via msix_setup_entries() or msi_setup_entry().
> > >
> > > - pcim_release() will free those msi_desc and irq_affinity_desc
> > > structures.
> > >
> > > - Even before this series, pcim_release() frees msi_desc and
> > > irq_affinity_desc structures by calling pci_disable_msi() and
> > > pci_disable_msix().
> > >
> > > - Calling pci_free_irq_vectors() (or pci_disable_msi() or
> > > pci_disable_msix()) twice is unnecessary but probably harmless
> > > because they bail out early.
> >
> > > So this series actually does not fix any problems whatsoever.
> >
> > I tend to disagree.
> >
> > The PCI managed API is currently inconsistent and what you got is
> > what I already know and had been using until (see below) Christoph
> > told not to do [1].
> >
> > Even do you as PCI maintainer it took some time to figure this out.
> > But current APIs make it hard for mere users who wants to use it in
> > the drivers.
> >
> > So, main point of fix here is _API inconsistency_ [0].
> >
> > But hey, I believe you have been Cc'ed to the initial submission of
> > the pci_*_irq_vector*() rework done by Christoph [2] (hmm... don't
> > see your name there). And he updated documentation as well [3].
> >
> > Moreover, he insisted to use pci_free_irq_vectors() whenever we are
> > using pci_alloc_irq_vectors(). And he suggested if we want to avoid
> > this we have to make pcim_ variant of the API (see [1] again).
>
> I'd like to consider this, but it's hard without a reference :)

Sorry it took a bit too long to answer here.

https://lore.kernel.org/linux-serial/0250c46e-da6c-71f3-50ae-b7c17fd0bd2c@siemens.com/T/#u

> I do think it would be helpful to have clear guidance about when
> drivers need to use pci_free_irq_vectors(). The existing text in
> msi-howto.rst doesn't address pcim_ at all.

Christoph suggested to create an explicit managed API. For now it may be
looking the same as non-managed, but by design it will require different
approaches in case it divert.

> > Maybe you, guys, should got some agreement and clarify it in the
> > documentation?
>
> I agree that the pcim_*() API is confusing at best and it would be
> nice to improve it and document it, but I don't think this series
> really does it.
>
> There are several MSI-related interfaces that use alloc_msi_entry()
> and hence magically become managed if we call pcim_enable_device():
>
> pci_alloc_irq_vectors() # ~150 callers
> pci_alloc_irq_vectors_affinity() # ~10 callers
> pci_enable_msix_exact() # ~20 callers (deprecated)
> pci_enable_msix_range() # ~50 callers (deprecated)
> pci_enable_msi() # ~100 callers (deprecated)
>
> This series adds pcim_alloc_irq_vectors(), which sort of fixes *one*
> of them and makes this sequence look nice:
>
> pcim_enable_device();
> pcim_alloc_irq_vectors();
>
> but all the others are still potentially managed even though the name
> doesn't indicate it.

> And it really doesn't improve the documentation.
>
> Possible steps forward:
>
> - Add comments in include/linux/pci.h to indicate deprecation
> (AFAICS, deprecation is currently only mentioned in
> msi-howto.rst).
>
> - Migrate callers away from deprecated interfaces (a lot of work).

AFAIU you the proposal is to convert all drivers to use explicit
error handling for pci_alloc_irq_vectors() and then undo that after
introducing pcim_alloc_irq_vectors().

Why not having less churn by applying this series and then clean up things?
We have a lot of unneeded churn now because of this rather lexicographical
issue.

> - Remove deprecated interfaces.
>
> - Add pcim_ variants of remaining interfaces (I think only
> pci_alloc_*()). Consider returning error for pci_alloc_*() usage
> by managed drivers.
>
> - Convert managed callers from pci_alloc_*() to pcim_alloc_*() and
> remove usage of pci_free_irq_vectors(), pci_disable_msi(),
> pci_disable_msix().

Seems to me a lot of useless churn, but maybe I'm missing the point?

> > [0]: We have a few functions with pcim_ prefix, few without and some from the
> > latter group imply to behave _differently_ when pcim_enable_device() had
> > been called.
> > [1]: I'm not able to find the archive of the mailing, but I remember that it
> > was something like that IIRC during 8250_lpss.c development.
> > [2]: https://lore.kernel.org/linux-pci/1467621574-8277-1-git-send-email-hch@lst.de/
> > [3]: https://www.kernel.org/doc/html/latest/PCI/msi-howto.html#using-msi
> >
> > > It *does* remove unnecessary pci_free_irq_vectors() calls from
> > > i2c-designware-pcidrv.c.
> > >
> > > But because pci_alloc_irq_vectors() and related interfaces are
> > > *already* managed as soon as a driver calls pcim_enable_device(),
> > > we can simply remove the pci_free_irq_vectors() without doing anything
> > > else.
> > >
> > > I don't think we *should* do anything else.
> >
> > See above.
> >
> > > There are many callers of
> > > pcim_enable_device() that also call pci_alloc_irq_vectors(),
> > > pci_enable_msix_range(), etc. We don't have pcim_enable_msix_range(),
> > > pcim_enable_msi(), pcim_alloc_irq_vectors_affinity(), etc. I don't
> > > think it's worth the churn of adding all those and changing all the
> > > callers to use pcim_*() (as in patch 4/4 here).
> > >
> > > Browsing the output of this:
> > >
> > > git grep -En "pcim_enable_device|pci_alloc_irq_vectors|pci_enable_msix_|pci_free_irq_vectors|pci_disable_msi"
> > >
> > > leads me to believe there are similar calls of pci_free_irq_vectors()
> > > that could be removed here:
> > >
> > > mtip_pci_probe
> > > sp_pci_probe
> > > dw_edma_pcie_probe
> > > hisi_dma_probe
> > > ioat_pci_probe
> > > plx_dma_probe
> > > cci_pci_probe
> > > hibmc_pci_probe
> > > ...
> > >
> > > and many more, but I got tired of looking.
> > >
> > > > > > +/**
> > > > > > + * pcim_alloc_irq_vectors - a device-managed pci_alloc_irq_vectors()
> > > > > > + * @dev: PCI device to operate on
> > > > > > + * @min_vecs: minimum number of vectors required (must be >= 1)
> > > > > > + * @max_vecs: maximum (desired) number of vectors
> > > > > > + * @flags: flags or quirks for the allocation
> > > > > > + *
> > > > > > + * Return the number of vectors allocated, (which might be smaller than
> > > > > > + * @max_vecs) if successful, or a negative error code on error. If less
> > > > > > + * than @min_vecs interrupt vectors are available for @dev the function
> > > > > > + * will fail with -ENOSPC.
> > > > > > + *
> > > > > > + * It depends on calling pcim_enable_device() to make IRQ resources
> > > > > > + * manageable.
> > > > > > + */
> > > > > > +static inline int
> > > > > > +pcim_alloc_irq_vectors(struct pci_dev *dev, unsigned int min_vecs,
> > > > > > + unsigned int max_vecs, unsigned int flags)
> > > > > > +{
> > > > > > + if (!pci_is_managed(dev))
> > > > > > + return -EINVAL;
> > > > > > + return pci_alloc_irq_vectors(dev, min_vecs, max_vecs, flags);
> > > > >
> > > > > This is great, but can you explain how pci_alloc_irq_vectors()
> > > > > magically becomes a managed interface if we've already called
> > > > > pcim_enable_device()?
> > > > >
> > > > > I certainly believe it does; I'd just like to put a hint in the commit
> > > > > log since my 5 minutes of grepping around didn't make it obvious to
> > > > > me.
> > > > >
> > > > > I see that pcim_enable_device() sets pdev->is_managed, but I didn't
> > > > > find the connection between that and pci_alloc_irq_vectors().
> > > >
> > > > One needs to read and understand the code, I agree. The explanation is spread
> > > > between pcim_release() and __pci_enable_msi/x_range().
> > > >
> > > > The call chain is
> > > >
> > > > msi_capability_init() / msix_capability_init()
> > > > ...
> > > > <- __pci_enable_msi/x_range()
> > > > <- pci_alloc_irq_vectors_affinity()
> > > > <- pci_alloc_irq_vectors()
> > > >
> > > > where device msi_enabled / msix_enabled is set.
> > > >
> > > > So, it may deserve to be explained in the commit message.
> > > >
> > > > > > +}

--
With Best Regards,
Andy Shevchenko