Received-SPF: pass (google.com: domain of linux-kernel+bounces-160774-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <6b6298c4-485d-ebe5-fa8d-9edc461467a0@quicinc.com>
Date: Fri, 26 Apr 2024 16:25:29 -0700
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.15.1
Subject: Re: [PATCH v7 5/6] soc: qcom: add pd-mapper implementation
To: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>,
        Bjorn Andersson
	<andersson@kernel.org>,
        Konrad Dybcio <konrad.dybcio@linaro.org>,
        Sibi Sankar
	<quic_sibis@quicinc.com>,
        Mathieu Poirier <mathieu.poirier@linaro.org>
CC: <linux-arm-msm@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <linux-remoteproc@vger.kernel.org>,
        Johan Hovold <johan+linaro@kernel.org>,
        Xilin Wu <wuxilin123@gmail.com>,
        Bryan O'Donoghue
	<bryan.odonoghue@linaro.org>
References: <20240424-qcom-pd-mapper-v7-0-05f7fc646e0f@linaro.org>
 <20240424-qcom-pd-mapper-v7-5-05f7fc646e0f@linaro.org>
Content-Language: en-US
From: Chris Lew <quic_clew@quicinc.com>
In-Reply-To: <20240424-qcom-pd-mapper-v7-5-05f7fc646e0f@linaro.org>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit


On 4/24/2024 2:28 AM, Dmitry Baryshkov wrote:

> +static int qcom_pdm_start(void)
> +{
> +	const struct of_device_id *match;
> +	const struct qcom_pdm_domain_data * const *domains;
> +	struct device_node *root;
> +	int ret, i;
> +
> +	root = of_find_node_by_path("/");
> +	if (!root)
> +		return -ENODEV;
> +
> +	match = of_match_node(qcom_pdm_domains, root);
> +	of_node_put(root);
> +	if (!match) {
> +		pr_notice("PDM: no support for the platform, userspace daemon might be required.\n");
> +		return -ENODEV;
> +	}
> +
> +	domains = match->data;
> +	if (!domains) {
> +		pr_debug("PDM: no domains\n");
> +		return -ENODEV;
> +	}
> +
> +	mutex_lock(&qcom_pdm_mutex);
> +	for (i = 0; domains[i]; i++) {
> +		ret = qcom_pdm_add_domain(domains[i]);
> +		if (ret)
> +			goto free_domains;
> +	}
> +
> +	ret = qmi_handle_init(&qcom_pdm_handle, 1024,
> +			      NULL, qcom_pdm_msg_handlers);

1024 here seems arbitrary, I think most other usage of qmi_handle_init 
has a macro defined for the max message length of the qmi service.

> +	if (ret)
> +		goto free_domains;
> +
> +	ret = qmi_add_server(&qcom_pdm_handle, SERVREG_LOCATOR_SERVICE,
> +			     SERVREG_QMI_VERSION, SERVREG_QMI_INSTANCE);
> +	if (ret) {
> +		pr_err("PDM: error adding server %d\n", ret);
> +		goto release_handle;
> +	}
> +	mutex_unlock(&qcom_pdm_mutex);
> +
> +	return 0;
> +
> +release_handle:
> +	qmi_handle_release(&qcom_pdm_handle);
> +
> +free_domains:
> +	qcom_pdm_free_domains();
> +	mutex_unlock(&qcom_pdm_mutex);
> +
> +	return ret;
> +}
> +
> +static void qcom_pdm_stop(void)
> +{
> +	qmi_del_server(&qcom_pdm_handle, SERVREG_LOCATOR_SERVICE,
> +		       SERVREG_QMI_VERSION, SERVREG_QMI_INSTANCE);
> +
> +	qmi_handle_release(&qcom_pdm_handle);
> +

I don't think doing an explicit qmi_del_server() is necessary. As part 
of the qmi_handle_release(), the qrtr socket will be closed and the qrtr 
ns will broadcast a DEL_SERVER and DEL_CLIENT notification as part of 
the cleanup.

> +	qcom_pdm_free_domains();
> +}
> +