[dpdk-dev] [PATCH] net/thunderx: sync mailbox definitions with Linux PF driver

[Ferruh Yigit]

On 3/22/2017 9:39 AM, Jerin Jacob wrote:
> On Tue, Mar 21, 2017 at 02:53:29PM +0000, Ferruh Yigit wrote:
>> On 3/21/2017 2:38 PM, Jerin Jacob wrote:
>>> On Tue, Mar 21, 2017 at 02:31:41PM +0000, Ferruh Yigit wrote:
>>>> On 3/20/2017 2:10 PM, Jerin Jacob wrote:
>>>>> - bgx_link_status mbox definition was changed in Linux
>>>>> commit 1cc702591bae ("net: thunderx: Add ethtool support")
>>>>> - NIC_MBOX_MSG_RES_BIT related changes were never part of Linux PF driver
>>>>>
>>>>> Signed-off-by: Jerin Jacob <jerin.jacob at caviumnetworks.com>
>>>>
>>>> <...>
>>>>
>>>>> @@ -157,6 +151,7 @@ struct rss_cfg_msg {
>>>>>  /* Physical interface link status */
>>>>>  struct bgx_link_status {
>>>>>  	uint8_t    msg;
>>>>> +	uint8_t    mac_type;
>>>>
>>>> Hi Jerin,
>>>>
>>>> Is this modification related to this patch?
>>>
>>> Yes Ferruh.
>>>
>>> This was related to the following section in git log comment.
>>> ----
>>> - bgx_link_status mbox definition was changed in Linux
>>> commit 1cc702591bae ("net: thunderx: Add ethtool support") 
>>> ---
>>
>> I see now, thanks. Since this is to sync with Linux PF, shouldn't it be
>> used in driver, perhaps something like in Linux driver:
>> "nic->mac_type = mbx.link_status.mac_type"
>>
>> What is the point of just adding definition without using it?
> 
> That is to keep "link_up"(next element in the struct bgx_link_status) points
> to correct location after the kernel change. 

I see, thanks. It is pity that new field added into middle (or beginning
if you exclude msg) of the struct.

> I thought about, the backward
> compatibility with older kernel, Is it OK to use linux/version.h in PMD drivers
> to detect the kernel version?

Technically possible, but kernel version check has its problems:
1) Adds maintenance cost, which gets worse by time.
2) If added a compile time check, it creates binary distribution
problems, distro guys may not like PMD has dependency to kernel.
3) It will create dependency to Linux, assuming Thunderx PMD supported
in FreeBSD

> drivers/net/mlx5/mlx5_ethdev.c has similar
> kernel detection mechanism to make it backward compatible.

Yes, at least mlx added a dynamic check which prevents above issue 2.

> If there are no issue with such approach, I will roll out a new revision.

Is there any way to detect this dynamically, first things I can think of:
- Any mbx versioning to rely on?
- Any common NIC user header file provided by kernel, to use in DPDK, to
prevent any this kind of issues in the future?
- Any msg size value to detect if mac_type exists or not?
- Heuristic approach to other values to detect mac_type existence?

> 
> 
>>
>>>
>>>
>>>>
>>>>>  	uint8_t    link_up;
>>>>>  	uint8_t    duplex;
>>>>>  	uint32_t   speed;
>>>>>
>>>>
>>