All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] media: omap3isp: fix unbalanced dma_iommu_mapping
@ 2018-03-12 16:52 ` Suman Anna
  0 siblings, 0 replies; 12+ messages in thread
From: Suman Anna @ 2018-03-12 16:52 UTC (permalink / raw)
  To: Laurent Pinchart, Mauro Carvalho Chehab
  Cc: Pavel Machek, Sakari Ailus, Tony Lindgren, Suman Anna,
	linux-media, linux-omap, linux-arm-kernel

The OMAP3 ISP driver manages its MMU mappings through the IOMMU-aware
ARM DMA backend. The current code creates a dma_iommu_mapping and
attaches this to the ISP device, but never detaches the mapping in
either the probe failure paths or the driver remove path resulting
in an unbalanced mapping refcount and a memory leak. Fix this properly.

Reported-by: Pavel Machek <pavel@ucw.cz>
Signed-off-by: Suman Anna <s-anna@ti.com>
Tested-by: Pavel Machek <pavel@ucw.cz>
---
Hi Mauro, Laurent,

This fixes an issue reported by Pavel and discussed on this
thread,
https://marc.info/?l=linux-omap&m=152051945803598&w=2

Posting this again to the appropriate lists.

regards
Suman

 drivers/media/platform/omap3isp/isp.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
index 8eb000e3d8fd..c7d667bfc2af 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -1945,6 +1945,7 @@ static int isp_initialize_modules(struct isp_device *isp)
 
 static void isp_detach_iommu(struct isp_device *isp)
 {
+	arm_iommu_detach_device(isp->dev);
 	arm_iommu_release_mapping(isp->mapping);
 	isp->mapping = NULL;
 }
@@ -1971,13 +1972,15 @@ static int isp_attach_iommu(struct isp_device *isp)
 	ret = arm_iommu_attach_device(isp->dev, mapping);
 	if (ret < 0) {
 		dev_err(isp->dev, "failed to attach device to VA mapping\n");
-		goto error;
+		goto error_attach;
 	}
 
 	return 0;
 
+error_attach:
+	arm_iommu_release_mapping(isp->mapping);
+	isp->mapping = NULL;
 error:
-	isp_detach_iommu(isp);
 	return ret;
 }
 
-- 
2.16.2

^ permalink raw reply related	[flat|nested] 12+ messages in thread
* [PATCH] media: omap3isp: fix unbalanced dma_iommu_mapping
@ 2018-03-12 16:52 ` Suman Anna
  0 siblings, 0 replies; 12+ messages in thread
From: Suman Anna @ 2018-03-12 16:52 UTC (permalink / raw)
  To: Laurent Pinchart, Mauro Carvalho Chehab
  Cc: Tony Lindgren, Sakari Ailus, Pavel Machek, linux-omap,
	linux-arm-kernel, linux-media

The OMAP3 ISP driver manages its MMU mappings through the IOMMU-aware
ARM DMA backend. The current code creates a dma_iommu_mapping and
attaches this to the ISP device, but never detaches the mapping in
either the probe failure paths or the driver remove path resulting
in an unbalanced mapping refcount and a memory leak. Fix this properly.

Reported-by: Pavel Machek <pavel@ucw.cz>
Signed-off-by: Suman Anna <s-anna@ti.com>
Tested-by: Pavel Machek <pavel@ucw.cz>
---
Hi Mauro, Laurent,

This fixes an issue reported by Pavel and discussed on this
thread,
https://marc.info/?l=linux-omap&m=152051945803598&w=2

Posting this again to the appropriate lists.

regards
Suman

 drivers/media/platform/omap3isp/isp.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
index 8eb000e3d8fd..c7d667bfc2af 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -1945,6 +1945,7 @@ static int isp_initialize_modules(struct isp_device *isp)
 
 static void isp_detach_iommu(struct isp_device *isp)
 {
+	arm_iommu_detach_device(isp->dev);
 	arm_iommu_release_mapping(isp->mapping);
 	isp->mapping = NULL;
 }
@@ -1971,13 +1972,15 @@ static int isp_attach_iommu(struct isp_device *isp)
 	ret = arm_iommu_attach_device(isp->dev, mapping);
 	if (ret < 0) {
 		dev_err(isp->dev, "failed to attach device to VA mapping\n");
-		goto error;
+		goto error_attach;
 	}
 
 	return 0;
 
+error_attach:
+	arm_iommu_release_mapping(isp->mapping);
+	isp->mapping = NULL;
 error:
-	isp_detach_iommu(isp);
 	return ret;
 }
 
-- 
2.16.2

^ permalink raw reply related	[flat|nested] 12+ messages in thread
* [PATCH] media: omap3isp: fix unbalanced dma_iommu_mapping
@ 2018-03-12 16:52 ` Suman Anna
  0 siblings, 0 replies; 12+ messages in thread
From: Suman Anna @ 2018-03-12 16:52 UTC (permalink / raw)
  To: linux-arm-kernel

The OMAP3 ISP driver manages its MMU mappings through the IOMMU-aware
ARM DMA backend. The current code creates a dma_iommu_mapping and
attaches this to the ISP device, but never detaches the mapping in
either the probe failure paths or the driver remove path resulting
in an unbalanced mapping refcount and a memory leak. Fix this properly.

Reported-by: Pavel Machek <pavel@ucw.cz>
Signed-off-by: Suman Anna <s-anna@ti.com>
Tested-by: Pavel Machek <pavel@ucw.cz>
---
Hi Mauro, Laurent,

This fixes an issue reported by Pavel and discussed on this
thread,
https://marc.info/?l=linux-omap&m=152051945803598&w=2

Posting this again to the appropriate lists.

regards
Suman

 drivers/media/platform/omap3isp/isp.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
index 8eb000e3d8fd..c7d667bfc2af 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -1945,6 +1945,7 @@ static int isp_initialize_modules(struct isp_device *isp)
 
 static void isp_detach_iommu(struct isp_device *isp)
 {
+	arm_iommu_detach_device(isp->dev);
 	arm_iommu_release_mapping(isp->mapping);
 	isp->mapping = NULL;
 }
@@ -1971,13 +1972,15 @@ static int isp_attach_iommu(struct isp_device *isp)
 	ret = arm_iommu_attach_device(isp->dev, mapping);
 	if (ret < 0) {
 		dev_err(isp->dev, "failed to attach device to VA mapping\n");
-		goto error;
+		goto error_attach;
 	}
 
 	return 0;
 
+error_attach:
+	arm_iommu_release_mapping(isp->mapping);
+	isp->mapping = NULL;
 error:
-	isp_detach_iommu(isp);
 	return ret;
 }
 
-- 
2.16.2

^ permalink raw reply related	[flat|nested] 12+ messages in thread
* Re: [PATCH] media: omap3isp: fix unbalanced dma_iommu_mapping
  2018-03-12 16:52 ` Suman Anna
  (?)
@ 2018-03-13 11:14   ` Sakari Ailus
  -1 siblings, 0 replies; 12+ messages in thread
From: Sakari Ailus @ 2018-03-13 11:14 UTC (permalink / raw)
  To: Suman Anna
  Cc: Laurent Pinchart, Mauro Carvalho Chehab, Pavel Machek,
	Tony Lindgren, linux-media, linux-omap, linux-arm-kernel

Hi Suman,

Thanks for the patch.

On Mon, Mar 12, 2018 at 11:52:07AM -0500, Suman Anna wrote:
> The OMAP3 ISP driver manages its MMU mappings through the IOMMU-aware
> ARM DMA backend. The current code creates a dma_iommu_mapping and
> attaches this to the ISP device, but never detaches the mapping in
> either the probe failure paths or the driver remove path resulting
> in an unbalanced mapping refcount and a memory leak. Fix this properly.
> 
> Reported-by: Pavel Machek <pavel@ucw.cz>
> Signed-off-by: Suman Anna <s-anna@ti.com>
> Tested-by: Pavel Machek <pavel@ucw.cz>
> ---
> Hi Mauro, Laurent,
> 
> This fixes an issue reported by Pavel and discussed on this
> thread,
> https://marc.info/?l=linux-omap&m=152051945803598&w=2
> 
> Posting this again to the appropriate lists.
> 
> regards
> Suman
> 
>  drivers/media/platform/omap3isp/isp.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
> index 8eb000e3d8fd..c7d667bfc2af 100644
> --- a/drivers/media/platform/omap3isp/isp.c
> +++ b/drivers/media/platform/omap3isp/isp.c
> @@ -1945,6 +1945,7 @@ static int isp_initialize_modules(struct isp_device *isp)
>  
>  static void isp_detach_iommu(struct isp_device *isp)
>  {
> +	arm_iommu_detach_device(isp->dev);
>  	arm_iommu_release_mapping(isp->mapping);
>  	isp->mapping = NULL;
>  }
> @@ -1971,13 +1972,15 @@ static int isp_attach_iommu(struct isp_device *isp)
>  	ret = arm_iommu_attach_device(isp->dev, mapping);
>  	if (ret < 0) {
>  		dev_err(isp->dev, "failed to attach device to VA mapping\n");
> -		goto error;
> +		goto error_attach;

Instead of changing the label here, could you return immediately where the
previous point of error handling is? No need to add another label.

After fixing that you can add:

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

>  	}
>  
>  	return 0;
>  
> +error_attach:
> +	arm_iommu_release_mapping(isp->mapping);
> +	isp->mapping = NULL;
>  error:
> -	isp_detach_iommu(isp);
>  	return ret;
>  }
>  
> 

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi

^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [PATCH] media: omap3isp: fix unbalanced dma_iommu_mapping
@ 2018-03-13 11:14   ` Sakari Ailus
  0 siblings, 0 replies; 12+ messages in thread
From: Sakari Ailus @ 2018-03-13 11:14 UTC (permalink / raw)
  To: Suman Anna
  Cc: Mauro Carvalho Chehab, Tony Lindgren, Laurent Pinchart,
	Pavel Machek, linux-omap, linux-arm-kernel, linux-media

Hi Suman,

Thanks for the patch.

On Mon, Mar 12, 2018 at 11:52:07AM -0500, Suman Anna wrote:
> The OMAP3 ISP driver manages its MMU mappings through the IOMMU-aware
> ARM DMA backend. The current code creates a dma_iommu_mapping and
> attaches this to the ISP device, but never detaches the mapping in
> either the probe failure paths or the driver remove path resulting
> in an unbalanced mapping refcount and a memory leak. Fix this properly.
> 
> Reported-by: Pavel Machek <pavel@ucw.cz>
> Signed-off-by: Suman Anna <s-anna@ti.com>
> Tested-by: Pavel Machek <pavel@ucw.cz>
> ---
> Hi Mauro, Laurent,
> 
> This fixes an issue reported by Pavel and discussed on this
> thread,
> https://marc.info/?l=linux-omap&m=152051945803598&w=2
> 
> Posting this again to the appropriate lists.
> 
> regards
> Suman
> 
>  drivers/media/platform/omap3isp/isp.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
> index 8eb000e3d8fd..c7d667bfc2af 100644
> --- a/drivers/media/platform/omap3isp/isp.c
> +++ b/drivers/media/platform/omap3isp/isp.c
> @@ -1945,6 +1945,7 @@ static int isp_initialize_modules(struct isp_device *isp)
>  
>  static void isp_detach_iommu(struct isp_device *isp)
>  {
> +	arm_iommu_detach_device(isp->dev);
>  	arm_iommu_release_mapping(isp->mapping);
>  	isp->mapping = NULL;
>  }
> @@ -1971,13 +1972,15 @@ static int isp_attach_iommu(struct isp_device *isp)
>  	ret = arm_iommu_attach_device(isp->dev, mapping);
>  	if (ret < 0) {
>  		dev_err(isp->dev, "failed to attach device to VA mapping\n");
> -		goto error;
> +		goto error_attach;

Instead of changing the label here, could you return immediately where the
previous point of error handling is? No need to add another label.

After fixing that you can add:

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

>  	}
>  
>  	return 0;
>  
> +error_attach:
> +	arm_iommu_release_mapping(isp->mapping);
> +	isp->mapping = NULL;
>  error:
> -	isp_detach_iommu(isp);
>  	return ret;
>  }
>  
> 

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi

^ permalink raw reply	[flat|nested] 12+ messages in thread
* [PATCH] media: omap3isp: fix unbalanced dma_iommu_mapping
@ 2018-03-13 11:14   ` Sakari Ailus
  0 siblings, 0 replies; 12+ messages in thread
From: Sakari Ailus @ 2018-03-13 11:14 UTC (permalink / raw)
  To: linux-arm-kernel

Hi Suman,

Thanks for the patch.

On Mon, Mar 12, 2018 at 11:52:07AM -0500, Suman Anna wrote:
> The OMAP3 ISP driver manages its MMU mappings through the IOMMU-aware
> ARM DMA backend. The current code creates a dma_iommu_mapping and
> attaches this to the ISP device, but never detaches the mapping in
> either the probe failure paths or the driver remove path resulting
> in an unbalanced mapping refcount and a memory leak. Fix this properly.
> 
> Reported-by: Pavel Machek <pavel@ucw.cz>
> Signed-off-by: Suman Anna <s-anna@ti.com>
> Tested-by: Pavel Machek <pavel@ucw.cz>
> ---
> Hi Mauro, Laurent,
> 
> This fixes an issue reported by Pavel and discussed on this
> thread,
> https://marc.info/?l=linux-omap&m=152051945803598&w=2
> 
> Posting this again to the appropriate lists.
> 
> regards
> Suman
> 
>  drivers/media/platform/omap3isp/isp.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
> index 8eb000e3d8fd..c7d667bfc2af 100644
> --- a/drivers/media/platform/omap3isp/isp.c
> +++ b/drivers/media/platform/omap3isp/isp.c
> @@ -1945,6 +1945,7 @@ static int isp_initialize_modules(struct isp_device *isp)
>  
>  static void isp_detach_iommu(struct isp_device *isp)
>  {
> +	arm_iommu_detach_device(isp->dev);
>  	arm_iommu_release_mapping(isp->mapping);
>  	isp->mapping = NULL;
>  }
> @@ -1971,13 +1972,15 @@ static int isp_attach_iommu(struct isp_device *isp)
>  	ret = arm_iommu_attach_device(isp->dev, mapping);
>  	if (ret < 0) {
>  		dev_err(isp->dev, "failed to attach device to VA mapping\n");
> -		goto error;
> +		goto error_attach;

Instead of changing the label here, could you return immediately where the
previous point of error handling is? No need to add another label.

After fixing that you can add:

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

>  	}
>  
>  	return 0;
>  
> +error_attach:
> +	arm_iommu_release_mapping(isp->mapping);
> +	isp->mapping = NULL;
>  error:
> -	isp_detach_iommu(isp);
>  	return ret;
>  }
>  
> 

-- 
Sakari Ailus
e-mail: sakari.ailus at iki.fi

^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [PATCH] media: omap3isp: fix unbalanced dma_iommu_mapping
  2018-03-13 11:14   ` Sakari Ailus
  (?)
@ 2018-03-13 15:47     ` Suman Anna
  -1 siblings, 0 replies; 12+ messages in thread
From: Suman Anna @ 2018-03-13 15:47 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: Laurent Pinchart, Mauro Carvalho Chehab, Pavel Machek,
	Tony Lindgren, linux-media, linux-omap, linux-arm-kernel

Hi Sakari,

On 03/13/2018 06:14 AM, Sakari Ailus wrote:
> Hi Suman,
> 
> Thanks for the patch.
> 
> On Mon, Mar 12, 2018 at 11:52:07AM -0500, Suman Anna wrote:
>> The OMAP3 ISP driver manages its MMU mappings through the IOMMU-aware
>> ARM DMA backend. The current code creates a dma_iommu_mapping and
>> attaches this to the ISP device, but never detaches the mapping in
>> either the probe failure paths or the driver remove path resulting
>> in an unbalanced mapping refcount and a memory leak. Fix this properly.
>>
>> Reported-by: Pavel Machek <pavel@ucw.cz>
>> Signed-off-by: Suman Anna <s-anna@ti.com>
>> Tested-by: Pavel Machek <pavel@ucw.cz>
>> ---
>> Hi Mauro, Laurent,
>>
>> This fixes an issue reported by Pavel and discussed on this
>> thread,
>> https://marc.info/?l=linux-omap&m=152051945803598&w=2
>>
>> Posting this again to the appropriate lists.
>>
>> regards
>> Suman
>>
>>  drivers/media/platform/omap3isp/isp.c | 7 +++++--
>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
>> index 8eb000e3d8fd..c7d667bfc2af 100644
>> --- a/drivers/media/platform/omap3isp/isp.c
>> +++ b/drivers/media/platform/omap3isp/isp.c
>> @@ -1945,6 +1945,7 @@ static int isp_initialize_modules(struct isp_device *isp)
>>  
>>  static void isp_detach_iommu(struct isp_device *isp)
>>  {
>> +	arm_iommu_detach_device(isp->dev);
>>  	arm_iommu_release_mapping(isp->mapping);
>>  	isp->mapping = NULL;
>>  }
>> @@ -1971,13 +1972,15 @@ static int isp_attach_iommu(struct isp_device *isp)
>>  	ret = arm_iommu_attach_device(isp->dev, mapping);
>>  	if (ret < 0) {
>>  		dev_err(isp->dev, "failed to attach device to VA mapping\n");
>> -		goto error;
>> +		goto error_attach;
> 
> Instead of changing the label here, could you return immediately where the
> previous point of error handling is? No need to add another label.

Yeah, I debated about this while doing the patch, and chose to retain
the previous common return on the error paths. There are only 2 error
paths, so didn't want to mix them up. If you still prefer the mixed
style, I can post a v2.

regards
Suman

> 
> After fixing that you can add:
> 
> Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> 
>>  	}
>>  
>>  	return 0;
>>  
>> +error_attach:
>> +	arm_iommu_release_mapping(isp->mapping);
>> +	isp->mapping = NULL;
>>  error:
>> -	isp_detach_iommu(isp);
>>  	return ret;
>>  }
>>  
>>
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [PATCH] media: omap3isp: fix unbalanced dma_iommu_mapping
@ 2018-03-13 15:47     ` Suman Anna
  0 siblings, 0 replies; 12+ messages in thread
From: Suman Anna @ 2018-03-13 15:47 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: Mauro Carvalho Chehab, Tony Lindgren, Laurent Pinchart,
	Pavel Machek, linux-omap, linux-arm-kernel, linux-media

Hi Sakari,

On 03/13/2018 06:14 AM, Sakari Ailus wrote:
> Hi Suman,
> 
> Thanks for the patch.
> 
> On Mon, Mar 12, 2018 at 11:52:07AM -0500, Suman Anna wrote:
>> The OMAP3 ISP driver manages its MMU mappings through the IOMMU-aware
>> ARM DMA backend. The current code creates a dma_iommu_mapping and
>> attaches this to the ISP device, but never detaches the mapping in
>> either the probe failure paths or the driver remove path resulting
>> in an unbalanced mapping refcount and a memory leak. Fix this properly.
>>
>> Reported-by: Pavel Machek <pavel@ucw.cz>
>> Signed-off-by: Suman Anna <s-anna@ti.com>
>> Tested-by: Pavel Machek <pavel@ucw.cz>
>> ---
>> Hi Mauro, Laurent,
>>
>> This fixes an issue reported by Pavel and discussed on this
>> thread,
>> https://marc.info/?l=linux-omap&m=152051945803598&w=2
>>
>> Posting this again to the appropriate lists.
>>
>> regards
>> Suman
>>
>>  drivers/media/platform/omap3isp/isp.c | 7 +++++--
>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
>> index 8eb000e3d8fd..c7d667bfc2af 100644
>> --- a/drivers/media/platform/omap3isp/isp.c
>> +++ b/drivers/media/platform/omap3isp/isp.c
>> @@ -1945,6 +1945,7 @@ static int isp_initialize_modules(struct isp_device *isp)
>>  
>>  static void isp_detach_iommu(struct isp_device *isp)
>>  {
>> +	arm_iommu_detach_device(isp->dev);
>>  	arm_iommu_release_mapping(isp->mapping);
>>  	isp->mapping = NULL;
>>  }
>> @@ -1971,13 +1972,15 @@ static int isp_attach_iommu(struct isp_device *isp)
>>  	ret = arm_iommu_attach_device(isp->dev, mapping);
>>  	if (ret < 0) {
>>  		dev_err(isp->dev, "failed to attach device to VA mapping\n");
>> -		goto error;
>> +		goto error_attach;
> 
> Instead of changing the label here, could you return immediately where the
> previous point of error handling is? No need to add another label.

Yeah, I debated about this while doing the patch, and chose to retain
the previous common return on the error paths. There are only 2 error
paths, so didn't want to mix them up. If you still prefer the mixed
style, I can post a v2.

regards
Suman

> 
> After fixing that you can add:
> 
> Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> 
>>  	}
>>  
>>  	return 0;
>>  
>> +error_attach:
>> +	arm_iommu_release_mapping(isp->mapping);
>> +	isp->mapping = NULL;
>>  error:
>> -	isp_detach_iommu(isp);
>>  	return ret;
>>  }
>>  
>>
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread
* [PATCH] media: omap3isp: fix unbalanced dma_iommu_mapping
@ 2018-03-13 15:47     ` Suman Anna
  0 siblings, 0 replies; 12+ messages in thread
From: Suman Anna @ 2018-03-13 15:47 UTC (permalink / raw)
  To: linux-arm-kernel

Hi Sakari,

On 03/13/2018 06:14 AM, Sakari Ailus wrote:
> Hi Suman,
> 
> Thanks for the patch.
> 
> On Mon, Mar 12, 2018 at 11:52:07AM -0500, Suman Anna wrote:
>> The OMAP3 ISP driver manages its MMU mappings through the IOMMU-aware
>> ARM DMA backend. The current code creates a dma_iommu_mapping and
>> attaches this to the ISP device, but never detaches the mapping in
>> either the probe failure paths or the driver remove path resulting
>> in an unbalanced mapping refcount and a memory leak. Fix this properly.
>>
>> Reported-by: Pavel Machek <pavel@ucw.cz>
>> Signed-off-by: Suman Anna <s-anna@ti.com>
>> Tested-by: Pavel Machek <pavel@ucw.cz>
>> ---
>> Hi Mauro, Laurent,
>>
>> This fixes an issue reported by Pavel and discussed on this
>> thread,
>> https://marc.info/?l=linux-omap&m=152051945803598&w=2
>>
>> Posting this again to the appropriate lists.
>>
>> regards
>> Suman
>>
>>  drivers/media/platform/omap3isp/isp.c | 7 +++++--
>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
>> index 8eb000e3d8fd..c7d667bfc2af 100644
>> --- a/drivers/media/platform/omap3isp/isp.c
>> +++ b/drivers/media/platform/omap3isp/isp.c
>> @@ -1945,6 +1945,7 @@ static int isp_initialize_modules(struct isp_device *isp)
>>  
>>  static void isp_detach_iommu(struct isp_device *isp)
>>  {
>> +	arm_iommu_detach_device(isp->dev);
>>  	arm_iommu_release_mapping(isp->mapping);
>>  	isp->mapping = NULL;
>>  }
>> @@ -1971,13 +1972,15 @@ static int isp_attach_iommu(struct isp_device *isp)
>>  	ret = arm_iommu_attach_device(isp->dev, mapping);
>>  	if (ret < 0) {
>>  		dev_err(isp->dev, "failed to attach device to VA mapping\n");
>> -		goto error;
>> +		goto error_attach;
> 
> Instead of changing the label here, could you return immediately where the
> previous point of error handling is? No need to add another label.

Yeah, I debated about this while doing the patch, and chose to retain
the previous common return on the error paths. There are only 2 error
paths, so didn't want to mix them up. If you still prefer the mixed
style, I can post a v2.

regards
Suman

> 
> After fixing that you can add:
> 
> Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> 
>>  	}
>>  
>>  	return 0;
>>  
>> +error_attach:
>> +	arm_iommu_release_mapping(isp->mapping);
>> +	isp->mapping = NULL;
>>  error:
>> -	isp_detach_iommu(isp);
>>  	return ret;
>>  }
>>  
>>
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [PATCH] media: omap3isp: fix unbalanced dma_iommu_mapping
  2018-03-13 15:47     ` Suman Anna
  (?)
@ 2018-03-14  9:28       ` Sakari Ailus
  -1 siblings, 0 replies; 12+ messages in thread
From: Sakari Ailus @ 2018-03-14  9:28 UTC (permalink / raw)
  To: Suman Anna
  Cc: Laurent Pinchart, Mauro Carvalho Chehab, Pavel Machek,
	Tony Lindgren, linux-media, linux-omap, linux-arm-kernel

On Tue, Mar 13, 2018 at 10:47:08AM -0500, Suman Anna wrote:
> Hi Sakari,
> 
> On 03/13/2018 06:14 AM, Sakari Ailus wrote:
> > Hi Suman,
> > 
> > Thanks for the patch.
> > 
> > On Mon, Mar 12, 2018 at 11:52:07AM -0500, Suman Anna wrote:
> >> The OMAP3 ISP driver manages its MMU mappings through the IOMMU-aware
> >> ARM DMA backend. The current code creates a dma_iommu_mapping and
> >> attaches this to the ISP device, but never detaches the mapping in
> >> either the probe failure paths or the driver remove path resulting
> >> in an unbalanced mapping refcount and a memory leak. Fix this properly.
> >>
> >> Reported-by: Pavel Machek <pavel@ucw.cz>
> >> Signed-off-by: Suman Anna <s-anna@ti.com>
> >> Tested-by: Pavel Machek <pavel@ucw.cz>
> >> ---
> >> Hi Mauro, Laurent,
> >>
> >> This fixes an issue reported by Pavel and discussed on this
> >> thread,
> >> https://marc.info/?l=linux-omap&m=152051945803598&w=2
> >>
> >> Posting this again to the appropriate lists.
> >>
> >> regards
> >> Suman
> >>
> >>  drivers/media/platform/omap3isp/isp.c | 7 +++++--
> >>  1 file changed, 5 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
> >> index 8eb000e3d8fd..c7d667bfc2af 100644
> >> --- a/drivers/media/platform/omap3isp/isp.c
> >> +++ b/drivers/media/platform/omap3isp/isp.c
> >> @@ -1945,6 +1945,7 @@ static int isp_initialize_modules(struct isp_device *isp)
> >>  
> >>  static void isp_detach_iommu(struct isp_device *isp)
> >>  {
> >> +	arm_iommu_detach_device(isp->dev);
> >>  	arm_iommu_release_mapping(isp->mapping);
> >>  	isp->mapping = NULL;
> >>  }
> >> @@ -1971,13 +1972,15 @@ static int isp_attach_iommu(struct isp_device *isp)
> >>  	ret = arm_iommu_attach_device(isp->dev, mapping);
> >>  	if (ret < 0) {
> >>  		dev_err(isp->dev, "failed to attach device to VA mapping\n");
> >> -		goto error;
> >> +		goto error_attach;
> > 
> > Instead of changing the label here, could you return immediately where the
> > previous point of error handling is? No need to add another label.
> 
> Yeah, I debated about this while doing the patch, and chose to retain
> the previous common return on the error paths. There are only 2 error
> paths, so didn't want to mix them up. If you still prefer the mixed
> style, I can post a v2.

Yes, please. In general if you only need return a value, a label isn't
needed for that even if goto + labels would be otherwise used for error
handling.

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi

^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [PATCH] media: omap3isp: fix unbalanced dma_iommu_mapping
@ 2018-03-14  9:28       ` Sakari Ailus
  0 siblings, 0 replies; 12+ messages in thread
From: Sakari Ailus @ 2018-03-14  9:28 UTC (permalink / raw)
  To: Suman Anna
  Cc: Mauro Carvalho Chehab, Tony Lindgren, Laurent Pinchart,
	Pavel Machek, linux-omap, linux-arm-kernel, linux-media

On Tue, Mar 13, 2018 at 10:47:08AM -0500, Suman Anna wrote:
> Hi Sakari,
> 
> On 03/13/2018 06:14 AM, Sakari Ailus wrote:
> > Hi Suman,
> > 
> > Thanks for the patch.
> > 
> > On Mon, Mar 12, 2018 at 11:52:07AM -0500, Suman Anna wrote:
> >> The OMAP3 ISP driver manages its MMU mappings through the IOMMU-aware
> >> ARM DMA backend. The current code creates a dma_iommu_mapping and
> >> attaches this to the ISP device, but never detaches the mapping in
> >> either the probe failure paths or the driver remove path resulting
> >> in an unbalanced mapping refcount and a memory leak. Fix this properly.
> >>
> >> Reported-by: Pavel Machek <pavel@ucw.cz>
> >> Signed-off-by: Suman Anna <s-anna@ti.com>
> >> Tested-by: Pavel Machek <pavel@ucw.cz>
> >> ---
> >> Hi Mauro, Laurent,
> >>
> >> This fixes an issue reported by Pavel and discussed on this
> >> thread,
> >> https://marc.info/?l=linux-omap&m=152051945803598&w=2
> >>
> >> Posting this again to the appropriate lists.
> >>
> >> regards
> >> Suman
> >>
> >>  drivers/media/platform/omap3isp/isp.c | 7 +++++--
> >>  1 file changed, 5 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
> >> index 8eb000e3d8fd..c7d667bfc2af 100644
> >> --- a/drivers/media/platform/omap3isp/isp.c
> >> +++ b/drivers/media/platform/omap3isp/isp.c
> >> @@ -1945,6 +1945,7 @@ static int isp_initialize_modules(struct isp_device *isp)
> >>  
> >>  static void isp_detach_iommu(struct isp_device *isp)
> >>  {
> >> +	arm_iommu_detach_device(isp->dev);
> >>  	arm_iommu_release_mapping(isp->mapping);
> >>  	isp->mapping = NULL;
> >>  }
> >> @@ -1971,13 +1972,15 @@ static int isp_attach_iommu(struct isp_device *isp)
> >>  	ret = arm_iommu_attach_device(isp->dev, mapping);
> >>  	if (ret < 0) {
> >>  		dev_err(isp->dev, "failed to attach device to VA mapping\n");
> >> -		goto error;
> >> +		goto error_attach;
> > 
> > Instead of changing the label here, could you return immediately where the
> > previous point of error handling is? No need to add another label.
> 
> Yeah, I debated about this while doing the patch, and chose to retain
> the previous common return on the error paths. There are only 2 error
> paths, so didn't want to mix them up. If you still prefer the mixed
> style, I can post a v2.

Yes, please. In general if you only need return a value, a label isn't
needed for that even if goto + labels would be otherwise used for error
handling.

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi

^ permalink raw reply	[flat|nested] 12+ messages in thread
* [PATCH] media: omap3isp: fix unbalanced dma_iommu_mapping
@ 2018-03-14  9:28       ` Sakari Ailus
  0 siblings, 0 replies; 12+ messages in thread
From: Sakari Ailus @ 2018-03-14  9:28 UTC (permalink / raw)
  To: linux-arm-kernel

On Tue, Mar 13, 2018 at 10:47:08AM -0500, Suman Anna wrote:
> Hi Sakari,
> 
> On 03/13/2018 06:14 AM, Sakari Ailus wrote:
> > Hi Suman,
> > 
> > Thanks for the patch.
> > 
> > On Mon, Mar 12, 2018 at 11:52:07AM -0500, Suman Anna wrote:
> >> The OMAP3 ISP driver manages its MMU mappings through the IOMMU-aware
> >> ARM DMA backend. The current code creates a dma_iommu_mapping and
> >> attaches this to the ISP device, but never detaches the mapping in
> >> either the probe failure paths or the driver remove path resulting
> >> in an unbalanced mapping refcount and a memory leak. Fix this properly.
> >>
> >> Reported-by: Pavel Machek <pavel@ucw.cz>
> >> Signed-off-by: Suman Anna <s-anna@ti.com>
> >> Tested-by: Pavel Machek <pavel@ucw.cz>
> >> ---
> >> Hi Mauro, Laurent,
> >>
> >> This fixes an issue reported by Pavel and discussed on this
> >> thread,
> >> https://marc.info/?l=linux-omap&m=152051945803598&w=2
> >>
> >> Posting this again to the appropriate lists.
> >>
> >> regards
> >> Suman
> >>
> >>  drivers/media/platform/omap3isp/isp.c | 7 +++++--
> >>  1 file changed, 5 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
> >> index 8eb000e3d8fd..c7d667bfc2af 100644
> >> --- a/drivers/media/platform/omap3isp/isp.c
> >> +++ b/drivers/media/platform/omap3isp/isp.c
> >> @@ -1945,6 +1945,7 @@ static int isp_initialize_modules(struct isp_device *isp)
> >>  
> >>  static void isp_detach_iommu(struct isp_device *isp)
> >>  {
> >> +	arm_iommu_detach_device(isp->dev);
> >>  	arm_iommu_release_mapping(isp->mapping);
> >>  	isp->mapping = NULL;
> >>  }
> >> @@ -1971,13 +1972,15 @@ static int isp_attach_iommu(struct isp_device *isp)
> >>  	ret = arm_iommu_attach_device(isp->dev, mapping);
> >>  	if (ret < 0) {
> >>  		dev_err(isp->dev, "failed to attach device to VA mapping\n");
> >> -		goto error;
> >> +		goto error_attach;
> > 
> > Instead of changing the label here, could you return immediately where the
> > previous point of error handling is? No need to add another label.
> 
> Yeah, I debated about this while doing the patch, and chose to retain
> the previous common return on the error paths. There are only 2 error
> paths, so didn't want to mix them up. If you still prefer the mixed
> style, I can post a v2.

Yes, please. In general if you only need return a value, a label isn't
needed for that even if goto + labels would be otherwise used for error
handling.

-- 
Sakari Ailus
e-mail: sakari.ailus at iki.fi

^ permalink raw reply	[flat|nested] 12+ messages in thread
end of thread, other threads:[~2018-03-14  9:28 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-12 16:52 [PATCH] media: omap3isp: fix unbalanced dma_iommu_mapping Suman Anna
2018-03-12 16:52 ` Suman Anna
2018-03-12 16:52 ` Suman Anna
2018-03-13 11:14 ` Sakari Ailus
2018-03-13 11:14   ` Sakari Ailus
2018-03-13 11:14   ` Sakari Ailus
2018-03-13 15:47   ` Suman Anna
2018-03-13 15:47     ` Suman Anna
2018-03-13 15:47     ` Suman Anna
2018-03-14  9:28     ` Sakari Ailus
2018-03-14  9:28       ` Sakari Ailus
2018-03-14  9:28       ` Sakari Ailus

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.