On Tue, Feb 16, 2021 at 06:46:01PM +0100, Krzysztof Wilczyński wrote:
Hi Krzysztof,

Hi Dejin,

Thank you for all the changes, looks good!

You could improve the subject line, as it is very vague - what is the
new function name more correct?  Was the other and/or the previous one
not correct?  Seems like you are correcting a typo of sorts, rather than
introducing a new function in this file.

If you have read the following commit comments, As you know, the
pci_alloc_irq_vectors() is not a real device-managed function. But
in some specific cases, it will act as an device-managed function.
Such naming will cause controversy, So In the case of need device-managed,
should be used pcim_alloc_irq_vectors(), an explicit device-managed
function. So the subject name is "Use the correct name of device-managed function".

quoted

Use the new function pcim_alloc_irq_vectors() to allocate IRQ vectors,
the pcim_alloc_irq_vectors() function, an explicit device-managed
version of pci_alloc_irq_vectors(). If pcim_enable_device() has been
called before, then pci_alloc_irq_vectors() is actually
a device-managed function. It is used here as a device-managed
function, So replace it with pcim_alloc_irq_vectors().

The commit is good, but it could use some polish, so to speak.

A few suggestions to think about:

  - What are we adding and/or changing, and why
  - Why is using pcim_alloc_irq_vectors(), which is part
    of the managed devices framework, a better alternative
    to the pci_alloc_irq_vectors()
  - And finally why this change allowed us to remove the
    pci_free_irq_vectors()

These are all explained by the device-managed function mechanism.

quoted

At the same time, simplify the error handling path.

The change simplifies the error handling path, how?  A line of two which
explains how it has been achieved might help should someone reads the
commit message in the future.

To put it simply, if the driver probe fail, the device-managed function
mechanism will automatically call pcim_release(), then the pci_free_irq_vectors()
will be executed. For details, please see the relevant code.

[...]

quoted

 	if (controller->setup) {
 		r = controller->setup(pdev, controller);
-		if (r) {
-			pci_free_irq_vectors(pdev);
+		if (r)
 			return r;
-		}
 	}
 
 	i2c_dw_adjust_bus_speed(dev);

@@ -246,10 +244,8 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev,
 		i2c_dw_acpi_configure(&pdev->dev);
 
 	r = i2c_dw_validate_speed(dev);
-	if (r) {
-		pci_free_irq_vectors(pdev);
+	if (r)
 		return r;
-	}
 
 	i2c_dw_configure(dev);
 

@@ -269,10 +265,8 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev,
 	adap->nr = controller->bus_num;
 
 	r = i2c_dw_probe(dev);
-	if (r) {
-		pci_free_irq_vectors(pdev);
+	if (r)
 		return r;
-	}
 
 	pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
 	pm_runtime_use_autosuspend(&pdev->dev);

@@ -292,7 +286,6 @@ static void i2c_dw_pci_remove(struct pci_dev *pdev)
 
 	i2c_del_adapter(&dev->adapter);
 	devm_free_irq(&pdev->dev, dev->irq, dev);
-	pci_free_irq_vectors(pdev);

If pcim_release() is called should the pci_driver's probe callback fail,

Yes, you guessed right.

and I assume that this is precisely the case, then all of the above make
sense in the view of using pcim_alloc_irq_vectors().

Reviewed-by: Krzysztof Wilczyński <redacted>

Krzysztof

BR,
Dejin