| Message ID | 20207E0578DCE44C+20230724092544.73531-3-mengyuanlou@net-swift.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Wangxun ngbe nics nsci support | expand |
+ 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 > >
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 > >
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.
> 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.
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.
> 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
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 :(
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
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
> 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 >
> > 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
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.
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 */
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(-)