| 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: <CAK+owog+FYU0Y+ceg1=DtLdS+Nw0JMjDEOoYs71YAEGDbKrXfw@mail.gmail.com>
Date: Tue, 20 May 2025 17:33:36 +0200
From: Stefano Radaelli <stefano.radaelli21@...il.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
Cc: netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
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>, Xu Liang <lxu@...linear.com>
Subject: Re: [PATCH net-next v4] net: phy: add driver for MaxLinear MxL86110 PHY
Hi Russel,
Thank you very much for your feedback and for pointing out these aspects.
I really appreciate your review and the time you took to highlight these
improvements.
> As these are all unlocked variants, I wonder whether they should have
> __ prefixes. I'm wondering whether our paged accessors could be re-used
> for this phy, even though effectively there is only one "paged" register
> at offset 31 with the page index at offset 30.
>
> Also, given the number of single accesses to the registers, I wonder if
> it also makes sense to have variants that take the MDIO bus lock, which
> would allow simplification of sites such as...
Regarding the __ prefixes: I absolutely agree with following the established
naming convention, as seen for instance with __phy_write vs phy_write
and __mdiobus_write vs mdiobus_write.
I also believe it would make sense to rename these functions to
__mxl86110_{read,write}_extended_reg and add corresponding locked variants
like mxl86110_{read,write}_extended_reg() to allow simplifying the call sites
you mentioned.
As for the paged accessors: personally, I would prefer to keep the current
approach to explicitly reflect that we're accessing what the datasheet refers to
as "Extended Registers". Introducing a generic paging mechanism here might
obscure that intention and potentially cause confusion for readers less familiar
with this PHY. What do you think?
If you agree, I’d be happy to proceed with the changes accordingly.
Best Regards,
Stefano
Il giorno mar 20 mag 2025 alle ore 16:47 Russell King (Oracle)
<linux@...linux.org.uk> ha scritto:
>
> On Tue, May 20, 2025 at 02:45:18PM +0200, stefano.radaelli21@...il.com wrote:
> > +/**
> > + * mxl86110_write_extended_reg() - write to a PHY's extended register
> > + * @phydev: pointer to a &struct phy_device
> > + * @regnum: register number to write
> > + * @val: value to write to @regnum
> > + *
> > + * Note: This function assumes the caller already holds the MDIO bus lock
> > + * or otherwise has exclusive access to the PHY.
> > + *
> > + * Return: 0 or negative error code
> > + */
> > +static int mxl86110_write_extended_reg(struct phy_device *phydev,
> > + u16 regnum, u16 val)
> > +{
> > + int ret;
> > +
> > + ret = __phy_write(phydev, MXL86110_EXTD_REG_ADDR_OFFSET, regnum);
> > + if (ret < 0)
> > + return ret;
> > +
> > + return __phy_write(phydev, MXL86110_EXTD_REG_ADDR_DATA, val);
> > +}
> > +
> > +/**
> > + * mxl86110_read_extended_reg - Read a PHY's extended register
> > + * @phydev: Pointer to the PHY device structure
> > + * @regnum: Extended register number to read (address written to reg 30)
> > + *
> > + * Reads the content of a PHY extended register using the MaxLinear
> > + * 2-step access mechanism: write the register address to reg 30 (0x1E),
> > + * then read the value from reg 31 (0x1F).
> > + *
> > + * Note: This function assumes the caller already holds the MDIO bus lock
> > + * or otherwise has exclusive access to the PHY.
> > + *
> > + * Return: 16-bit register value on success, or negative errno code on failure.
> > + */
> > +static int mxl86110_read_extended_reg(struct phy_device *phydev, u16 regnum)
> > +{
> > + int ret;
> > +
> > + ret = __phy_write(phydev, MXL86110_EXTD_REG_ADDR_OFFSET, regnum);
> > + if (ret < 0)
> > + return ret;
> > + return __phy_read(phydev, MXL86110_EXTD_REG_ADDR_DATA);
> > +}
> > +
> > +/**
> > + * mxl86110_modify_extended_reg() - modify bits of a PHY's extended register
> > + * @phydev: pointer to the phy_device
> > + * @regnum: register number to write
> > + * @mask: bit mask of bits to clear
> > + * @set: bit mask of bits to set
> > + *
> > + * Note: register value = (old register value & ~mask) | set.
> > + * This function assumes the caller already holds the MDIO bus lock
> > + * or otherwise has exclusive access to the PHY.
> > + *
> > + * Return: 0 or negative error code
> > + */
> > +static int mxl86110_modify_extended_reg(struct phy_device *phydev,
> > + u16 regnum, u16 mask, u16 set)
> > +{
> > + int ret;
> > +
> > + ret = __phy_write(phydev, MXL86110_EXTD_REG_ADDR_OFFSET, regnum);
> > + if (ret < 0)
> > + return ret;
> > +
> > + return __phy_modify(phydev, MXL86110_EXTD_REG_ADDR_DATA, mask, set);
> > +}
>
> As these are all unlocked variants, I wonder whether they should have
> __ prefixes. I'm wondering whether our paged accessors could be re-used
> for this phy, even though effectively there is only one "paged" register
> at offset 31 with the page index at offset 30.
>
> Also, given the number of single accesses to the registers, I wonder if
> it also makes sense to have variants that take the MDIO bus lock, which
> would allow simplification of sites such as...
>
> > +
> > +/**
> > + * mxl86110_get_wol() - report if wake-on-lan is enabled
> > + * @phydev: pointer to the phy_device
> > + * @wol: a pointer to a &struct ethtool_wolinfo
> > + */
> > +static void mxl86110_get_wol(struct phy_device *phydev,
> > + struct ethtool_wolinfo *wol)
> > +{
> > + int value;
> > +
> > + wol->supported = WAKE_MAGIC;
> > + wol->wolopts = 0;
> > + phy_lock_mdio_bus(phydev);
> > + value = mxl86110_read_extended_reg(phydev, MXL86110_EXT_WOL_CFG_REG);
> > + phy_unlock_mdio_bus(phydev);
>
> ... here.
>
> > + if (value >= 0 && (value & MXL86110_WOL_CFG_WOLE_MASK))
> > + wol->wolopts |= WAKE_MAGIC;
> > +}
> > +
> > +/**
> > + * mxl86110_set_wol() - enable/disable wake-on-lan
> > + * @phydev: pointer to the phy_device
> > + * @wol: a pointer to a &struct ethtool_wolinfo
> > + *
> > + * Configures the WOL Magic Packet MAC
> > + *
> > + * Return: 0 or negative errno code
> > + */
> > +static int mxl86110_set_wol(struct phy_device *phydev,
> > + struct ethtool_wolinfo *wol)
> > +{
> > + struct net_device *netdev;
> > + const u8 *mac;
>
> Use "const unsigned char *mac" - that way you don't need the cast below.
>
> > + int ret = 0;
> > +
> > + phy_lock_mdio_bus(phydev);
> > +
> > + if (wol->wolopts & WAKE_MAGIC) {
> > + netdev = phydev->attached_dev;
> > + if (!netdev) {
> > + ret = -ENODEV;
> > + goto out;
> > + }
> > +
> > + /* Configure the MAC address of the WOL magic packet */
> > + mac = (const u8 *)netdev->dev_addr;
> > + ret = mxl86110_write_extended_reg(phydev,
> > + MXL86110_EXT_MAC_ADDR_CFG1,
> > + ((mac[0] << 8) | mac[1]));
> > + if (ret < 0)
> > + goto out;
> > +
> > + ret = mxl86110_write_extended_reg(phydev,
> > + MXL86110_EXT_MAC_ADDR_CFG2,
> > + ((mac[2] << 8) | mac[3]));
> > + if (ret < 0)
> > + goto out;
> > +
> > + ret = mxl86110_write_extended_reg(phydev,
> > + MXL86110_EXT_MAC_ADDR_CFG3,
> > + ((mac[4] << 8) | mac[5]));
> > + if (ret < 0)
> > + goto out;
> > +
> > + ret = mxl86110_modify_extended_reg(phydev,
> > + MXL86110_EXT_WOL_CFG_REG,
> > + MXL86110_WOL_CFG_WOLE_MASK,
> > + MXL86110_EXT_WOL_CFG_WOLE);
> > + if (ret < 0)
> > + goto out;
> > +
> > + /* Enables Wake-on-LAN interrupt in the PHY. */
> > + ret = __phy_modify(phydev, PHY_IRQ_ENABLE_REG, 0,
> > + PHY_IRQ_ENABLE_REG_WOL);
> > + if (ret < 0)
> > + goto out;
> > +
> > + phydev_dbg(phydev,
> > + "%s, MAC Addr: %02X:%02X:%02X:%02X:%02X:%02X\n",
> > + __func__,
> > + mac[0], mac[1], mac[2], mac[3], mac[4], mac[5]);
> > + } else {
> > + ret = mxl86110_modify_extended_reg(phydev,
> > + MXL86110_EXT_WOL_CFG_REG,
> > + MXL86110_WOL_CFG_WOLE_MASK,
> > + 0);
> > + if (ret < 0)
> > + goto out;
> > +
> > + /* Disables Wake-on-LAN interrupt in the PHY. */
> > + ret = __phy_modify(phydev, PHY_IRQ_ENABLE_REG,
> > + PHY_IRQ_ENABLE_REG_WOL, 0);
> > + }
> > +
> > +out:
> > + phy_unlock_mdio_bus(phydev);
> > + return ret;
> > +}
> > +
>
> ...
> > +static int mxl86110_led_hw_control_get(struct phy_device *phydev, u8 index,
> > + unsigned long *rules)
> > +{
> > + int val;
> > +
> > + if (index >= MXL86110_MAX_LEDS)
> > + return -EINVAL;
> > +
> > + phy_lock_mdio_bus(phydev);
> > + val = mxl86110_read_extended_reg(phydev,
> > + MXL86110_LED0_CFG_REG + index);
> > + phy_unlock_mdio_bus(phydev);
>
> This could also be simplified with the locking accessors.
>
> > + if (val < 0)
> > + return val;
> > +
> > + if (val & MXL86110_LEDX_CFG_LINK_UP_TX_ACT_ON)
> > + *rules |= BIT(TRIGGER_NETDEV_TX);
> > +
> > + if (val & MXL86110_LEDX_CFG_LINK_UP_RX_ACT_ON)
> > + *rules |= BIT(TRIGGER_NETDEV_RX);
> > +
> > + if (val & MXL86110_LEDX_CFG_LINK_UP_HALF_DUPLEX_ON)
> > + *rules |= BIT(TRIGGER_NETDEV_HALF_DUPLEX);
> > +
> > + if (val & MXL86110_LEDX_CFG_LINK_UP_FULL_DUPLEX_ON)
> > + *rules |= BIT(TRIGGER_NETDEV_FULL_DUPLEX);
> > +
> > + if (val & MXL86110_LEDX_CFG_LINK_UP_10MB_ON)
> > + *rules |= BIT(TRIGGER_NETDEV_LINK_10);
> > +
> > + if (val & MXL86110_LEDX_CFG_LINK_UP_100MB_ON)
> > + *rules |= BIT(TRIGGER_NETDEV_LINK_100);
> > +
> > + if (val & MXL86110_LEDX_CFG_LINK_UP_1GB_ON)
> > + *rules |= BIT(TRIGGER_NETDEV_LINK_1000);
> > +
> > + return 0;
> > +};
> > +
> > +static int mxl86110_led_hw_control_set(struct phy_device *phydev, u8 index,
> > + unsigned long rules)
> > +{
> > + u16 val = 0;
> > + int ret;
> > +
> > + if (index >= MXL86110_MAX_LEDS)
> > + return -EINVAL;
> > +
> > + if (rules & BIT(TRIGGER_NETDEV_LINK_10))
> > + val |= MXL86110_LEDX_CFG_LINK_UP_10MB_ON;
> > +
> > + if (rules & BIT(TRIGGER_NETDEV_LINK_100))
> > + val |= MXL86110_LEDX_CFG_LINK_UP_100MB_ON;
> > +
> > + if (rules & BIT(TRIGGER_NETDEV_LINK_1000))
> > + val |= MXL86110_LEDX_CFG_LINK_UP_1GB_ON;
> > +
> > + if (rules & BIT(TRIGGER_NETDEV_TX))
> > + val |= MXL86110_LEDX_CFG_LINK_UP_TX_ACT_ON;
> > +
> > + if (rules & BIT(TRIGGER_NETDEV_RX))
> > + val |= MXL86110_LEDX_CFG_LINK_UP_RX_ACT_ON;
> > +
> > + if (rules & BIT(TRIGGER_NETDEV_HALF_DUPLEX))
> > + val |= MXL86110_LEDX_CFG_LINK_UP_HALF_DUPLEX_ON;
> > +
> > + if (rules & BIT(TRIGGER_NETDEV_FULL_DUPLEX))
> > + val |= MXL86110_LEDX_CFG_LINK_UP_FULL_DUPLEX_ON;
> > +
> > + if (rules & BIT(TRIGGER_NETDEV_TX) ||
> > + rules & BIT(TRIGGER_NETDEV_RX))
> > + val |= MXL86110_LEDX_CFG_LAB_BLINK;
> > +
> > + phy_lock_mdio_bus(phydev);
> > + ret = mxl86110_write_extended_reg(phydev,
> > + MXL86110_LED0_CFG_REG + index, val);
> > + phy_unlock_mdio_bus(phydev);
>
> and this... and with the locking accessors it could become simply:
>
> return mxl86110_write_extended_reg(...);
>
> > + if (ret)
> > + return ret;
> > +
> > + return 0;
> > +};
> > +
> > +/**
> > + * mxl86110_synce_clk_cfg() - applies syncE/clk output configuration
> > + * @phydev: pointer to the phy_device
> > + *
> > + * Note: This function assumes the caller already holds the MDIO bus lock
> > + * or otherwise has exclusive access to the PHY.
> > + *
> > + * Return: 0 or negative errno code
> > + */
> > +static int mxl86110_synce_clk_cfg(struct phy_device *phydev)
> > +{
> > + u16 mask = 0, val = 0;
> > + int ret;
> > +
> > + /*
> > + * Configures the clock output to its default
> > + * setting as per the datasheet.
> > + * This results in a 25MHz clock output being selected in the
> > + * COM_EXT_SYNCE_CFG register for SyncE configuration.
> > + */
> > + val = MXL86110_EXT_SYNCE_CFG_EN_SYNC_E |
> > + FIELD_PREP(MXL86110_EXT_SYNCE_CFG_CLK_SRC_SEL_MASK,
> > + MXL86110_EXT_SYNCE_CFG_CLK_SRC_SEL_25M);
> > + mask = MXL86110_EXT_SYNCE_CFG_EN_SYNC_E |
> > + MXL86110_EXT_SYNCE_CFG_CLK_SRC_SEL_MASK |
> > + MXL86110_EXT_SYNCE_CFG_CLK_FRE_SEL;
> > +
> > + /* Write clock output configuration */
> > + ret = mxl86110_modify_extended_reg(phydev, MXL86110_EXT_SYNCE_CFG_REG,
> > + mask, val);
> > +
> > + return ret;
>
> No need for "ret":
>
> return mxl86110_modify_extended_reg(phydev, MXL86110_EXT_SYNCE_CFG_REG,
> mask, val);
>
> > +}
> > +
> > +/**
> > + * mxl86110_broadcast_cfg - Configure MDIO broadcast setting for PHY
> > + * @phydev: Pointer to the PHY device structure
> > + *
> > + * This function configures the MDIO broadcast behavior of the MxL86110 PHY.
> > + * Currently, broadcast mode is explicitly disabled by clearing the EPA0 bit
> > + * in the RGMII_MDIO_CFG extended register.
> > + *
> > + * Note: This function assumes the caller already holds the MDIO bus lock
> > + * or otherwise has exclusive access to the PHY.
> > + *
> > + * Return: 0 on success or a negative errno code on failure.
> > + */
> > +static int mxl86110_broadcast_cfg(struct phy_device *phydev)
> > +{
> > + int ret;
> > +
> > + ret = mxl86110_modify_extended_reg(phydev,
> > + MXL86110_EXT_RGMII_MDIO_CFG,
> > + MXL86110_RGMII_MDIO_CFG_EPA0_MASK,
> > + 0);
> > +
> > + return ret;
>
> No need for "ret".
>
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
Powered by blists - more mailing lists