Messages in this thread | | | Subject | Re: [PATCH net-next 00/14] redefine some macros of feature abilities judgement | From | "huangguangbin (A)" <> | Date | Tue, 27 Sep 2022 11:21:12 +0800 |
| |
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!
| |