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

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

[Jerin Jacob]

- 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@caviumnetworks.com>
---
 drivers/net/thunderx/base/nicvf_mbox.c |  7 ++-----
 drivers/net/thunderx/base/nicvf_mbox.h | 11 +++--------
 2 files changed, 5 insertions(+), 13 deletions(-)

diff --git a/drivers/net/thunderx/base/nicvf_mbox.c b/drivers/net/thunderx/base/nicvf_mbox.c
index 3b7b8a5..8b3477d 100644
--- a/drivers/net/thunderx/base/nicvf_mbox.c
+++ b/drivers/net/thunderx/base/nicvf_mbox.c
@@ -62,9 +62,6 @@ static const char *mbox_message[NIC_MBOX_MSG_MAX] =  {
 	[NIC_MBOX_MSG_RESET_STAT_COUNTER] = "NIC_MBOX_MSG_RESET_STAT_COUNTER",
 	[NIC_MBOX_MSG_CFG_DONE]           = "NIC_MBOX_MSG_CFG_DONE",
 	[NIC_MBOX_MSG_SHUTDOWN]           = "NIC_MBOX_MSG_SHUTDOWN",
-	[NIC_MBOX_MSG_RES_BIT]            = "NIC_MBOX_MSG_RES_BIT",
-	[NIC_MBOX_MSG_RSS_SIZE_RES_BIT]   = "NIC_MBOX_MSG_RSS_SIZE",
-	[NIC_MBOX_MSG_ALLOC_SQS_RES_BIT]  = "NIC_MBOX_MSG_ALLOC_SQS",
 };
 
 static inline const char * __attribute__((unused))
@@ -176,7 +173,7 @@ nicvf_handle_mbx_intr(struct nicvf *nic)
 	case NIC_MBOX_MSG_NACK:
 		nic->pf_nacked = true;
 		break;
-	case NIC_MBOX_MSG_RSS_SIZE_RES_BIT:
+	case NIC_MBOX_MSG_RSS_SIZE:
 		nic->rss_info.rss_size = mbx.rss_size.ind_tbl_size;
 		nic->pf_acked = true;
 		break;
@@ -186,7 +183,7 @@ nicvf_handle_mbx_intr(struct nicvf *nic)
 		nic->speed = mbx.link_status.speed;
 		nic->pf_acked = true;
 		break;
-	case NIC_MBOX_MSG_ALLOC_SQS_RES_BIT:
+	case NIC_MBOX_MSG_ALLOC_SQS:
 		assert_primary(nic);
 		if (mbx.sqs_alloc.qs_count != nic->sqs_count) {
 			nicvf_log_error("Received %" PRIu8 "/%" PRIu8
diff --git a/drivers/net/thunderx/base/nicvf_mbox.h b/drivers/net/thunderx/base/nicvf_mbox.h
index 084f3a7..8675fe8 100644
--- a/drivers/net/thunderx/base/nicvf_mbox.h
+++ b/drivers/net/thunderx/base/nicvf_mbox.h
@@ -68,16 +68,10 @@
 #define	NIC_MBOX_MSG_ALLOC_SQS		0x12	/* Allocate secondary Qset */
 #define	NIC_MBOX_MSG_LOOPBACK		0x16	/* Set interface in loopback */
 #define	NIC_MBOX_MSG_RESET_STAT_COUNTER 0x17	/* Reset statistics counters */
-#define	NIC_MBOX_MSG_CFG_DONE		0x7E	/* VF configuration done */
-#define	NIC_MBOX_MSG_SHUTDOWN		0x7F	/* VF is being shutdown */
-#define	NIC_MBOX_MSG_RES_BIT		0x80	/* Reset bit from PF */
+#define	NIC_MBOX_MSG_CFG_DONE		0xF0	/* VF configuration done */
+#define	NIC_MBOX_MSG_SHUTDOWN		0xF1	/* VF is being shutdown */
 #define	NIC_MBOX_MSG_MAX		0x100	/* Maximum number of messages */
 
-#define NIC_MBOX_MSG_RSS_SIZE_RES_BIT \
-	(NIC_MBOX_MSG_RSS_SIZE | NIC_MBOX_MSG_RES_BIT)
-#define NIC_MBOX_MSG_ALLOC_SQS_RES_BIT \
-	(NIC_MBOX_MSG_ALLOC_SQS | NIC_MBOX_MSG_RES_BIT)
-
 /* Get vNIC VF configuration */
 struct nic_cfg_msg {
 	uint8_t    msg;
@@ -157,6 +151,7 @@ struct rss_cfg_msg {
 /* Physical interface link status */
 struct bgx_link_status {
 	uint8_t    msg;
+	uint8_t    mac_type;
 	uint8_t    link_up;
 	uint8_t    duplex;
 	uint32_t   speed;
-- 
2.5.5

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

[Ferruh Yigit]

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@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?

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

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

[Jerin Jacob]

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@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") 
---


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

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

[Ferruh Yigit]

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@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?

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

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

[Jerin Jacob]

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@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 thought about, the backward
compatibility with older kernel, Is it OK to use linux/version.h in PMD drivers
to detect the kernel version? drivers/net/mlx5/mlx5_ethdev.c has similar
kernel detection mechanism to make it backward compatible.
If there are no issue with such approach, I will roll out a new revision.


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

Re: [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@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;
>>>>>
>>>>
>>

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

[Jerin Jacob]

On Wed, Mar 22, 2017 at 10:33:14AM +0000, Ferruh Yigit wrote:
> 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@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?

Thanks for sharing the options.
First three options wont work. Heuristic option is pretty limited,
mac_type value can also be 0 or 1(same as link_up)
So I think, The best option now to use the patch as is(without kernel
check).
What do you think?

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

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

[Ferruh Yigit]

On 3/23/2017 1:20 PM, Jerin Jacob wrote:
> On Wed, Mar 22, 2017 at 10:33:14AM +0000, Ferruh Yigit wrote:
>> 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@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?
> 
> Thanks for sharing the options.
> First three options wont work. Heuristic option is pretty limited,
> mac_type value can also be 0 or 1(same as link_up)
> So I think, The best option now to use the patch as is(without kernel
> check).
> What do you think?

If this won't cause a problem for you with older kernels, I would prefer
no kernel check option.

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

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

[Jerin Jacob]

On Thu, Mar 23, 2017 at 01:26:15PM +0000, Ferruh Yigit wrote:
> On 3/23/2017 1:20 PM, Jerin Jacob wrote:
> > On Wed, Mar 22, 2017 at 10:33:14AM +0000, Ferruh Yigit wrote:
> >> 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@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?
> > 
> > Thanks for sharing the options.
> > First three options wont work. Heuristic option is pretty limited,
> > mac_type value can also be 0 or 1(same as link_up)
> > So I think, The best option now to use the patch as is(without kernel
> > check).
> > What do you think?
> 
> If this won't cause a problem for you with older kernels, I would prefer
> no kernel check option.

OK. I will ask our kernel team to backport the offending the kernel change
to old versions based on the requirement. Please merge this patch if you don't
see any other issue.
I can post a separate patch add to the kernel version check if it
is really required.


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

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

[Ferruh Yigit]

On 3/23/2017 2:00 PM, Jerin Jacob wrote:
> On Thu, Mar 23, 2017 at 01:26:15PM +0000, Ferruh Yigit wrote:
>> On 3/23/2017 1:20 PM, Jerin Jacob wrote:
>>> On Wed, Mar 22, 2017 at 10:33:14AM +0000, Ferruh Yigit wrote:
>>>> 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@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?
>>>
>>> Thanks for sharing the options.
>>> First three options wont work. Heuristic option is pretty limited,
>>> mac_type value can also be 0 or 1(same as link_up)
>>> So I think, The best option now to use the patch as is(without kernel
>>> check).
>>> What do you think?
>>
>> If this won't cause a problem for you with older kernels, I would prefer
>> no kernel check option.
> 
> OK. I will ask our kernel team to backport the offending the kernel change
> to old versions based on the requirement. Please merge this patch if you don't
> see any other issue.
> I can post a separate patch add to the kernel version check if it
> is really required.

OK

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

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

[Ferruh Yigit]

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@caviumnetworks.com>

Applied to dpdk-next-net/master, thanks.