[dpdk-dev] [PATCH v2 1/2] drivers/i40e: fix X722 macro absence result in compile

[Chilikin, Andrey]

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