netdev - Re: [PATCH RFC 11/26] phylink: add phylink infrastructure

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]

Message-ID: <568EC5FF.8030803@gmail.com>
Date:	Thu, 07 Jan 2016 12:09:35 -0800
From:	Florian Fainelli <f.fainelli@...il.com>
To:	Russell King <rmk+kernel@....linux.org.uk>,
	Thomas Petazzoni <thomas.petazzoni@...e-electrons.com>
CC:	netdev@...r.kernel.org
Subject: Re: [PATCH RFC 11/26] phylink: add phylink infrastructure

On 07/12/15 09:38, Russell King wrote:
[snip]
> +void phylink_phy_change(struct phy_device *phy, bool up, bool do_carrier)
> +{
> +	struct phylink *pl = phy->phylink;
> +
> +	mutex_lock(&pl->state_mutex);
> +	pl->phy_state.speed = phy->speed;
> +	pl->phy_state.duplex = phy->duplex;
> +	pl->phy_state.pause = MLO_PAUSE_NONE;
> +	if (phy->pause)
> +		pl->phy_state.pause |= MLO_PAUSE_SYM;
> +	if (phy->asym_pause)
> +		pl->phy_state.pause |= MLO_PAUSE_ASYM;
> +	pl->phy_state.link = up;
> +	mutex_unlock(&pl->state_mutex);
> +
> +	phylink_run_resolve(pl);
> +
> +	netdev_dbg(pl->netdev, "phy link %s\n", up ? "up" : "down");
> +}

Should this function be exported?

[snip]

> +
> +	phy_node = of_parse_phandle(dn, "phy-handle", 0);
> +	if (!phy_node)
> +		phy_node = of_parse_phandle(dn, "phy", 0);
> +	if (!phy_node)
> +		phy_node = of_parse_phandle(dn, "phy-device", 0);
> +
> +	if (!phy_node) {

This could be worth becoming a helper function that drivers could use,
as a subsequent patch for instance.


> +		if (pl->link_an_mode == MLO_AN_PHY) {
> +			netdev_err(pl->netdev, "unable to find PHY node\n");
> +			return -ENODEV;
> +		}
> +		return 0;
> +	}


> +
> +	phy_dev = of_phy_attach(pl->netdev, phy_node, 0, pl->link_interface);
> +	/* We're done with the phy_node handle */
> +	of_node_put(phy_node);
> +
> +	if (!phy_dev)
> +		return -ENODEV;
> +
> +	ret = phylink_bringup_phy(pl, phy_dev);
> +	if (ret)
> +		phy_detach(phy_dev);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(phylink_of_phy_connect);

Overall, this looks good to me, I am not very comfortable with the API
similarities between PHY link and traditional PHY devices, since that
forces PHY link to duplicate a bit of code (ioctl among other things).

Would it be better to have a generic PHY device have the option to be
overloaded with PHY link operations such that drivers do not have to
absorb API changes?
-- 
Florian