netdev - Re: [PATCH net-next 00/14] redefine some macros of feature abilities judgement

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]

Date:   Tue, 27 Sep 2022 11:21:12 +0800
From:   "huangguangbin (A)" <huangguangbin2@...wei.com>
To:     Jakub Kicinski <kuba@...nel.org>
CC:     Leon Romanovsky <leon@...nel.org>, <davem@...emloft.net>,
        <edumazet@...gle.com>, <pabeni@...hat.com>,
        <netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <lipeng321@...wei.com>, <lanhao@...wei.com>
Subject: Re: [PATCH net-next 00/14] redefine some macros of feature abilities
 judgement



On 2022/9/27 1:11, Jakub Kicinski wrote:
> On Mon, 26 Sep 2022 20:56:26 +0800 huangguangbin (A) wrote:
>> On 2022/9/24 19:27, Leon Romanovsky wrote:
>>> On Sat, Sep 24, 2022 at 10:30:10AM +0800, Guangbin Huang wrote:
>>>> The macros hnae3_dev_XXX_supported just can be used in hclge layer, but
>>>> hns3_enet layer may need to use, so this serial redefine these macros.
>>>
>>> IMHO, you shouldn't add new obfuscated code, but delete it.
>>>
>>> Jakub,
>>>
>>> The more drivers authors will obfuscate in-kernel primitives and reinvent
>>> their own names, macros e.t.c, the less external reviewers you will be able
>>> to attract.
>>>
>>> IMHO, netdev should have more active position do not allow obfuscated code.
>>>
>>> Thanks
>>>    
>>
>> Hi, Leon
>> I'm sorry, I can not get your point. Can you explain in more detail?
>> Do you mean the name "macro" should not be used?
> 
> He is saying that you should try to remove those macros rather than
> touch them up. The macros may seem obvious to people working on the
> driver but to upstream reviewers any local conventions obfuscate the
> code and require looking up definitions.
> 
> For example the first patch is better off as:
> 
> diff --git a/drivers/net/ethernet/hisilicon/hns3/hnae3.h b/drivers/net/ethernet/hisilicon/hns3/hnae3.h
> index 0179fc288f5f..449d496b824b 100644
> --- a/drivers/net/ethernet/hisilicon/hns3/hnae3.h
> +++ b/drivers/net/ethernet/hisilicon/hns3/hnae3.h
> @@ -107,9 +107,6 @@ enum HNAE3_DEV_CAP_BITS {
>   #define hnae3_ae_dev_gro_supported(ae_dev) \
>   		test_bit(HNAE3_DEV_SUPPORT_GRO_B, (ae_dev)->caps)
>   
> -#define hnae3_dev_fec_supported(hdev) \
> -	test_bit(HNAE3_DEV_SUPPORT_FEC_B, (hdev)->ae_dev->caps)
> -
>   #define hnae3_dev_udp_gso_supported(hdev) \
>   	test_bit(HNAE3_DEV_SUPPORT_UDP_GSO_B, (hdev)->ae_dev->caps)
>   
> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
> index 6962a9d69cf8..ded92f7dbd79 100644
> --- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
> @@ -1179,7 +1179,7 @@ static void hclge_parse_fiber_link_mode(struct hclge_dev *hdev,
>   	hclge_convert_setting_sr(speed_ability, mac->supported);
>   	hclge_convert_setting_lr(speed_ability, mac->supported);
>   	hclge_convert_setting_cr(speed_ability, mac->supported);
> -	if (hnae3_dev_fec_supported(hdev))
> +	if (test_bit(HNAE3_DEV_SUPPORT_FEC_B, hdev->caps))
>   		hclge_convert_setting_fec(mac);
>   
>   	if (hnae3_dev_pause_supported(hdev))
> @@ -1195,7 +1195,7 @@ static void hclge_parse_backplane_link_mode(struct hclge_dev *hdev,
>   	struct hclge_mac *mac = &hdev->hw.mac;
>   
>   	hclge_convert_setting_kr(speed_ability, mac->supported);
> -	if (hnae3_dev_fec_supported(hdev))
> +	if (test_bit(HNAE3_DEV_SUPPORT_FEC_B, hdev->caps))
>   		hclge_convert_setting_fec(mac);
>   
>   	if (hnae3_dev_pause_supported(hdev))
> @@ -3232,7 +3232,7 @@ static void hclge_update_advertising(struct hclge_dev *hdev)
>   static void hclge_update_port_capability(struct hclge_dev *hdev,
>   					 struct hclge_mac *mac)
>   {
> -	if (hnae3_dev_fec_supported(hdev))
> +	if (test_bit(HNAE3_DEV_SUPPORT_FEC_B, hdev->caps))
>   		hclge_convert_setting_fec(mac);
>   
>   	/* firmware can not identify back plane type, the media type
> .
> 
Ok, I see, thanks!