[PATCH 0/3] VMCI: Various fixes

[PATCH 0/3] VMCI: Various fixes

[Christophe JAILLET]

Patch 1 and 2 should be straighforward.

But review with much more care the 3rd one, as explained below the --- in the
patch.

Christophe JAILLET (3):
  VMCI: Fix the description of vmci_check_host_caps()
  VMCI: No need to clear memory after a dma_alloc_coherent() call
  VMCI: Fix some error handling paths in vmci_guest_probe_device()

 drivers/misc/vmw_vmci/vmci_guest.c | 30 ++++++++++++++----------------
 1 file changed, 14 insertions(+), 16 deletions(-)

-- 
2.32.0

[PATCH 1/3] VMCI: Fix the description of vmci_check_host_caps()

[Christophe JAILLET]

vmci_check_host_caps() doesn't return a bool but an int.
Fix the description accordingly.

Fixes: 	782f24453536 ("VMCI: fix error handling path when registering guest driver")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
 drivers/misc/vmw_vmci/vmci_guest.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/misc/vmw_vmci/vmci_guest.c b/drivers/misc/vmw_vmci/vmci_guest.c
index aa61a687b3e2..1a1858742f75 100644
--- a/drivers/misc/vmw_vmci/vmci_guest.c
+++ b/drivers/misc/vmw_vmci/vmci_guest.c
@@ -253,9 +253,9 @@ static void vmci_guest_cid_update(u32 sub_id,
 
 /*
  * Verify that the host supports the hypercalls we need. If it does not,
- * try to find fallback hypercalls and use those instead.  Returns
- * true if required hypercalls (or fallback hypercalls) are
- * supported by the host, false otherwise.
+ * try to find fallback hypercalls and use those instead.  Returns 0 if
+ * required hypercalls (or fallback hypercalls) are supported by the host,
+ * an error code otherwise.
  */
 static int vmci_check_host_caps(struct pci_dev *pdev)
 {
-- 
2.32.0

[PATCH 2/3] VMCI: No need to clear memory after a dma_alloc_coherent() call

[Christophe JAILLET]

dma_alloc_coherent() already clear the allocated memory, there is no need
to explicitly call memset().
This saves a few cycles and a few lines of code.

Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
 drivers/misc/vmw_vmci/vmci_guest.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/misc/vmw_vmci/vmci_guest.c b/drivers/misc/vmw_vmci/vmci_guest.c
index 1a1858742f75..02d4722d8474 100644
--- a/drivers/misc/vmw_vmci/vmci_guest.c
+++ b/drivers/misc/vmw_vmci/vmci_guest.c
@@ -706,13 +706,11 @@ static int vmci_guest_probe_device(struct pci_dev *pdev,
 		vmci_dev->notification_bitmap = dma_alloc_coherent(
 			&pdev->dev, PAGE_SIZE, &vmci_dev->notification_base,
 			GFP_KERNEL);
-		if (!vmci_dev->notification_bitmap) {
+		if (!vmci_dev->notification_bitmap)
 			dev_warn(&pdev->dev,
 				 "Unable to allocate notification bitmap\n");
-		} else {
-			memset(vmci_dev->notification_bitmap, 0, PAGE_SIZE);
+		else
 			caps_in_use |= VMCI_CAPS_NOTIFICATIONS;
-		}
 	}
 
 	if (mmio_base != NULL) {
-- 
2.32.0

[PATCH 3/3] VMCI: Fix some error handling paths in vmci_guest_probe_device()

[Christophe JAILLET]

The 'err_remove_vmci_dev_g' error label is not at the right place.
This could lead to un-released resource.

There is also a missing label. If pci_alloc_irq_vectors() fails, the
previous vmci_event_subscribe() call must be undone.

Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
Review with GREAT care.

This patch is a recent rebase of an old patch that has never been
submitted.
This function is huge and modifying its error handling path is error
prone (at least for me).

The patch is compile-tested only.
---
 drivers/misc/vmw_vmci/vmci_guest.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/misc/vmw_vmci/vmci_guest.c b/drivers/misc/vmw_vmci/vmci_guest.c
index 02d4722d8474..2e8ad36895ae 100644
--- a/drivers/misc/vmw_vmci/vmci_guest.c
+++ b/drivers/misc/vmw_vmci/vmci_guest.c
@@ -765,7 +765,7 @@ static int vmci_guest_probe_device(struct pci_dev *pdev,
 	/* Check host capabilities. */
 	error = vmci_check_host_caps(pdev);
 	if (error)
-		goto err_remove_bitmap;
+		goto err_remove_vmci_dev_g;
 
 	/* Enable device. */
 
@@ -795,7 +795,7 @@ static int vmci_guest_probe_device(struct pci_dev *pdev,
 		error = pci_alloc_irq_vectors(pdev, 1, 1,
 				PCI_IRQ_MSIX | PCI_IRQ_MSI | PCI_IRQ_LEGACY);
 		if (error < 0)
-			goto err_remove_bitmap;
+			goto err_unsubscrive_event;
 	} else {
 		vmci_dev->exclusive_vectors = true;
 	}
@@ -871,13 +871,19 @@ static int vmci_guest_probe_device(struct pci_dev *pdev,
 err_disable_msi:
 	pci_free_irq_vectors(pdev);
 
+err_unsubscrive_event:
 	vmci_err = vmci_event_unsubscribe(ctx_update_sub_id);
 	if (vmci_err < VMCI_SUCCESS)
 		dev_warn(&pdev->dev,
 			 "Failed to unsubscribe from event (type=%d) with subscriber (ID=0x%x): %d\n",
 			 VMCI_EVENT_CTX_ID_UPDATE, ctx_update_sub_id, vmci_err);
 
-err_remove_bitmap:
+err_remove_vmci_dev_g:
+	spin_lock_irq(&vmci_dev_spinlock);
+	vmci_pdev = NULL;
+	vmci_dev_g = NULL;
+	spin_unlock_irq(&vmci_dev_spinlock);
+
 	if (vmci_dev->notification_bitmap) {
 		vmci_write_reg(vmci_dev, VMCI_CONTROL_RESET, VMCI_CONTROL_ADDR);
 		dma_free_coherent(&pdev->dev, PAGE_SIZE,
@@ -885,12 +891,6 @@ static int vmci_guest_probe_device(struct pci_dev *pdev,
 				  vmci_dev->notification_base);
 	}
 
-err_remove_vmci_dev_g:
-	spin_lock_irq(&vmci_dev_spinlock);
-	vmci_pdev = NULL;
-	vmci_dev_g = NULL;
-	spin_unlock_irq(&vmci_dev_spinlock);
-
 err_free_data_buffers:
 	vmci_free_dg_buffers(vmci_dev);
 
-- 
2.32.0

Re: [PATCH 3/3] VMCI: Fix some error handling paths in vmci_guest_probe_device()

[Dan Carpenter]

On Thu, Feb 10, 2022 at 11:27:34PM +0100, Christophe JAILLET wrote:
> The 'err_remove_vmci_dev_g' error label is not at the right place.
> This could lead to un-released resource.
> 
> There is also a missing label. If pci_alloc_irq_vectors() fails, the
> previous vmci_event_subscribe() call must be undone.
> 
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> Review with GREAT care.
> 
> This patch is a recent rebase of an old patch that has never been
> submitted.
> This function is huge and modifying its error handling path is error
> prone (at least for me).
> 
> The patch is compile-tested only.

There is still one bug.  Sorry if the line numbers are off.

drivers/misc/vmw_vmci/vmci_guest.c
   705                if (capabilities & VMCI_CAPS_NOTIFICATIONS) {
   706                        vmci_dev->notification_bitmap = dma_alloc_coherent(
                                                                  ^^^^^
Alloc

   707                                &pdev->dev, PAGE_SIZE, &vmci_dev->notification_base,
   708                                GFP_KERNEL);
   709                        if (!vmci_dev->notification_bitmap) {
   710                                dev_warn(&pdev->dev,
   711                                         "Unable to allocate notification bitmap\n");
   712                        } else {
   713                                memset(vmci_dev->notification_bitmap, 0, PAGE_SIZE);
   714                                caps_in_use |= VMCI_CAPS_NOTIFICATIONS;
   715                        }
   716                }
   717
   718                if (mmio_base != NULL) {
   719                        if (capabilities & VMCI_CAPS_DMA_DATAGRAM) {
   720                                caps_in_use |= VMCI_CAPS_DMA_DATAGRAM;
   721                        } else {
   722                                dev_err(&pdev->dev,
   723                                        "Missing capability: VMCI_CAPS_DMA_DATAGRAM\n");
   724                                error = -ENXIO;
   725                                goto err_free_data_buffers;

This should be goto err_free_notification_bitmap;

   726                        }
   727                }

On of the rules for error handling is that the unwind code should mirror
the allocation code but instead of that this code will have:

Alloc:
	if (capabilities & VMCI_CAPS_NOTIFICATIONS)
Free:
	if (vmci_dev->notification_bitmap)

It's the same if statement but you wouldn't really know it from just
looking at it so it's confusing.  Whatever...  But where this really
hurts is with:

Alloc:
	if (vmci_dev->exclusive_vectors) {
		error = request_irq(pci_irq_vector(pdev, 1), ...
Free:
	free_irq(pci_irq_vector(pdev, 1), vmci_dev);

No if statement.  It works because it's the last allocation but it's
confusing and fragile.

The other question I had was:

   882        err_remove_bitmap:
   883                if (vmci_dev->notification_bitmap) {
   884                        vmci_write_reg(vmci_dev, VMCI_CONTROL_RESET, VMCI_CONTROL_ADDR);
                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This doesn't mirror anything in the allocation code so who knows if its
done in the correct place/order.

   885                        dma_free_coherent(&pdev->dev, PAGE_SIZE,
   886                                          vmci_dev->notification_bitmap,
   887                                          vmci_dev->notification_base);
   888                }

regards,
dan carpenter

Re: [PATCH 3/3] VMCI: Fix some error handling paths in vmci_guest_probe_device()

[Christophe JAILLET]

Le 11/02/2022 à 15:09, Dan Carpenter a écrit :
> On Thu, Feb 10, 2022 at 11:27:34PM +0100, Christophe JAILLET wrote:
>> The 'err_remove_vmci_dev_g' error label is not at the right place.
>> This could lead to un-released resource.
>>
>> There is also a missing label. If pci_alloc_irq_vectors() fails, the
>> previous vmci_event_subscribe() call must be undone.
>>
>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>> ---
>> Review with GREAT care.
>>
>> This patch is a recent rebase of an old patch that has never been
>> submitted.
>> This function is huge and modifying its error handling path is error
>> prone (at least for me).
>>
>> The patch is compile-tested only.
> 
> There is still one bug.  Sorry if the line numbers are off.

Thanks for the review Dan.
Much appreciated.

> 
> drivers/misc/vmw_vmci/vmci_guest.c
>     705                if (capabilities & VMCI_CAPS_NOTIFICATIONS) {
>     706                        vmci_dev->notification_bitmap = dma_alloc_coherent(
>                                                                    ^^^^^
> Alloc
> 
>     707                                &pdev->dev, PAGE_SIZE, &vmci_dev->notification_base,
>     708                                GFP_KERNEL);
>     709                        if (!vmci_dev->notification_bitmap) {
>     710                                dev_warn(&pdev->dev,
>     711                                         "Unable to allocate notification bitmap\n");
>     712                        } else {
>     713                                memset(vmci_dev->notification_bitmap, 0, PAGE_SIZE);
>     714                                caps_in_use |= VMCI_CAPS_NOTIFICATIONS;
>     715                        }
>     716                }
>     717
>     718                if (mmio_base != NULL) {
>     719                        if (capabilities & VMCI_CAPS_DMA_DATAGRAM) {
>     720                                caps_in_use |= VMCI_CAPS_DMA_DATAGRAM;
>     721                        } else {
>     722                                dev_err(&pdev->dev,
>     723                                        "Missing capability: VMCI_CAPS_DMA_DATAGRAM\n");
>     724                                error = -ENXIO;
>     725                                goto err_free_data_buffers;
> 
> This should be goto err_free_notification_bitmap;

Agreed.
The error handling path still looked odd to me because 2 things were 
undone without a label between the 2 steps.
That was it. An err_free_notification_bitmap should be added and used.
I missed it.

> 
>     726                        }
>     727                }
> 
> On of the rules for error handling is that the unwind code should mirror
> the allocation code but instead of that this code will have:
> 
> Alloc:
> 	if (capabilities & VMCI_CAPS_NOTIFICATIONS)
> Free:
> 	if (vmci_dev->notification_bitmap)
> 
> It's the same if statement but you wouldn't really know it from just
> looking at it so it's confusing.

This one is fine I think. If the allocation of notification_bitmap 
fails, it is not an error. So it looks fine to test it the way it is done.
Or we should have both 'if'.


> Whatever...  But where this really
> hurts is with:
> 
> Alloc:
> 	if (vmci_dev->exclusive_vectors) {
> 		error = request_irq(pci_irq_vector(pdev, 1), ...
> Free:
> 	free_irq(pci_irq_vector(pdev, 1), vmci_dev);
> 
> No if statement.  It works because it's the last allocation but it's
> confusing and fragile.

Agreed.

> 
> The other question I had was:
> 
>     882        err_remove_bitmap:
>     883                if (vmci_dev->notification_bitmap) {
>     884                        vmci_write_reg(vmci_dev, VMCI_CONTROL_RESET, VMCI_CONTROL_ADDR);
>                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> This doesn't mirror anything in the allocation code so who knows if its
> done in the correct place/order.

Agreed. It puzzled me as well.

vmci_guest_remove_device() also has this kind of code, but it is not 
done the same way in this function. It is unconditional and not close to 
the dma_free_coherent() call.
Odd.

I won't touch it by myself :)

> 
>     885                        dma_free_coherent(&pdev->dev, PAGE_SIZE,
>     886                                          vmci_dev->notification_bitmap,
>     887                                          vmci_dev->notification_base);
>     888                }
> 
> regards,
> dan carpenter
> 
> 

All your comments are unrelated to my patch and looks like additional fixes.

Until recently, this file was mostly untouched.
So, let see if a maintainer looks interested in these patches and if he 
prefers a patch that fixes everything or several patches, maybe easier 
to review.

Once again, big thanks.

CJ

Re: [PATCH 1/3] VMCI: Fix the description of vmci_check_host_caps()

[Vishnu Dasa]

> On Feb 10, 2022, at 2:27 PM, Christophe JAILLET <christophe.jaillet@wanadoo.fr> wrote:
> 
> vmci_check_host_caps() doesn't return a bool but an int.
> Fix the description accordingly.
> 
> Fixes: 	782f24453536 ("VMCI: fix error handling path when registering guest driver")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> drivers/misc/vmw_vmci/vmci_guest.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/misc/vmw_vmci/vmci_guest.c b/drivers/misc/vmw_vmci/vmci_guest.c
> index aa61a687b3e2..1a1858742f75 100644
> --- a/drivers/misc/vmw_vmci/vmci_guest.c
> +++ b/drivers/misc/vmw_vmci/vmci_guest.c
> @@ -253,9 +253,9 @@ static void vmci_guest_cid_update(u32 sub_id,
> 
> /*
>  * Verify that the host supports the hypercalls we need. If it does not,
> - * try to find fallback hypercalls and use those instead.  Returns
> - * true if required hypercalls (or fallback hypercalls) are
> - * supported by the host, false otherwise.
> + * try to find fallback hypercalls and use those instead.  Returns 0 if
> + * required hypercalls (or fallback hypercalls) are supported by the host,
> + * an error code otherwise.
>  */
> static int vmci_check_host_caps(struct pci_dev *pdev)
> {
> -- 
> 2.32.0
> 

Thanks!

Acked-by: Vishnu Dasa <vdasa@vmware.com>

Re: [PATCH 2/3] VMCI: No need to clear memory after a dma_alloc_coherent() call

[Vishnu Dasa]

> On Feb 10, 2022, at 2:27 PM, Christophe JAILLET <christophe.jaillet@wanadoo.fr> wrote:
> 
> dma_alloc_coherent() already clear the allocated memory, there is no need
> to explicitly call memset().
> This saves a few cycles and a few lines of code.
> 
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> drivers/misc/vmw_vmci/vmci_guest.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/misc/vmw_vmci/vmci_guest.c b/drivers/misc/vmw_vmci/vmci_guest.c
> index 1a1858742f75..02d4722d8474 100644
> --- a/drivers/misc/vmw_vmci/vmci_guest.c
> +++ b/drivers/misc/vmw_vmci/vmci_guest.c
> @@ -706,13 +706,11 @@ static int vmci_guest_probe_device(struct pci_dev *pdev,
> 		vmci_dev->notification_bitmap = dma_alloc_coherent(
> 			&pdev->dev, PAGE_SIZE, &vmci_dev->notification_base,
> 			GFP_KERNEL);
> -		if (!vmci_dev->notification_bitmap) {
> +		if (!vmci_dev->notification_bitmap)
> 			dev_warn(&pdev->dev,
> 				 "Unable to allocate notification bitmap\n");
> -		} else {
> -			memset(vmci_dev->notification_bitmap, 0, PAGE_SIZE);
> +		else
> 			caps_in_use |= VMCI_CAPS_NOTIFICATIONS;
> -		}
> 	}
> 
> 	if (mmio_base != NULL) {
> -- 
> 2.32.0
> 

Thanks!

Acked-by: Vishnu Dasa <vdasa@vmware.com>

Re: [PATCH 3/3] VMCI: Fix some error handling paths in vmci_guest_probe_device()

[Vishnu Dasa]

> On Feb 10, 2022, at 2:27 PM, Christophe JAILLET <christophe.jaillet@wanadoo.fr> wrote:
> 
> The 'err_remove_vmci_dev_g' error label is not at the right place.
> This could lead to un-released resource.
> 
> There is also a missing label. If pci_alloc_irq_vectors() fails, the
> previous vmci_event_subscribe() call must be undone.
> 
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>

Thanks for the patch!
Let's deal with the points which Dan brought up separately.

Acked-by: Vishnu Dasa <vdasa@vmware.com>

> ---
> Review with GREAT care.
> 
> This patch is a recent rebase of an old patch that has never been
> submitted.
> This function is huge and modifying its error handling path is error
> prone (at least for me).
> 
> The patch is compile-tested only.
> ---
> drivers/misc/vmw_vmci/vmci_guest.c | 18 +++++++++---------
> 1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/misc/vmw_vmci/vmci_guest.c b/drivers/misc/vmw_vmci/vmci_guest.c
> index 02d4722d8474..2e8ad36895ae 100644
> --- a/drivers/misc/vmw_vmci/vmci_guest.c
> +++ b/drivers/misc/vmw_vmci/vmci_guest.c
> @@ -765,7 +765,7 @@ static int vmci_guest_probe_device(struct pci_dev *pdev,
> 	/* Check host capabilities. */
> 	error = vmci_check_host_caps(pdev);
> 	if (error)
> -		goto err_remove_bitmap;
> +		goto err_remove_vmci_dev_g;
> 
> 	/* Enable device. */
> 
> @@ -795,7 +795,7 @@ static int vmci_guest_probe_device(struct pci_dev *pdev,
> 		error = pci_alloc_irq_vectors(pdev, 1, 1,
> 				PCI_IRQ_MSIX | PCI_IRQ_MSI | PCI_IRQ_LEGACY);
> 		if (error < 0)
> -			goto err_remove_bitmap;
> +			goto err_unsubscrive_event;
> 	} else {
> 		vmci_dev->exclusive_vectors = true;
> 	}
> @@ -871,13 +871,19 @@ static int vmci_guest_probe_device(struct pci_dev *pdev,
> err_disable_msi:
> 	pci_free_irq_vectors(pdev);
> 
> +err_unsubscrive_event:

Please rename this to err_unsubscribe_event

> 	vmci_err = vmci_event_unsubscribe(ctx_update_sub_id);
> 	if (vmci_err < VMCI_SUCCESS)
> 		dev_warn(&pdev->dev,
> 			 "Failed to unsubscribe from event (type=%d) with subscriber (ID=0x%x): %d\n",
> 			 VMCI_EVENT_CTX_ID_UPDATE, ctx_update_sub_id, vmci_err);
> 
> -err_remove_bitmap:
> +err_remove_vmci_dev_g:
> +	spin_lock_irq(&vmci_dev_spinlock);
> +	vmci_pdev = NULL;
> +	vmci_dev_g = NULL;
> +	spin_unlock_irq(&vmci_dev_spinlock);
> +
> 	if (vmci_dev->notification_bitmap) {
> 		vmci_write_reg(vmci_dev, VMCI_CONTROL_RESET, VMCI_CONTROL_ADDR);
> 		dma_free_coherent(&pdev->dev, PAGE_SIZE,
> @@ -885,12 +891,6 @@ static int vmci_guest_probe_device(struct pci_dev *pdev,
> 				  vmci_dev->notification_base);
> 	}
> 
> -err_remove_vmci_dev_g:
> -	spin_lock_irq(&vmci_dev_spinlock);
> -	vmci_pdev = NULL;
> -	vmci_dev_g = NULL;
> -	spin_unlock_irq(&vmci_dev_spinlock);
> -
> err_free_data_buffers:
> 	vmci_free_dg_buffers(vmci_dev);
> 
> -- 
> 2.32.0
> 

Re: [PATCH 3/3] VMCI: Fix some error handling paths in vmci_guest_probe_device()

[Vishnu Dasa]

> On Feb 11, 2022, at 12:06 PM, Christophe JAILLET <christophe.jaillet@wanadoo.fr> wrote:
> 
> Le 11/02/2022 à 15:09, Dan Carpenter a écrit :
>> On Thu, Feb 10, 2022 at 11:27:34PM +0100, Christophe JAILLET wrote:
>>> The 'err_remove_vmci_dev_g' error label is not at the right place.
>>> This could lead to un-released resource.
>>> 
>>> There is also a missing label. If pci_alloc_irq_vectors() fails, the
>>> previous vmci_event_subscribe() call must be undone.
>>> 
>>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>>> ---
>>> Review with GREAT care.
>>> 
>>> This patch is a recent rebase of an old patch that has never been
>>> submitted.
>>> This function is huge and modifying its error handling path is error
>>> prone (at least for me).
>>> 
>>> The patch is compile-tested only.
>> There is still one bug.  Sorry if the line numbers are off.
> 
> Thanks for the review Dan.
> Much appreciated.

Thanks, Christophe and Dan!

> 
>> drivers/misc/vmw_vmci/vmci_guest.c
>>    705                if (capabilities & VMCI_CAPS_NOTIFICATIONS) {
>>    706                        vmci_dev->notification_bitmap = dma_alloc_coherent(
>>                                                                   ^^^^^
>> Alloc
>>    707                                &pdev->dev, PAGE_SIZE, &vmci_dev->notification_base,
>>    708                                GFP_KERNEL);
>>    709                        if (!vmci_dev->notification_bitmap) {
>>    710                                dev_warn(&pdev->dev,
>>    711                                         "Unable to allocate notification bitmap\n");
>>    712                        } else {
>>    713                                memset(vmci_dev->notification_bitmap, 0, PAGE_SIZE);
>>    714                                caps_in_use |= VMCI_CAPS_NOTIFICATIONS;
>>    715                        }
>>    716                }
>>    717
>>    718                if (mmio_base != NULL) {
>>    719                        if (capabilities & VMCI_CAPS_DMA_DATAGRAM) {
>>    720                                caps_in_use |= VMCI_CAPS_DMA_DATAGRAM;
>>    721                        } else {
>>    722                                dev_err(&pdev->dev,
>>    723                                        "Missing capability: VMCI_CAPS_DMA_DATAGRAM\n");
>>    724                                error = -ENXIO;
>>    725                                goto err_free_data_buffers;
>> This should be goto err_free_notification_bitmap;
> 
> Agreed.
> The error handling path still looked odd to me because 2 things were undone without a label between the 2 steps.
> That was it. An err_free_notification_bitmap should be added and used.
> I missed it.

Good catch.  This fixes recent code, so a separate patch would be good.
"[PATCH v3 3/8] VMCI: dma dg: detect DMA datagram capability"

> 
>>    726                        }
>>    727                }
>> On of the rules for error handling is that the unwind code should mirror
>> the allocation code but instead of that this code will have:
>> Alloc:
>> 	if (capabilities & VMCI_CAPS_NOTIFICATIONS)
>> Free:
>> 	if (vmci_dev->notification_bitmap)
>> It's the same if statement but you wouldn't really know it from just
>> looking at it so it's confusing.
> 
> This one is fine I think. If the allocation of notification_bitmap fails, it is not an error. So it looks fine to test it the way it is done.
> Or we should have both 'if'.
> 

Right.  And we would need to check 'capabilities & VMCI_CAPS_NOTIFICATIONS',
'caps_in_use & VMCI_CAPS_NOTIFICATIONS' and then
'vmci_dev->notification_bitmap' if we go that route.  I think we can leave it as is.

> 
>> Whatever...  But where this really
>> hurts is with:
>> Alloc:
>> 	if (vmci_dev->exclusive_vectors) {
>> 		error = request_irq(pci_irq_vector(pdev, 1), ...
>> Free:
>> 	free_irq(pci_irq_vector(pdev, 1), vmci_dev);
>> No if statement.  It works because it's the last allocation but it's
>> confusing and fragile.
> 
> Agreed.

Sorry, I hope I'm not missing something obvious or misunderstanding the point.
But I don't think the problem implied here exists?

If 'request_irq(pci_irq_vector(pdev, 0), ...' fails we goto err_disable_msi and there
is no free_irq in this path.  If 'request_irq(pci_irq_vector(pdev, 1), ...' fails then we
goto err_free_irq and we do 'free_irq(pci_irq_vector(pdev, 0), vmci_dev)'.  Note that
this is for the previous one without the check for vmci_dev->exclusive_vectors.

> 
>> The other question I had was:
>>    882        err_remove_bitmap:
>>    883                if (vmci_dev->notification_bitmap) {
>>    884                        vmci_write_reg(vmci_dev, VMCI_CONTROL_RESET, VMCI_CONTROL_ADDR);
>>                               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> This doesn't mirror anything in the allocation code so who knows if its
>> done in the correct place/order.
> 
> Agreed. It puzzled me as well.
> 
> vmci_guest_remove_device() also has this kind of code, but it is not done the same way in this function. It is unconditional and not close to the dma_free_coherent() call.
> Odd.
> 
> I won't touch it by myself :)
> 
>>    885                        dma_free_coherent(&pdev->dev, PAGE_SIZE,
>>    886                                          vmci_dev->notification_bitmap,
>>    887                                          vmci_dev->notification_base);
>>    888                }
>> regards,
>> dan carpenter
> 
> All your comments are unrelated to my patch and looks like additional fixes.
> 
> Until recently, this file was mostly untouched.
> So, let see if a maintainer looks interested in these patches and if he prefers a patch that fixes everything or several patches, maybe easier to review.
> 
> Once again, big thanks.
> 
> CJ

Re: [PATCH 3/3] VMCI: Fix some error handling paths in vmci_guest_probe_device()

[Dan Carpenter]

On Thu, Feb 24, 2022 at 06:53:10AM +0000, Vishnu Dasa wrote:
> 
> > On Feb 11, 2022, at 12:06 PM, Christophe JAILLET <christophe.jaillet@wanadoo.fr> wrote:
> > 
> > Le 11/02/2022 à 15:09, Dan Carpenter a écrit :
> >> On Thu, Feb 10, 2022 at 11:27:34PM +0100, Christophe JAILLET wrote:

> >> Whatever...  But where this really
> >> hurts is with:
> >> Alloc:
> >> 	if (vmci_dev->exclusive_vectors) {
> >> 		error = request_irq(pci_irq_vector(pdev, 1), ...
> >> Free:
> >> 	free_irq(pci_irq_vector(pdev, 1), vmci_dev);
> >> No if statement.  It works because it's the last allocation but it's
> >> confusing and fragile.
> > 
> > Agreed.
> 
> Sorry, I hope I'm not missing something obvious or misunderstanding the point.
> But I don't think the problem implied here exists?
> 
> If 'request_irq(pci_irq_vector(pdev, 0), ...' fails we goto err_disable_msi and there
> is no free_irq in this path.  If 'request_irq(pci_irq_vector(pdev, 1), ...' fails then we
> goto err_free_irq and we do 'free_irq(pci_irq_vector(pdev, 0), vmci_dev)'.  Note that
> this is for the previous one without the check for vmci_dev->exclusive_vectors.

It's not a bug, but it's bad style.

Ideally the allocation code and the free code should mirror each other
as much as possible.  If there is an if statement in the allocation code
we should copy that same if statement into the free code.  Even if we
can leave the if statement out, we should still include it for
readability.

Also if we add another allocation at the end of the function then we
will have to add the if statement back.  Maybe the person who adds the
if statement will forget.  So it's fragile.

regards,
dan carpenter

Re: [PATCH 3/3] VMCI: Fix some error handling paths in vmci_guest_probe_device()

[Vishnu Dasa]

> On Feb 24, 2022, at 2:03 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> 
> On Thu, Feb 24, 2022 at 06:53:10AM +0000, Vishnu Dasa wrote:
>> 
>>> On Feb 11, 2022, at 12:06 PM, Christophe JAILLET <christophe.jaillet@wanadoo.fr> wrote:
>>> 
>>> Le 11/02/2022 à 15:09, Dan Carpenter a écrit :
>>>> On Thu, Feb 10, 2022 at 11:27:34PM +0100, Christophe JAILLET wrote:
> 
>>>> Whatever...  But where this really
>>>> hurts is with:
>>>> Alloc:
>>>> 	if (vmci_dev->exclusive_vectors) {
>>>> 		error = request_irq(pci_irq_vector(pdev, 1), ...
>>>> Free:
>>>> 	free_irq(pci_irq_vector(pdev, 1), vmci_dev);
>>>> No if statement.  It works because it's the last allocation but it's
>>>> confusing and fragile.
>>> 
>>> Agreed.
>> 
>> Sorry, I hope I'm not missing something obvious or misunderstanding the point.
>> But I don't think the problem implied here exists?
>> 
>> If 'request_irq(pci_irq_vector(pdev, 0), ...' fails we goto err_disable_msi and there
>> is no free_irq in this path.  If 'request_irq(pci_irq_vector(pdev, 1), ...' fails then we
>> goto err_free_irq and we do 'free_irq(pci_irq_vector(pdev, 0), vmci_dev)'.  Note that
>> this is for the previous one without the check for vmci_dev->exclusive_vectors.
> 
> It's not a bug, but it's bad style.
> 
> Ideally the allocation code and the free code should mirror each other
> as much as possible.  If there is an if statement in the allocation code
> we should copy that same if statement into the free code.  Even if we
> can leave the if statement out, we should still include it for
> readability.
> 
> Also if we add another allocation at the end of the function then we
> will have to add the if statement back.  Maybe the person who adds the
> if statement will forget.  So it's fragile.

No disagreements about that.  Making sure I wasn't missing anything
else.

Could we have a separate fix for this which fixes?
"VMCI: dma dg: register dummy IRQ handlers for DMA datagrams"

Thanks!