Re: [PATCH net-next 1/2] net: hns3: support wake on lan configuration and query

[Andrew Lunn <andrew@lunn.ch>]

> +enum HCLGE_WOL_MODE {
> +	HCLGE_WOL_PHY		= BIT(0),
> +	HCLGE_WOL_UNICAST	= BIT(1),
> +	HCLGE_WOL_MULTICAST	= BIT(2),
> +	HCLGE_WOL_BROADCAST	= BIT(3),
> +	HCLGE_WOL_ARP		= BIT(4),
> +	HCLGE_WOL_MAGIC		= BIT(5),
> +	HCLGE_WOL_MAGICSECURED	= BIT(6),
> +	HCLGE_WOL_FILTER	= BIT(7),
> +	HCLGE_WOL_DISABLE	= 0,
> +};

These are the exact same values as WAKE_PHY, WAKE_CAST etc. Since they
are ABI, they will never change. So you may as well throw these away
and just use the Linux values.

>  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 4e54f91f7a6c..88cb5c05bc43 100644
> --- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
> @@ -11500,6 +11500,201 @@ static void hclge_uninit_rxd_adv_layout(struct hclge_dev *hdev)
>  		hclge_write_dev(&hdev->hw, HCLGE_RXD_ADV_LAYOUT_EN_REG, 0);
>  }
>  
> +static __u32 hclge_wol_mode_to_ethtool(u32 mode)
> +{
> +	__u32 ret = 0;
> +
> +	if (mode & HCLGE_WOL_PHY)
> +		ret |= WAKE_PHY;
> +
> +	if (mode & HCLGE_WOL_UNICAST)
> +		ret |= WAKE_UCAST;
> +
> +	if (mode & HCLGE_WOL_MULTICAST)
> +		ret |= WAKE_MCAST;
> +
> +	if (mode & HCLGE_WOL_BROADCAST)
> +		ret |= WAKE_BCAST;
> +
> +	if (mode & HCLGE_WOL_ARP)
> +		ret |= WAKE_ARP;
> +
> +	if (mode & HCLGE_WOL_MAGIC)
> +		ret |= WAKE_MAGIC;
> +
> +	if (mode & HCLGE_WOL_MAGICSECURED)
> +		ret |= WAKE_MAGICSECURE;
> +
> +	if (mode & HCLGE_WOL_FILTER)
> +		ret |= WAKE_FILTER;

Once you throw away HCLGE_WOL_*, this function becomes much simpler.

> +
> +	return ret;
> +}
> +
> +static u32 hclge_wol_mode_from_ethtool(__u32 mode)
> +{
> +	u32 ret = HCLGE_WOL_DISABLE;
> +
> +	if (mode & WAKE_PHY)
> +		ret |= HCLGE_WOL_PHY;
> +
> +	if (mode & WAKE_UCAST)
> +		ret |= HCLGE_WOL_UNICAST;

This one two.

> @@ -12075,6 +12275,8 @@ static int hclge_reset_ae_dev(struct hnae3_ae_dev *ae_dev)
>  
>  	hclge_init_rxd_adv_layout(hdev);
>  
> +	(void)hclge_update_wol(hdev);

Please avoid casts like this. If there is an error, you should not
ignore it. If it cannot fail, make it a void function.	

       Andrew