* Re: [PATCH net-next 4/5] ipv6: Compute multipath hash for sent ICMP errors from offending packet
From: Jakub Sitnicki @ 2016-10-27 22:00 UTC (permalink / raw)
To: David Miller
Cc: netdev, linux-kernel, kuznet, jmorris, yoshfuji, kaber, edumazet
In-Reply-To: <20161027.112413.1616222035100737672.davem@davemloft.net>
On Thu, Oct 27, 2016 at 03:24 PM GMT, David Miller wrote:
> From: Jakub Sitnicki <jkbs@redhat.com>
> Date: Mon, 24 Oct 2016 11:28:51 +0200
>
>> diff --git a/include/linux/icmpv6.h b/include/linux/icmpv6.h
>> index 57086e9..6282e03 100644
>> --- a/include/linux/icmpv6.h
>> +++ b/include/linux/icmpv6.h
>> @@ -45,4 +45,6 @@ extern void icmpv6_flow_init(struct sock *sk,
>> const struct in6_addr *saddr,
>> const struct in6_addr *daddr,
>> int oif);
>> +struct ipv6hdr;
>> +extern u32 icmpv6_multipath_hash(const struct ipv6hdr *iph);
>> #endif
>
> We do not use "extern" in external function declarations in header file any more.
My mistake, will remote it.
^ permalink raw reply
* [PATCH] net: phy: at803x: the Atheros 8031 supports pause frames
From: Timur Tabi @ 2016-10-27 22:05 UTC (permalink / raw)
To: netdev, zefir.kurtisi, scampbel, alokc, shankerd, andrew,
f.fainelli
The Atheros 8031 PHY supports the 802.3 extension for symmetric and
asymmetric pause frames, so set that to the list of features supported
by the phy.
Signed-off-by: Timur Tabi <timur@codeaurora.org>
---
Without this patch, my NIC (the Qualcomm EMAC) receives a lot of frame
check sequence (aka CRC) errors, resulting in about 10% packet loss.
Can someone help me understand why? Because of this patch, I can't use
the generic phy driver in phylib. Why would a MAC controller require
its PHY to support pause frames?
drivers/net/phy/at803x.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index a52b560..fb80413 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -440,7 +440,8 @@ static struct phy_driver at803x_driver[] = {
.get_wol = at803x_get_wol,
.suspend = at803x_suspend,
.resume = at803x_resume,
- .features = PHY_GBIT_FEATURES,
+ .features = PHY_GBIT_FEATURES |
+ SUPPORTED_Pause | SUPPORTED_Asym_Pause,
.flags = PHY_HAS_INTERRUPT,
.config_aneg = genphy_config_aneg,
.read_status = genphy_read_status,
--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc. Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
^ permalink raw reply related
* Re: [PATCH net-next 5/5] ipv6: Compute multipath hash for forwarded ICMP errors from offending packet
From: Jakub Sitnicki @ 2016-10-27 22:10 UTC (permalink / raw)
To: David Miller; +Cc: netdev, linux-kernel, kuznet, jmorris, yoshfuji, kaber
In-Reply-To: <20161027.112525.2290598329610980726.davem@davemloft.net>
On Thu, Oct 27, 2016 at 03:25 PM GMT, David Miller wrote:
> From: Jakub Sitnicki <jkbs@redhat.com>
> Date: Mon, 24 Oct 2016 11:28:52 +0200
>
>> + inner_iph = skb_header_pointer(
>> + skb, skb_transport_offset(skb) + sizeof(*icmph),
>> + sizeof(_inner_iph), &_inner_iph);
>
> Please do not style this call like this, put as many arguments as
> you can on the first line.
>
> inner_iph = skb_header_pointer(skb,
> skb_transport_offset(skb) + sizeof(*icmph),
> sizeof(_inner_iph), &_inner_iph);
>
> And on the second and subsequent lines, indent to the first column after
> the openning parenthesis of the first line.
FWIW, I had it styled like that and then changed it. Will change back.
In my defense - checkpatch.pl made me do it, Your Honor! (line too long)
^ permalink raw reply
* Re: [PATCH] net: phy: at803x: the Atheros 8031 supports pause frames
From: Florian Fainelli @ 2016-10-27 22:12 UTC (permalink / raw)
To: Timur Tabi, netdev, zefir.kurtisi, scampbel, alokc, shankerd,
andrew
In-Reply-To: <1477605901-30906-1-git-send-email-timur@codeaurora.org>
On 10/27/2016 03:05 PM, Timur Tabi wrote:
> The Atheros 8031 PHY supports the 802.3 extension for symmetric and
> asymmetric pause frames, so set that to the list of features supported
> by the phy.
>
> Signed-off-by: Timur Tabi <timur@codeaurora.org>
> ---
>
> Without this patch, my NIC (the Qualcomm EMAC) receives a lot of frame
> check sequence (aka CRC) errors, resulting in about 10% packet loss.
Hu? In my experience that should not come from supporting Pause frames
or not, but rather properly configuring a (RG)MII delay, but your
mileage may vary.
> Can someone help me understand why? Because of this patch, I can't use
> the generic phy driver in phylib. Why would a MAC controller require
> its PHY to support pause frames?
It does not, support for Pause frames is a MAC-level feature, yet,
PHYLIB (and that's been on my todo for a while now) insists on reporting
the confusing phydev->pause and phydev->asym_pause, which really is what
has been determined from auto-negotiating with your partner, as opposed
to being a supported thing or not. The PHY has absolutely not business
in that.
Your change should probably be in the Ethernet MAC driver, when you have
successfully connected to the PHY, update phydev->supported to include
SUPPORTED_Pause | SUPPORTED_Asym_pause.
>
> drivers/net/phy/at803x.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
> index a52b560..fb80413 100644
> --- a/drivers/net/phy/at803x.c
> +++ b/drivers/net/phy/at803x.c
> @@ -440,7 +440,8 @@ static struct phy_driver at803x_driver[] = {
> .get_wol = at803x_get_wol,
> .suspend = at803x_suspend,
> .resume = at803x_resume,
> - .features = PHY_GBIT_FEATURES,
> + .features = PHY_GBIT_FEATURES |
> + SUPPORTED_Pause | SUPPORTED_Asym_Pause,
> .flags = PHY_HAS_INTERRUPT,
> .config_aneg = genphy_config_aneg,
> .read_status = genphy_read_status,
>
--
Florian
^ permalink raw reply
* Re: [net 4/4] i40e: fix call of ndo_dflt_bridge_getlink()
From: Greg @ 2016-10-27 22:13 UTC (permalink / raw)
To: Jeff Kirsher
Cc: davem, Huaibin Wang, netdev, nhorman, sassmann, jogreene,
guru.anbalagane, Carolyn Wyborny, Nicolas Dichtel
In-Reply-To: <1477603677-39957-5-git-send-email-jeffrey.t.kirsher@intel.com>
On Thu, 2016-10-27 at 14:27 -0700, Jeff Kirsher wrote:
> From: Huaibin Wang <huaibin.wang@6wind.com>
>
> Order of arguments is wrong.
> The wrong code has been introduced by commit 7d4f8d871ab1, but is compiled
> only since commit 9df70b66418e.
>
> Note that this may break netlink dumps.
>
> Fixes: 9df70b66418e ("i40e: Remove incorrect #ifdef's")
> Fixes: 7d4f8d871ab1 ("switchdev; add VLAN support for port's bridge_getlink")
> CC: Carolyn Wyborny <carolyn.wyborny@intel.com>
> Signed-off-by: Huaibin Wang <huaibin.wang@6wind.com>
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> Tested-by: Andrew Bowers <andrewx.bowers@intel.com>
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> ---
> drivers/net/ethernet/intel/i40e/i40e_main.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
> index 6abc130..31c97e3 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
> @@ -9034,7 +9034,7 @@ static int i40e_ndo_bridge_getlink(struct sk_buff *skb, u32 pid, u32 seq,
> return 0;
>
> return ndo_dflt_bridge_getlink(skb, pid, seq, dev, veb->bridge_mode,
> - nlflags, 0, 0, filter_mask, NULL);
> + 0, 0, nlflags, filter_mask, NULL);
Yikes! And NDO function with 10 parameters? No wonder they get mixed
up.
- Greg
> }
>
> /* Hardware supports L4 tunnel length of 128B (=2^7) which includes
^ permalink raw reply
* Re: [PATCH net-next v3 3/9] ipv6: sr: add support for SRH encapsulation and injection with lwtunnels
From: kbuild test robot @ 2016-10-27 22:21 UTC (permalink / raw)
To: David Lebrun; +Cc: kbuild-all, netdev, David Lebrun
In-Reply-To: <1477575239-11228-4-git-send-email-david.lebrun@uclouvain.be>
[-- Attachment #1: Type: text/plain, Size: 965 bytes --]
Hi David,
[auto build test ERROR on net-next/master]
url: https://github.com/0day-ci/linux/commits/David-Lebrun/ipv6-implement-dataplane-support-for-rthdr-type-4-Segment-Routing-Header/20161027-233201
config: arm-iop13xx_defconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=arm
All errors (new ones prefixed by >>):
net/built-in.o: In function `seg6_output':
>> sysctl_net.c:(.text+0xdfc8c): undefined reference to `dst_cache_get'
>> sysctl_net.c:(.text+0xdfd78): undefined reference to `dst_cache_set_ip6'
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 14388 bytes --]
^ permalink raw reply
* Re: [PATCH 4/5] net: ethernet: bgmac: add NS2 support
From: Ray Jui @ 2016-10-27 22:21 UTC (permalink / raw)
To: Jon Mason, Florian Fainelli
Cc: David Miller, Rob Herring, Mark Rutland,
rafal-g1n6cQUeyibVItvQsEIGlw,
bcm-kernel-feedback-list-dY08KVG/lbpWk0Htik3J/w,
netdev-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20161027205117.GA24397-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
On 10/27/2016 1:51 PM, Jon Mason wrote:
> On Wed, Oct 26, 2016 at 02:50:39PM -0700, Florian Fainelli wrote:
>> On 10/26/2016 12:36 PM, Jon Mason wrote:
>>> Add support for the variant of amac hardware present in the Broadcom
>>> Northstar2 based SoCs. Northstar2 requires an additional register to be
>>> configured with the port speed/duplexity (NICPM). This can be added to
>>> the link callback to hide it from the instances that do not use this.
>>> Also, the bgmac_chip_reset() was intentionally removed to prevent the
>>> resetting of the chip to the default values on open. Finally, clearing
>>> of the pending interrupts on init is required due to observed issues on
>>> some platforms.
>>>
>>> Signed-off-by: Jon Mason <jon.mason-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
>>> ---
>>
>>> +static void bgmac_nicpm_speed_set(struct net_device *net_dev)
>>> +{
>>> + struct bgmac *bgmac = netdev_priv(net_dev);
>>> + u32 val;
>>> +
>>> + if (!bgmac->plat.nicpm_base)
>>> + return;
>>> +
>>> + val = NICPM_IOMUX_CTRL_INIT_VAL;
>>> + switch (bgmac->net_dev->phydev->speed) {
>>> + default:
>>> + pr_err("Unsupported speed. Defaulting to 1000Mb\n");
>>> + case SPEED_1000:
>>> + val |= NICPM_IOMUX_CTRL_SPD_1000M << NICPM_IOMUX_CTRL_SPD_SHIFT;
>>> + break;
>>> + case SPEED_100:
>>> + val |= NICPM_IOMUX_CTRL_SPD_100M << NICPM_IOMUX_CTRL_SPD_SHIFT;
>>> + break;
>>> + case SPEED_10:
>>> + val |= NICPM_IOMUX_CTRL_SPD_10M << NICPM_IOMUX_CTRL_SPD_SHIFT;
>>> + break;
>>> + }
>>> +
>>> + writel(val, bgmac->plat.nicpm_base + NICPM_IOMUX_CTRL);
>>> +
>>> + usleep_range(10, 100);
>>
>> Does not seem like a good idea, do you need that sleep?
>
> Oops, that's not supposed to be there. Removed.
>
>
>>> +
>>> + bgmac_adjust_link(bgmac->net_dev);
>>> +}
>>> +
>>> static int platform_phy_connect(struct bgmac *bgmac)
>>> {
>>> struct phy_device *phy_dev;
>>>
>>> - phy_dev = of_phy_get_and_connect(bgmac->net_dev, bgmac->dev->of_node,
>>> - bgmac_adjust_link);
>>> + if (bgmac->plat.nicpm_base)
>>> + phy_dev = of_phy_get_and_connect(bgmac->net_dev,
>>> + bgmac->dev->of_node,
>>> + bgmac_nicpm_speed_set);
>>> + else
>>> + phy_dev = of_phy_get_and_connect(bgmac->net_dev,
>>> + bgmac->dev->of_node,
>>> + bgmac_adjust_link);
>>> if (!phy_dev) {
>>> dev_err(bgmac->dev, "Phy connect failed\n");
>>> return -ENODEV;
>>> }
>>>
>>> + if (bgmac->feature_flags & BGMAC_FEAT_LANE_SWAP)
>>> + phy_dev->dev_flags |= PHY_BRCM_EXP_LANE_SWAP;
>>> +
>>> return 0;
>>> }
>>>
>>> @@ -140,6 +188,9 @@ static int bgmac_probe(struct platform_device *pdev)
>>>
>>> platform_set_drvdata(pdev, bgmac);
>>>
>>> + if (of_property_read_bool(np, "brcm,enet-phy-lane-swap"))
>>> + bgmac->feature_flags |= BGMAC_FEAT_LANE_SWAP;
>>> +
>>> /* Set the features of the 4707 family */
>>> bgmac->feature_flags |= BGMAC_FEAT_CLKCTLST;
>>> bgmac->feature_flags |= BGMAC_FEAT_NO_RESET;
>>> @@ -182,6 +233,14 @@ static int bgmac_probe(struct platform_device *pdev)
>>> if (IS_ERR(bgmac->plat.idm_base))
>>> return PTR_ERR(bgmac->plat.idm_base);
>>>
>>> + regs = platform_get_resource_byname(pdev, IORESOURCE_MEM, "nicpm_base");
>>> + if (regs) {
>>> + bgmac->plat.nicpm_base = devm_ioremap_resource(&pdev->dev,
>>> + regs);
>>> + if (IS_ERR(bgmac->plat.nicpm_base))
>>> + return PTR_ERR(bgmac->plat.nicpm_base);
>>> + }
>>> +
>>> bgmac->read = platform_bgmac_read;
>>> bgmac->write = platform_bgmac_write;
>>> bgmac->idm_read = platform_bgmac_idm_read;
>>> @@ -213,6 +272,7 @@ static int bgmac_remove(struct platform_device *pdev)
>>> static const struct of_device_id bgmac_of_enet_match[] = {
>>> {.compatible = "brcm,amac",},
>>> {.compatible = "brcm,nsp-amac",},
>>> + {.compatible = "brcm,ns2-amac",},
>>> {},
>>> };
>>>
>>> diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c
>>> index 38876ec..1796208 100644
>>> --- a/drivers/net/ethernet/broadcom/bgmac.c
>>> +++ b/drivers/net/ethernet/broadcom/bgmac.c
>>> @@ -1082,6 +1082,9 @@ static void bgmac_enable(struct bgmac *bgmac)
>>> /* http://bcm-v4.sipsolutions.net/mac-gbit/gmac/chipinit */
>>> static void bgmac_chip_init(struct bgmac *bgmac)
>>> {
>>> + /* Clear any erroneously pending interrupts */
>>> + bgmac_write(bgmac, BGMAC_INT_STATUS, ~0);
>>> +
>>> /* 1 interrupt per received frame */
>>> bgmac_write(bgmac, BGMAC_INT_RECV_LAZY, 1 << BGMAC_IRL_FC_SHIFT);
>>>
>>> @@ -1158,8 +1161,6 @@ static int bgmac_open(struct net_device *net_dev)
>>> struct bgmac *bgmac = netdev_priv(net_dev);
>>> int err = 0;
>>>
>>> - bgmac_chip_reset(bgmac);
>>> -
>>
>> Is this removal intentional? Maybe it should be special cased with
>> checking for a NS2 BGMAC instance and not do it in that case?
>
> The reset seems completely unnecessary. There is already 2 resets in
> the probe routine, another reset serves no purpose. I can add it
> back, as it does not seem to have the negative effect I was seeing
> before.
After repeated ifconfig down/up, does Ethernet still work with this
reset removed?
Thanks.
>
> Of course, if I remove this one I should remove the reset in the close
> too (which seems even more unnecessary, but I didn't remove it).
>
> Thanks,
> Jon
>
>> --
>> Florian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" 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: phy: at803x: the Atheros 8031 supports pause frames
From: Timur Tabi @ 2016-10-27 22:24 UTC (permalink / raw)
To: Florian Fainelli, netdev, zefir.kurtisi, scampbel, alokc,
shankerd, andrew
In-Reply-To: <f34cd143-bf04-c17f-b3d6-e3077715a72a@gmail.com>
Florian Fainelli wrote:
> Hu? In my experience that should not come from supporting Pause frames
> or not, but rather properly configuring a (RG)MII delay, but your
> mileage may vary.
I can assure you, I'm more confused than you. I've been working in this
for almost two weeks, and not only does this patch align with other phy
drivers, but it does fix my bug. Without this patch, phylib does not
set the pause frame bits (10 and 11) in MII_ADVERTISE.
> It does not, support for Pause frames is a MAC-level feature, yet,
> PHYLIB (and that's been on my todo for a while now) insists on reporting
> the confusing phydev->pause and phydev->asym_pause, which really is what
> has been determined from auto-negotiating with your partner, as opposed
> to being a supported thing or not. The PHY has absolutely not business
> in that.
But there are pause frame bits in the MII_ADVERTISE register, and this
setting directly impacts whether those bits are set. I don't see how
this is a MAC-level feature.
> Your change should probably be in the Ethernet MAC driver, when you have
> successfully connected to the PHY, update phydev->supported to include
> SUPPORTED_Pause | SUPPORTED_Asym_pause.
The phylib documentation (networking/phy.txt) says:
Now just make sure that phydev->supported and phydev->advertising have any
values pruned from them which don't make sense for your controller (a
10/100
controller may be connected to a gigabit capable PHY, so you would need to
mask off SUPPORTED_1000baseT*). See include/linux/ethtool.h for
definitions
for these bitfields.
and then it says:
> Note that you should not SET any bits, or the PHY may
> get put into an unsupported state.
So are you sure I'm supposed to set those bits?
--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc. Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
^ permalink raw reply
* Re: [PATCH net-next RFC WIP] Patch for XDP support for virtio_net
From: Michael S. Tsirkin @ 2016-10-27 22:25 UTC (permalink / raw)
To: David Miller
Cc: john.fastabend, brouer, shrijeet, tom, netdev, shm, roopa,
nikolay
In-Reply-To: <20161027.174218.654961317277160121.davem@davemloft.net>
On Thu, Oct 27, 2016 at 05:42:18PM -0400, David Miller wrote:
> From: "Michael S. Tsirkin" <mst@redhat.com>
> Date: Fri, 28 Oct 2016 00:30:35 +0300
>
> > Something I'd like to understand is how does XDP address the
> > problem that 100Byte packets are consuming 4K of memory now.
>
> Via page pools. We're going to make a generic one, but right now
> each and every driver implements a quick list of pages to allocate
> from (and thus avoid the DMA man/unmap overhead, etc.)
So to clarify, ATM virtio doesn't attempt to avoid dma map/unmap
so there should be no issue with that even when using sub/page
regions, assuming DMA APIs support sub-page map/unmap correctly.
The only reason virtio attempts to use sub-page fragments is to conserve
memory so that truesize for a 100byte packets won't be 4K. Are you
saying pools will somehow mean we won't need to worry about that so
effectively truesize=100byte even though the rest of the 4k page goes
unused? If so we'll happily go back to allocate memory in 4K chunks like
we used to before 2613af0ed18a, and remove a bunch of complexity.
--
MST
^ permalink raw reply
* Re: [PATCH 4/5] net: ethernet: bgmac: add NS2 support
From: Ray Jui @ 2016-10-27 22:32 UTC (permalink / raw)
To: Jon Mason, Florian Fainelli
Cc: David Miller, Rob Herring, Mark Rutland,
rafal-g1n6cQUeyibVItvQsEIGlw,
bcm-kernel-feedback-list-dY08KVG/lbpWk0Htik3J/w,
netdev-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <ccbaf726-b1fc-975a-215c-17e2870328a6-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
On 10/27/2016 3:21 PM, Ray Jui wrote:
>
>
> On 10/27/2016 1:51 PM, Jon Mason wrote:
>> On Wed, Oct 26, 2016 at 02:50:39PM -0700, Florian Fainelli wrote:
>>> On 10/26/2016 12:36 PM, Jon Mason wrote:
>>>> Add support for the variant of amac hardware present in the Broadcom
>>>> Northstar2 based SoCs. Northstar2 requires an additional register to be
>>>> configured with the port speed/duplexity (NICPM). This can be added to
>>>> the link callback to hide it from the instances that do not use this.
>>>> Also, the bgmac_chip_reset() was intentionally removed to prevent the
>>>> resetting of the chip to the default values on open. Finally, clearing
>>>> of the pending interrupts on init is required due to observed issues on
>>>> some platforms.
>>>>
>>>> Signed-off-by: Jon Mason <jon.mason-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
>>>> ---
>>>
>>>> +static void bgmac_nicpm_speed_set(struct net_device *net_dev)
>>>> +{
>>>> + struct bgmac *bgmac = netdev_priv(net_dev);
>>>> + u32 val;
>>>> +
>>>> + if (!bgmac->plat.nicpm_base)
>>>> + return;
>>>> +
>>>> + val = NICPM_IOMUX_CTRL_INIT_VAL;
>>>> + switch (bgmac->net_dev->phydev->speed) {
>>>> + default:
>>>> + pr_err("Unsupported speed. Defaulting to 1000Mb\n");
>>>> + case SPEED_1000:
>>>> + val |= NICPM_IOMUX_CTRL_SPD_1000M << NICPM_IOMUX_CTRL_SPD_SHIFT;
>>>> + break;
>>>> + case SPEED_100:
>>>> + val |= NICPM_IOMUX_CTRL_SPD_100M << NICPM_IOMUX_CTRL_SPD_SHIFT;
>>>> + break;
>>>> + case SPEED_10:
>>>> + val |= NICPM_IOMUX_CTRL_SPD_10M << NICPM_IOMUX_CTRL_SPD_SHIFT;
>>>> + break;
>>>> + }
>>>> +
>>>> + writel(val, bgmac->plat.nicpm_base + NICPM_IOMUX_CTRL);
>>>> +
>>>> + usleep_range(10, 100);
>>>
>>> Does not seem like a good idea, do you need that sleep?
>>
>> Oops, that's not supposed to be there. Removed.
>>
>>
>>>> +
>>>> + bgmac_adjust_link(bgmac->net_dev);
>>>> +}
>>>> +
>>>> static int platform_phy_connect(struct bgmac *bgmac)
>>>> {
>>>> struct phy_device *phy_dev;
>>>>
>>>> - phy_dev = of_phy_get_and_connect(bgmac->net_dev, bgmac->dev->of_node,
>>>> - bgmac_adjust_link);
>>>> + if (bgmac->plat.nicpm_base)
>>>> + phy_dev = of_phy_get_and_connect(bgmac->net_dev,
>>>> + bgmac->dev->of_node,
>>>> + bgmac_nicpm_speed_set);
>>>> + else
>>>> + phy_dev = of_phy_get_and_connect(bgmac->net_dev,
>>>> + bgmac->dev->of_node,
>>>> + bgmac_adjust_link);
>>>> if (!phy_dev) {
>>>> dev_err(bgmac->dev, "Phy connect failed\n");
>>>> return -ENODEV;
>>>> }
>>>>
>>>> + if (bgmac->feature_flags & BGMAC_FEAT_LANE_SWAP)
>>>> + phy_dev->dev_flags |= PHY_BRCM_EXP_LANE_SWAP;
>>>> +
>>>> return 0;
>>>> }
>>>>
>>>> @@ -140,6 +188,9 @@ static int bgmac_probe(struct platform_device *pdev)
>>>>
>>>> platform_set_drvdata(pdev, bgmac);
>>>>
>>>> + if (of_property_read_bool(np, "brcm,enet-phy-lane-swap"))
>>>> + bgmac->feature_flags |= BGMAC_FEAT_LANE_SWAP;
>>>> +
>>>> /* Set the features of the 4707 family */
>>>> bgmac->feature_flags |= BGMAC_FEAT_CLKCTLST;
>>>> bgmac->feature_flags |= BGMAC_FEAT_NO_RESET;
>>>> @@ -182,6 +233,14 @@ static int bgmac_probe(struct platform_device *pdev)
>>>> if (IS_ERR(bgmac->plat.idm_base))
>>>> return PTR_ERR(bgmac->plat.idm_base);
>>>>
>>>> + regs = platform_get_resource_byname(pdev, IORESOURCE_MEM, "nicpm_base");
>>>> + if (regs) {
>>>> + bgmac->plat.nicpm_base = devm_ioremap_resource(&pdev->dev,
>>>> + regs);
>>>> + if (IS_ERR(bgmac->plat.nicpm_base))
>>>> + return PTR_ERR(bgmac->plat.nicpm_base);
>>>> + }
>>>> +
>>>> bgmac->read = platform_bgmac_read;
>>>> bgmac->write = platform_bgmac_write;
>>>> bgmac->idm_read = platform_bgmac_idm_read;
>>>> @@ -213,6 +272,7 @@ static int bgmac_remove(struct platform_device *pdev)
>>>> static const struct of_device_id bgmac_of_enet_match[] = {
>>>> {.compatible = "brcm,amac",},
>>>> {.compatible = "brcm,nsp-amac",},
>>>> + {.compatible = "brcm,ns2-amac",},
>>>> {},
>>>> };
>>>>
>>>> diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c
>>>> index 38876ec..1796208 100644
>>>> --- a/drivers/net/ethernet/broadcom/bgmac.c
>>>> +++ b/drivers/net/ethernet/broadcom/bgmac.c
>>>> @@ -1082,6 +1082,9 @@ static void bgmac_enable(struct bgmac *bgmac)
>>>> /* http://bcm-v4.sipsolutions.net/mac-gbit/gmac/chipinit */
>>>> static void bgmac_chip_init(struct bgmac *bgmac)
>>>> {
>>>> + /* Clear any erroneously pending interrupts */
>>>> + bgmac_write(bgmac, BGMAC_INT_STATUS, ~0);
>>>> +
>>>> /* 1 interrupt per received frame */
>>>> bgmac_write(bgmac, BGMAC_INT_RECV_LAZY, 1 << BGMAC_IRL_FC_SHIFT);
>>>>
>>>> @@ -1158,8 +1161,6 @@ static int bgmac_open(struct net_device *net_dev)
>>>> struct bgmac *bgmac = netdev_priv(net_dev);
>>>> int err = 0;
>>>>
>>>> - bgmac_chip_reset(bgmac);
>>>> -
>>>
>>> Is this removal intentional? Maybe it should be special cased with
>>> checking for a NS2 BGMAC instance and not do it in that case?
>>
>> The reset seems completely unnecessary. There is already 2 resets in
>> the probe routine, another reset serves no purpose. I can add it
>> back, as it does not seem to have the negative effect I was seeing
>> before.
>
> After repeated ifconfig down/up, does Ethernet still work with this
> reset removed?
>
> Thanks.
>
Sorry, I meant to say, after removing both reset calls in the open and
close functions, does Ethernet still work after repeated ifconfig
down/up with data traffic in the background?
I would guess this reset is required, to flush out and clean up the
internal memories and state machine of the AMAC core each time before
you bring up the interface. It might also be required in the close
function, such that after the interface bring down, there's no staled
data coming out of the AMAC core. But I cannot say for sure since I do
not try this out myself.
Thanks.
>>
>> Of course, if I remove this one I should remove the reset in the close
>> too (which seems even more unnecessary, but I didn't remove it).
>>
>> Thanks,
>> Jon
>>
>>> --
>>> Florian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" 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-next 5/5] ipv6: Compute multipath hash for forwarded ICMP errors from offending packet
From: Tom Herbert @ 2016-10-27 22:35 UTC (permalink / raw)
To: Jakub Sitnicki
Cc: Linux Kernel Network Developers, LKML, David S. Miller,
Alexey Kuznetsov, James Morris, Hideaki YOSHIFUJI,
Patrick McHardy
In-Reply-To: <1477301332-23954-6-git-send-email-jkbs@redhat.com>
On Mon, Oct 24, 2016 at 2:28 AM, Jakub Sitnicki <jkbs@redhat.com> wrote:
> Same as for the transmit path, let's do our best to ensure that received
> ICMP errors that may be subject to forwarding will be routed the same
> path as flow that triggered the error, if it was going in the opposite
> direction.
>
Unfortunately our ability to do this is generally quite limited. This
patch will select the route for multipath, but I don't believe sets
the same link in LAG and definitely can't help switches doing ECMP to
route the ICMP packet in the same way as the flow would be. Did you
see a problem that warrants solving this case?
Tom
> Signed-off-by: Jakub Sitnicki <jkbs@redhat.com>
> ---
> net/ipv6/route.c | 26 ++++++++++++++++++++++++++
> 1 file changed, 26 insertions(+)
>
> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index 1184c2b..c0f38ea 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -1150,6 +1150,30 @@ struct dst_entry *ip6_route_input_lookup(struct net *net,
> }
> EXPORT_SYMBOL_GPL(ip6_route_input_lookup);
>
> +static u32 ip6_multipath_icmp_hash(const struct sk_buff *skb)
> +{
> + const struct icmp6hdr *icmph = icmp6_hdr(skb);
> + const struct ipv6hdr *inner_iph;
> + struct ipv6hdr _inner_iph;
> +
> + if (icmph->icmp6_type != ICMPV6_DEST_UNREACH &&
> + icmph->icmp6_type != ICMPV6_PKT_TOOBIG &&
> + icmph->icmp6_type != ICMPV6_TIME_EXCEED &&
> + icmph->icmp6_type != ICMPV6_PARAMPROB)
> + goto standard_hash;
> +
> + inner_iph = skb_header_pointer(
> + skb, skb_transport_offset(skb) + sizeof(*icmph),
> + sizeof(_inner_iph), &_inner_iph);
> + if (!inner_iph)
> + goto standard_hash;
> +
> + return icmpv6_multipath_hash(inner_iph);
> +
> +standard_hash:
> + return 0; /* compute it later, if needed */
> +}
> +
> void ip6_route_input(struct sk_buff *skb)
> {
> const struct ipv6hdr *iph = ipv6_hdr(skb);
> @@ -1168,6 +1192,8 @@ void ip6_route_input(struct sk_buff *skb)
> tun_info = skb_tunnel_info(skb);
> if (tun_info && !(tun_info->mode & IP_TUNNEL_INFO_TX))
> fl6.flowi6_tun_key.tun_id = tun_info->key.tun_id;
> + if (unlikely(fl6.flowi6_proto == IPPROTO_ICMPV6))
> + fl6.mp_hash = ip6_multipath_icmp_hash(skb);
I will point out that this is only
> skb_dst_drop(skb);
> skb_dst_set(skb, ip6_route_input_lookup(net, skb->dev, &fl6, flags));
> }
> --
> 2.7.4
>
^ permalink raw reply
* Re: [PATCH] net: phy: at803x: the Atheros 8031 supports pause frames
From: Florian Fainelli @ 2016-10-27 22:39 UTC (permalink / raw)
To: Timur Tabi, netdev, zefir.kurtisi, scampbel, alokc, shankerd,
andrew
In-Reply-To: <58127E8E.4020301@codeaurora.org>
On 10/27/2016 03:24 PM, Timur Tabi wrote:
> Florian Fainelli wrote:
>
>> Hu? In my experience that should not come from supporting Pause frames
>> or not, but rather properly configuring a (RG)MII delay, but your
>> mileage may vary.
>
> I can assure you, I'm more confused than you. I've been working in this
> for almost two weeks, and not only does this patch align with other phy
> drivers, but it does fix my bug. Without this patch, phylib does not
> set the pause frame bits (10 and 11) in MII_ADVERTISE.
And that's expected, but if your MAC does not act upon phydev->pause and
phydev->asym_pause, then chances are that you can indeed lose packets
every once in a while.
The part that is totally crappy about Pause frames and PHYLIB is that we
need some information about whether this should be supported or not,
such that we can change MII_ADVERTISE accordingly to reflect that, and
what best way to do this than use one of these SUPPORTED_* bits to set
that, except, that unlike speed (which is both a MAC and PHY property),
Pause is exclusively MAC, yet, it transpires in PHYLIB.
MACs that I work with for instance need to be told to ignore pause
frames, or not ignore them, it all depends on what you want to
advertise/support.
>
>> It does not, support for Pause frames is a MAC-level feature, yet,
>> PHYLIB (and that's been on my todo for a while now) insists on reporting
>> the confusing phydev->pause and phydev->asym_pause, which really is what
>> has been determined from auto-negotiating with your partner, as opposed
>> to being a supported thing or not. The PHY has absolutely not business
>> in that.
>
> But there are pause frame bits in the MII_ADVERTISE register, and this
> setting directly impacts whether those bits are set. I don't see how
> this is a MAC-level feature.
This is a MAC level feature, that needs to be auto-negotiated with your
link partner, which is why there is room for this in MII_ADVERTISE, but
the PHY absolutely does not participate in Pause frames, other than
passing them through the MAC, like normal frames. Whether your MAC acts
upon that or not is a MAC dependent feature.
>
>> Your change should probably be in the Ethernet MAC driver, when you have
>> successfully connected to the PHY, update phydev->supported to include
>> SUPPORTED_Pause | SUPPORTED_Asym_pause.
>
> The phylib documentation (networking/phy.txt) says:
>
> Now just make sure that phydev->supported and phydev->advertising have any
> values pruned from them which don't make sense for your controller (a
> 10/100
> controller may be connected to a gigabit capable PHY, so you would need to
> mask off SUPPORTED_1000baseT*). See include/linux/ethtool.h for
> definitions
> for these bitfields.
>
> and then it says:
>
>> Note that you should not SET any bits, or the PHY may
>> get put into an unsupported state.
>
> So are you sure I'm supposed to set those bits?
Yes I am sure you should set these bits, the documentation is not
necessarily the authoritative source here (although it should). What
this probably meant to be written is something like: don't set any bits
that are not defined in ethtool.h, i.e: do not use this to store
persistent states.
--
Florian
^ permalink raw reply
* Re: [PATCH 1/5] net: phy: broadcom: Add BCM54810 phy entry
From: Jon Mason @ 2016-10-27 22:43 UTC (permalink / raw)
To: Andrew Lunn
Cc: David Miller, Rob Herring, Mark Rutland, Florian Fainelli,
devicetree, netdev, linux-kernel, bcm-kernel-feedback-list, rafal,
Vikas Soni, linux-arm-kernel
In-Reply-To: <20161027091505.GB12841@lunn.ch>
On Thu, Oct 27, 2016 at 11:15:05AM +0200, Andrew Lunn wrote:
> On Wed, Oct 26, 2016 at 03:35:57PM -0400, Jon Mason wrote:
> > From: Vikas Soni <vsoni@broadcom.com>
> >
> > Add BCM54810 phy entry
>
> Hi Jon, Vikis
>
> The subject line is a bit misleading. It does more than add a PHY ID
> entry.
All of the parts are related to adding the BCM54810 Phy, but I agree it
could be more verbose about what is happening.
> > Signed-off-by: Vikas Soni <vsoni@broadcom.com>
> > Signed-off-by: Jon Mason <jon.mason@broadcom.com>
> > ---
> > drivers/net/phy/Kconfig | 2 +-
> > drivers/net/phy/broadcom.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++
> > include/linux/brcmphy.h | 7 +++++
> > 3 files changed, 73 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> > index 45f68ea..31967ca 100644
> > --- a/drivers/net/phy/Kconfig
> > +++ b/drivers/net/phy/Kconfig
> > @@ -217,7 +217,7 @@ config BROADCOM_PHY
> > select BCM_NET_PHYLIB
> > ---help---
> > Currently supports the BCM5411, BCM5421, BCM5461, BCM54616S, BCM5464,
> > - BCM5481 and BCM5482 PHYs.
> > + BCM5481, BCM54810 and BCM5482 PHYs.
> >
> > config CICADA_PHY
> > tristate "Cicada PHYs"
> > diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
> > index 870327e..cdce761 100644
> > --- a/drivers/net/phy/broadcom.c
> > +++ b/drivers/net/phy/broadcom.c
> > @@ -35,6 +35,35 @@ static int bcm54xx_auxctl_write(struct phy_device *phydev, u16 regnum, u16 val)
> > return phy_write(phydev, MII_BCM54XX_AUX_CTL, regnum | val);
> > }
> >
> > +static int bcm54810_config(struct phy_device *phydev)
> > +{
> > + int rc;
> > +
> > + /* Disable BroadR-Reach */
> > + rc = bcm_phy_write_exp(phydev, BCM54810_EXP_BROADREACH_LRE_MISC_CTL, 0);
> > + if (rc < 0)
> > + return rc;
> > +
> > + /* SKEW DISABLE */
> > + rc = bcm54xx_auxctl_write(phydev, MII_BCM54XX_AUXCTL_SHDWSEL_MISC,
> > + 0xF0E0);
> > + if (rc < 0)
> > + return rc;
> > +
> > + /* DELAY DISABLE */
> > + rc = bcm54xx_auxctl_write(phydev, MII_BCM54XX_AUXCTL_SHDWSEL_MISC,
> > + 0x7000);
>
> This driver mostly uses symbolic names, not #defines. Please can you
> use #defines here and else were in this patch.
Will do. After looking at this, this appears to be setup for a read
that doesn't follow. I'll audit this, clean it up and resend.
> > + if (rc < 0)
> > + return rc;
> > +
> > + /* DELAY DISABLE */
> > + rc = bcm_phy_write_shadow(phydev, BCM54810_SHD_CLK_CTL, 0);
> > + if (rc < 0)
> > + return rc;
>
> Twice the same comment?
>
> > +
> > + return 0;
> > +}
> > +
> > /* Needs SMDSP clock enabled via bcm54xx_phydsp_config() */
> > static int bcm50610_a0_workaround(struct phy_device *phydev)
> > {
> > @@ -207,6 +236,20 @@ static int bcm54xx_config_init(struct phy_device *phydev)
> > (phydev->dev_flags & PHY_BRCM_AUTO_PWRDWN_ENABLE))
> > bcm54xx_adjust_rxrefclk(phydev);
> >
> > + if (BRCM_PHY_MODEL(phydev) == PHY_ID_BCM54810) {
> > + err = bcm54810_config(phydev);
> > + if (err)
> > + return err;
> > +
> > + reg = phy_read(phydev, MII_BMCR);
> > + if (reg < 0)
> > + return reg;
> > +
> > + err = phy_write(phydev, MII_BMCR, reg & ~BMCR_PDOWN);
> > + if (err)
> > + return err;
>
> This seems a bit odd. I would expect the PHY core correctly handles
> the PHY being powered down. Can you explain this a bit more, why it is
> needed.
I believe it was needed in earlier versions of the code, but doesn't
seem to be needed anymore. Removing.
>
> Thanks
> Andrew
^ permalink raw reply
* [PATCH net 1/1] tipc: fix broadcast link synchronization problem
From: Jon Maloy @ 2016-10-27 22:51 UTC (permalink / raw)
To: davem; +Cc: Jon Maloy, netdev, Paul Gortmaker, tipc-discussion
In commit 2d18ac4ba745 ("tipc: extend broadcast link initialization
criteria") we tried to fix a problem with the initial synchronization
of broadcast link acknowledge values. Unfortunately that solution is
not sufficient to solve the issue.
We have seen it happen that LINK_PROTOCOL/STATE packets with a valid
non-zero unicast acknowledge number may bypass BCAST_PROTOCOL
initialization, NAME_DISTRIBUTOR and other STATE packets with invalid
broadcast acknowledge numbers, leading to premature opening of the
broadcast link. When the bypassed packets finally arrive, they are
inadvertently accepted, and the already correctly initialized
acknowledge number in the broadcast receive link is overwritten by
the invalid (zero) value of the said packets. After this the broadcast
link goes stale.
We now fix this by marking the packets where we know the acknowledge
value is or may be invalid, and then ignoring the acks from those.
To this purpose, we claim an unused bit in the header to indicate that
the value is invalid. We set the bit to 1 in the initial BCAST_PROTOCOL
synchronization packet and all initial ("bulk") NAME_DISTRIBUTOR
packets, plus those LINK_PROTOCOL packets sent out before the broadcast
links are fully synchronized.
This minor protocol update is fully backwards compatible.
Reported-by: John Thompson <thompa.atl@gmail.com>
Tested-by: John Thompson <thompa.atl@gmail.com>
Signed-off-by: Jon Maloy <jon.maloy@ericsson.com>
---
net/tipc/bcast.c | 14 ++++++++++----
net/tipc/bcast.h | 3 ++-
net/tipc/link.c | 2 ++
net/tipc/msg.h | 17 +++++++++++++++++
net/tipc/name_distr.c | 1 +
net/tipc/node.c | 2 +-
6 files changed, 33 insertions(+), 6 deletions(-)
diff --git a/net/tipc/bcast.c b/net/tipc/bcast.c
index 753f774..aa1babb 100644
--- a/net/tipc/bcast.c
+++ b/net/tipc/bcast.c
@@ -247,11 +247,17 @@ int tipc_bcast_rcv(struct net *net, struct tipc_link *l, struct sk_buff *skb)
*
* RCU is locked, no other locks set
*/
-void tipc_bcast_ack_rcv(struct net *net, struct tipc_link *l, u32 acked)
+void tipc_bcast_ack_rcv(struct net *net, struct tipc_link *l,
+ struct tipc_msg *hdr)
{
struct sk_buff_head *inputq = &tipc_bc_base(net)->inputq;
+ u16 acked = msg_bcast_ack(hdr);
struct sk_buff_head xmitq;
+ /* Ignore bc acks sent by peer before bcast synch point was received */
+ if (msg_bc_ack_invalid(hdr))
+ return;
+
__skb_queue_head_init(&xmitq);
tipc_bcast_lock(net);
@@ -279,11 +285,11 @@ int tipc_bcast_sync_rcv(struct net *net, struct tipc_link *l,
__skb_queue_head_init(&xmitq);
tipc_bcast_lock(net);
- if (msg_type(hdr) == STATE_MSG) {
+ if (msg_type(hdr) != STATE_MSG) {
+ tipc_link_bc_init_rcv(l, hdr);
+ } else if (!msg_bc_ack_invalid(hdr)) {
tipc_link_bc_ack_rcv(l, msg_bcast_ack(hdr), &xmitq);
rc = tipc_link_bc_sync_rcv(l, hdr, &xmitq);
- } else {
- tipc_link_bc_init_rcv(l, hdr);
}
tipc_bcast_unlock(net);
diff --git a/net/tipc/bcast.h b/net/tipc/bcast.h
index 5ffe344..855d53c 100644
--- a/net/tipc/bcast.h
+++ b/net/tipc/bcast.h
@@ -55,7 +55,8 @@ void tipc_bcast_dec_bearer_dst_cnt(struct net *net, int bearer_id);
int tipc_bcast_get_mtu(struct net *net);
int tipc_bcast_xmit(struct net *net, struct sk_buff_head *list);
int tipc_bcast_rcv(struct net *net, struct tipc_link *l, struct sk_buff *skb);
-void tipc_bcast_ack_rcv(struct net *net, struct tipc_link *l, u32 acked);
+void tipc_bcast_ack_rcv(struct net *net, struct tipc_link *l,
+ struct tipc_msg *hdr);
int tipc_bcast_sync_rcv(struct net *net, struct tipc_link *l,
struct tipc_msg *hdr);
int tipc_nl_add_bc_link(struct net *net, struct tipc_nl_msg *msg);
diff --git a/net/tipc/link.c b/net/tipc/link.c
index b36e16c..1055164 100644
--- a/net/tipc/link.c
+++ b/net/tipc/link.c
@@ -1312,6 +1312,7 @@ static void tipc_link_build_proto_msg(struct tipc_link *l, int mtyp, bool probe,
msg_set_next_sent(hdr, l->snd_nxt);
msg_set_ack(hdr, l->rcv_nxt - 1);
msg_set_bcast_ack(hdr, bcl->rcv_nxt - 1);
+ msg_set_bc_ack_invalid(hdr, !node_up);
msg_set_last_bcast(hdr, l->bc_sndlink->snd_nxt - 1);
msg_set_link_tolerance(hdr, tolerance);
msg_set_linkprio(hdr, priority);
@@ -1574,6 +1575,7 @@ static void tipc_link_build_bc_init_msg(struct tipc_link *l,
__skb_queue_head_init(&list);
if (!tipc_link_build_bc_proto_msg(l->bc_rcvlink, false, 0, &list))
return;
+ msg_set_bc_ack_invalid(buf_msg(skb_peek(&list)), true);
tipc_link_xmit(l, &list, xmitq);
}
diff --git a/net/tipc/msg.h b/net/tipc/msg.h
index c3832cd..50a7398 100644
--- a/net/tipc/msg.h
+++ b/net/tipc/msg.h
@@ -714,6 +714,23 @@ static inline void msg_set_peer_stopping(struct tipc_msg *m, u32 s)
msg_set_bits(m, 5, 13, 0x1, s);
}
+static inline bool msg_bc_ack_invalid(struct tipc_msg *m)
+{
+ switch (msg_user(m)) {
+ case BCAST_PROTOCOL:
+ case NAME_DISTRIBUTOR:
+ case LINK_PROTOCOL:
+ return msg_bits(m, 5, 14, 0x1);
+ default:
+ return false;
+ }
+}
+
+static inline void msg_set_bc_ack_invalid(struct tipc_msg *m, bool invalid)
+{
+ msg_set_bits(m, 5, 14, 0x1, invalid);
+}
+
static inline char *msg_media_addr(struct tipc_msg *m)
{
return (char *)&m->hdr[TIPC_MEDIA_INFO_OFFSET];
diff --git a/net/tipc/name_distr.c b/net/tipc/name_distr.c
index a04fe9b..c1cfd92 100644
--- a/net/tipc/name_distr.c
+++ b/net/tipc/name_distr.c
@@ -156,6 +156,7 @@ static void named_distribute(struct net *net, struct sk_buff_head *list,
pr_warn("Bulk publication failure\n");
return;
}
+ msg_set_bc_ack_invalid(buf_msg(skb), true);
item = (struct distr_item *)msg_data(buf_msg(skb));
}
diff --git a/net/tipc/node.c b/net/tipc/node.c
index 7ef14e2..9d2f4c2 100644
--- a/net/tipc/node.c
+++ b/net/tipc/node.c
@@ -1535,7 +1535,7 @@ void tipc_rcv(struct net *net, struct sk_buff *skb, struct tipc_bearer *b)
if (unlikely(usr == LINK_PROTOCOL))
tipc_node_bc_sync_rcv(n, hdr, bearer_id, &xmitq);
else if (unlikely(tipc_link_acked(n->bc_entry.link) != bc_ack))
- tipc_bcast_ack_rcv(net, n->bc_entry.link, bc_ack);
+ tipc_bcast_ack_rcv(net, n->bc_entry.link, hdr);
/* Receive packet directly if conditions permit */
tipc_node_read_lock(n);
--
2.7.4
------------------------------------------------------------------------------
The Command Line: Reinvented for Modern Developers
Did the resurgence of CLI tooling catch you by surprise?
Reconnect with the command line and become more productive.
Learn the new .NET and ASP.NET CLI. Get your free copy!
http://sdm.link/telerik
^ permalink raw reply related
* Re: AW: Redundancy support through HSR and PRP
From: Murali Karicheri @ 2016-10-27 22:54 UTC (permalink / raw)
To: HEISE, Peter P, Arvid Brodin
Cc: open list:TI NETCP ETHERNET DRIVER, David Miller
In-Reply-To: <5027_1477401856_580F5D00_5027_1483_1_48dbc6a7637e4ed8a1bec40e850061b8@DE0-03DAG08-P03.central.mail.corp>
On 10/25/2016 09:24 AM, HEISE, Peter P wrote:
> Hi Murali, hi Arvid,
>
> also no work on PRP from my side.
>
> Should be pretty straight forward to get PRP support from the existing HSR code.
Thanks Arvid and Peter!
Basically I have following to be done w.r.t HSR/PRP.
1. Add PRP support
2. Add h/w offload some of the rx/tx redundancy and L2 forward handling to firmware.
If you have any thoughts on 2, please let me know. Our hardware is a switch
with HSR/PRP support. So I believe I need to have L2 offload done using the DSA
and run the hsr/prp driver on top of that. The MIB handling is done in the
firmware. So that has to be offloaded as well. My work has just begun and I hope
to come up with an RFC patch for HSR offload first followed by PRP support.
>From high level, it appears to be a straight forward to add the PRP support.
I might have to rename the net/hsr to something like net/hsr-prp ??
Also restructure the code a bit to support PRP
I have brought up HSR driver on my platform (HSRv0 and HSRv1) and it seems to work.
So I might be able to regress the existing once I add my change.
Peter,
Do you think you could help me do some testing in the future when patches become
available?
Murali
>
>
> -----Ursprüngliche Nachricht-----
> Von: Arvid Brodin [mailto:arvid.brodin@alten.se]
> Gesendet: Dienstag, 25. Oktober 2016 15:04
> An: Murali Karicheri
> Cc: open list:TI NETCP ETHERNET DRIVER; David Miller; HEISE, Peter P
> Betreff: Re: Redundancy support through HSR and PRP
>
> On 2016-10-24 18:35, Murali Karicheri wrote:
>>> On 10/10/2016 02:34 PM, Murali Karicheri wrote:
>>>> All,
>>>>
>>>> Wondering if there plan to add PRP driver support, like HSR in
>>>> Linux? AFAIK, PRP adds trailor to Ethernet frame and is used for Redundancy management like HSR.
>>>> So wondering why this is not supported.
>>>>
>>>> Thanks
>>>>
>>> I need to work on a prp driver for Linux. So if there is already
>>> someone working on this, I would like to join and contribute. Either
>>> way please respond so that I can work to add this support.
>>>
>> + Arvid
>>
>> Didn't copy HSR owner in my original email. Copying now.
>>
>
> Hi Murali,
>
> I'm not aware of anyone working on PRP support.
>
> I will have very little time, if any, to help out with this (or even test changes).
> In fact, if you want to send a patch for the MAINTAINERS file to take over maintainership of the HSR/PRP driver, that would probably be a good thing.
>
> (I'm CC'ing also Peter Heise to this conversation, since he's done some recent work on the HSR driver, and might want to keep updated.)
>
>
--
Murali Karicheri
Linux Kernel, Keystone
^ permalink raw reply
* Re: [PATCH net 1/2] vxlan: avoid using stale vxlan socket.
From: Stephen Hemminger @ 2016-10-27 23:02 UTC (permalink / raw)
To: Pravin B Shelar; +Cc: netdev
In-Reply-To: <1477594316-118234-1-git-send-email-pshelar@ovn.org>
On Thu, 27 Oct 2016 11:51:55 -0700
Pravin B Shelar <pshelar@ovn.org> wrote:
> - vxlan->vn4_sock = NULL;
> + rcu_assign_pointer(vxlan->vn4_sock, NULL);
> #if IS_ENABLED(CONFIG_IPV6)
> - vxlan->vn6_sock = NULL;
> + rcu_assign_pointer(vxlan->vn6_sock, NULL);
> if (ipv6 || metadata)
User RCU_INIT_POINTER when setting pointer to NULL.
It avoids unnecessary barrier.
^ permalink raw reply
* [PATCH net] enic: fix rq disable
From: Govindarajulu Varadarajan @ 2016-10-27 23:01 UTC (permalink / raw)
To: davem, netdev, benve; +Cc: Govindarajulu Varadarajan, Govindarajulu Varadarajan
When MTU is changed from 9000 to 1500 while there is burst of inbound 9000
bytes packets, adaptor sometimes delivers 9000 bytes packets to 1500 bytes
buffers. This causes memory corruption and sometimes crash.
This is because of a race condition in adaptor between "RQ disable"
clearing descriptor mini-cache and mini-cache valid bit being set by
completion of descriptor fetch. This can result in stale RQ desc being
cached and used when packets arrive. In this case, the stale descriptor
have old MTU value.
Solution is to write RQ->disable twice. The first write will stop any
further desc fetches, allowing the second disable to clear the mini-cache
valid bit without danger of a race.
Also, the check for rq->running becoming 0 after writing rq->enable to 0
is not done properly. When incoming packets are flooding the interface,
rq->running will pulse high for each dropped packet. Since the driver was
waiting for 10us between each poll, it is possible to see rq->running = 1
1000 times in a row, even though it is not actually stuck running.
This results in false failure of vnic_rq_disable(). Fix is to try more
than 1000 time without delay between polls to ensure we do not miss when
running goes low.
In old adaptors rq->enable needs to be re-written to 0 when posted_index
is reset in vnic_rq_clean() in order to keep rq->prefetch_index in sync.
Signed-off-by: Govindarajulu Varadarajan <_govind@gmx.com>
---
drivers/net/ethernet/cisco/enic/vnic_rq.c | 32 ++++++++++++++++++++++---------
1 file changed, 23 insertions(+), 9 deletions(-)
diff --git a/drivers/net/ethernet/cisco/enic/vnic_rq.c b/drivers/net/ethernet/cisco/enic/vnic_rq.c
index e572a52..36bc2c7 100644
--- a/drivers/net/ethernet/cisco/enic/vnic_rq.c
+++ b/drivers/net/ethernet/cisco/enic/vnic_rq.c
@@ -169,19 +169,28 @@ int vnic_rq_disable(struct vnic_rq *rq)
{
unsigned int wait;
struct vnic_dev *vdev = rq->vdev;
+ int i;
- iowrite32(0, &rq->ctrl->enable);
+ /* Due to a race condition with clearing RQ "mini-cache" in hw, we need
+ * to disable the RQ twice to guarantee that stale descriptors are not
+ * used when this RQ is re-enabled.
+ */
+ for (i = 0; i < 2; i++) {
+ iowrite32(0, &rq->ctrl->enable);
- /* Wait for HW to ACK disable request */
- for (wait = 0; wait < 1000; wait++) {
- if (!(ioread32(&rq->ctrl->running)))
- return 0;
- udelay(10);
- }
+ /* Wait for HW to ACK disable request */
+ for (wait = 20000; wait > 0; wait--)
+ if (!ioread32(&rq->ctrl->running))
+ break;
+ if (!wait) {
+ vdev_neterr(vdev, "Failed to disable RQ[%d]\n",
+ rq->index);
- vdev_neterr(vdev, "Failed to disable RQ[%d]\n", rq->index);
+ return -ETIMEDOUT;
+ }
+ }
- return -ETIMEDOUT;
+ return 0;
}
void vnic_rq_clean(struct vnic_rq *rq,
@@ -212,6 +221,11 @@ void vnic_rq_clean(struct vnic_rq *rq,
[fetch_index % VNIC_RQ_BUF_BLK_ENTRIES(count)];
iowrite32(fetch_index, &rq->ctrl->posted_index);
+ /* Anytime we write fetch_index, we need to re-write 0 to rq->enable
+ * to re-sync internal VIC state.
+ */
+ iowrite32(0, &rq->ctrl->enable);
+
vnic_dev_clear_desc_ring(&rq->ring);
}
--
2.10.1
^ permalink raw reply related
* RE: [PATCH net-next RFC WIP] Patch for XDP support for virtio_net
From: Shrijeet Mukherjee @ 2016-10-28 0:02 UTC (permalink / raw)
To: John Fastabend, Jesper Dangaard Brouer, David Miller
Cc: mst, shrijeet, tom, netdev, Roopa Prabhu, Nikolay Aleksandrov
In-Reply-To: <58126CF4.5090500@gmail.com>
> >
> > Looking at the virtio_net.c code, the function call receive_big()
> > might actually be okay for XDP, unless the incoming packet is larger
> > than PAGE_SIZE and thus uses several pages (via a linked list in page-
> >private).
> >
> > The receive_mergeable() does not look compatible with XDP.
> >
>
> Looks to me the correct conditions can be met by getting the correct
> feature negotiation to happen, specifically no VIRTIO_NET_F_MRG_RXBUF
> and one of the TSO/ECN/GSO feature bits set the big_packets flag as
Jesper
> notes.
>
> Srijeet, are you going to give this a try? I'm trying to get the device
side
> working by the way on the vhost interface.
>
Yup, I did try it .. but it looked like the device gets to make that call
(Unless someone tells me I did something wrong) If I turned that feature
off in the driver it crashed (did not chase too far down that hole) but
let me try that again.
Wanted to consider a path, which looks like has been considered and
discarded, but for the mergable buffers case, if we disallow edit's (no
writes) would that be an acceptable answer ? sorry in advance for the
repeat question, but the benefit of mergeable seems like it can be real
for all packets that are not being XDP'ed .. so maybe apps can make a
choice of doing a copy into a local buffer and XDP_TX from there ?
^ permalink raw reply
* Re: [PATCH v2 1/5] kconfig: introduce the "imply" keyword
From: Paul Bolle @ 2016-10-28 0:17 UTC (permalink / raw)
To: Nicolas Pitre, John Stultz, Richard Cochran, Michal Marek
Cc: Thomas Gleixner, Josh Triplett, Edward Cree, netdev, linux-kbuild,
linux-kernel
In-Reply-To: <1477448931-29051-2-git-send-email-nicolas.pitre@linaro.org>
On Tue, 2016-10-25 at 22:28 -0400, Nicolas Pitre wrote:
> The "imply" keyword is a weak version of "select" where the target
> config symbol can still be turned off, avoiding those pitfalls that come
> with the "select" keyword.
>
> This is useful e.g. with multiple drivers that want to indicate their
> ability to hook into a given subsystem
"hook into a [...] subsystem" is rather vague.
> while still being able to configure that subsystem out
s/being able to/allowing the user to/, correct?
> and keep those drivers selected.
Perhaps replace that with: "without also having to unset these
drivers"?
> Currently, the same effect can almost be achieved with:
>
> config DRIVER_A
> tristate
>
> config DRIVER_B
> tristate
>
> config DRIVER_C
> tristate
>
> config DRIVER_D
> tristate
>
> [...]
>
> config SUBSYSTEM_X
> tristate
> default DRIVER_A || DRIVER_B || DRIVER_C || DRIVER_D || [...]
>
> This is unwieldly
unwieldy
> to maintain especially with a large number of drivers.
> Furthermore, there is no easy way to restrict the choice for SUBSYSTEM_X
> to y or n, excluding m, when some drivers are built-in. The "select"
> keyword allows for excluding m, but it excludes n as well. Hence
> this "imply" keyword. The above becomes:
>
> config DRIVER_A
> tristate
> imply SUBSYSTEM_X
>
> config DRIVER_B
> tristate
> imply SUBSYSTEM_X
>
> [...]
>
> config SUBSYSTEM_X
> tristate
>
> This is much cleaner, and way more flexible than "select". SUBSYSTEM_X
> can still be configured out, and it can be set as a module when none of
> the drivers are selected or all of them are also modular.
[I already commented on this sentence in a previous message.]
> --- a/Documentation/kbuild/kconfig-language.txt
> +++ b/Documentation/kbuild/kconfig-language.txt
> That will limit the usefulness but on the other hand avoid
> the illegal configurations all over.
>
> +- weak reverse dependencies: "imply" <symbol> ["if" <expr>]
You probably got "["if" <expr>]" for free by copying what's there for
select. But this series doesn't use it, so perhaps it would be better
to not document it yet. But that is rather sneaky. Dunno.
> + This is similar to "select" as it enforces a lower limit on another
> + symbol except that the "implied" config symbol's value may still be
> + set to n from a direct dependency or with a visible prompt.
This is seriously hard to parse. But it's late here, so it might just
be me.
> + Given the following example:
> +
> + config FOO
> + tristate
> + imply BAZ
> +
> + config BAZ
> + tristate
> + depends on BAR
> +
> + The following values are possible:
> +
> + FOO BAR BAZ's default choice for BAZ
> + --- --- ------------- --------------
> + n y n N/m/y
> + m y m M/y/n
> + y y y Y/n
Also
n n * N
m n * N
Is that right?
> + y n * N
But what does '*' mean?
What happens when a tristate symbol is implied by a symbol set to 'y'
and by a symbol set to 'm'?
And in your example BAR is bool, right? Does the above get more
complicated if BAR would be tristate?
How does setting a direct default for BAZ interfere with the implied
values?
> + This is useful e.g. with multiple drivers that want to indicate their
> + ability to hook into a given subsystem while still being able to
> + configure that subsystem out and keep those drivers selected.
See my comments above.
> b) Match dependency semantics:
> b1) Swap all "select FOO" to "depends on FOO" or,
> b2) Swap all "depends on FOO" to "select FOO"
> + c) Consider the use of "imply" instead of "select"
If memory serves me right this text was added after a long discussion
with Luis Rodriguez. Luis might want to have a look at any
Haven't looked at the code yet, sorry. I'm still trying to see whether
I can wrap my mind around this. It looks like just setting a default on
another symbol, but there could be a twist I missed.
Paul Bolle
^ permalink raw reply
* RE: [PATCH net-next RFC WIP] Patch for XDP support for virtio_net
From: Shrijeet Mukherjee @ 2016-10-28 0:46 UTC (permalink / raw)
To: John Fastabend, Jesper Dangaard Brouer, David Miller
Cc: mst, shrijeet, tom, netdev, Roopa Prabhu, Nikolay Aleksandrov
In-Reply-To: <49429fa1fdf77d896112528861ae3854@mail.gmail.com>
> -----Original Message-----
> From: Shrijeet Mukherjee [mailto:shm@cumulusnetworks.com]
> Sent: Thursday, October 27, 2016 5:02 PM
> To: John Fastabend <john.fastabend@gmail.com>; Jesper Dangaard Brouer
> <brouer@redhat.com>; David Miller <davem@davemloft.net>
> Cc: mst@redhat.com; shrijeet@gmail.com; tom@herbertland.com;
> netdev@vger.kernel.org; Roopa Prabhu <roopa@cumulusnetworks.com>;
> Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
> Subject: RE: [PATCH net-next RFC WIP] Patch for XDP support for virtio_net
>
> > >
> > > Looking at the virtio_net.c code, the function call receive_big()
> > >might actually be okay for XDP, unless the incoming packet is larger
> > >than PAGE_SIZE and thus uses several pages (via a linked list in
> > >page- private).
> > >
> > > The receive_mergeable() does not look compatible with XDP.
> > >
> >
> > Looks to me the correct conditions can be met by getting the correct
> > feature negotiation to happen, specifically no
> VIRTIO_NET_F_MRG_RXBUF
> > and one of the TSO/ECN/GSO feature bits set the big_packets flag as
> Jesper
> > notes.
> >
> > Srijeet, are you going to give this a try? I'm trying to get the
> > device
> side
> > working by the way on the vhost interface.
> >
>
> Yup, I did try it .. but it looked like the device gets to make that call
> (Unless
> someone tells me I did something wrong) If I turned that feature off in
> the
> driver it crashed (did not chase too far down that hole) but let me try
> that
> again.
Nevermind, I must have had some other issue. This does seem to work using
TSO as the function. Will spin something up to have the device export an XDP
capability which would block MERGEABLE for now and we can sort out alternate
plans later ?
>
> Wanted to consider a path, which looks like has been considered and
> discarded, but for the mergable buffers case, if we disallow edit's (no
> writes) would that be an acceptable answer ? sorry in advance for the
> repeat question, but the benefit of mergeable seems like it can be real
> for
> all packets that are not being XDP'ed .. so maybe apps can make a choice
> of
> doing a copy into a local buffer and XDP_TX from there ?
^ permalink raw reply
* Re: [PATCH net-next 2/2] net/mlx4_en: Refactor the XDP forwarding rings scheme
From: Alexei Starovoitov @ 2016-10-28 1:07 UTC (permalink / raw)
To: Tariq Toukan; +Cc: David S. Miller, netdev, Eran Ben Elisha, Brenden Blanco
In-Reply-To: <1477579924-32737-3-git-send-email-tariqt@mellanox.com>
On Thu, Oct 27, 2016 at 05:52:04PM +0300, Tariq Toukan wrote:
> Separately manage the two types of TX rings: regular ones, and XDP.
> Upon an XDP set, do not borrow regular TX rings and convert them
> into XDP ones, but allocate new ones, unless we hit the max number
> of rings.
> Which means that in systems with smaller #cores we will not consume
> the current TX rings for XDP, while we are still in the num TX limit.
The commit log is too scarce for details...
So questions:
- Did you test with changing the number of channels after xdp prog is loaded?
That was the recent bug that Brenden fixed.
- does it still have 256 tx queue limit or xdp tx rings can go over?
- Any performance implications ?
Brenden, could you please review this patch?
^ permalink raw reply
* Re: [PATCH net-next RFC WIP] Patch for XDP support for virtio_net
From: David Miller @ 2016-10-28 1:35 UTC (permalink / raw)
To: mst; +Cc: john.fastabend, brouer, shrijeet, tom, netdev, shm, roopa,
nikolay
In-Reply-To: <20161028011739-mutt-send-email-mst@kernel.org>
From: "Michael S. Tsirkin" <mst@redhat.com>
Date: Fri, 28 Oct 2016 01:25:48 +0300
> On Thu, Oct 27, 2016 at 05:42:18PM -0400, David Miller wrote:
>> From: "Michael S. Tsirkin" <mst@redhat.com>
>> Date: Fri, 28 Oct 2016 00:30:35 +0300
>>
>> > Something I'd like to understand is how does XDP address the
>> > problem that 100Byte packets are consuming 4K of memory now.
>>
>> Via page pools. We're going to make a generic one, but right now
>> each and every driver implements a quick list of pages to allocate
>> from (and thus avoid the DMA man/unmap overhead, etc.)
>
> So to clarify, ATM virtio doesn't attempt to avoid dma map/unmap
> so there should be no issue with that even when using sub/page
> regions, assuming DMA APIs support sub-page map/unmap correctly.
That's not what I said.
The page pools are meant to address the performance degradation from
going to having one packet per page for the sake of XDP's
requirements.
You still need to have one packet per page for correct XDP operation
whether you do page pools or not, and whether you have DMA mapping
(or it's equivalent virutalization operation) or not.
^ permalink raw reply
* Re: [PATCH net-next RFC WIP] Patch for XDP support for virtio_net
From: Alexander Duyck @ 2016-10-28 1:43 UTC (permalink / raw)
To: David Miller
Cc: Michael S. Tsirkin, John Fastabend, Jesper Dangaard Brouer,
shrijeet, Tom Herbert, Netdev, Shrijeet Mukherjee, roopa, nikolay
In-Reply-To: <20161027.213512.334468356710231957.davem@davemloft.net>
On Thu, Oct 27, 2016 at 6:35 PM, David Miller <davem@davemloft.net> wrote:
> From: "Michael S. Tsirkin" <mst@redhat.com>
> Date: Fri, 28 Oct 2016 01:25:48 +0300
>
>> On Thu, Oct 27, 2016 at 05:42:18PM -0400, David Miller wrote:
>>> From: "Michael S. Tsirkin" <mst@redhat.com>
>>> Date: Fri, 28 Oct 2016 00:30:35 +0300
>>>
>>> > Something I'd like to understand is how does XDP address the
>>> > problem that 100Byte packets are consuming 4K of memory now.
>>>
>>> Via page pools. We're going to make a generic one, but right now
>>> each and every driver implements a quick list of pages to allocate
>>> from (and thus avoid the DMA man/unmap overhead, etc.)
>>
>> So to clarify, ATM virtio doesn't attempt to avoid dma map/unmap
>> so there should be no issue with that even when using sub/page
>> regions, assuming DMA APIs support sub-page map/unmap correctly.
>
> That's not what I said.
>
> The page pools are meant to address the performance degradation from
> going to having one packet per page for the sake of XDP's
> requirements.
>
> You still need to have one packet per page for correct XDP operation
> whether you do page pools or not, and whether you have DMA mapping
> (or it's equivalent virutalization operation) or not.
Maybe I am missing something here, but why do you need to limit things
to one packet per page for correct XDP operation? Most of the drivers
out there now are usually storing something closer to at least 2
packets per page, and with the DMA API fixes I am working on there
should be no issue with changing the contents inside those pages since
we won't invalidate or overwrite the data after the DMA buffer has
been synchronized for use by the CPU.
^ permalink raw reply
* [PATCH v2] ip6_tunnel: Clear IP6CB in ip6_tnl_xmit() after encapsulation
From: Eli Cooper @ 2016-10-28 1:52 UTC (permalink / raw)
To: netdev, David S . Miller
skb->cb may contain data from previous layers. In the observed scenario,
the garbage data were misinterpreted as IP6CB(skb)->frag_max_size, so
that small packets sent through the tunnel are mistakenly fragmented.
This patch clears the control buffer for the next layer, after an IPv6
header is installed.
Signed-off-by: Eli Cooper <elicooper@gmx.com>
---
v2: clears the whole IP6CB altogether and does it after encapsulation
net/ipv6/ip6_tunnel.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
index 202d16a..1487e17 100644
--- a/net/ipv6/ip6_tunnel.c
+++ b/net/ipv6/ip6_tunnel.c
@@ -1174,6 +1174,7 @@ int ip6_tnl_xmit(struct sk_buff *skb, struct net_device *dev, __u8 dsfield,
skb_push(skb, sizeof(struct ipv6hdr));
skb_reset_network_header(skb);
+ memset(skb->cb, 0, sizeof(struct inet6_skb_parm));
ipv6h = ipv6_hdr(skb);
ip6_flow_hdr(ipv6h, INET_ECN_encapsulate(0, dsfield),
ip6_make_flowlabel(net, skb, fl6->flowlabel, true, fl6));
--
2.10.1
^ permalink raw reply related
* Re: [PATCH net-next RFC WIP] Patch for XDP support for virtio_net
From: David Miller @ 2016-10-28 2:10 UTC (permalink / raw)
To: alexander.duyck
Cc: mst, john.fastabend, brouer, shrijeet, tom, netdev, shm, roopa,
nikolay
In-Reply-To: <CAKgT0Ufe3QYtFpmtsL39-nbyy+YEt=gZKU_WUr9Wir+LTYLuEQ@mail.gmail.com>
From: Alexander Duyck <alexander.duyck@gmail.com>
Date: Thu, 27 Oct 2016 18:43:59 -0700
> On Thu, Oct 27, 2016 at 6:35 PM, David Miller <davem@davemloft.net> wrote:
>> From: "Michael S. Tsirkin" <mst@redhat.com>
>> Date: Fri, 28 Oct 2016 01:25:48 +0300
>>
>>> On Thu, Oct 27, 2016 at 05:42:18PM -0400, David Miller wrote:
>>>> From: "Michael S. Tsirkin" <mst@redhat.com>
>>>> Date: Fri, 28 Oct 2016 00:30:35 +0300
>>>>
>>>> > Something I'd like to understand is how does XDP address the
>>>> > problem that 100Byte packets are consuming 4K of memory now.
>>>>
>>>> Via page pools. We're going to make a generic one, but right now
>>>> each and every driver implements a quick list of pages to allocate
>>>> from (and thus avoid the DMA man/unmap overhead, etc.)
>>>
>>> So to clarify, ATM virtio doesn't attempt to avoid dma map/unmap
>>> so there should be no issue with that even when using sub/page
>>> regions, assuming DMA APIs support sub-page map/unmap correctly.
>>
>> That's not what I said.
>>
>> The page pools are meant to address the performance degradation from
>> going to having one packet per page for the sake of XDP's
>> requirements.
>>
>> You still need to have one packet per page for correct XDP operation
>> whether you do page pools or not, and whether you have DMA mapping
>> (or it's equivalent virutalization operation) or not.
>
> Maybe I am missing something here, but why do you need to limit things
> to one packet per page for correct XDP operation? Most of the drivers
> out there now are usually storing something closer to at least 2
> packets per page, and with the DMA API fixes I am working on there
> should be no issue with changing the contents inside those pages since
> we won't invalidate or overwrite the data after the DMA buffer has
> been synchronized for use by the CPU.
Because with SKB's you can share the page with other packets.
With XDP you simply cannot.
It's software semantics that are the issue. SKB frag list pages
are read only, XDP packets are writable.
This has nothing to do with "writability" of the pages wrt. DMA
mapping or cpu mappings.
^ 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