On Fri, 2024-10-11 at 16:50 +0300, Andy Shevchenko wrote:

On Fri, Oct 11, 2024 at 02:16:06PM +0200, Philipp Stanner wrote:

quoted

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

quoted

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().

quoted

quoted

quoted

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.

It seems I got confused by these two paragraphs. Why the double
underscored
function is even mentioned here?

It's mentioned because it's being moved.

quoted

quoted

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

quoted

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.

No, the body gets one level off.

quoted

Plus, I just copied the code.

quoted

quoted

+		pci_write_config_word(pdev, PCI_COMMAND, new);

	if (new == pci_command)
		return;

	pci_write_config_word(pdev, PCI_COMMAND, new);

See the difference?
Also, imaging adding a new code in your case:

	if (new != pci_command)
		pci_write_config_word(pdev, PCI_COMMAND, new);

==>

	if (new != pci_command) {
		...foo...
		pci_write_config_word(pdev, PCI_COMMAND, new);
		...bar...
	}

And in mine:

	if (new == pci_command)
		return;

	...foo...
	pci_write_config_word(pdev, PCI_COMMAND, new);
	...bar...

I hope it's clear now what I meant.

It is clear.. I'm not necessarily convinced that it's better to review
than just copying the pre-existing code, but if you really want it we
can do it I guess.

P.