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

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

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...?

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?

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

-- 
With Best Regards,
Andy Shevchenko