| Message ID | 20220924023024.14219-1-huangguangbin2@huawei.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | redefine some macros of feature abilities judgement | expand |
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 > > Guangbin Huang (14): > net: hns3: modify macro hnae3_dev_fec_supported > net: hns3: modify macro hnae3_dev_udp_gso_supported > net: hns3: modify macro hnae3_dev_qb_supported > net: hns3: modify macro hnae3_dev_fd_forward_tc_supported > net: hns3: modify macro hnae3_dev_ptp_supported > net: hns3: modify macro hnae3_dev_int_ql_supported > net: hns3: modify macro hnae3_dev_hw_csum_supported > net: hns3: modify macro hnae3_dev_tx_push_supported > net: hns3: modify macro hnae3_dev_phy_imp_supported > net: hns3: modify macro hnae3_dev_ras_imp_supported > net: hns3: delete redundant macro hnae3_dev_tqp_txrx_indep_supported > net: hns3: modify macro hnae3_dev_hw_pad_supported > net: hns3: modify macro hnae3_dev_stash_supported > net: hns3: modify macro hnae3_dev_pause_supported > > drivers/net/ethernet/hisilicon/hns3/hnae3.h | 55 +++++++++---------- > .../hns3/hns3_common/hclge_comm_cmd.c | 2 +- > .../hns3/hns3_common/hclge_comm_cmd.h | 3 - > .../ethernet/hisilicon/hns3/hns3_debugfs.c | 2 +- > .../net/ethernet/hisilicon/hns3/hns3_enet.c | 10 ++-- > .../ethernet/hisilicon/hns3/hns3_ethtool.c | 14 ++--- > .../hisilicon/hns3/hns3pf/hclge_debugfs.c | 2 +- > .../hisilicon/hns3/hns3pf/hclge_main.c | 38 ++++++------- > .../hisilicon/hns3/hns3pf/hclge_ptp.c | 2 +- > .../hisilicon/hns3/hns3vf/hclgevf_main.c | 2 +- > 10 files changed, 62 insertions(+), 68 deletions(-) > > -- > 2.33.0 >
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?
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
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!
On Mon, Sep 26, 2022 at 10:11:35AM -0700, 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. Exactly, thanks Jakub.