[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <e8399fcf-e0d8-cc31-d9a7-b0f4f7cc3e71@quicinc.com>
Date: Tue, 18 Jul 2023 14:16:04 -0700
From: Nikunj Kela <quic_nkela@...cinc.com>
To: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
<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 7/18/2023 11:42 AM, Krzysztof Kozlowski wrote:
> 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.
Thanks, I thought you were referring to kzalloc cleanup. Will include
this fix. BTW, you may need to fix mailbox.c as well.
>
>>>> + 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