Re: [PATCH net-next 2/6] software node: allow named software node to be created

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Mon, Jul 18, 2022 at 08:11:40PM +0100, Russell King (Oracle) wrote:
> On Mon, Jul 18, 2022 at 09:43:41PM +0300, Andy Shevchenko wrote:
> > On Mon, Jul 18, 2022 at 02:27:02PM +0100, Russell King (Oracle) wrote:
> > > On Mon, Jul 18, 2022 at 03:29:52PM +0300, Andy Shevchenko wrote:
> > > > On Fri, Jul 15, 2022 at 11:48:41PM +0300, Vladimir Oltean wrote:
> > > > > So won't kobject_init_and_add() fail on namespace collision? Is it the
> > > > > problem that it's going to fail, or that it's not trivial to statically
> > > > > determine whether it'll fail?
> > > > > 
> > > > > Sorry, but I don't see something actionable about this.
> > > > 
> > > > I'm talking about validation before a runtime. But if you think that is fine,
> > > > let's fail it at runtime, okay, and consume more backtraces in the future.
> > > 
> > > Is there any sane way to do validation of this namespace before
> > > runtime?
> > 
> > For statically compiled, I think we can do it (to some extent).
> > Currently only three drivers, if I'm not mistaken, define software nodes with
> > names. It's easy to check that their node names are unique.
> > 
> > When you allow such an API then we might have tracebacks (from sysfs) bout name
> > collisions. Not that is something new to kernel (we have seen many of a kind),
> > but I prefer, if possible, to validate this before sysfs issues a traceback.
> > 
> > > The problem in this instance is we need a node named "fixed-link" that
> > > is attached to the parent node as that is defined in the binding doc,
> > > and we're creating swnodes to provide software generated nodes for
> > > this binding.
> > 
> > And how you guarantee that it will be only a single one with unique pathname?
> > 
> > For example, you have two DSA cards (or whatever it's called) in the SMP system,
> > it mean that there is non-zero probability of coexisting swnodes for them.
> 
> Good point - I guess we at least need to attach the swnode parent to the
> device so its path is unique, because right now that isn't the case. I'm
> guessing that:
> 
>         new_port_fwnode = fwnode_create_software_node(port_props, NULL);
> 
> will create something at the root of the swnode tree, and then:
> 
>         fixed_link_fwnode = fwnode_create_named_software_node(fixed_link_props,
>                                                               new_port_fwnode,
>                                                               "fixed-link");
> 
> will create a node with a fixed name. I guess it in part depends what
> pathname the first node gets (which we don't specify.) I'm not familiar
> with the swnode code to know what happens with the naming for the first
> node.

First node's name will be unique which is guaranteed by IDA framework. If we
have already 2B nodes, then yes, it would be problematic (but 2^31 ought to be
enough :-).

> However, it seems sensible to me to attach the first node to the device
> node, thus giving it a unique fwnode path. Does that solve the problem
> in swnode land?

Yes, but in the driver you will have that as child of the device, analogue in DT

  my_root_node { // equal the level of device node you attach it to
	  fixed-link {
	  }
  }

(Sorry, I don't know the DT syntax by heart, but I hope you got the idea.)

To access it will be something like

  child = fwnode_get_named_child_node(fwnode, "fixed-link");

And reading properties, if needed,

  ret = fnode_property_read_...(child, ...);


But this might require to adopt drivers, no? Or I misunderstand the hierarchy.

> > > There could be several such nodes scattered around, but in this
> > > instance they are very short-lived before they are destroyed, they
> > > don't even need to be published to userspace (and its probably a waste
> > > of CPU cycles for them to be published there.)
> > > 
> > > So, for this specific case, is this the best approach, or is there
> > > some better way to achieve what we need here?
> > 
> > Honestly, I don't know.
> > 
> > The "workaround" (but it looks to me rather a hack) is to create unique swnode
> > and make fixed-link as a child of it.
> > 
> > Or entire concept of the root swnodes (when name is provided) should be
> > reconsidered, so somehow we will have a uniqueness so that the entire
> > path(s) behind it will be caller-dependent. But this I also don't like.
> > 
> > Maybe Heikki, Sakari, Rafael can share their thoughts...
> > 
> > Just for my learning, why PHY uses "fixed-link" instead of relying on a
> > (firmware) graph? It might be the actual solution to your problem.
> 
> That's a question for Andrew, but I've tried to solicit his comments on
> several occasions concerning this "feature" of DSA but I keep getting
> no reply. Honestly, I don't know the answer to your question.
> 
> The only thing that I know is that Andrew has been promoting this
> feature where a switch port, whether it be connected to the CPU or
> to another switch, which doesn't specify any link parameters will
> automatically use the fastest "phy interface mode" and the fastest
> link speed that can be supported by the DSA device.
> 
> This has caused issues over the last few years which we've bodged
> around in various ways, and with updates to one of the DSA drivers
> this bodging is becoming more of a wart that's spreading. So, I'm
> trying to find a way to solve this.
> 
> My initial approach was to avoid fiddling with the firmware tree,
> but Vladimir proposed this approach as being cleaner - and it means
> the "bodge" becomes completely localised in the DSA (distributed
> switch architecture) code rather than being spread into phylink.
> 
> I wish we could get rid of this "feature" but since it's been
> established for many years, and we have at least one known driver
> that uses it, getting rid of it breaks existing firmware trees.
> I think we also have one other driver that makes use of it as
> well, but I can't say for certain (because it's not really possible
> to discern which drivers use this feature from reading the driver
> code.) I've tried asking Andrew if he knows and got no response.
> 
> So I'm in a complete information vacuum here - all that I know is
> that trying to convert the mv88e6xxx DSA driver to use phylink_pcs
> will break it (as reported by Marek Behún), because phylink doesn't
> get used if firmware is using this "defaulting" feature.
> 
> It's part of the DT binding, and remains so today - the properties
> specifying the "phy-mode", "fixed-link" etc all remain optional.

Okay, grepping the kernel I see this:

	dn = fwnode_get_named_child_node(fwnode, "fixed-link");

This seems the same what you need. I dunno why swnode should be created with
a name for this?

Eliminating an empty root node sounds plausible effect, but the consequences
are not 1:1 mapping of swnodes as it's designed for

  firmware device node		+=	unique root swnode
    property "X"		+=	property "Y"
    child "A"			+=	child "B"

Resulting firmware node as driver sees it:

	device node
		property "X"
		property "Y"
		child "A"
		child "B"

That's all said, I guess the way with a two swnodes (hierarhy) is the correct
one from the beginning.

To the API, now I can tell you how to validate!

Just be sure if there is no name provided, we are just fine. Otherwise
parent _swnode_ should be non-NULL. In such case parent can be only set
either dynamically _or_ statically assigned with a name.

-- 
With Best Regards,
Andy Shevchenko