On 18.02.21 16:01:56, Andy Shevchenko wrote:

The problem this series solves is an imbalanced API.

This (added) API is bloated and incomplete. It adds functions without
benefit, the only is to have a single pcim alloc function in addition
to the pairing of alloc/free functions. I agree, it is hard to detect
which parts are released if pcim_enable_device() is used.

Additional, you need to go through pcim_release() to add other
pcim_*() functions for everything else that is released there.
Otherwise that new API is still incomplete. But this adds another
bunch of useless functions.

Christoph IIRC was clear that if we want to use PCI IRQ allocation API the
caller must know what's going on. Hiding this behind the scenes is not good.
And this series unhides that.

IMO, this is more a documentation issue. pcim_enable_device() must be
better documented and list all enable/alloc functions that are going
to be released out of the box later.

Even better, make sure everything is managed and thus all of a pci_dev
is released, no matter how it was setup (this could even already be
the case).

In addition you could implement a static code checker.

Also, you may go and clean up all pci_free_irq_vectors() when
pcim_enable_device() is called, but I guess you will get painful process and
rejection in a pile of cases.

Why should something be rejected if it is not correctly freed?

Even if pci_free_irq_vectors() is called, pcim_release() will not
complain if it was already freed before. So using
pci_free_irq_vectors() is ok even in conjunction with
pcim_enable_device().

In the end, let's make sure everything is released in pci_dev if it is
managed and document this.

Thanks,

-Robert