On Tue, 2024-08-20 at 13:37 +0300, Andy Shevchenko wrote:

On Tue, Aug 20, 2024 at 09:52:40AM +0200, Philipp Stanner wrote:

quoted

On Mon, 2024-08-19 at 21:28 +0300, Andy Shevchenko wrote:

quoted

On Mon, Aug 19, 2024 at 06:51:47PM +0200, Philipp Stanner wrote:

...

quoted

quoted

loongson_dwmac_probe()

quoted

+	memset(&res, 0, sizeof(res));
+	res.addr = pcim_iomap_region(pdev, 0, pci_name(pdev));
+	if (IS_ERR(res.addr)) {
+		ret = PTR_ERR(res.addr);
+		goto err_disable_device;

It seems your series reveals issues in the error paths of
.probe():s
in many drivers...

If we use pcim variant to enable device, why do we need to
explicitly
disable it?

No.

Can you elaborate? No issues being revealed, or no need to disable it
explicitly, or...?

Oh, my bad, I overlooked your "why" in that question.

We do not explicitly have to disable it. It's wrong / unnecessary, as
many of the other calls you criticized in this series.

pcim_enable_device() (in pci/devres.c) calls devm_add_action(...,
pcim_disable_device,  ...), which will disable the device on driver
detach.

So the call of pci_disable_device() above is redundant. We could remove
it.

quoted

quoted

quoted

 	}

...

quoted

quoted

loongson_dwmac_remove()

quoted

 	pci_disable_msi(pdev);
 	pci_disable_device(pdev);

Not sure why we need these either...

It's complicated.

The code uses pciM_enable_device(), but here in remove
pci_disable_device().

pcim_enable_device() sets up a disable callback which only calls
pci_disable_device() if pcim_pin_device() has not been called.

This code doesn't seem to call pcim_pin_device(), so I think
pci_disable_device() could be removed.


I definitely would not feel confident touching pci_disable_msi(),
though. The AFAIK biggest problem remaining in PCI devres is that
the
MSI code base implicitly calls into devres, see here [1]

But isn't it a busyness of PCI core to call pci_disable_msi() at the
right
moment? Okay, I admit that there might be devices that require a
special
workflow WRT MSI, is this the case here?

I don't know enough about how MSI is intended to be used.

From what I've seen in the code base, pcim_setup_msi_release() does
register a devres callback that will indeed call pci_disable_msi()
after some intermediate calls.

But in my honest opinion, that code is _very_ broken. I was thinking
about how we might clean it up, but couldn't come up with an idea yet.

Only after the code in pci/msi/ has been cleanly separated from
implicit devres I myself would start touching function calls related to
MSI.

That being said, I suspect that one can remove pci_disable_msi() in the
line above. But the risk-benefit-ratio doesn't pay off for me.

P.

quoted

[1]
https://lore.kernel.org/all/ee44ea7ac760e73edad3f20b30b4d2fff66c1a85.camel@redhat.com/ (local)