diff mbox series

[6/8] staging: vchiq_arm: Lower indentation of a conditional block

Message ID 20241011072210.494672-7-umang.jain@ideasonboard.com (mailing list archive)
State New
Headers show
Series staging: vchiq: Lower indentation at various places | expand

Commit Message

Umang Jain Oct. 11, 2024, 7:22 a.m. UTC
Check early if we need to allocate the bulk waiter. This helps to
improve readability and reduces the indentation of the 'if (waiter)'
conditional block.

No functional changes intended in this patch.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 .../interface/vchiq_arm/vchiq_arm.c           | 34 +++++++++----------
 1 file changed, 17 insertions(+), 17 deletions(-)

Comments

Stefan Wahren Oct. 11, 2024, 11:05 a.m. UTC | #1
Hi Umang,

Am 11.10.24 um 09:22 schrieb Umang Jain:
> Check early if we need to allocate the bulk waiter. This helps to
> improve readability and reduces the indentation of the 'if (waiter)'
> conditional block.
>
> No functional changes intended in this patch.
>
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
>   .../interface/vchiq_arm/vchiq_arm.c           | 34 +++++++++----------
>   1 file changed, 17 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> index 27ceaac8f6cc..a4a7f31b124a 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> @@ -564,28 +564,28 @@ vchiq_blocking_bulk_transfer(struct vchiq_instance *instance, unsigned int handl
>   	}
>   	mutex_unlock(&instance->bulk_waiter_list_mutex);
>
> -	if (waiter) {
> -		struct vchiq_bulk *bulk = waiter->bulk_waiter.bulk;
> -
> -		if (bulk) {
> -			/* This thread has an outstanding bulk transfer. */
> -			/* FIXME: why compare a dma address to a pointer? */
> -			if ((bulk->data != (dma_addr_t)(uintptr_t)data) || (bulk->size != size)) {
> -				/*
> -				 * This is not a retry of the previous one.
> -				 * Cancel the signal when the transfer completes.
> -				 */
> -				spin_lock(&service->state->bulk_waiter_spinlock);
> -				bulk->userdata = NULL;
> -				spin_unlock(&service->state->bulk_waiter_spinlock);
> -			}
> -		}
> -	} else {
> +	if (!waiter) {
>   		waiter = kzalloc(sizeof(*waiter), GFP_KERNEL);
>   		if (!waiter)
>   			return -ENOMEM;
>   	}
>
> +	struct vchiq_bulk *bulk = waiter->bulk_waiter.bulk;
I think this is a behavior change, which might lead to a null pointer
dereference in case waiter is freshly allocated.

Tbh I don't think indentation prevent us from unstaging this driver.
There are more important issues (e.g. resource leaks in probe error
paths or excessive usage of WARN) in this driver.

Regards
> +
> +	if (bulk) {
> +		/* This thread has an outstanding bulk transfer. */
> +		/* FIXME: why compare a dma address to a pointer? */
> +		if ((bulk->data != (dma_addr_t)(uintptr_t)data) || (bulk->size != size)) {
> +			/*
> +			 * This is not a retry of the previous one.
> +			 * Cancel the signal when the transfer completes.
> +			 */
> +			spin_lock(&service->state->bulk_waiter_spinlock);
> +			bulk->userdata = NULL;
> +			spin_unlock(&service->state->bulk_waiter_spinlock);
> +		}
> +	}
> +
>   	ret = vchiq_bulk_xfer_blocking(instance, handle, data, NULL, size,
>   				       &waiter->bulk_waiter, dir);
>   	if ((ret != -EAGAIN) || fatal_signal_pending(current) || !waiter->bulk_waiter.bulk) {
diff mbox series

Patch

diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index 27ceaac8f6cc..a4a7f31b124a 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -564,28 +564,28 @@  vchiq_blocking_bulk_transfer(struct vchiq_instance *instance, unsigned int handl
 	}
 	mutex_unlock(&instance->bulk_waiter_list_mutex);
 
-	if (waiter) {
-		struct vchiq_bulk *bulk = waiter->bulk_waiter.bulk;
-
-		if (bulk) {
-			/* This thread has an outstanding bulk transfer. */
-			/* FIXME: why compare a dma address to a pointer? */
-			if ((bulk->data != (dma_addr_t)(uintptr_t)data) || (bulk->size != size)) {
-				/*
-				 * This is not a retry of the previous one.
-				 * Cancel the signal when the transfer completes.
-				 */
-				spin_lock(&service->state->bulk_waiter_spinlock);
-				bulk->userdata = NULL;
-				spin_unlock(&service->state->bulk_waiter_spinlock);
-			}
-		}
-	} else {
+	if (!waiter) {
 		waiter = kzalloc(sizeof(*waiter), GFP_KERNEL);
 		if (!waiter)
 			return -ENOMEM;
 	}
 
+	struct vchiq_bulk *bulk = waiter->bulk_waiter.bulk;
+
+	if (bulk) {
+		/* This thread has an outstanding bulk transfer. */
+		/* FIXME: why compare a dma address to a pointer? */
+		if ((bulk->data != (dma_addr_t)(uintptr_t)data) || (bulk->size != size)) {
+			/*
+			 * This is not a retry of the previous one.
+			 * Cancel the signal when the transfer completes.
+			 */
+			spin_lock(&service->state->bulk_waiter_spinlock);
+			bulk->userdata = NULL;
+			spin_unlock(&service->state->bulk_waiter_spinlock);
+		}
+	}
+
 	ret = vchiq_bulk_xfer_blocking(instance, handle, data, NULL, size,
 				       &waiter->bulk_waiter, dir);
 	if ((ret != -EAGAIN) || fatal_signal_pending(current) || !waiter->bulk_waiter.bulk) {