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
