netdev - Re: [PATCH RFC net-next 6/7] net: dsa: mv88e6xxx: provide software node for default settings

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 24 Mar 2023 16:49:32 +0200
From:   Heikki Krogerus <heikki.krogerus@...ux.intel.com>
To:     "Russell King (Oracle)" <rmk+kernel@...linux.org.uk>
Cc:     Andrew Lunn <andrew@...n.ch>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Daniel Scally <djrscally@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jakub Kicinski <kuba@...nel.org>, linux-acpi@...r.kernel.org,
        netdev@...r.kernel.org, Paolo Abeni <pabeni@...hat.com>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Sakari Ailus <sakari.ailus@...ux.intel.com>,
        Vladimir Oltean <olteanv@...il.com>
Subject: Re: [PATCH RFC net-next 6/7] net: dsa: mv88e6xxx: provide software
 node for default settings

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.

Here's an example how you can add the nodes with the already existing
APIs. To keep this example simple, I'm expecting that you have members
for the software nodes to the struct mv88e6xxx_chip:

struct mv88e6xxx_chip {
        ...
        /* swnodes */
        struct software_node port_swnode;
        struct software_node fixed_link_swnode;
};

Of course, you don't have to add those members if you don't want to.
Then you just need to allocate the nodes separately, but that should
not be a problem. In any case, something like this:

static struct fwnode_handle *mv88e6xxx_create_port_swnode(struct mv88e6xxx_chip *chip,
                                                          phy_interface_t mode,
							  int speed,
							  int duplex)
{
	struct property_entry fixed_link_props[3] = { };
	struct property_entry port_props[2] = { };
	int ret;

        /*
         * First register the port node.
         */
	port_props[0] = PROPERTY_ENTRY_STRING("phy-mode", phy_modes(mode));

	chip->port_swnode.properties = property_entries_dup(port_props);
        if (IS_ERR(chip->port_swnode.properties))
                return ERR_CAST(chip->port_swnode.properties);

	ret = software_node_register(&chip->port_swnode);
	if (ret) {
                kfree(chip->port_swnode.properties);
		return ERR_PTR(ret);
        }

        /*
         * Then the second node, child of the port node.
         */
	fixed_link_props[0] = PROPERTY_ENTRY_U32("speed", speed);
	if (duplex == DUPLEX_FULL)
		fixed_link_props[1] = PROPERTY_ENTRY_BOOL("full-duplex");

        chip->fixed_link_swnode.name = "fixed-link";
        chip->fixed_link_swnode.parent = &chip->port_swnode;
	chip->fixed_link_swnode.properties = property_entries_dup(fixed_link_props);
        if (IS_ERR(chip->port_swnode.properties)) {
                software_node_unregister(&chip->port_swnode);
                kfree(chip->port_swnode.properties);
                return ERR_CAST(chip->fixed_link_swnode.properties);
        }

	ret = software_node_register(&chip->fixed_link_swnode);
        if (ret) {
                software_node_unregister(&chip->port_swnode);
                kfree(chip->port_swnode.properties);
                kfree(chip->fixed_link_swnode.properties);
		return ERR_PTR(ret);
        }

        /*
         * Finally, return the port fwnode.
         */
        return software_node_fwnode(&chip->port_swnode);
}

thanks,

-- 
heikki