public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH] ArmPkg/Library/ArmDmaLib: Deallocate Map buffer in case of error
@ 2016-09-22 22:37 Daniil Egranov
  2016-09-23  8:00 ` Ard Biesheuvel
  0 siblings, 1 reply; 5+ messages in thread
From: Daniil Egranov @ 2016-09-22 22:37 UTC (permalink / raw)
  To: edk2-devel; +Cc: leif.lindholm, Daniil Egranov

The patch is fixing memory leak in case of errors.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Daniil Egranov <daniil.egranov@arm.com>
---
 ArmPkg/Library/ArmDmaLib/ArmDmaLib.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c b/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c
index d48d6ff..e0006c0 100644
--- a/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c
+++ b/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c
@@ -90,14 +90,13 @@ DmaMap (
     return  EFI_OUT_OF_RESOURCES;
   }
 
-  *Mapping = Map;
-
   if ((((UINTN)HostAddress & (gCacheAlignment - 1)) != 0) ||
       ((*NumberOfBytes & (gCacheAlignment - 1)) != 0)) {
 
     // Get the cacheability of the region
     Status = gDS->GetMemorySpaceDescriptor (*DeviceAddress, &GcdDescriptor);
     if (EFI_ERROR(Status)) {
+      FreePool(Map);
       return Status;
     }
 
@@ -112,6 +111,7 @@ DmaMap (
           "%a: Operation type 'MapOperationBusMasterCommonBuffer' is only supported\n"
           "on memory regions that were allocated using DmaAllocateBuffer ()\n",
           __FUNCTION__));
+        FreePool(Map);
         return EFI_UNSUPPORTED;
       }
 
@@ -122,6 +122,7 @@ DmaMap (
       Map->DoubleBuffer  = TRUE;
       Status = DmaAllocateBuffer (EfiBootServicesData, EFI_SIZE_TO_PAGES (*NumberOfBytes), &Buffer);
       if (EFI_ERROR (Status)) {
+        FreePool(Map);
         return Status;
       }
 
@@ -162,6 +163,8 @@ DmaMap (
   Map->NumberOfBytes = *NumberOfBytes;
   Map->Operation     = Operation;
 
+  *Mapping = Map;
+
   return EFI_SUCCESS;
 }
 
-- 
2.7.4



^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] ArmPkg/Library/ArmDmaLib: Deallocate Map buffer in case of error
  2016-09-22 22:37 [PATCH] ArmPkg/Library/ArmDmaLib: Deallocate Map buffer in case of error Daniil Egranov
@ 2016-09-23  8:00 ` Ard Biesheuvel
  2016-11-01 17:58   ` Ryan Harkin
  0 siblings, 1 reply; 5+ messages in thread
From: Ard Biesheuvel @ 2016-09-23  8:00 UTC (permalink / raw)
  To: Daniil Egranov; +Cc: edk2-devel@lists.01.org, Leif Lindholm

On 22 September 2016 at 23:37, Daniil Egranov <daniil.egranov@arm.com> wrote:
> The patch is fixing memory leak in case of errors.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Daniil Egranov <daniil.egranov@arm.com>

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---
>  ArmPkg/Library/ArmDmaLib/ArmDmaLib.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c b/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c
> index d48d6ff..e0006c0 100644
> --- a/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c
> +++ b/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c
> @@ -90,14 +90,13 @@ DmaMap (
>      return  EFI_OUT_OF_RESOURCES;
>    }
>
> -  *Mapping = Map;
> -
>    if ((((UINTN)HostAddress & (gCacheAlignment - 1)) != 0) ||
>        ((*NumberOfBytes & (gCacheAlignment - 1)) != 0)) {
>
>      // Get the cacheability of the region
>      Status = gDS->GetMemorySpaceDescriptor (*DeviceAddress, &GcdDescriptor);
>      if (EFI_ERROR(Status)) {
> +      FreePool(Map);
>        return Status;
>      }
>
> @@ -112,6 +111,7 @@ DmaMap (
>            "%a: Operation type 'MapOperationBusMasterCommonBuffer' is only supported\n"
>            "on memory regions that were allocated using DmaAllocateBuffer ()\n",
>            __FUNCTION__));
> +        FreePool(Map);
>          return EFI_UNSUPPORTED;
>        }
>
> @@ -122,6 +122,7 @@ DmaMap (
>        Map->DoubleBuffer  = TRUE;
>        Status = DmaAllocateBuffer (EfiBootServicesData, EFI_SIZE_TO_PAGES (*NumberOfBytes), &Buffer);
>        if (EFI_ERROR (Status)) {
> +        FreePool(Map);
>          return Status;
>        }
>
> @@ -162,6 +163,8 @@ DmaMap (
>    Map->NumberOfBytes = *NumberOfBytes;
>    Map->Operation     = Operation;
>
> +  *Mapping = Map;
> +
>    return EFI_SUCCESS;
>  }
>
> --
> 2.7.4
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] ArmPkg/Library/ArmDmaLib: Deallocate Map buffer in case of error
  2016-09-23  8:00 ` Ard Biesheuvel
@ 2016-11-01 17:58   ` Ryan Harkin
  2016-11-02 11:56     ` Ryan Harkin
  0 siblings, 1 reply; 5+ messages in thread
From: Ryan Harkin @ 2016-11-01 17:58 UTC (permalink / raw)
  To: Ard Biesheuvel; +Cc: Daniil Egranov, edk2-devel@lists.01.org, Leif Lindholm

Hi Leif,

Is this waiting for a Tested-by or something else?

Cheers,
Ryan.

On 23 September 2016 at 09:00, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 22 September 2016 at 23:37, Daniil Egranov <daniil.egranov@arm.com> wrote:
>> The patch is fixing memory leak in case of errors.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Daniil Egranov <daniil.egranov@arm.com>
>
> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>
>> ---
>>  ArmPkg/Library/ArmDmaLib/ArmDmaLib.c | 7 +++++--
>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c b/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c
>> index d48d6ff..e0006c0 100644
>> --- a/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c
>> +++ b/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c
>> @@ -90,14 +90,13 @@ DmaMap (
>>      return  EFI_OUT_OF_RESOURCES;
>>    }
>>
>> -  *Mapping = Map;
>> -
>>    if ((((UINTN)HostAddress & (gCacheAlignment - 1)) != 0) ||
>>        ((*NumberOfBytes & (gCacheAlignment - 1)) != 0)) {
>>
>>      // Get the cacheability of the region
>>      Status = gDS->GetMemorySpaceDescriptor (*DeviceAddress, &GcdDescriptor);
>>      if (EFI_ERROR(Status)) {
>> +      FreePool(Map);
>>        return Status;
>>      }
>>
>> @@ -112,6 +111,7 @@ DmaMap (
>>            "%a: Operation type 'MapOperationBusMasterCommonBuffer' is only supported\n"
>>            "on memory regions that were allocated using DmaAllocateBuffer ()\n",
>>            __FUNCTION__));
>> +        FreePool(Map);
>>          return EFI_UNSUPPORTED;
>>        }
>>
>> @@ -122,6 +122,7 @@ DmaMap (
>>        Map->DoubleBuffer  = TRUE;
>>        Status = DmaAllocateBuffer (EfiBootServicesData, EFI_SIZE_TO_PAGES (*NumberOfBytes), &Buffer);
>>        if (EFI_ERROR (Status)) {
>> +        FreePool(Map);
>>          return Status;
>>        }
>>
>> @@ -162,6 +163,8 @@ DmaMap (
>>    Map->NumberOfBytes = *NumberOfBytes;
>>    Map->Operation     = Operation;
>>
>> +  *Mapping = Map;
>> +
>>    return EFI_SUCCESS;
>>  }
>>
>> --
>> 2.7.4
>>
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] ArmPkg/Library/ArmDmaLib: Deallocate Map buffer in case of error
  2016-11-01 17:58   ` Ryan Harkin
@ 2016-11-02 11:56     ` Ryan Harkin
  2016-11-15 13:28       ` Ard Biesheuvel
  0 siblings, 1 reply; 5+ messages in thread
From: Ryan Harkin @ 2016-11-02 11:56 UTC (permalink / raw)
  To: Ard Biesheuvel; +Cc: Daniil Egranov, edk2-devel@lists.01.org, Leif Lindholm

On 1 November 2016 at 17:58, Ryan Harkin <ryan.harkin@linaro.org> wrote:
> Hi Leif,
>
> Is this waiting for a Tested-by or something else?
>

Either way, I tested it on Juno R0/1/2 while I was testing the MAC
address change...


> Cheers,
> Ryan.
>
> On 23 September 2016 at 09:00, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>> On 22 September 2016 at 23:37, Daniil Egranov <daniil.egranov@arm.com> wrote:
>>> The patch is fixing memory leak in case of errors.
>>>
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Daniil Egranov <daniil.egranov@arm.com>
>>
>> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>

Tested-by; Ryan Harkin <ryan.harkin@linaro.org>

>>> ---
>>>  ArmPkg/Library/ArmDmaLib/ArmDmaLib.c | 7 +++++--
>>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c b/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c
>>> index d48d6ff..e0006c0 100644
>>> --- a/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c
>>> +++ b/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c
>>> @@ -90,14 +90,13 @@ DmaMap (
>>>      return  EFI_OUT_OF_RESOURCES;
>>>    }
>>>
>>> -  *Mapping = Map;
>>> -
>>>    if ((((UINTN)HostAddress & (gCacheAlignment - 1)) != 0) ||
>>>        ((*NumberOfBytes & (gCacheAlignment - 1)) != 0)) {
>>>
>>>      // Get the cacheability of the region
>>>      Status = gDS->GetMemorySpaceDescriptor (*DeviceAddress, &GcdDescriptor);
>>>      if (EFI_ERROR(Status)) {
>>> +      FreePool(Map);
>>>        return Status;
>>>      }
>>>
>>> @@ -112,6 +111,7 @@ DmaMap (
>>>            "%a: Operation type 'MapOperationBusMasterCommonBuffer' is only supported\n"
>>>            "on memory regions that were allocated using DmaAllocateBuffer ()\n",
>>>            __FUNCTION__));
>>> +        FreePool(Map);
>>>          return EFI_UNSUPPORTED;
>>>        }
>>>
>>> @@ -122,6 +122,7 @@ DmaMap (
>>>        Map->DoubleBuffer  = TRUE;
>>>        Status = DmaAllocateBuffer (EfiBootServicesData, EFI_SIZE_TO_PAGES (*NumberOfBytes), &Buffer);
>>>        if (EFI_ERROR (Status)) {
>>> +        FreePool(Map);
>>>          return Status;
>>>        }
>>>
>>> @@ -162,6 +163,8 @@ DmaMap (
>>>    Map->NumberOfBytes = *NumberOfBytes;
>>>    Map->Operation     = Operation;
>>>
>>> +  *Mapping = Map;
>>> +
>>>    return EFI_SUCCESS;
>>>  }
>>>
>>> --
>>> 2.7.4
>>>
>>> _______________________________________________
>>> edk2-devel mailing list
>>> edk2-devel@lists.01.org
>>> https://lists.01.org/mailman/listinfo/edk2-devel
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] ArmPkg/Library/ArmDmaLib: Deallocate Map buffer in case of error
  2016-11-02 11:56     ` Ryan Harkin
@ 2016-11-15 13:28       ` Ard Biesheuvel
  0 siblings, 0 replies; 5+ messages in thread
From: Ard Biesheuvel @ 2016-11-15 13:28 UTC (permalink / raw)
  To: Ryan Harkin; +Cc: Daniil Egranov, edk2-devel@lists.01.org, Leif Lindholm

On 2 November 2016 at 11:56, Ryan Harkin <ryan.harkin@linaro.org> wrote:
> On 1 November 2016 at 17:58, Ryan Harkin <ryan.harkin@linaro.org> wrote:
>> Hi Leif,
>>
>> Is this waiting for a Tested-by or something else?
>>
>
> Either way, I tested it on Juno R0/1/2 while I was testing the MAC
> address change...
>
>
>> Cheers,
>> Ryan.
>>
>> On 23 September 2016 at 09:00, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>> On 22 September 2016 at 23:37, Daniil Egranov <daniil.egranov@arm.com> wrote:
>>>> The patch is fixing memory leak in case of errors.
>>>>
>>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>>> Signed-off-by: Daniil Egranov <daniil.egranov@arm.com>
>>>
>>> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>>
>
> Tested-by; Ryan Harkin <ryan.harkin@linaro.org>
>

Pushed, thanks


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2016-11-15 13:28 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-09-22 22:37 [PATCH] ArmPkg/Library/ArmDmaLib: Deallocate Map buffer in case of error Daniil Egranov
2016-09-23  8:00 ` Ard Biesheuvel
2016-11-01 17:58   ` Ryan Harkin
2016-11-02 11:56     ` Ryan Harkin
2016-11-15 13:28       ` Ard Biesheuvel

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox