[PATCH net-next 1/2] net: ngbe: add ncsi_enable flag for wangxun nics

[PATCH net-next 1/2] net: ngbe: add ncsi_enable flag for wangxun nics

[Mengyuan Lou]

Add ncsi_enabled flag to struct netdev to indicate wangxun
nics which support NCSI.

Signed-off-by: Mengyuan Lou <mengyuanlou@net-swift.com>
---
 drivers/net/ethernet/wangxun/libwx/wx_type.h  | 2 +-
 drivers/net/ethernet/wangxun/ngbe/ngbe_main.c | 5 +++--
 include/linux/netdevice.h                     | 3 +++
 3 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/wangxun/libwx/wx_type.h b/drivers/net/ethernet/wangxun/libwx/wx_type.h
index 1de88a33a698..1b932e66044e 100644
--- a/drivers/net/ethernet/wangxun/libwx/wx_type.h
+++ b/drivers/net/ethernet/wangxun/libwx/wx_type.h
@@ -851,7 +851,7 @@ struct wx {
 	struct phy_device *phydev;
 
 	bool wol_hw_supported;
-	bool ncsi_enabled;
+	bool ncsi_hw_supported;
 	bool gpio_ctrl;
 	raw_spinlock_t gpio_lock;
 
diff --git a/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c b/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
index 2b431db6085a..e42e4dd26700 100644
--- a/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
+++ b/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
@@ -63,8 +63,8 @@ static void ngbe_init_type_code(struct wx *wx)
 		       em_mac_type_mdi;
 
 	wx->wol_hw_supported = (wol_mask == NGBE_WOL_SUP) ? 1 : 0;
-	wx->ncsi_enabled = (ncsi_mask == NGBE_NCSI_MASK ||
-			   type_mask == NGBE_SUBID_OCP_CARD) ? 1 : 0;
+	wx->ncsi_hw_supported = (ncsi_mask == NGBE_NCSI_MASK ||
+				 type_mask == NGBE_SUBID_OCP_CARD) ? 1 : 0;
 
 	switch (type_mask) {
 	case NGBE_SUBID_LY_YT8521S_SFP:
@@ -639,6 +639,7 @@ static int ngbe_probe(struct pci_dev *pdev,
 	netdev->wol_enabled = !!(wx->wol);
 	wr32(wx, NGBE_PSR_WKUP_CTL, wx->wol);
 	device_set_wakeup_enable(&pdev->dev, wx->wol);
+	netdev->ncsi_enabled = wx->ncsi_hw_supported;
 
 	/* Save off EEPROM version number and Option Rom version which
 	 * together make a unique identify for the eeprom
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index b828c7a75be2..dfa14e4c8e95 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -2024,6 +2024,8 @@ enum netdev_ml_priv_type {
  *
  *	@wol_enabled:	Wake-on-LAN is enabled
  *
+ *	@ncsi_enabled:	NCSI is enabled.
+ *
  *	@threaded:	napi threaded mode is enabled
  *
  *	@net_notifier_list:	List of per-net netdev notifier block
@@ -2393,6 +2395,7 @@ struct net_device {
 	struct lock_class_key	*qdisc_tx_busylock;
 	bool			proto_down;
 	unsigned		wol_enabled:1;
+	unsigned		ncsi_enabled:1;
 	unsigned		threaded:1;
 
 	struct list_head	net_notifier_list;
-- 
2.41.0

Re: [PATCH net-next 1/2] net: ngbe: add ncsi_enable flag for wangxun nics

[Jakub Kicinski]

On Mon, 24 Jul 2023 17:24:58 +0800 Mengyuan Lou wrote:
> +	netdev->ncsi_enabled = wx->ncsi_hw_supported;

I don't think that enabled and supported are the same thing.
If server has multiple NICs or a NIC with multiple ports and
BMC only uses one, or even none, we shouldn't keep the PHY up.
By that logic 99% of server NICs should report NCSI as enabled.

Re: [PATCH net-next 1/2] net: ngbe: add ncsi_enable flag for wangxun nics

[mengyuanlou]

> 2023年7月26日 07:22，Jakub Kicinski <kuba@kernel.org> 写道：
> 
> On Mon, 24 Jul 2023 17:24:58 +0800 Mengyuan Lou wrote:
>> + netdev->ncsi_enabled = wx->ncsi_hw_supported;
> 
> I don't think that enabled and supported are the same thing.
> If server has multiple NICs or a NIC with multiple ports and
> BMC only uses one, or even none, we shouldn't keep the PHY up.
> By that logic 99% of server NICs should report NCSI as enabled.
> 
> 

For a NIC with multiple ports, BMC switch connection for port0 to port1 online,
Drivers can not know port1 should keep up, if do not set ncsi_enabled before. 

Re: [PATCH net-next 1/2] net: ngbe: add ncsi_enable flag for wangxun nics

[Jakub Kicinski]

On Wed, 26 Jul 2023 09:59:15 +0800 mengyuanlou@net-swift.com wrote:
> > 2023年7月26日 07:22，Jakub Kicinski <kuba@kernel.org> 写道：
> > On Mon, 24 Jul 2023 17:24:58 +0800 Mengyuan Lou wrote:  
> >> + netdev->ncsi_enabled = wx->ncsi_hw_supported;  
> > 
> > I don't think that enabled and supported are the same thing.
> > If server has multiple NICs or a NIC with multiple ports and
> > BMC only uses one, or even none, we shouldn't keep the PHY up.
> > By that logic 99% of server NICs should report NCSI as enabled.
> 
> For a NIC with multiple ports, BMC switch connection for port0 to port1 online,
> Drivers can not know port1 should keep up, if do not set ncsi_enabled before. 

I'm not crystal clear on what you're saying. But BMC sends a enable
command to the NIC to enable a channel (or some such). This is all
handled by FW. The FW can tell the host that the NCSI is now active
on port1 and not port0.

Re: [PATCH net-next 1/2] net: ngbe: add ncsi_enable flag for wangxun nics

[mengyuanlou]

> 2023年7月26日 10:44，Jakub Kicinski <kuba@kernel.org> 写道：
> 
> On Wed, 26 Jul 2023 09:59:15 +0800 mengyuanlou@net-swift.com wrote:
>>> 2023年7月26日 07:22，Jakub Kicinski <kuba@kernel.org> 写道：
>>> On Mon, 24 Jul 2023 17:24:58 +0800 Mengyuan Lou wrote:  
>>>> + netdev->ncsi_enabled = wx->ncsi_hw_supported;  
>>> 
>>> I don't think that enabled and supported are the same thing.
>>> If server has multiple NICs or a NIC with multiple ports and
>>> BMC only uses one, or even none, we shouldn't keep the PHY up.
>>> By that logic 99% of server NICs should report NCSI as enabled.
>> 
>> For a NIC with multiple ports, BMC switch connection for port0 to port1 online,
>> Drivers can not know port1 should keep up, if do not set ncsi_enabled before. 
> 
> I'm not crystal clear on what you're saying. But BMC sends a enable
> command to the NIC to enable a channel (or some such). This is all
> handled by FW. The FW can tell the host that the NCSI is now active
> on port1 and not port0.
> 
> 
Ok, I think I understand.
Thanks.

Another question.
Then, after drivers know that portx is using for BMC, it is necessary to
let phy to know this port should not be suspended?
I mean this patch 2/2 is useful.

Re: [PATCH net-next 1/2] net: ngbe: add ncsi_enable flag for wangxun nics

[Jakub Kicinski]

On Wed, 26 Jul 2023 11:12:41 +0800 mengyuanlou@net-swift.com wrote:
> Another question.
> Then, after drivers know that portx is using for BMC, it is necessary to
> let phy to know this port should not be suspended?
> I mean this patch 2/2 is useful.

Right, I think being more selective about which port sets
netdev->ncsi_enabled is independent from patch 2. Some form
of patch 2 is still needed, but how exactly it should look
is up to the PHYLIB maintainers.

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

[Mengyuan Lou]

Add flag keep_data_connection to struct phydev indicating whether
phy need to keep data connection.
Phy_suspend() will use it to decide whether PHY can be suspended
or not.

Signed-off-by: Mengyuan Lou <mengyuanlou@net-swift.com>
---
 drivers/net/phy/phy_device.c | 6 ++++--
 include/linux/phy.h          | 3 +++
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 0c2014accba7..4fe26660458e 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1860,8 +1860,10 @@ int phy_suspend(struct phy_device *phydev)
 
 	phy_ethtool_get_wol(phydev, &wol);
 	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))
+	phydev->keep_data_connection = phydev->wol_enabled ||
+				       (netdev && netdev->ncsi_enabled);
+	/* We cannot suspend the PHY, when phy and mac need to receive packets. */
+	if (phydev->keep_data_connection && !(phydrv->flags & PHY_ALWAYS_CALL_SUSPEND))
 		return -EBUSY;
 
 	if (!phydrv || !phydrv->suspend)
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 11c1e91563d4..bda646e7cc23 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -554,6 +554,8 @@ struct macsec_ops;
  * @mac_managed_pm: Set true if MAC driver takes of suspending/resuming PHY
  * @wol_enabled: Set to true if the PHY or the attached MAC have Wake-on-LAN
  * 		 enabled.
+ * @keep_data_connection: Set to true if the PHY or the attached MAC need
+ *                        physical connection to receive packets.
  * @state: State of the PHY for management purposes
  * @dev_flags: Device-specific flags used by the PHY driver.
  *
@@ -651,6 +653,7 @@ struct phy_device {
 	unsigned is_on_sfp_module:1;
 	unsigned mac_managed_pm:1;
 	unsigned wol_enabled:1;
+	unsigned keep_data_connection:1;
 
 	unsigned autoneg:1;
 	/* The most recently read link state */
-- 
2.41.0

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

[Simon Horman]

+ Jakub Kicinski, "Russell King (Oracle)", "David S. Miller", Paolo Abeni,
  Eric Dumazet, Heiner Kallweit, Andrew Lunn

On Mon, Jul 24, 2023 at 05:24:59PM +0800, Mengyuan Lou wrote:
> Add flag keep_data_connection to struct phydev indicating whether
> phy need to keep data connection.
> Phy_suspend() will use it to decide whether PHY can be suspended
> or not.

This feels like a bug fix.
What is the behaviour of the system without this change?

> Signed-off-by: Mengyuan Lou <mengyuanlou@net-swift.com>
> ---
>  drivers/net/phy/phy_device.c | 6 ++++--
>  include/linux/phy.h          | 3 +++
>  2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 0c2014accba7..4fe26660458e 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -1860,8 +1860,10 @@ int phy_suspend(struct phy_device *phydev)
>  
>  	phy_ethtool_get_wol(phydev, &wol);
>  	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))
> +	phydev->keep_data_connection = phydev->wol_enabled ||
> +				       (netdev && netdev->ncsi_enabled);
> +	/* We cannot suspend the PHY, when phy and mac need to receive packets. */
> +	if (phydev->keep_data_connection && !(phydrv->flags & PHY_ALWAYS_CALL_SUSPEND))

As it stands, it seems that keep_data_connection is only used in this
function. Could it be a local variable rather than field of struct
phy_device.

That said, I think Russell and Andrew will likely have a deeper insight here.

>  		return -EBUSY;
>  
>  	if (!phydrv || !phydrv->suspend)
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 11c1e91563d4..bda646e7cc23 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -554,6 +554,8 @@ struct macsec_ops;
>   * @mac_managed_pm: Set true if MAC driver takes of suspending/resuming PHY
>   * @wol_enabled: Set to true if the PHY or the attached MAC have Wake-on-LAN
>   * 		 enabled.
> + * @keep_data_connection: Set to true if the PHY or the attached MAC need
> + *                        physical connection to receive packets.
>   * @state: State of the PHY for management purposes
>   * @dev_flags: Device-specific flags used by the PHY driver.
>   *
> @@ -651,6 +653,7 @@ struct phy_device {
>  	unsigned is_on_sfp_module:1;
>  	unsigned mac_managed_pm:1;
>  	unsigned wol_enabled:1;
> +	unsigned keep_data_connection:1;
>  
>  	unsigned autoneg:1;
>  	/* The most recently read link state */
> -- 
> 2.41.0
> 
> 

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

[Russell King (Oracle)]

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!

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

[mengyuanlou]

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

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

[Russell King (Oracle)]

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!

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

[Andrew Lunn]

> 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

As far as i understand it, the host MAC is actually a switch, with the
BMC connected to the second port of the switch. Does the BMC care
about the PHY status? Does it need to know about link status?  Does
the NCSI core on the host need to know about the PHY?

You might want to take a step back and think about this in general. Do
we need to extend the phylink core to support NCSI? Do we need an API
for NCSI?

    Andrew

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

[Jakub Kicinski]

Sorry for chiming in, hopefully the comments are helpful..

On Wed, 26 Jul 2023 10:54:25 +0200 Andrew Lunn wrote:
> As far as i understand it, the host MAC is actually a switch, with the
> BMC connected to the second port of the switch.

Not a learning switch (usually, sigh), but yes.

> Does the BMC care about the PHY status?
> Does it need to know about link status? 

Yes, NIC sends link state notifications over the NCSI "link?" (which 
is a separate RGMII?/RMII from NIC to the BMC). BMC can select which
"channel" (NIC port) it uses based on PHY status.

> Does the NCSI core on the host need to know about the PHY?

There is no NCSI core on the host.. Hosts are currently completely
oblivious to NCSI. The NCSI we have in tree is for the BMC, Linux
running on the BMC (e.g. OpenBMC).

> You might want to take a step back and think about this in general. Do
> we need to extend the phylink core to support NCSI? Do we need an API
> for NCSI?

Today it's mostly configured via "BIOS". But I think letting user know
that the link is shared with NCSI would be useful.

Last week someone was asking me why a certain NIC is "weird and shuts
down its PHY when ifdown'ed". I'm guessing some sysadmins may be so used
to NCSI keeping links up they come to expect it, without understanding
why it happens :(

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

[Andrew Lunn]

On Wed, Jul 26, 2023 at 09:08:12AM -0700, Jakub Kicinski wrote:
> Sorry for chiming in, hopefully the comments are helpful..

Thanks for commenting. This is a somewhat unusual setup, a server
style Ethernet interface which Linux is driving, not firmware. So its
more like an embedded SoC setup, but has NCSI which embedded systems
don't.

> On Wed, 26 Jul 2023 10:54:25 +0200 Andrew Lunn wrote:
> > As far as i understand it, the host MAC is actually a switch, with the
> > BMC connected to the second port of the switch.
> 
> Not a learning switch (usually, sigh), but yes.
> 
> > Does the BMC care about the PHY status?
> > Does it need to know about link status? 
> 
> Yes, NIC sends link state notifications over the NCSI "link?" (which 
> is a separate RGMII?/RMII from NIC to the BMC). BMC can select which
> "channel" (NIC port) it uses based on PHY status.

How do you define NIC when Linux is driving the hardware, not
firmware? In this case we have a MAC driver, a PCS driver, a PHY
driver and phylink gluing it all together. Which part of this is
sending link state notifications over the NCSI "Link?".
 
> > Does the NCSI core on the host need to know about the PHY?
> 
> There is no NCSI core on the host.. Hosts are currently completely
> oblivious to NCSI. The NCSI we have in tree is for the BMC, Linux
> running on the BMC (e.g. OpenBMC).

But in this case, it is not oblivious to NCSI, since the host is
controlling the PHY. This patch is about Linux on the host not
shutting down the PHY because it is also being used by the BMC.

	 Andrew

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

[Jakub Kicinski]

On Wed, 26 Jul 2023 18:43:01 +0200 Andrew Lunn wrote:
> On Wed, Jul 26, 2023 at 09:08:12AM -0700, Jakub Kicinski wrote:
> > On Wed, 26 Jul 2023 10:54:25 +0200 Andrew Lunn wrote:  
> > > As far as i understand it, the host MAC is actually a switch, with the
> > > BMC connected to the second port of the switch.  
> > 
> > Not a learning switch (usually, sigh), but yes.
> >   
> > > Does the BMC care about the PHY status?
> > > Does it need to know about link status?   
> > 
> > Yes, NIC sends link state notifications over the NCSI "link?" (which 
> > is a separate RGMII?/RMII from NIC to the BMC). BMC can select which
> > "channel" (NIC port) it uses based on PHY status.  
> 
> How do you define NIC when Linux is driving the hardware, not
> firmware? In this case we have a MAC driver, a PCS driver, a PHY
> driver and phylink gluing it all together. Which part of this is
> sending link state notifications over the NCSI "Link?".

I've never seen a NCSI setup where Linux on the host controls the PHY.
So it's an open question.

The notifications are sent by the control FW on the NIC. There's a
handful of commands that need to be handled there - mostly getting MAC
address and configuring the filter. Commands are carried by
encapsulated Ethernet packets with magic ethertype, over the same RMII
as the data packets.

All of this is usually in FW so we should be able to shape the
implementation in the way we want...

> > > Does the NCSI core on the host need to know about the PHY?  
> > 
> > There is no NCSI core on the host.. Hosts are currently completely
> > oblivious to NCSI. The NCSI we have in tree is for the BMC, Linux
> > running on the BMC (e.g. OpenBMC).  
> 
> But in this case, it is not oblivious to NCSI, since the host is
> controlling the PHY. This patch is about Linux on the host not
> shutting down the PHY because it is also being used by the BMC.

Yup, we're entering a new territory with this device :S

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

[mengyuanlou]

> 2023年7月27日 02:29，Jakub Kicinski <kuba@kernel.org> 写道：
> 
> On Wed, 26 Jul 2023 18:43:01 +0200 Andrew Lunn wrote:
>> On Wed, Jul 26, 2023 at 09:08:12AM -0700, Jakub Kicinski wrote:
>>> On Wed, 26 Jul 2023 10:54:25 +0200 Andrew Lunn wrote:  
>>>> As far as i understand it, the host MAC is actually a switch, with the
>>>> BMC connected to the second port of the switch.  
>>> 
>>> Not a learning switch (usually, sigh), but yes.
>>> 
>>>> Does the BMC care about the PHY status?
>>>> Does it need to know about link status?   
>>> 
>>> Yes, NIC sends link state notifications over the NCSI "link?" (which 
>>> is a separate RGMII?/RMII from NIC to the BMC). BMC can select which
>>> "channel" (NIC port) it uses based on PHY status.  
>> 
>> How do you define NIC when Linux is driving the hardware, not
>> firmware? In this case we have a MAC driver, a PCS driver, a PHY
>> driver and phylink gluing it all together. Which part of this is
>> sending link state notifications over the NCSI "Link?".
> 
> I've never seen a NCSI setup where Linux on the host controls the PHY.
> So it's an open question.
> 
> The notifications are sent by the control FW on the NIC. There's a
> handful of commands that need to be handled there - mostly getting MAC
> address and configuring the filter. Commands are carried by
> encapsulated Ethernet packets with magic ethertype, over the same RMII
> as the data packets.
> 
> All of this is usually in FW so we should be able to shape the
> implementation in the way we want...
> 
We certainly can do all phy operations in Fw when we are using NCSI.

I’m confused about what other network cards do when it's used in both host os and BMC,
Because I can not find any codes in ethernet for now.

But this seems to go against the idea that we want to separate these modules.

>>>> Does the NCSI core on the host need to know about the PHY?  
>>> 
>>> There is no NCSI core on the host.. Hosts are currently completely
>>> oblivious to NCSI. The NCSI we have in tree is for the BMC, Linux
>>> running on the BMC (e.g. OpenBMC).  
>> 
>> But in this case, it is not oblivious to NCSI, since the host is
>> controlling the PHY. This patch is about Linux on the host not
>> shutting down the PHY because it is also being used by the BMC.
> 
> Yup, we're entering a new territory with this device :S
> 

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

[Andrew Lunn]

> > All of this is usually in FW so we should be able to shape the
> > implementation in the way we want...
> > 
> We certainly can do all phy operations in Fw when we are using NCSI.

I would actually prefer Linux does it, not firmware. My personal
preference is also Linux driver the hardware, since it is then
possible for the community to debug it, extend it with new
functionality, etc. Firmware is a black box only the vendor can do
anything with.

But as Jakub points out, we are entering a new territory here with
your device. All the other host devices which support NCSI have
firmware driving the hardware, not Linux. This is why you cannot find
code to copy. You need to actually write the host side of the NCSI
protocol, and figure out what the API to phylink should be, etc.

	Andrew

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

[Jakub Kicinski]

On Fri, 28 Jul 2023 11:48:36 +0200 Andrew Lunn wrote:
> > > All of this is usually in FW so we should be able to shape the
> > > implementation in the way we want...
> > >   
> > We certainly can do all phy operations in Fw when we are using NCSI.  
> 
> I would actually prefer Linux does it, not firmware. My personal
> preference is also Linux driver the hardware, since it is then
> possible for the community to debug it, extend it with new
> functionality, etc. Firmware is a black box only the vendor can do
> anything with.

Just to be clear, my comment was more about NCSI commands (which
I don't think should be handled by the host), not PHY control.

Re: [PATCH net-next 1/2] net: ngbe: add ncsi_enable flag for wangxun nics

[Simon Horman]

On Mon, Jul 24, 2023 at 05:24:59PM +0800, Mengyuan Lou wrote:

+ Jakub Kicinski, "Russell King (Oracle)", "David S. Miller", Paolo Abeni,
  Eric Dumazet, Heiner Kallweit, Andrew Lunn, Jiawen Wu

  Please use ./scripts/get_maintainer.pl --git-min-percent 25 this.patch
  to determine the CC list for Networking patches

> Add ncsi_enabled flag to struct netdev to indicate wangxun
> nics which support NCSI.

This patch adds ncsi_enabled to struct net_device.
Which does raise the question of if other NICs support NCSI,
and if so how they do so without this field.

This patch also renames an existing field in struct wx.
This is not reflected in the patch description.

> Signed-off-by: Mengyuan Lou <mengyuanlou@net-swift.com>
> ---
>  drivers/net/ethernet/wangxun/libwx/wx_type.h  | 2 +-
>  drivers/net/ethernet/wangxun/ngbe/ngbe_main.c | 5 +++--
>  include/linux/netdevice.h                     | 3 +++
>  3 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/wangxun/libwx/wx_type.h b/drivers/net/ethernet/wangxun/libwx/wx_type.h
> index 1de88a33a698..1b932e66044e 100644
> --- a/drivers/net/ethernet/wangxun/libwx/wx_type.h
> +++ b/drivers/net/ethernet/wangxun/libwx/wx_type.h
> @@ -851,7 +851,7 @@ struct wx {
>  	struct phy_device *phydev;
>  
>  	bool wol_hw_supported;
> -	bool ncsi_enabled;
> +	bool ncsi_hw_supported;
>  	bool gpio_ctrl;
>  	raw_spinlock_t gpio_lock;
>  
> diff --git a/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c b/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
> index 2b431db6085a..e42e4dd26700 100644
> --- a/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
> +++ b/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
> @@ -63,8 +63,8 @@ static void ngbe_init_type_code(struct wx *wx)
>  		       em_mac_type_mdi;
>  
>  	wx->wol_hw_supported = (wol_mask == NGBE_WOL_SUP) ? 1 : 0;
> -	wx->ncsi_enabled = (ncsi_mask == NGBE_NCSI_MASK ||
> -			   type_mask == NGBE_SUBID_OCP_CARD) ? 1 : 0;
> +	wx->ncsi_hw_supported = (ncsi_mask == NGBE_NCSI_MASK ||
> +				 type_mask == NGBE_SUBID_OCP_CARD) ? 1 : 0;
>  
>  	switch (type_mask) {
>  	case NGBE_SUBID_LY_YT8521S_SFP:
> @@ -639,6 +639,7 @@ static int ngbe_probe(struct pci_dev *pdev,
>  	netdev->wol_enabled = !!(wx->wol);
>  	wr32(wx, NGBE_PSR_WKUP_CTL, wx->wol);
>  	device_set_wakeup_enable(&pdev->dev, wx->wol);
> +	netdev->ncsi_enabled = wx->ncsi_hw_supported;
>  
>  	/* Save off EEPROM version number and Option Rom version which
>  	 * together make a unique identify for the eeprom
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index b828c7a75be2..dfa14e4c8e95 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -2024,6 +2024,8 @@ enum netdev_ml_priv_type {
>   *
>   *	@wol_enabled:	Wake-on-LAN is enabled
>   *
> + *	@ncsi_enabled:	NCSI is enabled.
> + *
>   *	@threaded:	napi threaded mode is enabled
>   *
>   *	@net_notifier_list:	List of per-net netdev notifier block
> @@ -2393,6 +2395,7 @@ struct net_device {
>  	struct lock_class_key	*qdisc_tx_busylock;
>  	bool			proto_down;
>  	unsigned		wol_enabled:1;
> +	unsigned		ncsi_enabled:1;
>  	unsigned		threaded:1;
>  
>  	struct list_head	net_notifier_list;
> -- 
> 2.41.0
> 
>