| 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: Sun, 4 Dec 2022 16:52:04 +0000
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Piergiorgio Beruto <piergiorgio.beruto@...il.com>
Cc: Andrew Lunn <andrew@...n.ch>,
Heiner Kallweit <hkallweit1@...il.com>,
"David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>, linux-kernel@...r.kernel.org,
netdev@...r.kernel.org, Oleksij Rempel <o.rempel@...gutronix.de>
Subject: Re: [PATCH net-next 3/4] drivers/net/phy: Add driver for the onsemi
NCN26000 10BASE-T1S PHY
Hi,
On Sun, Dec 04, 2022 at 03:31:33AM +0100, Piergiorgio Beruto wrote:
> diff --git a/drivers/net/phy/ncn26000.c b/drivers/net/phy/ncn26000.c
> new file mode 100644
> index 000000000000..65a34edc5b20
> --- /dev/null
> +++ b/drivers/net/phy/ncn26000.c
> @@ -0,0 +1,193 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause)
> +/*
> + * Driver for Analog Devices Industrial Ethernet T1L PHYs
> + *
> + * Copyright 2020 Analog Devices Inc.
> + */
> +#include <linux/kernel.h>
> +#include <linux/bitfield.h>
> +#include <linux/delay.h>
> +#include <linux/errno.h>
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/mii.h>
> +#include <linux/phy.h>
> +#include <linux/property.h>
> +
> +#define PHY_ID_NCN26000 0x180FF5A1
> +
> +#define NCN26000_REG_IRQ_CTL ((u16)16)
> +#define NCN26000_REG_IRQ_STATUS ((u16)17)
> +
> +#define NCN26000_IRQ_LINKST_BIT ((u16)1)
> +#define NCN26000_IRQ_PLCAST_BIT ((u16)(1 << 1))
> +#define NCN26000_IRQ_LJABBER_BIT ((u16)(1 << 2))
> +#define NCN26000_IRQ_RJABBER_BIT ((u16)(1 << 3))
> +#define NCN26000_IRQ_RJABBER_BIT ((u16)(1 << 3))
> +#define NCN26000_IRQ_PLCAREC_BIT ((u16)(1 << 4))
> +#define NCN26000_IRQ_PHYSCOL_BIT ((u16)(1 << 5))
There isn't much point in having the casts to u16 here. Also,
BIT() is useful for identifying single bits.
> +static int ncn26000_enable(struct phy_device *phydev)
> +{
This is actually the config_aneg() implementation, it should be named
as such.
> + phydev->mdix_ctrl = ETH_TP_MDI_AUTO;
> + phydev->mdix = ETH_TP_MDI;
> + phydev->pause = 0;
> + phydev->asym_pause = 0;
> + phydev->speed = SPEED_10;
> + phydev->duplex = DUPLEX_HALF;
Is this initialisation actually necessary?
> +
> + // bring up the link (link_ctrl is mapped to BMCR_ANENABLE)
> + // clear also ISOLATE mode and Collision Test
> + return phy_write(phydev, MII_BMCR, BMCR_ANENABLE);
You always use AN even when ethtool turns off AN? If AN is mandatory,
it seems there should be some way that phylib can force that to be
the case.
> +}
> +
> +static int ncn26000_soft_reset(struct phy_device *phydev)
> +{
> + int ret;
> +
> + ret = phy_set_bits(phydev, MII_BMCR, BMCR_RESET);
> +
> + if (ret != 0)
> + return ret;
> +
> + return phy_read_poll_timeout(phydev,
> + MII_BMCR,
> + ret,
> + !(ret & BMCR_RESET),
> + 500,
> + 20000,
> + true);
Isn't this just genphy_reset() ?
> +}
> +
> +static int ncn26000_get_features(struct phy_device *phydev)
> +{
> + linkmode_zero(phydev->supported);
> + linkmode_set_bit(ETHTOOL_LINK_MODE_MII_BIT, phydev->supported);
> +
> + linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT1S_P2MP_Half_BIT,
> + phydev->supported);
> +
> + linkmode_copy(phydev->advertising, phydev->supported);
> + return 0;
> +}
> +
> +static irqreturn_t ncn26000_handle_interrupt(struct phy_device *phydev)
> +{
> + const struct ncn26000_priv *const priv = phydev->priv;
> + u16 events;
> + int ret;
> +
> + // read and aknowledge the IRQ status register
> + ret = phy_read(phydev, NCN26000_REG_IRQ_STATUS);
> +
> + if (unlikely(ret < 0))
> + return IRQ_NONE;
> +
> + events = (u16)ret & priv->enabled_irqs;
> + if (events == 0)
> + return IRQ_NONE;
> +
> + if (events & NCN26000_IRQ_LINKST_BIT) {
> + ret = phy_read(phydev, MII_BMSR);
> +
> + if (unlikely(ret < 0)) {
> + phydev_err(phydev,
> + "error reading the status register (%d)\n",
> + ret);
> +
> + return IRQ_NONE;
> + }
> +
> + phydev->link = ((u16)ret & BMSR_ANEGCOMPLETE) ? 1 : 0;
1. aneg_complete shouldn't be used to set phydev->link.
2. phydev->link should be updated in the read_status() function, which
the state machine will call. Setting it here without taking the lock
introduces races.
> + }
> +
> + // handle more IRQs here
> +
> + phy_trigger_machine(phydev);
> + return IRQ_HANDLED;
> +}
> +
> +static int ncn26000_config_intr(struct phy_device *phydev)
> +{
> + int ret;
> + struct ncn26000_priv *priv = phydev->priv;
> +
> + if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
> + // acknowledge IRQs
> + ret = phy_read(phydev, NCN26000_REG_IRQ_STATUS);
> + if (ret < 0)
> + return ret;
> +
> + // get link status notifications
> + priv->enabled_irqs = NCN26000_IRQ_LINKST_BIT;
> + } else {
> + // disable all IRQs
> + priv->enabled_irqs = 0;
> + }
> +
> + ret = phy_write(phydev, NCN26000_REG_IRQ_CTL, priv->enabled_irqs);
> + if (ret != 0)
> + return ret;
> +
> + return 0;
> +}
> +
> +static int ncn26000_probe(struct phy_device *phydev)
> +{
> + struct device *dev = &phydev->mdio.dev;
> + struct ncn26000_priv *priv;
> +
> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + phydev->priv = priv;
> +
> + return 0;
> +}
> +
> +static void ncn26000_remove(struct phy_device *phydev)
> +{
> + struct device *dev = &phydev->mdio.dev;
> + struct ncn26000_priv *priv = phydev->priv;
> +
> + // free the private structure pointer
> + devm_kfree(dev, priv);
No need to call devm_kfree() - the point of devm_*() is that resources
are automatically released.
Thanks.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Powered by blists - more mailing lists