* [PATCH net] net: dsa: sja1105: Add dependency for NET_DSA_SJA1105_TAS
From: Mao Wenan @ 2019-09-19 6:38 UTC (permalink / raw)
To: olteanv, andrew, vivien.didelot, f.fainelli, davem
Cc: linux-kernel, netdev, kernel-janitors, Mao Wenan
If CONFIG_NET_DSA_SJA1105_TAS=y and CONFIG_NET_SCH_TAPRIO=n,
below error can be found:
drivers/net/dsa/sja1105/sja1105_tas.o: In function `sja1105_setup_tc_taprio':
sja1105_tas.c:(.text+0x318): undefined reference to `taprio_offload_free'
sja1105_tas.c:(.text+0x590): undefined reference to `taprio_offload_get'
drivers/net/dsa/sja1105/sja1105_tas.o: In function `sja1105_tas_teardown':
sja1105_tas.c:(.text+0x610): undefined reference to `taprio_offload_free'
make: *** [vmlinux] Error 1
sja1105_tas needs tc-taprio, so this patch add the dependency for it.
Fixes: 317ab5b86c8e ("net: dsa: sja1105: Configure the Time-Aware Scheduler via tc-taprio offload")
Signed-off-by: Mao Wenan <maowenan@huawei.com>
---
drivers/net/dsa/sja1105/Kconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/dsa/sja1105/Kconfig b/drivers/net/dsa/sja1105/Kconfig
index 55424f3..f40b248 100644
--- a/drivers/net/dsa/sja1105/Kconfig
+++ b/drivers/net/dsa/sja1105/Kconfig
@@ -27,6 +27,7 @@ config NET_DSA_SJA1105_PTP
config NET_DSA_SJA1105_TAS
bool "Support for the Time-Aware Scheduler on NXP SJA1105"
depends on NET_DSA_SJA1105
+ depends on NET_SCH_TAPRIO
help
This enables support for the TTEthernet-based egress scheduling
engine in the SJA1105 DSA driver, which is controlled using a
--
2.7.4
^ permalink raw reply related
* Re: [PATCH net] net: dsa: sja1105: Add dependency for NET_DSA_SJA1105_TAS
From: Vladimir Oltean @ 2019-09-19 6:25 UTC (permalink / raw)
To: Mao Wenan
Cc: Andrew Lunn, Vivien Didelot, Florian Fainelli, David S. Miller,
lkml, netdev, kernel-janitors
In-Reply-To: <20190919063819.164826-1-maowenan@huawei.com>
On Thu, 19 Sep 2019 at 09:22, Mao Wenan <maowenan@huawei.com> wrote:
>
> If CONFIG_NET_DSA_SJA1105_TAS=y and CONFIG_NET_SCH_TAPRIO=n,
> below error can be found:
> drivers/net/dsa/sja1105/sja1105_tas.o: In function `sja1105_setup_tc_taprio':
> sja1105_tas.c:(.text+0x318): undefined reference to `taprio_offload_free'
> sja1105_tas.c:(.text+0x590): undefined reference to `taprio_offload_get'
> drivers/net/dsa/sja1105/sja1105_tas.o: In function `sja1105_tas_teardown':
> sja1105_tas.c:(.text+0x610): undefined reference to `taprio_offload_free'
> make: *** [vmlinux] Error 1
>
> sja1105_tas needs tc-taprio, so this patch add the dependency for it.
>
> Fixes: 317ab5b86c8e ("net: dsa: sja1105: Configure the Time-Aware Scheduler via tc-taprio offload")
> Signed-off-by: Mao Wenan <maowenan@huawei.com>
> ---
Reviewed-by: Vladimir Oltean <olteanv@gmail.com>
> drivers/net/dsa/sja1105/Kconfig | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/net/dsa/sja1105/Kconfig b/drivers/net/dsa/sja1105/Kconfig
> index 55424f3..f40b248 100644
> --- a/drivers/net/dsa/sja1105/Kconfig
> +++ b/drivers/net/dsa/sja1105/Kconfig
> @@ -27,6 +27,7 @@ config NET_DSA_SJA1105_PTP
> config NET_DSA_SJA1105_TAS
> bool "Support for the Time-Aware Scheduler on NXP SJA1105"
> depends on NET_DSA_SJA1105
> + depends on NET_SCH_TAPRIO
> help
> This enables support for the TTEthernet-based egress scheduling
> engine in the SJA1105 DSA driver, which is controlled using a
> --
> 2.7.4
>
Thanks!
-Vladimir
^ permalink raw reply
* Re: [PATCH v4 1/3] kernel/notifier.c: intercepting duplicate registrations to avoid infinite loops
From: Greg KH @ 2019-09-19 6:36 UTC (permalink / raw)
To: Xiaoming Ni
Cc: akpm, vvs, torvalds, adobriyan, anna.schumaker, arjan, bfields,
chuck.lever, davem, jlayton, luto, mingo, Nadia.Derbey, paulmck,
semen.protsenko, stern, tglx, trond.myklebust, viresh.kumar,
stable, dylix.dailei, yuehaibing, linux-kernel, linux-nfs, netdev
In-Reply-To: <1568861888-34045-2-git-send-email-nixiaoming@huawei.com>
On Thu, Sep 19, 2019 at 10:58:06AM +0800, Xiaoming Ni wrote:
> Registering the same notifier to a hook repeatedly can cause the hook
> list to form a ring or lose other members of the list.
>
> case1: An infinite loop in notifier_chain_register() can cause soft lockup
> atomic_notifier_chain_register(&test_notifier_list, &test1);
> atomic_notifier_chain_register(&test_notifier_list, &test1);
> atomic_notifier_chain_register(&test_notifier_list, &test2);
>
> case2: An infinite loop in notifier_chain_register() can cause soft lockup
> atomic_notifier_chain_register(&test_notifier_list, &test1);
> atomic_notifier_chain_register(&test_notifier_list, &test1);
> atomic_notifier_call_chain(&test_notifier_list, 0, NULL);
>
> case3: lose other hook test2
> atomic_notifier_chain_register(&test_notifier_list, &test1);
> atomic_notifier_chain_register(&test_notifier_list, &test2);
> atomic_notifier_chain_register(&test_notifier_list, &test1);
>
> case4: Unregister returns 0, but the hook is still in the linked list,
> and it is not really registered. If you call notifier_call_chain
> after ko is unloaded, it will trigger oops.
>
> If the system is configured with softlockup_panic and the same
> hook is repeatedly registered on the panic_notifier_list, it
> will cause a loop panic.
>
> Add a check in notifier_chain_register(),
> Intercepting duplicate registrations to avoid infinite loops
>
> Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com>
> Reviewed-by: Vasily Averin <vvs@virtuozzo.com>
> ---
> kernel/notifier.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
<formletter>
This is not the correct way to submit patches for inclusion in the
stable kernel tree. Please read:
https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly.
</formletter>
Same thing goes for all of the patches in this series.
thanks,
greg k-h
^ permalink raw reply
* Re: [Patch net] net_sched: add max len check for TCA_KIND
From: Jiri Pirko @ 2019-09-19 6:40 UTC (permalink / raw)
To: Cong Wang
Cc: netdev, syzbot+618aacd49e8c8b8486bd, David Ahern,
Jamal Hadi Salim
In-Reply-To: <20190918232412.16718-1-xiyou.wangcong@gmail.com>
Thu, Sep 19, 2019 at 01:24:12AM CEST, xiyou.wangcong@gmail.com wrote:
>The TCA_KIND attribute is of NLA_STRING which does not check
>the NUL char. KMSAN reported an uninit-value of TCA_KIND which
>is likely caused by the lack of NUL.
>
>Change it to NLA_NUL_STRING and add a max len too.
>
>Fixes: 8b4c3cdd9dd8 ("net: sched: Add policy validation for tc attributes")
>Reported-and-tested-by: syzbot+618aacd49e8c8b8486bd@syzkaller.appspotmail.com
Acked-by: Jiri Pirko <jiri@mellanox.com>
^ permalink raw reply
* Re: [PATCH 1/4] seccomp: add SECCOMP_RET_USER_NOTIF_ALLOW
From: Christian Brauner @ 2019-09-19 6:53 UTC (permalink / raw)
To: Kees Cook
Cc: luto, jannh, wad, shuah, ast, daniel, kafai, songliubraving, yhs,
linux-kernel, linux-kselftest, netdev, bpf, Tycho Andersen,
Tyler Hicks
In-Reply-To: <201909181018.E3CEC9A81@keescook>
On Wed, Sep 18, 2019 at 10:30:00AM -0700, Kees Cook wrote:
> On Wed, Sep 18, 2019 at 10:48:30AM +0200, Christian Brauner wrote:
> > This allows the seccomp notifier to continue a syscall. A positive
> > discussion about this feature was triggered by a post to the
> > ksummit-discuss mailing list (cf. [3]) and took place during KSummit
> > (cf. [1]) and again at the containers/checkpoint-restore
> > micro-conference at Linux Plumbers.
> >
> > Recently we landed seccomp support for SECCOMP_RET_USER_NOTIF (cf. [4])
> > which enables a process (watchee) to retrieve an fd for its seccomp
> > filter. This fd can then be handed to another (usually more privileged)
> > process (watcher). The watcher will then be able to receive seccomp
> > messages about the syscalls having been performed by the watchee.
> >
> > This feature is heavily used in some userspace workloads. For example,
> > it is currently used to intercept mknod() syscalls in user namespaces
> > aka in containers.
> > The mknod() syscall can be easily filtered based on dev_t. This allows
> > us to only intercept a very specific subset of mknod() syscalls.
> > Furthermore, mknod() is not possible in user namespaces toto coelo and
> > so intercepting and denying syscalls that are not in the whitelist on
> > accident is not a big deal. The watchee won't notice a difference.
> >
> > In contrast to mknod(), a lot of other syscall we intercept (e.g.
> > setxattr()) cannot be easily filtered like mknod() because they have
> > pointer arguments. Additionally, some of them might actually succeed in
> > user namespaces (e.g. setxattr() for all "user.*" xattrs). Since we
> > currently cannot tell seccomp to continue from a user notifier we are
> > stuck with performing all of the syscalls in lieu of the container. This
> > is a huge security liability since it is extremely difficult to
> > correctly assume all of the necessary privileges of the calling task
> > such that the syscall can be successfully emulated without escaping
> > other additional security restrictions (think missing CAP_MKNOD for
> > mknod(), or MS_NODEV on a filesystem etc.). This can be solved by
> > telling seccomp to resume the syscall.
> >
> > One thing that came up in the discussion was the problem that another
> > thread could change the memory after userspace has decided to let the
> > syscall continue which is a well known TOCTOU with seccomp which is
> > present in other ways already.
> > The discussion showed that this feature is already very useful for any
> > syscall without pointer arguments. For any accidentally intercepted
> > non-pointer syscall it is safe to continue.
> > For syscalls with pointer arguments there is a race but for any cautious
> > userspace and the main usec cases the race doesn't matter. The notifier
> > is intended to be used in a scenario where a more privileged watcher
> > supervises the syscalls of lesser privileged watchee to allow it to get
> > around kernel-enforced limitations by performing the syscall for it
> > whenever deemed save by the watcher. Hence, if a user tricks the watcher
> > into allowing a syscall they will either get a deny based on
> > kernel-enforced restrictions later or they will have changed the
> > arguments in such a way that they manage to perform a syscall with
> > arguments that they would've been allowed to do anyway.
> > In general, it is good to point out again, that the notifier fd was not
> > intended to allow userspace to implement a security policy but rather to
> > work around kernel security mechanisms in cases where the watcher knows
> > that a given action is safe to perform.
> >
> > /* References */
> > [1]: https://linuxplumbersconf.org/event/4/contributions/560
> > [2]: https://linuxplumbersconf.org/event/4/contributions/477
> > [3]: https://lore.kernel.org/r/20190719093538.dhyopljyr5ns33qx@brauner.io
> > [4]: commit 6a21cc50f0c7 ("seccomp: add a return code to trap to userspace")
> >
> > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> > Cc: Kees Cook <keescook@chromium.org>
> > Cc: Andy Lutomirski <luto@amacapital.net>
> > Cc: Will Drewry <wad@chromium.org>
> > Cc: Tycho Andersen <tycho@tycho.ws>
> > CC: Tyler Hicks <tyhicks@canonical.com>
> > Cc: Jann Horn <jannh@google.com>
> > ---
> > include/uapi/linux/seccomp.h | 2 ++
> > kernel/seccomp.c | 24 ++++++++++++++++++++----
> > 2 files changed, 22 insertions(+), 4 deletions(-)
> >
> > diff --git a/include/uapi/linux/seccomp.h b/include/uapi/linux/seccomp.h
> > index 90734aa5aa36..2c23b9aa6383 100644
> > --- a/include/uapi/linux/seccomp.h
> > +++ b/include/uapi/linux/seccomp.h
> > @@ -76,6 +76,8 @@ struct seccomp_notif {
> > struct seccomp_data data;
> > };
> >
> > +#define SECCOMP_RET_USER_NOTIF_ALLOW 0x00000001
>
> nit: I'd like to avoid confusion here about what "family" these flags
> belong to. "SECCOMP_RET_..." is used for the cBPF filter return action
> value, so let's instead call this:
>
> #define SECCOMP_USER_NOTIF_CONTINUE BIT(0)
Ack.
>
> I'm thinking of "continue" as slightly different from "allow", in the
> sense that I'd like to hint that this doesn't mean arguments could have
> been reliably "filtered" via user notification.
Good point.
>
> And at the same time, please add a giant comment about this in the
> header that details the purpose ("check if I should do something on
> behalf of the process") and not "is this safe to allow?", due to the
> argument parsing ToCToU.
Yeah, I'll copy parts of what I described in the commit message down
into the code.
>
> > -static void seccomp_do_user_notification(int this_syscall,
> > +static bool seccomp_do_user_notification(int this_syscall,
>
> I'd prefer this stay an "int", just to keep it similar to the other
> functions that are checked in __seccomp_filter().
Ack.
>
> > + /* perform syscall */
>
> nit: expand this commit to something like "Userspace requests we
> continue and perform syscall".
Ack.
>
> > + if (flags & SECCOMP_RET_USER_NOTIF_ALLOW)
> > + return false;
>
> return 0;
>
> > +
> > syscall_set_return_value(current, task_pt_regs(current),
> > err, ret);
> > + return true;
>
> return -1;
>
> (This makes it look more like a "skip on failure")
Ack.
>
> > + if (resp.flags & ~SECCOMP_RET_USER_NOTIF_ALLOW)
> > + return -EINVAL;
> > +
> > + if ((resp.flags & SECCOMP_RET_USER_NOTIF_ALLOW) &&
> > + (resp.error || resp.val))
> > return -EINVAL;
>
> Ah yeah, good idea.
>
> Beyond these nits, yes, looks good and should help the usability of this
> feature. Thanks for getting it written and tested!
Will rework and resend!
Thanks for the review! :)
Christian
^ permalink raw reply
* Re: [PATCH 1/4] seccomp: add SECCOMP_RET_USER_NOTIF_ALLOW
From: Christian Brauner @ 2019-09-19 6:53 UTC (permalink / raw)
To: Tycho Andersen
Cc: Kees Cook, luto, jannh, wad, shuah, ast, daniel, kafai,
songliubraving, yhs, linux-kernel, linux-kselftest, netdev, bpf,
Tyler Hicks
In-Reply-To: <20190918180712.GG3835@cisco>
On Wed, Sep 18, 2019 at 12:07:12PM -0600, Tycho Andersen wrote:
> On Wed, Sep 18, 2019 at 10:30:00AM -0700, Kees Cook wrote:
> > On Wed, Sep 18, 2019 at 10:48:30AM +0200, Christian Brauner wrote:
> > > This allows the seccomp notifier to continue a syscall. A positive
> > > discussion about this feature was triggered by a post to the
> > > ksummit-discuss mailing list (cf. [3]) and took place during KSummit
> > > (cf. [1]) and again at the containers/checkpoint-restore
> > > micro-conference at Linux Plumbers.
> > >
> > > Recently we landed seccomp support for SECCOMP_RET_USER_NOTIF (cf. [4])
> > > which enables a process (watchee) to retrieve an fd for its seccomp
> > > filter. This fd can then be handed to another (usually more privileged)
> > > process (watcher). The watcher will then be able to receive seccomp
> > > messages about the syscalls having been performed by the watchee.
> > >
> > > This feature is heavily used in some userspace workloads. For example,
> > > it is currently used to intercept mknod() syscalls in user namespaces
> > > aka in containers.
> > > The mknod() syscall can be easily filtered based on dev_t. This allows
> > > us to only intercept a very specific subset of mknod() syscalls.
> > > Furthermore, mknod() is not possible in user namespaces toto coelo and
> > > so intercepting and denying syscalls that are not in the whitelist on
> > > accident is not a big deal. The watchee won't notice a difference.
> > >
> > > In contrast to mknod(), a lot of other syscall we intercept (e.g.
> > > setxattr()) cannot be easily filtered like mknod() because they have
> > > pointer arguments. Additionally, some of them might actually succeed in
> > > user namespaces (e.g. setxattr() for all "user.*" xattrs). Since we
> > > currently cannot tell seccomp to continue from a user notifier we are
> > > stuck with performing all of the syscalls in lieu of the container. This
> > > is a huge security liability since it is extremely difficult to
> > > correctly assume all of the necessary privileges of the calling task
> > > such that the syscall can be successfully emulated without escaping
> > > other additional security restrictions (think missing CAP_MKNOD for
> > > mknod(), or MS_NODEV on a filesystem etc.). This can be solved by
> > > telling seccomp to resume the syscall.
> > >
> > > One thing that came up in the discussion was the problem that another
> > > thread could change the memory after userspace has decided to let the
> > > syscall continue which is a well known TOCTOU with seccomp which is
> > > present in other ways already.
> > > The discussion showed that this feature is already very useful for any
> > > syscall without pointer arguments. For any accidentally intercepted
> > > non-pointer syscall it is safe to continue.
> > > For syscalls with pointer arguments there is a race but for any cautious
> > > userspace and the main usec cases the race doesn't matter. The notifier
> > > is intended to be used in a scenario where a more privileged watcher
> > > supervises the syscalls of lesser privileged watchee to allow it to get
> > > around kernel-enforced limitations by performing the syscall for it
> > > whenever deemed save by the watcher. Hence, if a user tricks the watcher
> > > into allowing a syscall they will either get a deny based on
> > > kernel-enforced restrictions later or they will have changed the
> > > arguments in such a way that they manage to perform a syscall with
> > > arguments that they would've been allowed to do anyway.
> > > In general, it is good to point out again, that the notifier fd was not
> > > intended to allow userspace to implement a security policy but rather to
> > > work around kernel security mechanisms in cases where the watcher knows
> > > that a given action is safe to perform.
> > >
> > > /* References */
> > > [1]: https://linuxplumbersconf.org/event/4/contributions/560
> > > [2]: https://linuxplumbersconf.org/event/4/contributions/477
> > > [3]: https://lore.kernel.org/r/20190719093538.dhyopljyr5ns33qx@brauner.io
> > > [4]: commit 6a21cc50f0c7 ("seccomp: add a return code to trap to userspace")
> > >
> > > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> > > Cc: Kees Cook <keescook@chromium.org>
> > > Cc: Andy Lutomirski <luto@amacapital.net>
> > > Cc: Will Drewry <wad@chromium.org>
> > > Cc: Tycho Andersen <tycho@tycho.ws>
> > > CC: Tyler Hicks <tyhicks@canonical.com>
> > > Cc: Jann Horn <jannh@google.com>
> > > ---
> > > include/uapi/linux/seccomp.h | 2 ++
> > > kernel/seccomp.c | 24 ++++++++++++++++++++----
> > > 2 files changed, 22 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/include/uapi/linux/seccomp.h b/include/uapi/linux/seccomp.h
> > > index 90734aa5aa36..2c23b9aa6383 100644
> > > --- a/include/uapi/linux/seccomp.h
> > > +++ b/include/uapi/linux/seccomp.h
> > > @@ -76,6 +76,8 @@ struct seccomp_notif {
> > > struct seccomp_data data;
> > > };
> > >
> > > +#define SECCOMP_RET_USER_NOTIF_ALLOW 0x00000001
> >
> > nit: I'd like to avoid confusion here about what "family" these flags
> > belong to. "SECCOMP_RET_..." is used for the cBPF filter return action
> > value, so let's instead call this:
> >
> > #define SECCOMP_USER_NOTIF_CONTINUE BIT(0)
>
> +1, I was thinking maybe even SECCOMP_USER_NOTIF_FLAG_CONTINUE.
I'll flip a coin between yours and Kees suggestion. :)
>
> But the whole series (minus the patch that already exists) looks good
> to me if we make this change:
>
> Reviewed-by: Tycho Andersen <tycho@tycho.ws>
Thanks for the review! :)
Christian
^ permalink raw reply
* [PATCH 1/2][ethtool] ethtool: sync ethtool-copy.h: adds support for EDPD
From: Alexandru Ardelean @ 2019-09-19 10:08 UTC (permalink / raw)
To: netdev; +Cc: linville, andrew, f.fainelli, Alexandru Ardelean
This change syncs the `ethtool-copy.h` file with Linux net-next to add
support for Energy Detect Powerdown control via phy tunable.
net-next commit/sync-point:
commit 1bab8d4c488be22d57f9dd09968c90a0ddc413bf
Merge: 990925fad5c2 00b368502d18
Author: David S. Miller <davem@davemloft.net>
Date: Tue Sep 17 23:51:10 2019 +0200
Merge ra.kernel.org:/pub/scm/linux/kernel/git/netdev/net
Pull in bug fixes from 'net' tree for the merge window.
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
ethtool-copy.h | 30 ++++++++++++++++++++++++++----
1 file changed, 26 insertions(+), 4 deletions(-)
diff --git a/ethtool-copy.h b/ethtool-copy.h
index ad16e8f..9afd2e6 100644
--- a/ethtool-copy.h
+++ b/ethtool-copy.h
@@ -257,10 +257,32 @@ struct ethtool_tunable {
#define ETHTOOL_PHY_FAST_LINK_DOWN_ON 0
#define ETHTOOL_PHY_FAST_LINK_DOWN_OFF 0xff
+/* Energy Detect Power Down (EDPD) is a feature supported by some PHYs, where
+ * the PHY's RX & TX blocks are put into a low-power mode when there is no
+ * link detected (typically cable is un-plugged). For RX, only a minimal
+ * link-detection is available, and for TX the PHY wakes up to send link pulses
+ * to avoid any lock-ups in case the peer PHY may also be running in EDPD mode.
+ *
+ * Some PHYs may support configuration of the wake-up interval for TX pulses,
+ * and some PHYs may support only disabling TX pulses entirely. For the latter
+ * a special value is required (ETHTOOL_PHY_EDPD_NO_TX) so that this can be
+ * configured from userspace (should the user want it).
+ *
+ * The interval units for TX wake-up are in milliseconds, since this should
+ * cover a reasonable range of intervals:
+ * - from 1 millisecond, which does not sound like much of a power-saver
+ * - to ~65 seconds which is quite a lot to wait for a link to come up when
+ * plugging a cable
+ */
+#define ETHTOOL_PHY_EDPD_DFLT_TX_MSECS 0xffff
+#define ETHTOOL_PHY_EDPD_NO_TX 0xfffe
+#define ETHTOOL_PHY_EDPD_DISABLE 0
+
enum phy_tunable_id {
ETHTOOL_PHY_ID_UNSPEC,
ETHTOOL_PHY_DOWNSHIFT,
ETHTOOL_PHY_FAST_LINK_DOWN,
+ ETHTOOL_PHY_EDPD,
/*
* Add your fresh new phy tunable attribute above and remember to update
* phy_tunable_strings[] in net/core/ethtool.c
@@ -1481,8 +1503,8 @@ enum ethtool_link_mode_bit_indices {
ETHTOOL_LINK_MODE_200000baseLR4_ER4_FR4_Full_BIT = 64,
ETHTOOL_LINK_MODE_200000baseDR4_Full_BIT = 65,
ETHTOOL_LINK_MODE_200000baseCR4_Full_BIT = 66,
- ETHTOOL_LINK_MODE_100baseT1_Full_BIT = 67,
- ETHTOOL_LINK_MODE_1000baseT1_Full_BIT = 68,
+ ETHTOOL_LINK_MODE_100baseT1_Full_BIT = 67,
+ ETHTOOL_LINK_MODE_1000baseT1_Full_BIT = 68,
/* must be last entry */
__ETHTOOL_LINK_MODE_MASK_NBITS
@@ -1712,8 +1734,8 @@ static __inline__ int ethtool_validate_duplex(__u8 duplex)
#define ETH_MODULE_SFF_8436 0x4
#define ETH_MODULE_SFF_8436_LEN 256
-#define ETH_MODULE_SFF_8636_MAX_LEN 640
-#define ETH_MODULE_SFF_8436_MAX_LEN 640
+#define ETH_MODULE_SFF_8636_MAX_LEN 640
+#define ETH_MODULE_SFF_8436_MAX_LEN 640
/* Reset flags */
/* The reset() operation must clear the flags for the components which
--
2.20.1
^ permalink raw reply related
* [PATCH 2/2][ethtool] ethtool: implement support for Energy Detect Power Down
From: Alexandru Ardelean @ 2019-09-19 10:08 UTC (permalink / raw)
To: netdev; +Cc: linville, andrew, f.fainelli, Alexandru Ardelean
In-Reply-To: <20190919100833.6208-1-alexandru.ardelean@analog.com>
This change adds control for enabling/disabling Energy Detect Power Down
mode, as well as configuring wake-up intervals for TX pulses, via the new
ETHTOOL_PHY_EDPD control added in PHY tunable, in the kernel.
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
ethtool.8.in | 28 +++++++++++++++++
ethtool.c | 87 +++++++++++++++++++++++++++++++++++++++++++++++++---
2 files changed, 111 insertions(+), 4 deletions(-)
diff --git a/ethtool.8.in b/ethtool.8.in
index cd3be91..609a05a 100644
--- a/ethtool.8.in
+++ b/ethtool.8.in
@@ -362,11 +362,17 @@ ethtool \- query or control network driver and hardware settings
.A1 on off
.BN msecs
.RB ]
+.RB [
+.B energy\-detect\-power\-down
+.A1 on off
+.BN msecs
+.RB ]
.HP
.B ethtool \-\-get\-phy\-tunable
.I devname
.RB [ downshift ]
.RB [ fast-link-down ]
+.RB [ energy-detect-power-down ]
.HP
.B ethtool \-\-reset
.I devname
@@ -1100,6 +1106,24 @@ lB l.
Sets the period after which the link is reported as down. Note that the PHY may choose
the closest supported value. Only on reading back the tunable do you get the actual value.
.TE
+.TP
+.A2 energy-detect-power-down on off
+Specifies whether Energy Detect Power Down (EDPD) should be enabled (if supported).
+This will put the RX and TX circuit blocks into a low power mode, and the PHY will
+wake up periodically to send link pulses to avoid any lock-up situation with a peer
+PHY that may also have EDPD enabled. By default, this setting will also enable the
+periodic transmission of TX pulses.
+.TS
+nokeep;
+lB l.
+.BI msecs \ N
+ Some PHYs support configuration of the wake-up interval to send TX pulses.
+ This setting allows the control of this interval, and 0 disables TX pulses
+ if the PHY supports this. Disabling TX pulses can create a lock-up situation
+ where neither of the PHYs wakes the other one. If unspecified the default
+ value (in milliseconds) will be used by the PHY.
+.TE
+.TP
.PD
.RE
.TP
@@ -1122,6 +1146,10 @@ Some PHYs support a Fast Link Down Feature and may allow configuration of the de
before a broken link is reported as being down.
Gets the PHY Fast Link Down status / period.
+.TP
+.B energy\-detect\-power\-down
+Gets the current configured setting for Energy Detect Power Down (if supported).
+
.RE
.TP
.B \-\-reset
diff --git a/ethtool.c b/ethtool.c
index c0e2903..45b5478 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -4897,6 +4897,30 @@ static int do_get_phy_tunable(struct cmd_context *ctx)
else
fprintf(stdout, "Fast Link Down enabled, %d msecs\n",
cont.msecs);
+ } else if (!strcmp(argp[0], "energy-detect-power-down")) {
+ struct {
+ struct ethtool_tunable ds;
+ u16 msecs;
+ } cont;
+
+ cont.ds.cmd = ETHTOOL_PHY_GTUNABLE;
+ cont.ds.id = ETHTOOL_PHY_EDPD;
+ cont.ds.type_id = ETHTOOL_TUNABLE_U16;
+ cont.ds.len = 2;
+ if (send_ioctl(ctx, &cont.ds) < 0) {
+ perror("Cannot Get PHY Energy Detect Power Down value");
+ return 87;
+ }
+
+ if (cont.msecs == ETHTOOL_PHY_EDPD_DISABLE)
+ fprintf(stdout, "Energy Detect Power Down: disabled\n");
+ else if (cont.msecs == ETHTOOL_PHY_EDPD_NO_TX)
+ fprintf(stdout,
+ "Energy Detect Power Down: enabled, TX disabled\n");
+ else
+ fprintf(stdout,
+ "Energy Detect Power Down: enabled, TX %u msecs\n",
+ cont.msecs);
} else {
exit_bad_args();
}
@@ -5018,7 +5042,8 @@ static int parse_named_bool(struct cmd_context *ctx, const char *name, u8 *on)
return 1;
}
-static int parse_named_u8(struct cmd_context *ctx, const char *name, u8 *val)
+static int parse_named_uint(struct cmd_context *ctx, const char *name,
+ void *val, enum tunable_type_id type_id)
{
if (ctx->argc < 2)
return 0;
@@ -5026,7 +5051,16 @@ static int parse_named_u8(struct cmd_context *ctx, const char *name, u8 *val)
if (strcmp(*ctx->argp, name))
return 0;
- *val = get_uint_range(*(ctx->argp + 1), 0, 0xff);
+ switch (type_id) {
+ case ETHTOOL_TUNABLE_U8:
+ *(u8 *)val = get_uint_range(*(ctx->argp + 1), 0, 0xff);
+ break;
+ case ETHTOOL_TUNABLE_U16:
+ *(u16 *)val = get_uint_range(*(ctx->argp + 1), 0, 0xffff);
+ break;
+ default:
+ return 0;
+ }
ctx->argc -= 2;
ctx->argp += 2;
@@ -5034,6 +5068,16 @@ static int parse_named_u8(struct cmd_context *ctx, const char *name, u8 *val)
return 1;
}
+static int parse_named_u8(struct cmd_context *ctx, const char *name, u8 *val)
+{
+ return parse_named_uint(ctx, name, val, ETHTOOL_TUNABLE_U8);
+}
+
+static int parse_named_u16(struct cmd_context *ctx, const char *name, u16 *val)
+{
+ return parse_named_uint(ctx, name, val, ETHTOOL_TUNABLE_U16);
+}
+
static int do_set_phy_tunable(struct cmd_context *ctx)
{
int err = 0;
@@ -5041,6 +5085,8 @@ static int do_set_phy_tunable(struct cmd_context *ctx)
u8 ds_changed = 0, ds_has_cnt = 0, ds_enable = 0;
u8 fld_changed = 0, fld_enable = 0;
u8 fld_msecs = ETHTOOL_PHY_FAST_LINK_DOWN_ON;
+ u8 edpd_changed = 0, edpd_enable = 0;
+ u16 edpd_tx_interval = ETHTOOL_PHY_EDPD_DFLT_TX_MSECS;
/* Parse arguments */
if (parse_named_bool(ctx, "downshift", &ds_enable)) {
@@ -5050,6 +5096,11 @@ static int do_set_phy_tunable(struct cmd_context *ctx)
fld_changed = 1;
if (fld_enable)
parse_named_u8(ctx, "msecs", &fld_msecs);
+ } else if (parse_named_bool(ctx, "energy-detect-power-down",
+ &edpd_enable)) {
+ edpd_changed = 1;
+ if (edpd_enable)
+ parse_named_u16(ctx, "msecs", &edpd_tx_interval);
} else {
exit_bad_args();
}
@@ -5074,6 +5125,16 @@ static int do_set_phy_tunable(struct cmd_context *ctx)
fld_msecs = ETHTOOL_PHY_FAST_LINK_DOWN_OFF;
else if (fld_msecs == ETHTOOL_PHY_FAST_LINK_DOWN_OFF)
exit_bad_args();
+ } else if (edpd_changed) {
+ if (!edpd_enable)
+ edpd_tx_interval = ETHTOOL_PHY_EDPD_DISABLE;
+ else if (edpd_tx_interval == 0)
+ edpd_tx_interval = ETHTOOL_PHY_EDPD_NO_TX;
+ else if (edpd_tx_interval > ETHTOOL_PHY_EDPD_NO_TX) {
+ fprintf(stderr, "'msecs' max value is %d.\n",
+ (ETHTOOL_PHY_EDPD_NO_TX - 1));
+ exit_bad_args();
+ }
}
/* Do it */
@@ -5109,6 +5170,22 @@ static int do_set_phy_tunable(struct cmd_context *ctx)
perror("Cannot Set PHY Fast Link Down value");
err = 87;
}
+ } else if (edpd_changed) {
+ struct {
+ struct ethtool_tunable fld;
+ u16 msecs;
+ } cont;
+
+ cont.fld.cmd = ETHTOOL_PHY_STUNABLE;
+ cont.fld.id = ETHTOOL_PHY_EDPD;
+ cont.fld.type_id = ETHTOOL_TUNABLE_U16;
+ cont.fld.len = 2;
+ cont.msecs = edpd_tx_interval;
+ err = send_ioctl(ctx, &cont.fld);
+ if (err < 0) {
+ perror("Cannot Set PHY Energy Detect Power Down");
+ err = 87;
+ }
}
return err;
@@ -5361,10 +5438,12 @@ static const struct option {
" [ tx-timer %d ]\n"},
{ "--set-phy-tunable", 1, do_set_phy_tunable, "Set PHY tunable",
" [ downshift on|off [count N] ]\n"
- " [ fast-link-down on|off [msecs N] ]\n"},
+ " [ fast-link-down on|off [msecs N] ]\n"
+ " [ energy-detect-power-down on|off [msecs N] ]\n"},
{ "--get-phy-tunable", 1, do_get_phy_tunable, "Get PHY tunable",
" [ downshift ]\n"
- " [ fast-link-down ]\n"},
+ " [ fast-link-down ]\n"
+ " [ energy-detect-power-down ]\n"},
{ "--reset", 1, do_reset, "Reset components",
" [ flags %x ]\n"
" [ mgmt ]\n"
--
2.20.1
^ permalink raw reply related
* Re: [Patch net] net_sched: add policy validation for action attributes
From: Jiri Pirko @ 2019-09-19 7:33 UTC (permalink / raw)
To: Cong Wang; +Cc: netdev, David Ahern, Jamal Hadi Salim
In-Reply-To: <20190919014443.32581-1-xiyou.wangcong@gmail.com>
Thu, Sep 19, 2019 at 03:44:43AM CEST, xiyou.wangcong@gmail.com wrote:
>Similar to commit 8b4c3cdd9dd8
>("net: sched: Add policy validation for tc attributes"), we need
>to add proper policy validation for TC action attributes too.
>
>Cc: David Ahern <dsahern@gmail.com>
>Cc: Jamal Hadi Salim <jhs@mojatatu.com>
>Cc: Jiri Pirko <jiri@resnulli.us>
>Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
Acked-by: Jiri Pirko <jiri@mellanox.com>
^ permalink raw reply
* Re: [PATCH v7 1/7] nfc: pn533: i2c: "pn532" as dt compatible string
From: Lars Poeschel @ 2019-09-19 7:36 UTC (permalink / raw)
To: Simon Horman
Cc: Allison Randal, Greg Kroah-Hartman, Jilayne Lovejoy, Kate Stewart,
Thomas Gleixner, open list:NFC SUBSYSTEM, open list, Johan Hovold
In-Reply-To: <20190918123457.wg6mtygr6cboqsp6@verge.net.au>
On Wed, Sep 18, 2019 at 02:34:57PM +0200, Simon Horman wrote:
> On Tue, Sep 10, 2019 at 11:31:21AM +0200, Lars Poeschel wrote:
> > It is favourable to have one unified compatible string for devices that
> > have multiple interfaces. So this adds simply "pn532" as the devicetree
> > binding compatible string and makes a note that the old ones are
> > deprecated.
>
> Do you also need to update
> Documentation/devicetree/bindings/net/nfc/pn533-i2c.txt
> to both document the new compat string and deprecate the old ones?
Simon, thank you for this hint.
The patch 2/7 adds a seperate binding doc, that contains the info about
the deprecated compat strings. But I think this is not the way to go. I
will change the patch 2/7 to update the info
Documentation/devicetree/bindings/net/nfc/pn533-i2c.txt
instead, rename it to pn532.txt and do not add a new binding doc.
^ permalink raw reply
* RE: [PATCH] stmmac: selftest: avoid large stack usage
From: Jose Abreu @ 2019-09-19 7:58 UTC (permalink / raw)
To: Arnd Bergmann, Giuseppe Cavallaro, Alexandre Torgue,
David S. Miller, Maxime Coquelin
Cc: netdev@vger.kernel.org, linux-stm32@st-md-mailman.stormreply.com,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
In-Reply-To: <20190918195454.2056139-1-arnd@arndb.de>
From: Arnd Bergmann <arnd@arndb.de>
Date: Sep/18/2019, 20:54:34 (UTC+00:00)
> + if (!cfg || !cfg->enable) {
> value &= ~XGMAC_RSSE;
> writel(value, ioaddr + XGMAC_RSS_CTRL);
> return 0;
> }
>
> for (i = 0; i < (sizeof(cfg->key) / sizeof(u32)); i++) {
> - ret = dwxgmac2_rss_write_reg(ioaddr, true, i, *key++);
> + if (cfg)
> + ret = dwxgmac2_rss_write_reg(ioaddr, true, i, cfg->key[i]);
> + else
> + ret = dwxgmac2_rss_write_reg(ioaddr, true, i, 0);
> +
> if (ret)
> return ret;
> }
>
> for (i = 0; i < ARRAY_SIZE(cfg->table); i++) {
> - ret = dwxgmac2_rss_write_reg(ioaddr, false, i, cfg->table[i]);
> + if (cfg)
> + ret = dwxgmac2_rss_write_reg(ioaddr, false, i, cfg->table[i]);
> + else
> + ret = dwxgmac2_rss_write_reg(ioaddr, false, i, 0);
> +
I don't get these "if (cfg)" checks. You already check earlier if cfg is
NULL and return if so. I don't think you need these extra checks.
Also, your subject line should be something like: "net: stmmac:
selftests: ..."
---
Thanks,
Jose Miguel Abreu
^ permalink raw reply
* Re: Fwd: [PATCH] bonding/802.3ad: fix slave initialization states race
From: Jay Vosburgh @ 2019-09-19 8:00 UTC (permalink / raw)
To: =?UTF-8?B?0JDQu9C10LrRgdC10Lkg0JfQsNGF0LDRgNC+0LI=?=; +Cc: netdev
In-Reply-To: <CAJYOGF8LDwbZXXeEioKAtx=0rq9eZBxFYuRfF3jdFCDUGnJ-Rg@mail.gmail.com>
Алексей Захаров wrote:
>> >Once a while, one of 802.3ad slaves fails to initialize and hangs in
>> >BOND_LINK_FAIL state. Commit 334031219a84 ("bonding/802.3ad: fix slave
>> >link initialization transition states") checks slave->last_link_up. But
>> >link can still hang in weird state.
>> >After physical link comes up it sends first two LACPDU messages and
>> >doesn't work properly after that. It doesn't send or receive LACPDU.
>> >Once it happens, the only message in dmesg is:
>> >bond1: link status up again after 0 ms for interface eth2
>>
>> I believe this message indicates that the slave entered
>> BOND_LINK_FAIL state, but downdelay was not set. The _FAIL state is
>> really for managing the downdelay expiration, and a slave should not be
>> in that state (outside of a brief transition entirely within
>> bond_miimon_inspect) if downdelay is 0.
>That's true, downdelay was set to 0, we only use updelay 500.
>Does it mean, that the bonding driver shouldn't set slave to FAIL
>state in this case?
It really shouldn't change the slave->link outside of the
monitoring functions at all, because there are side effects that are not
happening (user space notifications, updelay / downdelay, etc).
>> >This behavior can be reproduced (not every time):
>> >1. Set slave link down
>> >2. Wait for 1-3 seconds
>> >3. Set slave link up
>> >
>> >The fix is to check slave->link before setting it to BOND_LINK_FAIL or
>> >BOND_LINK_DOWN state. If got invalid Speed/Dupex values and link is in
>> >BOND_LINK_UP state, mark it as BOND_LINK_FAIL; otherwise mark it as
>> >BOND_LINK_DOWN.
>> >
>> >Fixes: 334031219a84 ("bonding/802.3ad: fix slave link initialization
>> >transition states")
>> >Signed-off-by: Aleksei Zakharov <zakharov.a.g@yandex.ru>
>> >---
>> > drivers/net/bonding/bond_main.c | 2 +-
>> > 1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> >diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> >index 931d9d935686..a28776d8f33f 100644
>> >--- a/drivers/net/bonding/bond_main.c
>> >+++ b/drivers/net/bonding/bond_main.c
>> >@@ -3135,7 +3135,7 @@ static int bond_slave_netdev_event(unsigned long event,
>> > */
>> > if (bond_update_speed_duplex(slave) &&
>> > BOND_MODE(bond) == BOND_MODE_8023AD) {
>> >- if (slave->last_link_up)
>> >+ if (slave->link == BOND_LINK_UP)
>> > slave->link = BOND_LINK_FAIL;
>> > else
>> > slave->link = BOND_LINK_DOWN;
>>
>> Is the core problem here that slaves are reporting link up, but
>> returning invalid values for speed and/or duplex? If so, what network
>> device are you testing with that is exhibiting this behavior?
>That's true, because link becomes FAIL right in this block of code.
>We use Mellanox ConnectX-3 Pro nic.
>
>>
>> If I'm not mistaken, there have been several iterations of
>> hackery on this block of code to work around this same problem, and each
>> time there's some corner case that still doesn't work.
>As i can see, commit 4d2c0cda0744 ("bonding: speed/duplex update at
>NETDEV_UP event")
>introduced BOND_LINK_DOWN state if update speed/duplex failed.
>
>Commit ea53abfab960 ("bonding/802.3ad: fix link_failure_count tracking")
>changed DOWN state to FAIL.
>
>Commit 334031219a84 ("bonding/802.3ad: fix slave link initialization
>transition states")
>implemented different new state for different current states, but it
>was based on slave->last_link_up.
>In our case slave->last_link_up !=0 when this code runs. But, slave is
>not in UP state at the moment. It becomes
>FAIL and hangs in this state.
>So, it looks like checking if slave is in UP mode is more appropriate
>here. At least it works in our case.
>
>There was one more commit 12185dfe4436 ("bonding: Force slave speed
>check after link state recovery for 802.3ad")
>but it doesn't help in our case.
>
>>
>> As Davem asked last time around, is the real problem that device
>> drivers report carrier up but supply invalid speed and duplex state?
>Probably, but I'm not quite sure right now. We didn't face this issue
>before 4d2c0cda0744 and ea53abfab960
>commits.
My concern here is that we keep adding special cases to this
code apparently without really understanding the root cause of the
failures. 4d2c0cda0744 asserts that there is a problem that drivers are
not supplying speed and duplex information at NETDEV_UP time, but is not
specific as to the details (hardware information). Before we add
another change, I would like to understand what the actual underlying
cause of the failure is, and if yours is somehow different from what
4d2c0cda0744 or ea53abfab960 were fixing (or trying to fix).
Would it be possible for you to instrument the code here to dump
out the duplex/speed failure information and carrier state of the slave
device at this point when it fails in your testing? Something like the
following (which I have not compile tested):
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 931d9d935686..758af8c2b9e1 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -378,15 +378,22 @@ static int bond_update_speed_duplex(struct slave *slave)
slave->duplex = DUPLEX_UNKNOWN;
res = __ethtool_get_link_ksettings(slave_dev, &ecmd);
- if (res < 0)
+ if (res < 0) {
+ pr_err("DBG ksettings res %d slave %s\n", res, slave_dev->name);
return 1;
- if (ecmd.base.speed == 0 || ecmd.base.speed == ((__u32)-1))
+ }
+ if (ecmd.base.speed == 0 || ecmd.base.speed == ((__u32)-1)) {
+ pr_err("DBG speed %u slave %s\n", ecmd.base.speed,
+ slave_dev->name);
return 1;
+ }
switch (ecmd.base.duplex) {
case DUPLEX_FULL:
case DUPLEX_HALF:
break;
default:
+ pr_err("DBG duplex %u slave %s\n", ecmd.base.duplex,
+ slave_dev->name);
return 1;
}
@@ -3135,6 +3142,9 @@ static int bond_slave_netdev_event(unsigned long event,
*/
if (bond_update_speed_duplex(slave) &&
BOND_MODE(bond) == BOND_MODE_8023AD) {
+ pr_err("DBG slave %s event %d carrier %d\n",
+ slave->dev->name, event,
+ netif_carrier_ok(slave->dev));
if (slave->last_link_up)
slave->link = BOND_LINK_FAIL;
else
Thanks,
-J
---
-Jay Vosburgh, jay.vosburgh@canonical.com
^ permalink raw reply related
* Re: dsa traffic priorization
From: Sascha Hauer @ 2019-09-19 8:00 UTC (permalink / raw)
To: Vladimir Oltean
Cc: netdev, Andrew Lunn, Vivien Didelot, Florian Fainelli, kernel
In-Reply-To: <CA+h21hpG52R6ScGpGX86Q7MuRHCgGNY-TxzaQGu2wZR8EtPtbA@mail.gmail.com>
Hi Vladimir,
On Wed, Sep 18, 2019 at 05:36:08PM +0300, Vladimir Oltean wrote:
> Hi Sascha,
>
> On Wed, 18 Sep 2019 at 17:03, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> >
> > Hi All,
> >
> > We have a customer using a Marvell 88e6240 switch with Ethercat on one port and
> > regular network traffic on another port. The customer wants to configure two things
> > on the switch: First Ethercat traffic shall be priorized over other network traffic
> > (effectively prioritizing traffic based on port). Second the ethernet controller
> > in the CPU is not able to handle full bandwidth traffic, so the traffic to the CPU
> > port shall be rate limited.
> >
>
> You probably already know this, but egress shaping will not drop
> frames, just let them accumulate in the egress queue until something
> else happens (e.g. queue occupancy threshold triggers pause frames, or
> tail dropping is enabled, etc). Is this what you want?
If I understand correctly then the switch has multiple output queues per
port. The Ethercat traffic will go to a higher priority queue and on
congestion on other queues, frames designated for that queue will be
dropped. I just talked to our customer and he verified that their
Ethercat traffic still goes through even when the ports with the general
traffic are jammed with packets. So yes, I think this is what I want.
> It sounds a bit
> strange to me to configure egress shaping on the CPU port of a DSA
> switch. That literally means you are buffering frames inside the
> system. What about ingress policing?
The bottleneck here is in the CPU interface. The SoC simply can't handle
all frames coming into a fully occupied link, so we indeed have to limit
the number of packets coming into the SoC which speaks for egress rate
limiting. We could of course limit the ingress packets on the other
ports, but that would mean we have to rate limit each port to the total
desired rate divided by the number of ports to be safe, not very
optimal.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply
* Re: [PATCH RFC] net/phy: fix Marvell PHYs probe failure when HWMON and THERMAL_OF are enabled
From: Peter Mamonov @ 2019-09-19 8:10 UTC (permalink / raw)
To: Andrew Lunn; +Cc: f.fainelli, hkallweit1, netdev
In-Reply-To: <20190919025016.GA12785@lunn.ch>
Hi, Andrew,
On Thu, Sep 19, 2019 at 04:50:16AM +0200, Andrew Lunn wrote:
> On Thu, Sep 19, 2019 at 12:38:37AM +0300, Peter Mamonov wrote:
> > Hello,
> >
> > Some time ago I've discovered that probe functions of certain Marvell PHYs
> > fail if both HWMON and THERMAL_OF config options are enabled.
>
> Hi Peter
>
> It probably affects more then Marvell PHYs.
>
> > The root
> > cause of this problem is a lack of an OF node for a PHY's built-in
> > temperature sensor. However I consider adding this OF node to be a bit
> > excessive solution. Am I wrong? Below you will find a one line patch which
> > fixes the problem.
>
> Your patch look sensible to me.
>
> > I've sent it to the releveant maintainers three weeks
> > ago without any feedback yet.
>
> Could you point me at the relevant mailing list archive?
Here it is: https://marc.info/?l=linux-pm&m=156691695616377&w=2
Regards,
Peter
>
> Thanks
> Andrew
^ permalink raw reply
* Re: [PATCH v2] net: dsa: sja1105: prevent leaking memory
From: David Miller @ 2019-09-19 8:10 UTC (permalink / raw)
To: olteanv
Cc: navid.emamdoost, andrew, emamd001, smccaman, kjlu, vivien.didelot,
f.fainelli, linux-kernel, netdev
In-Reply-To: <8d6f6c54-1758-7d98-c9b5-5c16b171c885@gmail.com>
From: Vladimir Oltean <olteanv@gmail.com>
Date: Wed, 18 Sep 2019 23:00:20 +0300
> Hi Navid,
>
> Thanks for the patch.
>
> On 9/18/19 9:04 PM, Navid Emamdoost wrote:
>> In sja1105_static_config_upload, in two cases memory is leaked: when
>> static_config_buf_prepare_for_upload fails and when sja1105_inhibit_tx
>> fails. In both cases config_buf should be released.
>> Fixes: 8aa9ebccae876 (avoid leaking config_buf)
>> Fixes: 1a4c69406cc1c (avoid leaking config_buf)
>>
>
> You're not supposed to add a short description of the patch here, but
> rather the commit message of the patch you're fixing.
> Add this to your ~/.gitconfig:
>
> [pretty]
> fixes = Fixes: %h (\"%s\")
>
> And then run:
> git show --pretty=fixes 8aa9ebccae87621d997707e4f25e53fddd7e30e4
>
> Fixes: 8aa9ebccae87 ("net: dsa: Introduce driver for NXP SJA1105
> 5-port L2 switch")
>
> git show --pretty=fixes 1a4c69406cc1c3c42bb7391c8eb544e93fe9b320
>
> Fixes: 1a4c69406cc1 ("net: dsa: sja1105: Prevent PHY jabbering during
> switch reset")
However the Fixes: line should not be broken up like this with newlines.
^ permalink raw reply
* Re: [PATCH v3] bonding: force enable lacp port after link state recovery for 802.3ad
From: Jay Vosburgh @ 2019-09-19 8:11 UTC (permalink / raw)
To: zhangsha (A)
Cc: vfalico@gmail.com, andy@greyhouse.net, davem@davemloft.net,
netdev@vger.kernel.org, linux-kernel@vger.kernel.org, yuehaibing,
hunongda, Chenzhendong (alex)
In-Reply-To: <e333c8d2f3624a898a378eb1073f5f29@huawei.com>
zhangsha (A) <zhangsha.zhang@huawei.com> wrote:
>> -----Original Message-----
>> From: zhangsha (A)
>> Sent: 2019年9月18日 21:06
>> To: jay.vosburgh@canonical.com; vfalico@gmail.com; andy@greyhouse.net;
>> davem@davemloft.net; netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
>> yuehaibing <yuehaibing@huawei.com>; hunongda <hunongda@huawei.com>;
>> Chenzhendong (alex) <alex.chen@huawei.com>; zhangsha (A)
>> <zhangsha.zhang@huawei.com>
>> Subject: [PATCH v3] bonding: force enable lacp port after link state recovery for
>> 802.3ad
>>
>> From: Sha Zhang <zhangsha.zhang@huawei.com>
>>
>> After the commit 334031219a84 ("bonding/802.3ad: fix slave link initialization
>> transition states") merged, the slave's link status will be changed to
>> BOND_LINK_FAIL from BOND_LINK_DOWN in the following scenario:
>> - Driver reports loss of carrier and
>> bonding driver receives NETDEV_DOWN notifier
>> - slave's duplex and speed is zerod and
>> its port->is_enabled is cleard to 'false';
>> - Driver reports link recovery and
>> bonding driver receives NETDEV_UP notifier;
>> - If speed/duplex getting failed here, the link status
>> will be changed to BOND_LINK_FAIL;
>> - The MII monotor later recover the slave's speed/duplex
>> and set link status to BOND_LINK_UP, but remains
>> the 'port->is_enabled' to 'false'.
>>
>> In this scenario, the lacp port will not be enabled even its speed and duplex are
>> valid. The bond will not send LACPDU's, and its state is 'AD_STATE_DEFAULTED'
>> forever. The simplest fix I think is to call bond_3ad_handle_link_change() in
>> bond_miimon_commit, this function can enable lacp after port slave speed
>> check.
>> As enabled, the lacp port can run its state machine normally after link recovery.
>>
>> Signed-off-by: Sha Zhang <zhangsha.zhang@huawei.com>
>> ---
>> drivers/net/bonding/bond_main.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/bonding/bond_main.c
>> b/drivers/net/bonding/bond_main.c index 931d9d9..76324a5 100644
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -2206,7 +2206,8 @@ static void bond_miimon_commit(struct bonding
>> *bond)
>> */
>> if (BOND_MODE(bond) == BOND_MODE_8023AD &&
>> slave->link == BOND_LINK_UP)
>> -
>> bond_3ad_adapter_speed_duplex_changed(slave);
>> + bond_3ad_handle_link_change(slave,
>> + BOND_LINK_UP);
>> continue;
>>
>> case BOND_LINK_UP:
>
>Hi, David,
>I have replied your email for a while, I guess you may miss my email, so I resend it.
>The following link address is the last email, please review the new one again, thank you.
>https://patchwork.ozlabs.org/patch/1151915/
>
>Last time, you doubted this is a driver specific problem,
>I prefer to believe it's not because I find the commit 4d2c0cda,
>its log says " Some NIC drivers don't have correct speed/duplex
>settings at the time they send NETDEV_UP notification ...".
>
>Anyway, I think the lacp status should be fixed correctly,
>since link-monitoring (miimon) set SPEED/DUPLEX right here.
I suspect this is going to be related to the concurrent
discussion with Aleksei, and would like to see the instrumentation
results from his tests before adding another change to attempt to
resolve this.
Also, what device are you using for your testing, and are you
able to run the instrumentation patch that I provided to Aleksei and
provide its results?
-J
---
-Jay Vosburgh, jay.vosburgh@canonical.com
^ permalink raw reply
* Re: [PATCH v3] net: dsa: sja1105: prevent leaking memory
From: David Miller @ 2019-09-19 8:13 UTC (permalink / raw)
To: navid.emamdoost
Cc: olteanv, emamd001, smccaman, kjlu, andrew, vivien.didelot,
f.fainelli, linux-kernel, netdev
In-Reply-To: <20190918203407.23826-1-navid.emamdoost@gmail.com>
From: Navid Emamdoost <navid.emamdoost@gmail.com>
Date: Wed, 18 Sep 2019 15:34:06 -0500
> Fixes: 8aa9ebccae87 ("net: dsa: Introduce driver for NXP SJA1105 5-port
> L2 switch")
>
> Fixes: 1a4c69406cc1 ("net: dsa: sja1105: Prevent PHY jabbering during
> switch reset")
Please:
1) Do not break Fixes: tags into multiples lines, that way the string
is easily greppable.
2) Do not separate the Fixes: from other tags with newlines. It is
just another tag like Signed-off-by: and Acked-by:
Thanks.
^ permalink raw reply
* Re: dsa traffic priorization
From: Vladimir Oltean @ 2019-09-19 8:18 UTC (permalink / raw)
To: Sascha Hauer
Cc: netdev, Andrew Lunn, Vivien Didelot, Florian Fainelli, kernel
In-Reply-To: <20190919080051.mr3cszpyypwqjwu4@pengutronix.de>
On Thu, 19 Sep 2019 at 11:00, Sascha Hauer <s.hauer@pengutronix.de> wrote:
>
> Hi Vladimir,
>
> On Wed, Sep 18, 2019 at 05:36:08PM +0300, Vladimir Oltean wrote:
> > Hi Sascha,
> >
> > On Wed, 18 Sep 2019 at 17:03, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > >
> > > Hi All,
> > >
> > > We have a customer using a Marvell 88e6240 switch with Ethercat on one port and
> > > regular network traffic on another port. The customer wants to configure two things
> > > on the switch: First Ethercat traffic shall be priorized over other network traffic
> > > (effectively prioritizing traffic based on port). Second the ethernet controller
> > > in the CPU is not able to handle full bandwidth traffic, so the traffic to the CPU
> > > port shall be rate limited.
> > >
> >
> > You probably already know this, but egress shaping will not drop
> > frames, just let them accumulate in the egress queue until something
> > else happens (e.g. queue occupancy threshold triggers pause frames, or
> > tail dropping is enabled, etc). Is this what you want?
>
> If I understand correctly then the switch has multiple output queues per
> port. The Ethercat traffic will go to a higher priority queue and on
> congestion on other queues, frames designated for that queue will be
> dropped. I just talked to our customer and he verified that their
> Ethercat traffic still goes through even when the ports with the general
> traffic are jammed with packets. So yes, I think this is what I want.
>
Yes, but I mean the egress shaper is per port, so when it goes out of
credits it goes out of credits, right? Meaning that even if EtherCAT
has higher strict priority, it will still experience latency caused by
the best-effort traffic consuming the port's global token bucket
credits. Sure, it may not be so bad as to actually cause tail drop,
but did you measure this?
> > It sounds a bit
> > strange to me to configure egress shaping on the CPU port of a DSA
> > switch. That literally means you are buffering frames inside the
> > system. What about ingress policing?
>
> The bottleneck here is in the CPU interface. The SoC simply can't handle
> all frames coming into a fully occupied link, so we indeed have to limit
> the number of packets coming into the SoC which speaks for egress rate
> limiting. We could of course limit the ingress packets on the other
> ports, but that would mean we have to rate limit each port to the total
> desired rate divided by the number of ports to be safe, not very
> optimal.
>
Not very optimal, but may offer better guarantees for the
high-priority traffic, and there is already a model for doing that,
unlike for egress shaping on the CPU port.
What about a software tc-police action on the DSA net device's ingress
qdisc? Is that still too high-pressure for the CPU?
Is there any flow steering rule on the CPU for processing EtherCAT
with higher priority (or affining it to a separate core)? I'm trying
to understand where the bottleneck really is.
> Sascha
>
> --
> Pengutronix e.K. | |
> Industrial Linux Solutions | http://www.pengutronix.de/ |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Thanks,
-Vladimir
^ permalink raw reply
* Re: [PATCH v2] net: dsa: sja1105: prevent leaking memory
From: Vladimir Oltean @ 2019-09-19 8:20 UTC (permalink / raw)
To: David Miller
Cc: Navid Emamdoost, Andrew Lunn, Navid Emamdoost, Stephen McCamant,
kjlu, Vivien Didelot, Florian Fainelli, lkml, netdev
In-Reply-To: <20190919.101059.1330167782179062709.davem@davemloft.net>
On Thu, 19 Sep 2019 at 11:11, David Miller <davem@davemloft.net> wrote:
>
> From: Vladimir Oltean <olteanv@gmail.com>
> Date: Wed, 18 Sep 2019 23:00:20 +0300
>
> > Hi Navid,
> >
> > Thanks for the patch.
> >
> > On 9/18/19 9:04 PM, Navid Emamdoost wrote:
> >> In sja1105_static_config_upload, in two cases memory is leaked: when
> >> static_config_buf_prepare_for_upload fails and when sja1105_inhibit_tx
> >> fails. In both cases config_buf should be released.
> >> Fixes: 8aa9ebccae876 (avoid leaking config_buf)
> >> Fixes: 1a4c69406cc1c (avoid leaking config_buf)
> >>
> >
> > You're not supposed to add a short description of the patch here, but
> > rather the commit message of the patch you're fixing.
> > Add this to your ~/.gitconfig:
> >
> > [pretty]
> > fixes = Fixes: %h (\"%s\")
> >
> > And then run:
> > git show --pretty=fixes 8aa9ebccae87621d997707e4f25e53fddd7e30e4
> >
> > Fixes: 8aa9ebccae87 ("net: dsa: Introduce driver for NXP SJA1105
> > 5-port L2 switch")
> >
> > git show --pretty=fixes 1a4c69406cc1c3c42bb7391c8eb544e93fe9b320
> >
> > Fixes: 1a4c69406cc1 ("net: dsa: sja1105: Prevent PHY jabbering during
> > switch reset")
>
> However the Fixes: line should not be broken up like this with newlines.
Sorry, my mail client did that automatically.
-Vladimir
^ permalink raw reply
* Re: dsa traffic priorization
From: Uwe Kleine-König @ 2019-09-19 8:36 UTC (permalink / raw)
To: Sascha Hauer
Cc: Vladimir Oltean, netdev, Vivien Didelot, Florian Fainelli, kernel,
Andrew Lunn
In-Reply-To: <20190919080051.mr3cszpyypwqjwu4@pengutronix.de>
On Thu, Sep 19, 2019 at 10:00:51AM +0200, Sascha Hauer wrote:
> Hi Vladimir,
>
> On Wed, Sep 18, 2019 at 05:36:08PM +0300, Vladimir Oltean wrote:
> > Hi Sascha,
> >
> > On Wed, 18 Sep 2019 at 17:03, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > >
> > > Hi All,
> > >
> > > We have a customer using a Marvell 88e6240 switch with Ethercat on one port and
> > > regular network traffic on another port. The customer wants to configure two things
> > > on the switch: First Ethercat traffic shall be priorized over other network traffic
> > > (effectively prioritizing traffic based on port). Second the ethernet controller
> > > in the CPU is not able to handle full bandwidth traffic, so the traffic to the CPU
> > > port shall be rate limited.
> > >
> >
> > You probably already know this, but egress shaping will not drop
> > frames, just let them accumulate in the egress queue until something
> > else happens (e.g. queue occupancy threshold triggers pause frames, or
> > tail dropping is enabled, etc). Is this what you want?
>
> If I understand correctly then the switch has multiple output queues per
> port. The Ethercat traffic will go to a higher priority queue and on
> congestion on other queues, frames designated for that queue will be
> dropped. I just talked to our customer and he verified that their
> Ethercat traffic still goes through even when the ports with the general
> traffic are jammed with packets. So yes, I think this is what I want.
Moreover egressing the cpu port has the advantage that network
participants on other ports that might be able to process packet quicker
are not limited.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply
* [PATCH net v3] net/sched: cls_api: Fix nooffloaddevcnt counter when indr block call success
From: wenxu @ 2019-09-19 8:37 UTC (permalink / raw)
To: davem; +Cc: netdev
From: wenxu <wenxu@ucloud.cn>
A vxlan or gretap device offload through indr block methord. If the device
successfully bind with a real hw through indr block call, It also add
nooffloadcnt counter. This counter will lead the rule add failed in
fl_hw_replace_filter-->tc_setup_cb_call with skip_sw flags.
In the tc_setup_cb_call will check the nooffloaddevcnt and skip_sw flags
as following:
if (block->nooffloaddevcnt && err_stop)
return -EOPNOTSUPP;
So with this patch, if the indr block call success, it will not modify
the nooffloaddevcnt counter.
Fixes: 7f76fa36754b ("net: sched: register callbacks for indirect tc block binds")
Signed-off-by: wenxu <wenxu@ucloud.cn>
---
v3: rebase to the net
net/sched/cls_api.c | 30 +++++++++++++++++-------------
1 file changed, 17 insertions(+), 13 deletions(-)
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 32577c2..c980127 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -607,11 +607,11 @@ static void tc_indr_block_get_and_ing_cmd(struct net_device *dev,
tc_indr_block_ing_cmd(dev, block, cb, cb_priv, command);
}
-static void tc_indr_block_call(struct tcf_block *block,
- struct net_device *dev,
- struct tcf_block_ext_info *ei,
- enum flow_block_command command,
- struct netlink_ext_ack *extack)
+static int tc_indr_block_call(struct tcf_block *block,
+ struct net_device *dev,
+ struct tcf_block_ext_info *ei,
+ enum flow_block_command command,
+ struct netlink_ext_ack *extack)
{
struct flow_block_offload bo = {
.command = command,
@@ -621,10 +621,15 @@ static void tc_indr_block_call(struct tcf_block *block,
.block_shared = tcf_block_shared(block),
.extack = extack,
};
+
INIT_LIST_HEAD(&bo.cb_list);
flow_indr_block_call(dev, &bo, command);
- tcf_block_setup(block, &bo);
+
+ if (list_empty(&bo.cb_list))
+ return -EOPNOTSUPP;
+
+ return tcf_block_setup(block, &bo);
}
static bool tcf_block_offload_in_use(struct tcf_block *block)
@@ -681,8 +686,6 @@ static int tcf_block_offload_bind(struct tcf_block *block, struct Qdisc *q,
goto no_offload_dev_inc;
if (err)
goto err_unlock;
-
- tc_indr_block_call(block, dev, ei, FLOW_BLOCK_BIND, extack);
up_write(&block->cb_lock);
return 0;
@@ -691,9 +694,10 @@ static int tcf_block_offload_bind(struct tcf_block *block, struct Qdisc *q,
err = -EOPNOTSUPP;
goto err_unlock;
}
+ err = tc_indr_block_call(block, dev, ei, FLOW_BLOCK_BIND, extack);
+ if (err)
+ block->nooffloaddevcnt++;
err = 0;
- block->nooffloaddevcnt++;
- tc_indr_block_call(block, dev, ei, FLOW_BLOCK_BIND, extack);
err_unlock:
up_write(&block->cb_lock);
return err;
@@ -706,8 +710,6 @@ static void tcf_block_offload_unbind(struct tcf_block *block, struct Qdisc *q,
int err;
down_write(&block->cb_lock);
- tc_indr_block_call(block, dev, ei, FLOW_BLOCK_UNBIND, NULL);
-
if (!dev->netdev_ops->ndo_setup_tc)
goto no_offload_dev_dec;
err = tcf_block_offload_cmd(block, dev, ei, FLOW_BLOCK_UNBIND, NULL);
@@ -717,7 +719,9 @@ static void tcf_block_offload_unbind(struct tcf_block *block, struct Qdisc *q,
return;
no_offload_dev_dec:
- WARN_ON(block->nooffloaddevcnt-- == 0);
+ err = tc_indr_block_call(block, dev, ei, FLOW_BLOCK_UNBIND, NULL);
+ if (err)
+ WARN_ON(block->nooffloaddevcnt-- == 0);
up_write(&block->cb_lock);
}
--
1.8.3.1
^ permalink raw reply related
* [PATCH] usbnet: sanity checking of packet sizes and device mtu
From: Oliver Neukum @ 2019-09-19 8:23 UTC (permalink / raw)
To: davem, netdev; +Cc: Oliver Neukum
After a reset packet sizes and device mtu can change and need
to be reevaluated to calculate queue sizes.
Malicious devices can set this to zero and we divide by it.
Introduce sanity checking.
Reported-and-tested-by: syzbot+6102c120be558c885f04@syzkaller.appspotmail.com
Signed-off-by: Oliver Neukum <oneukum@suse.com>
---
drivers/net/usb/usbnet.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index 58952a79b05f..e44849499b89 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -339,6 +339,8 @@ void usbnet_update_max_qlen(struct usbnet *dev)
{
enum usb_device_speed speed = dev->udev->speed;
+ if (!dev->rx_urb_size || !dev->hard_mtu)
+ goto insanity;
switch (speed) {
case USB_SPEED_HIGH:
dev->rx_qlen = MAX_QUEUE_MEMORY / dev->rx_urb_size;
@@ -355,6 +357,7 @@ void usbnet_update_max_qlen(struct usbnet *dev)
dev->tx_qlen = 5 * MAX_QUEUE_MEMORY / dev->hard_mtu;
break;
default:
+insanity:
dev->rx_qlen = dev->tx_qlen = 4;
}
}
--
2.16.4
^ permalink raw reply related
* Re: [PATCH net v3] net/sched: cls_api: Fix nooffloaddevcnt counter when indr block call success
From: wenxu @ 2019-09-19 8:40 UTC (permalink / raw)
To: jiri; +Cc: netdev
In-Reply-To: <1568882232-12847-1-git-send-email-wenxu@ucloud.cn>
sorry forget cc to jiri.
On 9/19/2019 4:37 PM, wenxu@ucloud.cn wrote:
> From: wenxu <wenxu@ucloud.cn>
>
> A vxlan or gretap device offload through indr block methord. If the device
> successfully bind with a real hw through indr block call, It also add
> nooffloadcnt counter. This counter will lead the rule add failed in
> fl_hw_replace_filter-->tc_setup_cb_call with skip_sw flags.
>
> In the tc_setup_cb_call will check the nooffloaddevcnt and skip_sw flags
> as following:
> if (block->nooffloaddevcnt && err_stop)
> return -EOPNOTSUPP;
>
> So with this patch, if the indr block call success, it will not modify
> the nooffloaddevcnt counter.
>
> Fixes: 7f76fa36754b ("net: sched: register callbacks for indirect tc block binds")
> Signed-off-by: wenxu <wenxu@ucloud.cn>
> ---
> v3: rebase to the net
>
> net/sched/cls_api.c | 30 +++++++++++++++++-------------
> 1 file changed, 17 insertions(+), 13 deletions(-)
>
> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
> index 32577c2..c980127 100644
> --- a/net/sched/cls_api.c
> +++ b/net/sched/cls_api.c
> @@ -607,11 +607,11 @@ static void tc_indr_block_get_and_ing_cmd(struct net_device *dev,
> tc_indr_block_ing_cmd(dev, block, cb, cb_priv, command);
> }
>
> -static void tc_indr_block_call(struct tcf_block *block,
> - struct net_device *dev,
> - struct tcf_block_ext_info *ei,
> - enum flow_block_command command,
> - struct netlink_ext_ack *extack)
> +static int tc_indr_block_call(struct tcf_block *block,
> + struct net_device *dev,
> + struct tcf_block_ext_info *ei,
> + enum flow_block_command command,
> + struct netlink_ext_ack *extack)
> {
> struct flow_block_offload bo = {
> .command = command,
> @@ -621,10 +621,15 @@ static void tc_indr_block_call(struct tcf_block *block,
> .block_shared = tcf_block_shared(block),
> .extack = extack,
> };
> +
> INIT_LIST_HEAD(&bo.cb_list);
>
> flow_indr_block_call(dev, &bo, command);
> - tcf_block_setup(block, &bo);
> +
> + if (list_empty(&bo.cb_list))
> + return -EOPNOTSUPP;
> +
> + return tcf_block_setup(block, &bo);
> }
>
> static bool tcf_block_offload_in_use(struct tcf_block *block)
> @@ -681,8 +686,6 @@ static int tcf_block_offload_bind(struct tcf_block *block, struct Qdisc *q,
> goto no_offload_dev_inc;
> if (err)
> goto err_unlock;
> -
> - tc_indr_block_call(block, dev, ei, FLOW_BLOCK_BIND, extack);
> up_write(&block->cb_lock);
> return 0;
>
> @@ -691,9 +694,10 @@ static int tcf_block_offload_bind(struct tcf_block *block, struct Qdisc *q,
> err = -EOPNOTSUPP;
> goto err_unlock;
> }
> + err = tc_indr_block_call(block, dev, ei, FLOW_BLOCK_BIND, extack);
> + if (err)
> + block->nooffloaddevcnt++;
> err = 0;
> - block->nooffloaddevcnt++;
> - tc_indr_block_call(block, dev, ei, FLOW_BLOCK_BIND, extack);
> err_unlock:
> up_write(&block->cb_lock);
> return err;
> @@ -706,8 +710,6 @@ static void tcf_block_offload_unbind(struct tcf_block *block, struct Qdisc *q,
> int err;
>
> down_write(&block->cb_lock);
> - tc_indr_block_call(block, dev, ei, FLOW_BLOCK_UNBIND, NULL);
> -
> if (!dev->netdev_ops->ndo_setup_tc)
> goto no_offload_dev_dec;
> err = tcf_block_offload_cmd(block, dev, ei, FLOW_BLOCK_UNBIND, NULL);
> @@ -717,7 +719,9 @@ static void tcf_block_offload_unbind(struct tcf_block *block, struct Qdisc *q,
> return;
>
> no_offload_dev_dec:
> - WARN_ON(block->nooffloaddevcnt-- == 0);
> + err = tc_indr_block_call(block, dev, ei, FLOW_BLOCK_UNBIND, NULL);
> + if (err)
> + WARN_ON(block->nooffloaddevcnt-- == 0);
> up_write(&block->cb_lock);
> }
>
^ permalink raw reply
* Re: dsa traffic priorization
From: Sascha Hauer @ 2019-09-19 8:41 UTC (permalink / raw)
To: Vladimir Oltean
Cc: netdev, Vivien Didelot, Florian Fainelli, kernel, Andrew Lunn
In-Reply-To: <CA+h21hqibrGksG=k7TkjiToDFZ-putaonO+tCb_=nxs1ig0djA@mail.gmail.com>
On Thu, Sep 19, 2019 at 11:18:24AM +0300, Vladimir Oltean wrote:
> On Thu, 19 Sep 2019 at 11:00, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> >
> > Hi Vladimir,
> >
> > On Wed, Sep 18, 2019 at 05:36:08PM +0300, Vladimir Oltean wrote:
> > > Hi Sascha,
> > >
> > > On Wed, 18 Sep 2019 at 17:03, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > > >
> > > > Hi All,
> > > >
> > > > We have a customer using a Marvell 88e6240 switch with Ethercat on one port and
> > > > regular network traffic on another port. The customer wants to configure two things
> > > > on the switch: First Ethercat traffic shall be priorized over other network traffic
> > > > (effectively prioritizing traffic based on port). Second the ethernet controller
> > > > in the CPU is not able to handle full bandwidth traffic, so the traffic to the CPU
> > > > port shall be rate limited.
> > > >
> > >
> > > You probably already know this, but egress shaping will not drop
> > > frames, just let them accumulate in the egress queue until something
> > > else happens (e.g. queue occupancy threshold triggers pause frames, or
> > > tail dropping is enabled, etc). Is this what you want?
> >
> > If I understand correctly then the switch has multiple output queues per
> > port. The Ethercat traffic will go to a higher priority queue and on
> > congestion on other queues, frames designated for that queue will be
> > dropped. I just talked to our customer and he verified that their
> > Ethercat traffic still goes through even when the ports with the general
> > traffic are jammed with packets. So yes, I think this is what I want.
> >
>
> Yes, but I mean the egress shaper is per port, so when it goes out of
> credits it goes out of credits, right? Meaning that even if EtherCAT
> has higher strict priority, it will still experience latency caused by
> the best-effort traffic consuming the port's global token bucket
> credits. Sure, it may not be so bad as to actually cause tail drop,
> but did you measure this?
I don't think this has been measured. I understand what you mean and
there might be latencies introduced, but it seems the latencies are
acceptable.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply
* Re: dsa traffic priorization
From: Vladimir Oltean @ 2019-09-19 8:42 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Sascha Hauer, netdev, Vivien Didelot, Florian Fainelli, kernel,
Andrew Lunn
In-Reply-To: <20190919083638.wgxrrgtqxwpjcsu3@pengutronix.de>
Hi Uwe,
On Thu, 19 Sep 2019 at 11:36, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> On Thu, Sep 19, 2019 at 10:00:51AM +0200, Sascha Hauer wrote:
> > Hi Vladimir,
> >
> > On Wed, Sep 18, 2019 at 05:36:08PM +0300, Vladimir Oltean wrote:
> > > Hi Sascha,
> > >
> > > On Wed, 18 Sep 2019 at 17:03, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > > >
> > > > Hi All,
> > > >
> > > > We have a customer using a Marvell 88e6240 switch with Ethercat on one port and
> > > > regular network traffic on another port. The customer wants to configure two things
> > > > on the switch: First Ethercat traffic shall be priorized over other network traffic
> > > > (effectively prioritizing traffic based on port). Second the ethernet controller
> > > > in the CPU is not able to handle full bandwidth traffic, so the traffic to the CPU
> > > > port shall be rate limited.
> > > >
> > >
> > > You probably already know this, but egress shaping will not drop
> > > frames, just let them accumulate in the egress queue until something
> > > else happens (e.g. queue occupancy threshold triggers pause frames, or
> > > tail dropping is enabled, etc). Is this what you want?
> >
> > If I understand correctly then the switch has multiple output queues per
> > port. The Ethercat traffic will go to a higher priority queue and on
> > congestion on other queues, frames designated for that queue will be
> > dropped. I just talked to our customer and he verified that their
> > Ethercat traffic still goes through even when the ports with the general
> > traffic are jammed with packets. So yes, I think this is what I want.
>
> Moreover egressing the cpu port has the advantage that network
> participants on other ports that might be able to process packet quicker
> are not limited.
>
Yes, that is true if you apply port-based policers (matchall), but not
necessarily so if your switch is able to do ingress classification (a
common key for switches being the {DMAC, VLAN} pair). Again, there is
a model for configuring even that, if the hardware is capable. Just
not for egress shaping on the CPU port.
> Best regards
> Uwe
>
> --
> Pengutronix e.K. | Uwe Kleine-König |
> Industrial Linux Solutions | http://www.pengutronix.de/ |
^ 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