In addition to Konstantin's point - some configuration settings, like RSS input set and PTYPEs, could be firmware dependent and change between fw versions for the same X710/X722 device. Moving mapping tables to the dev private data and initializing it on device start up will make code much cleaner.
Regards, /Andrey > -----Original Message----- > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Ananyev, Konstantin > Sent: Monday, October 17, 2016 10:55 AM > To: Guo, Jia <jia.guo at intel.com>; Zhang, Helin <helin.zhang at intel.com>; > Wu, > Jingjing <jingjing.wu at intel.com> > Cc: dev at dpdk.org > Subject: Re: [dpdk-dev] [PATCH v2 1/2] drivers/i40e: fix X722 macro absence > result in compile > > Hi Jeff, > > > > > hi, Konstantin > > Thanks your constructive suggestion. I don't think your question is > > silly and we also think about the code style simply and effective, but > > may be i would interpret the reason why we do that. > > > > 1) Sure, user definitely can choose to define the macro or not when > > building dpdk i40e PMD, but i don't think it is necessary to invoke a > > ret_config option to let up layer user freedom use it, because only > > the older version i40e driver does not support X722, the newer version > > i40e driver will always support X722, so the macro will be default > > hard code in the makefile. and we will use mac.type to distinguish the > > difference register configure in run time. So we may consider the > > macro just like a flag that highlight the difference of the shared > > code between X710 and X722, that would benify the X710/X722 pmd > > development but hardly no use to exposure to the up layer user. > > > > 2) i think the answer also could find from above. But i think if we > > develop go to a certain stage in the future, mute the macro or use > > script to remove them like the way from hw driver, for support all > > device types maybe not a bad idea, right? > > Sorry, but I still didn't get it. > If i40e driver will always support X722 then why do we need that macro at all? > Why just not to remove it completely then? > Same about run-time vs build-time choice: > If let say i40e_get_rss_key() has to behave in a different way, why not to > create > i40e_get_rss_key_x722() and use it when hw mactype is x7222? > Or at least inside i40e_get_rss_key() do something like: > if (hw->mac.type == I40E_MAC_X722) {...} else {...} ? > Why instead you have to pollute whole i40e code with all these #ifdef > x7222/#else ...? > Obviously that looks pretty ugly and hard to maintain. > Konstantin > > > > > > > > > On 10/16/2016 9:31 PM, Ananyev, Konstantin wrote: > > > Hi Jeff, > > > > > >> -----Original Message----- > > >> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Jeff Guo > > >> Sent: Sunday, October 16, 2016 2:40 AM > > >> To: Zhang, Helin <helin.zhang at intel.com>; Wu, Jingjing > > >> <jingjing.wu at intel.com> > > >> Cc: dev at dpdk.org; Guo, Jia <jia.guo at intel.com> > > >> Subject: [dpdk-dev] [PATCH v2 1/2] drivers/i40e: fix X722 macro > > >> absence result in compile > > >> > > >> Since some register only be supported by X722 but may not be > > >> supported by other NICs, so add X722 macro to distinguish that to > > >> avoid compile error when the X722 macro is undefined. > > > > > > Two probably silly questions: > > > 1) So who will setup X722_SUPPORT macro? > > > Is that a user responsibility when he is building dpdk i40e PMD? > > > If so, why it is not a rte_config option? > > > 2) Why this all has to be build time decision? > > > Why nor run-time? > > > Why i40e driver can't support all devices (including x722) and > > > invoke different config functions (write different registers) based > > > on device type/id information? > > > As it does for other device types/ids? > > > > > > Konstantin
