[Intel-gfx] [PATCH 1/4] drm/i915/opregion: fix leaking fw on error path

[Lucas De Marchi]

On Fri, Nov 08, 2019 at 11:16:47AM +0200, Jani Nikula wrote:
>On Thu, 07 Nov 2019, Lucas De Marchi <lucas.demarchi at intel.com> wrote:
>> Convert the code to return-early style and fix missing calls
>> to release_firmware() if vbt is not valid.
>
>I don't understand where anything would leak in the current code. Please
>elaborate.

It was my failure reading the current code... not sure why on a first
read I thought it was leaking.

>
>You could make a case for a change in style to avoid so much
>indentation, but don't claim it fixes stuff if it doesn't.

yeah. I will keep it out for now.

Thanks
Lucas De Marchi

>
>>
>> Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
>> ---
>>  drivers/gpu/drm/i915/display/intel_opregion.c | 28 +++++++++++--------
>>  1 file changed, 17 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_opregion.c b/drivers/gpu/drm/i915/display/intel_opregion.c
>> index 969ade623691..9738511147b1 100644
>> --- a/drivers/gpu/drm/i915/display/intel_opregion.c
>> +++ b/drivers/gpu/drm/i915/display/intel_opregion.c
>> @@ -872,23 +872,29 @@ static int intel_load_vbt_firmware(struct drm_i915_private *dev_priv)
>>  		return ret;
>>  	}
>>
>> -	if (intel_bios_is_valid_vbt(fw->data, fw->size)) {
>> -		opregion->vbt_firmware = kmemdup(fw->data, fw->size, GFP_KERNEL);
>> -		if (opregion->vbt_firmware) {
>> -			DRM_DEBUG_KMS("Found valid VBT firmware \"%s\"\n", name);
>> -			opregion->vbt = opregion->vbt_firmware;
>> -			opregion->vbt_size = fw->size;
>> -			ret = 0;
>> -		} else {
>> -			ret = -ENOMEM;
>> -		}
>> -	} else {
>> +	if (!intel_bios_is_valid_vbt(fw->data, fw->size)) {
>>  		DRM_DEBUG_KMS("Invalid VBT firmware \"%s\"\n", name);
>>  		ret = -EINVAL;
>> +		goto err_release_fw;
>> +	}
>> +
>> +	opregion->vbt_firmware = kmemdup(fw->data, fw->size, GFP_KERNEL);
>> +	if (!opregion->vbt_firmware) {
>> +		ret = -ENOMEM;
>> +		goto err_release_fw;
>>  	}
>>
>> +	opregion->vbt = opregion->vbt_firmware;
>> +	opregion->vbt_size = fw->size;
>> +
>> +	DRM_DEBUG_KMS("Found valid VBT firmware \"%s\"\n", name);
>> +
>>  	release_firmware(fw);
>>
>> +	return 0;
>
>With ret = 0 at the beginning you could just remove the the above three
>lines and let this run through the below code.
>
>> +
>> +err_release_fw:
>> +	release_firmware(fw);
>
>Usually we'd have a blank line before the ret.
>
>BR,
>Jani.
>
>>  	return ret;
>>  }
>
>-- 
>Jani Nikula, Intel Open Source Graphics Center