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

[Krzysztof Kozlowski <krzk@kernel.org>]

On 19/04/2024 20:10, Dmitry Baryshkov wrote:
> On Fri, 19 Apr 2024 at 20:07, Krzysztof Kozlowski <krzk@kernel.org> 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