netdev - Re: [PATCH net-next 2/2] net: phy: add keep_data

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: <ZL/KIjjw3AZmQcGn@shell.armlinux.org.uk>
Date: Tue, 25 Jul 2023 14:12:02 +0100
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Simon Horman <simon.horman@...igine.com>
Cc: Mengyuan Lou <mengyuanlou@...-swift.com>, netdev@...r.kernel.org,
	Jakub Kicinski <kuba@...nel.org>,
	"David S. Miller" <davem@...emloft.net>,
	Paolo Abeni <pabeni@...hat.com>, Eric Dumazet <edumazet@...gle.com>,
	Heiner Kallweit <hkallweit1@...il.com>,
	Andrew Lunn <andrew@...n.ch>
Subject: Re: [PATCH net-next 2/2] net: phy: add keep_data_connection to
 struct phydev

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!