* Re: [PATCH net 09/10] net: hns: adds limitation for debug port mtu
From: Yisen Zhuang @ 2016-03-22 1:19 UTC (permalink / raw)
To: Sergei Shtylyov
Cc: davem, yankejian, huangdaode, salil.mehta, lisheng011, lipeng321,
liguozhu, arnd, xieqianqian, andrew, ivecera, netdev,
linux-kernel, linuxarm
In-Reply-To: <56F0028D.4070500@cogentembedded.com>
在 2016/3/21 22:17, Sergei Shtylyov 写道:
> Hello.
>
> On 03/21/2016 02:06 PM, Yisen Zhuang wrote:
>
>> From: Kejian Yan <yankejian@huawei.com>
>>
>> If mtu for debug port is set more than 1500, it may cause that packets
>> are dropped by ppe. So maximum value for debug port should be 1500.
>>
>> Signed-off-by: Kejian Yan <yankejian@huawei.com>
>> Signed-off-by: Yisen Zhuang <Yisen.Zhuang@huawei.com>
> [...]
>> diff --git a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.h b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.h
>> index a69c8af..a68efd6 100644
>> --- a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.h
>> +++ b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.h
>> @@ -28,6 +28,7 @@ struct dsaf_device;
>> #define MAC_MAX_MTU 9600
>> #define MAC_MAX_MTU_V2 9728
>> #define MAC_MIN_MTU 68
>> +#define MAC_MAX_MTU_DBG MAC_DEFAULT_MTU
>
> Please use tabs, not spaces to indent the value, like above
Will fix it in next version.
Thanks,
Yisen
>
> [...]
>
> MBR, Sergei
>
>
> .
>
^ permalink raw reply
* Re: [PATCH net 06/10] net: hns: fix return value of the function about rss
From: Yisen Zhuang @ 2016-03-22 1:16 UTC (permalink / raw)
To: David Miller
Cc: yankejian, huangdaode, salil.mehta, lisheng011, lipeng321,
liguozhu, arnd, xieqianqian, andrew, ivecera, netdev,
linux-kernel, linuxarm
In-Reply-To: <20160321.114238.1469581550499984000.davem@davemloft.net>
在 2016/3/21 23:42, David Miller 写道:
> From: Yisen Zhuang <Yisen.Zhuang@huawei.com>
> Date: Mon, 21 Mar 2016 19:06:37 +0800
>
>> diff --git a/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c b/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c
>> index 3c4a3bc..f3a5e05 100644
>> --- a/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c
>> +++ b/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c
> ...
>> @@ -1235,12 +1226,11 @@ hns_set_rss(struct net_device *netdev, const u32 *indir, const u8 *key,
>> {
>> struct hns_nic_priv *priv = netdev_priv(netdev);
>> struct hnae_ae_ops *ops;
>> - int ret;
>>
>> if (AE_IS_VER1(priv->enet_ver)) {
>> netdev_err(netdev,
>> "RSS feature is not supported on this hardware\n");
>> - return -EOPNOTSUPP;
>> + return (u32)-EOPNOTSUPP;
>> }
>>
>
> This is incredibly broken, you cannot cast this negative error return value to
> an unsigned integer.
>
Hi David,
This is my misunderstanding of the return value. Will fix it in next version.
Many thanks,
Yisen
> .
>
^ permalink raw reply
* Re: [PATCH net 03/10] net: hns: add uc match for debug ports
From: Yisen Zhuang @ 2016-03-22 1:03 UTC (permalink / raw)
To: David Miller
Cc: yankejian, huangdaode, salil.mehta, lisheng011, lipeng321,
liguozhu, arnd, xieqianqian, andrew, ivecera, netdev,
linux-kernel, linuxarm
In-Reply-To: <20160321.114055.845640249977716034.davem@davemloft.net>
在 2016/3/21 23:40, David Miller 写道:
> From: Yisen Zhuang <Yisen.Zhuang@huawei.com>
> Date: Mon, 21 Mar 2016 19:06:34 +0800
>
>> + (void)hns_mac_set_promisc(mac_cb, (u8)!!en);
>
> This cast to void is unnecssary.
I think hns_mac_set_promisc don't need to return a value， so i will fix it in next submit.
Thanks
>
>> +static void hns_gmac_set_uc_match(void *mac_drv, u16 en)
>> +{
>> + struct mac_driver *drv = (struct mac_driver *)mac_drv;
>
> Casts from void pointers are unnecessary.
Will fix it in next submit.
Thanks
>
>> +{
>> + struct mac_driver *drv = (struct mac_driver *)mac_drv;
>
> Likewise.
Will fix it in next submit.
Thanks,
Yisen
>
> .
>
^ permalink raw reply
* Re: [PATCH net 01/10] net: hns: bug fix about ping6
From: Yisen Zhuang @ 2016-03-22 0:46 UTC (permalink / raw)
To: David Miller
Cc: yankejian, huangdaode, salil.mehta, lisheng011, lipeng321,
liguozhu, arnd, xieqianqian, andrew, ivecera, netdev,
linux-kernel, linuxarm
In-Reply-To: <20160321.113924.1745428836278929494.davem@davemloft.net>
在 2016/3/21 23:39, David Miller 写道:
> From: Yisen Zhuang <Yisen.Zhuang@huawei.com>
> Date: Mon, 21 Mar 2016 19:06:32 +0800
>
>> From: Kejian Yan <yankejian@huawei.com>
>>
>> The current upstreaming code fails to ping other IPv6 net device, because
>> the enet receives the multicast packets with the src mac addr whick is the
> ^^^^^
>
> "which"
Will fix it in next submit.
Thanks
>
>> @@ -670,6 +671,14 @@ out_bnum_err:
>> return -EFAULT;
>> }
>>
>> + /* filter out multicast pkt with the same src mac as this port */
>> + eh = (struct ethhdr *)skb->data;
>
> Please use the "eth_hdr()" helper.
Will fix it in next submit.
Thanks,
Yisen
>
> .
>
^ permalink raw reply
* [PATCH 1/1] net: ipv4: Fix truncated timestamp returned by inet_current_timestamp()
From: Deepa Dinamani @ 2016-03-22 0:41 UTC (permalink / raw)
To: y2038, netdev
Cc: David S. Miller, Alexey Kuznetsov, Hideaki YOSHIFUJI,
James Morris, Patrick McHardy, Arnd Bergmann
The millisecond timestamps returned by the function is
converted to network byte order by making a call to htons().
htons() only returns __be16 while __be32 is required here.
This was identified by the sparse warning from the buildbot:
net/ipv4/af_inet.c:1405:16: sparse: incorrect type in return
expression (different base types)
net/ipv4/af_inet.c:1405:16: expected restricted __be32
net/ipv4/af_inet.c:1405:16: got restricted __be16 [usertype] <noident>
Change the function to use htonl() to return the correct __be32 type
instead so that the millisecond value doesn't get truncated.
Signed-off-by: Deepa Dinamani <deepa.kernel@gmail.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Alexey Kuznetsov <kuznet@ms2.inr.ac.ru>
Cc: Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>
Cc: James Morris <jmorris@namei.org>
Cc: Patrick McHardy <kaber@trash.net>
Cc: Arnd Bergmann <arnd@arndb.de>
Fixes: 822c868532ca ("net: ipv4: Convert IP network timestamps to be y2038 safe")
Reported-by: Fengguang Wu <fengguang.wu@intel.com> [0-day test robot]
---
net/ipv4/af_inet.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
index 0cc923f..5fab7e3 100644
--- a/net/ipv4/af_inet.c
+++ b/net/ipv4/af_inet.c
@@ -1402,7 +1402,7 @@ __be32 inet_current_timestamp(void)
msecs += (u32)ts.tv_nsec / NSEC_PER_MSEC;
/* Convert to network byte order. */
- return htons(msecs);
+ return htonl(msecs);
}
EXPORT_SYMBOL(inet_current_timestamp);
--
1.9.1
^ permalink raw reply related
* Make DST_CACHE a silent config option
From: Dave Jones @ 2016-03-21 22:37 UTC (permalink / raw)
To: netdev
commit 911362c70d ("net: add dst_cache support") added a new
kconfig option that gets selected by other networking options.
It seems the intent wasn't to offer this as a user-selectable
option given the lack of help text, so this patch converts it
to a silent option.
Signed-off-by: Dave Jones <davej@codemonkey.org.uk>
diff --git a/net/Kconfig b/net/Kconfig
index e13449870d06..a8934d8c8fda 100644
--- a/net/Kconfig
+++ b/net/Kconfig
@@ -397,7 +397,7 @@ config LWTUNNEL
with light weight tunnel state associated with fib routes.
config DST_CACHE
- bool "dst cache"
+ bool
default n
config NET_DEVLINK
^ permalink raw reply related
* Re: [PATCH] net: phy: at803x: don't depend on GPIOLIB
From: Sergei Shtylyov @ 2016-03-21 21:56 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Sebastian Frias, David S. Miller, netdev, lkml, mason,
Daniel Mack
In-Reply-To: <20160321204111.GB6191@pengutronix.de>
On 03/21/2016 11:41 PM, Uwe Kleine-König wrote:
>>> Commit 687908c2b649 ("net: phy: at803x: simplify using
>>> devm_gpiod_get_optional and its 4th argument") introduced a dependency
>>> on GPIOLIB that was not there before.
>>>
>>> This commit removes such dependency by checking the return code and
>>> comparing it against ENOSYS which is returned when GPIOLIB is not
>>> selected.
>>>
>>> Fixes: 687908c2b649 ("net: phy: at803x: simplify using
>>> devm_gpiod_get_optional and its 4th argument")
>>>
>>> Signed-off-by: Sebastian Frias <sf84@laposte.net>
>>
>> Do you have the PHY that requires the GPIO reset workaround?
>> Askinjg because I have the patch adding the "reset-gpios" prop handling to
>> phylib and your patch made me aware that I'll have to modify this driver in
>> order to do that...
>>
>>> ---
>>> drivers/net/phy/at803x.c | 4 +++-
>>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
>>> index 2174ec9..88b7ff3 100644
>>> --- a/drivers/net/phy/at803x.c
>>> +++ b/drivers/net/phy/at803x.c
>>> @@ -252,7 +252,9 @@ static int at803x_probe(struct phy_device *phydev)
>>> return -ENOMEM;
>>>
>>> gpiod_reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
>>> - if (IS_ERR(gpiod_reset))
>>> + if (PTR_ERR(gpiod_reset) == -ENOSYS)
>>> + gpiod_reset = NULL;
>>> + else if (IS_ERR(gpiod_reset))
>> return PTR_ERR(gpiod_reset);
>>
>> My patch basically gets rid of all this code. The thing that worries me
>> is that the driver assumes that the reset singal is active low, despite what
>> the GPIO specifier in the device tree has for the GPIO polarity. In fact, it
>> will only work correctly if the specified has GPIO_ACTIVE_HIGH -- which is
>> wrong because the reset signal is active low!
>
> Note that gpio descriptors handle the polarity just fine (i.e. the pin
> is set to 0 after doing gpiod_set_value(1) if the gpio is active low).
I know. :-)
> But having said that, the driver gets it wrong.
>
> The right sequence to reset a device using a gpio is:
>
> gpiod_set_value(priv->gpiod_reset, 1);
> msleep(some_time);
> gpiod_set_value(priv->gpiod_reset, 0);
>
> and if the gpio is active low, this should be specified in the device
> tree. This was done wrong in 13a56b449325 (net: phy: at803x: Add support
> for hardware reset).
I wonder if that was done before GPIO_ACTIVE_* thing was introduced...
there are precedents in other MAC drivers that want a separate
"phy-reset-active-low" or even -"high" prop...
> Best regards
> Uwe
MBR, Sergei
^ permalink raw reply
* Re: [PATCH] net: Add missing kernel-doc for netdev ptype lists
From: Luis de Bethencourt @ 2016-03-21 21:47 UTC (permalink / raw)
To: Benjamin Poirier, netdev; +Cc: Salam Noureddine
In-Reply-To: <1458594508-14545-1-git-send-email-bpoirier@suse.com>
On 21/03/16 21:08, Benjamin Poirier wrote:
> .//include/linux/netdevice.h:1826: warning: No description found for parameter 'ptype_all'
> .//include/linux/netdevice.h:1826: warning: No description found for parameter 'ptype_specific'
>
> Introduced by commit 7866a621043f ("dev: add per net_device packet type
> chains")
>
> Cc: Salam Noureddine <noureddine@arista.com>
> Signed-off-by: Benjamin Poirier <bpoirier@suse.com>
> ---
> include/linux/netdevice.h | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 9a3d55c..1937bdd 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -1418,6 +1418,8 @@ enum netdev_priv_flags {
> * @unreg_list: List entry, that is used, when we are unregistering the
> * device, see the function unregister_netdev
> * @close_list: List entry, that is used, when we are closing the device
> + * @ptype_all: Device-specific packet handlers for all protocols
> + * @ptype_specific: Device-specific, protocol-specific packet handlers
> *
> * @adj_list: Directly linked devices, like slaves for bonding
> * @all_adj_list: All linked devices, *including* neighbours
>
Looks good to me.
Thanks Benjamin :)
Luis
^ permalink raw reply
* Re: [PATCH net-next] change nfqueue failopen to apply also to receive message buffer in addition to queue size
From: Pablo Neira Ayuso @ 2016-03-21 21:35 UTC (permalink / raw)
To: Yigal Reiss (yreiss)
Cc: 'netdev@vger.kernel.org', netfilter-devel@vger.kernel.org,
Florian Westphal (fw@strlen.de)
In-Reply-To: <2ba8dceec36a41149598e43f09af048e@XCH-RTP-014.cisco.com>
[-- Attachment #1: Type: text/plain, Size: 1493 bytes --]
On Mon, Mar 21, 2016 at 11:23:43AM +0000, Yigal Reiss (yreiss) wrote:
> @@ -582,10 +585,17 @@ __nfqnl_enqueue_packet(struct net *net, struct nfqnl_instance *queue,
> *packet_id_ptr = htonl(entry->id);
>
> /* nfnetlink_unicast will either free the nskb or add it to a socket */
> - err = nfnetlink_unicast(nskb, net, queue->peer_portid, MSG_DONTWAIT);
> + err = nfnetlink_unicast_nofree(nskb, net, queue->peer_portid, MSG_DONTWAIT);
This keeps nskb around, this skbuff contains the netlink message, not
the network packet itself that is located in entry->skb.
> if (err < 0) {
> - queue->queue_user_dropped++;
> - goto err_out_unlock;
> + if (queue->flags & NFQA_CFG_F_FAIL_OPEN) {
> + queue->nobuf_failopened++;
> + failopen = 1;
> + err = 0;
In case we couldn't deliver due to socket buffer overrun, if the
NFQA_CFG_F_FAIL_OPEN flag is set, you set failopen to 1.
> + }
> + else {
> + queue->queue_user_dropped++;
> + }
> + goto err_out_free_nskb;
And finally, jump to err_out_free_nskb.
> }
>
> __enqueue_entry(queue, entry);
> @@ -595,7 +605,6 @@ __nfqnl_enqueue_packet(struct net *net, struct nfqnl_instance *queue,
>
> err_out_free_nskb:
> kfree_skb(nskb);
Which just releases the netlink skbuff.
> -err_out_unlock:
> spin_unlock_bh(&queue->lock);
> if (failopen)
> nf_reinject(entry, NF_ACCEPT);
And reinjects the packet.
So isn't the more simple patch that I'm attaching achieving what you need?
Let me know, thanks.
[-- Attachment #2: x.patch --]
[-- Type: text/x-diff, Size: 628 bytes --]
diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c
index 7542999..cb5b630 100644
--- a/net/netfilter/nfnetlink_queue.c
+++ b/net/netfilter/nfnetlink_queue.c
@@ -582,7 +582,12 @@ __nfqnl_enqueue_packet(struct net *net, struct nfqnl_instance *queue,
/* nfnetlink_unicast will either free the nskb or add it to a socket */
err = nfnetlink_unicast(nskb, net, queue->peer_portid, MSG_DONTWAIT);
if (err < 0) {
- queue->queue_user_dropped++;
+ if (queue->flags & NFQA_CFG_F_FAIL_OPEN) {
+ failopen = 1;
+ err = 0;
+ } else {
+ queue->queue_user_dropped++;
+ }
goto err_out_unlock;
}
^ permalink raw reply related
* [PATCH] net: Add missing kernel-doc for netdev ptype lists
From: Benjamin Poirier @ 2016-03-21 21:08 UTC (permalink / raw)
To: netdev; +Cc: Luis de Bethencourt, Salam Noureddine
In-Reply-To: <56F0612E.20607@osg.samsung.com>
.//include/linux/netdevice.h:1826: warning: No description found for parameter 'ptype_all'
.//include/linux/netdevice.h:1826: warning: No description found for parameter 'ptype_specific'
Introduced by commit 7866a621043f ("dev: add per net_device packet type
chains")
Cc: Salam Noureddine <noureddine@arista.com>
Signed-off-by: Benjamin Poirier <bpoirier@suse.com>
---
include/linux/netdevice.h | 2 ++
1 file changed, 2 insertions(+)
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 9a3d55c..1937bdd 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1418,6 +1418,8 @@ enum netdev_priv_flags {
* @unreg_list: List entry, that is used, when we are unregistering the
* device, see the function unregister_netdev
* @close_list: List entry, that is used, when we are closing the device
+ * @ptype_all: Device-specific packet handlers for all protocols
+ * @ptype_specific: Device-specific, protocol-specific packet handlers
*
* @adj_list: Directly linked devices, like slaves for bonding
* @all_adj_list: All linked devices, *including* neighbours
--
2.7.2
^ permalink raw reply related
* RE: [PATCH] lan78xx: Protect runtime_auto check by #ifdef CONFIG_PM
From: Alan Stern @ 2016-03-21 21:02 UTC (permalink / raw)
To: Woojung.Huh-UWL1GkI3JZL3oGB3hsPCZA
Cc: oneukum-IBi9RG/b67k, geert-Td1EMuHUCqxL1ZNQvxDV9g,
UNGLinuxDriver-UWL1GkI3JZL3oGB3hsPCZA,
davem-fT/PcQaiUtIeIZ0/mPfg9Q, linux-0h96xk9xTtrk1uMJSBkQmQ,
rjw-LthD3rsA81gm4RdzfppkhA, netdev-u79uwXL29TY76Z2rM5mHXA,
linux-usb-u79uwXL29TY76Z2rM5mHXA, linux-pm-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <9235D6609DB808459E95D78E17F2E43D404CC1EF-8fD0d2gESGrABDY0R+s2SH2Vsf1/G1/XQQ4Iyu8u01E@public.gmane.org>
On Mon, 21 Mar 2016 Woojung.Huh-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org wrote:
> > > > But this leaves open the issue that querying the device too often will
> > > > prevent it from going into autosuspend. It seems to me that the best
> > > > way to deal with this is to make sure that the autosuspend timeout is
> > > > shorter than the interal between queries, not to make the querying
> > > > conditional on !runtime_auto.
> > >
> > > Short autosuspend timeout can affect performance. For instance our
> > experiments showed that
> > > shorter than 10sec timeout made Ethernet performance degrade because
> > of wakeup delays.
> > > So, just putting shorter timeout may have some side effects.
> >
> > Sure. This just means that you need a long statistics interval --
> > longer than the autosuspend timeout. That's why I suggested making the
> > interval adjustable.
>
> What do you mean statistics interval?
> Interval calling ndo_get_stats64 or another thread/timer or else getting statistics?
The time between calls to ndo_get_stats64, since that's the routine
which queries the device.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] net: add missing descriptions in net_device_priv_flags
From: Luis de Bethencourt @ 2016-03-21 21:01 UTC (permalink / raw)
To: Benjamin Poirier; +Cc: linux-kernel, davem, netdev, Eric Dumazet
In-Reply-To: <20160321204158.GA6454@f1.synalogic.ca>
On 21/03/16 20:41, Benjamin Poirier wrote:
> On 2016/03/21 20:20, Luis de Bethencourt wrote:
>> The flags IFF_XMIT_DST_RELEASE_PERM, IFF_IPVLAN_MASTER and
>> IFF_IPVLAN_SLAVE are missing descriptions for the Documentation. Adding
>> them.
>>
>> Signed-off-by: Luis de Bethencourt <luisbg@osg.samsung.com>
>> ---
>> Hi,
>>
>> I also noticed this issue when running make htmldocs. Having better
>> documentation is great :)
>>
>> Thanks,
>> Luis
>>
>> include/linux/netdevice.h | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>> index be693b3..db9ffe4 100644
>> --- a/include/linux/netdevice.h
>> +++ b/include/linux/netdevice.h
>> @@ -1320,6 +1320,9 @@ struct net_device_ops {
>> * @IFF_LIVE_ADDR_CHANGE: device supports hardware address
>> * change when it's running
>> * @IFF_MACVLAN: Macvlan device
>> + * @IFF_XMIT_DST_RELEASE_PERM: permanent IFF_XMIT_DST_RELEASE
>
> I also noticed these kernel-doc warnings and had some patches queued for
> them. May I suggest the following in this case:
>
> * @IFF_XMIT_DST_RELEASE_PERM: IFF_XMIT_DST_RELEASE not taking into account
> * underlying stacked devices
>
> I presume that a patch for netdev ptype lists is forthcoming. Here's
> what I had:
>
> * @ptype_all: Device-specific packet handlers for all protocols
> * @ptype_specific: Device-specific, protocol-specific packet handlers
>
Hi Benjamin,
I was going to submit the fix for ptype_all and ptype_specific next, but your
descriptions are better. You should do it instead or if you want me to, I can
do it but amending you as the Author of the commit. Attribution where it is
deserved.
Thanks for looking into this,
Luis
>> + * @IFF_IPVLAN_MASTER: IPvlan master device
>> + * @IFF_IPVLAN_SLAVE: IPvlan slave device
>> * @IFF_L3MDEV_MASTER: device is an L3 master device
>> * @IFF_NO_QUEUE: device can run without qdisc attached
>> * @IFF_OPENVSWITCH: device is a Open vSwitch master
>> --
>> 2.5.1
>>
^ permalink raw reply
* [PATCH v2] net: add missing descriptions in net_device_priv_flags
From: Luis de Bethencourt @ 2016-03-21 20:58 UTC (permalink / raw)
To: linux-kernel; +Cc: davem, netdev, benjamin.poirier, Luis de Bethencourt
The flags IFF_XMIT_DST_RELEASE_PERM, IFF_IPVLAN_MASTER and
IFF_IPVLAN_SLAVE are missing descriptions for the Documentation. Adding
them.
Signed-off-by: Luis de Bethencourt <luisbg@osg.samsung.com>
Suggested-by: Benjamin Poirier <benjamin.poirier@gmail.com>
---
Hi,
Changing the description of IFF_XMIT_DST_RELEASE_PERM as suggested by Benjamin
and adding him to the Suggested-by for it.
https://lkml.org/lkml/2016/3/21/693
Thanks,
Luis
include/linux/netdevice.h | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index be693b3..bcca180 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1320,6 +1320,10 @@ struct net_device_ops {
* @IFF_LIVE_ADDR_CHANGE: device supports hardware address
* change when it's running
* @IFF_MACVLAN: Macvlan device
+ * @IFF_XMIT_DST_RELEASE_PERM: IFF_XMIT_DST_RELEASE not taking into account
+ * underlying stacked devices
+ * @IFF_IPVLAN_MASTER: IPvlan master device
+ * @IFF_IPVLAN_SLAVE: IPvlan slave device
* @IFF_L3MDEV_MASTER: device is an L3 master device
* @IFF_NO_QUEUE: device can run without qdisc attached
* @IFF_OPENVSWITCH: device is a Open vSwitch master
--
2.5.1
^ permalink raw reply related
* Re: [PATCH] net: add missing descriptions in net_device_priv_flags
From: Benjamin Poirier @ 2016-03-21 20:41 UTC (permalink / raw)
To: Luis de Bethencourt; +Cc: linux-kernel, davem, netdev, Eric Dumazet
In-Reply-To: <1458591608-1088-1-git-send-email-luisbg@osg.samsung.com>
On 2016/03/21 20:20, Luis de Bethencourt wrote:
> The flags IFF_XMIT_DST_RELEASE_PERM, IFF_IPVLAN_MASTER and
> IFF_IPVLAN_SLAVE are missing descriptions for the Documentation. Adding
> them.
>
> Signed-off-by: Luis de Bethencourt <luisbg@osg.samsung.com>
> ---
> Hi,
>
> I also noticed this issue when running make htmldocs. Having better
> documentation is great :)
>
> Thanks,
> Luis
>
> include/linux/netdevice.h | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index be693b3..db9ffe4 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -1320,6 +1320,9 @@ struct net_device_ops {
> * @IFF_LIVE_ADDR_CHANGE: device supports hardware address
> * change when it's running
> * @IFF_MACVLAN: Macvlan device
> + * @IFF_XMIT_DST_RELEASE_PERM: permanent IFF_XMIT_DST_RELEASE
I also noticed these kernel-doc warnings and had some patches queued for
them. May I suggest the following in this case:
* @IFF_XMIT_DST_RELEASE_PERM: IFF_XMIT_DST_RELEASE not taking into account
* underlying stacked devices
I presume that a patch for netdev ptype lists is forthcoming. Here's
what I had:
* @ptype_all: Device-specific packet handlers for all protocols
* @ptype_specific: Device-specific, protocol-specific packet handlers
> + * @IFF_IPVLAN_MASTER: IPvlan master device
> + * @IFF_IPVLAN_SLAVE: IPvlan slave device
> * @IFF_L3MDEV_MASTER: device is an L3 master device
> * @IFF_NO_QUEUE: device can run without qdisc attached
> * @IFF_OPENVSWITCH: device is a Open vSwitch master
> --
> 2.5.1
>
^ permalink raw reply
* Re: [PATCH] net: phy: at803x: don't depend on GPIOLIB
From: Uwe Kleine-König @ 2016-03-21 20:41 UTC (permalink / raw)
To: Sergei Shtylyov
Cc: Sebastian Frias, David S. Miller, netdev, lkml, mason,
Daniel Mack
In-Reply-To: <56F05651.3030706@cogentembedded.com>
Hello Sergei,
On Mon, Mar 21, 2016 at 11:15:13PM +0300, Sergei Shtylyov wrote:
> On 03/16/2016 08:25 PM, Sebastian Frias wrote:
>
> >Commit 687908c2b649 ("net: phy: at803x: simplify using
> >devm_gpiod_get_optional and its 4th argument") introduced a dependency
> >on GPIOLIB that was not there before.
> >
> >This commit removes such dependency by checking the return code and
> >comparing it against ENOSYS which is returned when GPIOLIB is not
> >selected.
> >
> >Fixes: 687908c2b649 ("net: phy: at803x: simplify using
> >devm_gpiod_get_optional and its 4th argument")
> >
> >Signed-off-by: Sebastian Frias <sf84@laposte.net>
>
> Do you have the PHY that requires the GPIO reset workaround?
> Askinjg because I have the patch adding the "reset-gpios" prop handling to
> phylib and your patch made me aware that I'll have to modify this driver in
> order to do that...
>
> >---
> > drivers/net/phy/at803x.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> >diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
> >index 2174ec9..88b7ff3 100644
> >--- a/drivers/net/phy/at803x.c
> >+++ b/drivers/net/phy/at803x.c
> >@@ -252,7 +252,9 @@ static int at803x_probe(struct phy_device *phydev)
> > return -ENOMEM;
> >
> > gpiod_reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> >- if (IS_ERR(gpiod_reset))
> >+ if (PTR_ERR(gpiod_reset) == -ENOSYS)
> >+ gpiod_reset = NULL;
> >+ else if (IS_ERR(gpiod_reset))
> return PTR_ERR(gpiod_reset);
>
> My patch basically gets rid of all this code. The thing that worries me
> is that the driver assumes that the reset singal is active low, despite what
> the GPIO specifier in the device tree has for the GPIO polarity. In fact, it
> will only work correctly if the specified has GPIO_ACTIVE_HIGH -- which is
> wrong because the reset signal is active low!
Note that gpio descriptors handle the polarity just fine (i.e. the pin
is set to 0 after doing gpiod_set_value(1) if the gpio is active low).
But having said that, the driver gets it wrong.
The right sequence to reset a device using a gpio is:
gpiod_set_value(priv->gpiod_reset, 1);
msleep(some_time);
gpiod_set_value(priv->gpiod_reset, 0);
and if the gpio is active low, this should be specified in the device
tree. This was done wrong in 13a56b449325 (net: phy: at803x: Add support
for hardware reset).
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply
* [PATCH] ip.7: Fix incorrect sockopt name
From: Benjamin Poirier @ 2016-03-21 20:28 UTC (permalink / raw)
To: mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w
Cc: linux-man-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA,
Radek Pazdera
"IP_LEAVE_GROUP" does not exist. It was perhaps a confusion with
MCAST_LEAVE_GROUP. Change the text to IP_DROP_MEMBERSHIP which has the same
function as MCAST_LEAVE_GROUP and is documented in the ip.7 man page.
Reference:
Linux kernel net/ipv4/ip_sockglue.c do_ip_setsockopt()
Cc: Radek Pazdera <rpazdera-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Benjamin Poirier <bpoirier-IBi9RG/b67k@public.gmane.org>
---
man7/ip.7 | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/man7/ip.7 b/man7/ip.7
index 3905573..37e2c86 100644
--- a/man7/ip.7
+++ b/man7/ip.7
@@ -376,7 +376,7 @@ a given multicast group that come from a given source.
If the application has subscribed to multiple sources within
the same group, data from the remaining sources will still be delivered.
To stop receiving data from all sources at once, use
-.BR IP_LEAVE_GROUP .
+.BR IP_DROP_MEMBERSHIP .
.IP
Argument is an
.I ip_mreq_source
--
2.7.2
--
To unsubscribe from this list: send the line "unsubscribe linux-man" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* [PATCH 2/2] igmp: Document sysctl_igmp_max_msf
From: Benjamin Poirier @ 2016-03-21 20:21 UTC (permalink / raw)
To: netdev; +Cc: Jonathan Corbet
In-Reply-To: <1458591700-5547-1-git-send-email-bpoirier@suse.com>
Signed-off-by: Benjamin Poirier <bpoirier@suse.com>
---
Documentation/networking/ip-sysctl.txt | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/Documentation/networking/ip-sysctl.txt b/Documentation/networking/ip-sysctl.txt
index d3768e8..b183e2b 100644
--- a/Documentation/networking/ip-sysctl.txt
+++ b/Documentation/networking/ip-sysctl.txt
@@ -946,10 +946,15 @@ igmp_max_memberships - INTEGER
The value 5459 assumes no IP header options, so in practice
this number may be lower.
+igmp_max_msf - INTEGER
+ Maximum number of addresses allowed in the source filter list for a
+ multicast group.
+ Default: 10
+
igmp_qrv - INTEGER
- Controls the IGMP query robustness variable (see RFC2236 8.1).
- Default: 2 (as specified by RFC2236 8.1)
- Minimum: 1 (as specified by RFC6636 4.5)
+ Controls the IGMP query robustness variable (see RFC2236 8.1).
+ Default: 2 (as specified by RFC2236 8.1)
+ Minimum: 1 (as specified by RFC6636 4.5)
conf/interface/* changes special settings per interface (where
"interface" is the name of your network interface)
--
2.7.2
^ permalink raw reply related
* [PATCH 1/2] net: Fix indentation of the conf/ documentation block
From: Benjamin Poirier @ 2016-03-21 20:21 UTC (permalink / raw)
To: netdev; +Cc: Jonathan Corbet, Jeremy Eder
Commit d67ef35fff67 ("clarify documentation for
net.ipv4.igmp_max_memberships") mistakenly indented a block of
documentation such that it now looks like it belongs to a specific sysctl.
Restore that block's original position.
Cc: Jeremy Eder <jeder@redhat.com>
Signed-off-by: Benjamin Poirier <bpoirier@suse.com>
---
Documentation/networking/ip-sysctl.txt | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/Documentation/networking/ip-sysctl.txt b/Documentation/networking/ip-sysctl.txt
index d5df40c..d3768e8 100644
--- a/Documentation/networking/ip-sysctl.txt
+++ b/Documentation/networking/ip-sysctl.txt
@@ -946,16 +946,16 @@ igmp_max_memberships - INTEGER
The value 5459 assumes no IP header options, so in practice
this number may be lower.
- conf/interface/* changes special settings per interface (where
- "interface" is the name of your network interface)
-
- conf/all/* is special, changes the settings for all interfaces
-
igmp_qrv - INTEGER
Controls the IGMP query robustness variable (see RFC2236 8.1).
Default: 2 (as specified by RFC2236 8.1)
Minimum: 1 (as specified by RFC6636 4.5)
+conf/interface/* changes special settings per interface (where
+"interface" is the name of your network interface)
+
+conf/all/* is special, changes the settings for all interfaces
+
log_martians - BOOLEAN
Log packets with impossible addresses to kernel log.
log_martians for the interface will be enabled if at least one of
--
2.7.2
^ permalink raw reply related
* [PATCH] net: add missing descriptions in net_device_priv_flags
From: Luis de Bethencourt @ 2016-03-21 20:20 UTC (permalink / raw)
To: linux-kernel; +Cc: davem, netdev, Luis de Bethencourt
The flags IFF_XMIT_DST_RELEASE_PERM, IFF_IPVLAN_MASTER and
IFF_IPVLAN_SLAVE are missing descriptions for the Documentation. Adding
them.
Signed-off-by: Luis de Bethencourt <luisbg@osg.samsung.com>
---
Hi,
I also noticed this issue when running make htmldocs. Having better
documentation is great :)
Thanks,
Luis
include/linux/netdevice.h | 3 +++
1 file changed, 3 insertions(+)
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index be693b3..db9ffe4 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1320,6 +1320,9 @@ struct net_device_ops {
* @IFF_LIVE_ADDR_CHANGE: device supports hardware address
* change when it's running
* @IFF_MACVLAN: Macvlan device
+ * @IFF_XMIT_DST_RELEASE_PERM: permanent IFF_XMIT_DST_RELEASE
+ * @IFF_IPVLAN_MASTER: IPvlan master device
+ * @IFF_IPVLAN_SLAVE: IPvlan slave device
* @IFF_L3MDEV_MASTER: device is an L3 master device
* @IFF_NO_QUEUE: device can run without qdisc attached
* @IFF_OPENVSWITCH: device is a Open vSwitch master
--
2.5.1
^ permalink raw reply related
* Re: [PATCH] net: phy: at803x: don't depend on GPIOLIB
From: Sergei Shtylyov @ 2016-03-21 20:15 UTC (permalink / raw)
To: Sebastian Frias, Uwe Kleine-König, David S. Miller, netdev
Cc: lkml, mason
In-Reply-To: <56E99727.9040702@laposte.net>
Hello.
On 03/16/2016 08:25 PM, Sebastian Frias wrote:
> Commit 687908c2b649 ("net: phy: at803x: simplify using
> devm_gpiod_get_optional and its 4th argument") introduced a dependency
> on GPIOLIB that was not there before.
>
> This commit removes such dependency by checking the return code and
> comparing it against ENOSYS which is returned when GPIOLIB is not
> selected.
>
> Fixes: 687908c2b649 ("net: phy: at803x: simplify using
> devm_gpiod_get_optional and its 4th argument")
>
> Signed-off-by: Sebastian Frias <sf84@laposte.net>
Do you have the PHY that requires the GPIO reset workaround?
Askinjg because I have the patch adding the "reset-gpios" prop handling to
phylib and your patch made me aware that I'll have to modify this driver in
order to do that...
> ---
> drivers/net/phy/at803x.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
> index 2174ec9..88b7ff3 100644
> --- a/drivers/net/phy/at803x.c
> +++ b/drivers/net/phy/at803x.c
> @@ -252,7 +252,9 @@ static int at803x_probe(struct phy_device *phydev)
> return -ENOMEM;
>
> gpiod_reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> - if (IS_ERR(gpiod_reset))
> + if (PTR_ERR(gpiod_reset) == -ENOSYS)
> + gpiod_reset = NULL;
> + else if (IS_ERR(gpiod_reset))
return PTR_ERR(gpiod_reset);
My patch basically gets rid of all this code. The thing that worries me is
that the driver assumes that the reset singal is active low, despite what the
GPIO specifier in the device tree has for the GPIO polarity. In fact, it will
only work correctly if the specified has GPIO_ACTIVE_HIGH -- which is wrong
because the reset signal is active low!
Can you point me to an actual device tree using this erratic PHY?
WBR, Sergei
^ permalink raw reply
* Re: [PATCH] net: phy: at803x: don't depend on GPIOLIB
From: Uwe Kleine-König @ 2016-03-21 20:12 UTC (permalink / raw)
To: Sebastian Frias
Cc: Daniel Mack, David S. Miller, netdev, lkml, mason,
Florian Fainelli, Mans Rullgard, Fabio Estevam,
Martin Blumenstingl, Linus Walleij
In-Reply-To: <56F0150F.8080804@laposte.net>
Hello Sebastian,
On Mon, Mar 21, 2016 at 04:36:47PM +0100, Sebastian Frias wrote:
> On 03/21/2016 02:54 PM, Uwe Kleine-König wrote:
> >> - We'd also need to check that 'gpiod' is not NULL
> >
> > That is the optional part. If gpiod is NULL, you have one of the devices
> > that don't need to handle the enable line.
>
> gpiod=NULL (because the key is not there) or gpiod=ERR (because
> GPIOLIB=n + my patch) will result in the same behaviour, ie: driver
> binds, but fails to reset.
Assuming the source for the hardware description is dt (the same
argument applies if it's ACPI or something else).
The driver uses devm_gpiod_get_optional(..."reset"...). That means some
hardware has a reset line, some don't. If a reset gpio specification is
there, that means the hardware has such a signal and it seems that means
it must not be ignored. If there is no reset gpio specification that
means that driver has to assume there is no reset line. (In the real
world there might be other reasons the reset line isn't in the device
tree, but it's not in the scope of the driver to guess why it's missing.
If it's not there the only sensible thing to assume for the driver is:
There is no reset line.)
So the conclusions are: If there is a reset line in dt, it must be used.
If you don't know if there is a reset line (because GPIOLIB=n) the
driver should not bind. Everything else yields to more problems than
good.
> >> In the scenario at hand only some devices require a hack and
> >> gpiod_reset=NULL is valid (ie: device will not fail probing)
> >
> > With your patch and GPIOLIB=n you bind the driver even for the devices
> > that need the hack. This is broken!
>
> It is a degraded mode I agree and had proposed to print a warning.
> However, I fail to see how is GPIOLIB=n different from just not having
> "reset" present.
GPIOLIB=n means "the driver doesn't know if a reset line is
present/necessary", not having reset means "there is no reset line".
And don't do error handling by printk assuming anyone will read it. That
doesn't work.
> The fact that GPIOLIB=y does not means that the "reset" key will be there.
Right, but with GPIOLIB=y you know if it's there, and if yes, you can
control the line.
> I mean, you assume that "reset" will be present for devices that need
> the hack, yet there is no such guarantee and the code clearly assumes
> that it can be missing.
The driver must assume that the device tree is right. If it's not fix
the device tree, don't implement clumsy work arounds in the driver.
> For all we know, the key is actually missing on systems that do use the
> faulty PHY (all systems designed prior to the hack being implemented)
> And that, regardless of GPIOLIB status.
Then fix the device tree.
> Since the reset line can be missing, it can be missing for whatever reason.
> Whether it is because the "reset" key is not present or because
> GPIOLIB=n, it does not matter, it will result on the same outcome.
>
> Furthermore, if "reset" is really required for certain devices that need
> the hack, then both cases:
> - GPIOLIB=n
> - "reset" not present
> should be handled the same way (ie: not bind the driver) for such devices.
If you can detect by other means that "reset is necessary", then yes,
the driver should ideally also fail in this case with no reset specified
in dt.
> By the way, I think not binding the driver is too strong too.
> Having a GPIO free and being able to route it to the PHY for reset may
> not even be possible on some systems.
> Are they supposed to stop working?
Sometimes no driver is better than an unreliable one.
> >>> For consistency I'd recommend to do the
> >>> same for reset even though there is a chance to get a working device.
> >>
> >> I'm not so sure, because then information would be lost, handling both
> >> ("enable" and "reset") the same way aliases different intents and
> >> requirements.
> >> Indeed, only "enable" would be really mandatory, "reset" is essentially
> >> optional (only 1 of 3 devices of the family require the hack).
> >>
> >> Besides, if "reset" was really mandatory, we would also fail the probe
> >> if the pointer for the reset GPIO is NULL.
> >
> > No, if reset was mandatory you'd use gpiod_get instead of
> > gpiod_get_optional and fail iff that call fails, too.
>
> Ok, but the current code for "reset" is using devm_gpiod_get_optional()
> so "reset" is clearly optional.
> And that call will return NULL if "reset" is not present, even with
> GPIOLIB=y
s/even//. And returning NULL is not an error. It means: There is no
reset line specified in dt.
> >> Indeed, even with GPIOLIB=y the pointer can be NULL if the "reset" (or
> >> "enable" in your scenario) is not present.
> >> From a functionality perspective, a NULL pointer for "reset" will result
> >> in the same behaviour as GPIOLIB=n, ie: not being able to reset the PHY.
> >
> > Right, but you only get reset=NULL iff the device tree (or whatever is
> > the source of information for gpiod_get) doesn't specify a reset gpio
> > which then means "This device doesn't need a reset gpio.". This is
> > different from the GPIOLIB=n case, where the return code signals "It's
> > unknown if the device needs a reset gpio.".
>
> I think somehow you try to make a difference on the reason why the
> reset=INVALID (NULL or ERR), but IMHO there's none.
NULL is not invalid. With devm_gpiod_get_optional the driver asks: "Is
there a reset gpio, and if yes, which one?". The reset line is optional,
that means "some devices have one, some others don't.". It does NOT mean
"You can ignore if there is a reset line or not."!
> If somebody using AT8030 PHY (the one that requires the hack) either
> does not configure a "reset" key on the DT, either does not enable
> GPIOLIB, the result will be the same.
> There is no warning in either case and it will run on a degraded mode.
My expectation is that if the dt includes a reset property, it should be
used. So, there is a fine option for each situation:
- device that doesn't need the hack => everything is fine;
- AT8030 with reset to a gpio => specify the reset in dt;
- AT8030 without a controllable reset line => don't specify a reset
property;
if you have a broken device with a reset line you can even drop the
reset property from dt and not make use of the reset gpio if you think
it's a good idea.
> >>>> What would you think of making at803x_link_change_notify() print a
> >>>> message every time it should do a reset but does not has a way to do it?
> >>>
> >>> Then this question is obsolete because the device doesn't probe.
> >>
> >> I think you assume that "reset" is mandatory for all AT803x devices, but
> >> that's not what the code says.
> >
> > No, you're wrong here. I'm aware that the code supports devices without
> > reset.
>
> Ok, then I did not understand what you meant with "the question is
> obsolete because the device doesn't probe".
if (GPIOLIB=y):
if (device has a reset property):
driver runs fine with being able to reset.
else:
if (device needs reset to function properly):
ideally fail to probe
else:
driver runs fine without reset and without the need
to bother the user about the absense of the gpio
else:
driver fails to bind
In no branch of these cases there is a situation where you must issue a
reset but cannot. So there is no need to ask if you should issue a
message each time this happens. That's what I meant with "the question
is obsolete".
> Unless I'm missing something, the only way the driver won't bind
> correctly with current code is if GPIOLIB=n
Right, because with GPIOLIB=n you cannot control a gpio line, but you
don't know if you must.
Consider you are an electrician (driver) and you should repair a power
socket (control a device) but forgot your phase tester (GPIOLIB=n).
So there is no way you can determine if it's save to modify the socket.
You can choose the optimistic way and say: "If there is no voltage on the
socket, I can repair it successfully" and just try it. Or you take the
safe approach and say: "I don't know if it's safe to do, so I better
don't.".
The same applies to drivers: If it might be necessary to handle a gpio,
but you cannot know if that's the case or not: Don't try it.
> Systems using the faulty PHY and having GPIOLIB=y but with an outdated
> DT that does not has a "reset" key would have the PHY driver bind yet
> have it fail due to missing "reset".
dt compatibility is fine. So if you want to handle the situation
properly, change the compatible and there make sure that the presence or
absence of the reset property matches reality with the new binding. And
continue to handle the old compatible the way you did before. But don't
ignore errors returned by gpiod_get et al.
> >> - make another patch to print a warning when gpiod_reset is NULL (which
> >> can happen, even without my patch)
> >
> > This only happens if no reset gpio is needed and in this case the driver
> > does the right thing. So if you ask me, there is no need to modify the
> > driver. Better add a dependency to GPIOLIB. This is necessary even for
> > devices which don't need a reset gpio to answer the question "does this
> > driver need a reset gpio?" correctly.
>
> I don't see how the dependency on GPIOLIB=y improves the situation in
> any useful way.
>
> To put an example: in our case we don't use the faulty PHY.
> So, the DT does not has the "reset" key.
> Why should then the PHY driver be dependent on GPIOLIB?
You want GPIOLIB to know that you don't need to handle the reset gpio.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply
* Re: [RFC PATCH 0/9] RFC6864 compliant GRO and GSO partial offload
From: Jesse Gross @ 2016-03-21 20:10 UTC (permalink / raw)
To: Alexander Duyck
Cc: David Miller, Alex Duyck, Edward Cree, Netdev, Tom Herbert
In-Reply-To: <CAKgT0UdZU9u1i0Er2RhmQ1kOQgNRkQ3_DQUbGybpWcA3MzN-kw@mail.gmail.com>
On Mon, Mar 21, 2016 at 12:46 PM, Alexander Duyck
<alexander.duyck@gmail.com> wrote:
> On Mon, Mar 21, 2016 at 11:50 AM, David Miller <davem@davemloft.net> wrote:
>> From: Alexander Duyck <aduyck@mirantis.com>
>> Date: Fri, 18 Mar 2016 16:24:38 -0700
>>
>>> This patch series addresses two things.
>>>
>>> First it enables what I am calling RFC6864 compliant GRO. Basically what
>>> I am doing is allowing one of two patterns for incoming frames. Either the
>>> IP ID will increment, or if the DF bit is set it can either increment or
>>> stay the same value. This allows us to perform GRO if the IP ID is forced
>>> to stay at a static value as may occur if we are replicating an IP header
>>> instead of actually having it offloaded.
>>>
>>> The last 3 patches introduce what I am calling GSO partial. The best way
>>> to describe it is that it is a GSO offload in which the portion pointed to
>>> by csum_start must be handled by the hardware, and the region before that
>>> can be optionally handled. So for example with i40e the only pieces it was
>>> missing from the full offload was the checksum so this is computed in
>>> software and the hardware will update inner and outer IP headers. In the
>>> example for ixgbe the hardware will only update the outer IP header. The
>>> outer UDP or GRE header and inner IP header are unmodified.
>>
>> Conceptually I am completely fine with these changes.
>>
>>> The one concern here is that if the outer IP header does not have
>>> the DF bit set and does not update the IP ID field we run the risk
>>> of causing all sorts of problems if the packet is fragmented in
>>> flight.
>>
>> I think we absolutely cannot let such a packet be output from our
>> stack.
>
> My concern is that there may be devices doing just that. That is one
> of the motivations behind updating the GRO code so that if DF is not
> set we refuse to aggregate frames that do not increment the IP ID. If
> nothing else we just have to wait and see who comes screaming about
> performance regressions once the patch is applied. That will tell us
> who is cheating and segmenting frames with the IP ID not being updated
> and DF bit not being set. There was already the comment in
> inet_gro_receive about how some devices weren't updating this
> correctly so we probably need to figure out if those devices are at
> least setting the DF bit.
It's clearly wrong to send packets without the DF bit and the same IP
ID (but I agree that there may be some devices that are doing this by
accident). However, I don't think that we want the device to be
changing the value of the DF bit to somehow compensate for that.
I was surprised to see that the DF bit is not on for outer IP headers
- it seems like that should at least be the default. If it is turned
off and the device isn't capable of updating the outer IP ID, well,
then it shouldn't be doing TSO.
^ permalink raw reply
* RE: [PATCH] lan78xx: Protect runtime_auto check by #ifdef CONFIG_PM
From: Woojung.Huh @ 2016-03-21 20:09 UTC (permalink / raw)
To: stern
Cc: oneukum, geert, UNGLinuxDriver, davem, linux, rjw, netdev,
linux-usb, linux-pm, linux-kernel
In-Reply-To: <Pine.LNX.4.44L0.1603211523370.1708-100000@iolanthe.rowland.org>
> > > But this leaves open the issue that querying the device too often will
> > > prevent it from going into autosuspend. It seems to me that the best
> > > way to deal with this is to make sure that the autosuspend timeout is
> > > shorter than the interal between queries, not to make the querying
> > > conditional on !runtime_auto.
> >
> > Short autosuspend timeout can affect performance. For instance our
> experiments showed that
> > shorter than 10sec timeout made Ethernet performance degrade because
> of wakeup delays.
> > So, just putting shorter timeout may have some side effects.
>
> Sure. This just means that you need a long statistics interval --
> longer than the autosuspend timeout. That's why I suggested making the
> interval adjustable.
What do you mean statistics interval?
Interval calling ndo_get_stats64 or another thread/timer or else getting statistics?
Thanks.
Woojung
^ permalink raw reply
* Re: [PATCH 1/3] tc: f_u32: add missing spaces around operators
From: Stephen Hemminger @ 2016-03-21 18:53 UTC (permalink / raw)
To: Luca Lemmo; +Cc: netdev
In-Reply-To: <1458147374-8899-1-git-send-email-luca@linux.com>
On Wed, 16 Mar 2016 17:56:12 +0100
Luca Lemmo <luca@linux.com> wrote:
> Signed-off-by: Luca Lemmo <luca@linux.com>
All applied,
^ permalink raw reply
* Re: [iproute PATCH] Use ARRAY_SIZE macro everywhere
From: Stephen Hemminger @ 2016-03-21 18:59 UTC (permalink / raw)
To: Phil Sutter; +Cc: netdev
In-Reply-To: <1458242888-28978-1-git-send-email-phil@nwl.cc>
On Thu, 17 Mar 2016 20:28:08 +0100
Phil Sutter <phil@nwl.cc> wrote:
> This patch was generated by the following semantic patch (a trimmed down
> version of what is shipped with Linux sources):
>
> @@
> type T;
> T[] E;
> @@
> (
> - (sizeof(E)/sizeof(*E))
> + ARRAY_SIZE(E)
> |
> - (sizeof(E)/sizeof(E[...]))
> + ARRAY_SIZE(E)
> |
> - (sizeof(E)/sizeof(T))
> + ARRAY_SIZE(E)
> )
>
> The only manual adjustment was to include utils.h in misc/nstat.c to make
> the macro known there.
>
> Signed-off-by: Phil Sutter <phil@nwl.cc>
I went ahead and did some checkpatch spring cleaning to code.
This is no longer needed.
^ 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