On Fri, 19 Apr 2024 at 20:07, Krzysztof Kozlowski <krzk@xxxxxxxxxx> wrote:
>
> 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?
Ok, I should add this to the commit message.
For now:
This module is loaded automatically by the remoteproc drivers when
necessary. It uses a root node to match a protection domains map for a
particular device.
>
> > +
> > +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.
ack
>
> > +
> > + 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().
Except that it matches the root node instead of matching a device.
>
>
> > + 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.
I'll just drop it, qcom_pdm_free_domains() should free them.
>
> > +
> > + 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...
Yes... There is refcount_dec_and_mutex_lock(), but I found no
corresponding refcount_add_and_mutex_lock(). Maybe I'm
misunderstanding that framework.
I need to have a mutex after incrementing the lock from 0, so that the
driver can init QMI handlers.
> 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.
Module dependencies are handled by the symbol dependencies.
Remoteproc module depends on this symbol. Once q6v5 remoteproc modules
are unloaded this module can be unloaded too.
But I know what got missing. I should add 'depends on PD_MAPPER ||
!PD_MAPPER' to remoteproc Kconfig entries.
>
> > +
> > + return ret;
> > +}
>
> No export? Isn't this a module?
Ack, missed them.
>
> > +
> > +/**
> > + * qcom_pdm_release() - possibly stop PD mapper service
> > + */
> > +void qcom_pdm_release(void)
> > +{
>
> Best regards,
> Krzysztof
>
--
With best wishes
Dmitry
