Re: [PATCH 2/2] firmware: arm_scmi: Add qcom hvc/shmem transport

[Nikunj Kela]

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