On Thu, 2024-10-10 at 17:40 +0300, Andy Shevchenko wrote:

On Wed, Oct 09, 2024 at 10:35:07AM +0200, Philipp Stanner wrote:

quoted

pci_intx() is a hybrid function which sometimes performs devres
operations, depending on whether pcim_enable_device() has been used
to
enable the pci_dev. This sometimes-managed nature of the function
is
problematic. Notably, it causes the function to allocate under some
circumstances which makes it unusable from interrupt context.

To, ultimately, remove the hybrid nature from pci_intx(), it is
first
necessary to provide an always-managed and a never-managed version
of that function. Then, all callers of pci_intx() can be ported to
the
version they need, depending whether they use pci_enable_device()
or
pcim_enable_device().

An always-managed function exists, namely pcim_intx(), for which
__pcim_intx(), a never-managed version of pci_intx() had been
implemented.

quoted

Make __pcim_intx() a public function under the name
pci_intx_unmanaged(). Make pcim_intx() a public function.

To avoid an additional churn we can make just completely new APIs,
namely:
pcim_int_x()
pci_int_x()

You won't need all dirty dances with double underscored function
naming and
renaming.

Ähm.. I can't follow. The new version doesn't use double underscores
anymore. __pcim_intx() is being removed, effectively.
After this series, we'd end up with a clean:

	pci_intx() <-> pcim_intx()

just as in the other PCI APIs.

...

quoted

+	pci_read_config_word(pdev, PCI_COMMAND, &pci_command);
+
+	if (enable)
+		new = pci_command & ~PCI_COMMAND_INTX_DISABLE;
+	else
+		new = pci_command | PCI_COMMAND_INTX_DISABLE;
+
+	if (new != pci_command)

I would use positive conditionals as easy to read (yes, a couple of
lines
longer, but also a win is the indentation and avoiding an additional
churn in
the future in case we need to add something in this branch.

I can't follow. You mean:

if (new == pci_command)
    return;

?

That's exactly the same level of indentation. Plus, I just copied the
code.

quoted

+		pci_write_config_word(pdev, PCI_COMMAND, new);

...

Otherwise I'm for the idea in general.

\o/