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

On 18/07/2023 18:08, Nikunj Kela wrote:

Add a new transport channel to the SCMI firmware interface driver for
SCMI message exchange on Qualcomm virtual platforms.

The hypervisor associates an object-id also known as capability-id
with each hvc doorbell object. The capability-id is used to identify the
doorbell from the VM's capability namespace, similar to a file-descriptor.

...

+
+static bool qcom_hvc_chan_available(struct device_node *of_node, int idx)
+{
+	struct device_node *np = of_parse_phandle(of_node, "shmem", 0);
+
+	if (!np)
+		return false;
+
+	of_node_put(np);
+	return true;
+}
+
+static inline void qcom_hvc_channel_lock_init(struct scmi_qcom_hvc *scmi_info)
+{
+	mutex_init(&scmi_info->shmem_lock);
+}
+
+static inline void
+qcom_hvc_channel_lock_acquire(struct scmi_qcom_hvc *scmi_info,
+			      struct scmi_xfer *xfer __maybe_unused)

Why do you pass unused arguments?

+{
+	mutex_lock(&scmi_info->shmem_lock);

Why do you need these wrappers? Why not using mutexes directly?
If they are needed, then add lock __acquires annotation.

+}
+
+static inline void qcom_hvc_channel_lock_release(struct scmi_qcom_hvc
+						 *scmi_info)

Same comments.

+{
+	mutex_unlock(&scmi_info->shmem_lock);
+}
+
+static int qcom_hvc_chan_setup(struct scmi_chan_info *cinfo,
+			       struct device *dev, bool tx)
+{
+	struct device *cdev = cinfo->dev;
+	struct scmi_qcom_hvc *scmi_info;
+	resource_size_t size;
+	struct resource res;
+	struct device_node *np;
+	unsigned long cap_id;
+	u32 func_id;
+	int ret, irq;
+
+	if (!tx)
+		return -ENODEV;
+
+	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.

+		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.

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel