diff mbox series

[V2,05/10] PCI/MSI: Switch to MSI descriptor locking to guard()

Message ID 20250313130321.695027112@linutronix.de (mailing list archive)
State New
Headers show
Series genirq/msi: Spring cleaning | expand

Commit Message

Thomas Gleixner March 13, 2025, 1:03 p.m. UTC 
Convert the code to use the new guard(msi_descs_lock).

No functional change intended.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: linux-pci@vger.kernel.org
---
V2: Remove the gotos - Jonathan
---
 drivers/pci/msi/api.c |    6 --
 drivers/pci/msi/msi.c |  121 ++++++++++++++++++++++++--------------------------
 2 files changed, 62 insertions(+), 65 deletions(-)

Comments

Jonathan Cameron March 13, 2025, 3:50 p.m. UTC | #1 
On Thu, 13 Mar 2025 14:03:44 +0100 (CET)
Thomas Gleixner <tglx@linutronix.de> wrote:

> Convert the code to use the new guard(msi_descs_lock).
> 
> No functional change intended.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: linux-pci@vger.kernel.org
> ---
> V2: Remove the gotos - Jonathan
Hi Thomas,


There is a bit of the original code that is carried forwards here
that superficially seemed overly complex.  However as far as I can tell
this is functionally the same as you intended.  So with that in mind
if my question isn't complete garbage, maybe a readability issue for
another day.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> --- a/drivers/pci/msi/msi.c
> +++ b/drivers/pci/msi/msi.c



> +static int msix_setup_interrupts(struct pci_dev *dev, struct msix_entry *entries,
> +				 int nvec, struct irq_affinity *affd)
> +{
> +	struct irq_affinity_desc *masks __free(kfree) =
> +		affd ? irq_create_affinity_masks(nvec, affd) : NULL;
> +
> +	guard(msi_descs_lock)(&dev->dev);
> +	int ret = __msix_setup_interrupts(dev, entries, nvec, masks);
> +	if (ret)
> +		pci_free_msi_irqs(dev);

It's not immediately obvious what this is undoing (i.e. where the alloc
is).  I think it is at least mostly the pci_msi_setup_msi_irqs in
__msix_setup_interrupts

Why not handle the error in __msix_setup_interrupts and make that function
side effect free.  Does require gotos but in a function that isn't
doing any cleanup magic so should be fine.

Mind you I'm not following the logic in msix_setup_interrupts()
before this series either. i.e. why doesn't msix_setup_msi_descs()
clean up after itself on failure (i.e. undo loop iterations that
weren't failures) as that at least superficially looks like it
would give more readable code.

So this is the same as current and as such the patch is fine I think.

>  	return ret;
>  }
>
Thomas Gleixner March 13, 2025, 5:55 p.m. UTC | #2 
On Thu, Mar 13 2025 at 15:50, Jonathan Cameron wrote:
>> +	guard(msi_descs_lock)(&dev->dev);
>> +	int ret = __msix_setup_interrupts(dev, entries, nvec, masks);
>> +	if (ret)
>> +		pci_free_msi_irqs(dev);
>
> It's not immediately obvious what this is undoing (i.e. where the alloc
> is).  I think it is at least mostly the pci_msi_setup_msi_irqs in
> __msix_setup_interrupts

It's a universal cleanup for all possible error cases.

> Why not handle the error in __msix_setup_interrupts and make that function
> side effect free.  Does require gotos but in a function that isn't
> doing any cleanup magic so should be fine.

I had the gotos first and then hated them. But you are right, it's better
to have them than having the magic clean up at the call site.

I'll fold the delta patch below.

Thanks,

        tglx
---
--- a/drivers/pci/msi/msi.c
+++ b/drivers/pci/msi/msi.c
@@ -671,19 +671,23 @@ static int __msix_setup_interrupts(struc
 	int ret = msix_setup_msi_descs(dev, entries, nvec, masks);
 
 	if (ret)
-		return ret;
+		goto fail;
 
 	ret = pci_msi_setup_msi_irqs(dev, nvec, PCI_CAP_ID_MSIX);
 	if (ret)
-		return ret;
+		goto fail;
 
 	/* Check if all MSI entries honor device restrictions */
 	ret = msi_verify_entries(dev);
 	if (ret)
-		return ret;
+		goto fail;
 
 	msix_update_entries(dev, entries);
 	return 0;
+
+fail:
+	pci_free_msi_irqs(dev);
+	return ret;
 }
 
 static int msix_setup_interrupts(struct pci_dev *dev, struct msix_entry *entries,
@@ -693,10 +697,7 @@ static int msix_setup_interrupts(struct
 		affd ? irq_create_affinity_masks(nvec, affd) : NULL;
 
 	guard(msi_descs_lock)(&dev->dev);
-	int ret = __msix_setup_interrupts(dev, entries, nvec, masks);
-	if (ret)
-		pci_free_msi_irqs(dev);
-	return ret;
+	return __msix_setup_interrupts(dev, entries, nvec, masks);
 }
 
 /**
Peter Zijlstra March 14, 2025, 9:42 a.m. UTC | #3 
On Thu, Mar 13, 2025 at 06:55:12PM +0100, Thomas Gleixner wrote:
> On Thu, Mar 13 2025 at 15:50, Jonathan Cameron wrote:
> >> +	guard(msi_descs_lock)(&dev->dev);
> >> +	int ret = __msix_setup_interrupts(dev, entries, nvec, masks);
> >> +	if (ret)
> >> +		pci_free_msi_irqs(dev);
> >
> > It's not immediately obvious what this is undoing (i.e. where the alloc
> > is).  I think it is at least mostly the pci_msi_setup_msi_irqs in
> > __msix_setup_interrupts
> 
> It's a universal cleanup for all possible error cases.
> 
> > Why not handle the error in __msix_setup_interrupts and make that function
> > side effect free.  Does require gotos but in a function that isn't
> > doing any cleanup magic so should be fine.
> 
> I had the gotos first and then hated them. But you are right, it's better
> to have them than having the magic clean up at the call site.
> 
> I'll fold the delta patch below.
> 
> Thanks,
> 
>         tglx
> ---
> --- a/drivers/pci/msi/msi.c
> +++ b/drivers/pci/msi/msi.c
> @@ -671,19 +671,23 @@ static int __msix_setup_interrupts(struc
>  	int ret = msix_setup_msi_descs(dev, entries, nvec, masks);
>  
>  	if (ret)
> -		return ret;
> +		goto fail;
>  
>  	ret = pci_msi_setup_msi_irqs(dev, nvec, PCI_CAP_ID_MSIX);
>  	if (ret)
> -		return ret;
> +		goto fail;
>  
>  	/* Check if all MSI entries honor device restrictions */
>  	ret = msi_verify_entries(dev);
>  	if (ret)
> -		return ret;
> +		goto fail;
>  
>  	msix_update_entries(dev, entries);
>  	return 0;
> +
> +fail:
> +	pci_free_msi_irqs(dev);
> +	return ret;
>  }

How about something like:

int __msix_setup_interrupts(struct device *_dev,... )
{
	struct device *dev __free(msi_irqs) = _dev;

	int ret = msix_setup_msi_descs(dev, entries, nvec, masks);
	if (ret)
		return ret;

	ret = pci_msi_setup_msi_irqs(dev, nvec, PCI_CAP_ID_MSIX);
	if (ret)
		return ret;

	/* Check if all MSI entries honor device restrictions */
	ret = msi_verify_entries(dev);
	if (ret)
		return ret;

	retain_ptr(dev);
	msix_update_entries(dev, entries);
	return 0;
}

?
diff mbox series

Patch

--- a/drivers/pci/msi/api.c
+++ b/drivers/pci/msi/api.c
@@ -53,10 +53,9 @@  void pci_disable_msi(struct pci_dev *dev
 	if (!pci_msi_enabled() || !dev || !dev->msi_enabled)
 		return;
 
-	msi_lock_descs(&dev->dev);
+	guard(msi_descs_lock)(&dev->dev);
 	pci_msi_shutdown(dev);
 	pci_free_msi_irqs(dev);
-	msi_unlock_descs(&dev->dev);
 }
 EXPORT_SYMBOL(pci_disable_msi);
 
@@ -196,10 +195,9 @@  void pci_disable_msix(struct pci_dev *de
 	if (!pci_msi_enabled() || !dev || !dev->msix_enabled)
 		return;
 
-	msi_lock_descs(&dev->dev);
+	guard(msi_descs_lock)(&dev->dev);
 	pci_msix_shutdown(dev);
 	pci_free_msi_irqs(dev);
-	msi_unlock_descs(&dev->dev);
 }
 EXPORT_SYMBOL(pci_disable_msix);
 
--- a/drivers/pci/msi/msi.c
+++ b/drivers/pci/msi/msi.c
@@ -336,41 +336,11 @@  static int msi_verify_entries(struct pci
 	return !entry ? 0 : -EIO;
 }
 
-/**
- * msi_capability_init - configure device's MSI capability structure
- * @dev: pointer to the pci_dev data structure of MSI device function
- * @nvec: number of interrupts to allocate
- * @affd: description of automatic IRQ affinity assignments (may be %NULL)
- *
- * Setup the MSI capability structure of the device with the requested
- * number of interrupts.  A return value of zero indicates the successful
- * setup of an entry with the new MSI IRQ.  A negative return value indicates
- * an error, and a positive return value indicates the number of interrupts
- * which could have been allocated.
- */
-static int msi_capability_init(struct pci_dev *dev, int nvec,
-			       struct irq_affinity *affd)
+static int __msi_capability_init(struct pci_dev *dev, int nvec, struct irq_affinity_desc *masks)
 {
-	struct irq_affinity_desc *masks = NULL;
+	int ret = msi_setup_msi_desc(dev, nvec, masks);
 	struct msi_desc *entry, desc;
-	int ret;
-
-	/* Reject multi-MSI early on irq domain enabled architectures */
-	if (nvec > 1 && !pci_msi_domain_supports(dev, MSI_FLAG_MULTI_PCI_MSI, ALLOW_LEGACY))
-		return 1;
-
-	/*
-	 * Disable MSI during setup in the hardware, but mark it enabled
-	 * so that setup code can evaluate it.
-	 */
-	pci_msi_set_enable(dev, 0);
-	dev->msi_enabled = 1;
-
-	if (affd)
-		masks = irq_create_affinity_masks(nvec, affd);
 
-	msi_lock_descs(&dev->dev);
-	ret = msi_setup_msi_desc(dev, nvec, masks);
 	if (ret)
 		goto fail;
 
@@ -399,19 +369,48 @@  static int msi_capability_init(struct pc
 
 	pcibios_free_irq(dev);
 	dev->irq = entry->irq;
-	goto unlock;
-
+	return 0;
 err:
 	pci_msi_unmask(&desc, msi_multi_mask(&desc));
 	pci_free_msi_irqs(dev);
 fail:
 	dev->msi_enabled = 0;
-unlock:
-	msi_unlock_descs(&dev->dev);
-	kfree(masks);
 	return ret;
 }
 
+/**
+ * msi_capability_init - configure device's MSI capability structure
+ * @dev: pointer to the pci_dev data structure of MSI device function
+ * @nvec: number of interrupts to allocate
+ * @affd: description of automatic IRQ affinity assignments (may be %NULL)
+ *
+ * Setup the MSI capability structure of the device with the requested
+ * number of interrupts.  A return value of zero indicates the successful
+ * setup of an entry with the new MSI IRQ.  A negative return value indicates
+ * an error, and a positive return value indicates the number of interrupts
+ * which could have been allocated.
+ */
+static int msi_capability_init(struct pci_dev *dev, int nvec,
+			       struct irq_affinity *affd)
+{
+	/* Reject multi-MSI early on irq domain enabled architectures */
+	if (nvec > 1 && !pci_msi_domain_supports(dev, MSI_FLAG_MULTI_PCI_MSI, ALLOW_LEGACY))
+		return 1;
+
+	/*
+	 * Disable MSI during setup in the hardware, but mark it enabled
+	 * so that setup code can evaluate it.
+	 */
+	pci_msi_set_enable(dev, 0);
+	dev->msi_enabled = 1;
+
+	struct irq_affinity_desc *masks __free(kfree) =
+		affd ? irq_create_affinity_masks(nvec, affd) : NULL;
+
+	guard(msi_descs_lock)(&dev->dev);
+	return __msi_capability_init(dev, nvec, masks);
+}
+
 int __pci_enable_msi_range(struct pci_dev *dev, int minvec, int maxvec,
 			   struct irq_affinity *affd)
 {
@@ -666,37 +665,37 @@  static void msix_mask_all(void __iomem *
 		writel(ctrl, base + PCI_MSIX_ENTRY_VECTOR_CTRL);
 }
 
-static int msix_setup_interrupts(struct pci_dev *dev, struct msix_entry *entries,
-				 int nvec, struct irq_affinity *affd)
+static int __msix_setup_interrupts(struct pci_dev *dev, struct msix_entry *entries,
+				   int nvec, struct irq_affinity_desc *masks)
 {
-	struct irq_affinity_desc *masks = NULL;
-	int ret;
+	int ret = msix_setup_msi_descs(dev, entries, nvec, masks);
 
-	if (affd)
-		masks = irq_create_affinity_masks(nvec, affd);
-
-	msi_lock_descs(&dev->dev);
-	ret = msix_setup_msi_descs(dev, entries, nvec, masks);
 	if (ret)
-		goto out_free;
+		return ret;
 
 	ret = pci_msi_setup_msi_irqs(dev, nvec, PCI_CAP_ID_MSIX);
 	if (ret)
-		goto out_free;
+		return ret;
 
 	/* Check if all MSI entries honor device restrictions */
 	ret = msi_verify_entries(dev);
 	if (ret)
-		goto out_free;
+		return ret;
 
 	msix_update_entries(dev, entries);
-	goto out_unlock;
+	return 0;
+}
 
-out_free:
-	pci_free_msi_irqs(dev);
-out_unlock:
-	msi_unlock_descs(&dev->dev);
-	kfree(masks);
+static int msix_setup_interrupts(struct pci_dev *dev, struct msix_entry *entries,
+				 int nvec, struct irq_affinity *affd)
+{
+	struct irq_affinity_desc *masks __free(kfree) =
+		affd ? irq_create_affinity_masks(nvec, affd) : NULL;
+
+	guard(msi_descs_lock)(&dev->dev);
+	int ret = __msix_setup_interrupts(dev, entries, nvec, masks);
+	if (ret)
+		pci_free_msi_irqs(dev);
 	return ret;
 }
 
@@ -871,13 +870,13 @@  void __pci_restore_msix_state(struct pci
 
 	write_msg = arch_restore_msi_irqs(dev);
 
-	msi_lock_descs(&dev->dev);
-	msi_for_each_desc(entry, &dev->dev, MSI_DESC_ALL) {
-		if (write_msg)
-			__pci_write_msi_msg(entry, &entry->msg);
-		pci_msix_write_vector_ctrl(entry, entry->pci.msix_ctrl);
+	scoped_guard (msi_descs_lock, &dev->dev) {
+		msi_for_each_desc(entry, &dev->dev, MSI_DESC_ALL) {
+			if (write_msg)
+				__pci_write_msi_msg(entry, &entry->msg);
+			pci_msix_write_vector_ctrl(entry, entry->pci.msix_ctrl);
+		}
 	}
-	msi_unlock_descs(&dev->dev);
 
 	pci_msix_clear_and_set_ctrl(dev, PCI_MSIX_FLAGS_MASKALL, 0);
 }