On Fri, 19 Apr 2024 at 21:15, Krzysztof Kozlowski <krzk@xxxxxxxxxx> wrote:
>
> On 19/04/2024 20:10, Dmitry Baryshkov wrote:
> > 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
>
> Hm? How remoteproc loads this module?
remoteproc drivers call qcom_pdm_start(). This brings in this module
via symbol deps.
>
> > 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.
>
> Yep, but if this was proper device then things get simpler, don't they?
I don't think we are supposed to have devices for software things?
This is purely a software construct. It replaces userspace daemon for
the reason outlined in the cover letter, but other than that there is
no hardware entity. Not even a firmware entity to drive this thing.
> >>> +
> >>> + 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.
>
> You mean build dependencies, not runtime load.
No, I mean runtime load dependencies.
>
> > Remoteproc module depends on this symbol. Once q6v5 remoteproc modules
> > are unloaded this module can be unloaded too.
>
> I am pretty sure you can unload this and get crashes.
If for some reason somebody has called qcom_pdm_get() without
qcom_pdm_release(), then yes. To make it 100% safe, I can cleanup
actions to module_exit(), but for me it looks like an overkill.
--
With best wishes
Dmitry
