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
