Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <a113f6cf-db27-67f1-6996-39fd3912c0c8@kernel.org>
Date:   Mon, 14 Aug 2023 22:04:40 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.14.0
Subject: Re: [PATCH 02/33] iris: vidc: add core functions
Content-Language: en-US
To:     Dikshita Agarwal <quic_dikshita@quicinc.com>,
        Vikash Garodia <quic_vgarodia@quicinc.com>,
        stanimir.k.varbanov@gmail.com, agross@kernel.org,
        andersson@kernel.org, konrad.dybcio@linaro.org, mchehab@kernel.org,
        hans.verkuil@cisco.com, linux-kernel@vger.kernel.org,
        linux-media@vger.kernel.org, linux-arm-msm@vger.kernel.org
References: <1690550624-14642-1-git-send-email-quic_vgarodia@quicinc.com>
 <1690550624-14642-3-git-send-email-quic_vgarodia@quicinc.com>
 <0149fcd0-e64b-f155-05d8-f32a78d7e83b@kernel.org>
 <18ee06b6-65bc-a320-f06c-7d18ed29c739@quicinc.com>
From:   Krzysztof Kozlowski <krzk@kernel.org>
In-Reply-To: <18ee06b6-65bc-a320-f06c-7d18ed29c739@quicinc.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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