* Re: [PATCH v3 1/2] macvtap: include all checksum offloads in TUN_OFFLOAD mask
From: Michael S. Tsirkin @ 2013-08-15 19:09 UTC (permalink / raw)
To: Vlad Yasevich; +Cc: netdev
In-Reply-To: <520D21CA.4090400@redhat.com>
On Thu, Aug 15, 2013 at 02:45:30PM -0400, Vlad Yasevich wrote:
> On 08/15/2013 02:24 PM, Michael S. Tsirkin wrote:
> >On Thu, Aug 15, 2013 at 01:02:52PM -0400, Vlad Yasevich wrote:
> >>The features of the macvlan are based on the features of lower
> >>device and thus can have checksum featurs other then IFF_F_HW_CSUM
> >
> >s/featurs/features/
> >
> >:set spell spelllang=en_us
> >
> >or whatever's the equivalent in your editor of choice.
> >
> >>set. However, TUN_OFFLOAD mask only includes IFF_F_HW_CSUM. Thus
> >>when performing gso segmentation during macvtap_forward(),
> >>it is possbile to end up with skbs that have ip_summed set
> >>to CHECKSUM_PARTIAL. This is incorrect when the user
> >>turns off checksum offloading.
> >>
> >>Include all possible checksum offload values so that
> >>we'll properly mask them off when performing GSO.
> >>
> >>Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
> >>---
> >> drivers/net/macvtap.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >>diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
> >>index b51db2a..8121358 100644
> >>--- a/drivers/net/macvtap.c
> >>+++ b/drivers/net/macvtap.c
> >>@@ -65,7 +65,7 @@ static struct cdev macvtap_cdev;
> >>
> >> static const struct proto_ops macvtap_socket_ops;
> >>
> >>-#define TUN_OFFLOADS (NETIF_F_HW_CSUM | NETIF_F_TSO_ECN | NETIF_F_TSO | \
> >>+#define TUN_OFFLOADS (NETIF_F_ALL_CSUM | NETIF_F_TSO_ECN | NETIF_F_TSO | \
> >> NETIF_F_TSO6 | NETIF_F_UFO)
> >> #define RX_OFFLOADS (NETIF_F_GRO | NETIF_F_LRO)
> >> /*
> >
> >Okay so you are talking about hardware that sets some other
> >checksum bit besides HW_CSUM, e.g. IP_CSUM, so
> >
> > vlan->tap_features = vlan->dev->features &
> > (feature_mask | ~TUN_OFFLOADS);
> >
> >will not clear IP_CSUM even if feature_mask is 0.
> >
> >Maybe mention this in the changelog, in case user
> >will wonder whether his hardware is affected.
> >
> >So I agree, that's a bug, but if you make this change the reverse will hold
> >(on this hardware):
> >if user sets TUN_F_CSUM, we won't set NETIF_F_IP_CSUM
> >so checksum offloading won't work.
> >
> >So I think you want s/NETIF_F_HW_CSUM/NETIF_F_ALL_CSUM/ everywhere
> >and not just in this place.
>
> Yes. I thought about that, but forgot to do in this patch set.
In fact I wonder why do we do it like this at all.
tap_features have nothing to do with the device,
they only have to do with tap.
For example, you might have a dumb device with no
checksum support but userspace said it does not need a checksum,
we can have NETIF_F_HW_CSUM there anyway.
So why don't we just
vlan->tap_features = feature_mask;
?
In fact, this is exactly set_features, so why don't
we just use set_features everywhere?
Then this patch won't be needed at all.
Kind of like this - compile-tested only - do you have a setup with
IP_CSUM which you can use to test?
-->
macvtap: fix up tap features
There's no apparent need to have separate tap features
masked according to the lowerdev config:
we only use them in software so hardware configuration
does not matter.
Drop tap_features and use set_features directly
This fixes bugs for some unusual hardware configurations,
for example, we only masked HW_CSUM so for hardware with
e.g. IP_CSUM the checksum bit remained set, which would cause
packets with CHECKSUM_PARTIAL to be sent to userspace
even when offloads are disabled.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
index a98ed9f..26cea91 100644
--- a/drivers/net/macvtap.c
+++ b/drivers/net/macvtap.c
@@ -289,7 +289,7 @@ static int macvtap_forward(struct net_device *dev, struct sk_buff *skb)
/* Apply the forward feature mask so that we perform segmentation
* according to users wishes.
*/
- features = netif_skb_features(skb) & vlan->tap_features;
+ features = netif_skb_features(skb) & vlan->set_features;
if (netif_needs_gso(skb, features)) {
struct sk_buff *segs = __skb_gso_segment(skb, features, false);
@@ -382,11 +382,6 @@ static int macvtap_newlink(struct net *src_net,
struct macvlan_dev *vlan = netdev_priv(dev);
INIT_LIST_HEAD(&vlan->queue_list);
- /* Since macvlan supports all offloads by default, make
- * tap support all offloads also.
- */
- vlan->tap_features = TUN_OFFLOADS;
-
/* Don't put anything that may fail after macvlan_common_newlink
* because we can't undo what it does.
*/
@@ -1027,7 +1022,7 @@ static int set_offload(struct macvtap_queue *q, unsigned long arg)
features = vlan->dev->features;
if (arg & TUN_F_CSUM) {
- feature_mask = NETIF_F_HW_CSUM;
+ feature_mask = NETIF_F_HW_CSUM;
if (arg & (TUN_F_TSO4 | TUN_F_TSO6)) {
if (arg & TUN_F_TSO_ECN)
@@ -1055,11 +1050,6 @@ static int set_offload(struct macvtap_queue *q, unsigned long arg)
else
features &= ~RX_OFFLOADS;
- /* tap_features are the same as features on tun/tap and
- * reflect user expectations.
- */
- vlan->tap_features = vlan->dev->features &
- (feature_mask | ~TUN_OFFLOADS);
vlan->set_features = features;
netdev_update_features(vlan->dev);
diff --git a/include/linux/if_macvlan.h b/include/linux/if_macvlan.h
index ddd33fd..e446e82 100644
--- a/include/linux/if_macvlan.h
+++ b/include/linux/if_macvlan.h
@@ -76,7 +76,6 @@ struct macvlan_dev {
struct list_head queue_list;
int numvtaps;
int numqueues;
- netdev_features_t tap_features;
int minor;
};
^ permalink raw reply related
* Re: [PATCH v3 1/2] macvtap: include all checksum offloads in TUN_OFFLOAD mask
From: Michael S. Tsirkin @ 2013-08-15 19:20 UTC (permalink / raw)
To: Vlad Yasevich; +Cc: netdev
In-Reply-To: <20130815190945.GA11569@redhat.com>
On Thu, Aug 15, 2013 at 10:09:45PM +0300, Michael S. Tsirkin wrote:
> On Thu, Aug 15, 2013 at 02:45:30PM -0400, Vlad Yasevich wrote:
> > On 08/15/2013 02:24 PM, Michael S. Tsirkin wrote:
> > >On Thu, Aug 15, 2013 at 01:02:52PM -0400, Vlad Yasevich wrote:
> > >>The features of the macvlan are based on the features of lower
> > >>device and thus can have checksum featurs other then IFF_F_HW_CSUM
> > >
> > >s/featurs/features/
> > >
> > >:set spell spelllang=en_us
> > >
> > >or whatever's the equivalent in your editor of choice.
> > >
> > >>set. However, TUN_OFFLOAD mask only includes IFF_F_HW_CSUM. Thus
> > >>when performing gso segmentation during macvtap_forward(),
> > >>it is possbile to end up with skbs that have ip_summed set
> > >>to CHECKSUM_PARTIAL. This is incorrect when the user
> > >>turns off checksum offloading.
> > >>
> > >>Include all possible checksum offload values so that
> > >>we'll properly mask them off when performing GSO.
> > >>
> > >>Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
> > >>---
> > >> drivers/net/macvtap.c | 2 +-
> > >> 1 file changed, 1 insertion(+), 1 deletion(-)
> > >>
> > >>diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
> > >>index b51db2a..8121358 100644
> > >>--- a/drivers/net/macvtap.c
> > >>+++ b/drivers/net/macvtap.c
> > >>@@ -65,7 +65,7 @@ static struct cdev macvtap_cdev;
> > >>
> > >> static const struct proto_ops macvtap_socket_ops;
> > >>
> > >>-#define TUN_OFFLOADS (NETIF_F_HW_CSUM | NETIF_F_TSO_ECN | NETIF_F_TSO | \
> > >>+#define TUN_OFFLOADS (NETIF_F_ALL_CSUM | NETIF_F_TSO_ECN | NETIF_F_TSO | \
> > >> NETIF_F_TSO6 | NETIF_F_UFO)
> > >> #define RX_OFFLOADS (NETIF_F_GRO | NETIF_F_LRO)
> > >> /*
> > >
> > >Okay so you are talking about hardware that sets some other
> > >checksum bit besides HW_CSUM, e.g. IP_CSUM, so
> > >
> > > vlan->tap_features = vlan->dev->features &
> > > (feature_mask | ~TUN_OFFLOADS);
> > >
> > >will not clear IP_CSUM even if feature_mask is 0.
> > >
> > >Maybe mention this in the changelog, in case user
> > >will wonder whether his hardware is affected.
> > >
> > >So I agree, that's a bug, but if you make this change the reverse will hold
> > >(on this hardware):
> > >if user sets TUN_F_CSUM, we won't set NETIF_F_IP_CSUM
> > >so checksum offloading won't work.
> > >
> > >So I think you want s/NETIF_F_HW_CSUM/NETIF_F_ALL_CSUM/ everywhere
> > >and not just in this place.
> >
> > Yes. I thought about that, but forgot to do in this patch set.
>
> In fact I wonder why do we do it like this at all.
> tap_features have nothing to do with the device,
> they only have to do with tap.
> For example, you might have a dumb device with no
> checksum support but userspace said it does not need a checksum,
> we can have NETIF_F_HW_CSUM there anyway.
>
> So why don't we just
> vlan->tap_features = feature_mask;
> ?
> In fact, this is exactly set_features, so why don't
> we just use set_features everywhere?
> Then this patch won't be needed at all.
>
> Kind of like this - compile-tested only - do you have a setup with
> IP_CSUM which you can use to test?
Ugh this does not work even on a simple setup, sorry about the noise.
Maybe like this? Does this work for you?
-->
macvtap: fix up tap features
There's no apparent need to have tap features
masked according to the lowerdev config:
we only use them in software so hardware configuration
does not matter.
This patch fixes bugs for some unusual hardware configurations,
for example, we only masked HW_CSUM so for hardware with
e.g. IP_CSUM the checksum bit remained set, which would cause
packets with CHECKSUM_PARTIAL to be sent to userspace
even when offloads are disabled.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
index a98ed9f..3c18f12 100644
--- a/drivers/net/macvtap.c
+++ b/drivers/net/macvtap.c
@@ -1058,8 +1058,7 @@ static int set_offload(struct macvtap_queue *q, unsigned long arg)
/* tap_features are the same as features on tun/tap and
* reflect user expectations.
*/
- vlan->tap_features = vlan->dev->features &
- (feature_mask | ~TUN_OFFLOADS);
+ vlan->tap_features = feature_mask;
vlan->set_features = features;
netdev_update_features(vlan->dev);
^ permalink raw reply related
* Re: [PATCH v3 2/2] macvtap: Allow tap features change when IFF_VNET_HDR is disabled.
From: Michael S. Tsirkin @ 2013-08-15 19:27 UTC (permalink / raw)
To: Vlad Yasevich; +Cc: netdev
In-Reply-To: <520D24F5.3000602@redhat.com>
On Thu, Aug 15, 2013 at 02:59:01PM -0400, Vlad Yasevich wrote:
> On 08/15/2013 02:48 PM, Michael S. Tsirkin wrote:
> >On Thu, Aug 15, 2013 at 02:39:39PM -0400, Vlad Yasevich wrote:
> >>On 08/15/2013 01:33 PM, Michael S. Tsirkin wrote:
> >>>On Thu, Aug 15, 2013 at 01:02:53PM -0400, Vlad Yasevich wrote:
> >>>>When the user turns off IFF_VNET_HDR flag, attempts to change
> >>>>offload features via TUNSETOFFLOAD do not work. This could cause
> >>>>GSO packets to be delivered to the user when the user is
> >>>>not prepared to handle them.
> >>>>
> >>>>To solve, allow processing of TUNSETOFFLOAD when IFF_VNET_HDR is
> >>>>disabled. Also, take the setting of IFF_VNET_HDR into consideration
> >>>>when determining the feature set to apply to incommig traffic.
> >>>>If IFF_VNET_HDR is set, we use user-specfied feature set. if the
> >>>>IFF_VNET_HDR is clear, we mask off all tun-specific offload features,
> >>>>since user can not be notified of the possbile offloads.
> >>>>
> >>>>Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
> >>>>---
> >>>> drivers/net/macvtap.c | 30 ++++++++++++++++++++++++------
> >>>> 1 file changed, 24 insertions(+), 6 deletions(-)
> >>>>
> >>>>diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
> >>>>index 8121358..df45a59 100644
> >>>>--- a/drivers/net/macvtap.c
> >>>>+++ b/drivers/net/macvtap.c
> >>>>@@ -269,6 +269,28 @@ static void macvtap_del_queues(struct net_device *dev)
> >>>> sock_put(&qlist[j]->sk);
> >>>> }
> >>>>
> >>>>+/* Determine features to be applied to the packet as it
> >>>
> >>>/*
> >>> * Please use comments
> >>> * like this
> >>> */
> >>>
> >>>>+ * gets sent to the tap. The features depend on the offloads
> >>>>+ * set by user and by whether or not IFF_VNET_HDR is enabled.
> >>>>+ */
> >>>>+static netdev_features_t macvtap_skb_features(struct sk_buff *skb,
> >>>>+ struct macvtap_queue *q)
> >>>>+{
> >>>>+ struct macvlan_dev *vlan = netdev_priv(skb->dev);
> >>>>+ netdev_features_t features = netif_skb_features(skb);
> >>>>+
> >>>>+ /* If VNET_HDR is enabled, user can receive offload info so
> >>>>+ * use user-specified tap_features.
> >>>>+ * Otherwise, mask off all tap offload features.
> >>>>+ */
> >>>>+ if (q->flags & IFF_VNET_HDR)
> >>>>+ features &= vlan->tap_features;
> >>>>+ else
> >>>>+ features &= ~TUN_OFFLOADS;
> >>>>+
> >>>>+ return features;
> >>>>+}
> >>>>+
> >>>
> >>>Okay, but this means we need to do netdev_update_features
> >>>on SETIFF since it changes q->flags.
> >>
> >>Why?
> >> The whole point it not to change features on SETIFF, so that
> >>they are preserved for when the flags change again. It just that
> >>flag values either allow whatever offload user specified (correctly
> >>masked via netdev_update_features) or turn off all offloads at the tap.
> >>
> >>-vlad
> >
> >That's the point. As it is, if flag value changes, we don't call
> >netdev_update_features so offloads aren't masked/unmasked.
> >
> >Example:
> >
> >1. SETOFFLOADS to enable
> >2. VNET = 0
> >
> >offloads are still enabled
> >
>
> But unused since since VNET=0.
Well, I think that's true but it would still be better
to disable RX offloads, no?
> I am trying to be careful not to
> break things when ioctl calls get all inverted and reversed, but
> may be I am trying too hard and we should just let things break.
What exactly will break by call to netdev_update_features?
> I just feels kinds of silly since the app may now know all the
> details, but the kernel does and can do the "right thing".
>
> -vlad
>
> >
> >>>> /*
> >>>> * Forward happens for data that gets sent from one macvlan
> >>>> * endpoint to another one in bridge mode. We just take
> >>>>@@ -276,9 +298,9 @@ static void macvtap_del_queues(struct net_device *dev)
> >>>> */
> >>>> static int macvtap_forward(struct net_device *dev, struct sk_buff *skb)
> >>>> {
> >>>>- struct macvlan_dev *vlan = netdev_priv(dev);
> >>>> struct macvtap_queue *q = macvtap_get_queue(dev, skb);
> >>>> netdev_features_t features;
> >>>>+
> >>>> if (!q)
> >>>> goto drop;
> >>>>
> >>>>@@ -289,7 +311,7 @@ static int macvtap_forward(struct net_device *dev, struct sk_buff *skb)
> >>>> /* Apply the forward feature mask so that we perform segmentation
> >>>> * according to users wishes.
> >>>> */
> >>>>- features = netif_skb_features(skb) & vlan->tap_features;
> >>>>+ features = macvtap_skb_features(skb, q);
> >>>> if (netif_needs_gso(skb, features)) {
> >>>> struct sk_buff *segs = __skb_gso_segment(skb, features, false);
> >>>>
> >>>>@@ -1161,10 +1183,6 @@ static long macvtap_ioctl(struct file *file, unsigned int cmd,
> >>>> TUN_F_TSO_ECN | TUN_F_UFO))
> >>>> return -EINVAL;
> >>>>
> >>>>- /* TODO: only accept frames with the features that
> >>>>- got enabled for forwarded frames */
> >>>>- if (!(q->flags & IFF_VNET_HDR))
> >>>>- return -EINVAL;
> >>>> rtnl_lock();
> >>>> ret = set_offload(q, arg);
> >>>> rtnl_unlock();
> >>>>--
> >>>>1.8.1.4
^ permalink raw reply
* Re: [PATCH v3 1/2] macvtap: include all checksum offloads in TUN_OFFLOAD mask
From: Vlad Yasevich @ 2013-08-15 19:26 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: netdev
In-Reply-To: <20130815192046.GA11788@redhat.com>
On 08/15/2013 03:20 PM, Michael S. Tsirkin wrote:
> On Thu, Aug 15, 2013 at 10:09:45PM +0300, Michael S. Tsirkin wrote:
>> On Thu, Aug 15, 2013 at 02:45:30PM -0400, Vlad Yasevich wrote:
>>> On 08/15/2013 02:24 PM, Michael S. Tsirkin wrote:
>>>> On Thu, Aug 15, 2013 at 01:02:52PM -0400, Vlad Yasevich wrote:
>>>>> The features of the macvlan are based on the features of lower
>>>>> device and thus can have checksum featurs other then IFF_F_HW_CSUM
>>>>
>>>> s/featurs/features/
>>>>
>>>> :set spell spelllang=en_us
>>>>
>>>> or whatever's the equivalent in your editor of choice.
>>>>
>>>>> set. However, TUN_OFFLOAD mask only includes IFF_F_HW_CSUM. Thus
>>>>> when performing gso segmentation during macvtap_forward(),
>>>>> it is possbile to end up with skbs that have ip_summed set
>>>>> to CHECKSUM_PARTIAL. This is incorrect when the user
>>>>> turns off checksum offloading.
>>>>>
>>>>> Include all possible checksum offload values so that
>>>>> we'll properly mask them off when performing GSO.
>>>>>
>>>>> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
>>>>> ---
>>>>> drivers/net/macvtap.c | 2 +-
>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
>>>>> index b51db2a..8121358 100644
>>>>> --- a/drivers/net/macvtap.c
>>>>> +++ b/drivers/net/macvtap.c
>>>>> @@ -65,7 +65,7 @@ static struct cdev macvtap_cdev;
>>>>>
>>>>> static const struct proto_ops macvtap_socket_ops;
>>>>>
>>>>> -#define TUN_OFFLOADS (NETIF_F_HW_CSUM | NETIF_F_TSO_ECN | NETIF_F_TSO | \
>>>>> +#define TUN_OFFLOADS (NETIF_F_ALL_CSUM | NETIF_F_TSO_ECN | NETIF_F_TSO | \
>>>>> NETIF_F_TSO6 | NETIF_F_UFO)
>>>>> #define RX_OFFLOADS (NETIF_F_GRO | NETIF_F_LRO)
>>>>> /*
>>>>
>>>> Okay so you are talking about hardware that sets some other
>>>> checksum bit besides HW_CSUM, e.g. IP_CSUM, so
>>>>
>>>> vlan->tap_features = vlan->dev->features &
>>>> (feature_mask | ~TUN_OFFLOADS);
>>>>
>>>> will not clear IP_CSUM even if feature_mask is 0.
>>>>
>>>> Maybe mention this in the changelog, in case user
>>>> will wonder whether his hardware is affected.
>>>>
>>>> So I agree, that's a bug, but if you make this change the reverse will hold
>>>> (on this hardware):
>>>> if user sets TUN_F_CSUM, we won't set NETIF_F_IP_CSUM
>>>> so checksum offloading won't work.
>>>>
>>>> So I think you want s/NETIF_F_HW_CSUM/NETIF_F_ALL_CSUM/ everywhere
>>>> and not just in this place.
>>>
>>> Yes. I thought about that, but forgot to do in this patch set.
>>
>> In fact I wonder why do we do it like this at all.
>> tap_features have nothing to do with the device,
>> they only have to do with tap.
>> For example, you might have a dumb device with no
>> checksum support but userspace said it does not need a checksum,
>> we can have NETIF_F_HW_CSUM there anyway.
>>
>> So why don't we just
>> vlan->tap_features = feature_mask;
>> ?
>> In fact, this is exactly set_features, so why don't
>> we just use set_features everywhere?
>> Then this patch won't be needed at all.
>>
>> Kind of like this - compile-tested only - do you have a setup with
>> IP_CSUM which you can use to test?
>
> Ugh this does not work even on a simple setup, sorry about the noise.
> Maybe like this? Does this work for you?
>
No, this is really no different then what I had to start with.
> -->
>
> macvtap: fix up tap features
>
> There's no apparent need to have tap features
> masked according to the lowerdev config:
> we only use them in software so hardware configuration
> does not matter.
>
> This patch fixes bugs for some unusual hardware configurations,
> for example, we only masked HW_CSUM so for hardware with
> e.g. IP_CSUM the checksum bit remained set, which would cause
> packets with CHECKSUM_PARTIAL to be sent to userspace
> even when offloads are disabled.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> ---
>
> diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
> index a98ed9f..3c18f12 100644
> --- a/drivers/net/macvtap.c
> +++ b/drivers/net/macvtap.c
> @@ -1058,8 +1058,7 @@ static int set_offload(struct macvtap_queue *q, unsigned long arg)
> /* tap_features are the same as features on tun/tap and
> * reflect user expectations.
> */
> - vlan->tap_features = vlan->dev->features &
> - (feature_mask | ~TUN_OFFLOADS);
> + vlan->tap_features = feature_mask;
This wouldn't work when tap_features = 0, since the skb features in
forward would evaluate to 0. This means that netif_needs_gso() will
return false and a GSO packet will be queued to the socket without going
though segmentation.
-vlad
> vlan->set_features = features;
> netdev_update_features(vlan->dev);
>
>
^ permalink raw reply
* Re: [PATCH v4 4/5] ARM: davinci: da850: add DT node for eth0.
From: Sergei Shtylyov @ 2013-08-15 19:30 UTC (permalink / raw)
To: Lad, Prabhakar
Cc: Sekhar Nori, DLOS, LKML, devicetree-discuss, linux-arm-kernel,
netdev
In-Reply-To: <1376546497-26931-5-git-send-email-prabhakar.csengg@gmail.com>
Hello.
On 15-08-2013 10:01, Lad, Prabhakar wrote:
> From: "Lad, Prabhakar" <prabhakar.csengg@gmail.com>
> Add eth0 device tree node information and pinmux for mii to da850 by
> providing interrupt details and local mac address of eth0.
> Signed-off-by: Lad, Prabhakar <prabhakar.csengg@gmail.com>
[...]
> @@ -226,6 +242,20 @@
> #size-cells = <0>;
> reg = <0x224000 0x1000>;
> };
> + eth0: emac@1e20000 {
According to the ePAPR spec[1] section 2.2.2, the node should be named
"ethernet".
[1] http://www.power.org/resources/downloads/Power_ePAPR_APPROVED_v1.0.pdf
WBR, Sergei
^ permalink raw reply
* Re: [PATCH v4 5/5] ARM: davinci: da850: add OF_DEV_AUXDATA entry for eth0.
From: Sergei Shtylyov @ 2013-08-15 19:31 UTC (permalink / raw)
To: Lad, Prabhakar
Cc: Sekhar Nori, DLOS, LKML, devicetree-discuss, linux-arm-kernel,
netdev
In-Reply-To: <1376546497-26931-6-git-send-email-prabhakar.csengg@gmail.com>
Hello.
On 15-08-2013 10:01, Lad, Prabhakar wrote:
> From: "Lad, Prabhakar" <prabhakar.csengg@gmail.com>
> Add OF_DEV_AUXDATA for eth0 driver in da850 board dt
> file to use emac clock.
> Signed-off-by: Lad, Prabhakar <prabhakar.csengg@gmail.com>
[...]
> diff --git a/arch/arm/mach-davinci/da8xx-dt.c b/arch/arm/mach-davinci/da8xx-dt.c
> index d172563..caa9202 100644
> --- a/arch/arm/mach-davinci/da8xx-dt.c
> +++ b/arch/arm/mach-davinci/da8xx-dt.c
> @@ -44,6 +44,9 @@ static struct of_dev_auxdata da850_auxdata_lookup[] __initdata = {
> OF_DEV_AUXDATA("ns16550a", 0x01d0c000, "serial8250.1", NULL),
> OF_DEV_AUXDATA("ns16550a", 0x01d0d000, "serial8250.2", NULL),
> OF_DEV_AUXDATA("ti,davinci_mdio", 0x01e24000, "davinci_mdio.0", NULL),
> + OF_DEV_AUXDATA("ti,davinci-dm6467-emac", 0x01e20000, "davinci_emac.1",
> + NULL),
> +
Empty line hardly needed here.
> {}
> };
WBR, Sergei
^ permalink raw reply
* Re: [PATCH v3 1/2] macvtap: include all checksum offloads in TUN_OFFLOAD mask
From: Michael S. Tsirkin @ 2013-08-15 19:44 UTC (permalink / raw)
To: Vlad Yasevich; +Cc: netdev
In-Reply-To: <520D2B60.2010503@redhat.com>
On Thu, Aug 15, 2013 at 03:26:24PM -0400, Vlad Yasevich wrote:
> On 08/15/2013 03:20 PM, Michael S. Tsirkin wrote:
> >On Thu, Aug 15, 2013 at 10:09:45PM +0300, Michael S. Tsirkin wrote:
> >>On Thu, Aug 15, 2013 at 02:45:30PM -0400, Vlad Yasevich wrote:
> >>>On 08/15/2013 02:24 PM, Michael S. Tsirkin wrote:
> >>>>On Thu, Aug 15, 2013 at 01:02:52PM -0400, Vlad Yasevich wrote:
> >>>>>The features of the macvlan are based on the features of lower
> >>>>>device and thus can have checksum featurs other then IFF_F_HW_CSUM
> >>>>
> >>>>s/featurs/features/
> >>>>
> >>>>:set spell spelllang=en_us
> >>>>
> >>>>or whatever's the equivalent in your editor of choice.
> >>>>
> >>>>>set. However, TUN_OFFLOAD mask only includes IFF_F_HW_CSUM. Thus
> >>>>>when performing gso segmentation during macvtap_forward(),
> >>>>>it is possbile to end up with skbs that have ip_summed set
> >>>>>to CHECKSUM_PARTIAL. This is incorrect when the user
> >>>>>turns off checksum offloading.
> >>>>>
> >>>>>Include all possible checksum offload values so that
> >>>>>we'll properly mask them off when performing GSO.
> >>>>>
> >>>>>Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
> >>>>>---
> >>>>> drivers/net/macvtap.c | 2 +-
> >>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>>
> >>>>>diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
> >>>>>index b51db2a..8121358 100644
> >>>>>--- a/drivers/net/macvtap.c
> >>>>>+++ b/drivers/net/macvtap.c
> >>>>>@@ -65,7 +65,7 @@ static struct cdev macvtap_cdev;
> >>>>>
> >>>>> static const struct proto_ops macvtap_socket_ops;
> >>>>>
> >>>>>-#define TUN_OFFLOADS (NETIF_F_HW_CSUM | NETIF_F_TSO_ECN | NETIF_F_TSO | \
> >>>>>+#define TUN_OFFLOADS (NETIF_F_ALL_CSUM | NETIF_F_TSO_ECN | NETIF_F_TSO | \
> >>>>> NETIF_F_TSO6 | NETIF_F_UFO)
> >>>>> #define RX_OFFLOADS (NETIF_F_GRO | NETIF_F_LRO)
> >>>>> /*
> >>>>
> >>>>Okay so you are talking about hardware that sets some other
> >>>>checksum bit besides HW_CSUM, e.g. IP_CSUM, so
> >>>>
> >>>> vlan->tap_features = vlan->dev->features &
> >>>> (feature_mask | ~TUN_OFFLOADS);
> >>>>
> >>>>will not clear IP_CSUM even if feature_mask is 0.
> >>>>
> >>>>Maybe mention this in the changelog, in case user
> >>>>will wonder whether his hardware is affected.
> >>>>
> >>>>So I agree, that's a bug, but if you make this change the reverse will hold
> >>>>(on this hardware):
> >>>>if user sets TUN_F_CSUM, we won't set NETIF_F_IP_CSUM
> >>>>so checksum offloading won't work.
> >>>>
> >>>>So I think you want s/NETIF_F_HW_CSUM/NETIF_F_ALL_CSUM/ everywhere
> >>>>and not just in this place.
> >>>
> >>>Yes. I thought about that, but forgot to do in this patch set.
> >>
> >>In fact I wonder why do we do it like this at all.
> >>tap_features have nothing to do with the device,
> >>they only have to do with tap.
> >>For example, you might have a dumb device with no
> >>checksum support but userspace said it does not need a checksum,
> >>we can have NETIF_F_HW_CSUM there anyway.
> >>
> >>So why don't we just
> >> vlan->tap_features = feature_mask;
> >>?
> >>In fact, this is exactly set_features, so why don't
> >>we just use set_features everywhere?
> >>Then this patch won't be needed at all.
> >>
> >>Kind of like this - compile-tested only - do you have a setup with
> >>IP_CSUM which you can use to test?
> >
> >Ugh this does not work even on a simple setup, sorry about the noise.
> >Maybe like this? Does this work for you?
> >
>
> No, this is really no different then what I had to start with.
>
> >-->
> >
> >macvtap: fix up tap features
> >
> >There's no apparent need to have tap features
> >masked according to the lowerdev config:
> >we only use them in software so hardware configuration
> >does not matter.
> >
> >This patch fixes bugs for some unusual hardware configurations,
> >for example, we only masked HW_CSUM so for hardware with
> >e.g. IP_CSUM the checksum bit remained set, which would cause
> >packets with CHECKSUM_PARTIAL to be sent to userspace
> >even when offloads are disabled.
> >
> >Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >
> >---
> >
> >diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
> >index a98ed9f..3c18f12 100644
> >--- a/drivers/net/macvtap.c
> >+++ b/drivers/net/macvtap.c
> >@@ -1058,8 +1058,7 @@ static int set_offload(struct macvtap_queue *q, unsigned long arg)
> > /* tap_features are the same as features on tun/tap and
> > * reflect user expectations.
> > */
> >- vlan->tap_features = vlan->dev->features &
> >- (feature_mask | ~TUN_OFFLOADS);
> >+ vlan->tap_features = feature_mask;
>
> This wouldn't work when tap_features = 0, since the skb features in
> forward would evaluate to 0. This means that netif_needs_gso() will
> return false and a GSO packet will be queued to the socket without
> going though segmentation.
>
> -vlad
>
Hmm won't same thing happen if features == 0 on lowerdev?
> > vlan->set_features = features;
> > netdev_update_features(vlan->dev);
> >
> >
^ permalink raw reply
* Re: [RFC] phy: micrel: Convert micrel PHY driver to use OF
From: Linus Walleij @ 2013-08-15 19:55 UTC (permalink / raw)
To: Dinh Nguyen, devicetree@vger.kernel.org
Cc: Dinh Nguyen, netdev@vger.kernel.org, Richard Cochran,
Felipe Balbi, David S. Miller, Giuseppe Cavallaro, Olof Johansson,
Rob Herring
In-Reply-To: <1376412156-3899-1-git-send-email-dinguyen@altera.com>
On Tue, Aug 13, 2013 at 6:42 PM, <dinguyen@altera.com> wrote:
> From: Dinh Nguyen <dinguyen@altera.com>
>
> Convert the Micrel PHY driver to use OF. This initial patch is only
> the beginning of an idea to convert the PHY driver to device tree.
>
> Signed-of-by: Dinh Nguyen <dinguyen@altera.com>
This is full of device tree stuff so please copy it to
devicetree@vger.kernel.org.
It seems to add quite a few compatible strings so these should
be added to the bindings doc, or is it already?
Yours,
Linus Walleij
^ permalink raw reply
* Re: [patch] tun: signedness bug in tun_get_user()
From: Neil Horman @ 2013-08-15 20:08 UTC (permalink / raw)
To: Dan Carpenter
Cc: David S. Miller, Jason Wang, Michael S. Tsirkin, Eric Dumazet,
netdev, kernel-janitors
In-Reply-To: <20130815125257.GA16932@elgon.mountain>
On Thu, Aug 15, 2013 at 03:52:57PM +0300, Dan Carpenter wrote:
> The recent fix d9bf5f1309 "tun: compare with 0 instead of total_len" is
> not totally correct. Because "len" and "sizeof()" are size_t type, that
> means they are never less than zero.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index af987f0..7ed13cc 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -977,8 +977,9 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
> u32 rxhash;
>
> if (!(tun->flags & TUN_NO_PI)) {
> - if ((len -= sizeof(pi)) < 0)
> + if (len < sizeof(pi))
> return -EINVAL;
> + len -= sizeof(pi);
>
> if (memcpy_fromiovecend((void *)&pi, iv, 0, sizeof(pi)))
> return -EFAULT;
> @@ -986,8 +987,9 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
> }
>
> if (tun->flags & TUN_VNET_HDR) {
> - if ((len -= tun->vnet_hdr_sz) < 0)
> + if (len < tun->vnet_hdr_sz)
> return -EINVAL;
> + len -= tun->vnet_hdr_sz;
>
> if (memcpy_fromiovecend((void *)&gso, iv, offset, sizeof(gso)))
> return -EFAULT;
>
Acked-by: Neil Horman <nhorman@tuxdriver.com>
^ permalink raw reply
* Re: [PATCH v3 2/2] macvtap: Allow tap features change when IFF_VNET_HDR is disabled.
From: Vlad Yasevich @ 2013-08-15 19:36 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: netdev
In-Reply-To: <20130815192704.GA11826@redhat.com>
On 08/15/2013 03:27 PM, Michael S. Tsirkin wrote:
> On Thu, Aug 15, 2013 at 02:59:01PM -0400, Vlad Yasevich wrote:
>> On 08/15/2013 02:48 PM, Michael S. Tsirkin wrote:
>>> On Thu, Aug 15, 2013 at 02:39:39PM -0400, Vlad Yasevich wrote:
>>>> On 08/15/2013 01:33 PM, Michael S. Tsirkin wrote:
>>>>> On Thu, Aug 15, 2013 at 01:02:53PM -0400, Vlad Yasevich wrote:
>>>>>> When the user turns off IFF_VNET_HDR flag, attempts to change
>>>>>> offload features via TUNSETOFFLOAD do not work. This could cause
>>>>>> GSO packets to be delivered to the user when the user is
>>>>>> not prepared to handle them.
>>>>>>
>>>>>> To solve, allow processing of TUNSETOFFLOAD when IFF_VNET_HDR is
>>>>>> disabled. Also, take the setting of IFF_VNET_HDR into consideration
>>>>>> when determining the feature set to apply to incommig traffic.
>>>>>> If IFF_VNET_HDR is set, we use user-specfied feature set. if the
>>>>>> IFF_VNET_HDR is clear, we mask off all tun-specific offload features,
>>>>>> since user can not be notified of the possbile offloads.
>>>>>>
>>>>>> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
>>>>>> ---
>>>>>> drivers/net/macvtap.c | 30 ++++++++++++++++++++++++------
>>>>>> 1 file changed, 24 insertions(+), 6 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
>>>>>> index 8121358..df45a59 100644
>>>>>> --- a/drivers/net/macvtap.c
>>>>>> +++ b/drivers/net/macvtap.c
>>>>>> @@ -269,6 +269,28 @@ static void macvtap_del_queues(struct net_device *dev)
>>>>>> sock_put(&qlist[j]->sk);
>>>>>> }
>>>>>>
>>>>>> +/* Determine features to be applied to the packet as it
>>>>>
>>>>> /*
>>>>> * Please use comments
>>>>> * like this
>>>>> */
>>>>>
>>>>>> + * gets sent to the tap. The features depend on the offloads
>>>>>> + * set by user and by whether or not IFF_VNET_HDR is enabled.
>>>>>> + */
>>>>>> +static netdev_features_t macvtap_skb_features(struct sk_buff *skb,
>>>>>> + struct macvtap_queue *q)
>>>>>> +{
>>>>>> + struct macvlan_dev *vlan = netdev_priv(skb->dev);
>>>>>> + netdev_features_t features = netif_skb_features(skb);
>>>>>> +
>>>>>> + /* If VNET_HDR is enabled, user can receive offload info so
>>>>>> + * use user-specified tap_features.
>>>>>> + * Otherwise, mask off all tap offload features.
>>>>>> + */
>>>>>> + if (q->flags & IFF_VNET_HDR)
>>>>>> + features &= vlan->tap_features;
>>>>>> + else
>>>>>> + features &= ~TUN_OFFLOADS;
>>>>>> +
>>>>>> + return features;
>>>>>> +}
>>>>>> +
>>>>>
>>>>> Okay, but this means we need to do netdev_update_features
>>>>> on SETIFF since it changes q->flags.
>>>>
>>>> Why?
>>>> The whole point it not to change features on SETIFF, so that
>>>> they are preserved for when the flags change again. It just that
>>>> flag values either allow whatever offload user specified (correctly
>>>> masked via netdev_update_features) or turn off all offloads at the tap.
>>>>
>>>> -vlad
>>>
>>> That's the point. As it is, if flag value changes, we don't call
>>> netdev_update_features so offloads aren't masked/unmasked.
>>>
>>> Example:
>>>
>>> 1. SETOFFLOADS to enable
>>> 2. VNET = 0
>>>
>>> offloads are still enabled
>>>
>>
>> But unused since since VNET=0.
>
>
> Well, I think that's true but it would still be better
> to disable RX offloads, no?
Disabled RX offloads doesn't do anything since rx offload
processing only happens on the lowest level device and not
macvlan. I've toyed with disabling RX offloads on the lowest
device when all macvlans above it had RX disabled, but the
performance was lower then performing GSO. Didn't dig into
the why.
>
>> I am trying to be careful not to
>> break things when ioctl calls get all inverted and reversed, but
>> may be I am trying too hard and we should just let things break.
>
> What exactly will break by call to netdev_update_features?
I was referring to the proposed patch, not to calling
netdev_update_features. Sorry for the confusion.
There macvlan device features aren't changing, so why call
netdev_update_features().
The v1 of the patch attempted to change the features upon
flag change, and you didn't like that.
The way I see it, we have 2 options:
1) Don't bother with tracking the state of VNET_HDR, and let
broken apps find out the hard way that they are broken. This
would reduce this patch to just removing the 'if' check from
SETOFFLOADS ioctl.
2) Try to be smart and handle the situation where VNET_HDR is
turned off while offloads still set. Then we need essentially
ignore the state the offloads. We could change them, but then
there would be no way to restore them and we'll have asymmetric
behavior (v2 of the patch). This v3, removes that asymmetry.
I guess I'd rather we be smart and handle the situation for the clueless
user/app, but I am OK with option 1 as well.
-vlad
>
>> I just feels kinds of silly since the app may now know all the
>> details, but the kernel does and can do the "right thing".
>>
>> -vlad
>>
>>>
>>>>>> /*
>>>>>> * Forward happens for data that gets sent from one macvlan
>>>>>> * endpoint to another one in bridge mode. We just take
>>>>>> @@ -276,9 +298,9 @@ static void macvtap_del_queues(struct net_device *dev)
>>>>>> */
>>>>>> static int macvtap_forward(struct net_device *dev, struct sk_buff *skb)
>>>>>> {
>>>>>> - struct macvlan_dev *vlan = netdev_priv(dev);
>>>>>> struct macvtap_queue *q = macvtap_get_queue(dev, skb);
>>>>>> netdev_features_t features;
>>>>>> +
>>>>>> if (!q)
>>>>>> goto drop;
>>>>>>
>>>>>> @@ -289,7 +311,7 @@ static int macvtap_forward(struct net_device *dev, struct sk_buff *skb)
>>>>>> /* Apply the forward feature mask so that we perform segmentation
>>>>>> * according to users wishes.
>>>>>> */
>>>>>> - features = netif_skb_features(skb) & vlan->tap_features;
>>>>>> + features = macvtap_skb_features(skb, q);
>>>>>> if (netif_needs_gso(skb, features)) {
>>>>>> struct sk_buff *segs = __skb_gso_segment(skb, features, false);
>>>>>>
>>>>>> @@ -1161,10 +1183,6 @@ static long macvtap_ioctl(struct file *file, unsigned int cmd,
>>>>>> TUN_F_TSO_ECN | TUN_F_UFO))
>>>>>> return -EINVAL;
>>>>>>
>>>>>> - /* TODO: only accept frames with the features that
>>>>>> - got enabled for forwarded frames */
>>>>>> - if (!(q->flags & IFF_VNET_HDR))
>>>>>> - return -EINVAL;
>>>>>> rtnl_lock();
>>>>>> ret = set_offload(q, arg);
>>>>>> rtnl_unlock();
>>>>>> --
>>>>>> 1.8.1.4
^ permalink raw reply
* Re: [PATCH net] tun: compare with 0 instead of total_len
From: Ben Hutchings @ 2013-08-15 20:14 UTC (permalink / raw)
To: David Miller; +Cc: wpan, netdev
In-Reply-To: <20130813.192943.865924891130626725.davem@davemloft.net>
On Tue, 2013-08-13 at 19:29 -0700, David Miller wrote:
> From: Weiping Pan <wpan@redhat.com>
> Date: Tue, 13 Aug 2013 21:46:56 +0800
>
> > Since we set "len = total_len" in the beginning of tun_get_user(),
> > so we should compare the new len with 0, instead of total_len,
> > or the if statement always returns false.
> >
> > Signed-off-by: Weiping Pan <wpan@redhat.com>
>
> Definitely looks correct to me, applied, thanks.
len is unsigned. So while len > total_len is a poor way of checking for
underflow, len < 0 is much worse!
Ben.
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* Re: [PATCH v3 2/2] macvtap: Allow tap features change when IFF_VNET_HDR is disabled.
From: David Miller @ 2013-08-15 20:16 UTC (permalink / raw)
To: mst; +Cc: vyasevic, netdev
In-Reply-To: <20130815173347.GA10265@redhat.com>
From: "Michael S. Tsirkin" <mst@redhat.com>
Date: Thu, 15 Aug 2013 20:33:47 +0300
> On Thu, Aug 15, 2013 at 01:02:53PM -0400, Vlad Yasevich wrote:
>> When the user turns off IFF_VNET_HDR flag, attempts to change
>> offload features via TUNSETOFFLOAD do not work. This could cause
>> GSO packets to be delivered to the user when the user is
>> not prepared to handle them.
>>
>> To solve, allow processing of TUNSETOFFLOAD when IFF_VNET_HDR is
>> disabled. Also, take the setting of IFF_VNET_HDR into consideration
>> when determining the feature set to apply to incommig traffic.
>> If IFF_VNET_HDR is set, we use user-specfied feature set. if the
>> IFF_VNET_HDR is clear, we mask off all tun-specific offload features,
>> since user can not be notified of the possbile offloads.
>>
>> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
>> ---
>> drivers/net/macvtap.c | 30 ++++++++++++++++++++++++------
>> 1 file changed, 24 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
>> index 8121358..df45a59 100644
>> --- a/drivers/net/macvtap.c
>> +++ b/drivers/net/macvtap.c
>> @@ -269,6 +269,28 @@ static void macvtap_del_queues(struct net_device *dev)
>> sock_put(&qlist[j]->sk);
>> }
>>
>> +/* Determine features to be applied to the packet as it
>
> /*
> * Please use comments
> * like this
> */
Not in the networking, we use:
/* This
* style.
*/
Your misguiding people on this issue makes it hard for me
to get people to do things properly.
Thanks.
^ permalink raw reply
* Re: [PATCH net] net: tg3: fix NULL pointer dereference in tg3_io_error_detected and tg3_io_slot_reset
From: David Miller @ 2013-08-15 20:17 UTC (permalink / raw)
To: nsujir; +Cc: netdev, dborkman, shangw, mchan
In-Reply-To: <520D10CA.7040600@broadcom.com>
From: "Nithin Nayak Sujir" <nsujir@broadcom.com>
Date: Thu, 15 Aug 2013 10:32:58 -0700
> Sorry, I forgot to add Michael's Signed-off-by. Do you want me to
> resend it with his Signed-off-by added?
That's not necessary, it gets added automatically by patchwork
since he replied and added it.
^ permalink raw reply
* Re: [RFC][PATCH 2/2] bgmac: pass received packet to the netif instead of copying it
From: Rafał Miłecki @ 2013-08-15 20:21 UTC (permalink / raw)
To: Felix Fietkau
Cc: Network Development, Jonas Gorski, Hauke Mehrtens,
OpenWrt Development List
In-Reply-To: <520CBFD6.6030600@openwrt.org>
2013/8/15 Felix Fietkau <nbd@openwrt.org>:
> On 2013-08-15 1:36 PM, Rafał Miłecki wrote:
>> 2013/8/11 Rafał Miłecki <zajec5@gmail.com>:
>>> It makes more sense to allocate new (empty) skb and pass it to the
>>> hardware. That way we avoid copying whole packet into new skb which
>>> should result in better performance.
>>
>> I did some testing of this patch using "perf" tool and iperf -s
>> running on the OpenWrt machine (with bgmac supported hardware).
>>
>> So you can see that __copy_user_common usage has really decreased with
>> this patch!
>>
>> Unfortunately it didn't result in better performance... no idea why :(
> Running iperf on the router is not useful as an indicator of routing
> performance. Please focus on tests where you only push traffic through
> the router, not directly to it.
OK, so I started "iperf -s" on notebook plugged into WAN port, and
then played with "iperf -c" on notebook connected to LAN#2.
With some old 3.6.11 based OpenWrt build I got:
[ 4] 0.0-60.0 sec 690 MBytes 96.4 Mbits/sec
With very recent 3.10.4 based OpenWrt build:
[ 4] 0.0-60.0 sec 667 MBytes 93.2 Mbits/sec
After applying my patch on top of that 3.10.4:
[ 5] 0.0-60.0 sec 759 MBytes 106 Mbits/sec
And some dumps from "perf top":
3.10.4
6.75% [kernel] [k] __copy_user_common
6.73% [ip_tables] [k] ipt_do_table
4.33% [kernel] [k] arch_cpu_idle
3.96% [kernel] [k] arch_local_irq_restore
3.42% [bgmac] [k] 0x000007e0
3.35% [nf_conntrack] [k] nf_conntrack_proto_fini
2.72% [nf_conntrack] [k] nf_conntrack_in
2.50% [kernel] [k] __netif_receive_skb_core
2.42% [kernel] [k] r4k_dma_cache_inv
2.38% [kernel] [k] fib_table_lookup
2.20% [kernel] [k] dev_queue_xmit
2.11% [xt_conntrack] [k] 0x00000360
2.10% [kernel] [k] ip_route_input_noref
2.06% [nf_conntrack_ipv4] [k] need_ipv4_conntrack
3.10.4 + 0002-bgmac-pass-received-packet-to-the-netif-instead-of-c.patch
6.09% [ip_tables] [k] ipt_do_table
4.71% [kernel] [k] arch_cpu_idle
4.48% [bgmac] [k] 0x00000d7c
3.50% [nf_conntrack] [k] nf_conntrack_in
3.22% [kernel] [k] arch_local_irq_restore
3.16% [nf_conntrack] [k] nf_conntrack_proto_fini
2.88% [kernel] [k] __netif_receive_skb_core
2.78% [xt_conntrack] [k] 0x0000011c
2.69% [kernel] [k] r4k_dma_cache_inv
2.67% [iptable_nat] [k] 0x000002a0
2.36% [kernel] [k] ip_route_input_noref
2.27% [kernel] [k] ip_rcv
2.25% [nf_conntrack_ipv4] [k] need_ipv4_conntrack
2.23% [kernel] [k] nf_iterate
I've compiled bgmac into the kernel and it seems that the magic 0xd7c
was bgmac_poll.
I'm afraid this "perf top" output doesn't really tell us where to look
for optimizations :| I'll still try Felix ideas tomorrow, but I'm not
sure if they help, since there isn't __copy_user_common anymore in the
"perf top" output...
--
Rafał
_______________________________________________
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel
^ permalink raw reply
* Re: [PATCH] r8169: fix invalid register dump
From: Ben Hutchings @ 2013-08-15 20:33 UTC (permalink / raw)
To: Peter Wu; +Cc: Francois Romieu, netdev, nic_swsd
In-Reply-To: <2045708.ru9COLib4d@al>
On Wed, 2013-08-14 at 23:31 +0200, Peter Wu wrote:
> On Wednesday 14 August 2013 21:58:29 Francois Romieu wrote:
> > > - memcpy_fromio(p, tp->mmio_addr, regs->len);
> > > + if (regs->len >= 4) {
> > > + for (i = 0; i < regs->len - 4; i += 4)
> > > + memcpy_fromio(bytes + i, tp->mmio_addr + i, 4);
> > > + }
> > > + if (i < regs->len)
> >
> > Comparison with random stack stuff when regs->len < 4. :o/
>
> Right, let's rm $OLD_PATCH and consider this one.
>
> Checklist:
> 1. super large regs->len: won't be greater than R8169_REGS_SIZE (256)
> 2. regs->len == 0: 0 < 0 is false, nothing is copied
> 3. regs->len is 1, 2 or 3: i = 0, at most 3 bytes will be copied
> 4. regs->len is 4, i < 4 - 4, skip loop, 0 < regs->len, copy 4
> 5. regs->len is 5, i < 5 - 4, copy; 4 < regs->len, copy 1
[...]
The kernel buffer size is max(regs->len,
dev->ethtool_ops->get_regs_len()). So you can safely ignore regs->len
and always read all your registers.
Ben.
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* Re: [PATCH] macvtap: fix up direction in comment on offloading
From: Amos Kong @ 2013-08-15 20:41 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: linux-kernel, David S. Miller, Jason Wang, Eric Dumazet,
Vlad Yasevich, netdev
In-Reply-To: <20130815174626.GA10874@redhat.com>
On Fri, Aug 16, 2013 at 1:46 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> It speaks about receiving frames, so while
> it says GSO, it really means GRO.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> drivers/net/macvtap.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
> index a98fb0e..a98ed9f 100644
> --- a/drivers/net/macvtap.c
> +++ b/drivers/net/macvtap.c
> @@ -1047,7 +1047,7 @@ static int set_offload(struct macvtap_queue *q, unsigned long arg)
> * accept TSO frames and turning it off means that user space
> * does not support TSO.
> * For macvtap, we have to invert it to mean the same thing.
> - * When user space turns off TSO, we turn off GSO/LRO so that
> + * When user space turns off TSO, we turn off GRO/LRO so that
Right fix.
Reviewed-by: Amos Kong <kongjianjun@gmail.com>
> * user-space will not receive TSO frames.
> */
> if (feature_mask & (NETIF_F_TSO | NETIF_F_TSO6 | NETIF_F_UFO))
> --
> MST
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply
* Re: [PATCH net] net: tg3: fix NULL pointer dereference in tg3_io_error_detected and tg3_io_slot_reset
From: David Miller @ 2013-08-15 20:41 UTC (permalink / raw)
To: mchan; +Cc: nsujir, netdev, dborkman, shangw
In-Reply-To: <1376579720.9648.2.camel@LTIRV-MCHAN1.corp.ad.broadcom.com>
From: "Michael Chan" <mchan@broadcom.com>
Date: Thu, 15 Aug 2013 08:15:20 -0700
> On Thu, 2013-08-15 at 01:07 -0700, David Miller wrote:
>> From: "Nithin Nayak Sujir" <nsujir@broadcom.com>
>> Date: Tue, 13 Aug 2013 11:45:13 -0700
>>
>> > From: Daniel Borkmann <dborkman@redhat.com>
>> >
>> > Commit d8af4dfd8 ("net/tg3: Fix kernel crash") introduced a possible
>> > NULL pointer dereference in tg3 driver when !netdev || !netif_running(netdev)
>> > condition is met and netdev is NULL. Then, the jump to the 'done' label
>> > calls dev_close() with a netdevice that is NULL. Therefore, only call
>> > dev_close() when we have a netdevice, but one that is not running.
>> >
>> > [ Add the same checks in tg3_io_slot_reset() per Gavin Shan - by Nithin
>> > Nayak Sujir ]
>> >
>> > Reported-by: Dave Jones <davej@redhat.com>
>> > Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
>> > Cc: Gavin Shan <shangw@linux.vnet.ibm.com>
>> > Cc: Michael Chan <mchan@broadcom.com>
>> > Signed-off-by: Nithin Nayak Sujir <nsujir@broadcom.com>
>>
>> Can I get some reviews from the Broadcom folks?
>>
>
> We worked with Daniel at Red Hat and Gavin at IBM on this patch.
> Thanks.
>
> Signed-off-by: Michael Chan <mchan@broadcom.com>
Thanks for the info, applied.
^ permalink raw reply
* Re: [PATCH v3 2/2] macvtap: Allow tap features change when IFF_VNET_HDR is disabled.
From: Michael S. Tsirkin @ 2013-08-15 20:52 UTC (permalink / raw)
To: David Miller; +Cc: vyasevic, netdev
In-Reply-To: <20130815.131645.1042317207899492632.davem@davemloft.net>
On Thu, Aug 15, 2013 at 01:16:45PM -0700, David Miller wrote:
> From: "Michael S. Tsirkin" <mst@redhat.com>
> Date: Thu, 15 Aug 2013 20:33:47 +0300
>
> > On Thu, Aug 15, 2013 at 01:02:53PM -0400, Vlad Yasevich wrote:
> >> When the user turns off IFF_VNET_HDR flag, attempts to change
> >> offload features via TUNSETOFFLOAD do not work. This could cause
> >> GSO packets to be delivered to the user when the user is
> >> not prepared to handle them.
> >>
> >> To solve, allow processing of TUNSETOFFLOAD when IFF_VNET_HDR is
> >> disabled. Also, take the setting of IFF_VNET_HDR into consideration
> >> when determining the feature set to apply to incommig traffic.
> >> If IFF_VNET_HDR is set, we use user-specfied feature set. if the
> >> IFF_VNET_HDR is clear, we mask off all tun-specific offload features,
> >> since user can not be notified of the possbile offloads.
> >>
> >> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
> >> ---
> >> drivers/net/macvtap.c | 30 ++++++++++++++++++++++++------
> >> 1 file changed, 24 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
> >> index 8121358..df45a59 100644
> >> --- a/drivers/net/macvtap.c
> >> +++ b/drivers/net/macvtap.c
> >> @@ -269,6 +269,28 @@ static void macvtap_del_queues(struct net_device *dev)
> >> sock_put(&qlist[j]->sk);
> >> }
> >>
> >> +/* Determine features to be applied to the packet as it
> >
> > /*
> > * Please use comments
> > * like this
> > */
>
> Not in the networking, we use:
>
> /* This
> * style.
> */
>
> Your misguiding people on this issue makes it hard for me
> to get people to do things properly.
>
> Thanks.
OK in that case let's fix the rest of macvtap so I can stop getting
confused?
--->
macvtap: fix style for multiline comments
In the networking, we use:
/* This
* style.
*/
fix up macvtap to stop misguiding people on this issue.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
index a98ed9f..04b7575 100644
--- a/drivers/net/macvtap.c
+++ b/drivers/net/macvtap.c
@@ -22,15 +22,13 @@
#include <net/sock.h>
#include <linux/virtio_net.h>
-/*
- * A macvtap queue is the central object of this driver, it connects
+/* A macvtap queue is the central object of this driver, it connects
* an open character device to a macvlan interface. There can be
* multiple queues on one interface, which map back to queues
* implemented in hardware on the underlying device.
*
* macvtap_proto is used to allocate queues through the sock allocation
* mechanism.
- *
*/
struct macvtap_queue {
struct sock sk;
@@ -51,9 +49,7 @@ static struct proto macvtap_proto = {
.obj_size = sizeof (struct macvtap_queue),
};
-/*
- * Variables for dealing with macvtaps device numbers.
- */
+/* Variables for dealing with macvtaps device numbers. */
static dev_t macvtap_major;
#define MACVTAP_NUM_DEVS (1U << MINORBITS)
static DEFINE_MUTEX(minor_lock);
@@ -68,8 +64,8 @@ static const struct proto_ops macvtap_socket_ops;
#define TUN_OFFLOADS (NETIF_F_HW_CSUM | NETIF_F_TSO_ECN | NETIF_F_TSO | \
NETIF_F_TSO6 | NETIF_F_UFO)
#define RX_OFFLOADS (NETIF_F_GRO | NETIF_F_LRO)
-/*
- * RCU usage:
+
+/* RCU usage:
* The macvtap_queue and the macvlan_dev are loosely coupled, the
* pointers from one to the other can only be read while rcu_read_lock
* or rtnl is held.
@@ -162,8 +158,7 @@ static int macvtap_disable_queue(struct macvtap_queue *q)
return 0;
}
-/*
- * The file owning the queue got closed, give up both
+/* The file owning the queue got closed, give up both
* the reference that the files holds as well as the
* one from the macvlan_dev if that still exists.
*
@@ -193,8 +188,7 @@ static void macvtap_put_queue(struct macvtap_queue *q)
sock_put(&q->sk);
}
-/*
- * Select a queue based on the rxq of the device on which this packet
+/* Select a queue based on the rxq of the device on which this packet
* arrived. If the incoming device is not mq, calculate a flow hash
* to select a queue. If all fails, find the first available queue.
* Cache vlan->numvtaps since it can become zero during the execution
@@ -238,8 +232,7 @@ out:
return tap;
}
-/*
- * The net_device is going away, give up the reference
+/* The net_device is going away, give up the reference
* that it holds on all queues and safely set the pointer
* from the queues to NULL.
*/
@@ -269,8 +262,7 @@ static void macvtap_del_queues(struct net_device *dev)
sock_put(&qlist[j]->sk);
}
-/*
- * Forward happens for data that gets sent from one macvlan
+/* Forward happens for data that gets sent from one macvlan
* endpoint to another one in bridge mode. We just take
* the skb and put it into the receive queue.
*/
@@ -322,8 +314,7 @@ drop:
return NET_RX_DROP;
}
-/*
- * Receive is for data from the external interface (lowerdev),
+/* Receive is for data from the external interface (lowerdev),
* in case of macvtap, we can treat that the same way as
* forward, which macvlan cannot.
*/
@@ -462,8 +453,7 @@ static int macvtap_open(struct inode *inode, struct file *file)
q->flags = IFF_VNET_HDR | IFF_NO_PI | IFF_TAP;
q->vnet_hdr_sz = sizeof(struct virtio_net_hdr);
- /*
- * so far only KVM virtio_net uses macvtap, enable zero copy between
+ /* so far only KVM virtio_net uses macvtap, enable zero copy between
* guest kernel and host kernel when lower device supports zerocopy
*
* The macvlan supports zerocopy iff the lower device supports zero
@@ -616,8 +606,7 @@ static int zerocopy_sg_from_iovec(struct sk_buff *skb, const struct iovec *from,
return 0;
}
-/*
- * macvtap_skb_from_vnet_hdr and macvtap_skb_to_vnet_hdr should
+/* macvtap_skb_from_vnet_hdr and macvtap_skb_to_vnet_hdr should
* be shared with the tun/tap driver.
*/
static int macvtap_skb_from_vnet_hdr(struct sk_buff *skb,
@@ -1066,8 +1055,7 @@ static int set_offload(struct macvtap_queue *q, unsigned long arg)
return 0;
}
-/*
- * provide compatibility with generic tun/tap interface
+/* provide compatibility with generic tun/tap interface
*/
static long macvtap_ioctl(struct file *file, unsigned int cmd,
unsigned long arg)
@@ -1225,7 +1213,8 @@ static const struct proto_ops macvtap_socket_ops = {
/* Get an underlying socket object from tun file. Returns error unless file is
* attached to a device. The returned object works like a packet socket, it
* can be used for sock_sendmsg/sock_recvmsg. The caller is responsible for
- * holding a reference to the file for as long as the socket is in use. */
+ * holding a reference to the file for as long as the socket is in use.
+ */
struct socket *macvtap_get_socket(struct file *file)
{
struct macvtap_queue *q;
^ permalink raw reply related
* Re: [PATCH net-next 1/2] ipv4: IP_TOS and IP_TTL can be specified as ancillary data
From: David Miller @ 2013-08-15 20:51 UTC (permalink / raw)
To: ffusco; +Cc: netdev
In-Reply-To: <92ffe52615a79259cd7cc120e504a9cf41dd546e.1376494031.git.ffusco@redhat.com>
From: Francesco Fusco <ffusco@redhat.com>
Date: Wed, 14 Aug 2013 17:48:38 +0200
> + __s16 ttl;
> + __s16 tos;
You're range validating these values to make sure they are always
between 1 and 255, inclusive.
Why use a 16-bit value, and in particular a signed one? That makes
absolutely no sense at all.
These fit perfectly in a "u8" so use that instead.
^ permalink raw reply
* Re: [PATCH] vhost: Drop linux/socket.h
From: David Miller @ 2013-08-15 21:07 UTC (permalink / raw)
To: asias; +Cc: netdev, virtualization, kvm, mst
In-Reply-To: <1376536816-10951-1-git-send-email-asias@redhat.com>
From: Asias He <asias@redhat.com>
Date: Thu, 15 Aug 2013 11:20:16 +0800
> memcpy_fromiovec is moved to lib/iovec.c. No need to include
> linux/socket.h for it.
>
> Signed-off-by: Asias He <asias@redhat.com>
You can't do this.
Because this file doesn't include the header file that
provides the declaration, which is linux/uio.h
linux/socket.h includes linux/uio.h, so honestly leaving
things the way they are is a 1000 times better than your
patch.
^ permalink raw reply
* Re: [PATCH] bond: Don't set skb->queue_mapping in netpoll.
From: Sergei Shtylyov @ 2013-08-15 21:07 UTC (permalink / raw)
To: Tao Ma; +Cc: netdev, linux-kernel, David S. Miller, Cong Wang, Eric Dumazet
In-Reply-To: <1376555808-6531-1-git-send-email-tm@tao.ma>
On 08/15/2013 12:36 PM, Tao Ma wrote:
> From: Tao Ma <boyu.mt@taobao.com>
> When we are using netpoll, we don't go through the normal
> transmit process. In this case, bond_select_queue is not called
> and qdisc_skb_cb(skb)->slave_dev_queue_mapping isn't set.
> So when netpoll_send_skb_on_dev calls ndo_start_xmit and we
> enter bond_dev_queue_xmit, we will set skb->queue_mapping to
> an invalid value and in some cases cause the driver panic the
> kernel(We meet with bnx2 panic because of a very large queue_mapping).
> This patch skip skb->queue_mapping if we find we are in netpoll.
> CC: "David S. Miller" <davem@davemloft.net>
> CC: Cong Wang <amwang@redhat.com>
> CC: Eric Dumazet <eric.dumazet@gmail.com>
> Signed-off-by: Tao Ma <boyu.mt@taobao.com>
> ---
> drivers/net/bonding/bond_main.c | 5 +++--
> 1 files changed, 3 insertions(+), 2 deletions(-)
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 07f257d..97b2f52 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -405,12 +405,13 @@ int bond_dev_queue_xmit(struct bonding *bond, struct sk_buff *skb,
>
> BUILD_BUG_ON(sizeof(skb->queue_mapping) !=
> sizeof(qdisc_skb_cb(skb)->slave_dev_queue_mapping));
> - skb->queue_mapping = qdisc_skb_cb(skb)->slave_dev_queue_mapping;
>
> if (unlikely(netpoll_tx_running(bond->dev)))
> bond_netpoll_send_skb(bond_get_slave_by_dev(bond, slave_dev), skb);
> - else
> + else {
> + skb->queue_mapping = qdisc_skb_cb(skb)->slave_dev_queue_mapping;
> dev_queue_xmit(skb);
> + }
According to Documentation/CodingStyle, both arms of the *if* statement
should have {} if one arm has them.
WBR, Sergei
^ permalink raw reply
* Re: [PATCH] driver:net:stmmac: Disable DMA store and forward mode if platform data force_sf_dma_mode is negative.
From: David Miller @ 2013-08-15 21:20 UTC (permalink / raw)
To: sonic.adi; +Cc: peppe.cavallaro, netdev, adi-buildroot-devel, sonic.zhang
In-Reply-To: <1376552256-7421-1-git-send-email-sonic.adi@gmail.com>
From: Sonic Zhang <sonic.adi@gmail.com>
Date: Thu, 15 Aug 2013 15:37:36 +0800
> @@ -1157,7 +1157,9 @@ static void free_dma_desc_resources(struct stmmac_priv *priv)
> */
> static void stmmac_dma_operation_mode(struct stmmac_priv *priv)
> {
> - if (likely(priv->plat->force_sf_dma_mode ||
> + if (priv->plat->force_sf_dma_mode < 0)
> + priv->hw->dma->dma_mode(priv->ioaddr, tc, tc);
> + else if (likely(priv->plat->force_sf_dma_mode > 0 ||
> ((priv->plat->tx_coe) && (!priv->no_csum_insertion)))) {
You need to properly re-indent the last line here so that
the openning parenthesis lines up with the first column
after the openning parenthesis on the "else if" line.
^ permalink raw reply
* Re: [PATCHv2 net-next] xfrm: Make xfrm_state timer monotonic
From: David Miller @ 2013-08-15 21:23 UTC (permalink / raw)
To: fan.du; +Cc: steffen.klassert, netdev
In-Reply-To: <1376552946-18967-1-git-send-email-fan.du@windriver.com>
From: Fan Du <fan.du@windriver.com>
Date: Thu, 15 Aug 2013 15:49:06 +0800
> xfrm_state timer should be independent of system clock change,
> so switch to CLOCK_BOOTTIME base which is not only monotonic but
> also counting suspend time.
>
> Thus issue reported in commit: 9e0d57fd6dad37d72a3ca6db00ca8c76f2215454
> ("xfrm: SAD entries do not expire correctly after suspend-resume")
> could ALSO be avoided.
>
> Signed-off-by: Fan Du <fan.du@windriver.com>
>
> v2: Use CLOCK_BOOTTIME to count suspend time, but still monotonic.
This seems like a good fix:
Acked-by: David S. Miller <davem@davemloft.net>
^ permalink raw reply
* Re: [PATCH net 0/3] qlcnic: bug fixes
From: David Miller @ 2013-08-15 21:35 UTC (permalink / raw)
To: sucheta.chakraborty; +Cc: netdev, Dept-HSGLinuxNICDev
In-Reply-To: <1376569769-625-1-git-send-email-sucheta.chakraborty@qlogic.com>
From: Sucheta Chakraborty <sucheta.chakraborty@qlogic.com>
Date: Thu, 15 Aug 2013 08:29:26 -0400
> Please apply to net.
Series applied, thanks.
^ permalink raw reply
* Re: [PATCH net-next] net: proc_fs: trivial: print UIDs as unsigned int
From: David Miller @ 2013-08-15 21:38 UTC (permalink / raw)
To: ffusco; +Cc: netdev
In-Reply-To: <bee30aaa49e84c82c8a7ca9939e7321e2c0a741d.1376566630.git.ffusco@redhat.com>
From: Francesco Fusco <ffusco@redhat.com>
Date: Thu, 15 Aug 2013 13:42:14 +0200
> UIDs are printed in the proc_fs as signed int, whereas
> they are unsigned int.
>
> Signed-off-by: Francesco Fusco <ffusco@redhat.com>
Applied, thanks.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox