[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ec3d7769-8a5f-d938-7f77-351ddfe6fb45@linaro.org>
Date: Tue, 18 Jul 2023 20:42:41 +0200
From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To: Nikunj Kela <quic_nkela@...cinc.com>, sudeep.holla@....com
Cc: cristian.marussi@....com, robh+dt@...nel.org,
krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org,
agross@...nel.org, andersson@...nel.org, konrad.dybcio@...aro.org,
linux-arm-kernel@...ts.infradead.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org
Subject: Re: [PATCH 2/2] firmware: arm_scmi: Add qcom hvc/shmem transport
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
Powered by blists - more mailing lists