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: <ec3d7769-8a5f-d938-7f77-351ddfe6fb45@linaro.org>
Date:   Tue, 18 Jul 2023 20:42:41 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.13.0
Subject: Re: [PATCH 2/2] firmware: arm_scmi: Add qcom hvc/shmem transport
Content-Language: en-US
To:     Nikunj Kela <quic_nkela@quicinc.com>, sudeep.holla@arm.com
Cc:     cristian.marussi@arm.com, robh+dt@kernel.org,
        krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org,
        agross@kernel.org, andersson@kernel.org, konrad.dybcio@linaro.org,
        linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org
References: <20230718160833.36397-1-quic_nkela@quicinc.com>
 <20230718160833.36397-3-quic_nkela@quicinc.com>
 <d9d5ffd5-6f85-f091-5d69-12cdd8d04c99@linaro.org>
 <3ae2d618-490a-06da-b4b6-b5a24b0a9747@quicinc.com>
From:   Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
In-Reply-To: <3ae2d618-490a-06da-b4b6-b5a24b0a9747@quicinc.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 18/07/2023 20:25, Nikunj Kela wrote:
>>> +
>>> +	scmi_info = devm_kzalloc(dev, sizeof(*scmi_info), GFP_KERNEL);
>>> +	if (!scmi_info)
>>> +		return -ENOMEM;
>>> +
>>> +	np = of_parse_phandle(cdev->of_node, "shmem", 0);
>>> +	if (!of_device_is_compatible(np, "arm,scmi-shmem"))
>> You leak here reference.
> Wouldn't the devm_* API take care of that implicitly? It is same in 
> smc.c as well.

Thanks for bringing my attention to this. I sent a fix for smc.c. Fix
your patch as well, please.

>>> +		return -ENXIO;
>>> +
>>> +	ret = of_address_to_resource(np, 0, &res);
>>> +	of_node_put(np);
>>> +	if (ret) {
>>> +		dev_err(cdev, "failed to get SCMI Tx shared memory\n");
>>> +		return ret;
>>> +	}
>>> +
>>> +	size = resource_size(&res);
>>> +
>>> +	/* let's map 2 additional ulong since
>>> +	 * func-id & capability-id are kept after shmem.
>>> +	 *     +-------+
>>> +	 *     |       |
>>> +	 *     | shmem |
>>> +	 *     |       |
>>> +	 *     |       |
>>> +	 *     +-------+ <-- size
>>> +	 *     | funcId|
>>> +	 *     +-------+ <-- size + sizeof(ulong)
>>> +	 *     | capId |
>>> +	 *     +-------+ <-- size + 2*sizeof(ulong)
>>> +	 */
>>> +
>>> +	scmi_info->shmem = devm_ioremap(dev, res.start,
>>> +					size + 2 * sizeof(unsigned long));
>>> +	if (!scmi_info->shmem) {
>>> +		dev_err(dev, "failed to ioremap SCMI Tx shared memory\n");
>>> +		return -EADDRNOTAVAIL;
>>> +	}
>>> +
>>> +	func_id = readl((void *)(scmi_info->shmem) + size);
>>> +
>>> +#ifdef CONFIG_ARM64
>>> +	cap_id = readq((void *)(scmi_info->shmem) + size +
>>> +		       sizeof(unsigned long));
>>> +#else
>>> +	cap_id = readl((void *)(scmi_info->shmem) + size +
>>> +		       sizeof(unsigned long));
>>> +#endif
>>> +
>>> +	/*
>>> +	 * If there is an interrupt named "a2p", then the service and
>>> +	 * completion of a message is signaled by an interrupt rather than by
>>> +	 * the return of the hvc call.
>>> +	 */
>>> +	irq = of_irq_get_byname(cdev->of_node, "a2p");
>>> +	if (irq > 0) {
>>> +		ret = devm_request_irq(dev, irq, qcom_hvc_msg_done_isr,
>>> +				       IRQF_NO_SUSPEND,
>>> +				       dev_name(dev), scmi_info);
>>> +		if (ret) {
>>> +			dev_err(dev, "failed to setup SCMI completion irq\n");
>> return dev_err_probe, unless this is not called in probe... but then
>> using devm-interface raises questions.
> This is copied as is from existing smc.c

I understand and I hope you understand the code you copied. If there is
a bug in existing code, please do not copy it to new code (like leaking
OF node reference).

Best regards,
Krzysztof