Re: [PATCH 4/7] ASoC: Intel: Skylake: Shield against no-NHLT configurations

[Cezary Rojewski <cezary.rojewski@intel.com>]

On 2020-03-09 18:01, Pierre-Louis Bossart wrote:
> On 3/9/20 8:03 AM, Cezary Rojewski wrote:
>> On 2020-03-06 22:03, Pierre-Louis Bossart wrote:
>>>
>>>
>>>> -    intel_nhlt_free(skl->nhlt);
>>>> +    if (skl->nhlt)
>>>> +        intel_nhlt_free(skl->nhlt);
>>>
>>> we could alternatively move the test in intel_nhlt_free, which seems 
>>> like a more robust thing to do?
>>
>> Depends. In general kernel-internal API trusts its caller and 
>> appending 'ifs' everywhere would unnecessarily slow entire kernel 
>> down. While intel_nhlt_free is called rarely, I'd still argue caller 
>> should be sane about its invocation.
>>
>> 'if' in skl_probe could be avoided had the function's structure been 
>> better. 'if' in skl_remove is just fine, though.
>>
>> Let's leave it as is.
> 
> it's also used in SOF:
> 
> sound/soc/sof/intel/hda.c:              intel_nhlt_free(nhlt);
> 
> that's why I suggested to factor the test so that both users don't need 
> to add the if.

I understand, and my explanation still applies.

SOF's intel_nhlt_free usage is great example actually. Caller is sane 
about its doings as it should be. Internal API needs not to suffer from 
callers irresponsibility.

PCM does not call ::free() if ::open() fails, same as device-driver 
model does not care about ::remove() if ::probe() fails.

Czarek