| 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
| ||
|
Date: Fri, 14 Jul 2023 10:15:27 +0800
From: Feiyang Chen <chris.chenfeiyang@...il.com>
To: Andrew Lunn <andrew@...n.ch>
Cc: Feiyang Chen <chenfeiyang@...ngson.cn>, hkallweit1@...il.com, peppe.cavallaro@...com,
alexandre.torgue@...s.st.com, joabreu@...opsys.com, chenhuacai@...ngson.cn,
linux@...linux.org.uk, dongbiao@...ngson.cn,
loongson-kernel@...ts.loongnix.cn, netdev@...r.kernel.org,
loongarch@...ts.linux.dev
Subject: Re: [RFC PATCH 01/10] net: phy: Add driver for Loongson PHY
On Thu, Jul 13, 2023 at 12:00 PM Andrew Lunn <andrew@...n.ch> wrote:
>
> On Thu, Jul 13, 2023 at 10:46:53AM +0800, Feiyang Chen wrote:
> > Add support for the Loongson PHY.
> >
> > Signed-off-by: Feiyang Chen <chenfeiyang@...ngson.cn>
> > ---
> > drivers/net/phy/Kconfig | 5 +++
> > drivers/net/phy/Makefile | 1 +
> > drivers/net/phy/loongson-phy.c | 69 ++++++++++++++++++++++++++++++++++
>
> Please drop -phy from the filename. No other phy driver does this.
>
Hi, Andrew,
OK.
> > drivers/net/phy/phy_device.c | 16 ++++++++
> > include/linux/phy.h | 2 +
> > 5 files changed, 93 insertions(+)
> > create mode 100644 drivers/net/phy/loongson-phy.c
> >
> > diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> > index 93b8efc79227..4f8ea32cbc68 100644
> > --- a/drivers/net/phy/Kconfig
> > +++ b/drivers/net/phy/Kconfig
> > @@ -202,6 +202,11 @@ config INTEL_XWAY_PHY
> > PEF 7061, PEF 7071 and PEF 7072 or integrated into the Intel
> > SoCs xRX200, xRX300, xRX330, xRX350 and xRX550.
> >
> > +config LOONGSON_PHY
> > + tristate "Loongson PHY driver"
> > + help
> > + Supports the Loongson PHY.
> > +
> > config LSI_ET1011C_PHY
> > tristate "LSI ET1011C PHY"
> > help
> > diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
> > index f289ab16a1da..f775373e12b7 100644
> > --- a/drivers/net/phy/Makefile
> > +++ b/drivers/net/phy/Makefile
> > @@ -62,6 +62,7 @@ obj-$(CONFIG_DP83TD510_PHY) += dp83td510.o
> > obj-$(CONFIG_FIXED_PHY) += fixed_phy.o
> > obj-$(CONFIG_ICPLUS_PHY) += icplus.o
> > obj-$(CONFIG_INTEL_XWAY_PHY) += intel-xway.o
> > +obj-$(CONFIG_LOONGSON_PHY) += loongson-phy.o
> > obj-$(CONFIG_LSI_ET1011C_PHY) += et1011c.o
> > obj-$(CONFIG_LXT_PHY) += lxt.o
> > obj-$(CONFIG_MARVELL_10G_PHY) += marvell10g.o
> > diff --git a/drivers/net/phy/loongson-phy.c b/drivers/net/phy/loongson-phy.c
> > new file mode 100644
> > index 000000000000..d4aefa2110f8
> > --- /dev/null
> > +++ b/drivers/net/phy/loongson-phy.c
> > @@ -0,0 +1,69 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * LS7A PHY driver
> > + *
> > + * Copyright (C) 2020-2023 Loongson Technology Corporation Limited
> > + *
> > + * Author: Zhang Baoqi <zhangbaoqi@...ngson.cn>
> > + */
> > +
> > +#include <linux/mii.h>
> > +#include <linux/module.h>
> > +#include <linux/netdevice.h>
> > +#include <linux/pci.h>
> > +#include <linux/phy.h>
> > +
> > +#define PHY_ID_LS7A2000 0x00061ce0
>
> What is Loongson OUI?
>
Currently, we do not have an OUI for Loongson, but we are in the
process of applying for one.
> > +#define GNET_REV_LS7A2000 0x00
> > +
> > +static int ls7a2000_config_aneg(struct phy_device *phydev)
> > +{
> > + if (phydev->speed == SPEED_1000)
> > + phydev->autoneg = AUTONEG_ENABLE;
>
> Please explain.
>
The PHY itself supports half-duplex, but there are issues with the
controller used in the 7A2000 chip. Moreover, the controller only
supports auto-negotiation for gigabit speeds.
> > +
> > + if (linkmode_test_bit(ETHTOOL_LINK_MODE_10baseT_Full_BIT,
> > + phydev->advertising) ||
> > + linkmode_test_bit(ETHTOOL_LINK_MODE_100baseT_Full_BIT,
> > + phydev->advertising) ||
> > + linkmode_test_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT,
> > + phydev->advertising))
> > + return genphy_config_aneg(phydev);
> > +
> > + netdev_info(phydev->attached_dev, "Parameter Setting Error\n");
>
> This also needs explaining. How can it be asked to do something it
> does not support?
>
Perhaps I missed something, but I think that if someone uses ethtool,
they can request it to perform actions or configurations that the
tool does not support.
> > + return -1;
>
> Always use error codes. In this case EINVAL.
>
OK.
> > +}
> > +
> > +int ls7a2000_match_phy_device(struct phy_device *phydev)
> > +{
> > + struct net_device *ndev;
> > + struct pci_dev *pdev;
> > +
> > + if ((phydev->phy_id & 0xfffffff0) != PHY_ID_LS7A2000)
> > + return 0;
> > +
> > + ndev = phydev->mdio.bus->priv;
> > + pdev = to_pci_dev(ndev->dev.parent);
> > +
> > + return pdev->revision == GNET_REV_LS7A2000;
>
> That is very unusual. Why is the PHY ID not sufficient?
>
To work around the controller's issues, we enable the usage of this
driver specifically for a certain version of the 7A2000 chip.
> > +}
> > +
> > +static struct phy_driver loongson_phy_driver[] = {
> > + {
> > + PHY_ID_MATCH_MODEL(PHY_ID_LS7A2000),
> > + .name = "LS7A2000 PHY",
> > + .features = PHY_LOONGSON_FEATURES,
>
> So what are the capabilities of this PHY? You seem to have some very
> odd hacks here, and no explanation of why they are needed. If you do
> something which no other device does, you need to explain it.
>
> Does the PHY itself only support full duplex? No half duplex? Does the
> PHY support autoneg? Does it support fixed settings? What does
> genphy_read_abilities() return for this PHY?
>
As mentioned earlier, this driver is specifically designed for the PHY
on the problematic 7A2000 chip. Therefore, we assume that this PHY only
supports full- duplex mode and performs auto-negotiation exclusively for
gigabit speeds.
Thanks,
Feiyang
> Andrew
Powered by blists - more mailing lists