On Fri, Mar 24, 2023 at 04:49:32PM +0200, Heikki Krogerus wrote:
> Hi Russell,
>
> On Wed, Mar 22, 2023 at 12:00:21PM +0000, Russell King (Oracle) wrote:
> > +static struct fwnode_handle *mv88e6xxx_create_fixed_swnode(struct fwnode_handle *parent,
> > + int speed,
> > + int duplex)
> > +{
> > + struct property_entry fixed_link_props[3] = { };
> > +
> > + fixed_link_props[0] = PROPERTY_ENTRY_U32("speed", speed);
> > + if (duplex == DUPLEX_FULL)
> > + fixed_link_props[1] = PROPERTY_ENTRY_BOOL("full-duplex");
> > +
> > + return fwnode_create_named_software_node(fixed_link_props, parent,
> > + "fixed-link");
> > +}
> > +
> > +static struct fwnode_handle *mv88e6xxx_create_port_swnode(phy_interface_t mode,
> > + int speed,
> > + int duplex)
> > +{
> > + struct property_entry port_props[2] = {};
> > + struct fwnode_handle *fixed_link_fwnode;
> > + struct fwnode_handle *new_port_fwnode;
> > +
> > + port_props[0] = PROPERTY_ENTRY_STRING("phy-mode", phy_modes(mode));
> > + new_port_fwnode = fwnode_create_software_node(port_props, NULL);
> > + if (IS_ERR(new_port_fwnode))
> > + return new_port_fwnode;
> > +
> > + fixed_link_fwnode = mv88e6xxx_create_fixed_swnode(new_port_fwnode,
> > + speed, duplex);
> > + if (IS_ERR(fixed_link_fwnode)) {
> > + fwnode_remove_software_node(new_port_fwnode);
> > + return fixed_link_fwnode;
> > + }
> > +
> > + return new_port_fwnode;
> > +}
>
> That new fwnode_create_named_software_node() function looks like a
> conflict waiting to happen - if a driver adds a node to the root level
> (does not have to be root level), all the tests will pass because
> there is only a single device, but when a user later tries the driver
> with two devices, it fails, because the node already exist. But you
> don't need that function at all.
I think you're totally failing to explain how this can fail.
Let me reiterate what thestructure of the swnodes here is:
root
`- node%d (%d allocated by root IDA)
+- phy-mode property
`- fixed-link
+- speed property
`- optional full-duplex property
If we have two different devices creating these nodes, then at the
root level, they will end up having different root names. The
"fixed-link" is a child of this node.
swnode already allows multiple identical names at the sub-node
level - each node ends up with its own IDA to allocate the generic
"node%d" names from. So as soon as we have multiple nodes, they
end up as this:
root
+- node0
| `- node 0
+- node1
| `- node 0
+- node2
| `- node 0
etc
So, if we end up with two devices creating these at the same time,
we end up with:
root
+- nodeA (A allocated by root IDA)
| +- phy-mode property
| `- fixed-link
| +- speed property
| `- optional full-duplex property
`- nodeB (B allocated by root IDA, different from above)
+- phy-mode property
`- fixed-link
+- speed property
`- optional full-duplex property
Since the kobject is parented to the parent's kobject, what we
end up with in sysfs is:
.../nodeA/fixed-link/speed
.../nodeB/fixed-link/speed
Thus, the "fixed-link" ndoes can _not_ conflict.
Please explain in detail where you think the conflict is, because
so far no one has been able to counter my assertions that this is
_safe_ with a proper full technical description of the problem.
All I get is hand-wavey "this conflicts".
Honestly, I'm getting sick of poor quality reviews... the next
poor review that claims there's a conflict here without properly
explain it will be told where to go.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
