| 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: <0397c3a4cac5795fc33dd313ea74a8743a587d28.camel@microchip.com>
Date: Wed, 22 Jan 2025 04:02:42 +0000
From: <Thangaraj.S@...rochip.com>
To: <andrew+netdev@...n.ch>, <rmk+kernel@...linux.org.uk>,
<davem@...emloft.net>, <Woojung.Huh@...rochip.com>, <pabeni@...hat.com>,
<o.rempel@...gutronix.de>, <edumazet@...gle.com>, <kuba@...nel.org>
CC: <phil@...pberrypi.org>, <kernel@...gutronix.de>,
<linux-kernel@...r.kernel.org>, <netdev@...r.kernel.org>,
<UNGLinuxDriver@...rochip.com>
Subject: Re: [PATCH net-next v1 1/7] net: usb: lan78xx: Convert to PHYlink for
improved PHY and MAC management
Hi Oleksji,
On Wed, 2025-01-08 at 13:13 +0100, Oleksij Rempel wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
>
> Convert the LAN78xx driver to use the PHYlink framework for managing
> PHY and MAC interactions.
>
> Key changes include:
> - Replace direct PHY operations with phylink equivalents (e.g.,
> phylink_start, phylink_stop).
> - Introduce lan78xx_phylink_setup for phylink initialization and
> configuration.
> - Add phylink MAC operations (lan78xx_mac_config,
> lan78xx_mac_link_down, lan78xx_mac_link_up) for managing link
> settings and flow control.
> - Remove redundant and now phylink-managed functions like
> `lan78xx_link_status_change`.
>
> Signed-off-by: Oleksij Rempel <o.rempel@...gutronix.de>
> ---
> drivers/net/usb/lan78xx.c | 525 +++++++++++++++++++++---------------
> --
> 1 file changed, 287 insertions(+), 238 deletions(-)
>
> diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
> index a91bf9c7e31d..6dfd9301279f 100644
> --- a/drivers/net/usb/lan78xx.c
> +++ b/drivers/net/usb/lan78xx.c
> @@ -6,6 +6,7 @@
> #include <linux/netdevice.h>
> #include <linux/etherdevice.h>
> #include <linux/ethtool.h>
> +#include <linux/phylink.h>
> #include <linux/usb.h>
> #include <linux/crc32.h>
> #include <linux/signal.h>
> @@ -384,7 +385,7 @@ struct skb_data { /* skb->cb is one of
> these */
> #define EVENT_RX_HALT 1
> #define EVENT_RX_MEMORY 2
> #define EVENT_STS_SPLIT 3
> -#define EVENT_LINK_RESET 4
> +#define EVENT_PHY_INT_ACK 4
> #define EVENT_RX_PAUSED 5
> #define EVENT_DEV_WAKING 6
> #define EVENT_DEV_ASLEEP 7
> @@ -470,6 +471,9 @@ struct lan78xx_net {
> struct statstage stats;
>
> struct irq_domain_data domain_data;
> +
> + struct phylink *phylink;
> + struct phylink_config phylink_config;
> };
>
> /* use ethtool to change the level for any given device */
> @@ -1554,40 +1558,6 @@ static void lan78xx_set_multicast(struct
> net_device *netdev)
> schedule_work(&pdata->set_multicast);
> }
>
>
>
> -static int lan78xx_link_reset(struct lan78xx_net *dev)
> +static int lan78xx_phy_int_ack(struct lan78xx_net *dev)
> {
Is there any specific reason why this complete logic on phy interrupt
handling is removed?
> - struct phy_device *phydev = dev->net->phydev;
> - struct ethtool_link_ksettings ecmd;
> - int ladv, radv, ret, link;
> - u32 buf;
> -
> /* clear LAN78xx interrupt status */
> - ret = lan78xx_write_reg(dev, INT_STS, INT_STS_PHY_INT_);
> - if (unlikely(ret < 0))
> - return ret;
> -
> - mutex_lock(&phydev->lock);
> - phy_read_status(phydev);
> - link = phydev->link;
> - mutex_unlock(&phydev->lock);
> -
> - if (!link && dev->link_on) {
> - dev->link_on = false;
> -
> - /* reset MAC */
> - ret = lan78xx_mac_reset(dev);
> - if (ret < 0)
> - return ret;
> -
> - del_timer(&dev->stat_monitor);
> - } else if (link && !dev->link_on) {
> - dev->link_on = true;
> -
> - phy_ethtool_ksettings_get(phydev, &ecmd);
> -
> - if (dev->udev->speed == USB_SPEED_SUPER) {
> - if (ecmd.base.speed == 1000) {
> - /* disable U2 */
> - ret = lan78xx_read_reg(dev, USB_CFG1,
> &buf);
> - if (ret < 0)
> - return ret;
> - buf &= ~USB_CFG1_DEV_U2_INIT_EN_;
> - ret = lan78xx_write_reg(dev,
> USB_CFG1, buf);
> - if (ret < 0)
> - return ret;
> - /* enable U1 */
> - ret = lan78xx_read_reg(dev, USB_CFG1,
> &buf);
> - if (ret < 0)
> - return ret;
> - buf |= USB_CFG1_DEV_U1_INIT_EN_;
> - ret = lan78xx_write_reg(dev,
> USB_CFG1, buf);
> - if (ret < 0)
> - return ret;
> - } else {
> - /* enable U1 & U2 */
> - ret = lan78xx_read_reg(dev, USB_CFG1,
> &buf);
> - if (ret < 0)
> - return ret;
> - buf |= USB_CFG1_DEV_U2_INIT_EN_;
> - buf |= USB_CFG1_DEV_U1_INIT_EN_;
> - ret = lan78xx_write_reg(dev,
> USB_CFG1, buf);
> - if (ret < 0)
> - return ret;
> - }
> - }
> -
> - ladv = phy_read(phydev, MII_ADVERTISE);
> - if (ladv < 0)
> - return ladv;
> -
> - radv = phy_read(phydev, MII_LPA);
> - if (radv < 0)
> - return radv;
> -
> - netif_dbg(dev, link, dev->net,
> - "speed: %u duplex: %d anadv: 0x%04x anlpa:
> 0x%04x",
> - ecmd.base.speed, ecmd.base.duplex, ladv,
> radv);
> -
> - ret = lan78xx_update_flowcontrol(dev,
> ecmd.base.duplex, ladv,
> - radv);
> - if (ret < 0)
> - return ret;
> -
> - if (!timer_pending(&dev->stat_monitor)) {
> - dev->delta = 1;
> - mod_timer(&dev->stat_monitor,
> - jiffies + STAT_UPDATE_TIMER);
> - }
> -
> - lan78xx_rx_urb_submit_all(dev);
> -
> - local_bh_disable();
> - napi_schedule(&dev->napi);
> - local_bh_enable();
> - }
> -
> - return 0;
> + return lan78xx_write_reg(dev, INT_STS, INT_STS_PHY_INT_);
> }
>
> /* some work can't be done in tasklets, so we use keventd
> @@ -1757,7 +1638,7 @@ static void lan78xx_status(struct lan78xx_net
> *dev, struct urb *urb)
>
> if (intdata & INT_ENP_PHY_INT) {
> netif_dbg(dev, link, dev->net, "PHY INTR: 0x%08x\n",
> intdata);
> - lan78xx_defer_kevent(dev, EVENT_LINK_RESET);
> + lan78xx_defer_kevent(dev, EVENT_PHY_INT_ACK);
>
> if (dev->domain_data.phyirq > 0)
> generic_handle_irq_safe(dev-
> >domain_data.phyirq);
> @@ -2356,26 +2237,6 @@ static void lan78xx_remove_mdio(struct
> lan78xx_net *dev)
> mdiobus_free(dev->mdiobus);
> }
>
> -static void lan78xx_link_status_change(struct net_device *net)
> -{
> - struct lan78xx_net *dev = netdev_priv(net);
> - struct phy_device *phydev = net->phydev;
> - u32 data;
> - int ret;
> -
> - ret = lan78xx_read_reg(dev, MAC_CR, &data);
> - if (ret < 0)
> - return;
> -
> - if (phydev->enable_tx_lpi)
> - data |= MAC_CR_EEE_EN_;
> - else
> - data &= ~MAC_CR_EEE_EN_;
> - lan78xx_write_reg(dev, MAC_CR, data);
> -
> - phy_print_status(phydev);
> -}
> -
> static int irq_map(struct irq_domain *d, unsigned int irq,
> irq_hw_number_t hwirq)
> {
> @@ -2508,6 +2369,207 @@ static void lan78xx_remove_irq_domain(struct
> lan78xx_net *dev)
> dev->domain_data.irqdomain = NULL;
> }
>
> +static void lan78xx_mac_config(struct phylink_config *config,
> unsigned int mode,
> + const struct phylink_link_state
> *state)
> +{
> + struct net_device *net = to_net_dev(config->dev);
> + struct lan78xx_net *dev = netdev_priv(net);
> + u32 rgmii_id = 0;
This variable is not modified anywhere in this function. Remove this
variable if not needed.
> + u32 mac_cr = 0;
> + int ret;
> +
> + /* Check if the mode is supported */
> + if (mode != MLO_AN_FIXED && mode != MLO_AN_PHY) {
> + netdev_err(net, "Unsupported negotiation mode: %u\n",
> mode);
> + return;
> + }
> +
> + switch (state->interface) {
> + case PHY_INTERFACE_MODE_INTERNAL:
> + case PHY_INTERFACE_MODE_GMII:
> + mac_cr |= MAC_CR_GMII_EN_;
> + break;
> + case PHY_INTERFACE_MODE_RGMII_ID:
> + break;
> + default:
> + netdev_warn(net, "Unsupported interface mode: %d\n",
> + state->interface);
> + return;
> + }
> +
> + /* by the way, make sure auto speed and duplex are disabled
> */
> + ret = lan78xx_update_reg(dev, MAC_CR, MAC_CR_AUTO_DUPLEX_ |
> + MAC_CR_AUTO_SPEED_ |
> MAC_CR_GMII_EN_, mac_cr);
> + if (ret < 0)
> + goto mac_config_fail;
> +
> + ret = lan78xx_write_reg(dev, MAC_RGMII_ID, rgmii_id);
> + if (ret < 0)
> + goto mac_config_fail;
> +
> + return;
> +
> +mac_config_fail:
> + netdev_err(net, "Failed to config MAC with error %pe\n",
> ERR_PTR(ret));
> +}
> +
>
Powered by blists - more mailing lists