[3/3] Free allocated resource on error
diff mbox series

Message ID 6575cd1b9c82f5b28cd29d78308dc23821c9d246.1570628924.git.artem_mygaiev@epam.com
State New
Headers show
Series
  • Minor Coverity fixes
Related show

Commit Message

Artem Mygaiev Oct. 9, 2019, 2:20 p.m. UTC
Also do not set mark device as SMMU protected when there are no more
SMMU resources left

Coverity-ID: 1381862
Signed-off-by: Artem Mygaiev <artem_mygaiev@epam.com>
---
 xen/drivers/passthrough/arm/smmu.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

Comments

Julien Grall Oct. 9, 2019, 3:24 p.m. UTC | #1
Hi Artem,

On 09/10/2019 15:20, Artem Mygaiev wrote:
> Also do not set mark device as SMMU protected when there are no more
> SMMU resources left

This is a sanity check on the content of the node, not a lack of resource in
this case.

TBH, the current placement is probably not correct. But I am not convinced the 
new placement is better.

Xen 4.12 and earlier will ignore any failure and continue, so we cannot use this 
device at all. Indeed, Xen will configure the SMMU to deny any transaction. If 
you fail to initialize the device, then it will be in an unusable state. In this 
case, we don't want a domain (including Dom0) to use it at all. This could be 
achieved by calling dt_device_set_protected.

Xen 4.13 will stop booting if any of the SMMU fails to configure (this include 
Master Device cannot be assigned). So there are no difference there.

My preference is to cater for all Xen versions so we can consider the patch for 
a backport and potentially revert of the Xen 4.13 behavior (i.e. crashing when 
one IOMMU is not correctly setup). So we would need to move the call at the 
beginning of the function.

> 
> Coverity-ID: 1381862
> Signed-off-by: Artem Mygaiev <artem_mygaiev@epam.com>
> ---
>   xen/drivers/passthrough/arm/smmu.c | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
> index f151b9f5b5..cf42335eed 100644
> --- a/xen/drivers/passthrough/arm/smmu.c
> +++ b/xen/drivers/passthrough/arm/smmu.c
> @@ -804,9 +804,6 @@ static int register_smmu_master(struct arm_smmu_device *smmu,
>   	master->of_node			= masterspec->np;
>   	master->cfg.num_streamids	= masterspec->args_count;
>   
> -	/* Xen: Let Xen know that the device is protected by an SMMU */
> -	dt_device_set_protected(masterspec->np);
> -
>   	for (i = 0; i < master->cfg.num_streamids; ++i) {
>   		u16 streamid = masterspec->args[i];
>   
> @@ -815,10 +812,15 @@ static int register_smmu_master(struct arm_smmu_device *smmu,
>   			dev_err(dev,
>   				"stream ID for master device %s greater than maximum allowed (%d)\n",
>   				masterspec->np->name, smmu->num_mapping_groups);
> +			devm_free(master);
>   			return -ERANGE;
>   		}
>   		master->cfg.streamids[i] = streamid;
>   	}
> +
> +        /* Xen: Let Xen know that the device is protected by an SMMU */
> +        dt_device_set_protected(masterspec->np);

This code is using Linux coding style, so it should be hard tab here.

> +
>   	return insert_smmu_master(smmu, master);
>   }
>   
> 

Cheers,

Patch
diff mbox series

diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
index f151b9f5b5..cf42335eed 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -804,9 +804,6 @@  static int register_smmu_master(struct arm_smmu_device *smmu,
 	master->of_node			= masterspec->np;
 	master->cfg.num_streamids	= masterspec->args_count;
 
-	/* Xen: Let Xen know that the device is protected by an SMMU */
-	dt_device_set_protected(masterspec->np);
-
 	for (i = 0; i < master->cfg.num_streamids; ++i) {
 		u16 streamid = masterspec->args[i];
 
@@ -815,10 +812,15 @@  static int register_smmu_master(struct arm_smmu_device *smmu,
 			dev_err(dev,
 				"stream ID for master device %s greater than maximum allowed (%d)\n",
 				masterspec->np->name, smmu->num_mapping_groups);
+			devm_free(master);
 			return -ERANGE;
 		}
 		master->cfg.streamids[i] = streamid;
 	}
+
+        /* Xen: Let Xen know that the device is protected by an SMMU */
+        dt_device_set_protected(masterspec->np);
+
 	return insert_smmu_master(smmu, master);
 }