On Fri, 19 Apr 2024 at 21:45, Krzysztof Kozlowski <krzk@xxxxxxxxxx> wrote:
>
> 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?
It is a server. Or a service. Compare this to nfs-kernel-server or
historical things like khttpd.
>
> >
> >>>>> +
> >>>>> + 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.
Sure, I might easily be missing something.
--
With best wishes
Dmitry
