* Re: [PATCH net] net: openvswitch: fix possible memleak on create vport fails
From: Pravin Shelar @ 2019-09-22 3:17 UTC (permalink / raw)
To: Tonghao Zhang, Hillf Danton
Cc: Greg Rose, Linux Kernel Network Developers, Taehee Yoo
In-Reply-To: <1568734808-42628-1-git-send-email-xiangxia.m.yue@gmail.com>
On Sat, Sep 21, 2019 at 8:07 AM <xiangxia.m.yue@gmail.com> wrote:
>
> From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
>
> If we register a net device which name is not valid
> (dev_get_valid_name), register_netdevice will return err
> codes and will not run dev->priv_destructor. The memory
> will leak. This patch adds check in ovs_vport_free and
> set the vport NULL.
>
> Fixes: 309b66970ee2 ("net: openvswitch: do not free vport if register_netdevice() is failed.")
> Cc: Taehee Yoo <ap420073@gmail.com>
> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> ---
> net/openvswitch/vport-internal_dev.c | 8 ++------
> net/openvswitch/vport.c | 9 +++++++++
> 2 files changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/net/openvswitch/vport-internal_dev.c b/net/openvswitch/vport-internal_dev.c
> index d2437b5..074c43f 100644
> --- a/net/openvswitch/vport-internal_dev.c
> +++ b/net/openvswitch/vport-internal_dev.c
> @@ -159,7 +159,6 @@ static struct vport *internal_dev_create(const struct vport_parms *parms)
> struct internal_dev *internal_dev;
> struct net_device *dev;
> int err;
> - bool free_vport = true;
>
> vport = ovs_vport_alloc(0, &ovs_internal_vport_ops, parms);
> if (IS_ERR(vport)) {
> @@ -190,10 +189,8 @@ static struct vport *internal_dev_create(const struct vport_parms *parms)
>
> rtnl_lock();
> err = register_netdevice(vport->dev);
> - if (err) {
> - free_vport = false;
> + if (err)
> goto error_unlock;
> - }
>
> dev_set_promiscuity(vport->dev, 1);
> rtnl_unlock();
> @@ -207,8 +204,7 @@ static struct vport *internal_dev_create(const struct vport_parms *parms)
> error_free_netdev:
> free_netdev(dev);
> error_free_vport:
> - if (free_vport)
> - ovs_vport_free(vport);
> + ovs_vport_free(vport);
> error:
> return ERR_PTR(err);
> }
> diff --git a/net/openvswitch/vport.c b/net/openvswitch/vport.c
> index 3fc38d1..281259a 100644
> --- a/net/openvswitch/vport.c
> +++ b/net/openvswitch/vport.c
> @@ -157,11 +157,20 @@ struct vport *ovs_vport_alloc(int priv_size, const struct vport_ops *ops,
> */
> void ovs_vport_free(struct vport *vport)
> {
> + /* We should check whether vport is NULL.
> + * We may free it again, for example in internal_dev_create
> + * if register_netdevice fails, vport may have been freed via
> + * internal_dev_destructor.
> + */
> + if (unlikely(!vport))
> + return;
> +
> /* vport is freed from RCU callback or error path, Therefore
> * it is safe to use raw dereference.
> */
> kfree(rcu_dereference_raw(vport->upcall_portids));
> kfree(vport);
> + vport = NULL;
> }
> EXPORT_SYMBOL_GPL(ovs_vport_free);
>
> --
There is already patch a patch to fix this memory leak.
https://patchwork.ozlabs.org/patch/1144316/
Can you or Hillf post it on netdev list?
Thanks.
^ permalink raw reply
* Re: CONFIG_NET_TC_SKB_EXT
From: Pravin Shelar @ 2019-09-22 3:18 UTC (permalink / raw)
To: Daniel Borkmann
Cc: Jakub Kicinski, Vlad Buslov, David Miller, Paul Blakey,
netdev@vger.kernel.org, Jiri Pirko, Cong Wang, Jamal Hadi Salim,
Simon Horman, Alexei Starovoitov, Or Gerlitz
In-Reply-To: <f1983a74-d144-6d21-9b20-59cea9afc366@iogearbox.net>
On Fri, Sep 20, 2019 at 5:06 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 9/21/19 12:56 AM, Jakub Kicinski wrote:
> [...]
> >> I thought idea of stuffing things into skb extensions are only justified if
> >> it's not enabled by default for everyone. :(
> >>
> >> [0] https://lore.kernel.org/netdev/CAHC9VhSz1_KA1tCJtNjwK26BOkGhKGbPT7v1O82mWPduvWwd4A@mail.gmail.com/T/#u
> >
> > The skb ext allocation is only done with GOTO_CHAIN, which AFAIK only
> > has practical use for offload. We could perhaps add another static
> > branch there or move the OvS static branch out of the OvS module so
> > there are no linking issues?
> >
> > I personally have little sympathy for this piece of code, it is perhaps
> > the purest form of a wobbly narrow-use construct pushed into TC for HW
> > offload.
> >
> > Any suggestions on the way forward? :(
>
> Presumably there are no clean solutions here, but on the top of my head for
> this use case, you'd need to /own/ the underlying datapath anyway, so couldn't
> you program the OVS key->recirc_id based on skb->mark (or alternatively via
> skb->tc_index) which was previously set by tc ingress?
>
If we are going down this path, tc_index should be used. skb->mark is
already used in OVS flow match, overloading it for this purpose would
be complicated.
^ permalink raw reply
* Re: [PATCH] qede: qede_fp: simplify a bit 'qede_rx_build_skb()'
From: Jakub Kicinski @ 2019-09-22 3:18 UTC (permalink / raw)
To: Christophe JAILLET
Cc: aelior, GR-everest-linux-l2, davem, netdev, linux-kernel,
kernel-janitors
In-Reply-To: <20190920045656.3725-1-christophe.jaillet@wanadoo.fr>
On Fri, 20 Sep 2019 06:56:56 +0200, Christophe JAILLET wrote:
> Use 'skb_put_data()' instead of rewritting it.
> This improves readability.
>
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
Applied, thank you.
^ permalink raw reply
* Re: [PATCH V2 net 1/1] net/tls(TLS_SW): Fix list_del double free caused by a race condition in tls_tx_records
From: Pooja Trivedi @ 2019-09-22 3:19 UTC (permalink / raw)
To: Jakub Kicinski
Cc: netdev, davem, daniel, john.fastabend, davejwatson, aviadye,
borisp, Pooja Trivedi, Mallesham Jatharakonda
In-Reply-To: <20190918144528.57a5cb50@cakuba.netronome.com>
On Wed, Sep 18, 2019 at 5:45 PM Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
>
> On Wed, 18 Sep 2019 17:37:44 -0400, Pooja Trivedi wrote:
> > Hi Jakub,
> >
> > I have explained one potential way for the race to happen in my
> > original message to the netdev mailing list here:
> > https://marc.info/?l=linux-netdev&m=156805120229554&w=2
> >
> > Here is the part out of there that's relevant to your question:
> >
> > -----------------------------------------
> >
> > One potential way for race condition to appear:
> >
> > When under tcp memory pressure, Thread 1 takes the following code path:
> > do_sendfile ---> ... ---> .... ---> tls_sw_sendpage --->
> > tls_sw_do_sendpage ---> tls_tx_records ---> tls_push_sg --->
> > do_tcp_sendpages ---> sk_stream_wait_memory ---> sk_wait_event
>
> Ugh, so do_tcp_sendpages() can also release the lock :/
>
> Since the problem occurs in tls_sw_do_sendpage() and
> tls_sw_do_sendmsg() as well, should we perhaps fix it at that level?
That won't do because tls_tx_records also gets called when completion
callbacks schedule delayed work. That was the code path that caused
the crash for my test. Cavium's nitrox crypto offload driver calling
tls_encrypt_done, which calls schedule_delayed_work. Delayed work that
was scheduled would then be processed by tx_work_handler.
Notice in my previous reply,
"Thread 2 code path:
tx_work_handler ---> tls_tx_records"
"Thread 2 code path:
tx_work_handler ---> tls_tx_records"
^ permalink raw reply
* Re: CONFIG_NET_TC_SKB_EXT
From: Alexei Starovoitov @ 2019-09-22 4:51 UTC (permalink / raw)
To: Pravin Shelar
Cc: Daniel Borkmann, Jakub Kicinski, Vlad Buslov, David Miller,
Paul Blakey, netdev@vger.kernel.org, Jiri Pirko, Cong Wang,
Jamal Hadi Salim, Simon Horman, Or Gerlitz
In-Reply-To: <CAOrHB_Bqhq6cy6QgyEymHaUDk-BN9fkkQ-rzCqWeN35sqiym4w@mail.gmail.com>
On Sat, Sep 21, 2019 at 8:15 PM Pravin Shelar <pshelar@ovn.org> wrote:
> > > Any suggestions on the way forward? :(
since there is no clear and agreed by everyone path forward the simple
revert is the best option.
Next release cycle you can come up with something that works for everyone.
^ permalink raw reply
* Re: [PATCH net] net: ena: Add dependency for ENA_ETHERNET
From: maowenan @ 2019-09-22 5:08 UTC (permalink / raw)
To: Jakub Kicinski
Cc: netanel, saeedb, zorik, davem, netdev, linux-kernel,
kernel-janitors, Uwe Kleine-König
In-Reply-To: <20190921200741.1c3289e8@cakuba.netronome.com>
On 2019/9/22 11:07, Jakub Kicinski wrote:
> On Fri, 20 Sep 2019 16:44:05 +0800, Mao Wenan wrote:
>> If CONFIG_ENA_ETHERNET=y and CONFIG_DIMLIB=n,
>> below erros can be found:
>> drivers/net/ethernet/amazon/ena/ena_netdev.o: In function `ena_dim_work':
>> ena_netdev.c:(.text+0x21cc): undefined reference to `net_dim_get_rx_moderation'
>> ena_netdev.c:(.text+0x21cc): relocation truncated to
>> fit: R_AARCH64_CALL26 against undefined symbol `net_dim_get_rx_moderation'
>> drivers/net/ethernet/amazon/ena/ena_netdev.o: In function `ena_io_poll':
>> ena_netdev.c:(.text+0x7bd4): undefined reference to `net_dim'
>> ena_netdev.c:(.text+0x7bd4): relocation truncated to fit:
>> R_AARCH64_CALL26 against undefined symbol `net_dim'
>>
>> After commit 282faf61a053 ("net: ena: switch to dim algorithm for rx adaptive
>> interrupt moderation"), it introduces dim algorithm, which configured by CONFIG_DIMLIB.
>>
>> Fixes: 282faf61a053 ("net: ena: switch to dim algorithm for rx adaptive interrupt moderation")
>> Signed-off-by: Mao Wenan <maowenan@huawei.com>
>
> Thank you Mao, shortly after you posted your patch Uwe proposed to make
> DIMLIB a hidden symbol:
>
> https://lore.kernel.org/netdev/a85be675-ce56-f7ba-29e9-b749073aab6c@kleine-koenig.org/T/#t
>
> That patch will likely be applied soon, could you please rework your
> patch to use the "select" keyword instead of "depends". That's what
> other users do.
Ok, I will send v2.
>
>> diff --git a/drivers/net/ethernet/amazon/Kconfig b/drivers/net/ethernet/amazon/Kconfig
>> index 69ca99d..fe46df4 100644
>> --- a/drivers/net/ethernet/amazon/Kconfig
>> +++ b/drivers/net/ethernet/amazon/Kconfig
>> @@ -18,7 +18,7 @@ if NET_VENDOR_AMAZON
>>
>> config ENA_ETHERNET
>> tristate "Elastic Network Adapter (ENA) support"
>> - depends on PCI_MSI && !CPU_BIG_ENDIAN
>> + depends on PCI_MSI && !CPU_BIG_ENDIAN && DIMLIB
>> ---help---
>> This driver supports Elastic Network Adapter (ENA)"
>>
>
>
> .
>
^ permalink raw reply
* [PATCH v2 net] net: ena: Select DIMLIB for ENA_ETHERNET
From: Mao Wenan @ 2019-09-22 5:38 UTC (permalink / raw)
To: netanel, saeedb, zorik, davem
Cc: netdev, linux-kernel, kernel-janitors, Mao Wenan
In-Reply-To: <20190921200741.1c3289e8@cakuba.netronome.com>
If CONFIG_ENA_ETHERNET=y and CONFIG_DIMLIB=n,
below erros can be found:
drivers/net/ethernet/amazon/ena/ena_netdev.o: In function `ena_dim_work':
ena_netdev.c:(.text+0x21cc): undefined reference to `net_dim_get_rx_moderation'
ena_netdev.c:(.text+0x21cc): relocation truncated to
fit: R_AARCH64_CALL26 against undefined symbol `net_dim_get_rx_moderation'
drivers/net/ethernet/amazon/ena/ena_netdev.o: In function `ena_io_poll':
ena_netdev.c:(.text+0x7bd4): undefined reference to `net_dim'
ena_netdev.c:(.text+0x7bd4): relocation truncated to fit:
R_AARCH64_CALL26 against undefined symbol `net_dim'
After commit 282faf61a053 ("net: ena: switch to dim algorithm for rx adaptive
interrupt moderation"), it introduces dim algorithm, which configured by CONFIG_DIMLIB.
So, this patch is to select DIMLIB for ENA_ETHERNET.
Fixes: 282faf61a053 ("net: ena: switch to dim algorithm for rx adaptive interrupt moderation")
Signed-off-by: Mao Wenan <maowenan@huawei.com>
---
v2: change subject of patch, use the "select" keyword instead of "depends".
drivers/net/ethernet/amazon/Kconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/ethernet/amazon/Kconfig b/drivers/net/ethernet/amazon/Kconfig
index 69ca99d8ac26..cca72a75f551 100644
--- a/drivers/net/ethernet/amazon/Kconfig
+++ b/drivers/net/ethernet/amazon/Kconfig
@@ -19,6 +19,7 @@ if NET_VENDOR_AMAZON
config ENA_ETHERNET
tristate "Elastic Network Adapter (ENA) support"
depends on PCI_MSI && !CPU_BIG_ENDIAN
+ select DIMLIB
---help---
This driver supports Elastic Network Adapter (ENA)"
--
2.20.1
^ permalink raw reply related
* Re: [PATCH net] net: openvswitch: fix possible memleak on createvport fails
From: Pravin Shelar @ 2019-09-22 5:50 UTC (permalink / raw)
To: Hillf Danton
Cc: Tonghao Zhang, Greg Rose, Linux Kernel Network Developers,
Taehee Yoo
In-Reply-To: <5d86ef21.1c69fb81.d4519.2861SMTPIN_ADDED_MISSING@mx.google.com>
On Sat, Sep 21, 2019 at 8:48 PM Hillf Danton <hdanton@sina.com> wrote:
>
> On Sun, 9 Sep 2019 11:14 from Pravin Shelar <pshelar@ovn.org>
>
> >
>
> > There is already patch a patch to fix this memory leak.
>
> > https://patchwork.ozlabs.org/patch/1144316/
>
> > Can you or Hillf post it on netdev list?
>
>
>
> Was that posted without netdev Cced?
I do not see your patch on netdev patchwork, repost of the patch would
put it on netdev patchwork.
^ permalink raw reply
* [PATCH net] ppp: Fix memory leak in ppp_write
From: Takeshi Misawa @ 2019-09-22 7:45 UTC (permalink / raw)
To: Guillaume Nault, Paul Mackerras; +Cc: netdev
When ppp is closing, __ppp_xmit_process() failed to enqueue skb
and skb allocated in ppp_write() is leaked.
syzbot reported :
BUG: memory leak
unreferenced object 0xffff88812a17bc00 (size 224):
comm "syz-executor673", pid 6952, jiffies 4294942888 (age 13.040s)
hex dump (first 32 bytes):
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
backtrace:
[<00000000d110fff9>] kmemleak_alloc_recursive include/linux/kmemleak.h:43 [inline]
[<00000000d110fff9>] slab_post_alloc_hook mm/slab.h:522 [inline]
[<00000000d110fff9>] slab_alloc_node mm/slab.c:3262 [inline]
[<00000000d110fff9>] kmem_cache_alloc_node+0x163/0x2f0 mm/slab.c:3574
[<000000002d616113>] __alloc_skb+0x6e/0x210 net/core/skbuff.c:197
[<000000000167fc45>] alloc_skb include/linux/skbuff.h:1055 [inline]
[<000000000167fc45>] ppp_write+0x48/0x120 drivers/net/ppp/ppp_generic.c:502
[<000000009ab42c0b>] __vfs_write+0x43/0xa0 fs/read_write.c:494
[<00000000086b2e22>] vfs_write fs/read_write.c:558 [inline]
[<00000000086b2e22>] vfs_write+0xee/0x210 fs/read_write.c:542
[<00000000a2b70ef9>] ksys_write+0x7c/0x130 fs/read_write.c:611
[<00000000ce5e0fdd>] __do_sys_write fs/read_write.c:623 [inline]
[<00000000ce5e0fdd>] __se_sys_write fs/read_write.c:620 [inline]
[<00000000ce5e0fdd>] __x64_sys_write+0x1e/0x30 fs/read_write.c:620
[<00000000d9d7b370>] do_syscall_64+0x76/0x1a0 arch/x86/entry/common.c:296
[<0000000006e6d506>] entry_SYSCALL_64_after_hwframe+0x44/0xa9
Fix this by freeing skb, if ppp is closing.
Fixes: 6d066734e9f0 ("ppp: avoid loop in xmit recursion detection code")
Reported-and-tested-by: syzbot+d9c8bf24e56416d7ce2c@syzkaller.appspotmail.com
Signed-off-by: Takeshi Misawa <jeliantsurux@gmail.com>
---
Dear Guillaume Nault, Paul Mackerras
syzbot reported memory leak in net/ppp.
- memory leak in ppp_write
I send a patch that passed syzbot reproducer test.
Please consider this memory leak and patch.
Regards.
---
drivers/net/ppp/ppp_generic.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index a30e41a56085..9a1b006904a7 100644
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -1415,6 +1415,8 @@ static void __ppp_xmit_process(struct ppp *ppp, struct sk_buff *skb)
netif_wake_queue(ppp->dev);
else
netif_stop_queue(ppp->dev);
+ } else {
+ kfree_skb(skb);
}
ppp_xmit_unlock(ppp);
}
--
2.17.1
^ permalink raw reply related
* [PATCH 0/4] Attempt to fix regression with AR8035 speed downgrade
From: Russell King - ARM Linux admin @ 2019-09-22 10:59 UTC (permalink / raw)
To: Andrew Lunn, Florian Fainelli, Heiner Kallweit, tinywrkb
Cc: David S. Miller, netdev
Hi,
tinywrkb, please can you test this series to ensure that it fixes
your problem - the previous version has turned out to be a non-starter
as it introduces more problems, thanks!
The following series attempts to address an issue spotted by tinywrkb
with the AR8035 on the Cubox-i2 in a situation where the PHY downgrades
the negotiated link.
Before commit 5502b218e001 ("net: phy: use phy_resolve_aneg_linkmode in
genphy_read_status"), we would read not only the link partner's
advertisement, but also our own advertisement from the PHY registers,
and use both to derive the PHYs current link mode. This works when the
AR8035 downgrades the speed, because it appears that the AR8035 clears
link mode bits in the advertisement registers as part of the downgrade.
Commentary: what is not yet known is whether the AR8035 restores the
advertisement register when the link goes down to the
previous state.
However, since the above referenced commit, we no longer use the PHYs
advertisement registers, instead converting the link partner's
advertisement to the ethtool link mode array, and combine that with
phylib's cached version of our advertisement - which is not updated on
speed downgrade.
This results in phylib disagreeing with the actual operating mode of
the PHY.
Commentary: I wonder how many more PHY drivers are broken by this
commit, but have yet to be discovered.
The obvious way to address this would be to disable the downgrade
feature, and indeed this does fix the problem in tinywrkb's case - his
link partner instead downgrades the speed by reducing its
advertisement, resulting in phylib correctly evaluating a slower speed.
However, it has a serious drawback - the gigabit control register (MII
register 9) appears to become read only. It seems the only way to
update the register is to re-enable the downgrade feature, reset the
PHY, changing register 9, disable the downgrade feature, and reset the
PHY again.
This series attempts to address the problem using a different approach,
similar to the approach taken with Marvell PHYs. The AR8031, AR8033
and AR8035 have a PHY-Specific Status register which reports the
actual operating mode of the PHY - both speed and duplex. This
register correctly reports the operating mode irrespective of whether
autoneg is enabled or not. We use this register to fill in phylib's
speed and duplex parameters.
In detail:
Patch 1 fixes a bug where writing to register 9 does not update
phylib's advertisement mask in the same way that writing register 4
does; this looks like an omission from when gigabit PHY support came
into being.
Patch 2 seperates the generic phylib code which reads the link partners
advertisement from the PHY, so that we can re-use this in the Atheros
PHY driver.
Patch 3 seperates the generic phylib pause mode; phylib provides no
help for MAC drivers to ascertain the negotiated pause mode, it merely
copies the link partner's pause mode bits into its own variables.
Commentary: Both the aforementioned Atheros PHYs and Marvell PHYs
provide the resolved pause modes in terms of whether
we should transmit pause frames, or whether we should
allow reception of pause frames. Surely the resolution
of this should be in phylib?
Patch 4 provides the Atheros PHY driver with a private "read_status"
implementation that fills in phylib's speed and duplex settings
depending on the PHY-Specific status register. This ensures that
phylib and the MAC driver match the operating mode that the PHY has
decided to use. Since the register also gives us MDIX status, we
can trivially fill that status in as well.
Note that, although the bits mentioned in this patch for this register
match those in th Marvell PHY driver, and it is located at the same
address, the meaning of other register bits varies between the PHYs.
Therefore, I do not feel that it would be appropriate to make this some
kind of generic function.
drivers/net/phy/at803x.c | 69 ++++++++++++++++++++++++++++++++++++++++++++
drivers/net/phy/phy-core.c | 20 ++++++++-----
drivers/net/phy/phy.c | 5 ++++
drivers/net/phy/phy_device.c | 65 +++++++++++++++++++++++++----------------
include/linux/mii.h | 9 ++++++
include/linux/phy.h | 2 ++
6 files changed, 138 insertions(+), 32 deletions(-)
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
^ permalink raw reply
* [PATCH 1/4] net: phy: fix write to mii-ctrl1000 register
From: Russell King @ 2019-09-22 11:00 UTC (permalink / raw)
To: Andrew Lunn, Florian Fainelli, Heiner Kallweit, tinywrkb
Cc: David S. Miller, netdev
In-Reply-To: <20190922105932.GP25745@shell.armlinux.org.uk>
When userspace writes to the MII_ADVERTISE register, we update phylib's
advertising mask and trigger a renegotiation. However, writing to the
MII_CTRL1000 register, which contains the gigabit advertisement, does
neither. This can lead to phylib's copy of the advertisement becoming
de-synced with the values in the PHY register set, which can result in
incorrect negotiation resolution.
Fixes: 5502b218e001 ("net: phy: use phy_resolve_aneg_linkmode in genphy_read_status")
Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
drivers/net/phy/phy.c | 5 +++++
include/linux/mii.h | 9 +++++++++
2 files changed, 14 insertions(+)
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 7af390d8bbdb..068a08a50064 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -458,6 +458,11 @@ int phy_mii_ioctl(struct phy_device *phydev, struct ifreq *ifr, int cmd)
val);
change_autoneg = true;
break;
+ case MII_CTRL1000:
+ mii_ctrl1000_mod_linkmode_adv_t(phydev->advertising,
+ val);
+ change_autoneg = true;
+ break;
default:
/* do nothing */
break;
diff --git a/include/linux/mii.h b/include/linux/mii.h
index 5cd824c1c0ca..4ce8901a1af6 100644
--- a/include/linux/mii.h
+++ b/include/linux/mii.h
@@ -455,6 +455,15 @@ static inline void mii_lpa_mod_linkmode_lpa_t(unsigned long *lp_advertising,
lp_advertising, lpa & LPA_LPACK);
}
+static inline void mii_ctrl1000_mod_linkmode_adv_t(unsigned long *advertising,
+ u32 ctrl1000)
+{
+ linkmode_mod_bit(ETHTOOL_LINK_MODE_1000baseT_Half_BIT, advertising,
+ ctrl1000 & ADVERTISE_1000HALF);
+ linkmode_mod_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT, advertising,
+ ctrl1000 & ADVERTISE_1000FULL);
+}
+
/**
* linkmode_adv_to_lcl_adv_t
* @advertising:pointer to linkmode advertising
--
2.7.4
^ permalink raw reply related
* [PATCH 2/4] net: phy: extract link partner advertisement reading
From: Russell King @ 2019-09-22 11:00 UTC (permalink / raw)
To: Andrew Lunn, Florian Fainelli, Heiner Kallweit, tinywrkb
Cc: David S. Miller, netdev
In-Reply-To: <20190922105932.GP25745@shell.armlinux.org.uk>
Move reading the link partner advertisement out of genphy_read_status()
into its own separate function. This will allow re-use of this code by
PHY drivers that are able to read the resolved status from the PHY.
Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
drivers/net/phy/phy_device.c | 65 +++++++++++++++++++++++++++-----------------
include/linux/phy.h | 1 +
2 files changed, 41 insertions(+), 25 deletions(-)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 2c506c3c6c7b..d17aafcde5a1 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1782,32 +1782,9 @@ int genphy_update_link(struct phy_device *phydev)
}
EXPORT_SYMBOL(genphy_update_link);
-/**
- * genphy_read_status - check the link status and update current link state
- * @phydev: target phy_device struct
- *
- * Description: Check the link, then figure out the current state
- * by comparing what we advertise with what the link partner
- * advertises. Start by checking the gigabit possibilities,
- * then move on to 10/100.
- */
-int genphy_read_status(struct phy_device *phydev)
+int genphy_read_lpa(struct phy_device *phydev)
{
- int adv, lpa, lpagb, err, old_link = phydev->link;
-
- /* Update the link, but return if there was an error */
- err = genphy_update_link(phydev);
- if (err)
- return err;
-
- /* why bother the PHY if nothing can have changed */
- if (phydev->autoneg == AUTONEG_ENABLE && old_link && phydev->link)
- return 0;
-
- phydev->speed = SPEED_UNKNOWN;
- phydev->duplex = DUPLEX_UNKNOWN;
- phydev->pause = 0;
- phydev->asym_pause = 0;
+ int adv, lpa, lpagb;
linkmode_zero(phydev->lp_advertising);
@@ -1838,6 +1815,44 @@ int genphy_read_status(struct phy_device *phydev)
return lpa;
mii_lpa_mod_linkmode_lpa_t(phydev->lp_advertising, lpa);
+ }
+
+ return 0;
+}
+EXPORT_SYMBOL(genphy_read_lpa);
+
+/**
+ * genphy_read_status - check the link status and update current link state
+ * @phydev: target phy_device struct
+ *
+ * Description: Check the link, then figure out the current state
+ * by comparing what we advertise with what the link partner
+ * advertises. Start by checking the gigabit possibilities,
+ * then move on to 10/100.
+ */
+int genphy_read_status(struct phy_device *phydev)
+{
+ int err, old_link = phydev->link;
+
+ /* Update the link, but return if there was an error */
+ err = genphy_update_link(phydev);
+ if (err)
+ return err;
+
+ /* why bother the PHY if nothing can have changed */
+ if (phydev->autoneg == AUTONEG_ENABLE && old_link && phydev->link)
+ return 0;
+
+ phydev->speed = SPEED_UNKNOWN;
+ phydev->duplex = DUPLEX_UNKNOWN;
+ phydev->pause = 0;
+ phydev->asym_pause = 0;
+
+ err = genphy_read_lpa(phydev);
+ if (err < 0)
+ return err;
+
+ if (phydev->autoneg == AUTONEG_ENABLE && phydev->autoneg_complete) {
phy_resolve_aneg_linkmode(phydev);
} else if (phydev->autoneg == AUTONEG_DISABLE) {
int bmcr = phy_read(phydev, MII_BMCR);
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 0600754ce5e7..bef7f30af22d 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1085,6 +1085,7 @@ int genphy_config_eee_advert(struct phy_device *phydev);
int genphy_config_aneg(struct phy_device *phydev);
int genphy_aneg_done(struct phy_device *phydev);
int genphy_update_link(struct phy_device *phydev);
+int genphy_read_lpa(struct phy_device *phydev);
int genphy_read_status(struct phy_device *phydev);
int genphy_suspend(struct phy_device *phydev);
int genphy_resume(struct phy_device *phydev);
--
2.7.4
^ permalink raw reply related
* [PATCH 4/4] net: phy: at803x: use operating parameters from PHY-specific status
From: Russell King @ 2019-09-22 11:00 UTC (permalink / raw)
To: Andrew Lunn, Florian Fainelli, Heiner Kallweit, tinywrkb
Cc: David S. Miller, netdev
In-Reply-To: <20190922105932.GP25745@shell.armlinux.org.uk>
Read the PHY-specific status register for the current operating mode
(speed and duplex) of the PHY. This register reflects the actual
mode that the PHY has resolved depending on either the advertisements
of autoneg is enabled, or the forced mode if autoneg is disabled.
This ensures that phylib's software state always tracks the hardware
state.
It seems both AR8033 (which uses the AR8031 ID) and AR8035 support
this status register. AR8030 is not known at the present time.
Reported-by: tinywrkb <tinywrkb@gmail.com>
Fixes: 5502b218e001 ("net: phy: use phy_resolve_aneg_linkmode in genphy_read_status")
Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
drivers/net/phy/at803x.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 69 insertions(+)
diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index b3893347804d..7dca170fdd80 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -16,6 +16,15 @@
#include <linux/of_gpio.h>
#include <linux/gpio/consumer.h>
+#define AT803X_SPECIFIC_STATUS 0x11
+#define AT803X_SS_SPEED_MASK (3 << 14)
+#define AT803X_SS_SPEED_1000 (2 << 14)
+#define AT803X_SS_SPEED_100 (1 << 14)
+#define AT803X_SS_SPEED_10 (0 << 14)
+#define AT803X_SS_DUPLEX BIT(13)
+#define AT803X_SS_SPEED_DUPLEX_RESOLVED BIT(11)
+#define AT803X_SS_MDIX BIT(6)
+
#define AT803X_INTR_ENABLE 0x12
#define AT803X_INTR_ENABLE_AUTONEG_ERR BIT(15)
#define AT803X_INTR_ENABLE_SPEED_CHANGED BIT(14)
@@ -404,6 +413,64 @@ static int at803x_aneg_done(struct phy_device *phydev)
return aneg_done;
}
+static int at803x_read_status(struct phy_device *phydev)
+{
+ int ss, err, old_link = phydev->link;
+
+ /* Update the link, but return if there was an error */
+ err = genphy_update_link(phydev);
+ if (err)
+ return err;
+
+ /* why bother the PHY if nothing can have changed */
+ if (phydev->autoneg == AUTONEG_ENABLE && old_link && phydev->link)
+ return 0;
+
+ phydev->speed = SPEED_UNKNOWN;
+ phydev->duplex = DUPLEX_UNKNOWN;
+ phydev->pause = 0;
+ phydev->asym_pause = 0;
+
+ err = genphy_read_lpa(phydev);
+ if (err < 0)
+ return err;
+
+ /* Read the AT8035 PHY-Specific Status register, which indicates the
+ * speed and duplex that the PHY is actually using, irrespective of
+ * whether we are in autoneg mode or not.
+ */
+ ss = phy_read(phydev, AT803X_SPECIFIC_STATUS);
+ if (ss < 0)
+ return ss;
+
+ if (ss & AT803X_SS_SPEED_DUPLEX_RESOLVED) {
+ switch (ss & AT803X_SS_SPEED_MASK) {
+ case AT803X_SS_SPEED_10:
+ phydev->speed = SPEED_10;
+ break;
+ case AT803X_SS_SPEED_100:
+ phydev->speed = SPEED_100;
+ break;
+ case AT803X_SS_SPEED_1000:
+ phydev->speed = SPEED_1000;
+ break;
+ }
+ if (ss & AT803X_SS_DUPLEX)
+ phydev->duplex = DUPLEX_FULL;
+ else
+ phydev->duplex = DUPLEX_HALF;
+ if (ss & AT803X_SS_MDIX)
+ phydev->mdix = ETH_TP_MDI_X;
+ else
+ phydev->mdix = ETH_TP_MDI;
+ }
+
+ if (phydev->autoneg == AUTONEG_ENABLE && phydev->autoneg_complete)
+ phy_resolve_aneg_pause(phydev);
+
+ return 0;
+}
+
static struct phy_driver at803x_driver[] = {
{
/* ATHEROS 8035 */
@@ -417,6 +484,7 @@ static struct phy_driver at803x_driver[] = {
.suspend = at803x_suspend,
.resume = at803x_resume,
/* PHY_GBIT_FEATURES */
+ .read_status = at803x_read_status,
.ack_interrupt = at803x_ack_interrupt,
.config_intr = at803x_config_intr,
.get_eee = at8035_get_eee,
@@ -447,6 +515,7 @@ static struct phy_driver at803x_driver[] = {
.suspend = at803x_suspend,
.resume = at803x_resume,
/* PHY_GBIT_FEATURES */
+ .read_status = at803x_read_status,
.aneg_done = at803x_aneg_done,
.ack_interrupt = &at803x_ack_interrupt,
.config_intr = &at803x_config_intr,
--
2.7.4
^ permalink raw reply related
* [PATCH 3/4] net: phy: extract pause mode
From: Russell King @ 2019-09-22 11:00 UTC (permalink / raw)
To: Andrew Lunn, Florian Fainelli, Heiner Kallweit, tinywrkb
Cc: David S. Miller, netdev
In-Reply-To: <20190922105932.GP25745@shell.armlinux.org.uk>
Extract the update of phylib's software pause mode state from
genphy_read_status(), so that we can re-use this functionality with
PHYs that have alternative ways to read the negotiation results.
Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
drivers/net/phy/phy-core.c | 20 +++++++++++++-------
include/linux/phy.h | 1 +
2 files changed, 14 insertions(+), 7 deletions(-)
diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c
index 16667fbac8bf..91b856b31fd6 100644
--- a/drivers/net/phy/phy-core.c
+++ b/drivers/net/phy/phy-core.c
@@ -278,6 +278,18 @@ void of_set_phy_eee_broken(struct phy_device *phydev)
phydev->eee_broken_modes = broken;
}
+void phy_resolve_aneg_pause(struct phy_device *phydev)
+{
+ if (phydev->duplex == DUPLEX_FULL) {
+ phydev->pause = linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT,
+ phydev->lp_advertising);
+ phydev->asym_pause = linkmode_test_bit(
+ ETHTOOL_LINK_MODE_Asym_Pause_BIT,
+ phydev->lp_advertising);
+ }
+}
+EXPORT_SYMBOL_GPL(phy_resolve_aneg_pause);
+
/**
* phy_resolve_aneg_linkmode - resolve the advertisements into phy settings
* @phydev: The phy_device struct
@@ -300,13 +312,7 @@ void phy_resolve_aneg_linkmode(struct phy_device *phydev)
break;
}
- if (phydev->duplex == DUPLEX_FULL) {
- phydev->pause = linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT,
- phydev->lp_advertising);
- phydev->asym_pause = linkmode_test_bit(
- ETHTOOL_LINK_MODE_Asym_Pause_BIT,
- phydev->lp_advertising);
- }
+ phy_resolve_aneg_pause(phydev);
}
EXPORT_SYMBOL_GPL(phy_resolve_aneg_linkmode);
diff --git a/include/linux/phy.h b/include/linux/phy.h
index bef7f30af22d..224e81740963 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -686,6 +686,7 @@ static inline bool phy_is_started(struct phy_device *phydev)
return phydev->state >= PHY_UP;
}
+void phy_resolve_aneg_pause(struct phy_device *phydev);
void phy_resolve_aneg_linkmode(struct phy_device *phydev);
/**
--
2.7.4
^ permalink raw reply related
* [PATCH] atm: he: clean up an indentation issue
From: Colin King @ 2019-09-22 11:42 UTC (permalink / raw)
To: Chas Williams, linux-atm-general, netdev; +Cc: kernel-janitors, linux-kernel
From: Colin Ian King <colin.king@canonical.com>
There is a statement that is indented one level too many, remove
the extraneous tab.
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
drivers/atm/he.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/atm/he.c b/drivers/atm/he.c
index 70b00ae4ec38..8af793f5e811 100644
--- a/drivers/atm/he.c
+++ b/drivers/atm/he.c
@@ -1690,7 +1690,7 @@ he_service_rbrq(struct he_dev *he_dev, int group)
if (RBRQ_HBUF_ERR(he_dev->rbrq_head)) {
hprintk("HBUF_ERR! (cid 0x%x)\n", cid);
- atomic_inc(&vcc->stats->rx_drop);
+ atomic_inc(&vcc->stats->rx_drop);
goto return_host_buffers;
}
--
2.20.1
^ permalink raw reply related
* Re: CONFIG_NET_TC_SKB_EXT
From: Paul Blakey @ 2019-09-22 11:51 UTC (permalink / raw)
To: Pravin Shelar, Daniel B^Ckmann, Jakub Kicinski, Vlad Buslov,
David Miller, netdev@vger.kernel.org, Jiri Pirko, Cong Wang,
Jamal Hadi Salim, Simon Horman, Or Gerlitz
Cc: Paul Blakey
In-Reply-To: <CAADnVQJBxsWU8BddxWDBX==y87ZLoEsBdqq0DqhYD7NyEcDLzg@mail.gmail.com>
The skb extension is currently used for miss path of software offloading OvS rules with recirculation to tc.
However, we are also preparing patches to support the hardware side of things.
After the userspace OvS patches to support connection tracking, we'll have two users for
tc multi chain rules, one from those actually using tc and those translated from OvS rules.
With both of these use cases, there is similar issue of 'miss', from hardware to tc and tc to OvS and the skb
extension will serve to recover from both.
Consider, for example, the following multi chain tc rules:
tc filter add dev1 ... chain 0 flower dst_mac aa:bb:cc:dd:ee:ff action pedit munge ex set mac dst 02:a0:98:4e:8f:d1 pipe action goto chain 1
tc filter add dev1 ... chain 1 flower ip_dst 1.1.1.1 action mirred egress redirect dev2
tc filter add dev1 ... chain 1 flower ip_dst 2.2.2.2 action mirred egress redirect dev2
It's possible we offload the first two rules, but fail to offload the third rule, because of some hardware failure (e.g unsupported match or action).
If a packet with (dst_mac=aa:bb:cc:dd:ee:ff) and (dst ip=2.2.2.2) arrives,
we execute the goto chain 1 in hardware (and the pedit), and continue in chain 1, where we miss.
Currently we re-start the processing in tc from chain 0, even though we already did part of the processing in hardware.
The match on the dst_mac will fail as we already modified it, and we won't execute the third rule action.
In addition, if we did manage to execute any software tc rules, the packet will be counted twice.
We'll re-use this extension to solve this issue that currently exists by letting drivers tell tc on which chain to start the classification.
Regarding the config, we suggest changing the default to N and letting users decide to enable it, see attached patch.
Thanks,
Paul.
------------------------------------------------------------
Subject: [PATCH net-next] net/sched: Set default of CONFIG_NET_TC_SKB_EXT to N
This a new feature, it is prefered that it defaults to N.
Fixes: 95a7233c452a ('net: openvswitch: Set OvS recirc_id from tc chain index')
Signed-off-by: Paul Blakey <paulb@mellanox.com>
---
net/sched/Kconfig | 1 -
1 file changed, 1 deletion(-)
diff --git a/net/sched/Kconfig b/net/sched/Kconfig
index b3faafe..4bb10b7 100644
--- a/net/sched/Kconfig
+++ b/net/sched/Kconfig
@@ -966,7 +966,6 @@ config NET_IFE_SKBTCINDEX
config NET_TC_SKB_EXT
bool "TC recirculation support"
depends on NET_CLS_ACT
- default y if NET_CLS_ACT
select SKB_EXTENSIONS
help
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH net v3 10/11] vxlan: add adjacent link to limit depth level
From: Taehee Yoo @ 2019-09-22 12:35 UTC (permalink / raw)
To: Jakub Kicinski
Cc: David Miller, Netdev, j.vosburgh, vfalico, Andy Gospodarek,
Jiří Pírko, sd, Roopa Prabhu, saeedm, manishc,
rahulv, kys, haiyangz, Stephen Hemminger, sashal, hare, varun,
ubraun, kgraul, Jay Vosburgh
In-Reply-To: <20190920164816.55a77053@cakuba.netronome.com>
On Sat, 21 Sep 2019 at 08:48, Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
>
Hi Jakub,
Thank you so much for your detailed review!
> On Mon, 16 Sep 2019 22:48:01 +0900, Taehee Yoo wrote:
> > Current vxlan code doesn't limit the number of nested devices.
> > Nested devices would be handled recursively and this routine needs
> > huge stack memory. So, unlimited nested devices could make
> > stack overflow.
> >
> > In order to fix this issue, this patch adds adjacent links.
> > The adjacent link APIs internally check the depth level.
>
> > Fixes: acaf4e70997f ("net: vxlan: when lower dev unregisters remove vxlan dev as well")
> > Signed-off-by: Taehee Yoo <ap420073@gmail.com>
>
> Minor nit picks here, I hope you don't mind.
>
> > diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
> > index 3d9bcc957f7d..0d5c8d22d8a4 100644
> > --- a/drivers/net/vxlan.c
> > +++ b/drivers/net/vxlan.c
> > @@ -3567,6 +3567,8 @@ static int __vxlan_dev_create(struct net *net, struct net_device *dev,
> > struct vxlan_net *vn = net_generic(net, vxlan_net_id);
> > struct vxlan_dev *vxlan = netdev_priv(dev);
> > struct vxlan_fdb *f = NULL;
> > + struct net_device *remote_dev = NULL;
> > + struct vxlan_rdst *dst = &vxlan->default_dst;
>
> Especially in places where reverse christmas tree variable ordering is
> adhered to, could you please preserve it? That'd mean:
>
> struct vxlan_net *vn = net_generic(net, vxlan_net_id);
> struct vxlan_dev *vxlan = netdev_priv(dev);
> struct net_device *remote_dev = NULL;
> struct vxlan_fdb *f = NULL;
> bool unregister = false;
> struct vxlan_rdst *dst;
> int err;
>
> dst = &vxlan->default_dst;
>
> here.
>
> > bool unregister = false;
> > int err;
> >
> > @@ -3577,14 +3579,14 @@ static int __vxlan_dev_create(struct net *net, struct net_device *dev,
> > dev->ethtool_ops = &vxlan_ethtool_ops;
> >
> > /* create an fdb entry for a valid default destination */
> > - if (!vxlan_addr_any(&vxlan->default_dst.remote_ip)) {
> > + if (!vxlan_addr_any(&dst->remote_ip)) {
> > err = vxlan_fdb_create(vxlan, all_zeros_mac,
> > - &vxlan->default_dst.remote_ip,
> > + &dst->remote_ip,
> > NUD_REACHABLE | NUD_PERMANENT,
> > vxlan->cfg.dst_port,
> > - vxlan->default_dst.remote_vni,
> > - vxlan->default_dst.remote_vni,
> > - vxlan->default_dst.remote_ifindex,
> > + dst->remote_vni,
> > + dst->remote_vni,
> > + dst->remote_ifindex,
> > NTF_SELF, &f);
> > if (err)
> > return err;
> > @@ -3595,26 +3597,43 @@ static int __vxlan_dev_create(struct net *net, struct net_device *dev,
> > goto errout;
> > unregister = true;
> >
> > + if (dst->remote_ifindex) {
> > + remote_dev = __dev_get_by_index(net, dst->remote_ifindex);
> > + if (!remote_dev)
> > + goto errout;
> > +
> > + err = netdev_upper_dev_link(remote_dev, dev, extack);
> > + if (err)
> > + goto errout;
> > + }
> > +
> > err = rtnl_configure_link(dev, NULL);
> > if (err)
> > - goto errout;
> > + goto unlink;
> >
> > if (f) {
> > - vxlan_fdb_insert(vxlan, all_zeros_mac,
> > - vxlan->default_dst.remote_vni, f);
> > + vxlan_fdb_insert(vxlan, all_zeros_mac, dst->remote_vni, f);
> >
> > /* notify default fdb entry */
> > err = vxlan_fdb_notify(vxlan, f, first_remote_rtnl(f),
> > RTM_NEWNEIGH, true, extack);
> > if (err) {
> > vxlan_fdb_destroy(vxlan, f, false, false);
> > + if (remote_dev)
> > + netdev_upper_dev_unlink(remote_dev, dev);
> > goto unregister;
> > }
> > }
> >
> > list_add(&vxlan->next, &vn->vxlan_list);
> > + if (remote_dev) {
> > + dst->remote_dev = remote_dev;
> > + dev_hold(remote_dev);
> > + }
> > return 0;
> > -
> > +unlink:
> > + if (remote_dev)
> > + netdev_upper_dev_unlink(remote_dev, dev);
> > errout:
> > /* unregister_netdevice() destroys the default FDB entry with deletion
> > * notification. But the addition notification was not sent yet, so
> > @@ -3936,6 +3955,8 @@ static int vxlan_changelink(struct net_device *dev, struct nlattr *tb[],
> > struct net_device *lowerdev;
> > struct vxlan_config conf;
> > int err;
> > + bool linked = false;
> > + bool disabled = false;
>
> Same here.
>
> > err = vxlan_nl2conf(tb, data, dev, &conf, true, extack);
> > if (err)
> > @@ -3946,6 +3967,16 @@ static int vxlan_changelink(struct net_device *dev, struct nlattr *tb[],
> > if (err)
> > return err;
> >
> > + if (lowerdev) {
> > + if (dst->remote_dev && lowerdev != dst->remote_dev) {
> > + netdev_adjacent_dev_disable(dst->remote_dev, dev);
> > + disabled = true;
> > + }
> > + err = netdev_upper_dev_link(lowerdev, dev, extack);
> > + if (err)
> > + goto err;
>
> would you mind naming the label errout? there is an err variable, and
> other places in this file use errout
>
> > + linked = true;
> > + }
> > /* handle default dst entry */
> > if (!vxlan_addr_equal(&conf.remote_ip, &dst->remote_ip)) {
> > u32 hash_index = fdb_head_index(vxlan, all_zeros_mac, conf.vni);
> > @@ -3962,7 +3993,7 @@ static int vxlan_changelink(struct net_device *dev, struct nlattr *tb[],
> > NTF_SELF, true, extack);
> > if (err) {
> > spin_unlock_bh(&vxlan->hash_lock[hash_index]);
> > - return err;
> > + goto err;
> > }
> > }
> > if (!vxlan_addr_any(&dst->remote_ip))
> > @@ -3979,8 +4010,24 @@ static int vxlan_changelink(struct net_device *dev, struct nlattr *tb[],
> > if (conf.age_interval != vxlan->cfg.age_interval)
> > mod_timer(&vxlan->age_timer, jiffies);
> >
> > + if (disabled) {
> > + netdev_adjacent_dev_enable(dst->remote_dev, dev);
> > + netdev_upper_dev_unlink(dst->remote_dev, dev);
> > + dev_put(dst->remote_dev);
> > + }
> > + if (linked) {
> > + dst->remote_dev = lowerdev;
> > + dev_hold(dst->remote_dev);
> > + }
> > +
> > vxlan_config_apply(dev, &conf, lowerdev, vxlan->net, true);
> > return 0;
> > +err:
> > + if (linked)
> > + netdev_upper_dev_unlink(lowerdev, dev);
> > + if (disabled)
> > + netdev_adjacent_dev_enable(dst->remote_dev, dev);
> > + return err;
> > }
> >
> > static void vxlan_dellink(struct net_device *dev, struct list_head *head)
> > @@ -3991,6 +4038,10 @@ static void vxlan_dellink(struct net_device *dev, struct list_head *head)
> >
> > list_del(&vxlan->next);
> > unregister_netdevice_queue(dev, head);
> > + if (vxlan->default_dst.remote_dev) {
> > + netdev_upper_dev_unlink(vxlan->default_dst.remote_dev, dev);
> > + dev_put(vxlan->default_dst.remote_dev);
> > + }
> > }
> >
> > static size_t vxlan_get_size(const struct net_device *dev)
> > diff --git a/include/net/vxlan.h b/include/net/vxlan.h
> > index dc1583a1fb8a..08e237d7aa73 100644
> > --- a/include/net/vxlan.h
> > +++ b/include/net/vxlan.h
> > @@ -197,6 +197,7 @@ struct vxlan_rdst {
> > u8 offloaded:1;
> > __be32 remote_vni;
> > u32 remote_ifindex;
> > + struct net_device *remote_dev;
> > struct list_head list;
> > struct rcu_head rcu;
> > struct dst_cache dst_cache;
>
Thank you for letting me know about "reverse christmas tree variable ordering".
I will send a v4 patch that will include things that you mentioned.
Thank you!
^ permalink raw reply
* Re: [PATCH net-next] tuntap: Fallback to automq on TUNSETSTEERINGEBPF prog negative return
From: Michael S. Tsirkin @ 2019-09-22 12:37 UTC (permalink / raw)
To: Matthew Cover
Cc: davem, ast, daniel, kafai, songliubraving, yhs, jasowang,
edumazet, sdf, matthew.cover, mail, pabeni, nicolas.dichtel,
wangli39, lifei.shirley, tglx, netdev, linux-kernel, bpf
In-Reply-To: <20190920185843.4096-1-matthew.cover@stackpath.com>
On Fri, Sep 20, 2019 at 11:58:43AM -0700, Matthew Cover wrote:
> Treat a negative return from a TUNSETSTEERINGEBPF bpf prog as a signal
> to fallback to tun_automq_select_queue() for tx queue selection.
>
> Compilation of this exact patch was tested.
>
> For functional testing 3 additional printk()s were added.
>
> Functional testing results (on 2 txq tap device):
>
> [Fri Sep 20 18:33:27 2019] ========== tun no prog ==========
> [Fri Sep 20 18:33:27 2019] tuntap: tun_ebpf_select_queue() returned '-1'
> [Fri Sep 20 18:33:27 2019] tuntap: tun_automq_select_queue() ran
> [Fri Sep 20 18:33:27 2019] ========== tun prog -1 ==========
> [Fri Sep 20 18:33:27 2019] tuntap: bpf_prog_run_clear_cb() returned '-1'
> [Fri Sep 20 18:33:27 2019] tuntap: tun_ebpf_select_queue() returned '-1'
> [Fri Sep 20 18:33:27 2019] tuntap: tun_automq_select_queue() ran
> [Fri Sep 20 18:33:27 2019] ========== tun prog 0 ==========
> [Fri Sep 20 18:33:27 2019] tuntap: bpf_prog_run_clear_cb() returned '0'
> [Fri Sep 20 18:33:27 2019] tuntap: tun_ebpf_select_queue() returned '0'
> [Fri Sep 20 18:33:27 2019] ========== tun prog 1 ==========
> [Fri Sep 20 18:33:27 2019] tuntap: bpf_prog_run_clear_cb() returned '1'
> [Fri Sep 20 18:33:27 2019] tuntap: tun_ebpf_select_queue() returned '1'
> [Fri Sep 20 18:33:27 2019] ========== tun prog 2 ==========
> [Fri Sep 20 18:33:27 2019] tuntap: bpf_prog_run_clear_cb() returned '2'
> [Fri Sep 20 18:33:27 2019] tuntap: tun_ebpf_select_queue() returned '0'
>
> Signed-off-by: Matthew Cover <matthew.cover@stackpath.com>
Could you add a bit more motivation data here?
1. why is this a good idea
2. how do we know existing userspace does not rely on existing behaviour
3. why doesn't userspace need a way to figure out whether it runs on a kernel with and
without this patch
thanks,
MST
> ---
> drivers/net/tun.c | 20 +++++++++++---------
> 1 file changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index aab0be4..173d159 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -583,35 +583,37 @@ static u16 tun_automq_select_queue(struct tun_struct *tun, struct sk_buff *skb)
> return txq;
> }
>
> -static u16 tun_ebpf_select_queue(struct tun_struct *tun, struct sk_buff *skb)
> +static int tun_ebpf_select_queue(struct tun_struct *tun, struct sk_buff *skb)
> {
> struct tun_prog *prog;
> u32 numqueues;
> - u16 ret = 0;
> + int ret = -1;
>
> numqueues = READ_ONCE(tun->numqueues);
> if (!numqueues)
> return 0;
>
> + rcu_read_lock();
> prog = rcu_dereference(tun->steering_prog);
> if (prog)
> ret = bpf_prog_run_clear_cb(prog->prog, skb);
> + rcu_read_unlock();
>
> - return ret % numqueues;
> + if (ret >= 0)
> + ret %= numqueues;
> +
> + return ret;
> }
>
> static u16 tun_select_queue(struct net_device *dev, struct sk_buff *skb,
> struct net_device *sb_dev)
> {
> struct tun_struct *tun = netdev_priv(dev);
> - u16 ret;
> + int ret;
>
> - rcu_read_lock();
> - if (rcu_dereference(tun->steering_prog))
> - ret = tun_ebpf_select_queue(tun, skb);
> - else
> + ret = tun_ebpf_select_queue(tun, skb);
> + if (ret < 0)
> ret = tun_automq_select_queue(tun, skb);
> - rcu_read_unlock();
>
> return ret;
> }
> --
> 1.8.3.1
^ permalink raw reply
* Re: [PATCH net v3 09/11] net: core: add ignore flag to netdev_adjacent structure
From: Taehee Yoo @ 2019-09-22 12:54 UTC (permalink / raw)
To: Jakub Kicinski
Cc: David Miller, Netdev, j.vosburgh, vfalico, Andy Gospodarek,
Jiří Pírko, sd, Roopa Prabhu, saeedm, manishc,
rahulv, kys, haiyangz, Stephen Hemminger, sashal, hare, varun,
ubraun, kgraul, Jay Vosburgh
In-Reply-To: <20190920165504.2ed552ac@cakuba.netronome.com>
On Sat, 21 Sep 2019 at 08:55, Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
>
> On Mon, 16 Sep 2019 22:48:00 +0900, Taehee Yoo wrote:
> > In order to link an adjacent node, netdev_upper_dev_link() is used
> > and in order to unlink an adjacent node, netdev_upper_dev_unlink() is used.
> > unlink operation does not fail, but link operation can fail.
> >
> > In order to exchange adjacent nodes, we should unlink an old adjacent
> > node first. then, link a new adjacent node.
> > If link operation is failed, we should link an old adjacent node again.
> > But this link operation can fail too.
> > It eventually breaks the adjacent link relationship.
> >
> > This patch adds an ignore flag into the netdev_adjacent structure.
> > If this flag is set, netdev_upper_dev_link() ignores an old adjacent
> > node for a moment.
> > So we can skip unlink operation before link operation.
> >
> > Signed-off-by: Taehee Yoo <ap420073@gmail.com>
>
> Could this perhaps be achieved by creating prepare, commit, and abort
> helpers? That would make the API look slightly more canonical.
>
> netdev_adjacent_change_prepare(old, new, dev)
> netdev_adjacent_change_commit(old, new, dev)
> netdev_adjacent_change_abort(old, new, dev)
>
> The current naming makes the operation a little harder to follow if one
> is just reading the vxlan code.
>
I fully agree with your opinion.
I will provide these three functions that you mentioned.
netdev_adjacent_change_prepare
netdev_adjacent_change_commit
netdev_adjacent_change_abort
> Please let me know if I didn't read the code closely enough to
> understand why that's not fitting here.
>
> > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> > index 5bb5756129af..4506810c301b 100644
> > --- a/include/linux/netdevice.h
> > +++ b/include/linux/netdevice.h
> > @@ -4319,6 +4319,10 @@ int netdev_master_upper_dev_link(struct net_device *dev,
> > struct netlink_ext_ack *extack);
> > void netdev_upper_dev_unlink(struct net_device *dev,
> > struct net_device *upper_dev);
> > +void netdev_adjacent_dev_disable(struct net_device *upper_dev,
> > + struct net_device *lower_dev);
> > +void netdev_adjacent_dev_enable(struct net_device *upper_dev,
> > + struct net_device *lower_dev);
> > void netdev_adjacent_rename_links(struct net_device *dev, char *oldname);
> > void *netdev_lower_dev_get_private(struct net_device *dev,
> > struct net_device *lower_dev);
I found that I missed static keyword.
So I will fix this too.
Thank you!
^ permalink raw reply
* Re: [PATCH 0/4] Attempt to fix regression with AR8035 speed downgrade
From: Russell King - ARM Linux admin @ 2019-09-22 12:56 UTC (permalink / raw)
To: Andrew Lunn, Florian Fainelli, Heiner Kallweit, tinywrkb
Cc: David S. Miller, netdev
In-Reply-To: <20190922105932.GP25745@shell.armlinux.org.uk>
On Sun, Sep 22, 2019 at 11:59:32AM +0100, Russell King - ARM Linux admin wrote:
> Commentary: what is not yet known is whether the AR8035 restores the
> advertisement register when the link goes down to the
> previous state.
I've just found my 2-pair cable, and it appears that the AR8035 does
indeed restore the advertisement register.
Using the 2-pair cable (which takes a while for the link to come up
at 100mbit) results in register 9 becoming zero:
1140 796d 004d d072 15e1 c1e1 000d 2001
0000 0000 0800 0000 0000 4007 b29a a000
0862 7c30 0000 dc20 082c 0000 04e8 0000
3200 3000 0000 060d 0000 0005 2d47 8100.
Disconnecting the 2-pair, and connecting a 4-pair cable results in a
gigabit link with the advertisement restored:
1140 796d 004d d072 15e1 c1e1 000d 2001
0000 0200 2800 0000 0000 4007 b29a a000
0862 bc50 0000 dc00 082c 0000 04e8 0000
3200 3000 0000 060e 0000 0005 2d47 8100.
Note that register 0x11 bit 5 reports when downshift is active.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
^ permalink raw reply
* Re: [PATCH net v3 00/11] net: fix nested device bugs
From: Taehee Yoo @ 2019-09-22 14:36 UTC (permalink / raw)
To: Jakub Kicinski
Cc: David Miller, Netdev, j.vosburgh, vfalico, Andy Gospodarek,
Jiří Pírko, sd, Roopa Prabhu, saeedm, manishc,
rahulv, kys, haiyangz, Stephen Hemminger, sashal, hare, varun,
ubraun, kgraul, Jay Vosburgh
In-Reply-To: <20190920165942.7e0d6235@cakuba.netronome.com>
On Sat, 21 Sep 2019 at 08:59, Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
>
> On Mon, 16 Sep 2019 22:47:51 +0900, Taehee Yoo wrote:
> > This patchset fixes several bugs that are related to nesting
> > device infrastructure.
> > Current nesting infrastructure code doesn't limit the depth level of
> > devices. nested devices could be handled recursively. at that moment,
> > it needs huge memory and stack overflow could occur.
> > Below devices type have same bug.
> > VLAN, BONDING, TEAM, MACSEC, MACVLAN and VXLAN.
>
> Is this list exhaustive? Looks like qmi_wwan.c perhaps could also have
> similar problem? Or virt_wifi? Perhaps worth CCing the authors of those
> drivers and mentioning that?
>
> Thank you for working on this!
I couldn't test all interface types.
So there could be more interface types that have a similar problem.
I will send a v4 patch and add more authors to the CC list.
Thank you so much for your review again!
^ permalink raw reply
* Re: [PATCH 0/4] Attempt to fix regression with AR8035 speed downgrade
From: tinywrkb @ 2019-09-22 15:03 UTC (permalink / raw)
To: Russell King - ARM Linux admin
Cc: Andrew Lunn, Florian Fainelli, Heiner Kallweit, David S. Miller,
netdev
In-Reply-To: <20190922105932.GP25745@shell.armlinux.org.uk>
On Sun, Sep 22, 2019 at 11:59:32AM +0100, Russell King - ARM Linux admin wrote:
> Hi,
>
> tinywrkb, please can you test this series to ensure that it fixes
> your problem - the previous version has turned out to be a non-starter
> as it introduces more problems, thanks!
Yes, this solves my issue.
Tested against v5.3.
Thanks Russell and everyone else who helped.
>
> The following series attempts to address an issue spotted by tinywrkb
> with the AR8035 on the Cubox-i2 in a situation where the PHY downgrades
> the negotiated link.
>
> Before commit 5502b218e001 ("net: phy: use phy_resolve_aneg_linkmode in
> genphy_read_status"), we would read not only the link partner's
> advertisement, but also our own advertisement from the PHY registers,
> and use both to derive the PHYs current link mode. This works when the
> AR8035 downgrades the speed, because it appears that the AR8035 clears
> link mode bits in the advertisement registers as part of the downgrade.
>
> Commentary: what is not yet known is whether the AR8035 restores the
> advertisement register when the link goes down to the
> previous state.
>
> However, since the above referenced commit, we no longer use the PHYs
> advertisement registers, instead converting the link partner's
> advertisement to the ethtool link mode array, and combine that with
> phylib's cached version of our advertisement - which is not updated on
> speed downgrade.
>
> This results in phylib disagreeing with the actual operating mode of
> the PHY.
>
> Commentary: I wonder how many more PHY drivers are broken by this
> commit, but have yet to be discovered.
>
> The obvious way to address this would be to disable the downgrade
> feature, and indeed this does fix the problem in tinywrkb's case - his
> link partner instead downgrades the speed by reducing its
> advertisement, resulting in phylib correctly evaluating a slower speed.
>
> However, it has a serious drawback - the gigabit control register (MII
> register 9) appears to become read only. It seems the only way to
> update the register is to re-enable the downgrade feature, reset the
> PHY, changing register 9, disable the downgrade feature, and reset the
> PHY again.
>
> This series attempts to address the problem using a different approach,
> similar to the approach taken with Marvell PHYs. The AR8031, AR8033
> and AR8035 have a PHY-Specific Status register which reports the
> actual operating mode of the PHY - both speed and duplex. This
> register correctly reports the operating mode irrespective of whether
> autoneg is enabled or not. We use this register to fill in phylib's
> speed and duplex parameters.
>
> In detail:
>
> Patch 1 fixes a bug where writing to register 9 does not update
> phylib's advertisement mask in the same way that writing register 4
> does; this looks like an omission from when gigabit PHY support came
> into being.
>
> Patch 2 seperates the generic phylib code which reads the link partners
> advertisement from the PHY, so that we can re-use this in the Atheros
> PHY driver.
>
> Patch 3 seperates the generic phylib pause mode; phylib provides no
> help for MAC drivers to ascertain the negotiated pause mode, it merely
> copies the link partner's pause mode bits into its own variables.
>
> Commentary: Both the aforementioned Atheros PHYs and Marvell PHYs
> provide the resolved pause modes in terms of whether
> we should transmit pause frames, or whether we should
> allow reception of pause frames. Surely the resolution
> of this should be in phylib?
>
> Patch 4 provides the Atheros PHY driver with a private "read_status"
> implementation that fills in phylib's speed and duplex settings
> depending on the PHY-Specific status register. This ensures that
> phylib and the MAC driver match the operating mode that the PHY has
> decided to use. Since the register also gives us MDIX status, we
> can trivially fill that status in as well.
>
> Note that, although the bits mentioned in this patch for this register
> match those in th Marvell PHY driver, and it is located at the same
> address, the meaning of other register bits varies between the PHYs.
> Therefore, I do not feel that it would be appropriate to make this some
> kind of generic function.
>
> drivers/net/phy/at803x.c | 69 ++++++++++++++++++++++++++++++++++++++++++++
> drivers/net/phy/phy-core.c | 20 ++++++++-----
> drivers/net/phy/phy.c | 5 ++++
> drivers/net/phy/phy_device.c | 65 +++++++++++++++++++++++++----------------
> include/linux/mii.h | 9 ++++++
> include/linux/phy.h | 2 ++
> 6 files changed, 138 insertions(+), 32 deletions(-)
>
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
> According to speedtest.net: 11.9Mbps down 500kbps up
^ permalink raw reply
* Re: [PATCH 1/4] net: phy: fix write to mii-ctrl1000 register
From: Andrew Lunn @ 2019-09-22 16:33 UTC (permalink / raw)
To: Russell King
Cc: Florian Fainelli, Heiner Kallweit, tinywrkb, David S. Miller,
netdev
In-Reply-To: <E1iBzb8-00006n-4B@rmk-PC.armlinux.org.uk>
On Sun, Sep 22, 2019 at 12:00:10PM +0100, Russell King wrote:
> When userspace writes to the MII_ADVERTISE register, we update phylib's
> advertising mask and trigger a renegotiation. However, writing to the
> MII_CTRL1000 register, which contains the gigabit advertisement, does
> neither. This can lead to phylib's copy of the advertisement becoming
> de-synced with the values in the PHY register set, which can result in
> incorrect negotiation resolution.
>
> Fixes: 5502b218e001 ("net: phy: use phy_resolve_aneg_linkmode in genphy_read_status")
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* Re: [PATCH 2/4] net: phy: extract link partner advertisement reading
From: Andrew Lunn @ 2019-09-22 16:35 UTC (permalink / raw)
To: Russell King
Cc: Florian Fainelli, Heiner Kallweit, tinywrkb, David S. Miller,
netdev
In-Reply-To: <E1iBzbD-00006v-Fb@rmk-PC.armlinux.org.uk>
On Sun, Sep 22, 2019 at 12:00:15PM +0100, Russell King wrote:
> Move reading the link partner advertisement out of genphy_read_status()
> into its own separate function. This will allow re-use of this code by
> PHY drivers that are able to read the resolved status from the PHY.
>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* Re: [PATCH 3/4] net: phy: extract pause mode
From: Andrew Lunn @ 2019-09-22 16:38 UTC (permalink / raw)
To: Russell King
Cc: Florian Fainelli, Heiner Kallweit, tinywrkb, David S. Miller,
netdev
In-Reply-To: <E1iBzbI-000072-R4@rmk-PC.armlinux.org.uk>
On Sun, Sep 22, 2019 at 12:00:20PM +0100, Russell King wrote:
> Extract the update of phylib's software pause mode state from
> genphy_read_status(), so that we can re-use this functionality with
> PHYs that have alternative ways to read the negotiation results.
>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ 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