net-next build failure due to 16e5cc64?

net-next build failure due to 16e5cc64?

[Sowmini Varadhan]

John,

I'm getting a failure when I try to build the latest net-next. I see

net/sched/sch_mqprio.c: In function `mqprio_init':
net/sched/sch_mqprio.c:145: error: unknown field `tc' specified in initializer
net/sched/sch_mqprio.c:145: warning: missing braces around initializer
net/sched/sch_mqprio.c:145: warning: (near initialization for `tc.<anonymous>')
make[2]: *** [net/sched/sch_mqprio.o] Error 1

I'm not sure why this wasn't noticed before, but in case this is due to my
compiler version:
   # gcc --version
   gcc (GCC) 4.4.7 20120313 (Red Hat 4.4.7-16)
   Copyright (C) 2010 Free Software Foundation, Inc.

I'm finding that the minimum fix to avoid this error should be something like

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index e52077f..8b7199f 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -794,7 +794,7 @@ struct tc_to_netdev {
        union {
                u8 tc;
                struct tc_cls_u32_offload *cls_u32;
-       };
+       } tc_u;
 };

But then all the users of this structure would need to be changed,
which results in this patch below. Should I submit this to net-next,
or is it addressed elsewhere? 

diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
index 45843d1..34ff0e8 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
@@ -4277,7 +4277,7 @@ int __bnx2x_setup_tc(struct net_device *dev, u32 handle, __be16 proto,
 {
 	if (handle != TC_H_ROOT || tc->type != TC_SETUP_MQPRIO)
 		return -EINVAL;
-	return bnx2x_setup_tc(dev, tc->tc);
+	return bnx2x_setup_tc(dev, tc->tc_u.tc);
 }
 
 /* called with rtnl_lock */
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index cf4b729..b2c9355 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -8403,20 +8403,20 @@ int __ixgbe_setup_tc(struct net_device *dev, u32 handle, __be16 proto,
 		if (!(dev->features & NETIF_F_HW_TC))
 			return -EINVAL;
 
-		switch (tc->cls_u32->command) {
+		switch (tc->tc_u.cls_u32->command) {
 		case TC_CLSU32_NEW_KNODE:
 		case TC_CLSU32_REPLACE_KNODE:
 			return ixgbe_configure_clsu32(adapter,
-						      proto, tc->cls_u32);
+						      proto, tc->tc_u.cls_u32);
 		case TC_CLSU32_DELETE_KNODE:
-			return ixgbe_delete_clsu32(adapter, tc->cls_u32);
+			return ixgbe_delete_clsu32(adapter, tc->tc_u.cls_u32);
 		case TC_CLSU32_NEW_HNODE:
 		case TC_CLSU32_REPLACE_HNODE:
 			return ixgbe_configure_clsu32_add_hnode(adapter, proto,
-								tc->cls_u32);
+								tc->tc_u.cls_u32);
 		case TC_CLSU32_DELETE_HNODE:
 			return ixgbe_configure_clsu32_del_hnode(adapter,
-								tc->cls_u32);
+								tc->tc_u.cls_u32);
 		default:
 			return -EINVAL;
 		}
@@ -8425,7 +8425,7 @@ int __ixgbe_setup_tc(struct net_device *dev, u32 handle, __be16 proto,
 	if (handle != TC_H_ROOT || tc->type != TC_SETUP_MQPRIO)
 		return -EINVAL;
 
-	return ixgbe_setup_tc(dev, tc->tc);
+	return ixgbe_setup_tc(dev, tc->tc_u.tc);
 }
 
 #ifdef CONFIG_PCI_IOV
diff --git a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
index 96d95cb..90aac4d 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
@@ -75,7 +75,7 @@ static int __mlx4_en_setup_tc(struct net_device *dev, u32 handle, __be16 proto,
 	if (handle != TC_H_ROOT || tc->type != TC_SETUP_MQPRIO)
 		return -EINVAL;
 
-	return mlx4_en_setup_tc(dev, tc->tc);
+	return mlx4_en_setup_tc(dev, tc->tc_u.tc);
 }
 
 #ifdef CONFIG_RFS_ACCEL
diff --git a/drivers/net/ethernet/sfc/tx.c b/drivers/net/ethernet/sfc/tx.c
index 2cdb571..8925426 100644
--- a/drivers/net/ethernet/sfc/tx.c
+++ b/drivers/net/ethernet/sfc/tx.c
@@ -574,7 +574,7 @@ int efx_setup_tc(struct net_device *net_dev, u32 handle, __be16 proto,
 	if (handle != TC_H_ROOT || ntc->type != TC_SETUP_MQPRIO)
 		return -EINVAL;
 
-	num_tc = ntc->tc;
+	num_tc = ntc->tc_u.tc;
 
 	if (efx_nic_rev(efx) < EFX_REV_FALCON_B0 || num_tc > EFX_MAX_TX_TC)
 		return -EINVAL;
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index e52077f..8b7199f 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -794,7 +794,7 @@ struct tc_to_netdev {
 	union {
 		u8 tc;
 		struct tc_cls_u32_offload *cls_u32;
-	};
+	} tc_u;
 };
 
 
diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
index d54bc94..b41071f 100644
--- a/net/sched/cls_u32.c
+++ b/net/sched/cls_u32.c
@@ -432,11 +432,11 @@ static void u32_remove_hw_knode(struct tcf_proto *tp, u32 handle)
 	struct tc_to_netdev offload;
 
 	offload.type = TC_SETUP_CLSU32;
-	offload.cls_u32 = &u32_offload;
+	offload.tc_u.cls_u32 = &u32_offload;
 
 	if (dev->netdev_ops->ndo_setup_tc) {
-		offload.cls_u32->command = TC_CLSU32_DELETE_KNODE;
-		offload.cls_u32->knode.handle = handle;
+		offload.tc_u.cls_u32->command = TC_CLSU32_DELETE_KNODE;
+		offload.tc_u.cls_u32->knode.handle = handle;
 		dev->netdev_ops->ndo_setup_tc(dev, tp->q->handle,
 					      tp->protocol, &offload);
 	}
@@ -449,13 +449,13 @@ static void u32_replace_hw_hnode(struct tcf_proto *tp, struct tc_u_hnode *h)
 	struct tc_to_netdev offload;
 
 	offload.type = TC_SETUP_CLSU32;
-	offload.cls_u32 = &u32_offload;
+	offload.tc_u.cls_u32 = &u32_offload;
 
 	if (dev->netdev_ops->ndo_setup_tc) {
-		offload.cls_u32->command = TC_CLSU32_NEW_HNODE;
-		offload.cls_u32->hnode.divisor = h->divisor;
-		offload.cls_u32->hnode.handle = h->handle;
-		offload.cls_u32->hnode.prio = h->prio;
+		offload.tc_u.cls_u32->command = TC_CLSU32_NEW_HNODE;
+		offload.tc_u.cls_u32->hnode.divisor = h->divisor;
+		offload.tc_u.cls_u32->hnode.handle = h->handle;
+		offload.tc_u.cls_u32->hnode.prio = h->prio;
 
 		dev->netdev_ops->ndo_setup_tc(dev, tp->q->handle,
 					      tp->protocol, &offload);
@@ -469,13 +469,13 @@ static void u32_clear_hw_hnode(struct tcf_proto *tp, struct tc_u_hnode *h)
 	struct tc_to_netdev offload;
 
 	offload.type = TC_SETUP_CLSU32;
-	offload.cls_u32 = &u32_offload;
+	offload.tc_u.cls_u32 = &u32_offload;
 
 	if (dev->netdev_ops->ndo_setup_tc) {
-		offload.cls_u32->command = TC_CLSU32_DELETE_HNODE;
-		offload.cls_u32->hnode.divisor = h->divisor;
-		offload.cls_u32->hnode.handle = h->handle;
-		offload.cls_u32->hnode.prio = h->prio;
+		offload.tc_u.cls_u32->command = TC_CLSU32_DELETE_HNODE;
+		offload.tc_u.cls_u32->hnode.divisor = h->divisor;
+		offload.tc_u.cls_u32->hnode.handle = h->handle;
+		offload.tc_u.cls_u32->hnode.prio = h->prio;
 
 		dev->netdev_ops->ndo_setup_tc(dev, tp->q->handle,
 					      tp->protocol, &offload);
@@ -489,23 +489,23 @@ static void u32_replace_hw_knode(struct tcf_proto *tp, struct tc_u_knode *n)
 	struct tc_to_netdev offload;
 
 	offload.type = TC_SETUP_CLSU32;
-	offload.cls_u32 = &u32_offload;
+	offload.tc_u.cls_u32 = &u32_offload;
 
 	if (dev->netdev_ops->ndo_setup_tc) {
-		offload.cls_u32->command = TC_CLSU32_REPLACE_KNODE;
-		offload.cls_u32->knode.handle = n->handle;
-		offload.cls_u32->knode.fshift = n->fshift;
+		offload.tc_u.cls_u32->command = TC_CLSU32_REPLACE_KNODE;
+		offload.tc_u.cls_u32->knode.handle = n->handle;
+		offload.tc_u.cls_u32->knode.fshift = n->fshift;
 #ifdef CONFIG_CLS_U32_MARK
-		offload.cls_u32->knode.val = n->val;
-		offload.cls_u32->knode.mask = n->mask;
+		offload.tc_u.cls_u32->knode.val = n->val;
+		offload.tc_u.cls_u32->knode.mask = n->mask;
 #else
-		offload.cls_u32->knode.val = 0;
-		offload.cls_u32->knode.mask = 0;
+		offload.tc_u.cls_u32->knode.val = 0;
+		offload.tc_u.cls_u32->knode.mask = 0;
 #endif
-		offload.cls_u32->knode.sel = &n->sel;
-		offload.cls_u32->knode.exts = &n->exts;
+		offload.tc_u.cls_u32->knode.sel = &n->sel;
+		offload.tc_u.cls_u32->knode.exts = &n->exts;
 		if (n->ht_down)
-			offload.cls_u32->knode.link_handle = n->ht_down->handle;
+			offload.tc_u.cls_u32->knode.link_handle = n->ht_down->handle;
 
 		dev->netdev_ops->ndo_setup_tc(dev, tp->q->handle,
 					      tp->protocol, &offload);
diff --git a/net/sched/sch_mqprio.c b/net/sched/sch_mqprio.c
index f9947d1..b399faf 100644
--- a/net/sched/sch_mqprio.c
+++ b/net/sched/sch_mqprio.c
@@ -142,7 +142,7 @@ static int mqprio_init(struct Qdisc *sch, struct nlattr *opt)
 	 */
 	if (qopt->hw) {
 		struct tc_to_netdev tc = {.type = TC_SETUP_MQPRIO,
-					  .tc = qopt->num_tc};
+					  .tc_u.tc = qopt->num_tc};
 
 		priv->hw_owned = 1;
 		err = dev->netdev_ops->ndo_setup_tc(dev, sch->handle, 0, &tc);
diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
index 70d9605..00b4561 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -5330,7 +5330,7 @@ static int __i40e_setup_tc(struct net_device *netdev, u32 handle, __be16 proto,
 {
 	if (handle != TC_H_ROOT || tc->type != TC_SETUP_MQPRIO)
 		return -EINVAL;
-	return i40e_setup_tc(netdev, tc->tc);
+	return i40e_setup_tc(netdev, tc->tc_u.tc);
 }
 
 /**

Re: net-next build failure due to 16e5cc64?

[Cong Wang]

On Tue, Mar 1, 2016 at 9:42 AM, Sowmini Varadhan
<sowmini.varadhan@oracle.com> wrote:
>
> John,
>
> I'm getting a failure when I try to build the latest net-next. I see
>
> net/sched/sch_mqprio.c: In function `mqprio_init':
> net/sched/sch_mqprio.c:145: error: unknown field `tc' specified in initializer
> net/sched/sch_mqprio.c:145: warning: missing braces around initializer
> net/sched/sch_mqprio.c:145: warning: (near initialization for `tc.<anonymous>')
> make[2]: *** [net/sched/sch_mqprio.o] Error 1
>
> I'm not sure why this wasn't noticed before, but in case this is due to my
> compiler version:
>    # gcc --version
>    gcc (GCC) 4.4.7 20120313 (Red Hat 4.4.7-16)
>    Copyright (C) 2010 Free Software Foundation, Inc.
>
> I'm finding that the minimum fix to avoid this error should be something like
>

Why not just initialize these fields explicitly? For example,

diff --git a/net/sched/sch_mqprio.c b/net/sched/sch_mqprio.c
index f9947d1..19323b6 100644
--- a/net/sched/sch_mqprio.c
+++ b/net/sched/sch_mqprio.c
@@ -28,7 +28,7 @@ static void mqprio_destroy(struct Qdisc *sch)
 {
        struct net_device *dev = qdisc_dev(sch);
        struct mqprio_sched *priv = qdisc_priv(sch);
-       struct tc_to_netdev tc = {.type = TC_SETUP_MQPRIO};
+       struct tc_to_netdev tc;
        unsigned int ntx;

        if (priv->qdiscs) {
@@ -39,6 +39,7 @@ static void mqprio_destroy(struct Qdisc *sch)
                kfree(priv->qdiscs);
        }

+       tc.type = TC_SETUP_MQPRIO;
        if (priv->hw_owned && dev->netdev_ops->ndo_setup_tc)
                dev->netdev_ops->ndo_setup_tc(dev, sch->handle, 0, &tc);
        else

Re: net-next build failure due to 16e5cc64?

[Cong Wang]

On Tue, Mar 1, 2016 at 10:13 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Tue, Mar 1, 2016 at 9:42 AM, Sowmini Varadhan
> <sowmini.varadhan@oracle.com> wrote:
>>
>> John,
>>
>> I'm getting a failure when I try to build the latest net-next. I see
>>
>> net/sched/sch_mqprio.c: In function `mqprio_init':
>> net/sched/sch_mqprio.c:145: error: unknown field `tc' specified in initializer
>> net/sched/sch_mqprio.c:145: warning: missing braces around initializer
>> net/sched/sch_mqprio.c:145: warning: (near initialization for `tc.<anonymous>')
>> make[2]: *** [net/sched/sch_mqprio.o] Error 1
>>
>> I'm not sure why this wasn't noticed before, but in case this is due to my
>> compiler version:
>>    # gcc --version
>>    gcc (GCC) 4.4.7 20120313 (Red Hat 4.4.7-16)
>>    Copyright (C) 2010 Free Software Foundation, Inc.
>>
>> I'm finding that the minimum fix to avoid this error should be something like
>>
>
> Why not just initialize these fields explicitly? For example,
>
> diff --git a/net/sched/sch_mqprio.c b/net/sched/sch_mqprio.c
> index f9947d1..19323b6 100644
> --- a/net/sched/sch_mqprio.c
> +++ b/net/sched/sch_mqprio.c
> @@ -28,7 +28,7 @@ static void mqprio_destroy(struct Qdisc *sch)
>  {
>         struct net_device *dev = qdisc_dev(sch);
>         struct mqprio_sched *priv = qdisc_priv(sch);
> -       struct tc_to_netdev tc = {.type = TC_SETUP_MQPRIO};
> +       struct tc_to_netdev tc;
>         unsigned int ntx;
>
>         if (priv->qdiscs) {
> @@ -39,6 +39,7 @@ static void mqprio_destroy(struct Qdisc *sch)
>                 kfree(priv->qdiscs);
>         }
>
> +       tc.type = TC_SETUP_MQPRIO;

And also set the rest to zero...

>         if (priv->hw_owned && dev->netdev_ops->ndo_setup_tc)
>                 dev->netdev_ops->ndo_setup_tc(dev, sch->handle, 0, &tc);
>         else

Re: net-next build failure due to 16e5cc64?

[Sowmini Varadhan]

On (03/01/16 10:13), Cong Wang wrote:
> > net/sched/sch_mqprio.c: In function `mqprio_init':
> > net/sched/sch_mqprio.c:145: error: unknown field `tc' specified in initializer
    :
> Why not just initialize these fields explicitly? For example,
> 
> +       struct tc_to_netdev tc;
             :
> +       tc.type = TC_SETUP_MQPRIO;

The compiler error is for fields within the union which lacks
both a tag and a union-name.  So I'm not sure how the above will
help.

--Sowmini

Re: net-next build failure due to 16e5cc64?

[Cong Wang]

On Tue, Mar 1, 2016 at 10:21 AM, Sowmini Varadhan
<sowmini.varadhan@oracle.com> wrote:
> On (03/01/16 10:13), Cong Wang wrote:
>> > net/sched/sch_mqprio.c: In function `mqprio_init':
>> > net/sched/sch_mqprio.c:145: error: unknown field `tc' specified in initializer
>     :
>> Why not just initialize these fields explicitly? For example,
>>
>> +       struct tc_to_netdev tc;
>              :
>> +       tc.type = TC_SETUP_MQPRIO;
>
> The compiler error is for fields within the union which lacks
> both a tag and a union-name.  So I'm not sure how the above will
> help.
>

Come on.. we have plenty of such anonymous unions in skbuff.h...

Re: net-next build failure due to 16e5cc64?

[Sowmini Varadhan]

On (03/01/16 10:23), Cong Wang wrote:
> > The compiler error is for fields within the union which lacks
> > both a tag and a union-name.  So I'm not sure how the above will
> > help.
> >
> 
> Come on.. we have plenty of such anonymous unions in skbuff.h...

I realize that, even netdevice.h itself has other instances.
Thus I dont understand why this one generated a compiler error 
for me.  Thus I was wondering why others had not run into this
and not suggesting it should be patched.

However, changing the way we set up type etc. doesnt seem relevant
to what the compiler is flagging.

--Sowmini

Re: net-next build failure due to 16e5cc64?

[Cong Wang]

On Tue, Mar 1, 2016 at 10:41 AM, Sowmini Varadhan
<sowmini.varadhan@oracle.com> wrote:
> On (03/01/16 10:23), Cong Wang wrote:
>> > The compiler error is for fields within the union which lacks
>> > both a tag and a union-name.  So I'm not sure how the above will
>> > help.
>> >
>>
>> Come on.. we have plenty of such anonymous unions in skbuff.h...
>
> I realize that, even netdevice.h itself has other instances.
> Thus I dont understand why this one generated a compiler error
> for me.  Thus I was wondering why others had not run into this
> and not suggesting it should be patched.
>
> However, changing the way we set up type etc. doesnt seem relevant
> to what the compiler is flagging.
>

Clearly the compiler indicates something is wrong in the initializer.

I don't know why you don't want to try the way I suggest or maybe
the following:

diff --git a/net/sched/sch_mqprio.c b/net/sched/sch_mqprio.c
index f9947d1..77743e8 100644
--- a/net/sched/sch_mqprio.c
+++ b/net/sched/sch_mqprio.c
@@ -142,7 +142,7 @@ static int mqprio_init(struct Qdisc *sch, struct
nlattr *opt)
         */
        if (qopt->hw) {
                struct tc_to_netdev tc = {.type = TC_SETUP_MQPRIO,
-                                         .tc = qopt->num_tc};
+                                         {.tc = qopt->num_tc} };

                priv->hw_owned = 1;
                err = dev->netdev_ops->ndo_setup_tc(dev, sch->handle, 0, &tc);

Re: net-next build failure due to 16e5cc64?

[Sowmini Varadhan]

On (03/01/16 10:51), Cong Wang wrote:
> --- a/net/sched/sch_mqprio.c
> +++ b/net/sched/sch_mqprio.c
> @@ -142,7 +142,7 @@ static int mqprio_init(struct Qdisc *sch, struct
> nlattr *opt)
>          */
>         if (qopt->hw) {
>                 struct tc_to_netdev tc = {.type = TC_SETUP_MQPRIO,
> -                                         .tc = qopt->num_tc};
> +                                         {.tc = qopt->num_tc} };
> 

this fix works.

The first suggestion had no effect (needs changes to a lot of files
I suspect).

BTW, I remain surprised that no one else has noticed this. It did not
take me any effort to reproduce it on an x86.

--Sowmini

Re: net-next build failure due to 16e5cc64?

[Cong Wang]

On Tue, Mar 1, 2016 at 11:09 AM, Sowmini Varadhan
<sowmini.varadhan@oracle.com> wrote:
> On (03/01/16 10:51), Cong Wang wrote:
>> --- a/net/sched/sch_mqprio.c
>> +++ b/net/sched/sch_mqprio.c
>> @@ -142,7 +142,7 @@ static int mqprio_init(struct Qdisc *sch, struct
>> nlattr *opt)
>>          */
>>         if (qopt->hw) {
>>                 struct tc_to_netdev tc = {.type = TC_SETUP_MQPRIO,
>> -                                         .tc = qopt->num_tc};
>> +                                         {.tc = qopt->num_tc} };
>>
>
> this fix works.
>
> The first suggestion had no effect (needs changes to a lot of files
> I suspect).
>

It must work, you just didn't try it hard.

> BTW, I remain surprised that no one else has noticed this. It did not
> take me any effort to reproduce it on an x86.

I think that is probably because most people use newer gcc, at least
I don't see any compile error with gcc 4.9.2.