Re: [PATCH 02/33] iris: vidc: add core functions

[Krzysztof Kozlowski <krzk@kernel.org>]

On 14/08/2023 20:54, Dikshita Agarwal wrote:
>>> +
>>> +	core = devm_kzalloc(&pdev->dev, sizeof(struct msm_vidc_core), GFP_KERNEL);
>>> +	if (!core) {
>>> +		d_vpr_e("%s: failed to alloc memory for core\n", __func__);
>>
>> Ooops, this for sure did not pass any checks by tools. Sorry, please run
>> basic checks like coccinelle, smatch, sparse, W=1 builds.
>>
> we ran check patch and smatch on this code but no errors were reported.

coccinelle is missing

> please elaborate what issue you see with above code?
>>> +		return -ENOMEM;

We did quite a lot of cleanups long, long time ago removing all unneeded
error messages from memory allocations failures.

Maybe Your use of custom printks() confuses coccinelle, which is a proof
that your code is here an anti-pattern.

...

>>
> Sure, will remove these custom wrapper for locks and use standard
> mutex_lock/unlock APIs directly.
>>> +	allow = msm_vidc_allow_pm_suspend(core);
>>> +
>>> +	if (allow == MSM_VIDC_IGNORE) {
>>> +		d_vpr_h("%s: pm already suspended\n", __func__);
>>
>> So you have bug in PM runtime code? Runtime PM does not suspend devices
>> twice.
>> core is power collapsed in case of idle state when there is no processing
> happening by hardware, this can also change the core state to power_disable.
> this check is ensuring if the core is already in power disable state due to
> that, then no need to suspend it again.

No, you just re-implemented runtime PM.


Best regards,
Krzysztof