netdev - Re: [PATCH net-next v1 1/7] net: usb: lan78xx: Convert to PHYlink for improved PHY and MAC management

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: <Z5CLAMsLJtKA43kM@pengutronix.de>
Date: Wed, 22 Jan 2025 07:06:56 +0100
From: Oleksij Rempel <o.rempel@...gutronix.de>
To: Thangaraj.S@...rochip.com
Cc: andrew+netdev@...n.ch, rmk+kernel@...linux.org.uk, davem@...emloft.net,
	Woojung.Huh@...rochip.com, pabeni@...hat.com, edumazet@...gle.com,
	kuba@...nel.org, 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 Thangaraj,

On Wed, Jan 22, 2025 at 04:02:42AM +0000, Thangaraj.S@...rochip.com wrote:
> Hi Oleksji,
> 
> On Wed, 2025-01-08 at 13:13 +0100, Oleksij Rempel wrote:
> >  /* 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?

### Before: Old PHY Interrupt Handling

In the old implementation, the driver processed PHY interrupts as follows:

1. When a status URB was received, the driver checked for the `INT_ENP_PHY_INT`
   flag to detect a PHY-related interrupt:
   - Upon detecting the interrupt, it triggered a deferred kevent
     (`lan78xx_defer_kevent`) with the `EVENT_LINK_RESET` event.
   - It also called `generic_handle_irq_safe` to notify the PHY subsystem of
     the interrupt, allowing it to update the PHY state.

2. The deferred kevent was handled by `lan78xx_link_reset`, which:
   - It invoked `phy_read_status` to retrieve the PHY state.
   - Based on the PHY state, it adjusted MAC settings (e.g., flow control,
     USB state) and restarted the RX/TX paths if needed.
   - Enabled/disrupted timers and submitted RX URBs when link changes occurred.

This design required the driver to manually handle both PHY and MAC state
management, leading to complex and redundant logic.

### Now: PHYlink Integration

In the updated code, the handling process is simplified:

1. When a status URB detects a PHY interrupt (`INT_ENP_PHY_INT`), the driver
   still calls `lan78xx_defer_kevent(dev, EVENT_PHY_INT_ACK)`. However:
   - The deferred event now serves only to acknowledge the interrupt by calling
     `lan78xx_phy_int_ack`.
   - This separation is necessary because the URB completion context cannot
     directly perform register writes.

2. The interrupt is forwarded to the PHY subsystem, which updates the PHY state
   as before. No additional logic is performed in this step.

3. Once the PHY state is updated, PHYlink invokes appropriate callbacks to
   handle MAC reconfiguration:
   - `mac_config` for initial setup.
   - `mac_link_up` and `mac_link_down` to manage link transitions, flow
     control, and USB state.

### Why the Old Logic is No Longer Needed

The MAC is now reconfigured only through PHYlink callbacks, eliminating the
need for deferred events to handle complex link reset logic.

With PHYlink coordinating PHY and MAC interactions, the driver no longer needs
custom logic to manage state transitions.

> > +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.

Thank you. I'll update it.

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |