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?
> 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?
...
>>> +
>>> + 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.
> 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.
Best regards,
Krzysztof
