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:

quoted

On Mon, Jul 18, 2022 at 02:27:02PM +0100, Russell King (Oracle) wrote:

quoted

On Mon, Jul 18, 2022 at 03:29:52PM +0300, Andy Shevchenko wrote:

quoted

On Fri, Jul 15, 2022 at 11:48:41PM +0300, Vladimir Oltean wrote:

quoted

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.

quoted

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.

quoted

quoted

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