| 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
| ||
|
Message-ID: <20231002095946.GA21694@gmail.com>
Date: Mon, 2 Oct 2023 10:59:46 +0100
From: Martin Habets <habetsm.xilinx@...il.com>
To: edward.cree@....com
Cc: linux-net-drivers@....com, davem@...emloft.net, kuba@...nel.org,
edumazet@...gle.com, pabeni@...hat.com,
Edward Cree <ecree.xilinx@...il.com>, netdev@...r.kernel.org,
sudheer.mogilappagari@...el.com, jdamato@...tly.com, andrew@...n.ch,
mw@...ihalf.com, linux@...linux.org.uk, sgoutham@...vell.com,
gakula@...vell.com, sbhatta@...vell.com, hkelam@...vell.com,
saeedm@...dia.com, leon@...nel.org, hkallweit1@...il.com,
nic_swsd@...ltek.com, jiawenwu@...stnetic.com,
mengyuanlou@...-swift.com
Subject: Re: [PATCH v4 net-next 1/7] net: move ethtool-related netdev state
into its own struct
On Wed, Sep 27, 2023 at 07:13:32PM +0100, edward.cree@....com wrote:
> From: Edward Cree <ecree.xilinx@...il.com>
>
> net_dev->ethtool is a pointer to new struct ethtool_netdev_state, which
> currently contains only the wol_enabled field.
>
> Suggested-by: Jakub Kicinski <kuba@...nel.org>
> Signed-off-by: Edward Cree <ecree.xilinx@...il.com>
One small nit below.
Reviewed-by: Martin Habets <habetsm.xilinx@...il.com>
> ---
> drivers/net/ethernet/realtek/r8169_main.c | 4 ++--
> drivers/net/ethernet/wangxun/ngbe/ngbe_ethtool.c | 4 ++--
> drivers/net/ethernet/wangxun/ngbe/ngbe_main.c | 2 +-
> drivers/net/phy/phy.c | 2 +-
> drivers/net/phy/phy_device.c | 5 +++--
> drivers/net/phy/phylink.c | 2 +-
> include/linux/ethtool.h | 8 ++++++++
> include/linux/netdevice.h | 7 ++++---
> net/core/dev.c | 4 ++++
> net/ethtool/ioctl.c | 2 +-
> net/ethtool/wol.c | 2 +-
> 11 files changed, 28 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
> index 6351a2dc13bc..fe69416f2a93 100644
> --- a/drivers/net/ethernet/realtek/r8169_main.c
> +++ b/drivers/net/ethernet/realtek/r8169_main.c
> @@ -1455,7 +1455,7 @@ static void __rtl8169_set_wol(struct rtl8169_private *tp, u32 wolopts)
>
> if (tp->dash_type == RTL_DASH_NONE) {
> rtl_set_d3_pll_down(tp, !wolopts);
> - tp->dev->wol_enabled = wolopts ? 1 : 0;
> + tp->dev->ethtool->wol_enabled = wolopts ? 1 : 0;
> }
> }
>
> @@ -5321,7 +5321,7 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
> rtl_set_d3_pll_down(tp, true);
> } else {
> rtl_set_d3_pll_down(tp, false);
> - dev->wol_enabled = 1;
> + dev->ethtool->wol_enabled = 1;
> }
>
> jumbo_max = rtl_jumbo_max(tp);
> diff --git a/drivers/net/ethernet/wangxun/ngbe/ngbe_ethtool.c b/drivers/net/ethernet/wangxun/ngbe/ngbe_ethtool.c
> index ec0e869e9aac..091ee3d3e74d 100644
> --- a/drivers/net/ethernet/wangxun/ngbe/ngbe_ethtool.c
> +++ b/drivers/net/ethernet/wangxun/ngbe/ngbe_ethtool.c
> @@ -34,9 +34,9 @@ static int ngbe_set_wol(struct net_device *netdev,
> wx->wol = 0;
> if (wol->wolopts & WAKE_MAGIC)
> wx->wol = WX_PSR_WKUP_CTL_MAG;
> - netdev->wol_enabled = !!(wx->wol);
> + netdev->ethtool->wol_enabled = !!(wx->wol);
> wr32(wx, WX_PSR_WKUP_CTL, wx->wol);
> - device_set_wakeup_enable(&pdev->dev, netdev->wol_enabled);
> + device_set_wakeup_enable(&pdev->dev, netdev->ethtool->wol_enabled);
>
> return 0;
> }
> diff --git a/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c b/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
> index 2b431db6085a..6752f8d04d9c 100644
> --- a/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
> +++ b/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
> @@ -636,7 +636,7 @@ static int ngbe_probe(struct pci_dev *pdev,
> if (wx->wol_hw_supported)
> wx->wol = NGBE_PSR_WKUP_CTL_MAG;
>
> - netdev->wol_enabled = !!(wx->wol);
> + netdev->ethtool->wol_enabled = !!(wx->wol);
> wr32(wx, NGBE_PSR_WKUP_CTL, wx->wol);
> device_set_wakeup_enable(&pdev->dev, wx->wol);
>
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index a5fa077650e8..32bfea9b5c6c 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -1282,7 +1282,7 @@ static irqreturn_t phy_interrupt(int irq, void *phy_dat)
> if (netdev) {
> struct device *parent = netdev->dev.parent;
>
> - if (netdev->wol_enabled)
> + if (netdev->ethtool->wol_enabled)
> pm_system_wakeup();
> else if (device_may_wakeup(&netdev->dev))
> pm_wakeup_dev_event(&netdev->dev, 0, true);
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 2ce74593d6e4..62afc0424fbd 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -285,7 +285,7 @@ static bool mdio_bus_phy_may_suspend(struct phy_device *phydev)
> if (!netdev)
> goto out;
>
> - if (netdev->wol_enabled)
> + if (netdev->ethtool->wol_enabled)
> return false;
>
> /* As long as not all affected network drivers support the
> @@ -1859,7 +1859,8 @@ int phy_suspend(struct phy_device *phydev)
> return 0;
>
> phy_ethtool_get_wol(phydev, &wol);
> - phydev->wol_enabled = wol.wolopts || (netdev && netdev->wol_enabled);
> + phydev->wol_enabled = wol.wolopts ||
> + (netdev && netdev->ethtool->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;
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 0d7354955d62..b808ba1197c3 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -2172,7 +2172,7 @@ void phylink_suspend(struct phylink *pl, bool mac_wol)
> {
> ASSERT_RTNL();
>
> - if (mac_wol && (!pl->netdev || pl->netdev->wol_enabled)) {
> + if (mac_wol && (!pl->netdev || pl->netdev->ethtool->wol_enabled)) {
> /* Wake-on-Lan enabled, MAC handling */
> mutex_lock(&pl->state_mutex);
>
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index 62b61527bcc4..8aeefc0b4e10 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -935,6 +935,14 @@ int ethtool_virtdev_set_link_ksettings(struct net_device *dev,
> const struct ethtool_link_ksettings *cmd,
> u32 *dev_speed, u8 *dev_duplex);
>
> +/**
> + * struct ethtool_netdev_state - per-netdevice state for ethtool features
> + * @wol_enabled: Wake-on-LAN is enabled
> + */
> +struct ethtool_netdev_state {
> + unsigned wol_enabled:1;
The use of bool seems to be quite well established in the kernel these
days. I suggest you use that.
Martin
> +};
> +
> struct phy_device;
> struct phy_tdr_config;
> struct phy_plca_cfg;
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 7e520c14eb8c..05ea6cb56800 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -79,6 +79,7 @@ struct xdp_buff;
> struct xdp_frame;
> struct xdp_metadata_ops;
> struct xdp_md;
> +struct ethtool_netdev_state;
> /* DPLL specific */
> struct dpll_pin;
>
> @@ -2014,8 +2015,6 @@ enum netdev_ml_priv_type {
> * switch driver and used to set the phys state of the
> * switch port.
> *
> - * @wol_enabled: Wake-on-LAN is enabled
> - *
> * @threaded: napi threaded mode is enabled
> *
> * @net_notifier_list: List of per-net netdev notifier block
> @@ -2027,6 +2026,7 @@ enum netdev_ml_priv_type {
> * @udp_tunnel_nic_info: static structure describing the UDP tunnel
> * offload capabilities of the device
> * @udp_tunnel_nic: UDP tunnel offload state
> + * @ethtool: ethtool related state
> * @xdp_state: stores info on attached XDP BPF programs
> *
> * @nested_level: Used as a parameter of spin_lock_nested() of
> @@ -2389,7 +2389,6 @@ struct net_device {
> struct sfp_bus *sfp_bus;
> struct lock_class_key *qdisc_tx_busylock;
> bool proto_down;
> - unsigned wol_enabled:1;
> unsigned threaded:1;
>
> struct list_head net_notifier_list;
> @@ -2401,6 +2400,8 @@ struct net_device {
> const struct udp_tunnel_nic_info *udp_tunnel_nic_info;
> struct udp_tunnel_nic *udp_tunnel_nic;
>
> + struct ethtool_netdev_state *ethtool;
> +
> /* protected by rtnl_lock */
> struct bpf_xdp_entity xdp_state[__MAX_XDP_MODE];
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 606a366cc209..9e85a71e33ed 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -10769,6 +10769,9 @@ struct net_device *alloc_netdev_mqs(int sizeof_priv, const char *name,
> dev->real_num_rx_queues = rxqs;
> if (netif_alloc_rx_queues(dev))
> goto free_all;
> + dev->ethtool = kzalloc(sizeof(*dev->ethtool), GFP_KERNEL_ACCOUNT);
> + if (!dev->ethtool)
> + goto free_all;
>
> strcpy(dev->name, name);
> dev->name_assign_type = name_assign_type;
> @@ -10819,6 +10822,7 @@ void free_netdev(struct net_device *dev)
> return;
> }
>
> + kfree(dev->ethtool);
> netif_free_tx_queues(dev);
> netif_free_rx_queues(dev);
>
> diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
> index 0b0ce4f81c01..de78b24fffc9 100644
> --- a/net/ethtool/ioctl.c
> +++ b/net/ethtool/ioctl.c
> @@ -1461,7 +1461,7 @@ static int ethtool_set_wol(struct net_device *dev, char __user *useraddr)
> if (ret)
> return ret;
>
> - dev->wol_enabled = !!wol.wolopts;
> + dev->ethtool->wol_enabled = !!wol.wolopts;
> ethtool_notify(dev, ETHTOOL_MSG_WOL_NTF, NULL);
>
> return 0;
> diff --git a/net/ethtool/wol.c b/net/ethtool/wol.c
> index 0ed56c9ac1bc..a39d8000d808 100644
> --- a/net/ethtool/wol.c
> +++ b/net/ethtool/wol.c
> @@ -137,7 +137,7 @@ ethnl_set_wol(struct ethnl_req_info *req_info, struct genl_info *info)
> ret = dev->ethtool_ops->set_wol(dev, &wol);
> if (ret)
> return ret;
> - dev->wol_enabled = !!wol.wolopts;
> + dev->ethtool->wol_enabled = !!wol.wolopts;
> return 1;
> }
>
Powered by blists - more mailing lists