Re: [PATCH net-next 2/2] net: phy: add keep_data_connection to struct phydev

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Wed, Jul 26, 2023 at 10:35:32AM +0800, mengyuanlou@net-swift.com wrote:
> 
> 
> > 2023年7月25日 21:12，Russell King (Oracle) <linux@armlinux.org.uk> 写道：
> > 
> > Hi Simon,
> > 
> > Thanks for spotting that this wasn't sent to those who should have
> > been.
> > 
> > Mengyuan Lou, please ensure that you address your patches to
> > appropriate recipients.
> > 
> > On Tue, Jul 25, 2023 at 02:05:36PM +0200, Simon Horman wrote:
> >>> + * @keep_data_connection: Set to true if the PHY or the attached MAC need
> >>> + *                        physical connection to receive packets.
> > 
> > Having had a brief read through, this comment seems to me to convey
> > absolutely no useful information what so ever.
> > 
> > In order to receive packets, a physical connection between the MAC and
> > PHY is required. So, based on that comment, keep_data_connection must
> > always be true!
> > 
> > So, the logic in phylib at the moment is:
> > 
> >        phydev->wol_enabled = wol.wolopts || (netdev && netdev->wol_enabled);
> >        /* If the device has WOL enabled, we cannot suspend the PHY */
> >        if (phydev->wol_enabled && !(phydrv->flags & PHY_ALWAYS_CALL_SUSPEND))
> >                return -EBUSY;
> > 
> > wol_enabled will be true if the PHY driver reports that WoL is
> > enabled at the PHY, or the network device marks that WoL is
> > enabled at the network device. netdev->wol_enabled should be set
> > when the network device is looking for the wakeup packets.
> > 
> > Then, the PHY_ALWAYS_CALL_SUSPEND flag basically says that "even
> > in these cases, we want to suspend the PHY".
> > 
> > This patch appears to drop netdev->wol_enabled, replacing it with
> > netdev->ncsi_enabled, whatever that is - and this change alone is
> > probably going to break drivers, since they will already be
> > expecting that netdev->wol_enabled causes the PHY _not_ to be
> > suspended.
> > 
> > Therefore, I'm not sure this patch makes much sense.
> > 
> > Since the phylib maintainers were not copied with the original
> > patch, that's also a reason to NAK it.
> > 
> > Thanks.
> > 
> > -- 
> > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
> > 
> 
> 
> Now Mac and phy in kernel is separated into two parts.
> There are some features need to keep data connection.
> 
> Phy ——— Wake-on-Lan —— magic packets
> 
> When NIC as a ethernet in host os and it also supports ncsi as a bmc network port at same time.
> Mac/mng —— LLDP/NCSI —— ncsi packtes
> I think it need a way to notice phy modules.

Right, so this is _in addtion_ to WoL. Therefore, when adding support
for it, you need to _keep_ the existing WoL support, not remove it in
preference for NCSI.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!