Re: [PATCH 0/4] platform/x86: x86-android-tablets: use real firmware node references with intel drivers

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

On Fri, Mar 20, 2026 at 01:33:06PM -0700, Bartosz Golaszewski wrote:
> On Fri, 20 Mar 2026 02:49:02 +0100, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> said:
> > Hi Bartosz,
> >
> > On Thu, Mar 19, 2026 at 05:10:53PM +0100, Bartosz Golaszewski wrote:
> >>
> >> This series proposes a solution in the form of automatic secondary
> >> software node assignment (I'm open to better naming ideas). We extend
> >> the swnode API with functions allowing to set up a behind-the-scenes bus
> >> notifier for a group of named software nodes. It will wait for bus
> >> events and when a device is added, it will check its name against the
> >> software node's name and - on match - assign the software node as the
> >> secondary firmware node of the device's *real* firmware node.
> >
> > The more I think about the current approaches with strict identity
> > matching the less I like them, and the reason is that strict identity
> > matching establishes rigid links between consumers and producers of
> > GPIOS/swnodes, and puts us into link order hell. For example, I believe
> > if andoird tablets drivers were in drivers/android vs
> > drivers/platform/... the current scheme would break since the nodes
> > would not be registered and GPIO lookups would fail with -ENOENT vs
> > -EPROBE_DEFER.
> >
> 
> Why would they not get registered? They get attached when the target devices
> are added in modules this module depends on. They are exported symbols so the
> prerequisite modules will get loaded before and the module's init function
> will have run by the time the software nodes are referred to by the fwnode
> interface at which point they will have been registered with the swnode
> framework.

I mentioned link order, which implies no modules are involved. When code
is built into the kernel initialization follows link order, which is
typically alphabetical. To ensure the order you require you either need
to move targets inside makefiles or change some drivers from
module_init() to <some other>_initcall(). This is known as "link order
hell" that was very common before deferred probing was introduced.

> 
> > Given that this series somewhat re-introduces the name matching, I
> > wonder if we can not do something like the following (the rough draft):
> >
> 
> I'm open to better ideas and possibly multiple matching mechanisms but this
> just fit in this particular case. I'm not overly attached to name matching. We
> may as well use whatever properties ACPI provides to identify the devices and
> assign them their swnodes.

What ACPI has to do with this? Oftentimes we are dealing with non x86
systems.

> 
> > diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
> > index 51320837f3a9..b0e8923a092c 100644
> > --- a/drivers/base/swnode.c
> > +++ b/drivers/base/swnode.c
> > @@ -509,6 +509,7 @@ software_node_get_reference_args(const struct fwnode_handle *fwnode,
> >  	struct swnode *swnode = to_swnode(fwnode);
> >  	const struct software_node_ref_args *ref_array;
> >  	const struct software_node_ref_args *ref;
> > +	const struct software_node *ref_swnode;
> >  	const struct property_entry *prop;
> >  	struct fwnode_handle *refnode;
> >  	u32 nargs_prop_val;
> > @@ -550,7 +551,10 @@ software_node_get_reference_args(const struct fwnode_handle *fwnode,
> >  		refnode = software_node_fwnode(ref->swnode);
> >  	else if (ref->fwnode)
> >  		refnode = ref->fwnode;
> > -	else
> > +	else if (ref->swnode_name) {
> > +		ref_swnode = software_node_find_by_name(NULL, ref->swnode_name);
> > +		refnode = ref_swnode ? software_node_fwnode(ref_swnode) : NULL;
> > +	} else
> >  		return -EINVAL;
> >
> >  	if (!refnode)
> > diff --git a/include/linux/property.h b/include/linux/property.h
> > index e30ef23a9af3..44e96ee47272 100644
> > --- a/include/linux/property.h
> > +++ b/include/linux/property.h
> > @@ -363,6 +363,7 @@ struct software_node;
> >  struct software_node_ref_args {
> >  	const struct software_node *swnode;
> >  	struct fwnode_handle *fwnode;
> > +	const char *swnode_name;
> >  	unsigned int nargs;
> >  	u64 args[NR_FWNODE_REFERENCE_ARGS];
> >  };
> > @@ -373,6 +374,9 @@ struct software_node_ref_args {
> >  			   const struct software_node *: _ref_,	\
> >  			   struct software_node *: _ref_,	\
> >  			   default: NULL),			\
> > +	.swnode_name = _Generic(_ref_,				\
> > +				const char *: _ref_,		\
> > +				default: NULL),			\
> >  	.fwnode = _Generic(_ref_,				\
> >  			   struct fwnode_handle *: _ref_,	\
> >  			   default: NULL),			\
> >
> > This will allow consumers specify top-level software node name instead
> > of software node structure, and it will get resolved to concrete
> > firmware node. GPIOLIB can continue matching on node identity.
> >
> > WDYT?
> >
> 
> I think it's bad design and even bigger abuse of the software node concept.
> What you're proposing is effectively allowing to use struct software_node as
> a misleading string wrapper. You wouldn't use it to pass any properties to
> the device you're pointing to - because how if there's no link between them -
> you would just store an arbitrary string in a structure that serves
> a completely different purpose.

I think you completely misunderstood the proposal. We are not using
software node as a string wrapper, we give an opportunity to use
software node name to resolve to the real software node at the time we
try to resolve the reference. The software node is still expected to be
bound to the target device (unlike the original approach that has a
dangling software node which name expected to match gpiochip label).

I think this actually a very good approach: it severs the tight coupling
while still maintains the spirit of firmware nodes being attached to
devices. The only difference we are using object's name and not its
address as starting point. Similarly how you use name derived from
device name to locate and assign secondary node in this patch series.

> 
> Which is BTW exactly what was done in GPIO and - while there's no denying that
> I signed-off on these patches - it goes to show just how misleading this design
> is - I was aware of this development and queued the patches but only really
> understood what was going on when it was too late and this pattern was
> copy-pasted all over the kernel.
> 
> Software nodes are just an implementation of firmware nodes and as such should
> follow the same principles. If a software node describes a device, it should be
> attached to it

Yes, and the patch above solves this.

> so that references can be resolved by checking the address of
> the underlying firmware node handle and not by string matching. I will die on
> that hill. :)

I would like to understand why. I outlined the problems with this
approach (too tight coupling, needing to export nodes, include
additional headers, and deal with the link order issues, along with
potential changes to the system initialization/probe order). What are
the drawbacks of name matching as long as we do not create
dangling/shadow nodes?

You are saying that you want resolve strictly by address, but have you
looked at how OF references are resolved? Hint: phandle is not a raw
address. It's actually a 32 bit integer! Look at ACPI. It also does not
simply have a pointer to another ACPI node there, the data structure is
much more complex.

So why are you trying to make software nodes different from all the
other firmware nodes?

> 
> If you want to match string, use GPIO lookup tables. I remember your point about
> wanting to use PROPERTY_ENTRY_REF() for consistency and fully support it, but
> please do use *references* because otherwise it's just a lookup table with extra
> steps.
> 
> Just think about it: what if we try to generalize fw_devlink to software nodes?
> It would be completely broken for the offending GPIO users because there's no
> real link between the software nodes supposedly pointing to the GPIO
> controllers and the controllers themselves.

There is, you just misunderstood the proposal I'm afraid.

Thanks.

-- 
Dmitry