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

[Ananyev, Konstantin]

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