| 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
| ||
|
Message-ID: <fa4fca9c-5950-8ec1-5746-91e51085cecf@gmail.com>
Date: Tue, 4 Sep 2018 17:27:54 -0700
From: Florian Fainelli <f.fainelli@...il.com>
To: Moritz Fischer <mdf@...nel.org>, netdev@...r.kernel.org
Cc: davem@...emloft.net, andrew@...n.ch, alex.williams@...com,
moritz.fischer@...us.com, linux-kernel@...r.kernel.org
Subject: Re: [RFC/PATCH] net: nixge: Add PHYLINK support
On 09/04/2018 05:15 PM, Moritz Fischer wrote:
> Add basic PHYLINK support to driver.
>
> Suggested-by: Andrew Lunn <andrew@...n.ch>
> Signed-off-by: Moritz Fischer <mdf@...nel.org>
> ---
>
> Hi all,
>
> as Andrew suggested in order to enable SFP as
> well as fixed-link support add PHYLINK support.
>
> A couple of questions are still open (hence the RFC):
>
> 1) It seems odd to implement PHYLINK callbacks that
> are all empty? If so, should we have generic empty
> ones in drivers/net/phy/phylink.c like we have for
> genphys?
Yes it is odd, the validate callback most certainly should not be empty,
neither should the mac_config and mac_link_{up,down}, but, with some
luck, you can get things to just work, typically with MDIO PHYs, since a
large amount of what they can do is discoverable.
If you had an existing adjust_link callback from PHYLIB, it's really
about breaking it down such that the MAC configuration of
speed/duplex/pause happens in mac_config, and the link setting (if
necessary), happens in mac_link_{up,down}, and that's pretty much it for
MLO_AN_PHY cases.
>
> 2) If this is ok, then I'll go ahead rework this with
> a DT binding update to deprecate the non-'mdio'-subnode
> case (since there are no in-tree users we might just
> change the binding)?
>
> 3) I'm again not sure about the 'select PHYLINK', wouldn't
> wanna break the build again...
>
> Thanks again for your time!
>
> Moritz
>
> ---
> drivers/net/ethernet/ni/Kconfig | 1 +
> drivers/net/ethernet/ni/nixge.c | 115 +++++++++++++++++++++++---------
> 2 files changed, 83 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/net/ethernet/ni/Kconfig b/drivers/net/ethernet/ni/Kconfig
> index c73978474c4b..80cd72948551 100644
> --- a/drivers/net/ethernet/ni/Kconfig
> +++ b/drivers/net/ethernet/ni/Kconfig
> @@ -21,6 +21,7 @@ config NI_XGE_MANAGEMENT_ENET
> depends on HAS_IOMEM && HAS_DMA
> select PHYLIB
> select OF_MDIO if OF
> + select PHYLINK
> help
> Simple LAN device for debug or management purposes. Can
> support either 10G or 1G PHYs via SFP+ ports.
> diff --git a/drivers/net/ethernet/ni/nixge.c b/drivers/net/ethernet/ni/nixge.c
> index 74cf52e3fb09..a0e790d07b1c 100644
> --- a/drivers/net/ethernet/ni/nixge.c
> +++ b/drivers/net/ethernet/ni/nixge.c
> @@ -11,6 +11,7 @@
> #include <linux/of_mdio.h>
> #include <linux/of_net.h>
> #include <linux/of_platform.h>
> +#include <linux/phylink.h>
> #include <linux/of_irq.h>
> #include <linux/skbuff.h>
> #include <linux/phy.h>
> @@ -165,7 +166,7 @@ struct nixge_priv {
> struct device *dev;
>
> /* Connection to PHY device */
> - struct device_node *phy_node;
> + struct phylink *phylink;
> phy_interface_t phy_mode;
>
> int link;
> @@ -416,20 +417,6 @@ static void nixge_device_reset(struct net_device *ndev)
> netif_trans_update(ndev);
> }
>
> -static void nixge_handle_link_change(struct net_device *ndev)
> -{
> - struct nixge_priv *priv = netdev_priv(ndev);
> - struct phy_device *phydev = ndev->phydev;
> -
> - if (phydev->link != priv->link || phydev->speed != priv->speed ||
> - phydev->duplex != priv->duplex) {
> - priv->link = phydev->link;
> - priv->speed = phydev->speed;
> - priv->duplex = phydev->duplex;
> - phy_print_status(phydev);
> - }
> -}
> -
> static void nixge_tx_skb_unmap(struct nixge_priv *priv,
> struct nixge_tx_skb *tx_skb)
> {
> @@ -859,17 +846,15 @@ static void nixge_dma_err_handler(unsigned long data)
> static int nixge_open(struct net_device *ndev)
> {
> struct nixge_priv *priv = netdev_priv(ndev);
> - struct phy_device *phy;
> int ret;
>
> nixge_device_reset(ndev);
>
> - phy = of_phy_connect(ndev, priv->phy_node,
> - &nixge_handle_link_change, 0, priv->phy_mode);
> - if (!phy)
> - return -ENODEV;
> + ret = phylink_of_phy_connect(priv->phylink, priv->dev->of_node, 0);
> + if (ret < 0)
> + return ret;
>
> - phy_start(phy);
> + phylink_start(priv->phylink);
>
> /* Enable tasklets for Axi DMA error handling */
> tasklet_init(&priv->dma_err_tasklet, nixge_dma_err_handler,
> @@ -893,8 +878,7 @@ static int nixge_open(struct net_device *ndev)
> err_rx_irq:
> free_irq(priv->tx_irq, ndev);
> err_tx_irq:
> - phy_stop(phy);
> - phy_disconnect(phy);
> + phylink_disconnect_phy(priv->phylink);
> tasklet_kill(&priv->dma_err_tasklet);
> netdev_err(ndev, "request_irq() failed\n");
> return ret;
> @@ -908,9 +892,9 @@ static int nixge_stop(struct net_device *ndev)
> netif_stop_queue(ndev);
> napi_disable(&priv->napi);
>
> - if (ndev->phydev) {
> - phy_stop(ndev->phydev);
> - phy_disconnect(ndev->phydev);
> + if (priv->phylink) {
> + phylink_stop(priv->phylink);
> + phylink_disconnect_phy(priv->phylink);
> }
>
> cr = nixge_dma_read_reg(priv, XAXIDMA_RX_CR_OFFSET);
> @@ -1076,13 +1060,31 @@ static int nixge_ethtools_set_phys_id(struct net_device *ndev,
> return 0;
> }
>
> +static int
> +nixge_ethtool_set_link_ksettings(struct net_device *ndev,
> + const struct ethtool_link_ksettings *cmd)
> +{
> + struct nixge_priv *priv = netdev_priv(ndev);
> +
> + return phylink_ethtool_ksettings_set(priv->phylink, cmd);
> +}
> +
> +static int
> +nixge_ethtool_get_link_ksettings(struct net_device *ndev,
> + struct ethtool_link_ksettings *cmd)
> +{
> + struct nixge_priv *priv = netdev_priv(ndev);
> +
> + return phylink_ethtool_ksettings_get(priv->phylink, cmd);
> +}
> +
> static const struct ethtool_ops nixge_ethtool_ops = {
> .get_drvinfo = nixge_ethtools_get_drvinfo,
> .get_coalesce = nixge_ethtools_get_coalesce,
> .set_coalesce = nixge_ethtools_set_coalesce,
> .set_phys_id = nixge_ethtools_set_phys_id,
> - .get_link_ksettings = phy_ethtool_get_link_ksettings,
> - .set_link_ksettings = phy_ethtool_set_link_ksettings,
> + .get_link_ksettings = nixge_ethtool_get_link_ksettings,
> + .set_link_ksettings = nixge_ethtool_set_link_ksettings,
> .get_link = ethtool_op_get_link,
> };
>
> @@ -1225,11 +1227,52 @@ static void *nixge_get_nvmem_address(struct device *dev)
> return mac;
> }
>
> +static void nixge_validate(struct net_device *ndev, unsigned long *supported,
> + struct phylink_link_state *state)
> +{
> +}
> +
> +static int nixge_mac_link_state(struct net_device *ndev,
> + struct phylink_link_state *state)
> +{
> + return 0;
> +}
> +
> +static void nixge_mac_config(struct net_device *ndev, unsigned int mode,
> + const struct phylink_link_state *state)
> +{
> +}
> +
> +static void nixge_mac_an_restart(struct net_device *ndev)
> +{
> +}
> +
> +static void nixge_mac_link_down(struct net_device *ndev, unsigned int mode,
> + phy_interface_t interface)
> +{
> +}
> +
> +static void nixge_mac_link_up(struct net_device *ndev, unsigned int mode,
> + phy_interface_t interface,
> + struct phy_device *phy)
> +{
> +}
> +
> +static const struct phylink_mac_ops nixge_phylink_ops = {
> + .validate = nixge_validate,
> + .mac_link_state = nixge_mac_link_state,
> + .mac_an_restart = nixge_mac_an_restart,
> + .mac_config = nixge_mac_config,
> + .mac_link_down = nixge_mac_link_down,
> + .mac_link_up = nixge_mac_link_up,
> +};
> +
> static int nixge_probe(struct platform_device *pdev)
> {
> struct nixge_priv *priv;
> struct net_device *ndev;
> struct resource *dmares;
> + struct device_node *mn;
> const u8 *mac_addr;
> int err;
>
> @@ -1286,7 +1329,13 @@ static int nixge_probe(struct platform_device *pdev)
> priv->coalesce_count_rx = XAXIDMA_DFT_RX_THRESHOLD;
> priv->coalesce_count_tx = XAXIDMA_DFT_TX_THRESHOLD;
>
> - err = nixge_mdio_setup(priv, pdev->dev.of_node);
> + mn = of_get_child_by_name(pdev->dev.of_node, "mdio");
> + if (!mn) {
> + dev_warn(&pdev->dev, "No \"mdio\" subnode found, defaulting to legacy\n");
> + mn = pdev->dev.of_node;
> + }
> +
> + err = nixge_mdio_setup(priv, mn);
> if (err) {
> netdev_err(ndev, "error registering mdio bus");
> goto free_netdev;
> @@ -1299,10 +1348,10 @@ static int nixge_probe(struct platform_device *pdev)
> goto unregister_mdio;
> }
>
> - priv->phy_node = of_parse_phandle(pdev->dev.of_node, "phy-handle", 0);
> - if (!priv->phy_node) {
> - netdev_err(ndev, "not find \"phy-handle\" property\n");
> - err = -EINVAL;
> + priv->phylink = phylink_create(ndev, pdev->dev.fwnode, priv->phy_mode,
> + &nixge_phylink_ops);
> + if (IS_ERR(priv->phylink)) {
> + err = PTR_ERR(priv->phylink);
> goto unregister_mdio;
> }
>
>
--
Florian
Powered by blists - more mailing lists