linux-kernel - Re: [PATCH v5 2/5] coresight: Add coresight QMI driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <86bc707d-72d7-44f2-b77f-3fa89e25789a@oss.qualcomm.com>
Date: Fri, 25 Apr 2025 21:58:38 +0200
From: Konrad Dybcio <konrad.dybcio@....qualcomm.com>
To: Mao Jinlong <quic_jinlmao@...cinc.com>,
        Suzuki K Poulose <suzuki.poulose@....com>,
        Mike Leach
 <mike.leach@...aro.org>,
        James Clark <james.clark@...aro.org>, Rob Herring <robh@...nel.org>,
        Krzysztof Kozlowski <krzk+dt@...nel.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Bjorn Andersson <andersson@...nel.org>,
        Konrad Dybcio <konradybcio@...nel.org>
Cc: coresight@...ts.linaro.org, linux-arm-kernel@...ts.infradead.org,
        linux-arm-msm@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 2/5] coresight: Add coresight QMI driver

On 4/24/25 1:58 PM, Mao Jinlong wrote:
> Coresight QMI driver uses QMI(Qualcomm Messaging Interface) interfaces
> to communicate with remote subsystems. Driver gets the instance id and
> service id from device tree node and init the QMI connections to remote
> subsystems. Send request function is for other coresight drivers to
> communicate with remote subsystems.
> 
> Signed-off-by: Mao Jinlong <quic_jinlmao@...cinc.com>
> ---

[...]

> +static int coresight_qmi_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *node = pdev->dev.of_node;
> +	struct device_node *child_node;
> +	int ret;
> +
> +	/**

Two starts means kerneldoc, please use one for a normal multiline comment

> +	 * Get the instance id and service id of the QMI service connection
> +	 * from DT node. Creates QMI handle and register new lookup for each

The first sentence is a bit redundant.

In the second, Creates -> Create for imperative mood

[...]

> +static const struct of_device_id coresight_qmi_match[] = {
> +	{.compatible = "qcom,coresight-qmi"},

Nit: please add a space after { and before }

> +	{}
> +};
> +
> +static struct platform_driver coresight_qmi_driver = {
> +	.probe          = coresight_qmi_probe,
> +	.remove         = coresight_qmi_remove,
> +	.driver         = {
> +		.name   = "coresight-qmi",
> +		.of_match_table = coresight_qmi_match,
> +	},
> +};
> +
> +static int __init coresight_qmi_init(void)
> +{
> +	return platform_driver_register(&coresight_qmi_driver);
> +}
> +module_init(coresight_qmi_init);
> +
> +static void __exit coresight_qmi_exit(void)
> +{
> +	platform_driver_unregister(&coresight_qmi_driver);
> +}
> +module_exit(coresight_qmi_exit);

You can drop the __init and __exit funcs and substitute them
with module_platform_driver()

> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("CoreSight QMI driver");
> diff --git a/drivers/hwtracing/coresight/coresight-qmi.h b/drivers/hwtracing/coresight/coresight-qmi.h
> new file mode 100644
> index 000000000000..1d57e46177b8
> --- /dev/null
> +++ b/drivers/hwtracing/coresight/coresight-qmi.h
> @@ -0,0 +1,107 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (c) 2025 Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +#ifndef _CORESIGHT_QMI_H
> +#define _CORESIGHT_QMI_H
> +
> +#include <linux/soc/qcom/qmi.h>
> +
> +#define CORESIGHT_QMI_VERSION			(1)
> +
> +#define CORESIGHT_QMI_SET_ETM_REQ_V01		(0x002C)
> +#define CORESIGHT_QMI_SET_ETM_RESP_V01		(0x002C)
> +
> +#define CORESIGHT_QMI_MAX_MSG_LEN (50)
> +
> +#define TIMEOUT_MS				(10000)

Parentheses around constants are unnecesary

> +
> +/* Qmi data for the QMI connection */
> +struct qmi_data {
> +	u32			qmi_id;
> +	u32			service_id;
> +	struct list_head	node;
> +	struct qmi_handle	handle;
> +	bool			service_connected;
> +	struct sockaddr_qrtr	s_addr;
> +};
> +
> +/**
> + * QMI service IDs

This is not valid kerneldoc, try make W=1

Konrad