On Mon, Mar 11, 2024 at 05:34:03PM +0200, Dmitry Baryshkov wrote:
> Existing userspace protection domain mapper implementation has several
> issue. It doesn't play well with CONFIG_EXTRA_FIRMWARE, it doesn't
> reread JSON files if firmware location is changed (or if firmware was
> not available at the time pd-mapper was started but the corresponding
> directory is mounted later), etc.
>
> Provide in-kernel service implementing protection domain mapping
> required to work with several services, which are provided by the DSP
> firmware.
>
> Signed-off-by: Dmitry Baryshkov <[email protected]>
Just a couple of drive-by comments below.
> +int qcom_pdm_add_domains(const struct qcom_pdm_domain_data * const *data,
> size_t num_data)
> +{
> + int ret;
> + int i;
> +
> + mutex_lock(&qcom_pdm_mutex);
> +
> + if (qcom_pdm_server_added) {
> + ret = qmi_del_server(&qcom_pdm_handle, SERVREG_QMI_SERVICE,
> + SERVREG_QMI_VERSION, SERVREG_QMI_INSTANCE);
> + if (ret)
> + goto err_out;
Name error labels after what they do, that is, 'err_unlock' in this
case.
> + }
> +
> + for (i = 0; i < num_data; i++) {
> + ret = qcom_pdm_add_domain_locked(data[i]);
> + if (ret)
> + goto err;
And err_del_domains here.
> + }
> +
> + ret = qmi_add_server(&qcom_pdm_handle, SERVREG_QMI_SERVICE,
> + SERVREG_QMI_VERSION, SERVREG_QMI_INSTANCE);
> + if (ret) {
> + pr_err("PDM: error adding server %d\n", ret);
> + goto err;
> + }
> +
> + qcom_pdm_server_added = true;
> +
> + mutex_unlock(&qcom_pdm_mutex);
> +
> + return 0;
> +
> +err:
> + while (--i >= 0)
> + qcom_pdm_del_domain_locked(data[i]);
> +
> + qmi_add_server(&qcom_pdm_handle, SERVREG_QMI_SERVICE,
> + SERVREG_QMI_VERSION, SERVREG_QMI_INSTANCE);
Should the server really be added unconditionally here? And if so,
shouldn't you update that flag?
> +
> +err_out:
> + mutex_unlock(&qcom_pdm_mutex);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(qcom_pdm_add_domains);
> +static void qcom_pdm_get_domain_list(struct qmi_handle *qmi,
> + struct sockaddr_qrtr *sq,
> + struct qmi_txn *txn,
> + const void *decoded)
> +{
> + const struct servreg_loc_get_domain_list_req *req = decoded;
> + struct servreg_loc_get_domain_list_resp *rsp = kzalloc(sizeof(*rsp),
> GFP_KERNEL);
> + struct qcom_pdm_service *service;
> + u32 offset;
> + int ret;
> +
> + offset = req->offset_valid ? req->offset : 0;
> +
> + rsp->rsp.result = QMI_RESULT_SUCCESS_V01;
> + rsp->rsp.error = QMI_ERR_NONE_V01;
> +
> + rsp->db_revision_valid = true;
> + rsp->db_revision = 1;
> +
> + rsp->total_domains_valid = true;
> + rsp->total_domains = 0;
> +
> + mutex_lock(&qcom_pdm_mutex);
> +
> + service = qcom_pdm_find_locked(req->name);
> + if (service) {
> + struct qcom_pdm_domain *domain;
> +
> + rsp->domain_list_valid = true;
> + rsp->domain_list_len = 0;
> +
> + list_for_each_entry(domain, &service->domains, list) {
> + u32 i = rsp->total_domains++;
> +
> + if (i >= offset && i < SERVREG_LOC_MAX_DOMAINS) {
> + u32 j = rsp->domain_list_len++;
> +
> + strscpy(rsp->domain_list[j].name, domain->name,
> + sizeof(rsp->domain_list[i].name));
> + rsp->domain_list[j].instance_id =
> domain->instance_id;
> +
> + pr_debug("PDM: found %s / %d\n", domain->name,
> + domain->instance_id);
> + }
> + }
> +
Stray newline.
> + }
> +
> + mutex_unlock(&qcom_pdm_mutex);
> +
> + pr_debug("PDM: service '%s' offset %d returning %d domains (of %d)\n",
> req->name,
> + req->offset_valid ? req->offset : -1, rsp->domain_list_len,
> rsp->total_domains);
> +
> + ret = qmi_send_response(qmi, sq, txn, SERVREG_LOC_GET_DOMAIN_LIST,
> + 2658,
> + servreg_loc_get_domain_list_resp_ei, rsp);
> + if (ret)
> + pr_err("Error sending servreg response: %d\n", ret);
> +
> + kfree(rsp);
> +}
Johan
