Message-ID: <3b65d8d8d90dd7c2d29a007b4b1746950d6fc788.camel@gmail.com>
Date:   Mon, 06 Feb 2023 08:04:30 -0800
From:   Alexander H Duyck <alexander.duyck@...il.com>
To:     Hao Lan <lanhao@...wei.com>, andrew@...n.ch, davem@...emloft.net,
        kuba@...nel.org
Cc:     yisen.zhuang@...wei.com, salil.mehta@...wei.com,
        edumazet@...gle.com, pabeni@...hat.com, richardcochran@...il.com,
        shenjian15@...wei.com, netdev@...r.kernel.org,
        wangjie125@...wei.com
Subject: Re: [PATCH v2 net-next] net: hns3: support wake on lan
 configuration and query

On Mon, 2023-02-06 at 22:06 +0800, Hao Lan wrote:
> HNS3 (HiSilicon Network System 3) supports Wake-on-LAN,
> magic mode and magic security mode on each pf.
> This patch supports the ethtool LAN wake-up configuration and
> query interfaces.
> It does not support the suspend resume interface because
> there is no corresponding application scenario.
> 
> Signed-off-by: Hao Lan <lanhao@...wei.com>
> ---
>  drivers/net/ethernet/hisilicon/hns3/hnae3.h   |  12 ++
>  .../hns3/hns3_common/hclge_comm_cmd.c         |   1 +
>  .../hns3/hns3_common/hclge_comm_cmd.h         |   3 +
>  .../ethernet/hisilicon/hns3/hns3_debugfs.c    |   3 +
>  .../ethernet/hisilicon/hns3/hns3_ethtool.c    |  30 ++++
>  .../hisilicon/hns3/hns3pf/hclge_cmd.h         |  14 ++
>  .../hisilicon/hns3/hns3pf/hclge_main.c        | 149 ++++++++++++++++++
>  .../hisilicon/hns3/hns3pf/hclge_main.h        |   8 +
>  8 files changed, 220 insertions(+)
> 
> diff --git a/drivers/net/ethernet/hisilicon/hns3/hnae3.h b/drivers/net/ethernet/hisilicon/hns3/hnae3.h
> index 17137de9338c..312ac1cccd39 100644
> --- a/drivers/net/ethernet/hisilicon/hns3/hnae3.h
> +++ b/drivers/net/ethernet/hisilicon/hns3/hnae3.h
> @@ -99,6 +99,7 @@ enum HNAE3_DEV_CAP_BITS {
>  	HNAE3_DEV_SUPPORT_CQ_B,
>  	HNAE3_DEV_SUPPORT_FEC_STATS_B,
>  	HNAE3_DEV_SUPPORT_LANE_NUM_B,
> +	HNAE3_DEV_SUPPORT_WOL_B,
>  };
>  
>  #define hnae3_ae_dev_fd_supported(ae_dev) \
> @@ -167,6 +168,9 @@ enum HNAE3_DEV_CAP_BITS {
>  #define hnae3_ae_dev_lane_num_supported(ae_dev) \
>  	test_bit(HNAE3_DEV_SUPPORT_LANE_NUM_B, (ae_dev)->caps)
>  
> +#define hnae3_ae_dev_wol_supported(ae_dev) \
> +	test_bit(HNAE3_DEV_SUPPORT_WOL_B, (ae_dev)->caps)
> +
>  enum HNAE3_PF_CAP_BITS {
>  	HNAE3_PF_SUPPORT_VLAN_FLTR_MDF_B = 0,
>  };
> @@ -560,6 +564,10 @@ struct hnae3_ae_dev {
>   *   Get phc info
>   * clean_vf_config
>   *   Clean residual vf info after disable sriov
> + * get_wol
> + *   Get wake on lan info
> + * set_wol
> + *   Config wake on lan
>   */
>  struct hnae3_ae_ops {
>  	int (*init_ae_dev)(struct hnae3_ae_dev *ae_dev);
> @@ -759,6 +767,10 @@ struct hnae3_ae_ops {
>  	void (*clean_vf_config)(struct hnae3_ae_dev *ae_dev, int num_vfs);
>  	int (*get_dscp_prio)(struct hnae3_handle *handle, u8 dscp,
>  			     u8 *tc_map_mode, u8 *priority);
> +	void (*get_wol)(struct hnae3_handle *handle,
> +			struct ethtool_wolinfo *wol);
> +	int (*set_wol)(struct hnae3_handle *handle,
> +		       struct ethtool_wolinfo *wol);
>  };
>  
>  struct hnae3_dcb_ops {
> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_common/hclge_comm_cmd.c b/drivers/net/ethernet/hisilicon/hns3/hns3_common/hclge_comm_cmd.c
> index f671a63cecde..cbbab5b2b402 100644
> --- a/drivers/net/ethernet/hisilicon/hns3/hns3_common/hclge_comm_cmd.c
> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3_common/hclge_comm_cmd.c
> @@ -155,6 +155,7 @@ static const struct hclge_comm_caps_bit_map hclge_pf_cmd_caps[] = {
>  	{HCLGE_COMM_CAP_FD_B, HNAE3_DEV_SUPPORT_FD_B},
>  	{HCLGE_COMM_CAP_FEC_STATS_B, HNAE3_DEV_SUPPORT_FEC_STATS_B},
>  	{HCLGE_COMM_CAP_LANE_NUM_B, HNAE3_DEV_SUPPORT_LANE_NUM_B},
> +	{HCLGE_COMM_CAP_WOL_B, HNAE3_DEV_SUPPORT_WOL_B},
>  };
>  
>  static const struct hclge_comm_caps_bit_map hclge_vf_cmd_caps[] = {
> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_common/hclge_comm_cmd.h b/drivers/net/ethernet/hisilicon/hns3/hns3_common/hclge_comm_cmd.h
> index b1f9383b418f..de72ecbfd5ad 100644
> --- a/drivers/net/ethernet/hisilicon/hns3/hns3_common/hclge_comm_cmd.h
> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3_common/hclge_comm_cmd.h
> @@ -294,6 +294,8 @@ enum hclge_opcode_type {
>  	HCLGE_PPP_CMD0_INT_CMD		= 0x2100,
>  	HCLGE_PPP_CMD1_INT_CMD		= 0x2101,
>  	HCLGE_MAC_ETHERTYPE_IDX_RD      = 0x2105,
> +	HCLGE_OPC_WOL_GET_SUPPORTED_MODE	= 0x2201,
> +	HCLGE_OPC_WOL_CFG		= 0x2202,
>  	HCLGE_NCSI_INT_EN		= 0x2401,
>  
>  	/* ROH MAC commands */
> @@ -345,6 +347,7 @@ enum HCLGE_COMM_CAP_BITS {
>  	HCLGE_COMM_CAP_FD_B = 21,
>  	HCLGE_COMM_CAP_FEC_STATS_B = 25,
>  	HCLGE_COMM_CAP_LANE_NUM_B = 27,
> +	HCLGE_COMM_CAP_WOL_B = 28,
>  };
>  
>  enum HCLGE_COMM_API_CAP_BITS {
> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_debugfs.c b/drivers/net/ethernet/hisilicon/hns3/hns3_debugfs.c
> index 66feb23f7b7b..4c3e90a1c4d0 100644
> --- a/drivers/net/ethernet/hisilicon/hns3/hns3_debugfs.c
> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3_debugfs.c
> @@ -408,6 +408,9 @@ static struct hns3_dbg_cap_info hns3_dbg_cap[] = {
>  	}, {
>  		.name = "support lane num",
>  		.cap_bit = HNAE3_DEV_SUPPORT_LANE_NUM_B,
> +	}, {
> +		.name = "support wake on lan",
> +		.cap_bit = HNAE3_DEV_SUPPORT_WOL_B,
>  	}
>  };
>  
> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c b/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
> index 55306fe8a540..da766461d4ad 100644
> --- a/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
> @@ -2063,6 +2063,34 @@ static int hns3_get_link_ext_state(struct net_device *netdev,
>  	return -ENODATA;
>  }
>  
> +static void hns3_get_wol(struct net_device *netdev, struct ethtool_wolinfo *wol)
> +{
> +	struct hnae3_handle *handle = hns3_get_handle(netdev);
> +	struct hnae3_ae_dev *ae_dev = pci_get_drvdata(handle->pdev);
> +	const struct hnae3_ae_ops *ops = handle->ae_algo->ops;
> +
> +	if (!hnae3_ae_dev_wol_supported(ae_dev) || !ops->get_wol || !wol)
> +		return;
> +

As far as the !wol that is some defensive programming that is likely
not necessary. If ethtool is passing NULL values for that pointer we
likely have a serious programming issue in the kernel anyway.

Also in the !ops->get_wol case it would seem like we should be
indicating that we don't support wol. Perhaps it would make sense to
set wol->supported to 0 at the start of this funciton so that it is set
in all return cases.

> +	ops->get_wol(handle, wol);
> +}
> +
> +static int hns3_set_wol(struct net_device *netdev,
> +			struct ethtool_wolinfo *wol)
> +{
> +	struct hnae3_handle *handle = hns3_get_handle(netdev);
> +	struct hnae3_ae_dev *ae_dev = pci_get_drvdata(handle->pdev);
> +	const struct hnae3_ae_ops *ops = handle->ae_algo->ops;
> +
> +	if (!wol)
> +		return -EINVAL;
> +

Same here. If wol is NULL then ethtool has serious problems. I probably
wouldn't bother with checking this since callers are required to
provide it.

> +	if (!hnae3_ae_dev_wol_supported(ae_dev) || !ops->set_wol)
> +		return -EOPNOTSUPP;
> +
> +	return ops->set_wol(handle, wol);
> +}
> +
>  static const struct ethtool_ops hns3vf_ethtool_ops = {
>  	.supported_coalesce_params = HNS3_ETHTOOL_COALESCE,
>  	.supported_ring_params = HNS3_ETHTOOL_RING,
> @@ -2139,6 +2167,8 @@ static const struct ethtool_ops hns3_ethtool_ops = {
>  	.set_tunable = hns3_set_tunable,
>  	.reset = hns3_set_reset,
>  	.get_link_ext_state = hns3_get_link_ext_state,
> +	.get_wol = hns3_get_wol,
> +	.set_wol = hns3_set_wol,
>  };
>  
>  void hns3_ethtool_set_ops(struct net_device *netdev)
> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_cmd.h b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_cmd.h
> index 43cada51d8cb..b2f0bbf23603 100644
> --- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_cmd.h
> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_cmd.h
> @@ -872,6 +872,20 @@ struct hclge_phy_reg_cmd {
>  	u8 rsv1[18];
>  };
>  
> +#define HCLGE_SOPASS_MAX	6
> +
> +struct hclge_wol_cfg_cmd {
> +	__le32 wake_on_lan_mode;
> +	u8 sopass[HCLGE_SOPASS_MAX];
> +	u8 sopass_size;
> +	u8 rsv[13];
> +};
> +
> +struct hclge_query_wol_supported_cmd {
> +	__le32 supported_wake_mode;
> +	u8 rsv[20];
> +};
> +
>  struct hclge_hw;
>  int hclge_cmd_send(struct hclge_hw *hw, struct hclge_desc *desc, int num);
>  enum hclge_comm_cmd_status hclge_cmd_mdio_write(struct hclge_hw *hw,
> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
> index 1853f499cd94..b34439361aca 100644
> --- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
> @@ -11533,6 +11533,143 @@ static void hclge_uninit_rxd_adv_layout(struct hclge_dev *hdev)
>  		hclge_write_dev(&hdev->hw, HCLGE_RXD_ADV_LAYOUT_EN_REG, 0);
>  }
>  
> +static int hclge_get_wol_supported_mode(struct hclge_dev *hdev,
> +					u32 *wol_supported)
> +{
> +	struct hclge_query_wol_supported_cmd *wol_supported_cmd;
> +	struct hclge_desc desc;
> +	int ret;
> +
> +	hclge_cmd_setup_basic_desc(&desc, HCLGE_OPC_WOL_GET_SUPPORTED_MODE,
> +				   true);
> +	wol_supported_cmd = (struct hclge_query_wol_supported_cmd *)desc.data;
> +
> +	ret = hclge_cmd_send(&hdev->hw, &desc, 1);
> +	if (ret) {
> +		dev_err(&hdev->pdev->dev,
> +			"failed to query wol supported, ret = %d\n", ret);
> +		return ret;
> +	}
> +
> +	*wol_supported = le32_to_cpu(wol_supported_cmd->supported_wake_mode);
> +
> +	return 0;
> +}
> +
> +static int hclge_get_wol_cfg(struct hclge_dev *hdev,
> +			     struct ethtool_wolinfo *wol)
> +{
> +	struct hclge_wol_cfg_cmd *wol_cfg_cmd;
> +	struct hclge_desc desc;
> +	int ret;
> +
> +	hclge_cmd_setup_basic_desc(&desc, HCLGE_OPC_WOL_CFG, true);
> +	ret = hclge_cmd_send(&hdev->hw, &desc, 1);
> +	if (ret) {
> +		dev_err(&hdev->pdev->dev,
> +			"failed to get wol config, ret = %d\n", ret);
> +		return ret;
> +	}
> +
> +	wol_cfg_cmd = (struct hclge_wol_cfg_cmd *)desc.data;
> +	wol->wolopts = le32_to_cpu(wol_cfg_cmd->wake_on_lan_mode);
> +	if (wol->wolopts & WAKE_MAGICSECURE)
> +		memcpy(wol->sopass, wol_cfg_cmd->sopass, HCLGE_SOPASS_MAX);
> +
> +	return 0;
> +}
> +
> +static int hclge_set_wol_cfg(struct hclge_dev *hdev,
> +			     struct hclge_wol_info *wol_info)
> +{
> +	struct hclge_wol_cfg_cmd *wol_cfg_cmd;
> +	struct hclge_desc desc;
> +	int ret;
> +
> +	hclge_cmd_setup_basic_desc(&desc, HCLGE_OPC_WOL_CFG, false);
> +	wol_cfg_cmd = (struct hclge_wol_cfg_cmd *)desc.data;
> +	wol_cfg_cmd->wake_on_lan_mode = cpu_to_le32(wol_info->wol_current_mode);
> +	wol_cfg_cmd->sopass_size = wol_info->wol_sopass_size;
> +	memcpy(wol_cfg_cmd->sopass, wol_info->wol_sopass, HCLGE_SOPASS_MAX);
> +
> +	ret = hclge_cmd_send(&hdev->hw, &desc, 1);
> +	if (ret)
> +		dev_err(&hdev->pdev->dev,
> +			"failed to set wol config, ret = %d\n", ret);
> +
> +	return ret;
> +}
> +
> +static int hclge_update_wol(struct hclge_dev *hdev)
> +{
> +	struct hclge_wol_info *wol_info = &hdev->hw.mac.wol;
> +
> +	if (!hnae3_ae_dev_wol_supported(hdev->ae_dev))
> +		return 0;
> +
> +	return hclge_set_wol_cfg(hdev, wol_info);
> +}
> +
> +static int hclge_init_wol(struct hclge_dev *hdev)
> +{
> +	struct hclge_wol_info *wol_info = &hdev->hw.mac.wol;
> +	int ret;
> +
> +	if (!hnae3_ae_dev_wol_supported(hdev->ae_dev))
> +		return 0;
> +
> +	memset(wol_info, 0, sizeof(struct hclge_wol_info));
> +	ret = hclge_get_wol_supported_mode(hdev,
> +					   &wol_info->wol_support_mode);
> +	if (ret) {
> +		wol_info->wol_support_mode = 0;
> +		return ret;
> +	}
> +
> +	return hclge_update_wol(hdev);
> +}
> +
> +static void hclge_get_wol(struct hnae3_handle *handle,
> +			  struct ethtool_wolinfo *wol)
> +{
> +	struct hclge_vport *vport = hclge_get_vport(handle);
> +	struct hclge_dev *hdev = vport->back;
> +
> +	if (hclge_get_wol_supported_mode(hdev, &wol->supported))
> +		return;
> +
> +	if (hclge_get_wol_cfg(hdev, wol))
> +		wol->supported = 0;
> +}
> +

Is there a reason for fetching this from hardware rather than using the
version you should have acquired during init that would be stored in
hclge_wol_info?

> +static int hclge_set_wol(struct hnae3_handle *handle,
> +			 struct ethtool_wolinfo *wol)
> +{
> +	struct hclge_vport *vport = hclge_get_vport(handle);
> +	struct hclge_dev *hdev = vport->back;
> +	struct hclge_wol_info *wol_info = &hdev->hw.mac.wol;
> +	u32 wol_mode;
> +	int ret;
> +
> +	wol_mode = wol->wolopts;
> +	if (wol_mode & ~wol_info->wol_support_mode)
> +		return -EINVAL;
> +
> +	wol_info->wol_current_mode = wol_mode;
> +	if (wol_mode & WAKE_MAGICSECURE) {
> +		memcpy(wol_info->wol_sopass, wol->sopass, HCLGE_SOPASS_MAX);
> +		wol_info->wol_sopass_size = HCLGE_SOPASS_MAX;
> +	} else {
> +		wol_info->wol_sopass_size = 0;
> +	}
> +
> +	ret = hclge_set_wol_cfg(hdev, wol_info);
> +	if (ret)
> +		wol_info->wol_current_mode = 0;
> +
> +	return ret;
> +}
> +
>  static int hclge_init_ae_dev(struct hnae3_ae_dev *ae_dev)
>  {
>  	struct pci_dev *pdev = ae_dev->pdev;
> @@ -11729,6 +11866,11 @@ static int hclge_init_ae_dev(struct hnae3_ae_dev *ae_dev)
>  	/* Enable MISC vector(vector0) */
>  	hclge_enable_vector(&hdev->misc_vector, true);
>  
> +	ret = hclge_init_wol(hdev);
> +	if (ret)
> +		dev_warn(&pdev->dev,
> +			 "failed to wake on lan init, ret = %d\n", ret);
> +
>  	hclge_state_init(hdev);
>  	hdev->last_reset_time = jiffies;
>  
> @@ -12108,6 +12250,11 @@ static int hclge_reset_ae_dev(struct hnae3_ae_dev *ae_dev)
>  
>  	hclge_init_rxd_adv_layout(hdev);
>  
> +	ret = hclge_update_wol(hdev);
> +	if (ret)
> +		dev_err(&pdev->dev,
> +			"fail(%d) to update wol config\n", ret);
> +
>  	dev_info(&pdev->dev, "Reset done, %s driver initialization finished.\n",
>  		 HCLGE_DRIVER_NAME);
>  
> @@ -13154,6 +13301,8 @@ static const struct hnae3_ae_ops hclge_ops = {
>  	.get_link_diagnosis_info = hclge_get_link_diagnosis_info,
>  	.clean_vf_config = hclge_clean_vport_config,
>  	.get_dscp_prio = hclge_get_dscp_prio,
> +	.get_wol = hclge_get_wol,
> +	.set_wol = hclge_set_wol,
>  };
>  
>  static struct hnae3_ae_algo ae_algo = {
> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.h b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.h
> index 13f23d606e77..bd23d57b2ef2 100644
> --- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.h
> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.h
> @@ -249,6 +249,13 @@ enum HCLGE_MAC_DUPLEX {
>  #define QUERY_SFP_SPEED		0
>  #define QUERY_ACTIVE_SPEED	1
>  
> +struct hclge_wol_info {
> +	u32 wol_support_mode; /* store the wake on lan info */
> +	u32 wol_current_mode;
> +	u8 wol_sopass[HCLGE_SOPASS_MAX];
> +	u8 wol_sopass_size;
> +};
> +
>  struct hclge_mac {
>  	u8 mac_id;
>  	u8 phy_addr;
> @@ -268,6 +275,7 @@ struct hclge_mac {
>  	u32 user_fec_mode;
>  	u32 fec_ability;
>  	int link;	/* store the link status of mac & phy (if phy exists) */
> +	struct hclge_wol_info wol;
>  	struct phy_device *phydev;
>  	struct mii_bus *mdio_bus;
>  	phy_interface_t phy_if;

Powered by blists - more mailing lists