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

[Krzysztof Kozlowski <krzk@kernel.org>]

On 19/04/2024 20:24, Dmitry Baryshkov wrote:
>>>>> +};
>>>>
>>>> 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.

Ah, right, I understand now. So this should not be loaded on its own on
the machine.

> 
>>
>>> 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.

Firmware interfaces are also not "devices" but we create device drivers
for them. The code lies in drivers, so it is a driver, even if somehow
kernel software construct. fs/pstore/ram also has a driver, even though
this is software device to handle ram dumps (it is not a driver for
RAM). net/qrtr/smd.c is not even in the drivers and as well describes
some sort of software daemon.

If this was not a driver, then it would be a subsystem... but it is not
a subsystem, right?

> 
>>>>> +
>>>>> +     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.

I'll come with some more concrete example if you are not convinced.

Best regards,
Krzysztof