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

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <0087bf43-a78c-7d3c-4ab2-de246afe25f8@huawei.com>
Date:   Wed, 4 Jan 2023 20:57:40 +0800
From:   Hao Lan <lanhao@...wei.com>
To:     Andrew Lunn <andrew@...n.ch>
CC:     <davem@...emloft.net>, <kuba@...nel.org>,
        <yisen.zhuang@...wei.com>, <salil.mehta@...wei.com>,
        <edumazet@...gle.com>, <pabeni@...hat.com>,
        <richardcochran@...il.com>, <shenjian15@...wei.com>,
        <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next 1/2] net: hns3: support wake on lan configuration
 and query

Hi Andrew,
Thank you for reviewing our code. Thank you very much.
You're right, such as WAKE_PHY, WAKE_CAST, these are implemented
in the kernel, they are ABI, they will never change, we use it
will directly simplify our code, your advice is very useful, thank
you very much for your advice.
However, these interfaces serve as a buffer between our firmware
and the linux community. Considering our interface expansion and
evolution, we may add some private modes in the future.
If the Linux community does not accept our private modes,
we will not be able to carry out these work.
So please let us keep enum HCLGE_WOL_MODE, thank you.

Best regards,
Hao Lan

On 2023/1/4 10:10, Andrew Lunn wrote:
>> +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
> .
>