Re: [PATCH v5 5/6] soc: qcom: add pd-mapper implementation

[Krzysztof Kozlowski <krzk@kernel.org>]

On 19/04/2024 16:00, 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.
> 

...

> +
> +static const struct of_device_id qcom_pdm_domains[] = {
> +	{ .compatible = "qcom,apq8096", .data = msm8996_domains, },
> +	{ .compatible = "qcom,msm8996", .data = msm8996_domains, },
> +	{ .compatible = "qcom,msm8998", .data = msm8998_domains, },
> +	{ .compatible = "qcom,qcm2290", .data = qcm2290_domains, },
> +	{ .compatible = "qcom,qcs404", .data = qcs404_domains, },
> +	{ .compatible = "qcom,sc7180", .data = sc7180_domains, },
> +	{ .compatible = "qcom,sc7280", .data = sc7280_domains, },
> +	{ .compatible = "qcom,sc8180x", .data = sc8180x_domains, },
> +	{ .compatible = "qcom,sc8280xp", .data = sc8280xp_domains, },
> +	{ .compatible = "qcom,sda660", .data = sdm660_domains, },
> +	{ .compatible = "qcom,sdm660", .data = sdm660_domains, },
> +	{ .compatible = "qcom,sdm670", .data = sdm670_domains, },
> +	{ .compatible = "qcom,sdm845", .data = sdm845_domains, },
> +	{ .compatible = "qcom,sm6115", .data = sm6115_domains, },
> +	{ .compatible = "qcom,sm6350", .data = sm6350_domains, },
> +	{ .compatible = "qcom,sm8150", .data = sm8150_domains, },
> +	{ .compatible = "qcom,sm8250", .data = sm8250_domains, },
> +	{ .compatible = "qcom,sm8350", .data = sm8350_domains, },
> +	{ .compatible = "qcom,sm8450", .data = sm8350_domains, },
> +	{ .compatible = "qcom,sm8550", .data = sm8550_domains, },
> +	{ .compatible = "qcom,sm8650", .data = sm8550_domains, },
> +	{},
> +};

If this is supposed to be a module, then why no MODULE_DEVICE_TABLE?

> +
> +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;
> +
> +	pr_debug("PDM: starting service\n");

Drop simple entry/exit debug messages.

> +
> +	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 0;
> +	}
> +
> +	domains = match->data;

All this is odd a bit. Why is this not a driver? You are open coding
here of_device_get_match_data().


> +	if (!domains) {
> +		pr_debug("PDM: no domains\n");
> +		return 0;
> +	}
> +
> +	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);
> +	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;
> +	}
> +
> +	return 0;
> +
> +release_handle:
> +	qmi_handle_release(&qcom_pdm_handle);
> +
> +free_domains:
> +	qcom_pdm_free_domains();
> +
> +	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);
> +
> +	qcom_pdm_free_domains();
> +
> +	WARN_ON(!list_empty(&qcom_pdm_services));

This should be handled, not warned.

> +
> +	pr_debug("PDM: stopped service\n");

Drop debug. Tracing gives you such information.

> +}
> +
> +/**
> + * qcom_pdm_get() - ensure that PD mapper is up and running
> + */

Please provide full kerneldoc, so also return and short description.

> +int qcom_pdm_get(void)
> +{
> +	int ret = 0;
> +
> +	mutex_lock(&qcom_pdm_mutex);
> +
> +	if (!qcom_pdm_count)
> +		ret = qcom_pdm_start();
> +
> +	if (!ret)
> +		++qcom_pdm_count;
> +
> +	mutex_unlock(&qcom_pdm_mutex);

Looks like you implement refcnt manually...

Also, what happens if this module gets unloaded? How do you handle
module dependencies? I don't see any device links. Bartosz won't be
happy... We really need to stop adding more of
old-style-buggy-never-unload-logic. At least for new code.

> +
> +	return ret;
> +}

No export? Isn't this a module?

> +
> +/**
> + * qcom_pdm_release() - possibly stop PD mapper service
> + */
> +void qcom_pdm_release(void)
> +{

Best regards,
Krzysztof