Hi Vladimir,
On Thu, Jul 21, 2022 at 01:56:52AM +0300, Vladimir Oltean wrote:
> Hi Sakari,
>
> On Tue, Jul 19, 2022 at 08:50:27AM +0000, Sakari Ailus wrote:
> > Basically what your patch is doing is adding a helper function that creates
> > an fwnode with a given name. This functionality was there previously through
> > software_node_register_nodes(), with node allocation responsibility residing
> > on the caller. It's used e.g. here:
> > drivers/media/pci/intel/ipu3/cio2-bridge.c .
> >
> > The larger question is perhaps when can you safely remove software nodes.
> > And which of these two APIs would be preferred. I haven't checked how many
> > users each has. There's no refcounting nor locking for software nodes, so
> > once made visible to the rest of the kernel, they're always expected to be
> > there, unchanged, or at least it needs to be known when they can be removed.
>
> Just for my clarity, are you saying that this printf selftest is
> violating the software nodes' expectation to always be there unchanged
> and never be removed?
No. This is the other case, i.e. it's known the nodes can be removed.
>
> static void __init fwnode_pointer(void)
> {
> const struct software_node softnodes[] = {
> { .name = "first", },
> { .name = "second", .parent = &softnodes[0], },
> { .name = "third", .parent = &softnodes[1], },
> { NULL /* Guardian */ }
> };
> const char * const full_name = "first/second/third";
> const char * const full_name_second = "first/second";
> const char * const second_name = "second";
> const char * const third_name = "third";
> int rval;
>
> rval = software_node_register_nodes(softnodes);
> if (rval) {
> pr_warn("cannot register softnodes; rval %d\n", rval);
> return;
> }
>
> test(full_name_second, "%pfw", software_node_fwnode(&softnodes[1]));
> test(full_name, "%pfw", software_node_fwnode(&softnodes[2]));
> test(full_name, "%pfwf", software_node_fwnode(&softnodes[2]));
> test(second_name, "%pfwP", software_node_fwnode(&softnodes[1]));
> test(third_name, "%pfwP", software_node_fwnode(&softnodes[2]));
>
> software_node_unregister_nodes(softnodes);
> }
>
> The use case in this patch set is essentially equivalent to what printf
> does: exposing the software nodes to the rest of the kernel and to user
> space is probably not necessary, it's just that we need to call a
> function that parses their structure (essentially an equivalent to
> calling "test" above). Could you indicate whether there is a better
> alternative of doing this?
I'm actually not suggesting to do otherwise. What I wanted to say was that
it'd be best to settle with a single API to create software nodes while
keeping in mind serialising access to the data structure as well as
the lifetime of the software nodes.
This patch is adding another API function to register software nodes which
expands the scope of another that effectively did not allow sub-nodes.
Lifetime management currently doesn't really exist for ACPI nodes (device
or data) and only exists in somewhat unsatisfactory form for DT nodes. That
might be still the best model for software nodes.
Perhaps the API this patch adds is nicer to use than
software_node_register_nodes() and better lends itself for adding
refcounting later on.
I wonder what Andy or Heikki think.
--
Kind regards,
Sakari Ailus
