netdev - Re: [PATCH net-next v2] net: phy: xilinx: add Xilinx PHY driver

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: <d8c22bc3-0a20-2c24-88bb-b1f5f8cc907a@gmail.com>
Date:   Tue, 4 Jun 2019 07:37:43 +0200
From:   Heiner Kallweit <hkallweit1@...il.com>
To:     Robert Hancock <hancock@...systems.ca>, netdev@...r.kernel.org,
        Florian Fainelli <f.fainelli@...il.com>,
        Andrew Lunn <andrew@...n.ch>
Subject: Re: [PATCH net-next v2] net: phy: xilinx: add Xilinx PHY driver

On 04.06.2019 01:12, Robert Hancock wrote:
> This adds a driver for the PHY device implemented in the Xilinx PCS/PMA
> Core logic. This is mostly a generic gigabit PHY, except that the
> features are explicitly set because the PHY wrongly indicates it has no
> extended status register when it actually does.
> 
> This version is a simplified version of the GPL 2+ version from the
> Xilinx kernel tree.
> 
> Signed-off-by: Robert Hancock <hancock@...systems.ca>
> ---
> 
> Differences from v1:
> -Removed unnecessary config_init method
> -Added comment to explain why features are explicitly set
> 
>  drivers/net/phy/Kconfig  |  6 ++++++
>  drivers/net/phy/Makefile |  1 +
>  drivers/net/phy/xilinx.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 58 insertions(+)
>  create mode 100644 drivers/net/phy/xilinx.c
> 
> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> index db5645b..101c794 100644
> --- a/drivers/net/phy/Kconfig
> +++ b/drivers/net/phy/Kconfig
> @@ -462,6 +462,12 @@ config VITESSE_PHY
>  	---help---
>  	  Currently supports the vsc8244
>  
> +config XILINX_PHY
> +	tristate "Drivers for Xilinx PHYs"
> +	help
> +	  This module provides a driver for the PHY implemented in the
> +	  Xilinx PCS/PMA Core.
> +
>  config XILINX_GMII2RGMII
>  	tristate "Xilinx GMII2RGMII converter driver"
>  	---help---
> diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
> index bac339e..3ee9cdb 100644
> --- a/drivers/net/phy/Makefile
> +++ b/drivers/net/phy/Makefile
> @@ -91,4 +91,5 @@ obj-$(CONFIG_SMSC_PHY)		+= smsc.o
>  obj-$(CONFIG_STE10XP)		+= ste10Xp.o
>  obj-$(CONFIG_TERANETICS_PHY)	+= teranetics.o
>  obj-$(CONFIG_VITESSE_PHY)	+= vitesse.o
> +obj-$(CONFIG_XILINX_PHY)	+= xilinx.o
>  obj-$(CONFIG_XILINX_GMII2RGMII) += xilinx_gmii2rgmii.o
> diff --git a/drivers/net/phy/xilinx.c b/drivers/net/phy/xilinx.c
> new file mode 100644
> index 0000000..0e5509b
> --- /dev/null
> +++ b/drivers/net/phy/xilinx.c
> @@ -0,0 +1,51 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/* Xilinx PCS/PMA Core phy driver
> + *
> + * Copyright (C) 2019 SED Systems, a division of Calian Ltd.
> + *
> + * Based upon Xilinx version of this driver:
> + * Copyright (C) 2015 Xilinx, Inc.
> + *
> + * Description:
> + * This driver is developed for PCS/PMA Core.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mii.h>
> +#include <linux/phy.h>
> +
> +/* Mask used for ID comparisons */
> +#define XILINX_PHY_ID_MASK		0xfffffff0
> +
> +/* Known PHY IDs */
> +#define XILINX_PHY_ID			0x01740c00
> +
> +static struct phy_driver xilinx_drivers[] = {
> +{
> +	.phy_id		= XILINX_PHY_ID,
> +	.phy_id_mask	= XILINX_PHY_ID_MASK,
> +	.name		= "Xilinx PCS/PMA PHY",

Please no slash in the name. This breaks sysfs.

> +	/* Xilinx PHY wrongly indicates BMSR_ESTATEN = 0 even though
> +	 * extended status registers are supported. So we force the PHY
> +	 * features to PHY_GBIT_FEATURES in order to allow gigabit support
> +	 * to be detected.
> +	 */
> +	.features	= PHY_GBIT_FEATURES,

BMSR_ESTATEN is used by genphy_config_advert() too. Means you would
need to implement your own config_aneg callback and basically
copy 99% of genphy_config_advert().

I think the better alternative is to implement a quirk flag in phylib
similar to PHY_RST_AFTER_CLK_EN. Let me come up with a proposal.

Last but not least: Not setting BMSR_ESTATEN for a GBit PHY violates
the standard. Any intention from Xilinx to fix this?

> +	.resume		= genphy_resume,
> +	.suspend	= genphy_suspend,
> +	.set_loopback   = genphy_loopback,
> +},
> +};
> +
> +module_phy_driver(xilinx_drivers);
> +
> +static struct mdio_device_id __maybe_unused xilinx_tbl[] = {
> +	{ XILINX_PHY_ID, XILINX_PHY_ID_MASK },
> +	{ }
> +};
> +
> +MODULE_DEVICE_TABLE(mdio, xilinx_tbl);
> +MODULE_DESCRIPTION("Xilinx PCS/PMA PHY driver");
> +MODULE_LICENSE("GPL");
> +
>